From eae811ba955f153f3b480fa10a8b5a76d267fd04 Mon Sep 17 00:00:00 2001 From: eftychis Date: Thu, 12 Mar 2020 17:59:15 -0700 Subject: [PATCH 1/5] Remove template in favor of an enum for signer Going over the spec plus the identity designs so far, it seems we shouldn't expect more than 1-2 more request types worst case, if any. This was discussed with Hans and Enzo at some point last week. While it makes the construction a bit bulkier it removes the generic and as a result removes one dependency. Also I think this might make it easier to modify the trait or future related trait realizations (implementations). --- Cargo.lock | 1 - src/ic_http_agent/Cargo.toml | 1 - src/ic_http_agent/src/agent/mod.rs | 6 ++- src/ic_http_agent/src/agent/replica_api.rs | 17 +++++--- src/ic_http_agent/src/agent/signer.rs | 50 ++++++++++------------ 5 files changed, 38 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 47fc03b02b..1502d98e1d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1503,7 +1503,6 @@ version = "0.1.0" dependencies = [ "byteorder 1.3.2 (registry+https://github.com/rust-lang/crates.io-index)", "crc8 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", - "erased-serde 0.3.10 (registry+https://github.com/rust-lang/crates.io-index)", "hex 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "leb128 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", "mockito 0.20.0 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/src/ic_http_agent/Cargo.toml b/src/ic_http_agent/Cargo.toml index 5c647d9c13..edb77ffa03 100644 --- a/src/ic_http_agent/Cargo.toml +++ b/src/ic_http_agent/Cargo.toml @@ -9,7 +9,6 @@ edition = "2018" [dependencies] byteorder = "1.3.2" crc8 = "0.1.1" -erased-serde = "0.3.10" hex = "0.4.0" leb128 = "0.2.4" openssl = "0.10.24" diff --git a/src/ic_http_agent/src/agent/mod.rs b/src/ic_http_agent/src/agent/mod.rs index f35a21cb6c..4e55101362 100644 --- a/src/ic_http_agent/src/agent/mod.rs +++ b/src/ic_http_agent/src/agent/mod.rs @@ -21,7 +21,9 @@ mod agent_test; pub mod signer; -use crate::agent::replica_api::{QueryResponseReply, ReadRequest, ReadResponse, SubmitRequest}; +use crate::agent::replica_api::{ + QueryResponseReply, ReadRequest, ReadResponse, Request, SubmitRequest, +}; use crate::agent::signer::Signer; use crate::{Blob, CanisterAttributes, CanisterId, RequestId}; @@ -82,7 +84,7 @@ impl Agent { async fn submit(&self, request: SubmitRequest<'_>) -> Result { // We need to calculate the signature, and thus also the // request id initially. - let request: Box> = Box::new(request); + let request = Request::Submit(request); let (request_id, signed_request) = self.signer.sign(request)?; let record = serde_cbor::to_vec(&signed_request)?; diff --git a/src/ic_http_agent/src/agent/replica_api.rs b/src/ic_http_agent/src/agent/replica_api.rs index eb8dbdf38f..4dbdba1f91 100644 --- a/src/ic_http_agent/src/agent/replica_api.rs +++ b/src/ic_http_agent/src/agent/replica_api.rs @@ -3,10 +3,10 @@ use serde::{Deserialize, Serialize}; /// Request payloads for the /api/v1/read endpoint. /// This never needs to be deserialized. -#[derive(Debug, Serialize)] +#[derive(Debug, Clone, Serialize)] #[serde(rename_all = "snake_case")] #[serde(tag = "request_type")] -pub(crate) enum ReadRequest<'a> { +pub enum ReadRequest<'a> { Query { canister_id: &'a CanisterId, method_name: &'a str, @@ -41,7 +41,7 @@ pub(crate) enum ReadResponse { #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "snake_case")] #[serde(tag = "request_type")] -pub(crate) enum SubmitRequest<'a> { +pub enum SubmitRequest<'a> { InstallCode { canister_id: &'a CanisterId, module: &'a Blob, @@ -57,6 +57,13 @@ pub(crate) enum SubmitRequest<'a> { }, } +#[derive(Debug, Clone, Serialize)] +#[serde(untagged)] +pub enum Request<'a> { + Submit(SubmitRequest<'a>), + Query(ReadRequest<'a>), +} + #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "snake_case")] pub struct MessageWithSender { @@ -67,9 +74,9 @@ pub struct MessageWithSender { #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "snake_case")] -pub struct SignedMessage { +pub struct SignedMessage<'a> { #[serde(flatten)] - pub request_with_sender: T, + pub request_with_sender: Request<'a>, pub sender_pubkey: Blob, #[serde(rename = "sender_sig")] pub signature: Blob, diff --git a/src/ic_http_agent/src/agent/signer.rs b/src/ic_http_agent/src/agent/signer.rs index c26360b4d0..900fd004c0 100644 --- a/src/ic_http_agent/src/agent/signer.rs +++ b/src/ic_http_agent/src/agent/signer.rs @@ -1,5 +1,5 @@ use crate::agent::agent_error::AgentError; -use crate::agent::replica_api::SignedMessage; +use crate::agent::replica_api::{Request, SignedMessage}; use crate::types::request_id::to_request_id; use crate::{Blob, RequestId}; @@ -16,16 +16,7 @@ use crate::{Blob, RequestId}; // lifetime, which ends up complicating and polluting the remaining // code. pub trait Signer: Sync { - fn sign<'a>( - &self, - request: Box<(dyn erased_serde::Serialize + Send + Sync + 'a)>, - ) -> Result< - ( - RequestId, - Box, - ), - AgentError, - >; + fn sign<'a>(&self, request: Request<'a>) -> Result<(RequestId, SignedMessage<'a>), AgentError>; } pub struct DummyIdentity {} @@ -44,18 +35,9 @@ pub struct DummyIdentity {} // process. Doing it this manually here ends up being messy and // distracts from the logic. Thus, we use the erased_serde crate. impl Signer for DummyIdentity { - fn sign<'a>( - &self, - request: Box<(dyn erased_serde::Serialize + Send + Sync + 'a)>, - ) -> Result< - ( - RequestId, - Box, - ), - AgentError, - > { - let mut sender = vec![0; 32]; - sender.push(0x02); + fn sign<'a>(&self, request: Request<'a>) -> Result<(RequestId, SignedMessage<'a>), AgentError> { + // let mut sender = vec![0; 32]; + // sender.push(0x02); // Bug(eftychis): Note normally the behavior here is to add a // sender field that contributes to the request id. Right now // there seems to be an issue with the behavior of sender in @@ -74,13 +56,17 @@ impl Signer for DummyIdentity { signature, sender_pubkey, }; - Ok((request_id, Box::new(signed_request))) + Ok((request_id, signed_request)) } } #[cfg(test)] mod test { use super::*; + + use crate::agent::replica_api::{ReadRequest, Request}; + use crate::CanisterId; + use proptest::prelude::*; use serde::Serialize; @@ -88,15 +74,23 @@ mod test { // API. proptest! { #[test] - fn request_id_dummy_signer(request: String) { + fn request_id_dummy_signer(request_body: String) { #[derive(Clone,Serialize)] struct TestAPI { inner : String} - let request = TestAPI { inner: request}; + let arg = Blob::random(10); + let canister_id = CanisterId::from(Blob::random(10)); + let request = ReadRequest::Query { + arg: &arg, + canister_id: &canister_id, + method_name: &request_body, + }; + + - let request_with_sender = request.clone(); + let request_with_sender = Request::Query(request.clone()); let actual_request_id = to_request_id(&request_with_sender).expect("Failed to produce request id"); let signer = DummyIdentity {}; - let request_id = signer.sign(Box::new(request)).expect("Failed to sign").0; + let request_id = signer.sign(request_with_sender).expect("Failed to sign").0; assert_eq!(request_id, actual_request_id) }} } From 53370655a62ba391279de79e1acc7681bfa0d9e9 Mon Sep 17 00:00:00 2001 From: eftychis Date: Mon, 16 Mar 2020 11:03:45 -0700 Subject: [PATCH 2/5] Update documentation --- src/ic_http_agent/src/agent/signer.rs | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/src/ic_http_agent/src/agent/signer.rs b/src/ic_http_agent/src/agent/signer.rs index 900fd004c0..4ba5956785 100644 --- a/src/ic_http_agent/src/agent/signer.rs +++ b/src/ic_http_agent/src/agent/signer.rs @@ -21,29 +21,16 @@ pub trait Signer: Sync { pub struct DummyIdentity {} -// Right now serialize can not be made into a trait object out of the -// box because of object safety. This should change in the future. For -// the same reason equipping the Signer with a generic function ends -// up in a trait that can not be made into a trait object at compile -// time that depends on a trait with a similar ailment. This makes -// things simply complicated. Making the Signer parametric on a -// Serialize type means we have to pass it along and pushes the issue -// to dfx or the agent main body of code. Thus, we simply treat the -// issue here at its root: we pick an erased Serde Serialize trait and -// return one too. This is compatible with serde Serialize, -// constructing a holder object and intermediate trait in the -// process. Doing it this manually here ends up being messy and -// distracts from the logic. Thus, we use the erased_serde crate. impl Signer for DummyIdentity { fn sign<'a>(&self, request: Request<'a>) -> Result<(RequestId, SignedMessage<'a>), AgentError> { - // let mut sender = vec![0; 32]; - // sender.push(0x02); // Bug(eftychis): Note normally the behavior here is to add a // sender field that contributes to the request id. Right now // there seems to be an issue with the behavior of sender in // the request id. Trying to figure out if the correct // behaviour changed and where the deviation happens. + // let mut sender = vec![0; 32]; + // sender.push(0x02); // let sender = Blob::from(sender); // let request_with_sender = MessageWithSender { request, sender }; let request_with_sender = request; From 78bf53c4a6205d35a4c1692fc4d97ee63f9daa92 Mon Sep 17 00:00:00 2001 From: eftychis Date: Fri, 20 Mar 2020 12:45:29 -0700 Subject: [PATCH 3/5] Update --- src/dfx/src/lib/identity.rs | 38 +++++++++++---------------- src/ic_http_agent/src/agent/mod.rs | 2 +- src/ic_http_agent/src/agent/signer.rs | 4 --- 3 files changed, 16 insertions(+), 28 deletions(-) diff --git a/src/dfx/src/lib/identity.rs b/src/dfx/src/lib/identity.rs index 8d4f8f8091..991351253a 100644 --- a/src/dfx/src/lib/identity.rs +++ b/src/dfx/src/lib/identity.rs @@ -1,9 +1,7 @@ use ic_http_agent::to_request_id; use ic_http_agent::AgentError; -use ic_http_agent::Blob; -use ic_http_agent::RequestId; -use ic_http_agent::SignedMessage; use ic_http_agent::Signer; +use ic_http_agent::{Blob, Request, RequestId, SignedMessage}; use std::path::PathBuf; pub struct Identity(ic_identity_manager::Identity); @@ -21,16 +19,7 @@ impl Identity { } impl Signer for Identity { - fn sign<'a>( - &self, - request: Box<(dyn erased_serde::Serialize + Send + Sync + 'a)>, - ) -> Result< - ( - RequestId, - Box, - ), - AgentError, - > { + fn sign<'a>(&self, request: Request<'a>) -> Result<(RequestId, SignedMessage<'a>), AgentError> { let request_id = to_request_id(&request).map_err(AgentError::from)?; let signature_tuple = self .0 @@ -43,32 +32,35 @@ impl Signer for Identity { signature, sender_pubkey, }; - Ok((request_id, Box::new(signed_request))) + Ok((request_id, signed_request)) } } #[cfg(test)] mod test { use super::*; + use ic_http_agent::{Blob, CanisterId, ReadRequest}; use proptest::prelude::*; - use serde::Serialize; use tempfile::tempdir; - // Dummy proptest checking request id is correct for now. proptest! { #[test] - fn request_id_identity(request: String) { + fn request_id_identity(request_body: String) { let dir = tempdir().unwrap(); + let arg = Blob::from(vec![4; 32]); + let canister_id = CanisterId::from(Blob::from(vec![4; 32])); + let request = ReadRequest::Query { + arg: &arg, + canister_id: &canister_id, + method_name: &request_body, + }; - #[derive(Clone,Serialize)] - struct TestAPI { inner : String} - let request = TestAPI { inner: request}; - + let request = Request::Query(request.clone()); let request_with_sender = request.clone(); - let actual_request_id = to_request_id(&request_with_sender).expect("Failed to produce request id"); + let actual_request_id = to_request_id(&request).expect("Failed to produce request id"); let signer = Identity::new(dir.into_path()); - let request_id = signer.sign(Box::new(request)).expect("Failed to sign").0; + let request_id = signer.sign(request_with_sender).expect("Failed to sign").0; assert_eq!(request_id, actual_request_id) }} } diff --git a/src/ic_http_agent/src/agent/mod.rs b/src/ic_http_agent/src/agent/mod.rs index 4e55101362..9a02ad554a 100644 --- a/src/ic_http_agent/src/agent/mod.rs +++ b/src/ic_http_agent/src/agent/mod.rs @@ -9,7 +9,7 @@ pub(crate) mod public { pub use super::agent_config::AgentConfig; pub use super::agent_error::AgentError; pub use super::nonce::NonceFactory; - pub use super::replica_api::{MessageWithSender, SignedMessage}; + pub use super::replica_api::{MessageWithSender, SignedMessage, Request, ReadRequest}; pub use super::response::RequestStatusResponse; pub use super::signer::Signer; pub use super::waiter::{Waiter, WaiterTrait}; diff --git a/src/ic_http_agent/src/agent/signer.rs b/src/ic_http_agent/src/agent/signer.rs index 4ba5956785..4581e99fc9 100644 --- a/src/ic_http_agent/src/agent/signer.rs +++ b/src/ic_http_agent/src/agent/signer.rs @@ -50,20 +50,16 @@ impl Signer for DummyIdentity { #[cfg(test)] mod test { use super::*; - use crate::agent::replica_api::{ReadRequest, Request}; use crate::CanisterId; use proptest::prelude::*; - use serde::Serialize; // TODO(eftychis): Provide arbitrary strategies for the replica // API. proptest! { #[test] fn request_id_dummy_signer(request_body: String) { - #[derive(Clone,Serialize)] - struct TestAPI { inner : String} let arg = Blob::random(10); let canister_id = CanisterId::from(Blob::random(10)); let request = ReadRequest::Query { From 6dc5aafd0b2a1afe5f67073c9118bf19def0e48e Mon Sep 17 00:00:00 2001 From: eftychis Date: Thu, 26 Mar 2020 22:34:11 -0700 Subject: [PATCH 4/5] Fix fmt --- src/ic_http_agent/src/agent/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ic_http_agent/src/agent/mod.rs b/src/ic_http_agent/src/agent/mod.rs index 9a02ad554a..85f94e4489 100644 --- a/src/ic_http_agent/src/agent/mod.rs +++ b/src/ic_http_agent/src/agent/mod.rs @@ -9,7 +9,7 @@ pub(crate) mod public { pub use super::agent_config::AgentConfig; pub use super::agent_error::AgentError; pub use super::nonce::NonceFactory; - pub use super::replica_api::{MessageWithSender, SignedMessage, Request, ReadRequest}; + pub use super::replica_api::{MessageWithSender, ReadRequest, Request, SignedMessage}; pub use super::response::RequestStatusResponse; pub use super::signer::Signer; pub use super::waiter::{Waiter, WaiterTrait}; From be43d9234364063d551229734d17359330b3c5ff Mon Sep 17 00:00:00 2001 From: eftychis Date: Fri, 27 Mar 2020 11:09:14 -0700 Subject: [PATCH 5/5] rm comment --- src/ic_http_agent/src/agent/signer.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/ic_http_agent/src/agent/signer.rs b/src/ic_http_agent/src/agent/signer.rs index 4581e99fc9..40481c2ed2 100644 --- a/src/ic_http_agent/src/agent/signer.rs +++ b/src/ic_http_agent/src/agent/signer.rs @@ -29,10 +29,6 @@ impl Signer for DummyIdentity { // the request id. Trying to figure out if the correct // behaviour changed and where the deviation happens. - // let mut sender = vec![0; 32]; - // sender.push(0x02); - // let sender = Blob::from(sender); - // let request_with_sender = MessageWithSender { request, sender }; let request_with_sender = request; let request_id = to_request_id(&request_with_sender).map_err(AgentError::from)?;