-
Notifications
You must be signed in to change notification settings - Fork 40
simln-lib/feat: Surface send_to_route for SimGraph #268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
simln-lib/feat: Surface send_to_route for SimGraph #268
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worried about the layering of adding this functionality to SimGraph
, would like to take another look at the high level design here.
Also needs a rebase!
simln-lib/src/sim_node.rs
Outdated
/// Uses the provided `hops` and `amount_msat` to build an LDK route. | ||
/// This is a helper function to build a route easily. | ||
/// The route built using this function can be passed into `SimGraph::send_to_route`. | ||
pub fn build_payment_route( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not sure that we need this helper for an advanced user?
Perhaps we an just point to build_route_from_hops
on the docs for send_to_route
?
simln-lib/src/sim_node.rs
Outdated
}) | ||
} | ||
|
||
pub async fn send_to_route( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about adding this in SimGraph
- feels like a layering violation. I can see how the discussion on the issue was a bit ambiguous - should have pointed out that it should still go on the node even though we don't need to implement it on the other types.
I think that this should live on SimNode
so that we can re-use the infrastructure that's already there (eg, we wouldn't need another track payment function, we already have it).
I think it's reasonable to expect the end user to look up the dispatching node that they want and then call SimNode.send_to_route
.
bd09d2b
to
b1db235
Compare
I didn't add any test cases this time around since |
e7684ad
to
f7f511a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just one API q from me + a test ask.
simln-lib/src/sim_node.rs
Outdated
let preimage = PaymentPreimage(rand::random()); | ||
let payment_hash = preimage.into(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I wonder whether there's any use for people to pass in their own payment hash?
WDYT, I'm on the fence...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be helpful if the user wants to simulate payments in an application that relies on a custom payment flow - for example the user could have a payment flow that requires generating the preimage and hash much earlier in their process, perhaps for coordination with other parts of their application, well before the send_to_route
method is called.
I think allowing users to pass in their own payment hash would give them the flexibility to accurately model such scenarios.
f7f511a
to
3f14a70
Compare
simln-lib/src/sim_node.rs
Outdated
@@ -526,6 +526,37 @@ impl<'a, T: SimNetwork> SimNode<'a, T> { | |||
scorer, | |||
} | |||
} | |||
|
|||
/// send_to_route dispatches a payment to a specified route. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: don't need to include send_to_route
in doc, just start with "Dispatches"
afaik this is rust convention, since the doc will always be displayed along with its function (unlike golang, where the convention is to alway start with the name).
Ignore all the places where we do this wrong in the codebase :')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore all the places where we do this wrong in the codebase :')
: )
simln-lib/src/sim_node.rs
Outdated
/// provided is triggered. This call will fail if the hash provided was not obtained by calling send_payment first. | ||
/// Payment hashes passed into send_to_route can also be tracked using track_payment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "This call will fail if the hash provided was not obtained by calling send_payment or track_payment* first?
Update: *track_payment -> send_to_route, my bad!
simln-lib/src/sim_node.rs
Outdated
.map_err(|e| { | ||
SimulationError::SimulatedNetworkError(format!( | ||
"An Error occurred while building route - {}", | ||
e.err | ||
)) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to map the error in a test - just unwrap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! go ahead with squash and I'll hit the button
c3b6cfc
to
dce1f3f
Compare
Description
This PR makes it possible for a user of simln-lib (in sim-node mode) to send payments to a predetermined route. This addresses the need for end-users to send custom payments, crucial for scenarios like channel jamming attacks or pathfinding experiments, which the current activity paradigm doesn't support.
Changes
send_to_route
method was added toSimGraph
.track_payment_to_route
method was added toSimGraph
.build_payment_route
helper function was added.This PR closes #257