Rename and refactor SignableMsg::signable_bytes (#780)

The previous method name was `sign_bytes`, which as was noted in #273 is
unclear to whether the resulting bytes have an appended signature.

The new name indicates that the message can be signed, but has not yet
been signed.

Additionally, this commit does a bit of refactoring to eliminate some
duplicated code within the method.

Closes #273

src/commands/ledger.rs

@@ -62,9 +62,8 @@ impl Runnable for InitCommand {
         };
         println!("{vote:?}");
         let sign_vote_req = SignableMsg::Vote(vote);
-        let mut to_sign = vec![];
-        sign_vote_req
-            .sign_bytes(config.validator[0].chain_id.clone(), &mut to_sign)
+        let to_sign = sign_vote_req
+            .signable_bytes(config.validator[0].chain_id.clone())
             .unwrap();
 
         let _sig = chain.keyring.sign(None, &to_sign).unwrap();

src/error.rs

@@ -184,6 +184,12 @@ impl From<serde_json::error::Error> for Error {
     }
 }
 
+impl From<signature::Error> for Error {
+    fn from(other: signature::Error) -> Self {
+        ErrorKind::CryptoError.context(other).into()
+    }
+}
+
 impl From<tendermint::Error> for Error {
     fn from(other: tendermint::error::Error) -> Self {
         ErrorKind::TendermintError.context(other).into()

src/keyring/signature.rs

@@ -5,19 +5,31 @@ pub use k256::ecdsa;
 
 /// Cryptographic signature used for block signing
 pub enum Signature {
+    /// ECDSA signature (e.g secp256k1)
+    Ecdsa(ecdsa::Signature),
+
     ///  ED25519 signature
     Ed25519(ed25519::Signature),
-
-    /// ECDSA signagure (e.g secp256k1)
-    Ecdsa(ecdsa::Signature),
 }
 
 impl Signature {
     /// Serialize this signature as a byte vector.
     pub fn to_vec(&self) -> Vec<u8> {
         match self {
-            Self::Ed25519(sig) => sig.to_vec(),
             Self::Ecdsa(sig) => sig.to_vec(),
+            Self::Ed25519(sig) => sig.to_vec(),
         }
     }
 }
+
+impl From<ecdsa::Signature> for Signature {
+    fn from(sig: ecdsa::Signature) -> Signature {
+        Self::Ecdsa(sig)
+    }
+}
+
+impl From<ed25519::Signature> for Signature {
+    fn from(sig: ed25519::Signature) -> Signature {
+        Self::Ed25519(sig)
+    }
+}

src/session.rs

@@ -139,9 +139,7 @@ impl Session {
             return Ok(signable_msg.error(remote_err));
         }
 
-        let mut to_sign = vec![];
-        signable_msg.sign_bytes(self.config.chain_id.clone(), &mut to_sign)?;
-
+        let to_sign = signable_msg.signable_bytes(self.config.chain_id.clone())?;
         let started_at = Instant::now();
 
         // TODO(ismail): figure out which key to use here instead of taking the only key

src/signing.rs

@@ -6,8 +6,9 @@ use crate::{
     prelude::{ensure, fail, format_err},
     rpc::Response,
 };
-use bytes::BufMut;
+use bytes::{Bytes, BytesMut};
 use prost::{EncodeError, Message as _};
+use signature::Signer;
 use tendermint::{block, chain, consensus};
 use tendermint_proto as proto;
 
@@ -98,31 +99,43 @@ impl SignableMsg {
         Ok(())
     }
 
-    /// Get the bytes to be signed for a given message.
-    pub fn sign_bytes<B>(
-        &self,
-        chain_id: chain::Id,
-        sign_bytes: &mut B,
-    ) -> Result<bool, EncodeError>
+    /// Sign the given message, returning a response with the signature appended.
+    pub fn sign<S>(self, chain_id: chain::Id, signer: impl Signer<S>) -> Result<Response, Error>
     where
-        B: BufMut,
+        S: Into<Signature>,
     {
+        let signable_bytes = self.signable_bytes(chain_id)?;
+        let signature = signer.try_sign(&signable_bytes)?;
+        self.add_signature(signature.into())
+    }
+
+    /// Get the bytes representing a canonically encoded message over which a
+    /// signature is to be computed.
+    pub fn signable_bytes(&self, chain_id: chain::Id) -> Result<Bytes, EncodeError> {
+        fn canonicalize_block_id(
+            block_id: &proto::types::BlockId,
+        ) -> Option<proto::types::CanonicalBlockId> {
+            if block_id.hash.is_empty() {
+                return None;
+            }
+
+            Some(proto::types::CanonicalBlockId {
+                hash: block_id.hash.clone(),
+                part_set_header: block_id.part_set_header.as_ref().map(|y| {
+                    proto::types::CanonicalPartSetHeader {
+                        total: y.total,
+                        hash: y.hash.clone(),
+                    }
+                }),
+            })
+        }
+
+        let mut bytes = BytesMut::new();
+
         match self {
             Self::Proposal(proposal) => {
                 let proposal = proposal.clone();
-                let block_id = match proposal.block_id.as_ref() {
-                    Some(x) if x.hash.is_empty() => None,
-                    Some(x) => Some(proto::types::CanonicalBlockId {
-                        hash: x.hash.clone(),
-                        part_set_header: x.part_set_header.as_ref().map(|y| {
-                            proto::types::CanonicalPartSetHeader {
-                                total: y.total,
-                                hash: y.hash.clone(),
-                            }
-                        }),
-                    }),
-                    None => None,
-                };
+                let block_id = proposal.block_id.as_ref().and_then(canonicalize_block_id);
 
                 let cp = proto::types::CanonicalProposal {
                     chain_id: chain_id.to_string(),
@@ -134,24 +147,11 @@ impl SignableMsg {
                     timestamp: proposal.timestamp.map(Into::into),
                 };
 
-                cp.encode_length_delimited(sign_bytes).unwrap();
-                Ok(true)
+                cp.encode_length_delimited(&mut bytes)?;
             }
             Self::Vote(vote) => {
                 let vote = vote.clone();
-                let block_id = match vote.block_id.as_ref() {
-                    Some(x) if x.hash.is_empty() => None,
-                    Some(x) => Some(proto::types::CanonicalBlockId {
-                        hash: x.hash.clone(),
-                        part_set_header: x.part_set_header.as_ref().map(|y| {
-                            proto::types::CanonicalPartSetHeader {
-                                total: y.total,
-                                hash: y.hash.clone(),
-                            }
-                        }),
-                    }),
-                    None => None,
-                };
+                let block_id = vote.block_id.as_ref().and_then(canonicalize_block_id);
 
                 let cv = proto::types::CanonicalVote {
                     r#type: vote.r#type,
@@ -161,10 +161,11 @@ impl SignableMsg {
                     timestamp: vote.timestamp.map(Into::into),
                     chain_id: chain_id.to_string(),
                 };
-                cv.encode_length_delimited(sign_bytes).unwrap();
-                Ok(true)
+                cv.encode_length_delimited(&mut bytes)?;
             }
         }
+
+        Ok(bytes.into())
     }
 
     /// Add a signature to this request, returning a response.

tests/integration.rs

@@ -365,28 +365,27 @@ fn handle_and_sign_proposal(key_type: KeyType) {
             other => panic!("unexpected message type in response: {other:?}"),
         };
 
-        let mut sign_bytes: Vec<u8> = vec![];
-        signable_msg
-            .sign_bytes(chain_id.parse().unwrap(), &mut sign_bytes)
+        let signable_bytes = signable_msg
+            .signable_bytes(chain_id.parse().unwrap())
             .unwrap();
 
         let prop = response
             .proposal
             .expect("proposal should be embedded but none was found");
 
-        let msg: &[u8] = sign_bytes.as_slice();
-
         let r = match key_type {
             KeyType::Account => {
                 let signature =
                     k256::ecdsa::Signature::try_from(prop.signature.as_slice()).unwrap();
-                test_secp256k1_keypair().1.verify(msg, &signature)
+                test_secp256k1_keypair()
+                    .1
+                    .verify(&signable_bytes, &signature)
             }
             KeyType::Consensus => {
                 let signature = ed25519::Signature::try_from(prop.signature.as_slice()).unwrap();
                 test_ed25519_keypair()
                     .verifying_key()
-                    .verify(msg, &signature)
+                    .verify(&signable_bytes, &signature)
             }
         };
         assert!(r.is_ok());
@@ -449,9 +448,8 @@ fn handle_and_sign_vote(key_type: KeyType) {
             other => panic!("unexpected message type in response: {other:?}"),
         };
 
-        let mut sign_bytes: Vec<u8> = vec![];
-        signable_msg
-            .sign_bytes(chain_id.parse().unwrap(), &mut sign_bytes)
+        let signable_bytes = signable_msg
+            .signable_bytes(chain_id.parse().unwrap())
             .unwrap();
 
         let vote_msg: proto::types::Vote = request
@@ -461,18 +459,18 @@ fn handle_and_sign_vote(key_type: KeyType) {
         let sig: Vec<u8> = vote_msg.signature;
         assert_ne!(sig.len(), 0);
 
-        let msg: &[u8] = sign_bytes.as_slice();
-
         let r = match key_type {
             KeyType::Account => {
                 let signature = k256::ecdsa::Signature::try_from(sig.as_slice()).unwrap();
-                test_secp256k1_keypair().1.verify(msg, &signature)
+                test_secp256k1_keypair()
+                    .1
+                    .verify(&signable_bytes, &signature)
             }
             KeyType::Consensus => {
                 let signature = ed25519::Signature::try_from(sig.as_slice()).unwrap();
                 test_ed25519_keypair()
                     .verifying_key()
-                    .verify(msg, &signature)
+                    .verify(&signable_bytes, &signature)
             }
         };
         assert!(r.is_ok());
@@ -537,9 +535,8 @@ fn exceed_max_height(key_type: KeyType) {
             other => panic!("unexpected message type in response: {other:?}"),
         };
 
-        let mut sign_bytes: Vec<u8> = vec![];
-        signable_msg
-            .sign_bytes(chain_id.parse().unwrap(), &mut sign_bytes)
+        let signable_bytes = signable_msg
+            .signable_bytes(chain_id.parse().unwrap())
             .unwrap();
 
         let vote_msg = response
@@ -549,18 +546,18 @@ fn exceed_max_height(key_type: KeyType) {
         let sig: Vec<u8> = vote_msg.signature;
         assert_ne!(sig.len(), 0);
 
-        let msg: &[u8] = sign_bytes.as_slice();
-
         let r = match key_type {
             KeyType::Account => {
                 let signature = k256::ecdsa::Signature::try_from(sig.as_slice()).unwrap();
-                test_secp256k1_keypair().1.verify(msg, &signature)
+                test_secp256k1_keypair()
+                    .1
+                    .verify(&signable_bytes, &signature)
             }
             KeyType::Consensus => {
                 let signature = ed25519::Signature::try_from(sig.as_slice()).unwrap();
                 test_ed25519_keypair()
                     .verifying_key()
-                    .verify(msg, &signature)
+                    .verify(&signable_bytes, &signature)
             }
         };
         assert!(r.is_ok());