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

Expand PaymentClaimable to include all inbound channel IDs for a payment #3655

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shaavan
Copy link
Member

@shaavan shaavan commented Mar 8, 2025

Resolves #2274

This PR expands PaymentClaimable to include all inbound channel_ids and user_channel_ids associated with a payment.
This update ensures a clearer representation of the channels used, especially for MPP payments.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Mar 8, 2025

👋 Thanks for assigning @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@shaavan
Copy link
Member Author

shaavan commented Mar 8, 2025

@benthecarman

Would also be nice if the same information is available on the PaymentClaimed event too

In the current version, I haven’t included channel_ids and user_channel_ids for PaymentClaimed, as they can be easily derived from the event itself:

let channel_ids = payment_claimed_event.htlcs.iter().map(|htlc| htlc.channel_id).collect();

That said, let me know if you’d still prefer to include channel_ids in PaymentClaimed, and I'd be happy to make that change. Thanks!

@benthecarman
Copy link
Contributor

Thanks, @shaavan , that is good enough

@ldk-reviews-bot
Copy link

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

@valentinewallace
Copy link
Contributor

nit: limit commit subject line in 2nd commit to ~50 characters

@shaavan
Copy link
Member Author

shaavan commented Mar 12, 2025

Updated from pr3655.01 to pr3655.02 (diff):
Addressed @valentinewallace comments

  1. Updated the fields to be Vec<ChannelId>, and Vec<u128>
  2. Updated test utilities to check for all the channels id in events list, for improved test completeness.

Comment on lines 1553 to 1557
// legacy field
// (3, via_channel_id, option),
(4, amount_msat, required),
(5, via_user_channel_id, option),
// legacy field
// (5, via_user_channel_id, option),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should still write these fields for the sake of users who downgrade (can always write the first item in the vec). Also would be nice to comment in what version the fields became legacy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! pr3655.03
I’ve updated the writing so that we maintain downgrade support.
Haven’t added a comment yet since I’m not sure what the next version number will be 😅 (would it be 0.1.3?)

@shaavan
Copy link
Member Author

shaavan commented Mar 13, 2025

Updated from pr3655.02 to pr3655.03 (diff):
Addressed @valentinewallace comments

Changes:

  1. Expanded Documentation
  2. Properly write the legacy fields to maintain downgrade support.

@valentinewallace
Copy link
Contributor

Looks good, feel free to squash! Thanks!

@shaavan
Copy link
Member Author

shaavan commented Mar 15, 2025

Updated from pr3655.03 to pr3655.04 (diff):
Addressed @valentinewallace comments

Changes:

  1. Squash commits.

Copy link

codecov bot commented Mar 15, 2025

Codecov Report

Attention: Patch coverage is 96.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.37%. Comparing base (5bc9ffa) to head (591c5c8).
Report is 180 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/events/mod.rs 90.47% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3655      +/-   ##
==========================================
+ Coverage   89.21%   91.37%   +2.16%     
==========================================
  Files         155      156       +1     
  Lines      118966   141344   +22378     
  Branches   118966   141344   +22378     
==========================================
+ Hits       106133   129159   +23026     
+ Misses      10253     9799     -454     
+ Partials     2580     2386     -194     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member Author

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Looks good, feel free to squash! Thanks!

Awesome — thanks a lot for the review, Val! 🚀

@shaavan
Copy link
Member Author

shaavan commented Mar 25, 2025

Updated from pr3655.04 to pr3655.05 (diff):
Addressed @valentinewallace comments

Changes:

  • Added comment documenting when, and why via_channel_id_legacy, and via_user_channel_id_legacy were deprecated.

shaavan added 2 commits April 4, 2025 18:49
These two utilities will be used in following commit to get all the
inbound channel_ids, and user_channel_ids associated with all the
parts of a payment.
Previously, `channel_id` in `PaymentClaimable` only listed a single
inbound channel, which was misleading for MPP payments arriving via
multiple channels.

To better represent MPP scenarios, this update introduces:
- `via_channel_ids`: A list of all inbound channels used in the payment.
- `via_user_channel_ids`: The corresponding user-defined channel IDs for
  each inbound channel.

This change ensures a more accurate representation of multi-path payments
while maintaining backward compatibility.
@shaavan
Copy link
Member Author

shaavan commented Apr 4, 2025

Updated from pr3655.05 to pr3655.06 (diff):
Addressed @valentinewallace comments

Changes:

  1. Updated documentation to correctly reflect that the vector will be incomplete for HTLC created using versions prior to LDK 0.1.0 or prior.

@valentinewallace valentinewallace self-requested a review April 4, 2025 17:11
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PaymentClaimable lists inbound channel id
4 participants