feat(cheatcodes): display cheatcode name in error message (#8533)

* feat(cheatcodes): display cheatcode name in error message

* fix: tests

* fix

* rm

* fix

* fix

Author: DaniPopes

Cargo.lock

@@ -45,23 +45,24 @@ parking_lot.workspace = true
 alloy-consensus = { workspace = true, features = ["k256"] }
 alloy-rlp.workspace = true
 
+base64.workspace = true
+chrono.workspace = true
+dialoguer = "0.11.0"
 eyre.workspace = true
 itertools.workspace = true
 jsonpath_lib.workspace = true
+k256.workspace = true
+memchr = "2.7"
+p256 = "0.13.2"
+rand = "0.8"
 revm.workspace = true
+rustc-hash.workspace = true
+semver.workspace = true
 serde_json.workspace = true
-base64.workspace = true
+thiserror.workspace = true
 toml = { workspace = true, features = ["preserve_order"] }
 tracing.workspace = true
-k256.workspace = true
 walkdir.workspace = true
-p256 = "0.13.2"
-thiserror.workspace = true
-semver.workspace = true
-rustc-hash.workspace = true
-dialoguer = "0.11.0"
-rand = "0.8"
-chrono.workspace = true
 
 [dev-dependencies]
 proptest.workspace = true

crates/cheatcodes/Cargo.toml

@@ -27,7 +27,7 @@ the Foundry cheatcodes externally.
 
 For example, here are some tools that make use of the JSON interface:
 - Internally, this is used to generate [a simple Solidity interface](../../testdata/cheats/Vm.sol) for testing
-- (WIP) Used by [`forge-std`](https://github.com/foundry-rs/forge-std) to generate [user-friendly Solidity interfaces](https://github.com/foundry-rs/forge-std/blob/master/src/Vm.sol)
+- Used by [`forge-std`](https://github.com/foundry-rs/forge-std) to generate [user-friendly Solidity interfaces](https://github.com/foundry-rs/forge-std/blob/master/src/Vm.sol)
 - (WIP) Used by [the Foundry book](https://github.com/foundry-rs/book) to generate [the cheatcodes reference](https://book.getfoundry.sh/cheatcodes)
 - ...
 

crates/cheatcodes/README.md

@@ -68,17 +68,14 @@ macro_rules! ensure {
 macro_rules! ensure_not_precompile {
     ($address:expr, $ctxt:expr) => {
         if $ctxt.is_precompile($address) {
-            return Err($crate::error::precompile_error(
-                <Self as $crate::CheatcodeDef>::CHEATCODE.func.id,
-                $address,
-            ))
+            return Err($crate::error::precompile_error($address));
         }
     };
 }
 
 #[cold]
-pub(crate) fn precompile_error(id: &'static str, address: &Address) -> Error {
-    fmt_err!("cannot call `{id}` on precompile {address}")
+pub(crate) fn precompile_error(address: &Address) -> Error {
+    fmt_err!("cannot use precompile {address} as an argument")
 }
 
 /// Error thrown by cheatcodes.
@@ -158,7 +155,7 @@ impl Error {
     }
 
     /// Returns the kind of this error.
-    #[inline(always)]
+    #[inline]
     pub fn kind(&self) -> ErrorKind<'_> {
         let data = self.data();
         if self.is_str {
@@ -170,32 +167,38 @@ impl Error {
     }
 
     /// Returns the raw data of this error.
-    #[inline(always)]
+    #[inline]
     pub fn data(&self) -> &[u8] {
         unsafe { &*self.data }
     }
 
-    #[inline(always)]
+    /// Returns `true` if this error is a human-readable string.
+    #[inline]
+    pub fn is_str(&self) -> bool {
+        self.is_str
+    }
+
+    #[inline]
     fn new_str(data: &'static str) -> Self {
         Self::_new(true, false, data.as_bytes())
     }
 
-    #[inline(always)]
+    #[inline]
     fn new_string(data: String) -> Self {
         Self::_new(true, true, Box::into_raw(data.into_boxed_str().into_boxed_bytes()))
     }
 
-    #[inline(always)]
+    #[inline]
     fn new_bytes(data: &'static [u8]) -> Self {
         Self::_new(false, false, data)
     }
 
-    #[inline(always)]
+    #[inline]
     fn new_vec(data: Vec<u8>) -> Self {
         Self::_new(false, true, Box::into_raw(data.into_boxed_slice()))
     }
 
-    #[inline(always)]
+    #[inline]
     fn _new(is_str: bool, drop: bool, data: *const [u8]) -> Self {
         debug_assert!(!data.is_null());
         Self { is_str, drop, data }

crates/cheatcodes/src/error.rs

@@ -275,13 +275,10 @@ impl Cheatcode for resumeGasMeteringCall {
 impl Cheatcode for lastCallGasCall {
     fn apply(&self, state: &mut Cheatcodes) -> Result {
         let Self {} = self;
-        ensure!(state.last_call_gas.is_some(), "`lastCallGas` is only available after a call");
-        Ok(state
-            .last_call_gas
-            .as_ref()
-            // This should never happen, as we ensure `last_call_gas` is `Some` above.
-            .expect("`lastCallGas` is only available after a call")
-            .abi_encode())
+        let Some(last_call_gas) = &state.last_call_gas else {
+            bail!("no external call was made yet");
+        };
+        Ok(last_call_gas.abi_encode())
     }
 }
 
@@ -354,7 +351,7 @@ impl Cheatcode for blobhashesCall {
         let Self { hashes } = self;
         ensure!(
             ccx.ecx.spec_id() >= SpecId::CANCUN,
-            "`blobhash` is not supported before the Cancun hard fork; \
+            "`blobhashes` is not supported before the Cancun hard fork; \
              see EIP-4844: https://eips.ethereum.org/EIPS/eip-4844"
         );
         ccx.ecx.env.tx.blob_hashes.clone_from(hashes);
@@ -367,7 +364,7 @@ impl Cheatcode for getBlobhashesCall {
         let Self {} = self;
         ensure!(
             ccx.ecx.spec_id() >= SpecId::CANCUN,
-            "`blobhash` is not supported before the Cancun hard fork; \
+            "`getBlobhashes` is not supported before the Cancun hard fork; \
              see EIP-4844: https://eips.ethereum.org/EIPS/eip-4844"
         );
         Ok(ccx.ecx.env.tx.blob_hashes.clone().abi_encode())
@@ -605,9 +602,8 @@ impl Cheatcode for broadcastRawTransactionCall {
         executor: &mut E,
     ) -> Result {
         let mut data = self.data.as_ref();
-        let tx = TxEnvelope::decode(&mut data).map_err(|err| {
-            fmt_err!("broadcastRawTransaction: error decoding transaction ({err})")
-        })?;
+        let tx = TxEnvelope::decode(&mut data)
+            .map_err(|err| fmt_err!("failed to decode RLP-encoded transaction: {err}"))?;
 
         ccx.ecx.db.transact_from_tx(
             tx.clone().into(),

crates/cheatcodes/src/evm.rs

@@ -352,7 +352,7 @@ fn get_artifact_code(state: &Cheatcodes, path: &str, deployed: bool) -> Result<B
         }
 
         let version = if let Some(version) = version {
-            Some(Version::parse(version).map_err(|_| fmt_err!("Error parsing version"))?)
+            Some(Version::parse(version).map_err(|e| fmt_err!("failed parsing version: {e}"))?)
         } else {
             None
         };
@@ -388,7 +388,7 @@ fn get_artifact_code(state: &Cheatcodes, path: &str, deployed: bool) -> Result<B
                 .collect::<Vec<_>>();
 
             let artifact = match &filtered[..] {
-                [] => Err(fmt_err!("No matching artifact found")),
+                [] => Err(fmt_err!("no matching artifact found")),
                 [artifact] => Ok(artifact),
                 filtered => {
                     // If we know the current script/test contract solc version, try to filter by it
@@ -403,7 +403,7 @@ fn get_artifact_code(state: &Cheatcodes, path: &str, deployed: bool) -> Result<B
                                 .collect::<Vec<_>>();
                             (filtered.len() == 1).then(|| filtered[0])
                         })
-                        .ok_or_else(|| fmt_err!("Multiple matching artifacts found"))
+                        .ok_or_else(|| fmt_err!("multiple matching artifacts found"))
                 }
             }?;
 
@@ -414,7 +414,7 @@ fn get_artifact_code(state: &Cheatcodes, path: &str, deployed: bool) -> Result<B
             };
 
             return maybe_bytecode
-                .ok_or_else(|| fmt_err!("No bytecode for contract. Is it abstract or unlinked?"));
+                .ok_or_else(|| fmt_err!("no bytecode for contract; is it abstract or unlinked?"));
         } else {
             let path_in_artifacts =
                 match (file.map(|f| f.to_string_lossy().to_string()), contract_name) {
@@ -439,7 +439,7 @@ fn get_artifact_code(state: &Cheatcodes, path: &str, deployed: bool) -> Result<B
     let data = fs::read_to_string(path)?;
     let artifact = serde_json::from_str::<ContractObject>(&data)?;
     let maybe_bytecode = if deployed { artifact.deployed_bytecode } else { artifact.bytecode };
-    maybe_bytecode.ok_or_else(|| fmt_err!("No bytecode for contract. Is it abstract or unlinked?"))
+    maybe_bytecode.ok_or_else(|| fmt_err!("no bytecode for contract; is it abstract or unlinked?"))
 }
 
 impl Cheatcode for ffiCall {

crates/cheatcodes/src/fs.rs

@@ -592,6 +592,7 @@ impl Cheatcodes {
             {
                 let expected_revert = std::mem::take(&mut self.expected_revert).unwrap();
                 return match expect::handle_expect_revert(
+                    false,
                     true,
                     expected_revert.reason.as_deref(),
                     outcome.result.result,
@@ -1035,7 +1036,7 @@ impl<DB: DatabaseExt> Inspector<DB> for Cheatcodes {
         // Handle expected reverts
         if let Some(expected_revert) = &self.expected_revert {
             if ecx.journaled_state.depth() <= expected_revert.depth {
-                let needs_processing: bool = match expected_revert.kind {
+                let needs_processing = match expected_revert.kind {
                     ExpectedRevertKind::Default => !cheatcode_call,
                     // `pending_processing` == true means that we're in the `call_end` hook for
                     // `vm.expectCheatcodeRevert` and shouldn't expect revert here
@@ -1047,6 +1048,7 @@ impl<DB: DatabaseExt> Inspector<DB> for Cheatcodes {
                 if needs_processing {
                     let expected_revert = std::mem::take(&mut self.expected_revert).unwrap();
                     return match expect::handle_expect_revert(
+                        cheatcode_call,
                         false,
                         expected_revert.reason.as_deref(),
                         outcome.result.result,
@@ -1071,9 +1073,7 @@ impl<DB: DatabaseExt> Inspector<DB> for Cheatcodes {
                 if let ExpectedRevertKind::Cheatcode { pending_processing } =
                     &mut self.expected_revert.as_mut().unwrap().kind
                 {
-                    if *pending_processing {
-                        *pending_processing = false;
-                    }
+                    *pending_processing = false;
                 }
             }
         }
@@ -1847,22 +1847,49 @@ fn apply_dispatch<DB: DatabaseExt, E: CheatcodesExecutor>(
         };
     }
 
-    let _guard = trace_span_and_call(calls);
-    let result = vm_calls!(dispatch);
-    trace_return(&result);
+    let mut dyn_cheat = DynCheatCache::new(calls);
+    let _guard = trace_span_and_call(&mut dyn_cheat);
+    let mut result = vm_calls!(dispatch);
+    fill_and_trace_return(&mut dyn_cheat, &mut result);
     result
 }
 
-fn trace_span_and_call(calls: &Vm::VmCalls) -> tracing::span::EnteredSpan {
-    let mut cheat = None;
-    let mut get_cheat = || *cheat.get_or_insert_with(|| calls_as_dyn_cheatcode(calls));
-    let span = debug_span!(target: "cheatcodes", "apply", id = %get_cheat().id());
+// Caches the result of `calls_as_dyn_cheatcode`.
+// TODO: Remove this once Cheatcode is object-safe, as caching would not be necessary anymore.
+struct DynCheatCache<'a> {
+    calls: &'a Vm::VmCalls,
+    slot: Option<&'a dyn DynCheatcode>,
+}
+
+impl<'a> DynCheatCache<'a> {
+    fn new(calls: &'a Vm::VmCalls) -> Self {
+        Self { calls, slot: None }
+    }
+
+    fn get(&mut self) -> &dyn DynCheatcode {
+        *self.slot.get_or_insert_with(|| calls_as_dyn_cheatcode(self.calls))
+    }
+}
+
+fn trace_span_and_call(dyn_cheat: &mut DynCheatCache) -> tracing::span::EnteredSpan {
+    let span = debug_span!(target: "cheatcodes", "apply", id = %dyn_cheat.get().id());
     let entered = span.entered();
-    trace!(target: "cheatcodes", cheat = ?get_cheat().as_debug(), "applying");
+    trace!(target: "cheatcodes", cheat = ?dyn_cheat.get().as_debug(), "applying");
     entered
 }
 
-fn trace_return(result: &Result) {
+fn fill_and_trace_return(dyn_cheat: &mut DynCheatCache, result: &mut Result) {
+    if let Err(e) = result {
+        if e.is_str() {
+            let name = dyn_cheat.get().name();
+            // Skip showing the cheatcode name for:
+            // - assertions: too verbose, and can already be inferred from the error message
+            // - `rpcUrl`: forge-std relies on it in `getChainWithUpdatedRpcUrl`
+            if !name.contains("assert") && name != "rpcUrl" {
+                *e = fmt_err!("vm.{name}: {e}");
+            }
+        }
+    }
     trace!(
         target: "cheatcodes",
         return = %match result {

crates/cheatcodes/src/inspector.rs

@@ -85,11 +85,15 @@ pub(crate) trait Cheatcode: CheatcodeDef + DynCheatcode {
 }
 
 pub(crate) trait DynCheatcode {
+    fn name(&self) -> &'static str;
     fn id(&self) -> &'static str;
     fn as_debug(&self) -> &dyn std::fmt::Debug;
 }
 
 impl<T: Cheatcode> DynCheatcode for T {
+    fn name(&self) -> &'static str {
+        T::CHEATCODE.func.signature.split('(').next().unwrap()
+    }
     fn id(&self) -> &'static str {
         T::CHEATCODE.func.id
     }

crates/cheatcodes/src/lib.rs

@@ -186,9 +186,7 @@ fn parse_value_fallback(s: &str, ty: &DynSolType) -> Option<Result<DynSolValue,
                 "0" => false,
                 s if s.eq_ignore_ascii_case("true") => true,
                 s if s.eq_ignore_ascii_case("false") => false,
-                _ => {
-                    return None;
-                }
+                _ => return None,
             };
             return Some(Ok(DynSolValue::Bool(b)));
         }

crates/cheatcodes/src/string.rs

@@ -542,6 +542,7 @@ fn expect_revert(
 }
 
 pub(crate) fn handle_expect_revert(
+    is_cheatcode: bool,
     is_create: bool,
     expected_revert: Option<&[u8]>,
     status: InstructionResult,
@@ -578,7 +579,9 @@ pub(crate) fn handle_expect_revert(
         }
     }
 
-    if actual_revert == expected_revert {
+    if actual_revert == expected_revert ||
+        (is_cheatcode && memchr::memmem::find(&actual_revert, expected_revert).is_some())
+    {
         Ok(success_return())
     } else {
         let stringify = |data: &[u8]| {