fix(cheatcode): expect revert only for calls with greater depth

crates/cheatcodes/src/inspector.rs

@@ -1182,6 +1182,11 @@ impl Inspector<&mut dyn DatabaseExt> for Cheatcodes {
     fn step(&mut self, interpreter: &mut Interpreter, ecx: Ecx) {
         self.pc = interpreter.program_counter();
 
+        // `expectRevert`: track the max call depth during `expectRevert`
+        if self.expected_revert.is_some() {
+            self.record_expect_revert_depth(ecx);
+        }
+
         // `pauseGasMetering`: pause / resume interpreter gas.
         if self.gas_metering.paused {
             self.meter_gas(interpreter);
@@ -1302,21 +1307,17 @@ impl Inspector<&mut dyn DatabaseExt> for Cheatcodes {
 
         // Handle expected reverts.
         if let Some(expected_revert) = &mut self.expected_revert {
-            // Record current reverter address before processing the expect revert if call reverted,
-            // expect revert is set with expected reverter address and no actual reverter set yet.
-            if outcome.result.is_revert() &&
-                expected_revert.reverter.is_some() &&
-                expected_revert.reverted_by.is_none()
-            {
-                expected_revert.reverted_by = Some(call.target_address);
-            } else if outcome.result.is_revert() &&
-                expected_revert.reverter.is_some() &&
-                expected_revert.reverted_by.is_some() &&
-                expected_revert.count > 1
-            {
-                // If we're expecting more than one revert, we need to reset the reverted_by address
-                // to latest reverter.
-                expected_revert.reverted_by = Some(call.target_address);
+            // Record current reverter address and call scheme before processing the expect revert
+            // if call reverted.
+            if outcome.result.is_revert() {
+                // Record current reverter address if expect revert is set with expected reverter
+                // address and no actual reverter was set yet or if we're expecting more than one
+                // revert.
+                if expected_revert.reverter.is_some() &&
+                    (expected_revert.reverted_by.is_none() || expected_revert.count > 1)
+                {
+                    expected_revert.reverted_by = Some(call.target_address);
+                }
             }
 
             if ecx.journaled_state.depth() <= expected_revert.depth {
@@ -2050,6 +2051,15 @@ impl Cheatcodes {
             (REVERT, 0, 1, false),
         );
     }
+
+    #[cold]
+    fn record_expect_revert_depth(&mut self, ecx: Ecx) {
+        if let Some(ref mut expected) = self.expected_revert {
+            if ecx.journaled_state.depth() > expected.reverted_depth {
+                expected.reverted_depth = ecx.journaled_state.depth();
+            }
+        }
+    }
 }
 
 /// Helper that expands memory, stores a revert string pertaining to a disallowed memory write,

crates/cheatcodes/src/test/expect.rs

@@ -87,6 +87,8 @@ pub struct ExpectedRevert {
     pub reverter: Option<Address>,
     /// Actual reverter of the call.
     pub reverted_by: Option<Address>,
+    /// Max depth at which call reverted. Filled in by inspector during next call execution.
+    pub reverted_depth: u64,
     /// Number of times this revert is expected.
     pub count: u64,
     /// Actual number of times this revert has been seen.
@@ -774,6 +776,7 @@ fn expect_revert(
         partial_match,
         reverter,
         reverted_by: None,
+        reverted_depth: 0,
         count,
         actual_count: 0,
     });
@@ -806,6 +809,13 @@ pub(crate) fn handle_expect_revert(
         hex::encode_prefixed(data)
     };
 
+    if !is_cheatcode {
+        ensure!(
+            expected_revert.reverted_depth > expected_revert.depth,
+            "call didn't revert at a lower depth than cheatcode call depth"
+        );
+    }
+
     if expected_revert.count == 0 {
         if expected_revert.reverter.is_none() && expected_revert.reason.is_none() {
             ensure!(

crates/forge/tests/it/repros.rs

@@ -389,3 +389,6 @@ test_repro!(8971; |config| {
 
 // https://github.com/foundry-rs/foundry/issues/8639
 test_repro!(8639);
+
+// https://github.com/foundry-rs/foundry/issues/7238
+test_repro!(7238);

testdata/default/cheats/AttachDelegation.t.sol

@@ -86,7 +86,7 @@ contract AttachDelegationTest is DSTest {
         assertEq(token.balanceOf(bob), 200);
     }
 
-    function testAttachDelegationRevertInvalidSignature() public {
+    function testFailAttachDelegationRevertInvalidSignature() public {
         Vm.SignedDelegation memory signedDelegation = vm.signDelegation(address(implementation), alice_pk);
         // change v from 1 to 0
         signedDelegation.v = (signedDelegation.v + 1) % 2;
@@ -98,7 +98,6 @@ contract AttachDelegationTest is DSTest {
 
         vm.broadcast(alice_pk);
         // empty revert because no bytecode was set to Alice's account
-        vm.expectRevert();
         SimpleDelegateContract(alice).execute(calls);
     }
 
@@ -109,7 +108,7 @@ contract AttachDelegationTest is DSTest {
         // send tx to increment alice's nonce
         token.mint(1, bob);
 
-        vm.expectRevert("vm.attachDelegation: invalid nonce");
+        vm._expectCheatcodeRevert("vm.attachDelegation: invalid nonce");
         vm.attachDelegation(signedDelegation);
     }
 

testdata/default/cheats/BroadcastRawTransaction.t.sol

@@ -117,8 +117,8 @@ contract BroadcastRawTransactionTest is DSTest {
         assertEq(address(from).balance, balance - (gasPrice * 21_000) - amountSent);
         assertEq(address(to).balance, amountSent);
 
-        vm.expectRevert();
-        assert(3 == 4);
+        vm._expectCheatcodeRevert();
+        vm.assertFalse(true);
     }
 
     function test_execute_multiple_signed_tx() public {

testdata/default/cheats/MemSafety.t.sol

@@ -413,11 +413,8 @@ contract MemSafetyTest is DSTest {
 
     /// @dev Tests that expanding memory outside of the range given to `expectSafeMemory`
     ///      will cause the test to fail while using the `MLOAD` opcode.
-    function testExpectSafeMemory_MLOAD_REVERT() public {
+    function testFailExpectSafeMemory_MLOAD_REVERT() public {
         vm.expectSafeMemory(0x80, 0x100);
-
-        vm.expectRevert();
-
         // This should revert. Ugly hack to make sure the mload isn't optimized
         // out.
         uint256 a;
@@ -504,9 +501,8 @@ contract MemSafetyTest is DSTest {
 
     /// @dev Tests that expanding memory outside of the range given to `expectSafeMemory`
     ///      will cause the test to fail while using the `LOG0` opcode.
-    function testExpectSafeMemory_LOG0_REVERT() public {
+    function testFailExpectSafeMemory_LOG0_REVERT() public {
         vm.expectSafeMemory(0x80, 0x100);
-        vm.expectRevert();
         // This should revert.
         assembly {
             log0(0x100, 0x20)

testdata/default/cheats/MockCall.t.sol

@@ -201,8 +201,11 @@ contract MockCallRevertTest is DSTest {
 
         // post-mock
         assertEq(target.numberA(), 1);
-        vm.expectRevert();
-        target.numberB();
+        try target.numberB() {
+            revert();
+        } catch (bytes memory err) {
+            require(keccak256(err) == keccak256(ERROR_MESSAGE));
+        }
     }
 
     function testMockRevertWithCustomError() public {
@@ -216,8 +219,11 @@ contract MockCallRevertTest is DSTest {
         vm.mockCallRevert(address(target), abi.encodeWithSelector(target.numberB.selector), customError);
 
         assertEq(target.numberA(), 1);
-        vm.expectRevert(customError);
-        target.numberB();
+        try target.numberB() {
+            revert();
+        } catch (bytes memory err) {
+            require(keccak256(err) == keccak256(customError));
+        }
     }
 
     function testMockNestedRevert() public {
@@ -228,8 +234,11 @@ contract MockCallRevertTest is DSTest {
 
         vm.mockCallRevert(address(inner), abi.encodeWithSelector(inner.numberB.selector), ERROR_MESSAGE);
 
-        vm.expectRevert(ERROR_MESSAGE);
-        target.sum();
+        try target.sum() {
+            revert();
+        } catch (bytes memory err) {
+            require(keccak256(err) == keccak256(ERROR_MESSAGE));
+        }
     }
 
     function testMockCalldataRevert() public {
@@ -241,8 +250,11 @@ contract MockCallRevertTest is DSTest {
 
         assertEq(target.add(6, 4), 10);
 
-        vm.expectRevert(ERROR_MESSAGE);
-        target.add(5, 5);
+        try target.add(5, 5) {
+            revert();
+        } catch (bytes memory err) {
+            require(keccak256(err) == keccak256(ERROR_MESSAGE));
+        }
     }
 
     function testClearMockRevertedCalls() public {
@@ -263,8 +275,11 @@ contract MockCallRevertTest is DSTest {
 
         assertEq(mock.add(1, 2), 3);
 
-        vm.expectRevert(ERROR_MESSAGE);
-        mock.add(2, 3);
+        try mock.add(2, 3) {
+            revert();
+        } catch (bytes memory err) {
+            require(keccak256(err) == keccak256(ERROR_MESSAGE));
+        }
     }
 
     function testMockCallRevertWithValue() public {
@@ -275,8 +290,11 @@ contract MockCallRevertTest is DSTest {
         assertEq(mock.pay(1), 1);
         assertEq(mock.pay(2), 2);
 
-        vm.expectRevert(ERROR_MESSAGE);
-        mock.pay{value: 10}(1);
+        try mock.pay{value: 10}(1) {
+            revert();
+        } catch (bytes memory err) {
+            require(keccak256(err) == keccak256(ERROR_MESSAGE));
+        }
     }
 
     function testMockCallResetsMockCallRevert() public {
@@ -296,8 +314,11 @@ contract MockCallRevertTest is DSTest {
 
         vm.mockCallRevert(address(mock), abi.encodeWithSelector(mock.add.selector), ERROR_MESSAGE);
 
-        vm.expectRevert(ERROR_MESSAGE);
-        mock.add(2, 3);
+        try mock.add(2, 3) {
+            revert();
+        } catch (bytes memory err) {
+            require(keccak256(err) == keccak256(ERROR_MESSAGE));
+        }
     }
 
     function testMockCallRevertWithCall() public {
@@ -317,7 +338,10 @@ contract MockCallRevertTest is DSTest {
 
         vm.mockCallRevert(address(mock), abi.encodeWithSelector(mock.add.selector), ERROR_MESSAGE);
 
-        vm.expectRevert(ERROR_MESSAGE);
-        mock.add(1, 2);
+        try mock.add(2, 3) {
+            revert();
+        } catch (bytes memory err) {
+            require(keccak256(err) == keccak256(ERROR_MESSAGE));
+        }
     }
 }

testdata/default/cheats/RandomCheatcodes.t.sol

@@ -11,7 +11,7 @@ contract RandomCheatcodesTest is DSTest {
     int128 constant max = 170141183460469231731687303715884105727;
 
     function test_int128() public {
-        vm.expectRevert("vm.randomInt: number of bits cannot exceed 256");
+        vm._expectCheatcodeRevert("vm.randomInt: number of bits cannot exceed 256");
         int256 val = vm.randomInt(type(uint256).max);
 
         val = vm.randomInt(128);
@@ -31,7 +31,7 @@ contract RandomCheatcodesTest is DSTest {
     }
 
     function test_randomUintLimit() public {
-        vm.expectRevert("vm.randomUint: number of bits cannot exceed 256");
+        vm._expectCheatcodeRevert("vm.randomUint: number of bits cannot exceed 256");
         uint256 val = vm.randomUint(type(uint256).max);
     }
 
@@ -67,7 +67,7 @@ contract RandomBytesTest is DSTest {
     }
 
     function test_symbolic_bytes_revert() public {
-        vm.expectRevert();
+        vm._expectCheatcodeRevert();
         bytes memory val = vm.randomBytes(type(uint256).max);
     }
 

testdata/default/repros/Issue7238.t.sol

@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: MIT OR Apache-2.0
+pragma solidity ^0.8.18;
+
+import "ds-test/test.sol";
+import "cheats/Vm.sol";
+
+contract Reverter {
+    function doNotRevert() public {}
+
+    function revertWithMessage(string calldata message) public {
+        revert(message);
+    }
+}
+
+// https://github.com/foundry-rs/foundry/issues/8639
+contract Issue7238Test is DSTest {
+    Vm constant vm = Vm(HEVM_ADDRESS);
+    function testExpectRevertString() public {
+        Reverter reverter = new Reverter();
+        vm.expectRevert("revert");
+        reverter.revertWithMessage("revert");
+    }
+
+    // FAIL
+    function testFailRevertNotOnImmediateNextCall() public {
+        Reverter reverter = new Reverter();
+        // expectRevert should only work for the next call. However,
+        // we do not inmediately revert, so,
+        // we fail.
+        vm.expectRevert("revert");
+        reverter.doNotRevert();
+        reverter.revertWithMessage("revert");
+    }
+
+    // FAIL
+    function testFailCheatcodeRevert() public {
+        // This expectRevert is hanging, as the next cheatcode call is ignored.
+        vm.expectRevert();
+        vm.fsMetadata("something/something"); // try to go to some non-existent path to cause a revert
+    }
+
+    function testFailEarlyRevert() public {
+        vm.expectRevert();
+        rever();
+    }
+
+    function rever() internal {
+        revert();
+    }
+}

testdata/default/repros/Issue7457.t.sol

@@ -61,9 +61,8 @@ contract Issue7457Test is DSTest, ITarget {
         target.emitAnonymousEventEmpty();
     }
 
-    function testEmitEventNonIndexedReverts() public {
+    function testFailEmitEventNonIndexedReverts() public {
         vm.expectEmit(false, false, false, true);
-        vm.expectRevert("use vm.expectEmitAnonymous to match anonymous events");
         emit AnonymousEventNonIndexed(1);
     }
 

testdata/paris/cheats/LastCallGas.t.sol

@@ -39,7 +39,7 @@ abstract contract LastCallGasFixture is DSTest {
     }
 
     function testRevertNoCachedLastCallGas() public {
-        vm.expectRevert();
+        vm._expectCheatcodeRevert();
         vm.lastCallGas();
     }