Minor refactor for peer sets in Polkadot#2220
Minor refactor for peer sets in Polkadot#2220eskimor wants to merge 3 commits intotka-companion-rm-prio-groupsfrom
Conversation
eskimor
added
A0-please_review
| /// Convert a peer set into a protocol name as understood by Substrate. | ||
| /// | ||
| /// With `ProtocolName` being a proper newtype we could use the `Into` trait here. | ||
| pub fn into_protocol_name(self) -> ProtocolName { |
There was a problem hiding this comment.
why not impl From<PeerSet> for ProtocolName?
Ah, should have read the docs first 🤦
There was a problem hiding this comment.
There was a problem hiding this comment.
Definitely, but I do not think it is valuable to have it for the generic Cow type. In that case I think a special method like into_protocol_name would be preferable.
into() ... with type signature PeerSet -> ProtocolName is pretty self explanatory, not so much with PeerSet -> Cow<'static, str>, therefore I opted for a more descriptive name.
| /// | ||
| /// Should be used during network configuration (added to [`NetworkConfiguration::extra_sets`]) | ||
| /// or shortly after startup to register the protocols with the network service. | ||
| pub fn peer_set_infos() -> Vec<sc_network::config::NonDefaultSetConfig> { |
There was a problem hiding this comment.
| pub fn peer_set_infos() -> Vec<sc_network::config::NonDefaultSetConfig> { | |
| pub fn peer_set_info() -> Vec<sc_network::config::NonDefaultSetConfig> { |
Nit: "information" is inherently plural; "infos" sounds weird.
There was a problem hiding this comment.
Fair :-) Ok I will go with peer_sets_info then.
|
|
||
| /// Protocol name as understood in substrate. | ||
| /// | ||
| /// Ideally this would be defined in substrate as a newtype. |
There was a problem hiding this comment.
Making the newtype would be a trivial but a pervasive change in substrate, likely not worth it, as being in substrate, it would also break code for other users. Type safety gains are probably low, as we are not using Cow<'static,str> much.
Just putting this out there to see what people think.
eskimor
force-pushed
the
rk-minor-refactor
branch
from
490d838 to
0c96592
Compare
|
All fixed. @tomaka do you want to merge this? |
|
I don't have any strong opinion in favour or against this |
Also always interested in weak opinions ;-) Anyhow, those changes (and the ones to come) makes addition of new protocols a bit easier and more straight forward. |
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!
eskimor
force-pushed
the
rk-minor-refactor
branch
from
0c96592 to
86453f0
Compare
|
Did you compile the whole Polkadot with this? I see that Also, I'd argue to do this after #2095 is merged. #2095 intentionally doesn't change anything (or rather, changes as little as possible) compared to master. |
|
I did compile all of Polkadot with --all-features with those changes. But delaying this until #2095 is merged is totally fine with me. |
|
Will be superseded by PR against master. |
4 participants
By having everything peer set related depend directly on the PeerSet 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.