Skip to content

Simplify ChannelUnavailable APIError handling with let-else#4736

Merged
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
Abeeujah:let-else
Jun 23, 2026
Merged

Simplify ChannelUnavailable APIError handling with let-else#4736
valentinewallace merged 2 commits into
lightningdevkit:mainfrom
Abeeujah:let-else

Conversation

@Abeeujah

Copy link
Copy Markdown
Contributor

e9e3060 Fixes the TODO for the error handling.
3d6a879 Fixes the triggered manual_str_repeat lint.

Refactor the nested match statement used during error construction into
a more idiomatic let-else construct (stabilised in Rust 1.65). The
previous implementation required verbose, nested matching to navigate
around borrow checker limitations.

By leveraging let-else alongside chaining unwrap_err on handle_error
Result, we achieve the same teardown logic (dropping state locks)
and error mapping with significantly less boilerplate and nesting.
@ldk-reviews-bot

ldk-reviews-bot commented Jun 22, 2026

Copy link
Copy Markdown

I've assigned @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.

@ldk-claude-review-bot

ldk-claude-review-bot commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

The code matches my prior review exactly and remains clean.

No issues found.

The PR is correct and behavior-preserving:

  • channelmanager.rs:11468-11473 — the let-else refactor is equivalent to the original match. handle_error is a map_err wrapper that always returns Err for Err input, so .unwrap_err() cannot panic; res remains owned in the else block since a failed let-else match moves nothing, making res.map(|_| ()) valid.
  • channelmanager.rs:19832 — dropping the now-unused Hasher import is correct.

@Hamza1610 Hamza1610 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Test Results: PASS

All tests passed successfully! However, there are a couple of minor warnings to address:

** Redundant Import in channelmanager.rs**
warning: the item Hasher is imported redundantly
--> lightning/src/ln/channelmanager.rs:19832:35
|
177 | use core::hash::{Hash, Hasher};
| ------ the item Hasher is already imported here
19832 | use core::hash::{BuildHasher, Hasher};
| ^^^^^^

Issue: Hasher is imported twice in the same file. Fix: Remove the redundant Hasher from line 19832, keeping only BuildHasher: use core::hash::{BuildHasher};

Overall: Tests pass |Only minor cleanup needed

Image Image

@Hamza1610

Copy link
Copy Markdown

When I was testing I noticed some ignored test. These might be deprecated test that are not in used anymore. I'm suggesting to create a new PR for this. @casey what do you think about that.

@Abeeujah

Copy link
Copy Markdown
Contributor Author

use core::hash::{Hash, Hasher};

Hi @Hamza1610

I appreciate the review! However, the unused import warning is actually unrelated to the changes in this PR. It looks like it was introduced to main in commit a1ad1a3

Given the ongoing Forgejo migration (#4734), it's probably best to avoid scope creep here to keep this PR clean and prevent untracked changes.

@Hamza1610

Copy link
Copy Markdown

Yes, you are right, PR has be be clean. Thanks for highlighting this.

@valentinewallace valentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! Second commit is redundant with #4727

@Abeeujah

Copy link
Copy Markdown
Contributor Author

Second commit is redundant with #4727

@valentinewallace thanks for the review, I've dropped the commit, and addressed the duplicate import warning.

@valentinewallace

Copy link
Copy Markdown
Contributor

Looks like the second commit is still there?

@Abeeujah

Copy link
Copy Markdown
Contributor Author

Looks like the second commit is still there?

My Bad, it's not anymore; Successfully dropped the commit via rebase.

@valentinewallace valentinewallace merged commit 1c1a4ad into lightningdevkit:main Jun 23, 2026
16 of 18 checks passed
@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.90%. Comparing base (b6f3ad7) to head (b4f7416).
⚠️ Report is 3215 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4736      +/-   ##
==========================================
+ Coverage   84.54%   86.90%   +2.35%     
==========================================
  Files         137      161      +24     
  Lines       77617   111680   +34063     
  Branches    77617   111680   +34063     
==========================================
+ Hits        65625    97051   +31426     
- Misses       9949    12121    +2172     
- Partials     2043     2508     +465     
Flag Coverage Δ
fuzzing-fake-hashes 7.77% <0.00%> (?)
fuzzing-real-hashes 29.82% <20.00%> (?)
tests 86.24% <100.00%> (?)

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

☔ View full report in Codecov by Harness.
📢 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.

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.

5 participants