refactor: Use strongly typed fields in JSON requests and responses (#291

) Ensures that all fields in JSON requests/responses that are serialized/deserialized by the EVM-RPC canister are strongly typed, e.g. use a wrapper around`[u8; 32]` instead of a `String` for fields that represent hashes. This ensures that the EVM-RPC canister only uses with syntactically valid data.
dfinity · Sep 26, 2024 · 9a94dba · 9a94dba
1 parent f32960e
commit 9a94dba
Show file tree

Hide file tree

Showing 9 changed files with 287 additions and 228 deletions.
diff --git a/evm_rpc_types/src/lib.rs b/evm_rpc_types/src/lib.rs
@@ -14,7 +14,7 @@ use serde::{Deserialize, Serialize};
 use std::fmt::Formatter;
 use std::str::FromStr;
 
-pub use request::{FeeHistoryArgs, GetLogsArgs, GetTransactionCountArgs};
+pub use request::{BlockTag, FeeHistoryArgs, GetLogsArgs, GetTransactionCountArgs};
 pub use response::{Block, FeeHistory, LogEntry, SendRawTransactionStatus, TransactionReceipt};
 pub use result::{
     HttpOutcallError, JsonRpcError, MultiRpcResult, ProviderError, RpcError, RpcResult,
@@ -25,17 +25,6 @@ pub use rpc_client::{
     RpcConfig, RpcService, RpcServices,
 };
 
-#[derive(Clone, Debug, PartialEq, Eq, CandidType, Deserialize, Default)]
-pub enum BlockTag {
-    #[default]
-    Latest,
-    Finalized,
-    Safe,
-    Earliest,
-    Pending,
-    Number(Nat256),
-}
-
 /// A `Nat` that is guaranteed to fit in 256 bits.
 #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(try_from = "candid::Nat", into = "candid::Nat")]
@@ -195,7 +184,13 @@ impl_hex_string!(Hex(Vec<u8>));
 /// `FromHex::from_hex` will return `Err(FromHexError::OddLength)`
 /// when trying to decode such strings.
 #[derive(Clone, Debug, PartialEq, Eq)]
-struct Byte([u8; 1]);
+pub struct Byte([u8; 1]);
+
+impl Byte {
+    pub fn into_byte(self) -> u8 {
+        self.0[0]
+    }
+}
 
 impl AsRef<[u8]> for Byte {
     fn as_ref(&self) -> &[u8] {

diff --git a/evm_rpc_types/src/request/mod.rs b/evm_rpc_types/src/request/mod.rs
@@ -1,7 +1,18 @@
-use crate::{BlockTag, Hex20, Hex32, Nat256};
+use crate::{Hex20, Hex32, Nat256};
 use candid::CandidType;
 use serde::Deserialize;
 
+#[derive(Clone, Debug, PartialEq, Eq, CandidType, Deserialize, Default)]
+pub enum BlockTag {
+    #[default]
+    Latest,
+    Finalized,
+    Safe,
+    Earliest,
+    Pending,
+    Number(Nat256),
+}
+
 #[derive(Clone, Debug, PartialEq, Eq, CandidType, Deserialize)]
 pub struct FeeHistoryArgs {
     /// Number of blocks in the requested range.

diff --git a/src/candid_rpc/cketh_conversion.rs b/src/candid_rpc/cketh_conversion.rs
@@ -1,10 +1,9 @@
-//! Conversion between ckETH types and EVM RPC types.
-//! This module is meant to be temporary and should be removed once the dependency on ckETH is removed,
-//! see <https://github.com/internet-computer-protocol/evm-rpc-canister/issues/243>
+//! Conversion between JSON types and Candid EVM RPC types.
 
 use crate::rpc_client::json::requests::BlockSpec;
 use crate::rpc_client::json::Hash;
-use evm_rpc_types::{BlockTag, Hex, Hex20, Hex256, Hex32, HexByte, Nat256};
+use evm_rpc_types::BlockTag;
+use evm_rpc_types::{Hex, Hex256, Hex32, HexByte, Nat256};
 
 pub(super) fn into_block_spec(value: BlockTag) -> BlockSpec {
     use crate::rpc_client::json::requests;
@@ -36,7 +35,7 @@ pub(super) fn into_get_logs_param(
             .map(|topic| {
                 topic
                     .into_iter()
-                    .map(|t| crate::rpc_client::json::FixedSizeData(t.into()))
+                    .map(|t| crate::rpc_client::json::FixedSizeData::new(t.into()))
                     .collect()
             })
             .collect(),
@@ -52,11 +51,15 @@ pub(super) fn from_log_entries(
 fn from_log_entry(value: crate::rpc_client::json::responses::LogEntry) -> evm_rpc_types::LogEntry {
     evm_rpc_types::LogEntry {
         address: from_address(value.address),
-        topics: value.topics.into_iter().map(|t| t.0.into()).collect(),
+        topics: value
+            .topics
+            .into_iter()
+            .map(|t| t.into_bytes().into())
+            .collect(),
         data: value.data.0.into(),
-        block_hash: value.block_hash.map(|x| x.0.into()),
+        block_hash: value.block_hash.map(|x| x.into_bytes().into()),
         block_number: value.block_number.map(Nat256::from),
-        transaction_hash: value.transaction_hash.map(|x| x.0.into()),
+        transaction_hash: value.transaction_hash.map(|x| x.into_bytes().into()),
         transaction_index: value.transaction_index.map(Nat256::from),
         log_index: value.log_index.map(Nat256::from),
         removed: value.removed,
@@ -105,26 +108,22 @@ pub(super) fn from_transaction_receipt(
     value: crate::rpc_client::json::responses::TransactionReceipt,
 ) -> evm_rpc_types::TransactionReceipt {
     evm_rpc_types::TransactionReceipt {
-        block_hash: Hex32::from(value.block_hash.0),
+        block_hash: Hex32::from(value.block_hash.into_bytes()),
         block_number: value.block_number.into(),
         effective_gas_price: value.effective_gas_price.into(),
         gas_used: value.gas_used.into(),
         status: value.status.map(|v| match v {
             crate::rpc_client::json::responses::TransactionStatus::Success => Nat256::from(1_u8),
             crate::rpc_client::json::responses::TransactionStatus::Failure => Nat256::from(0_u8),
         }),
-        transaction_hash: Hex32::from(value.transaction_hash.0),
-        // TODO 243: responses types from querying JSON-RPC providers should be strongly typed
-        // for all the following fields: contract_address, from, logs_bloom, to, transaction_index, tx_type
-        contract_address: value
-            .contract_address
-            .map(|address| Hex20::try_from(address).unwrap()),
-        from: Hex20::try_from(value.from).unwrap(),
+        transaction_hash: Hex32::from(value.transaction_hash.into_bytes()),
+        contract_address: value.contract_address.map(from_address),
+        from: from_address(value.from),
         logs: from_log_entries(value.logs),
-        logs_bloom: Hex256::try_from(value.logs_bloom).unwrap(),
-        to: value.to.map(|v| Hex20::try_from(v).unwrap()),
+        logs_bloom: Hex256::from(value.logs_bloom.into_bytes()),
+        to: value.to.map(from_address),
         transaction_index: value.transaction_index.into(),
-        tx_type: HexByte::try_from(value.r#type).unwrap(),
+        tx_type: HexByte::from(value.tx_type.into_byte()),
     }
 }
 
@@ -133,31 +132,31 @@ pub(super) fn from_block(value: crate::rpc_client::json::responses::Block) -> ev
         base_fee_per_gas: value.base_fee_per_gas.map(Nat256::from),
         number: value.number.into(),
         difficulty: value.difficulty.map(Nat256::from),
-        extra_data: Hex::try_from(value.extra_data).unwrap(),
+        extra_data: Hex::from(value.extra_data.0),
         gas_limit: value.gas_limit.into(),
         gas_used: value.gas_used.into(),
-        hash: Hex32::try_from(value.hash).unwrap(),
-        logs_bloom: Hex256::try_from(value.logs_bloom).unwrap(),
-        miner: Hex20::try_from(value.miner).unwrap(),
-        mix_hash: Hex32::try_from(value.mix_hash).unwrap(),
+        hash: Hex32::from(value.hash.into_bytes()),
+        logs_bloom: Hex256::from(value.logs_bloom.into_bytes()),
+        miner: from_address(value.miner),
+        mix_hash: Hex32::from(value.mix_hash.into_bytes()),
         nonce: value.nonce.into(),
-        parent_hash: Hex32::try_from(value.parent_hash).unwrap(),
-        receipts_root: Hex32::try_from(value.receipts_root).unwrap(),
-        sha3_uncles: Hex32::try_from(value.sha3_uncles).unwrap(),
+        parent_hash: Hex32::from(value.parent_hash.into_bytes()),
+        receipts_root: Hex32::from(value.receipts_root.into_bytes()),
+        sha3_uncles: Hex32::from(value.sha3_uncles.into_bytes()),
         size: value.size.into(),
-        state_root: Hex32::try_from(value.state_root).unwrap(),
+        state_root: Hex32::from(value.state_root.into_bytes()),
         timestamp: value.timestamp.into(),
         total_difficulty: value.total_difficulty.map(Nat256::from),
         transactions: value
             .transactions
             .into_iter()
-            .map(|tx| Hex32::try_from(tx).unwrap())
+            .map(|tx| Hex32::from(tx.into_bytes()))
             .collect(),
-        transactions_root: value.transactions_root.map(|x| Hex32::try_from(x).unwrap()),
+        transactions_root: value.transactions_root.map(|x| Hex32::from(x.into_bytes())),
         uncles: value
             .uncles
             .into_iter()
-            .map(|tx| Hex32::try_from(tx).unwrap())
+            .map(|tx| Hex32::from(tx.into_bytes()))
             .collect(),
     }
 }
@@ -183,7 +182,7 @@ pub(super) fn from_send_raw_transaction_result(
 }
 
 pub(super) fn into_hash(value: Hex32) -> Hash {
-    Hash(value.into())
+    Hash::new(value.into())
 }
 
 fn from_address(value: ic_ethereum_types::Address) -> evm_rpc_types::Hex20 {

diff --git a/src/candid_rpc.rs → src/candid_rpc/mod.rs b/src/candid_rpc.rs → src/candid_rpc/mod.rs
@@ -1,4 +1,6 @@
 mod cketh_conversion;
+#[cfg(test)]
+mod tests;
 
 use crate::rpc_client::{EthRpcClient, MultiCallError};
 use crate::{
@@ -42,6 +44,7 @@ fn process_result<T>(method: RpcMethod, result: Result<T, MultiCallError<T>>) ->
     }
 }
 
+/// Adapt the `EthRpcClient` to the `Candid` interface used by the EVM-RPC canister.
 pub struct CandidRpcClient {
     client: EthRpcClient,
 }
@@ -161,78 +164,3 @@ fn get_transaction_hash(raw_signed_transaction_hex: &Hex) -> Option<Hex32> {
     let transaction: Transaction = rlp::decode(raw_signed_transaction_hex.as_ref()).ok()?;
     Some(Hex32::from(transaction.hash.0))
 }
-
-#[cfg(test)]
-mod test {
-    use super::*;
-    use crate::rpc_client::{MultiCallError, MultiCallResults};
-    use evm_rpc_types::{ProviderError, RpcError};
-
-    #[test]
-    fn test_process_result_mapping() {
-        use evm_rpc_types::{EthMainnetService, RpcService};
-
-        let method = RpcMethod::EthGetTransactionCount;
-
-        assert_eq!(
-            process_result(method, Ok(5)),
-            MultiRpcResult::Consistent(Ok(5))
-        );
-        assert_eq!(
-            process_result(
-                method,
-                Err(MultiCallError::<()>::ConsistentError(
-                    RpcError::ProviderError(ProviderError::MissingRequiredProvider)
-                ))
-            ),
-            MultiRpcResult::Consistent(Err(RpcError::ProviderError(
-                ProviderError::MissingRequiredProvider
-            )))
-        );
-        assert_eq!(
-            process_result(
-                method,
-                Err(MultiCallError::<()>::InconsistentResults(
-                    MultiCallResults::default()
-                ))
-            ),
-            MultiRpcResult::Inconsistent(vec![])
-        );
-        assert_eq!(
-            process_result(
-                method,
-                Err(MultiCallError::InconsistentResults(
-                    MultiCallResults::from_non_empty_iter(vec![(
-                        RpcService::EthMainnet(EthMainnetService::Ankr),
-                        Ok(5)
-                    )])
-                ))
-            ),
-            MultiRpcResult::Inconsistent(vec![(
-                RpcService::EthMainnet(EthMainnetService::Ankr),
-                Ok(5)
-            )])
-        );
-        assert_eq!(
-            process_result(
-                method,
-                Err(MultiCallError::InconsistentResults(
-                    MultiCallResults::from_non_empty_iter(vec![
-                        (RpcService::EthMainnet(EthMainnetService::Ankr), Ok(5)),
-                        (
-                            RpcService::EthMainnet(EthMainnetService::Cloudflare),
-                            Err(RpcError::ProviderError(ProviderError::NoPermission))
-                        )
-                    ])
-                ))
-            ),
-            MultiRpcResult::Inconsistent(vec![
-                (RpcService::EthMainnet(EthMainnetService::Ankr), Ok(5)),
-                (
-                    RpcService::EthMainnet(EthMainnetService::Cloudflare),
-                    Err(RpcError::ProviderError(ProviderError::NoPermission))
-                )
-            ])
-        );
-    }
-}
diff --git a/src/candid_rpc/tests.rs b/src/candid_rpc/tests.rs
@@ -0,0 +1,73 @@
+use crate::candid_rpc::process_result;
+use crate::rpc_client::{MultiCallError, MultiCallResults};
+use crate::types::RpcMethod;
+use evm_rpc_types::MultiRpcResult;
+use evm_rpc_types::{ProviderError, RpcError};
+
+#[test]
+fn test_process_result_mapping() {
+    use evm_rpc_types::{EthMainnetService, RpcService};
+
+    let method = RpcMethod::EthGetTransactionCount;
+
+    assert_eq!(
+        process_result(method, Ok(5)),
+        MultiRpcResult::Consistent(Ok(5))
+    );
+    assert_eq!(
+        process_result(
+            method,
+            Err(MultiCallError::<()>::ConsistentError(
+                RpcError::ProviderError(ProviderError::MissingRequiredProvider)
+            ))
+        ),
+        MultiRpcResult::Consistent(Err(RpcError::ProviderError(
+            ProviderError::MissingRequiredProvider
+        )))
+    );
+    assert_eq!(
+        process_result(
+            method,
+            Err(MultiCallError::<()>::InconsistentResults(
+                MultiCallResults::default()
+            ))
+        ),
+        MultiRpcResult::Inconsistent(vec![])
+    );
+    assert_eq!(
+        process_result(
+            method,
+            Err(MultiCallError::InconsistentResults(
+                MultiCallResults::from_non_empty_iter(vec![(
+                    RpcService::EthMainnet(EthMainnetService::Ankr),
+                    Ok(5)
+                )])
+            ))
+        ),
+        MultiRpcResult::Inconsistent(vec![(
+            RpcService::EthMainnet(EthMainnetService::Ankr),
+            Ok(5)
+        )])
+    );
+    assert_eq!(
+        process_result(
+            method,
+            Err(MultiCallError::InconsistentResults(
+                MultiCallResults::from_non_empty_iter(vec![
+                    (RpcService::EthMainnet(EthMainnetService::Ankr), Ok(5)),
+                    (
+                        RpcService::EthMainnet(EthMainnetService::Cloudflare),
+                        Err(RpcError::ProviderError(ProviderError::NoPermission))
+                    )
+                ])
+            ))
+        ),
+        MultiRpcResult::Inconsistent(vec![
+            (RpcService::EthMainnet(EthMainnetService::Ankr), Ok(5)),
+            (
+                RpcService::EthMainnet(EthMainnetService::Cloudflare),
+                Err(RpcError::ProviderError(ProviderError::NoPermission))
+            )
+        ])
+    );
+}