Skip to content

Conversation

@joostjager
Copy link
Contributor

Follows up to #4275

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Dec 12, 2025

👋 Thanks for assigning @TheBlueMatt 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.

@codecov
Copy link

codecov bot commented Dec 12, 2025

Codecov Report

❌ Patch coverage is 99.39024% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.39%. Comparing base (593d6aa) to head (867bd6e).

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 99.39% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4281      +/-   ##
==========================================
+ Coverage   89.37%   89.39%   +0.02%     
==========================================
  Files         180      180              
  Lines      139847   139846       -1     
  Branches   139847   139846       -1     
==========================================
+ Hits       124983   125012      +29     
+ Misses      12269    12253      -16     
+ Partials     2595     2581      -14     
Flag Coverage Δ
fuzzing 35.22% <82.31%> (+<0.01%) ⬆️
tests 88.72% <99.39%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

/// [`ChannelManager::finish_close_channel`].
///
/// Returns a mapped error.
fn get_coop_close_err(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmmmm, its not really just a "getter", though. Even convert was maybe a bit of a misnomer, but its really doing a bunch of stuff - its doing the initial channel closure handling, at least the locked part. Maybe locked_handle_coop_close (and similarly rename the rest locked_handle_*_close)?

/// When a channel is removed, two things need to happen:
/// (a) [`ChannelManager::convert_channel_err`] must be called in the same `per_peer_state` lock as the
/// channel-closing action,
/// (a) The close error must be converted to [`MsgHandleErrInternal`] in the same `per_peer_state` lock as the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Depending on if we rename the other methods from convert this might also be confusing. But this also implies that you can just build the MsgHandleErrInternal rather than calling one of the 3 close-handle-convert methods, maybe just link all of them?

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

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.

3 participants