WEB3-124: chore: address TODOs

examples/erc20-counter/methods/guest/src/bin/balance_of.rs

@@ -55,7 +55,10 @@ fn main() {
 
     // Execute the view call; it returns the result in the type generated by the `sol!` macro.
     let call = IERC20::balanceOfCall { account };
-    let returns = Contract::new(contract, &env).call_builder(&call).call();
+    let returns = Contract::new(contract, &env)
+        .call_builder(&call)
+        .call()
+        .unwrap();
 
     // Check that the given account holds at least 1 token.
     assert!(returns._0 >= U256::from(1));

examples/erc20/README.md

@@ -70,7 +70,7 @@ fn main() {
 
     // Execute the view call; it returns the result in the type generated by the `sol!` macro.
     let contract = Contract::new(CONTRACT, &env);
-    let returns = contract.call_builder(&CALL).from(CALLER).call();
+    let returns = contract.call_builder(&CALL).from(CALLER).call().unwrap();
     println!("View call result: {}", returns._0);
 }
 ```

examples/erc20/methods/guest/src/main.rs

@@ -57,6 +57,6 @@ fn main() {
 
     // Execute the view call; it returns the result in the type generated by the `sol!` macro.
     let contract = Contract::new(CONTRACT, &env);
-    let returns = contract.call_builder(&CALL).from(CALLER).call();
+    let returns = contract.call_builder(&CALL).from(CALLER).call().unwrap();
     println!("View call result: {}", returns._0);
 }

examples/token-stats/methods/guest/src/main.rs

@@ -36,10 +36,12 @@ fn main() {
     let utilization = contract
         .call_builder(&CometMainInterface::getUtilizationCall {})
         .call()
+        .unwrap()
         ._0;
     let supply_rate = contract
         .call_builder(&CometMainInterface::getSupplyRateCall { utilization })
         .call()
+        .unwrap()
         ._0;
 
     // The formula for APR in percentage is the following:

steel/CHANGELOG.md

@@ -12,6 +12,8 @@ All notable changes to this project will be documented in this file.
 
 ### 🚨 Breaking Changes
 
+- `CallBuilder::call` in the guest now returns an error that needs to be handled, before it just panicked.
+
 ## [0.13.0](https://github.com/risc0/risc0-ethereum/releases/tag/steel-v0.13.0) - 2024-09-10
 
 ### ⚡️ Features

steel/src/beacon.rs

@@ -273,7 +273,7 @@ mod host {
     /// Returns the header, with `parent_root` equal to `parent.root`.
     ///
     /// It iteratively tries to fetch headers of successive slots until success.
-    /// TODO: use `eth/v1/beacon/headers?parent_root`, once all the nodes support it.
+    // TODO(#242): use `eth/v1/beacon/headers?parent_root` when more clients support it.
     async fn get_child_beacon_header(
         client: &BeaconClient,
         parent: GetBlockHeaderResponse,
@@ -297,6 +297,7 @@ mod host {
             }
         }
         // return the last error, if all calls failed
+        // safe unwrap: there must have been at least one error when we reach this line
         let err = anyhow::Error::from(request_error.unwrap());
         Err(err.context("no valid response received for the 32 consecutive slots"))
     }

steel/src/block.rs

@@ -28,7 +28,7 @@ pub struct BlockInput<H> {
 }
 
 impl<H: EvmBlockHeader> BlockInput<H> {
-    /// Converts the input into a [EvmEnv] for a verifiable state access in the guest.
+    /// Converts the input into a [EvmEnv] for verifiable state access in the guest.
     pub fn into_env(self) -> GuestEvmEnv<H> {
         // verify that the state root matches the state trie
         let state_root = self.state_trie.hash_slow();
@@ -55,7 +55,6 @@ impl<H: EvmBlockHeader> BlockInput<H> {
             previous_header = ancestor;
         }
 
-        // TODO(victor): When do we check that the storage tries are ok?
         let db = StateDb::new(
             self.state_trie,
             self.storage_tries,
@@ -95,6 +94,7 @@ pub mod host {
             H: EvmBlockHeader + TryFrom<<N as Network>::HeaderResponse>,
             <H as TryFrom<<N as Network>::HeaderResponse>>::Error: Display,
         {
+            // safe unwrap: env is never returned without a DB
             let mut db = env.db.unwrap();
             assert_eq!(
                 db.inner().block_hash(),

steel/src/contract.rs

@@ -67,7 +67,7 @@ use revm::{
 /// // Guest:
 /// let evm_env = evm_input.into_env();
 /// let contract = Contract::new(contract_address, &evm_env);
-/// contract.call_builder(&get_balance).call();
+/// contract.call_builder(&get_balance).call().unwrap();
 ///
 /// # Ok(())
 /// # }
@@ -201,6 +201,7 @@ mod host {
         /// list. This does *not* set the access list as part of the transaction (as specified in
         /// EIP-2930), and thus can only be specified during preflight on the host.
         pub async fn prefetch_access_list(self, access_list: AccessList) -> Result<Self> {
+            // safe unwrap: env is never returned without a DB
             let db = self.env.db.as_mut().unwrap();
             db.add_access_list(access_list).await?;
 
@@ -219,11 +220,11 @@ mod host {
                 self.tx.to
             );
 
-            let cfg = self.env.cfg_env.clone();
-            let header = self.env.header.inner().clone();
-            // we cannot clone the database, so it gets moved in and out of the task
+            // as mutable references are not possible, the DB must be moved in and out of the task
             let db = self.env.db.take().unwrap();
 
+            let cfg = self.env.cfg_env.clone();
+            let header = self.env.header.inner().clone();
             let (result, db) = tokio::task::spawn_blocking(move || {
                 let mut evm = new_evm(db, cfg, header);
                 let result = self.tx.transact(&mut evm);
@@ -232,8 +233,9 @@ mod host {
                 (result, db)
             })
             .await
-            .context("EVM execution panicked")?;
+            .expect("EVM execution panicked");
 
+            // restore the DB before handling errors, so that we never return an env without a DB
             self.env.db = Some(db);
 
             result.map_err(|err| anyhow!("call '{}' failed: {}", C::SIGNATURE, err))
@@ -249,6 +251,7 @@ mod host {
         /// [EvmEnv]: crate::EvmEnv
         pub async fn call_with_prefetch(self) -> Result<C::Return> {
             let access_list = {
+                // safe unwrap: env is never returned without a DB
                 let db = self.env.db.as_mut().unwrap();
 
                 let tx = <N as Network>::TransactionRequest::default()
@@ -285,14 +288,15 @@ where
     /// Executes the call with a [EvmEnv] constructed with [Contract::new].
     ///
     /// [EvmEnv]: crate::EvmEnv
-    pub fn call(self) -> C::Return {
+    pub fn call(self) -> Result<C::Return, String> {
+        // safe unwrap: env is never returned without a DB
         let state_db = self.env.db.as_ref().unwrap();
         let mut evm = new_evm::<_, H>(
             WrapStateDb::new(state_db),
             self.env.cfg_env.clone(),
             self.env.header.inner(),
         );
-        self.tx.transact(&mut evm).unwrap()
+        self.tx.transact(&mut evm)
     }
 }
 

steel/src/ethereum.rs

@@ -94,6 +94,7 @@ impl EvmBlockHeader for EthBlockHeader {
         // technically, this is only valid after EIP-4399 but revm makes sure it is not used before
         blk_env.prevrandao = Some(header.mix_hash);
         if let Some(excess_blob_gas) = header.excess_blob_gas {
+            // safe conversion: according to the consensus spec, excess_blob_gas is always an uint64
             blk_env.set_blob_excess_gas_and_price(excess_blob_gas.try_into().unwrap())
         };
     }

steel/src/host/db/alloy.rs

@@ -79,8 +79,17 @@ impl<T: Transport + Clone, N: Network, P: Provider<T, N>> ProviderDb<T, N, P> fo
     }
 }
 
+/// Errors returned by the [AlloyDb].
+#[derive(Debug, thiserror::Error)]
+pub enum Error {
+    #[error("RPC error")]
+    RPC(#[from] TransportError),
+    #[error("block not found")]
+    BlockNotFound,
+}
+
 impl<T: Transport + Clone, N: Network, P: Provider<T, N>> Database for AlloyDb<T, N, P> {
-    type Error = TransportError;
+    type Error = Error;
 
     fn basic(&mut self, address: Address) -> Result<Option<AccountInfo>, Self::Error> {
         let f = async {
@@ -143,11 +152,11 @@ impl<T: Transport + Clone, N: Network, P: Provider<T, N>> Database for AlloyDb<T
     }
 
     fn block_hash(&mut self, number: u64) -> Result<B256, Self::Error> {
-        let block = self
+        let block_response = self
             .handle
             .block_on(self.provider.get_block_by_number(number.into(), false))?;
-        // TODO: return proper error
-        let block = block.unwrap();
+        let block = block_response.ok_or(Error::BlockNotFound)?;
+
         Ok(block.header().hash())
     }
 }

steel/src/host/db/proof.rs

@@ -161,14 +161,19 @@ impl<T: Transport + Clone, N: Network, P: Provider<T, N>> ProofDb<AlloyDb<T, N,
                     .get_eip1186_proof(*address, storage_keys)
                     .await
                     .context("eth_getProof failed")?;
+                ensure!(
+                    &proof.address == address,
+                    "eth_getProof response does not match request"
+                );
                 add_proof(proofs, proof).context("invalid eth_getProof response")?;
             }
         }
 
         let state_nodes = self
             .accounts
-            .iter()
-            .flat_map(|(address, _)| proofs.get(address).unwrap().account_proof.iter());
+            .keys()
+            .filter_map(|address| proofs.get(address))
+            .flat_map(|proof| proof.account_proof.iter());
         let state_trie = MerkleTrie::from_rlp_nodes(state_nodes).context("accountProof invalid")?;
 
         let mut storage_tries = HashMap::new();
@@ -178,11 +183,13 @@ impl<T: Transport + Clone, N: Network, P: Provider<T, N>> ProofDb<AlloyDb<T, N,
                 continue;
             }
 
+            // safe unwrap: added a proof for each account in the previous loop
             let storage_proofs = &proofs.get(address).unwrap().storage_proofs;
 
             let storage_nodes = storage_keys
                 .iter()
-                .flat_map(|key| storage_proofs.get(key).unwrap().proof.iter());
+                .filter_map(|key| storage_proofs.get(key))
+                .flat_map(|proof| proof.proof.iter());
             let storage_trie =
                 MerkleTrie::from_rlp_nodes(storage_nodes).context("storageProof invalid")?;
             let storage_root_hash = storage_trie.hash_slow();

steel/src/host/mod.rs

@@ -91,7 +91,7 @@ where
     T: Transport + Clone,
     P: Provider<T, Ethereum>,
 {
-    /// Converts the environment into a [EvmInput] committing to a Ethereum Beacon block root.
+    /// Converts the environment into a [EvmInput] committing to an Ethereum Beacon block root.
     ///
     /// This function assumes that the
     /// [mainnet](https://github.com/ethereum/consensus-specs/blob/v1.4.0/configs/mainnet.yaml)

steel/src/lib.rs

@@ -73,6 +73,7 @@ pub struct EvmEnv<D, H> {
 
 impl<D, H: EvmBlockHeader> EvmEnv<D, H> {
     /// Creates a new environment.
+    ///
     /// It uses the default configuration for the latest specification.
     pub(crate) fn new(db: D, header: Sealed<H>) -> Self {
         let cfg_env = CfgEnvWithHandlerCfg::new_with_spec_id(Default::default(), SpecId::LATEST);
@@ -87,6 +88,8 @@ impl<D, H: EvmBlockHeader> EvmEnv<D, H> {
     }
 
     /// Sets the chain ID and specification ID from the given chain spec.
+    ///
+    /// This will panic when there is no valid specification ID for the current block.
     pub fn with_chain_spec(mut self, chain_spec: &config::ChainSpec) -> Self {
         self.cfg_env.chain_id = chain_spec.chain_id();
         self.cfg_env.handler_cfg.spec_id = chain_spec

steel/tests/common/mod.rs

@@ -70,7 +70,7 @@ where
 
     let result = {
         let contract = Contract::new(address, &env);
-        options.apply(contract.call_builder(&call)).call()
+        options.apply(contract.call_builder(&call)).call().unwrap()
     };
     assert_eq!(
         result, preflight_result,

steel/tests/corruption.rs

@@ -114,10 +114,12 @@ fn mock_anvil_guest(input: EthEvmInput) -> Commitment {
     let env = input.into_env().with_chain_spec(&ANVIL_CHAIN_SPEC);
     Contract::new(ANVIL_CONTRACT_ADDRESS, &env)
         .call_builder(&sol::Pair::aCall {})
-        .call();
+        .call()
+        .unwrap();
     Contract::new(ANVIL_CONTRACT_ADDRESS, &env)
         .call_builder(&sol::Pair::bCall {})
-        .call();
+        .call()
+        .unwrap();
 
     env.into_commitment()
 }
@@ -319,7 +321,7 @@ async fn corrupt_ancestor() {
 #[should_panic(expected = "Invalid commitment")]
 async fn corrupt_header_block_commitment() {
     let input = anvil_input().await.unwrap();
-    let exp_commitment = input.clone().into_env().into_commitment();
+    let exp_commit = input.clone().into_env().into_commitment();
 
     // get the JSON representation of the block header for the state
     let mut input_value = to_value(&input).unwrap();
@@ -331,8 +333,12 @@ async fn corrupt_header_block_commitment() {
     *header_value = to_value(header).unwrap();
 
     // executing this should lead to an Invalid commitment
-    let commitment = mock_anvil_guest(from_value(input_value).unwrap());
-    assert_eq!(commitment, exp_commitment, "Invalid commitment");
+    let commit = mock_anvil_guest(from_value(input_value).unwrap());
+    assert_eq!(commit.blockID, exp_commit.blockID, "Commitment changed");
+    assert_eq!(
+        commit.blockDigest, exp_commit.blockDigest,
+        "Invalid commitment"
+    );
 }
 
 #[test(tokio::test)]
@@ -343,24 +349,30 @@ async fn corrupt_header_beacon_commitment() {
     })
     .await
     .unwrap();
-    let exp_commitment = input.clone().into_env().into_commitment();
+    let exp_commit = input.clone().into_env().into_commitment();
 
     // get the JSON representation of the block header for the state
     let mut input_value = to_value(&input).unwrap();
     let header_value = &mut get_block_input_mut(&mut input_value)["header"];
 
-    // corrupt the header by modifying its timestamp
+    // corrupt the header by modifying its number
     let mut header: EthBlockHeader = from_value(header_value.clone()).unwrap();
-    header.inner_mut().timestamp = 0xdeadbeaf;
+    header.inner_mut().number = 0xdeadbeaf;
     *header_value = to_value(header).unwrap();
 
     // executing this should lead to an Invalid commitment
     let input: EthEvmInput = from_value(input_value).unwrap();
     let env = input.into_env().with_chain_spec(&ANVIL_CHAIN_SPEC);
     Contract::new(USDT_ADDRESS, &env)
         .call_builder(&USDT_CALL)
-        .call();
-    assert_eq!(env.into_commitment(), exp_commitment, "Invalid commitment");
+        .call()
+        .unwrap();
+    let commit = env.into_commitment();
+    assert_eq!(commit.blockID, exp_commit.blockID, "Changed commitment");
+    assert_eq!(
+        commit.blockDigest, exp_commit.blockDigest,
+        "Invalid commitment"
+    );
 }
 
 #[test(tokio::test)]
@@ -371,7 +383,7 @@ async fn corrupt_beacon_proof() {
     })
     .await
     .unwrap();
-    let exp_commitment = input.clone().into_env().into_commitment();
+    let exp_commit = input.clone().into_env().into_commitment();
 
     // get the JSON representation of the block header for the state
     let mut input_value = to_value(&input).unwrap();
@@ -385,6 +397,12 @@ async fn corrupt_beacon_proof() {
     let env = input.into_env().with_chain_spec(&ANVIL_CHAIN_SPEC);
     Contract::new(USDT_ADDRESS, &env)
         .call_builder(&USDT_CALL)
-        .call();
-    assert_eq!(env.into_commitment(), exp_commitment, "Invalid commitment");
+        .call()
+        .unwrap();
+    let commit = env.into_commitment();
+    assert_eq!(commit.blockID, exp_commit.blockID, "Commitment changed");
+    assert_eq!(
+        commit.blockDigest, exp_commit.blockDigest,
+        "Invalid commitment"
+    );
 }