Split ChannelManager::read into two stages #4332
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
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>
joostjager
force-pushed
the
split-chanmgr-read
branch
from
d762c44 to
6deae20
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 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
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:
|
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>
|
@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. |
|
🔔 1st Reminder Hey @jkczyz @valentinewallace! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @jkczyz @valentinewallace! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz @valentinewallace! This PR has been waiting for your review. |
lightning/src/ln/channelmanager.rs
Outdated
| pending_claiming_payments: pending_claiming_payments.unwrap(), | ||
| pending_claiming_payments: data.pending_claiming_payments.unwrap(), |
There was a problem hiding this comment.
For TLVs that may not be set -- but use the initialized Some value, IIUC -- should we instead unwrap when forming ChannelManagerData so the field there doesn't need an Option?
| pub(crate) fn channel_manager_from_data< | ||
| 'a, | ||
| M: Deref, | ||
| T: Deref, | ||
| ES: Deref, | ||
| NS: Deref, | ||
| SP: Deref, | ||
| F: Deref, | ||
| R: Deref, | ||
| MR: Deref, | ||
| L: Deref + Clone, | ||
| >( | ||
| data: ChannelManagerData<SP>, | ||
| mut args: ChannelManagerReadArgs<'a, M, T, ES, NS, SP, F, R, MR, L>, | ||
| ) -> Result<(BlockHash, ChannelManager<M, T, ES, NS, SP, F, R, MR, L>), DecodeError> | ||
| where | ||
| M::Target: chain::Watch<<SP::Target as SignerProvider>::EcdsaSigner>, | ||
| T::Target: BroadcasterInterface, | ||
| ES::Target: EntropySource, | ||
| NS::Target: NodeSigner, | ||
| SP::Target: SignerProvider, | ||
| F::Target: FeeEstimator, | ||
| R::Target: Router, | ||
| MR::Target: MessageRouter, | ||
| L::Target: Logger, | ||
| { |
There was a problem hiding this comment.
Any chance you can use an impl block to reduce the change set and thus keep the blame layer sane?
3 participants
Introduce
ChannelManagerDataas an intermediate DTO that holds all deserialized data from aChannelManagerbefore validation. This splits the read implementation into:ChannelManagerDataThe existing validation and reconstruction logic remains unchanged; only the deserialization portion was extracted into the DTO's
ReadableArgsimplementation.Rationale
ChannelManagerloading