Skip to content

Extract peer message handling into separate method. #585

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

Closed

Conversation

arik-so
Copy link
Contributor

@arik-so arik-so commented Apr 13, 2020

Additionally, convert some handling macros into a method to avoid hidden control flow logic affecting a "parent" scope.

@jkczyz jkczyz self-requested a review April 14, 2020 17:44
@jkczyz
Copy link
Contributor

jkczyz commented Apr 16, 2020

Concept ACK. Nice refactor!

I'd like to see the PR broken into smaller commits. Possibly:

  • Refactor encode_and_send_msg! macro as enqueue_message method
  • Refactor try_potential_handleerror! macro as process_lightning_error method
  • Refactor message handling out from do_read_event

@ariard
Copy link

ariard commented Apr 27, 2020

@arik-so Do you see this as a first step toward custom message passing ? Like adding a trait to PeerHandler, MessageExtensionProxy::handle_unknown_message -> MessageProcessingDecision ?

I agree with Jeff, splitting in smaller commits would be better.

@arik-so
Copy link
Contributor Author

arik-so commented Apr 27, 2020

I do indeed see it as a first step towards that. I'm currently focusing on some refactors for the modular handshake PR, and will break this one down into smaller pieces after.

One of the challenges of breaking this into smaller pieces is that the extraction of message handling into a separate method requires a redeclaration of the macros. But converting the macros into methods doesn't convey the necessity of doing so for the sake of refactoring.

@arik-so
Copy link
Contributor Author

arik-so commented Apr 27, 2020

MessageProcessingDecision could also be something much more involved in the medium term future, as there should be a way for non-Rust-languages to pass in lambdas for handling custom inputs through bindings. That in itself presents some challenges due to various environments' issues with C lambdas capturing context, but that's merely a side note here.

arik-so added a commit to arik-so/rust-lightning that referenced this pull request May 18, 2020
This is a response to splitting lightningdevkit#585 into smaller components. This extraction will permit a future extraction of all message handling into a separate method, and then later even into a separate trait that will be accessible through language bindings.
arik-so added a commit to arik-so/rust-lightning that referenced this pull request May 18, 2020
This is a response to splitting lightningdevkit#585 into smaller components. This extraction will permit a future extraction of all message handling into a separate method, and then later even into a separate trait that will be accessible through language bindings.
arik-so added a commit to arik-so/rust-lightning that referenced this pull request May 19, 2020
This is a response to splitting lightningdevkit#585 into smaller components. This extraction will permit a future extraction of all message handling into a separate method, and then later even into a separate trait that will be accessible through language bindings.
arik-so added a commit to arik-so/rust-lightning that referenced this pull request May 22, 2020
This is a response to splitting lightningdevkit#585 into smaller components. This extraction will permit a future extraction of all message handling into a separate method, and then later even into a separate trait that will be accessible through language bindings.
arik-so added a commit to arik-so/rust-lightning that referenced this pull request May 22, 2020
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.
arik-so added a commit to arik-so/rust-lightning that referenced this pull request May 26, 2020
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.
arik-so added a commit to arik-so/rust-lightning that referenced this pull request Jun 1, 2020
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.
arik-so added a commit to arik-so/rust-lightning that referenced this pull request Jun 1, 2020
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.
arik-so added a commit to arik-so/rust-lightning that referenced this pull request Jun 7, 2020
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.
@TheBlueMatt
Copy link
Collaborator

Before we go too far down this rabbit hole, it would be nice to get a fix for the relay of graph messages, ala TheBlueMatt@2ce646f. I'm worried that this direction is going to make it harder to do eventually and we may have to walk things back, so would be nice to get that incorporated.

naumenkogs pushed a commit to naumenkogs/rust-lightning that referenced this pull request Oct 21, 2020
This is a response to splitting lightningdevkit#585 into smaller components. This extraction will permit a future extraction of all message handling into a separate method, and then later even into a separate trait that will be accessible through language bindings.
naumenkogs pushed a commit to naumenkogs/rust-lightning that referenced this pull request Oct 21, 2020
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.
@TheBlueMatt
Copy link
Collaborator

This kinda happened in part on accident with the custom message work, and the rest is more actively maintained in #1023. Closing this here as superseded.

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

Successfully merging this pull request may close these issues.

4 participants