Expose whether a channel is closing in ChannelDetails #2347

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.

Sign up for GitHub

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

Jump to bottom

Merged

TheBlueMatt merged 1 commit into lightningdevkit:main from henghonglee:issue-2304

Jul 7, 2023

Contributor

henghonglee commented Jun 9, 2023 •

edited

Loading

fixes #2304

Add is_channel_shutting_down to ChannelDetails

henghonglee marked this pull request as ready for review

June 9, 2023 05:11

codecov-commenter commented Jun 9, 2023 •

edited

Loading

Codecov Report

Patch coverage: 93.80% and no project coverage change.

Comparison is base (86fd9e7) 90.32% compared to head (4a25aab) 90.33%.

❗ Current head 4a25aab differs from pull request most recent head 47cb45e. Consider uploading reports for the commit 47cb45e to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #2347    +/-   ##
========================================
  Coverage   90.32%   90.33%            
========================================
  Files         106      106            
  Lines       54968    55072   +104     
  Branches    54968    55072   +104     
========================================
+ Hits        49651    49750    +99     
- Misses       5317     5322     +5

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`93.65% <ø> (+0.10%)`	⬆️
lightning/src/ln/channelmanager.rs	`86.17% <37.50%> (-0.10%)`	⬇️
lightning/src/ln/channel.rs	`89.39% <83.33%> (-0.03%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.21% <100.00%> (+0.37%)`	⬆️
lightning/src/ln/shutdown_tests.rs	`98.40% <100.00%> (+0.22%)`	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

henghonglee force-pushed the issue-2304 branch from e6bc69b to 3b5ec3b Compare

June 9, 2023 07:09

dunxen reviewed

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

Collaborator

TheBlueMatt commented Jun 12, 2023

Would be nice for this to be an enum that also indicates what state we're in in the shutdown process, basically htlcsremain (ie pre-closing_signed waiting on some HTLCs to resolve) or closing-fee-negotiation.

Contributor Author

henghonglee commented Jun 13, 2023

thanks for the comments! ill make the necessary changes and tests!

henghonglee closed this

henghonglee force-pushed the issue-2304 branch from 3b5ec3b to d327c23 Compare

June 26, 2023 07:33

Contributor

dunxen commented Jun 26, 2023

Oops, did you mean to close this, @henghonglee? :)

Contributor Author

henghonglee commented Jun 27, 2023

oops, no sorry im working on another implementation, accidently pushed empty commit. will push a new one today

henghonglee reopened this

henghonglee force-pushed the issue-2304 branch 4 times, most recently from 127fdfb to 9347fa9 Compare

June 28, 2023 00:48

henghonglee requested a review from dunxen

June 28, 2023 00:49

henghonglee force-pushed the issue-2304 branch from 9347fa9 to eeaa251 Compare

June 28, 2023 02:48

TheBlueMatt reviewed

View reviewed changes

Collaborator

TheBlueMatt left a comment

Basically LGTM, one comment.

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

Collaborator

TheBlueMatt commented Jun 29, 2023

IMO we should just merge this if its ready for 116, there's little diff and its a nice simple extra feature.

dunxen reviewed

View reviewed changes

Contributor

dunxen left a comment

Just nits, but LGTM

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

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

Contributor Author

henghonglee commented Jun 29, 2023

@TheBlueMatt let me work on a quick fix of all the comments rn. should be done in abit

henghonglee force-pushed the issue-2304 branch from eeaa251 to 4dae282 Compare

June 29, 2023 16:15

henghonglee commented

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

henghonglee force-pushed the issue-2304 branch from 4dae282 to 4c117af Compare

June 30, 2023 03:26

henghonglee requested a review from TheBlueMatt

June 30, 2023 03:28

henghonglee force-pushed the issue-2304 branch 5 times, most recently from e3ed1b9 to 5d253ec Compare

June 30, 2023 13:44

henghonglee force-pushed the issue-2304 branch from 5d253ec to 6539864 Compare

June 30, 2023 15:32

Contributor Author

henghonglee commented Jul 1, 2023

hmmm i'm having a problem with the impl_writeable_tlv_based_enum macro. it causes a CI issue with addr2line

henghonglee force-pushed the issue-2304 branch 2 times, most recently from 9eb4b78 to 78e1c39 Compare

July 3, 2023 00:14

Contributor Author

henghonglee commented Jul 3, 2023

test failures related to #2385

henghonglee requested a review from dunxen

July 3, 2023 03:51

dunxen previously approved these changes

View reviewed changes

valentinewallace reviewed

View reviewed changes

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

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

henghonglee dismissed dunxen’s stale review via

a6bb211

July 3, 2023 14:56

henghonglee force-pushed the issue-2304 branch from 78e1c39 to a6bb211 Compare

July 3, 2023 14:56

henghonglee requested a review from valentinewallace

July 3, 2023 14:58

henghonglee force-pushed the issue-2304 branch 2 times, most recently from 008e700 to 4a25aab Compare

July 3, 2023 15:20

valentinewallace reviewed

View reviewed changes

lightning/src/ln/channel.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

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

henghonglee force-pushed the issue-2304 branch 3 times, most recently from 02393ba to 973943d Compare

July 4, 2023 03:06

henghonglee requested a review from valentinewallace

July 4, 2023 13:46

dunxen reviewed

View reviewed changes

Contributor

dunxen left a comment

LGTM

lightning/src/ln/shutdown_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

valentinewallace reviewed

View reviewed changes

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


          Add ChannelShutdownState to ChannelDetails

47cb45e

This commit adds the state of channel shutdown to channeldetails

henghonglee force-pushed the issue-2304 branch from 973943d to 47cb45e Compare

July 6, 2023 02:51

Contributor Author

henghonglee commented Jul 6, 2023

Addressed comments

valentinewallace approved these changes

View reviewed changes

TheBlueMatt approved these changes

View reviewed changes

TheBlueMatt merged commit 3236be1 into lightningdevkit:main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet