Add AWM sign request #99
Conversation
shaokun11
requested review from
gyuho,
hexfusion and
richardpringle
as code owners
There was a problem hiding this comment.
Hey @shaokun11, your code looks pretty good at first glance! Could you run cargo fmt before I do a proper review though?
Just now I have do it, please review. |
There was a problem hiding this comment.
I'm sorry, but I'm not sure that I agree with the approach here. Cloning the client every time you sign doesn't make sense.
The SignerClient_ trait provides little value too. The SignerClient<Channel> is a concrete type.
I think we need an example of actual usage as well. You might see that the need for traits completely disappears if you actually go to use the signer.
| @@ -29,7 +29,7 @@ pub trait CommonVm: AppHandler + Connector + Checkable { | |||
| type ChainHandler: Handle; | |||
| type StaticHandler: Handle; | |||
| type ValidatorState: validators::State; | |||
|
|
|||
| type WarpSigner: WarpSignerClient_; | |||
There was a problem hiding this comment.
Why do you need this type alias?
| @@ -4,6 +4,7 @@ use std::{ | |||
| time::{Duration, Instant}, | |||
| }; | |||
|
|
|||
| use crate::warp::client::WarpSignerClient; | |||
There was a problem hiding this comment.
Please keep imports merged.
| @@ -1,5 +1,6 @@ | |||
| use std::{collections::HashMap, io::Result}; | |||
|
|
|||
| use crate::warp::WarpSignerClient_; | |||
There was a problem hiding this comment.
Please keep imports merged.
| use std::io::{Error, ErrorKind, Read, Result}; | ||
| use std::str::FromStr; | ||
|
|
||
| use crate::ids::Id; | ||
| use crate::proto::pb::warp::{signer_client, SignRequest, SignResponse}; | ||
| use prost::bytes::Bytes; | ||
| use tonic::transport::Channel; |
There was a problem hiding this comment.
Please merge all imports
| inner: signer_client::SignerClient::new(client_conn) | ||
| .max_decoding_message_size(usize::MAX) | ||
| .max_encoding_message_size(usize::MAX), |
There was a problem hiding this comment.
These are already the defaults
| inner: signer_client::SignerClient::new(client_conn) | |
| .max_decoding_message_size(usize::MAX) | |
| .max_encoding_message_size(usize::MAX), | |
| inner: signer_client::SignerClient::new(client_conn), |
| source_chain_id: &str, | ||
| payload: &[u8], | ||
| ) -> Result<SignResponse> { | ||
| let mut client = self.inner.clone(); |
There was a problem hiding this comment.
So you have to create a new signer client every time you need to sign? The clone here should be explicit by the user (or they can wrap their channel in some kind of lock). This masks the internal functionality and doesn't give users the option to do things more efficiently.
| #[derive(Clone)] | ||
| pub struct WarpSignerClient { | ||
| inner: signer_client::SignerClient<Channel>, | ||
| } |
There was a problem hiding this comment.
What's the benefit of the wrapper here? Why not just use the SignerClient directly?
| pub trait CloneBox { | ||
| fn clone_box(&self) -> Box<dyn WarpSignerClient_ + Send + Sync>; | ||
| } | ||
|
|
||
| impl<T> CloneBox for T | ||
| where | ||
| T: 'static + WarpSignerClient_ + Clone + Send + Sync, | ||
| { | ||
| fn clone_box(&self) -> Box<dyn WarpSignerClient_ + Send + Sync> { | ||
| Box::new(self.clone()) | ||
| } | ||
| } | ||
|
|
||
| impl Clone for Box<dyn WarpSignerClient_ + Send + Sync> { | ||
| fn clone(&self) -> Box<dyn WarpSignerClient_ + Send + Sync> { | ||
| self.clone_box() | ||
| } | ||
| } |
There was a problem hiding this comment.
None of these are necessary:
https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#impl-Clone-for-Box%3CT,+A%3E
Why do you need the trait object to be Clone in the first place?
| @@ -40,6 +40,7 @@ pub trait CommonVm: AppHandler + Connector + Checkable { | |||
| to_engine: Sender<Message>, | |||
| fxs: &[Fx], | |||
| app_sender: Self::AppSender, | |||
| warp_signer: Self::WarpSigner, | |||
There was a problem hiding this comment.
You could just pass in the concrete type here. It's unclear if there would ever be any associated type other than SignerClient<Channel> that was ever used.
|
@richardpringle Thank you for providing such a detailed review. I realize now that I was overthinking the problem, and now I have directly placed the signer_client into the context,how about it? |
That looks better! Can we add an example of usage before we merge this? |
No description provided.