fix(cheatcodes): return early in case of reverts to not conflict wi…

…th expect* logic (#6172)

* feat: return early in case of reverts to not conflict with expect* logic

* chore: move diagnose revert logic

* add test

crates/evm/evm/src/inspectors/cheatcodes/mod.rs

@@ -831,9 +831,47 @@ impl<DB: DatabaseExt> Inspector<DB> for Cheatcodes {
             }
         }
 
+        // 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 == InstructionResult::Revert {
+            if let Some(err) = self.fork_revert_diagnostic.take() {
+                return (
+                    status,
+                    remaining_gas,
+                    DynSolValue::String(err.to_error_msg(&self.labels)).abi_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 == InstructionResult::Stop &&
+                call.contract != test_contract
+            {
+                self.fork_revert_diagnostic =
+                    data.db.diagnose_revert(call.contract, &data.journaled_state);
+            }
+        }
+
         // If the depth is 0, then this is the root call terminating
         if data.journaled_state.depth() == 0 {
-            // Match expected calls
+            // If we already have a revert, we shouldn't run the below logic as it can obfuscate an
+            // earlier error that happened first with unrelated information about
+            // another error when using cheatcodes.
+            if status == InstructionResult::Revert {
+                return (status, remaining_gas, retdata)
+            }
+
+            // If there's not a revert, we can continue on to run the last logic for expect*
+            // cheatcodes. Match expected calls
             for (address, calldatas) in &self.expected_calls {
                 // Loop over each address, and for each address, loop over each calldata it expects.
                 for (calldata, (expected, actual_count)) in calldatas {
@@ -918,36 +956,6 @@ impl<DB: DatabaseExt> Inspector<DB> for Cheatcodes {
             }
         }
 
-        // 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 == InstructionResult::Revert {
-            if let Some(err) = self.fork_revert_diagnostic.take() {
-                return (
-                    status,
-                    remaining_gas,
-                    DynSolValue::String(err.to_error_msg(&self.labels)).abi_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 == InstructionResult::Stop &&
-                call.contract != test_contract
-            {
-                self.fork_revert_diagnostic =
-                    data.db.diagnose_revert(call.contract, &data.journaled_state);
-            }
-        }
-
         (status, remaining_gas, retdata)
     }
 

crates/forge/tests/it/repros.rs

@@ -6,6 +6,7 @@ use crate::{
 };
 use alloy_primitives::Address;
 use ethers::abi::{Event, EventParam, Log, LogParam, ParamType, RawLog, Token};
+use forge::result::TestStatus;
 use foundry_config::{fs_permissions::PathPermission, Config, FsPermissions};
 use std::str::FromStr;
 
@@ -308,3 +309,13 @@ async fn test_issue_5808() {
 async fn test_issue_6115() {
     test_repro!("Issue6115");
 }
+
+// <https://github.com/foundry-rs/foundry/issues/6170>
+#[tokio::test(flavor = "multi_thread")]
+async fn test_issue_6170() {
+    let mut res = run_test_repro!("Issue6170");
+    let mut res = res.remove("repros/Issue6170.t.sol:Issue6170Test").unwrap();
+    let test = res.test_results.remove("test()").unwrap();
+    assert_eq!(test.status, TestStatus::Failure);
+    assert_eq!(test.reason, Some("Log != expected log".to_string()));
+}

testdata/repros/Issue6170.t.sol

@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: Unlicense
+pragma solidity 0.8.18;
+
+import "ds-test/test.sol";
+import "../cheats/Vm.sol";
+
+contract Emitter {
+    event Values(uint256 indexed a, uint256 indexed b);
+
+    function plsEmit(uint256 a, uint256 b) external {
+        emit Values(a, b);
+    }
+}
+
+// https://github.com/foundry-rs/foundry/issues/6170
+contract Issue6170Test is DSTest {
+    Vm constant vm = Vm(HEVM_ADDRESS);
+
+    event Values(uint256 indexed a, uint256 b);
+
+    Emitter e = new Emitter();
+
+    function test() public {
+        vm.expectEmit(true, true, false, true);
+        emit Values(69, 420);
+        e.plsEmit(69, 420);
+    }
+}