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

Add channel funding txo to Channel Event::ChannelClosed #2800

Merged

Conversation

optout21
Copy link
Contributor

@optout21 optout21 commented Dec 18, 2023

Reworked on top of #2809

Fixes #2772

  • extend ChannelClosed event with optional original channel funding txo; to help checking for the existence and confirmation status of the closing tx
  • internally extend ShutdownResult with funding txo
  • add checks for the new field of the event, in 2 test cases
  • also add checks for field user_channel_id (just for completeness)

@optout21 optout21 marked this pull request as ready for review December 18, 2023 12:31
Copy link
Contributor

@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.

The changes look good, and I think this PR can benefit by extending the test suite to cover this new change.

A few suggested nits I believe can further improve this PR.

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@optout21 optout21 marked this pull request as draft December 19, 2023 09:23
@optout21
Copy link
Contributor Author

optout21 commented Dec 19, 2023

Minor changes from review. Changed internal from_finish_shutdown to take a sinlge ChannelContext instead of capacity, user ID and txo fields.
TODO: Add checks for the contents of the new event field; squash.

@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (7bc2a14) 88.79% compared to head (efaba11) 89.19%.
Report is 43 commits behind head on main.

Files Patch % Lines
lightning/src/events/mod.rs 80.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2800      +/-   ##
==========================================
+ Coverage   88.79%   89.19%   +0.39%     
==========================================
  Files         114      114              
  Lines       95767    98266    +2499     
  Branches    95767    98266    +2499     
==========================================
+ Hits        85038    87649    +2611     
+ Misses       8289     8151     -138     
- Partials     2440     2466      +26     

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

@optout21 optout21 marked this pull request as ready for review December 19, 2023 13:56
Copy link
Contributor

@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.

LGTM!

  • The suggestions mentioned above are addressed.
  • The test suite is extended appropriately to address the current changes, and the tests work perfectly.

Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

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

ACK 11fe6ae. LGTM

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved
@TheBlueMatt
Copy link
Collaborator

Ugh, this basically LGTM, but in #2809 I rewrote the shutdown logic to move the event generation to clean up some of this logic. Is there any chance you want to rebase on that? Otherwise I'll merge this and have to partially revert it to rebase #2809 on top.

@optout21
Copy link
Contributor Author

optout21 commented Jan 9, 2024

Ugh, this basically LGTM, but in #2809 I rewrote the shutdown logic to move the event generation to clean up some of this logic. Is there any chance you want to rebase on that? Otherwise I'll merge this and have to partially revert it to rebase #2809 on top.

I'll redo this on top of #2809

@optout21 optout21 marked this pull request as draft January 9, 2024 11:01
@TheBlueMatt
Copy link
Collaborator

Thank you!

@optout21
Copy link
Contributor Author

optout21 commented Jan 9, 2024

This is now reworked on top of #2809 . Became simpler. Keeping as Draft until #2809 is merged.

@TheBlueMatt TheBlueMatt marked this pull request as ready for review January 10, 2024 22:38
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.

Nice, thanks.

@TheBlueMatt TheBlueMatt merged commit 4b70921 into lightningdevkit:main Jan 11, 2024
15 checks passed
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.

Add channel's funding txo to Channel Event::ChannelClosed
5 participants