Convert htlc structures to use named fields #4243
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
lightning/src/ln/channel.rs
Outdated
| InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(_)) => | ||
| Some(InboundHTLCStateDetails::AwaitingRemoteRevokeToRemoveFail), | ||
| InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailMalformed(_)) => | ||
| InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailMalformed{..}) => |
There was a problem hiding this comment.
nit: All of these are lacking some whitespace, i.e.:
| InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailMalformed{..}) => | |
| InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailMalformed{ .. }) => |
But that's non-blocking given that rustfmt will fix that for us eventually.
There was a problem hiding this comment.
Added rustfmt commit to clean up to avoid "eventually" 😂
lightning/src/ln/channel.rs
Outdated
| FailRelay(msgs::OnionErrorPacket), | ||
| FailMalformed(([u8; 32], u16)), | ||
| Fulfill(PaymentPreimage, Option<AttributionData>), | ||
| FailMalformed { hash: [u8; 32], code: u16 }, |
There was a problem hiding this comment.
Given we use sha256_of_onion in the message below, should we use that more-descriptive field name instead of hash here, too? Or would it be inaccurate?
Also maybe failure_code rather than code to be consistent?
There was a problem hiding this comment.
Yes, better indeed. Updated
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
See question above, otherwise LGTM
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4243 +/- ##
=======================================
Coverage 89.33% 89.33%
=======================================
Files 180 180
Lines 138680 138730 +50
Branches 138680 138730 +50
=======================================
+ Hits 123888 123938 +50
- Misses 12172 12174 +2
+ Partials 2620 2618 -2
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:
|
Preparation for serializing the enum. The serialization macros do not support multiple unnamed fields.
Clean up changes in previous commits.
joostjager
force-pushed
the
named-fields
branch
from
5780400 to
8116e0b
Compare
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
LGTM
Simple refactor, landing this.
|
👋 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
Preparation for serializing the enum. The serialization macros do not support multiple unnamed fields.
For enums values with >1 fields, named fields are more readable also.