feat: fork revert diagnostic

evm/src/executor/backend/diagnostic.rs

@@ -32,11 +32,8 @@ impl RevertDiagnostic {
 
                 format!(
                     r#"Contract {} does not exists on active fork with id `{}`
-        But exists on non active forks `{}`
-"#,
-                    contract_label,
-                    active,
-                    available_on.pretty()
+        But exists on non active forks: `{:?}`"#,
+                    contract_label, active, available_on
                 )
             }
             RevertDiagnostic::ContractDoesNotExist { contract, .. } => {

evm/src/executor/backend/mod.rs

@@ -121,6 +121,11 @@ pub trait DatabaseExt: Database {
     /// Returns the `ForkId` that's currently used in the database, if fork mode is on
     fn active_fork_id(&self) -> Option<LocalForkId>;
 
+    /// Whether the database is currently in forked
+    fn is_forked_mode(&self) -> bool {
+        self.active_fork_id().is_some()
+    }
+
     /// Ensures that an appropriate fork exits
     ///
     /// If `id` contains a requested `Fork` this will ensure it exits.

evm/src/executor/inspector/cheatcodes/expect.rs

@@ -1,5 +1,3 @@
-use std::cmp::Ordering;
-
 use super::Cheatcodes;
 use crate::{
     abi::HEVMCalls,
@@ -11,6 +9,7 @@ use ethers::{
     types::{Address, H160, U256},
 };
 use revm::{return_ok, Database, EVMData, Return};
+use std::cmp::Ordering;
 
 /// For some cheatcodes we may internally change the status of the call, i.e. in `expectRevert`.
 /// Solidity will see a successful call and attempt to decode the return data. Therefore, we need
@@ -72,37 +71,39 @@ pub fn handle_expect_revert(
         _ => None,
     };
 
-    let (err, actual_revert): (_, Bytes) = match string_data {
-        Some(data) => {
-            // It's a revert string, so we do some conversion to perform the check
-            let decoded_data = ethers::prelude::Bytes::decode(data)
-                .expect("String error code, but data can't be decoded as bytes");
+    let stringify = |data: &[u8]| {
+        String::decode(data)
+            .ok()
+            .or_else(|| String::from_utf8(data.to_vec()).ok())
+            .unwrap_or_else(|| format!("0x{}", hex::encode(data)))
+    };
+
+    let (err, actual_revert): (_, Bytes) = if let Some(data) = string_data {
+        // It's a revert string, so we do some conversion to perform the check
+        let decoded_data = ethers::prelude::Bytes::decode(data)
+            .expect("String error code, but data can't be decoded as bytes");
 
-            (
-                format!(
-                    "Error != expected error: '{}' != '{}'",
-                    String::from_utf8(decoded_data.to_vec())
-                        .ok()
-                        .unwrap_or_else(|| hex::encode(&decoded_data)),
-                    String::from_utf8(expected_revert.to_vec())
-                        .ok()
-                        .unwrap_or_else(|| hex::encode(&expected_revert))
-                )
-                .encode()
-                .into(),
-                decoded_data.0,
+        (
+            format!(
+                "Error != expected error: '{}' != '{}'",
+                stringify(&decoded_data),
+                stringify(expected_revert),
             )
-        }
-        _ => (
+            .encode()
+            .into(),
+            decoded_data.0,
+        )
+    } else {
+        (
             format!(
-                "Error != expected error: 0x{} != 0x{}",
-                hex::encode(&retdata),
-                hex::encode(&expected_revert)
+                "Error != expected error: {} != {}",
+                stringify(&retdata),
+                stringify(expected_revert),
             )
             .encode()
             .into(),
             retdata,
-        ),
+        )
     };
 
     if actual_revert == expected_revert {

evm/src/executor/inspector/cheatcodes/mod.rs

@@ -19,8 +19,8 @@ use ethers::{
     },
 };
 use revm::{
-    opcode, return_revert, BlockEnv, CallInputs, CreateInputs, EVMData, Gas, Inspector,
-    Interpreter, Return, TransactTo,
+    opcode, BlockEnv, CallInputs, CreateInputs, EVMData, Gas, Inspector, Interpreter, Return,
+    TransactTo,
 };
 use std::{
     collections::{BTreeMap, HashMap, VecDeque},
@@ -50,6 +50,7 @@ pub mod util;
 pub use util::{DEFAULT_CREATE2_DEPLOYER, MISSING_CREATE2_DEPLOYER};
 
 mod config;
+use crate::executor::backend::RevertDiagnostic;
 pub use config::CheatsConfig;
 
 /// An inspector that handles calls to various cheatcodes, each with their own behavior.
@@ -79,6 +80,9 @@ pub struct Cheatcodes {
     /// Expected revert information
     pub expected_revert: Option<ExpectedRevert>,
 
+    /// Additional diagnostic for reverts
+    pub fork_revert_diagnostic: Option<RevertDiagnostic>,
+
     /// Recorded storage reads and writes
     pub accesses: Option<RecordAccess>,
 
@@ -177,6 +181,40 @@ where
         Return::Continue
     }
 
+    fn step(&mut self, interpreter: &mut Interpreter, _: &mut EVMData<'_, DB>, _: bool) -> Return {
+        // Record writes and reads if `record` has been called
+        if let Some(storage_accesses) = &mut self.accesses {
+            match interpreter.contract.code[interpreter.program_counter()] {
+                opcode::SLOAD => {
+                    let key = try_or_continue!(interpreter.stack().peek(0));
+                    storage_accesses
+                        .reads
+                        .entry(interpreter.contract().address)
+                        .or_insert_with(Vec::new)
+                        .push(key);
+                }
+                opcode::SSTORE => {
+                    let key = try_or_continue!(interpreter.stack().peek(0));
+
+                    // An SSTORE does an SLOAD internally
+                    storage_accesses
+                        .reads
+                        .entry(interpreter.contract().address)
+                        .or_insert_with(Vec::new)
+                        .push(key);
+                    storage_accesses
+                        .writes
+                        .entry(interpreter.contract().address)
+                        .or_insert_with(Vec::new)
+                        .push(key);
+                }
+                _ => (),
+            }
+        }
+
+        Return::Continue
+    }
+
     fn log(&mut self, _: &mut EVMData<'_, DB>, address: &Address, topics: &[H256], data: &Bytes) {
         // Match logs if `expectEmit` has been called
         if !self.expected_emits.is_empty() {
@@ -307,40 +345,6 @@ where
         }
     }
 
-    fn step(&mut self, interpreter: &mut Interpreter, _: &mut EVMData<'_, DB>, _: bool) -> Return {
-        // Record writes and reads if `record` has been called
-        if let Some(storage_accesses) = &mut self.accesses {
-            match interpreter.contract.code[interpreter.program_counter()] {
-                opcode::SLOAD => {
-                    let key = try_or_continue!(interpreter.stack().peek(0));
-                    storage_accesses
-                        .reads
-                        .entry(interpreter.contract().address)
-                        .or_insert_with(Vec::new)
-                        .push(key);
-                }
-                opcode::SSTORE => {
-                    let key = try_or_continue!(interpreter.stack().peek(0));
-
-                    // An SSTORE does an SLOAD internally
-                    storage_accesses
-                        .reads
-                        .entry(interpreter.contract().address)
-                        .or_insert_with(Vec::new)
-                        .push(key);
-                    storage_accesses
-                        .writes
-                        .entry(interpreter.contract().address)
-                        .or_insert_with(Vec::new)
-                        .push(key);
-                }
-                _ => (),
-            }
-        }
-
-        Return::Continue
-    }
-
     fn call_end(
         &mut self,
         data: &mut EVMData<'_, DB>,
@@ -432,12 +436,27 @@ where
             }
         }
 
-        // handle reverts in multi-fork mode where a call is made to an address that does not exist
-        if let TransactTo::Call(to) = data.env.tx.transact_to {
-            if matches!(status, return_revert!()) {
-                if let Some(diagnostic) = data.db.diagnose_revert(to, &data.subroutine) {
-                    return (status, remaining_gas, diagnostic.to_error_msg(self).encode().into())
-                }
+        // if there's a revert and a previous call was diagnosed as fork related revert then we can
+        // return a better error here
+        if status == Return::Revert {
+            if let Some(err) = self.fork_revert_diagnostic.take() {
+                return (status, remaining_gas, err.to_error_msg(self).encode().into())
+            }
+        }
+
+        // this will ensure we don't have false positives when trying to diagnose reverts in fork
+        // mode
+        let _ = self.fork_revert_diagnostic.take();
+
+        // try to diagnose reverts in multi-fork mode where a call is made to an address that does
+        // not exist
+        if let TransactTo::Call(test_contract) = data.env.tx.transact_to {
+            // if a call to a different contract than the original test contract returned with
+            // `Stop` we check if the contract actually exists on the active fork
+            if data.db.is_forked_mode() && status == Return::Stop && call.contract != test_contract
+            {
+                self.fork_revert_diagnostic =
+                    data.db.diagnose_revert(call.contract, &data.subroutine);
             }
         }
 

evm/src/executor/inspector/stack.rs

@@ -212,9 +212,10 @@ where
                     is_static,
                 );
 
-                // If the inspector returns a different status we assume it wants to tell us
-                // something
-                if new_status != status {
+                // If the inspector returns a different status or a revert with a non-empty message,
+                // we assume it wants to tell us something
+                if new_status != status || (new_status == Return::Revert && new_retdata != retdata)
+                {
                     return (new_status, new_gas, new_retdata)
                 }
             }

evm/src/executor/mod.rs

@@ -671,7 +671,7 @@ fn convert_executed_call(
         _ => Bytes::default(),
     };
 
-    let InspectorData { logs, labels, traces, debug, cheatcodes, coverage, .. } =
+    let InspectorData { logs, labels, traces, coverage, debug, cheatcodes } =
         inspector.collect_inspector_states();
 
     let transactions = if let Some(cheats) = cheatcodes {

forge/src/multi_runner.rs

@@ -1188,13 +1188,41 @@ Reason: `setEnv` failed to set an environment variable `{}={}`",
         );
     }
 
-    /// Executes all fork cheatcodes
+    /// Executes reverting fork test
+    #[test]
+    fn test_cheats_fork_revert() {
+        let mut runner = runner();
+        let suite_result = runner
+            .test(
+                &Filter::new(
+                    "testNonExistingContractRevert",
+                    ".*",
+                    &format!(".*cheats{}Fork", RE_PATH_SEPARATOR),
+                ),
+                None,
+                true,
+            )
+            .unwrap();
+        assert_eq!(suite_result.len(), 1);
+
+        for (_, SuiteResult { test_results, .. }) in suite_result {
+            for (_, result) in test_results {
+                assert_eq!(
+                     result.reason.unwrap(),
+                     "Contract 0xCe71065D4017F316EC606Fe4422e11eB2c47c246 does not exists on active fork with id `1`\n        But exists on non active forks: `[0]`"
+                );
+            }
+        }
+    }
+
+    /// Executes all non-reverting fork cheatcodes
     #[test]
     fn test_cheats_fork() {
         let mut runner = runner();
         let suite_result = runner
             .test(
-                &Filter::new(".*", ".*", &format!(".*cheats{}Fork", RE_PATH_SEPARATOR)),
+                &Filter::new(".*", ".*", &format!(".*cheats{}Fork", RE_PATH_SEPARATOR))
+                    .exclude_tests(".*Revert"),
                 None,
                 true,
             )

forge/src/test_helpers.rs

@@ -95,6 +95,7 @@ pub mod filter {
         test_regex: Regex,
         contract_regex: Regex,
         path_regex: Regex,
+        exclude_tests: Option<Regex>,
     }
 
     impl Filter {
@@ -103,21 +104,37 @@ pub mod filter {
                 test_regex: Regex::new(test_pattern).unwrap(),
                 contract_regex: Regex::new(contract_pattern).unwrap(),
                 path_regex: Regex::new(path_pattern).unwrap(),
+                exclude_tests: None,
             }
         }
 
+        /// All tests to also exclude
+        ///
+        /// This is a workaround since regex does not support negative look aheads
+        pub fn exclude_tests(mut self, pattern: &str) -> Self {
+            self.exclude_tests = Some(Regex::new(pattern).unwrap());
+            self
+        }
+
         pub fn matches_all() -> Self {
             Filter {
                 test_regex: Regex::new(".*").unwrap(),
                 contract_regex: Regex::new(".*").unwrap(),
                 path_regex: Regex::new(".*").unwrap(),
+                exclude_tests: None,
             }
         }
     }
 
     impl TestFilter for Filter {
         fn matches_test(&self, test_name: impl AsRef<str>) -> bool {
-            self.test_regex.is_match(test_name.as_ref())
+            let test_name = test_name.as_ref();
+            if let Some(ref exclude) = self.exclude_tests {
+                if exclude.is_match(test_name) {
+                    return false
+                }
+            }
+            self.test_regex.is_match(test_name)
         }
 
         fn matches_contract(&self, contract_name: impl AsRef<str>) -> bool {