Some refactoring in network-bridge in the course of dealing with #2177 #2263
Conversation
By having everything peer set related depend directly on the enum the code becomes more clear and it is also straight forward to add more peersets/protocols as the compiler will complain if you forget to implement parts of it.
For feature real_overseer. + Fixes from review. Thanks @coriolinus and @ordian!
Some changes, which would have helped me in groking the code faster. Entry points/public types more to the top. Factored out implementation in their own files, to clear up the top-level view.
Does not add much at this level.
eskimor
added
A0-please_review
node/network/bridge/src/network.rs
Outdated
|
|
||
| /// Send a message to the network. | ||
| /// | ||
| /// Usually this will be a `WireMessage`. |
There was a problem hiding this comment.
Well right now, it always is a WireMessage, but the function does not care. On the other hand, it takes a PeerSet which describes which protocol to use and therefore actually specifies what a valid message would be. Which leads again to the question I asked you the other day:
"Unrelated: Are there any particular reasons why WireMessage is parametric over M which is either ValidationProtocol or CollationProtocol right now, instead of WireMessage just having a separate constructor for each?"
If we had a single sum type which specifies valid messages, which naturally maps to the PeerSet enum, this invariant would easily be expressible. (We would just not pass the PeerSet, as it could be derived from the passed message and therefore we make the expression of invalid combinations impossible.)
For this internally and not often used function it should not matter much actually, but that relationship would be handy in a couple of places. Also, I got the feeling that we will likely want more fine grained peer sets than we have right now, so making the implementation of new peer sets straight forward and less error prone seems worthwhile, which was the motivation for the other changes.
Ok, I derailed a bit on my answer, but I guess it gives some context to this PR in general.
There was a problem hiding this comment.
For completeness: Your answer was, that we want subsystems to only receive their messages, which is a good invariant to preserve obviously.
There was a problem hiding this comment.
Thanks. Can you amend the doc to better reflect that although this function doesn't restrict the message type, it is only accessible by the network bridge which is responsible for setting it to the correct type for the PeerSet?
eskimor
force-pushed
the
rk-minor-refactor-2177
branch
from
231c48a
to
9fd02b4
Compare
Extend copyright on new files to 2021 as well. Co-authored-by: Andronik Ordian <write@reusable.software>
|
bot merge |
|
Trying merge. |
ghost
merged commit 
…tytech#2177 (paritytech#2263) * More doc fixes. * Minor refactorings in the process of paritytech#2177 By having everything peer set related depend directly on the enum the code becomes more clear and it is also straight forward to add more peersets/protocols as the compiler will complain if you forget to implement parts of it. * Add peer set infos on startup properly For feature real_overseer. + Fixes from review. Thanks @coriolinus and @ordian! * More structure in network-bridge Some changes, which would have helped me in groking the code faster. Entry points/public types more to the top. Factored out implementation in their own files, to clear up the top-level view. * Get rid of local ProtocolName type definition. Does not add much at this level. * Fix tests + import cleanup. * Make spaces tabs. * Clarify what correct parameters to send_message are * Be more less vague in docs of send_message. * Apply suggestions from code review Extend copyright on new files to 2021 as well. Co-authored-by: Andronik Ordian <write@reusable.software> Co-authored-by: Andronik Ordian <write@reusable.software>
Changes fall in these categories:
For 2, in particular, I changed the code, so the compiler will tell you what to implement if you add another peer set. This leads to more robust code in this area .. you can not easily forget some parts of the implementation. The addition of 1, where I moved code around makes it a bit harder to review 2, but you can see the changes quite clearly by looking at this particular commit of this PR:
3af1f60 (I re-added the apparently missing peers_sets_info() function of this commit here).
If need be, I can also split this PR up in smaller ones.
Anyhow, wanted to get this out of the way as it is prone to merge conflicts.