create common component for message serialization and signing #153
Conversation
| move || { | ||
| let signature = match sender.sign(&message) { | ||
| Ok(signature) => signature, | ||
| Err(err) => { |
There was a problem hiding this comment.
Do you know if we need to propagate this error?
There was a problem hiding this comment.
This is one of those errors that really really should never happen, and where there is not really a way to fall back. So there is no reason to propagate it somewhere.
There was a problem hiding this comment.
According to Murphy's law: Anything that can go wrong will go wrong :) It might be useful to test what happens to the rest of the system when this happens. I'll keep this in mind in my tasks.
| use ssv_types::consensus::UnsignedSSVMessage; | ||
| use ssv_types::message::SignedSSVMessage; | ||
|
|
||
| pub trait MessageSender: Send + Sync { |
There was a problem hiding this comment.
Very nice. In my PR, I was thinking about a way to split the "interface" and the implementation. First I created two crates, one with the trait and types and another with the implementation. The component C, which needs the functionality, imports only the interface crate, and who create C imports the implementation. But maybe two crates is too much overhead. I think we can achieve the same using one crate with different modules and features. Wdyt?
There was a problem hiding this comment.
Yeah, I was also tempted to do that. This is for example what we do for the validator_store: This only contains the trait and types used in the interface, and there are creates lighthouse_validator_store in LH repo and anchor_validator_store in Anchor repo.
However, as you said, for small stuff like this, it is likely too much noise and too little benefit to split it up.
There was a problem hiding this comment.
One benefit is enforcing modularity/encapsulation and to make sure that clients of the crate can only use what they are supposed to and nothing else. As a crate that provides a functionality grows, it's tempting for people to just use implementation details directly, bypassing the API when it's more convenient.
There was a problem hiding this comment.
That's the whole point of the module system, right? Consider the message validator. It's harder to do that with only one crate as the network needs access to the interface and the client module needs access to the implementation, so both need to be public. That's why I mentioned features, I guess that's the only option if there's only one crate.
There was a problem hiding this comment.
I don't get your point. I am using modules. How are features related? They are only for conditional compilation.
There was a problem hiding this comment.
something like this
// src/lib.rs
#[cfg(feature = "api")]
pub mod api;
#[cfg(feature = "impl")]
pub mod message_sender;| database: watch::Receiver<NetworkState>, | ||
| operator_id: OperatorId, | ||
| subnet_count: usize, | ||
| ) -> Result<Arc<Self>, String> { |
There was a problem hiding this comment.
Would it be better to return only Self and let the caller decide if and when to wrap it in an Arc?
There was a problem hiding this comment.
Usually I'd agree, but we impl MessageSender on Arc<NetworkMessageSender>. So it is only useful wrapped.
|
|
||
| impl TestingMessageSender { | ||
| pub fn new( | ||
| message_tx: mpsc::UnboundedSender<SignedSSVMessage>, |
There was a problem hiding this comment.
nit: this could be created inside the constructor if no assertion is needed in the tests
There was a problem hiding this comment.
But then we have to return (Self, Receiver), which is also kind of ugly.
|
Thanks for all your feedback <3 I am currently integrating this into the signature collector, and will likely gather some ways to also improve this in the process of doing so. I will address your feedback as soon as I'm fully confident with the design. |
| // Channel to pass signed messages along to the network | ||
| network_tx: mpsc::UnboundedSender<SignedSSVMessage>, | ||
| // Utility to sign and serialize network messages | ||
| message_sender: Arc<dyn MessageSender>, |
There was a problem hiding this comment.
Have you considered using generics?
Both the partial signature manager and the QBFT manager need to send messages. This PR introduces a component that schedules such tasks via the processor.
It also provides a mock message sender for use in tests.