-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
Cargo.lock
Outdated
@@ -119,7 +119,7 @@ name = "bigint" | |||
version = "3.0.0" | |||
source = "registry+https://github.com/rust-lang/crates.io-index" | |||
dependencies = [ | |||
"byteorder 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", | |||
"byteorder 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)", |
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.
do you need 1.1 version update for this pr?
better keep version updates separately just in case to revert easy
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.
yes, it is needed because 1.0 does not define behavior of read_f64
to mask out sNaN, while 1.1.0 does. I don't see why it would be reverted, as 1.1.0 is fully backwards compatible with 1.0.
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.
Sure, i was talking about cases where dependencies upgraded for no actual reason, which is obviously not a case here!
Ready for review again -- version field has been dropped because it's largely useless. Note that all that geth and parity agree upon here is the network protocol. The RPCs are still up in the air. |
@@ -496,6 +496,8 @@ pub trait ManageNetwork : Send + Sync { | |||
fn stop_network(&self); | |||
/// Query the current configuration of the network | |||
fn network_config(&self) -> NetworkConfiguration; | |||
/// Get network context for protocol. | |||
fn with_proto_context(&self, proto: ProtocolId, f: &mut FnMut(&NetworkContext)); |
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.
can you think of better way of extending this api which could be interprocess one day?
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.
The main problem it solves is creating a pathway for the whisper RPC to post messages to the whisper network. Extending the API to make that possible would require us to come up with a cross-binary network context, which is a desirable feature in general. Changing the signature to fn proto_context(&self, proto: ProtocolId) -> Option<&NetworkContext>
could work, but would require pretty large changes to the inner workings.
whisper/src/net.rs
Outdated
return Err(Error::InvalidPowReq); | ||
} | ||
|
||
// as of byteorder 1.1.0, this is always defined and |
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.
... and what ?
@@ -339,7 +329,7 @@ impl<P: PoolHandle + 'static, M: Send + Sync + 'static> Whisper for WhisperClien | |||
payload: encrypted, | |||
topics: req.topics.into_iter().map(|x| abridge_topic(&x.into_inner())).collect(), | |||
work: req.priority, | |||
}); | |||
}).map_err(|_| whisper_error("Empty topics"))?; |
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.
Is it ("Empty topics"
) the only possible reason for Message::create
to fail?
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.
For now at least, why?
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.
Just asking, because it's far from obvious in the context
With extension for multiple topics when connected to parity peers.
Also immediately dispatches messages upon post from RPC now. Rallying is not required in this version.