Skip to content

Split ChannelManager::read into two stages #4332

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
Open
joostjager wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Split ChannelManager::read into two stages #4332

joostjager wants to merge 2 commits into from

Conversation

@joostjager
Copy link
Contributor

@joostjager joostjager commented Jan 22, 2026 •
edited
Loading

Introduce ChannelManagerData as an intermediate DTO that holds all deserialized data from a ChannelManager before validation. This splits the read implementation into:

  1. Pure deserialization into ChannelManagerData
  2. Validation and reconstruction using the DTO

The existing validation and reconstruction logic remains unchanged; only the deserialization portion was extracted into the DTO's ReadableArgs implementation.

Rationale

  • Makes it easier to work with ChannelManager loading
  • Better testability: DTOs can be inspected/modified for edge cases
  • Better readability: clear separation between deserialization and validation concerns
  • Opens up path to potential incremental manager updates

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jan 22, 2026 •
edited
Loading

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

@joostjager @claude
Split ChannelManager::read into two stages
6deae20 
Introduce ChannelManagerData<SP> as an intermediate DTO that holds all
deserialized data from a ChannelManager before validation. This splits
the read implementation into:

1. Stage 1: Pure deserialization into ChannelManagerData
2. Stage 2: Validation and reconstruction using the DTO

The existing validation and reconstruction logic remains unchanged;
only the deserialization portion was extracted into the DTO's
ReadableArgs implementation.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@codecov
Copy link

codecov bot commented Jan 22, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.54%. Comparing base (8cdc86a) to head (50f3726).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #4332   +/-   ##
=======================================
  Coverage   86.53%   86.54%           
=======================================
  Files         158      158           
  Lines      103190   103245   +55     
  Branches   103190   103245   +55     
=======================================
+ Hits        89300    89351   +51     
- Misses      11469    11470    +1     
- Partials     2421     2424    +3
Flag Coverage Δ
fuzzing 37.14% <ø> (+0.07%) ⬆️
tests 85.83% <ø> (+<0.01%) ⬆️

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.

@joostjager @claude
Extract channel_manager_from_data from ChannelManager::read
50f3726 
Extract the stage 2 validation and reconstruction logic from the
ReadableArgs implementation into a standalone pub(crate) function.
This enables reuse of the ChannelManager construction logic from
deserialized data.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@joostjager joostjager self-assigned this Jan 22, 2026
@joostjager joostjager marked this pull request as ready for review January 22, 2026 18:12
@joostjager
Copy link
Contributor Author

joostjager commented Jan 23, 2026

@jkczyz pointed to #2819 (comment)

This PR could serve as a first step toward addressing #2819. An UninitializedChannelManager could potentially wrap ChannelManagerData? That said, keeping this PR focused on the separation of concerns and testability benefits mentioned in the description probably makes sense given that #2819 has been deprioritized.

@joostjager joostjager requested a review from jkczyz January 23, 2026 22:25
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jan 24, 2026

🔔 1st Reminder

Hey @jkczyz @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

1 similar comment
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jan 26, 2026

🔔 1st Reminder

Hey @jkczyz @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jan 26, 2026

🔔 2nd Reminder

Hey @jkczyz @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

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

Reviewers

@valentinewallace valentinewallace Awaiting requested review from valentinewallace

@jkczyz jkczyz Awaiting requested review from jkczyz

At least 1 approving review is required to merge this pull request.

Assignees

@joostjager joostjager

Labels

None yet

Projects

Weekly Goals
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@joostjager @ldk-reviews-bot