Added peers details to ethcore_netPeers RPC #2580
Conversation
|
It should be ok with the Parity UI in master (accessed via localhost:8080/home/ -> status) and will be for the new UI as well (js branch) since in both cases we only look at the active/connected/max members. Once merged however I will update the JS branch (specifically the new Parity.js JSAPI to do proper formatting of the newly returned structure as well as update the documentation). Issue to be logged. |
|
@tomusdrw There are some errors with the current UI on the props validation side. We need to get those sorted in conjunction with this if this is good to go. So please take a peek at this one to make sure you are happy and it is valid to make the (minor) changes to adjust to the new format. |
There was a problem hiding this comment.
A couple of style concerns, looks good generally.
I'm not particularly concerned about the backwards compatibility aspect of this change, but I have a couple things to note:
- This is additive, so it won't break backwards compatibility. Once we commit to this format, though, we have to support it (at least after we make an official release containing it -- unstable can change at-will). Consider the case of supporting an arbitrary number of protocols, each with their own data.
- Parity will probably support LES soon, which means that we'll have to have nullable subprotocol fields for each protocol. A more extensible format might just have an array of { name, props } objects for each peer.
| /// Remote endpoint address | ||
| pub remote_address: String, | ||
| /// Local endpoint address | ||
| pub local_address: String, |
There was a problem hiding this comment.
extra spaces before client_version, remote_address, and local_address
|
|
||
| /// Peer connection information | ||
| #[derive(Binary)] | ||
| pub struct PeerInfo { |
There was a problem hiding this comment.
public-facing types should derive Debug as well.
| self.peers.iter() | ||
| .filter_map(|(&peer_id, ref peer_data)| { | ||
| if let Some(session_info) = io.peer_session_info(peer_id) { | ||
| return Some(PeerInfoDigest { |
There was a problem hiding this comment.
seems like a good use-case for Option::map
| fn peer_info(&self, peer_id: PeerId) -> String { | ||
| peer_id.to_string() | ||
| } | ||
| /// Returns peer client identifier string | ||
| fn peer_session_info(&self, peer_id: PeerId) -> Option<SessionInfo>; |
| match host.as_ref() { | ||
| Some(ref host) => Some(host.with_context_eval(protocol, &io, action)), | ||
| None => None | ||
| } |
There was a problem hiding this comment.
host.as_ref().map(|ref host| ...)
| let net_config = take_weak!(self.net).network_config(); | ||
| let peers = sync.peers().into_iter().map(|s| s.into()).collect(); |
There was a problem hiding this comment.
map(Into::into) is a little more idiomatic
| pub struct PeerProtocolsInfo { | ||
| /// Ethereum protocol information | ||
| pub eth: PeerEthereumProtocolInfo | ||
| } |
There was a problem hiding this comment.
This struct doesn't seem to be particularly extensible -- we might soon see cases where peers are connected either on eth, les, or both. Maybe the fields on this struct could be Optional? Or maybe that would be a premature refactoring.
There was a problem hiding this comment.
- I personally think that { name, props } isn't a good pattern for public APIs - it is hard to document, hard to use (in long term) & hard to maintain. Do you think this will be better than strictly typed struct's for each protocol?
- as for converting eth to an Option<> - I could do that right now. But when you'll met some code which has return type Option<> and unconditionally returns Some(...) - isn't it a bad pattern?
@rphmeier Could you, please, comment so we could agree on this?
There was a problem hiding this comment.
@svyatonik I agree that strictly typed is better. For now, I think it can be left as is, since the only peers we'll find will be connected to eth, but it should be documented as a nullable/optional field in the JSON structure so we can accommodate future subprotocols without breaking backwards compatibility.
|
Changes Unknown when pulling d02d67c on svyatonik:master into * on ethcore:master*. |
|
Changes Unknown when pulling d02d67c on svyatonik:master into * on ethcore:master*. |
- spaces -> tabs - Rust-way Option's handling
| @@ -368,6 +369,25 @@ impl ChainSync { | |||
| } | |||
| } | |||
|
|
|||
| /// @returns Information on peers connections | |||
There was a problem hiding this comment.
rustdoc doesn't do anything with the @ symbol like doxygen does, so it's probably unnecessary here.
rphmeier
added
A6-mustntgrumble 💦
|
LGTM modulo tiny docs format issue. |
rphmeier
added
A8-looksgood 🦄
closes ethcore/parity#2433
I've tried to make returned data to look exactly like Geth' admin.peers output. However, protocols.eth.difficulty is returned as decimal number in Geth, while in ethcore_* APIs all bigints are returned as hex numbers (at my first glance). Not sure if I should break this rule here => difficulty is also returned as hex number here.
I'm still not sure whether it is ok to alter existing RPC - Parity already had ethcore_netPeers RPC before - I've just added another member to the returned value. Hope this won't break any clients.