-
Notifications
You must be signed in to change notification settings - Fork 407
Extract wire message handling into a method. #629
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.
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
Extract wire message handling into a method. #629
Conversation
2da6b77
to
9c6d5ea
Compare
Codecov Report
@@ Coverage Diff @@
## master #629 +/- ##
==========================================
- Coverage 91.26% 91.24% -0.02%
==========================================
Files 35 35
Lines 20918 20942 +24
==========================================
+ Hits 19090 19109 +19
- Misses 1828 1833 +5
Continue to review full report at Codecov.
|
lightning/src/ln/peer_handler.rs
Outdated
enum MessageHandlingResult { | ||
Ok, | ||
PeerHandleError(PeerHandleError), | ||
LightningError(LightningError), | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use Result
with an enum error instead of rolling a custom result type? You should be able to match
on complex patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, that's a much better approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(done)
Second all of Jeff's comments but this looks like a nice change. Might appreciate a high-level comment with what the end goal |
9c6d5ea
to
73b379f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update -- looks good! Just some minor stuff
return Err(PeerHandleError{ no_connection_possible: false }.into()); | ||
} | ||
|
||
log_info!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems "unknown local flags" and "unknown global" flags could use some clarification, since they're the same thing bits here? Maybe just a comment if there's good reasoning, else rename one more descriptively and get rid of the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rephrased this
73b379f
to
bf11673
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Can you rebase?
This is a response to splitting lightningdevkit#585 into smaller components. This extraction should allow the subsequent creation of a trait for all message handling, thereby enabling more flexibility in the state machine, particularly for bindings.
836c7b0
to
3838c04
Compare
This is a response to splitting #585 into smaller components. This extraction should allow the subsequent creation of a trait for all message handling, thereby enabling more flexibility in the state machine, particularly for bindings.