[devp2p discovery]: cleanup#11547
Conversation
ordian
added
A2-insubstantial 👶
This commit it removes the lifetime on type `Discovery` by making `request_backoff: &'static [Duration]` instead.
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
lgtm; added a few suggestions (maybe for another PR?)
| sent_at: Instant::now(), | ||
| node: node.clone(), | ||
| echo_hash: hash, | ||
| deprecated_echo_hash: old_parity_hash, |
There was a problem hiding this comment.
Is there a chance we can get rid of this you think?
There was a problem hiding this comment.
what do you mean?
the entire in_flight_pings.insert ?
There was a problem hiding this comment.
No, I mean deprecated_echo_hash – I guess we must have made a change here at some point and kept the old hash around for backwards compatibility. Do we still have to include it you think? Can we ever remove it?
There was a problem hiding this comment.
Probably but I'm not sure.
| fn send_packet(&mut self, packet_id: u8, address: &SocketAddr, payload: &[u8]) -> Result<H256, Error> { | ||
| fn send_packet(&mut self, packet_id: u8, address: SocketAddr, payload: &[u8]) -> Result<H256, Error> { | ||
| let packet = assemble_packet(packet_id, payload, &self.secret)?; | ||
| let hash = H256::from_slice(&packet[0..32]); |
There was a problem hiding this comment.
This is a bit annoying, we just built one of these in assemble_packet(); maybe return the signed_hash from there?
There was a problem hiding this comment.
hehe, right I do get your concern
In terms, of clean code if we change assemble_packet then it does clearly more than one thing but it could be quite nice if we could introduce another type Packet (or similar) to avoid copy_from_slice stuff.
But, let's address it another PR :)
There was a problem hiding this comment.
Do you really think having assemble_packet return Result<(Bytes, H256), Error> would be so bad? It's a private method after all, and it wouldn't be doing more or different work?
But, let's address it another PR :)
Absolutely.
There was a problem hiding this comment.
Do you really think having assemble_packet return Result<(Bytes, H256), Error> would be so bad? It's a private method after all, and it wouldn't be doing more or different work?
It is fine but my general opinion it should not :)
* master: informant: display I/O stats (#11523) [devp2p discovery]: remove `deprecated_echo_hash` (#11564) [secretstore] create db_version file when database doesn't exist (#11570) Remove Parity's Security Policy (#11565) ethcore/res: enable ecip-1088 phoenix upgrade for kotti and mordor testnets (#11529) Misc docs and renames …and one less clone (#11556) [secretstore]: don't sign message with only zeroes (#11561) [devp2p discovery]: cleanup (#11547) Code cleanup in the sync module (#11552) initial cleanup (#11542) Warn if genesis constructor revert (#11550)
3 participants
Fixes:
It can be merged after #11540 to avoid conflicts.