Skip to content
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

Implement conversion from LDK route to Barq Route Output #31

Merged
merged 1 commit into from
Aug 4, 2024

Conversation

raehat
Copy link
Collaborator

@raehat raehat commented Jul 30, 2024

@raehat raehat added the enhancement New feature or request label Jul 30, 2024
@raehat raehat requested a review from vincenzopalazzo July 30, 2024 19:34
@raehat raehat self-assigned this Jul 30, 2024
@raehat raehat force-pushed the ldk-route-to-barq-output branch 2 times, most recently from 3257340 to 914019b Compare July 30, 2024 20:04

unimplemented!("convert_route_to_output not implemented yet.")
// TODO: Implement the logic to convert the LDK Route to RouteOutput
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not already implemented with this code?

hop.pubkey.to_string(),
hop.short_channel_id.to_string(),
hop.cltv_expiry_delta,
hop.fee_msat,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in core lightning the amount_msat is the fee_msat?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact, this is wrong, from core lightning docs amount_msat (msat): The amount expected by the node at the end of this hop.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pay attention to this because we are playing with people money

@raehat raehat force-pushed the ldk-route-to-barq-output branch from 914019b to 3ae8daf Compare August 4, 2024 12:36
@raehat
Copy link
Collaborator Author

raehat commented Aug 4, 2024

This branch is not well-tested. Current test cases only cover the direct strategy, not the probabilistic strategy. Once test cases are written for the probabilistic strategy and they pass, only then will this PR be ready to merge.

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT, let move forward with this and be able to test it

@vincenzopalazzo vincenzopalazzo merged commit 409b4fe into main Aug 4, 2024
2 checks passed
@vincenzopalazzo vincenzopalazzo deleted the ldk-route-to-barq-output branch August 4, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants