ChannelManager read refactor follow ups #4374
Conversation
Reorder the struct fields to place all three `_legacy` fields together at the end, allowing the explanatory comment to appear once instead of being duplicated three times. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Initialize pending_claiming_payments and monitor_update_blocked_actions_per_peer with None and resolve with unwrap_or_else, matching the pattern used for other optional hash map fields like pending_intercepted_htlcs_legacy. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add step comments to from_channel_manager_data to clarify the seven logical phases of ChannelManager reconstruction: 1. Channel/monitor reconciliation 2. Initialize missing optional fields 3. Replay in-flight monitor updates 4. Reconstruct HTLC state from monitors 5. Reconstruct claimable payments 6. Construct the ChannelManager 7. Replay pending claims and fail HTLCs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
🎉 This PR is now ready for review! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4374 +/- ##
=======================================
Coverage 85.99% 86.00%
=======================================
Files 156 156
Lines 102766 102764 -2
Branches 102766 102764 -2
=======================================
+ Hits 88378 88383 +5
+ Misses 11879 11873 -6
+ Partials 2509 2508 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
Care to throw in any of these mentioned updates? #4332 (comment) Seems like a good opportunity
| testing_dnssec_proof_offer_resolution_override: Mutex::new(new_hash_map()), | ||
| }; | ||
|
|
||
| // Step 7: Replay pending claims and fail HTLCs. |
There was a problem hiding this comment.
In my experience using step numbers in ldk-sample main, it's kinda annoying to have to update all of them when something changes. Maybe that's not as much of a risk here
There was a problem hiding this comment.
I thought it might help a bit with navigation in this huge function. In other languages one would quickly extract methods, but in my experience that isn't always in Rust. Would you prefer that? I can try it.
There was a problem hiding this comment.
I wouldn't really prefer extracting methods. Just wanted to point it out, we can keep the numbers
|
👋 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. |
3 participants
Cleanup commits to improve the readability and maintainability of the
ChannelManagerdeserialization code._legacyfields together inChannelManagerDatato deduplicate their explanatory commentunwrap_or_elsepattern for optional TLV fields instead of initializing withSomeand usingunwrapfrom_channel_manager_data(stage 2 of deserialization)