review feedback

contracts/multisig-prover/src/encoding/abi.rs

@@ -20,7 +20,7 @@ use ethabi::ethereum_types;
 
 pub const GATEWAY_EXECUTE_FUNCTION_NAME: &str = "execute";
 
-pub fn encode(data: &Data) -> Result<HexBinary, ContractError> {
+pub fn encode(data: &Data) -> HexBinary {
     let destination_chain_id = Token::Uint(ethabi::ethereum_types::U256::from_big_endian(
         &data.destination_chain_id.to_be_bytes(),
     ));
@@ -37,17 +37,17 @@ pub fn encode(data: &Data) -> Result<HexBinary, ContractError> {
         })
         .multiunzip();
 
-    Ok(ethabi::encode(&[
+    ethabi::encode(&[
         destination_chain_id,
         Token::Array(commands_ids),
         Token::Array(commands_types),
         Token::Array(commands_params),
     ])
-    .into())
+    .into()
 }
 
-pub fn msg_to_sign(command_batch: &CommandBatch) -> Result<HexBinary, ContractError> {
-    let msg = Keccak256::digest(encode(&command_batch.data)?.as_slice());
+pub fn msg_digest(command_batch: &CommandBatch) -> HexBinary {
+    let msg = Keccak256::digest(encode(&command_batch.data).as_slice());
 
     // Prefix for standard EVM signed data https://eips.ethereum.org/EIPS/eip-191
     let unsigned = [
@@ -56,7 +56,7 @@ pub fn msg_to_sign(command_batch: &CommandBatch) -> Result<HexBinary, ContractEr
     ]
     .concat();
 
-    Ok(Keccak256::digest(unsigned).as_slice().into())
+    Keccak256::digest(unsigned).as_slice().into()
 }
 
 pub fn encode_execute_data(
@@ -65,7 +65,7 @@ pub fn encode_execute_data(
     signers: Vec<(Signer, Option<Signature>)>,
 ) -> Result<HexBinary, ContractError> {
     let param = ethabi::encode(&[
-        Token::Bytes(encode(&command_batch.data)?.into()),
+        Token::Bytes(encode(&command_batch.data).into()),
         Token::Bytes(encode_proof(quorum, signers)?.into()),
     ]);
 
@@ -589,8 +589,7 @@ mod test {
         let data = decode_data(&encoded_data);
         let res = data.encode(Encoder::Abi);
 
-        assert!(res.is_ok());
-        assert_eq!(res.unwrap(), encoded_data);
+        assert_eq!(res, encoded_data);
     }
 
     #[test]
@@ -613,11 +612,10 @@ mod test {
             encoder: Encoder::Abi,
         };
 
-        let res = batch.msg_to_sign();
+        let res = batch.msg_digest();
         let expected_msg = test_data::msg_to_sign();
 
-        assert!(res.is_ok());
-        assert_eq!(res.unwrap(), expected_msg);
+        assert_eq!(res, expected_msg);
     }
 
     #[test]

contracts/multisig-prover/src/encoding/bcs.rs

@@ -1,6 +1,6 @@
 use bcs::to_bytes;
-use cosmwasm_std::HexBinary;
-use itertools::Itertools;
+use cosmwasm_std::{HexBinary, Uint256};
+use itertools::{chain, Itertools};
 
 use crate::error::ContractError;
 
@@ -12,29 +12,48 @@ pub fn command_params(
     destination_address: String,
     payload_hash: HexBinary,
 ) -> Result<HexBinary, ContractError> {
+    assert!(
+        destination_address.len() == 64,
+        "destination_address ({}) is not 32 bytes",
+        destination_address
+    );
     let ret = to_bytes(&(
         source_chain,
         source_address,
         <[u8; 32]>::try_from(&HexBinary::from_hex(&destination_address)?.to_vec()[..32])
-            .expect("couldn't convert destination_address to 32 byte array"), // TODO: is this right? Why are addresses 32 bytes?
+            .expect("couldn't convert destination_address to 32 byte array"),
         payload_hash.to_vec(),
     ))?;
 
     Ok(ret.into())
 }
 
-pub fn encode(data: &Data) -> Result<HexBinary, ContractError> {
-    // destination chain id is u64
-    let destination_chain_id = &u64::from_le_bytes(
-        data.destination_chain_id.to_le_bytes()[..8]
+// destination chain id must be u64 for sui
+fn chain_id_as_u64(chain_id: Uint256) -> u64 {
+    assert!(
+        chain_id <= Uint256::from(u64::MAX),
+        "chain_id ({}) is greater than u64 max",
+        chain_id
+    );
+    u64::from_le_bytes(
+        chain_id.to_le_bytes()[..8]
             .try_into()
             .expect("Couldn't convert u256 to u64"),
-    );
+    )
+}
+
+pub fn encode(data: &Data) -> HexBinary {
+    let destination_chain_id = chain_id_as_u64(data.destination_chain_id);
 
     let (commands_ids, command_types, command_params): (Vec<[u8; 32]>, Vec<String>, Vec<Vec<u8>>) =
         data.commands
             .iter()
             .map(|command| {
+                assert!(
+                    command.id.len() == 32,
+                    "command.id ({}) is not 32 bytes",
+                    command.id
+                );
                 (
                     <[u8; 32]>::try_from(&command.id.to_vec()[..32])
                         .expect("couldn't convert command id to 32 byte array"), // command-ids are fixed length sequences
@@ -44,13 +63,14 @@ pub fn encode(data: &Data) -> Result<HexBinary, ContractError> {
             })
             .multiunzip();
 
-    Ok(to_bytes(&(
+    to_bytes(&(
         destination_chain_id,
         commands_ids,
         command_types,
         command_params,
-    ))?
-    .into())
+    ))
+    .expect("couldn't encode batch as bcs")
+    .into()
 }
 
 #[cfg(test)]
@@ -59,16 +79,28 @@ mod test {
     use std::vec;
 
     use bcs::from_bytes;
-    use cosmwasm_std::HexBinary;
+    use cosmwasm_std::{HexBinary, Uint256};
 
     use crate::{
         encoding::{
-            bcs::{command_params, encode},
+            bcs::{chain_id_as_u64, command_params, encode},
             Data,
         },
         types::Command,
     };
 
+    #[test]
+    fn test_chain_id_as_u64() {
+        let chain_id = 1u64;
+        assert_eq!(chain_id, chain_id_as_u64(Uint256::from(chain_id as u128)));
+    }
+    #[test]
+    #[should_panic]
+    fn test_chain_id_as_u64_fails() {
+        let chain_id = u128::MAX;
+        chain_id_as_u64(Uint256::from(chain_id));
+    }
+
     #[test]
     fn test_command_params() {
         let res = command_params(
@@ -100,6 +132,17 @@ mod test {
         assert_eq!(payload_hash, vec![2]);
     }
 
+    #[test]
+    #[should_panic]
+    fn test_invalid_destination_address() {
+        let _ = command_params(
+            "Ethereum".into(),
+            "00".into(),
+            "01".into(),
+            HexBinary::from_hex("02").unwrap(),
+        );
+    }
+
     #[test]
     fn test_encode() {
         let source_chain = "Ethereum";
@@ -122,9 +165,7 @@ mod test {
                 .unwrap(),
             }],
         };
-        let res = encode(&data);
-        assert!(res.is_ok());
-        let encoded = res.unwrap();
+        let encoded = encode(&data);
         let decoded: Result<(u64, Vec<[u8; 32]>, Vec<String>, Vec<Vec<u8>>), _> =
             from_bytes(&encoded.to_vec());
         assert!(decoded.is_ok());

contracts/multisig-prover/src/encoding/mod.rs

@@ -110,9 +110,9 @@ impl CommandBatchBuilder {
 }
 
 impl CommandBatch {
-    pub fn msg_to_sign(&self) -> Result<HexBinary, ContractError> {
+    pub fn msg_digest(&self) -> HexBinary {
         match self.encoder {
-            Encoder::Abi => abi::msg_to_sign(self),
+            Encoder::Abi => abi::msg_digest(self),
             Encoder::Bcs => todo!(),
         }
     }
@@ -136,7 +136,7 @@ pub struct Data {
 }
 
 impl Data {
-    pub fn encode(&self, encoder: Encoder) -> Result<HexBinary, ContractError> {
+    pub fn encode(&self, encoder: Encoder) -> HexBinary {
         match encoder {
             Encoder::Abi => abi::encode(self),
             Encoder::Bcs => bcs::encode(self),

contracts/multisig-prover/src/execute.rs

@@ -49,7 +49,7 @@ pub fn construct_proof(deps: DepsMut, message_ids: Vec<String>) -> Result<Respon
 
     let start_sig_msg = multisig::msg::ExecuteMsg::StartSigningSession {
         key_id,
-        msg: command_batch.msg_to_sign()?,
+        msg: command_batch.msg_digest(),
     };
 
     let wasm_msg = wasm_execute(config.multisig, &start_sig_msg, vec![])?;
@@ -203,7 +203,7 @@ pub fn update_worker_set(deps: DepsMut, env: Env) -> Result<Response, ContractEr
 
             let start_sig_msg = multisig::msg::ExecuteMsg::StartSigningSession {
                 key_id: cur_worker_set.id(), // TODO remove the key_id
-                msg: batch.msg_to_sign()?,
+                msg: batch.msg_digest(),
             };
 
             Ok(Response::new().add_submessage(SubMsg::reply_on_success(