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

Include counterparty node ids in PaymentForwarded #3458

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

shaavan
Copy link
Contributor

@shaavan shaavan commented Dec 11, 2024

resolves #3455

This commit adds counterparty node IDs to PaymentForwarded to address the potential ambiguity of using ChannelIds alone, especially in cases like v1 0conf opens where ChannelIds may not be unique. Including the counterparty node IDs provides better clarity and makes the information more useful.

lightning/src/events/mod.rs Outdated Show resolved Hide resolved
lightning/src/events/mod.rs Outdated Show resolved Hide resolved
lightning/src/events/mod.rs Outdated Show resolved Hide resolved
lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.86%. Comparing base (abf72a5) to head (69f3136).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3458      +/-   ##
==========================================
+ Coverage   89.69%   89.86%   +0.16%     
==========================================
  Files         130      130              
  Lines      107422   108677    +1255     
  Branches   107422   108677    +1255     
==========================================
+ Hits        96354    97661    +1307     
+ Misses       8672     8640      -32     
+ Partials     2396     2376      -20     

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

@shaavan shaavan force-pushed the i3455 branch 2 times, most recently from 3ec8edf to 69f3136 Compare December 11, 2024 13:45
@shaavan
Copy link
Contributor Author

shaavan commented Dec 11, 2024

Updated from pr3458.01 to pr3458.02 (diff):
Addressed @tnull comments

Changes:

  1. General code cleanups.
  2. Reverted modifications to test utilities. Instead, added a new test check to verify that node IDs in events correctly match the actual node IDs.

tnull
tnull previously approved these changes Dec 11, 2024
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM

@TheBlueMatt TheBlueMatt added this to the 0.1 milestone Dec 11, 2024
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

LGTM, modulo one tiny nit.

lightning/src/events/mod.rs Outdated Show resolved Hide resolved
This commit adds counterparty node IDs to `PaymentForwarded`
to address the potential ambiguity of using `ChannelIds` alone,
especially in cases like v1 0conf opens where `ChannelIds`
may not be unique. Including the counterparty node IDs
provides better clarity and makes the information more useful.
@shaavan
Copy link
Contributor Author

shaavan commented Dec 11, 2024

Updated from pr3458.02 to pr3458.03 (diff):
Addressed @TheBlueMatt comments

Changes:

  1. Update documentation to be more accurate.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks!

@TheBlueMatt TheBlueMatt merged commit e0d81ca into lightningdevkit:main Dec 11, 2024
17 of 19 checks passed
@MaxFangX
Copy link
Contributor

Wow, so fast! Thanks everyone!

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.

PaymentForwarded should include counterparty nodes
4 participants