-
Notifications
You must be signed in to change notification settings - Fork 41
Updated channels to reuse logic #938
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
Conversation
9b7e3ac to
f9ad784
Compare
|
@0xMimir when you are done please add a description on the summary of this PR explaining the change. Skimming through it I am still not sure I understand the benefits, and it seems to be changing the mental model of how channels work (we used to have one channel per kind of message, and each channel had one specific message, now it is one channel per peer with multiple types of messages?). It is important that the difference is clear so that we can properly evaluate it. |
|
f9ad784 to
bbcfc4d
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.
Could you provide a detailed list of the benefits of the channel reuse logic?
I'm not fully convinced about the extra level of wrapping that is required for pushing actions.
|
@binier if you have time, it would be useful to get your feedback on this one |
892eb07 to
4a132fe
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.
Looks good. Needs just some minor cosmetic changes.
p2p/src/channels/signaling/discovery/p2p_channels_signaling_discovery_reducer.rs
Outdated
Show resolved
Hide resolved
p2p/src/channels/signaling/discovery/p2p_channels_signaling_discovery_reducer.rs
Outdated
Show resolved
Hide resolved
e08b45e to
883b592
Compare
p2p/src/channels/transaction/p2p_channels_transaction_reducer.rs
Outdated
Show resolved
Hide resolved
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.
left a few more comments, but overall it LGTM now + it reduces the service interaction surface which is a big plus
883b592 to
74386ab
Compare
Added abstraction over channels service (
P2pChannelsService), main reason is to simplify channels layer and reduce number of actions that perform same thing, before this each channel had its effectful counterpart which would interact with service, now each channel interacts with new actions that are abstraction over methods onP2pChannelsService.Mental model of channels shouldn't change, only chain of actions from:
to:
Every channel still has its own ID and messages
Changes:
Request/Response would be sent like following:
P2pChannelsEffectfulAction::InitChannelaction.Benefits: