Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AWM sign request #99

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions crates/avalanche-types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
//! ecosystem.
//!
#![cfg_attr(docsrs, feature(doc_cfg))]
#[cfg(feature = "avalanchego")]
#[cfg_attr(docsrs, doc(cfg(feature = "avalanchego")))]
pub mod avalanchego;
pub mod avm;
pub mod choices;
pub mod codec;
Expand All @@ -29,10 +32,7 @@ pub mod txs;
pub mod units;
pub mod utils;
pub mod verify;

#[cfg(feature = "avalanchego")]
#[cfg_attr(docsrs, doc(cfg(feature = "avalanchego")))]
pub mod avalanchego;
pub mod warp;

#[cfg(feature = "coreth")]
#[cfg_attr(docsrs, doc(cfg(feature = "coreth")))]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::{collections::HashMap, io::Result};

use crate::warp::WarpSignerClient_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep imports merged.

use crate::{
ids,
subnet::rpc::{
Expand All @@ -18,7 +19,6 @@ use crate::{
},
};
use tokio::sync::mpsc::Sender;

/// Vm describes the trait that all consensus VMs must implement.
///
/// ref. <https://pkg.go.dev/github.com/ava-labs/avalanchego/snow/engine/common#Vm>
Expand All @@ -29,7 +29,7 @@ pub trait CommonVm: AppHandler + Connector + Checkable {
type ChainHandler: Handle;
type StaticHandler: Handle;
type ValidatorState: validators::State;

type WarpSigner: WarpSignerClient_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this type alias?

async fn initialize(
&mut self,
ctx: Option<Context<Self::ValidatorState>>,
Expand All @@ -40,6 +40,7 @@ pub trait CommonVm: AppHandler + Connector + Checkable {
to_engine: Sender<Message>,
fxs: &[Fx],
app_sender: Self::AppSender,
warp_signer: Self::WarpSigner,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) -> Result<()>;
async fn set_state(&self, state: State) -> Result<()>;
async fn shutdown(&self) -> Result<()>;
Expand Down
5 changes: 4 additions & 1 deletion crates/avalanche-types/src/subnet/rpc/vm/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::{
time::{Duration, Instant},
};

use crate::warp::client::WarpSignerClient;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep imports merged.

use crate::{
ids,
packer::U32_LEN,
Expand Down Expand Up @@ -96,6 +97,7 @@ where
DatabaseManager = DatabaseManager,
AppSender = AppSenderClient,
ValidatorState = ValidatorStateClient,
WarpSigner = WarpSignerClient,
> + Send
+ Sync
+ 'static,
Expand Down Expand Up @@ -182,7 +184,7 @@ where
return tonic::Status::unknown("engine receiver closed unexpectedly");
}
});

let warp_signer = WarpSignerClient::new(client_conn.clone());
let mut inner_vm = self.vm.write().await;
inner_vm
.initialize(
Expand All @@ -194,6 +196,7 @@ where
tx_engine,
&[()],
AppSenderClient::new(client_conn.clone()),
warp_signer,
)
.await
.map_err(|e| tonic::Status::unknown(e.to_string()))?;
Expand Down
43 changes: 43 additions & 0 deletions crates/avalanche-types/src/warp/client.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please merge all imports


#[derive(Clone)]
pub struct WarpSignerClient {
inner: signer_client::SignerClient<Channel>,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of the wrapper here? Why not just use the SignerClient directly?


impl WarpSignerClient {
pub fn new(client_conn: Channel) -> Self {
Self {
inner: signer_client::SignerClient::new(client_conn)
.max_decoding_message_size(usize::MAX)
.max_encoding_message_size(usize::MAX),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are already the defaults

Suggested change
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),

}
}
}

#[tonic::async_trait]
impl super::WarpSignerClient_ for WarpSignerClient {
async fn sign(
&self,
network_id: u32,
source_chain_id: &str,
payload: &[u8],
) -> Result<SignResponse> {
let mut client = self.inner.clone();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let res = client
.sign(SignRequest {
network_id,
source_chain_id: Bytes::from(Id::from_str(source_chain_id).unwrap().to_vec()),
payload: Bytes::from(payload.to_vec()),
})
.await
.map_err(|e| Error::new(ErrorKind::Other, format!("sign failed: {:?}", e)))?;
Ok(res.into_inner())
}
}
33 changes: 33 additions & 0 deletions crates/avalanche-types/src/warp/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
pub mod client;

use crate::proto::warp::SignResponse;
use std::io::Result;

#[tonic::async_trait]
pub trait WarpSignerClient_: Send + Sync + CloneBox {
async fn sign(
&self,
network_id: u32,
source_chain_id: &str,
payload: &[u8],
) -> Result<SignResponse>;
}

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()
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?