refactor: Remove template in favor of an enum for signer (#454)

* 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).

* Update documentation

* Update

* Fix fmt

* rm comment

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<dyn erased_serde::Serialize + Send + Sync + 'a>,
-        ),
-        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)
     }}
 }

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"

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, ReadRequest, Request, SignedMessage};
     pub use super::response::RequestStatusResponse;
     pub use super::signer::Signer;
     pub use super::waiter::{Waiter, WaiterTrait};
@@ -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<RequestId, AgentError> {
         // We need to calculate the signature, and thus also the
         // request id initially.
-        let request: Box<SubmitRequest<'_>> = 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)?;

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<A> {
 #[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<T: Serialize> {
@@ -67,9 +74,9 @@ pub struct MessageWithSender<T: Serialize> {
 
 #[derive(Debug, Clone, Serialize)]
 #[serde(rename_all = "snake_case")]
-pub struct SignedMessage<T: Serialize> {
+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,

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,54 +16,19 @@ 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<dyn erased_serde::Serialize + Send + Sync + 'a>,
-        ),
-        AgentError,
-    >;
+    fn sign<'a>(&self, request: Request<'a>) -> Result<(RequestId, SignedMessage<'a>), AgentError>;
 }
 
 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: Box<(dyn erased_serde::Serialize + Send + Sync + 'a)>,
-    ) -> Result<
-        (
-            RequestId,
-            Box<dyn erased_serde::Serialize + Send + Sync + 'a>,
-        ),
-        AgentError,
-    > {
-        let mut sender = vec![0; 32];
-        sender.push(0x02);
+    fn sign<'a>(&self, request: Request<'a>) -> Result<(RequestId, SignedMessage<'a>), AgentError> {
         // 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 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)?;
 
@@ -74,29 +39,37 @@ 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;
 
     // TODO(eftychis): Provide arbitrary strategies for the replica
     // API.
     proptest! {
     #[test]
-    fn request_id_dummy_signer(request: String) {
-        #[derive(Clone,Serialize)]
-        struct TestAPI { inner : String}
-        let request = TestAPI { inner: request};
+    fn request_id_dummy_signer(request_body: String) {
+        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)
     }}
 }