diff --git a/crates/cheatcodes/src/inspector.rs b/crates/cheatcodes/src/inspector.rs index 329b89d0ebc2f..56e73669853d5 100644 --- a/crates/cheatcodes/src/inspector.rs +++ b/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, diff --git a/crates/cheatcodes/src/test/expect.rs b/crates/cheatcodes/src/test/expect.rs index a3ddef8c16c97..e1bdf72daa76c 100644 --- a/crates/cheatcodes/src/test/expect.rs +++ b/crates/cheatcodes/src/test/expect.rs @@ -87,6 +87,8 @@ pub struct ExpectedRevert { pub reverter: Option
, /// Actual reverter of the call. pub reverted_by: Option
, + /// 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!( diff --git a/crates/forge/tests/it/repros.rs b/crates/forge/tests/it/repros.rs index 2a47d3d3efd16..7a9661dca43d5 100644 --- a/crates/forge/tests/it/repros.rs +++ b/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); diff --git a/testdata/default/cheats/AttachDelegation.t.sol b/testdata/default/cheats/AttachDelegation.t.sol index 7befc9a32047c..1b8ea73a2b1b1 100644 --- a/testdata/default/cheats/AttachDelegation.t.sol +++ b/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); } diff --git a/testdata/default/cheats/BroadcastRawTransaction.t.sol b/testdata/default/cheats/BroadcastRawTransaction.t.sol index 5bd400a9f71e3..36682bc893359 100644 --- a/testdata/default/cheats/BroadcastRawTransaction.t.sol +++ b/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 { diff --git a/testdata/default/cheats/MemSafety.t.sol b/testdata/default/cheats/MemSafety.t.sol index a5c0a5a4ff614..78d8fff5d4c0c 100644 --- a/testdata/default/cheats/MemSafety.t.sol +++ b/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) diff --git a/testdata/default/cheats/MockCall.t.sol b/testdata/default/cheats/MockCall.t.sol index f85e9c8239bdc..f11fd20984571 100644 --- a/testdata/default/cheats/MockCall.t.sol +++ b/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)); + } } } diff --git a/testdata/default/cheats/RandomCheatcodes.t.sol b/testdata/default/cheats/RandomCheatcodes.t.sol index beeee9862bbb6..4c3e1fffdfde0 100644 --- a/testdata/default/cheats/RandomCheatcodes.t.sol +++ b/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); } diff --git a/testdata/default/repros/Issue7238.t.sol b/testdata/default/repros/Issue7238.t.sol new file mode 100644 index 0000000000000..be806ea81babb --- /dev/null +++ b/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(); + } +} diff --git a/testdata/default/repros/Issue7457.t.sol b/testdata/default/repros/Issue7457.t.sol index 1836c48254d55..e0574cbf8fdb8 100644 --- a/testdata/default/repros/Issue7457.t.sol +++ b/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); } diff --git a/testdata/paris/cheats/LastCallGas.t.sol b/testdata/paris/cheats/LastCallGas.t.sol index bc7ac42639505..23f6df224963f 100644 --- a/testdata/paris/cheats/LastCallGas.t.sol +++ b/testdata/paris/cheats/LastCallGas.t.sol @@ -39,7 +39,7 @@ abstract contract LastCallGasFixture is DSTest { } function testRevertNoCachedLastCallGas() public { - vm.expectRevert(); + vm._expectCheatcodeRevert(); vm.lastCallGas(); }