From 93aec0327248dc074a95f568780c86da345cab87 Mon Sep 17 00:00:00 2001 From: grandizzy Date: Wed, 11 Dec 2024 16:23:05 +0200 Subject: [PATCH 1/2] fix(cheatcode): expect revert only for calls with greater depth --- crates/cheatcodes/src/inspector.rs | 32 ++++++----- crates/cheatcodes/src/test/expect.rs | 10 ++++ crates/forge/tests/it/repros.rs | 3 + .../default/cheats/AttachDelegation.t.sol | 5 +- .../cheats/BroadcastRawTransaction.t.sol | 4 +- testdata/default/cheats/MemSafety.t.sol | 8 +-- testdata/default/cheats/MockCall.t.sol | 56 +++++++++++++------ .../default/cheats/RandomCheatcodes.t.sol | 6 +- testdata/default/repros/Issue7238.t.sol | 51 +++++++++++++++++ testdata/default/repros/Issue7457.t.sol | 3 +- testdata/paris/cheats/LastCallGas.t.sol | 2 +- 11 files changed, 132 insertions(+), 48 deletions(-) create mode 100644 testdata/default/repros/Issue7238.t.sol diff --git a/crates/cheatcodes/src/inspector.rs b/crates/cheatcodes/src/inspector.rs index 329b89d0ebc2..b819cd12cad6 100644 --- a/crates/cheatcodes/src/inspector.rs +++ b/crates/cheatcodes/src/inspector.rs @@ -54,6 +54,7 @@ use revm::{ }; use serde_json::Value; use std::{ + cmp::max, collections::{BTreeMap, VecDeque}, fs::File, io::BufReader, @@ -1176,6 +1177,11 @@ impl Inspector<&mut dyn DatabaseExt> for Cheatcodes { if self.gas_metering.paused { self.gas_metering.paused_frames.push(interpreter.gas); } + + // `expectRevert`: track the max call depth during `expectRevert` + if let Some(ref mut expected) = self.expected_revert { + expected.max_depth = max(ecx.journaled_state.depth(), expected.max_depth); + } } #[inline] @@ -1302,21 +1308,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 { diff --git a/crates/cheatcodes/src/test/expect.rs b/crates/cheatcodes/src/test/expect.rs index a3ddef8c16c9..af9c397a8dc3 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 call depth reached during next call execution. + pub max_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, + max_depth: depth, count, actual_count: 0, }); @@ -806,6 +809,13 @@ pub(crate) fn handle_expect_revert( hex::encode_prefixed(data) }; + if !is_cheatcode { + ensure!( + expected_revert.max_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 2a47d3d3efd1..7a9661dca43d 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 7befc9a32047..1b8ea73a2b1b 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 5bd400a9f71e..36682bc89335 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 a5c0a5a4ff61..78d8fff5d4c0 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 f85e9c8239bd..f11fd2098457 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 beeee9862bbb..4c3e1fffdfde 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 000000000000..73befa3eaaab --- /dev/null +++ b/testdata/default/repros/Issue7238.t.sol @@ -0,0 +1,51 @@ +// 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/7238 +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 1836c48254d5..e0574cbf8fdb 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 bc7ac4263950..23f6df224963 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(); } From df77c40346947bdfb7a606a0a9f2756dab158f77 Mon Sep 17 00:00:00 2001 From: grandizzy Date: Thu, 12 Dec 2024 08:14:07 +0200 Subject: [PATCH 2/2] Add config to allow expect revert for internal calls --- crates/cheatcodes/src/config.rs | 4 ++++ crates/cheatcodes/src/inspector.rs | 2 ++ crates/cheatcodes/src/test/expect.rs | 4 +++- crates/config/src/lib.rs | 3 +++ crates/forge/tests/cli/config.rs | 1 + crates/test-utils/src/util.rs | 1 + testdata/default/cheats/AttachDelegation.t.sol | 4 +++- testdata/default/cheats/MemSafety.t.sol | 10 ++++++++-- testdata/default/repros/Issue7457.t.sol | 4 +++- 9 files changed, 28 insertions(+), 5 deletions(-) diff --git a/crates/cheatcodes/src/config.rs b/crates/cheatcodes/src/config.rs index e0463dfc4535..229adbb86252 100644 --- a/crates/cheatcodes/src/config.rs +++ b/crates/cheatcodes/src/config.rs @@ -57,6 +57,8 @@ pub struct CheatsConfig { pub assertions_revert: bool, /// Optional seed for the RNG algorithm. pub seed: Option, + /// Whether to allow `expectRevert` to work for internal calls. + pub internal_expect_revert: bool, } impl CheatsConfig { @@ -98,6 +100,7 @@ impl CheatsConfig { running_version, assertions_revert: config.assertions_revert, seed: config.fuzz.seed, + internal_expect_revert: config.allow_internal_expect_revert, } } @@ -239,6 +242,7 @@ impl Default for CheatsConfig { running_version: Default::default(), assertions_revert: true, seed: None, + internal_expect_revert: false, } } } diff --git a/crates/cheatcodes/src/inspector.rs b/crates/cheatcodes/src/inspector.rs index b819cd12cad6..7cc2bd52f2de 100644 --- a/crates/cheatcodes/src/inspector.rs +++ b/crates/cheatcodes/src/inspector.rs @@ -759,6 +759,7 @@ where { let handler_result = expect::handle_expect_revert( false, true, + self.config.internal_expect_revert, &mut expected_revert, outcome.result.result, outcome.result.output.clone(), @@ -1339,6 +1340,7 @@ impl Inspector<&mut dyn DatabaseExt> for Cheatcodes { let handler_result = expect::handle_expect_revert( cheatcode_call, false, + self.config.internal_expect_revert, &mut expected_revert, outcome.result.result, outcome.result.output.clone(), diff --git a/crates/cheatcodes/src/test/expect.rs b/crates/cheatcodes/src/test/expect.rs index af9c397a8dc3..b28756b90905 100644 --- a/crates/cheatcodes/src/test/expect.rs +++ b/crates/cheatcodes/src/test/expect.rs @@ -786,6 +786,7 @@ fn expect_revert( pub(crate) fn handle_expect_revert( is_cheatcode: bool, is_create: bool, + internal_expect_revert: bool, expected_revert: &mut ExpectedRevert, status: InstructionResult, retdata: Bytes, @@ -809,7 +810,8 @@ pub(crate) fn handle_expect_revert( hex::encode_prefixed(data) }; - if !is_cheatcode { + // Check depths if it's not an expect cheatcode call and if internal expect reverts not enabled. + if !is_cheatcode && !internal_expect_revert { ensure!( expected_revert.max_depth > expected_revert.depth, "call didn't revert at a lower depth than cheatcode call depth" diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 805aaf7cd163..f4dcd5180f50 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -315,6 +315,8 @@ pub struct Config { pub invariant: InvariantConfig, /// Whether to allow ffi cheatcodes in test pub ffi: bool, + /// Whether to allow `expectRevert` for internal functions. + pub allow_internal_expect_revert: bool, /// Use the create 2 factory in all cases including tests and non-broadcasting scripts. pub always_use_create_2_factory: bool, /// Sets a timeout in seconds for vm.prompt cheatcodes @@ -2310,6 +2312,7 @@ impl Default for Config { invariant: InvariantConfig::new("cache/invariant".into()), always_use_create_2_factory: false, ffi: false, + allow_internal_expect_revert: false, prompt_timeout: 120, sender: Self::DEFAULT_SENDER, tx_origin: Self::DEFAULT_SENDER, diff --git a/crates/forge/tests/cli/config.rs b/crates/forge/tests/cli/config.rs index 545cebac87a8..201315193648 100644 --- a/crates/forge/tests/cli/config.rs +++ b/crates/forge/tests/cli/config.rs @@ -87,6 +87,7 @@ forgetest!(can_extract_config_values, |prj, cmd| { ..Default::default() }, ffi: true, + allow_internal_expect_revert: false, always_use_create_2_factory: false, prompt_timeout: 0, sender: "00a329c0648769A73afAc7F9381D08FB43dBEA72".parse().unwrap(), diff --git a/crates/test-utils/src/util.rs b/crates/test-utils/src/util.rs index e7410ed607ed..17bffcb6451a 100644 --- a/crates/test-utils/src/util.rs +++ b/crates/test-utils/src/util.rs @@ -207,6 +207,7 @@ impl ExtTester { test_cmd.env("FOUNDRY_FORK_BLOCK_NUMBER", fork_block.to_string()); } test_cmd.env("FOUNDRY_INVARIANT_DEPTH", "15"); + test_cmd.env("FOUNDRY_ALLOW_INTERNAL_EXPECT_REVERT", "true"); test_cmd.assert_success(); } diff --git a/testdata/default/cheats/AttachDelegation.t.sol b/testdata/default/cheats/AttachDelegation.t.sol index 1b8ea73a2b1b..2b2e829ae350 100644 --- a/testdata/default/cheats/AttachDelegation.t.sol +++ b/testdata/default/cheats/AttachDelegation.t.sol @@ -86,7 +86,8 @@ contract AttachDelegationTest is DSTest { assertEq(token.balanceOf(bob), 200); } - function testFailAttachDelegationRevertInvalidSignature() public { + /// forge-config: default.allow_internal_expect_revert = true + function testAttachDelegationRevertInvalidSignature() public { Vm.SignedDelegation memory signedDelegation = vm.signDelegation(address(implementation), alice_pk); // change v from 1 to 0 signedDelegation.v = (signedDelegation.v + 1) % 2; @@ -98,6 +99,7 @@ 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); } diff --git a/testdata/default/cheats/MemSafety.t.sol b/testdata/default/cheats/MemSafety.t.sol index 78d8fff5d4c0..2093c20fd56e 100644 --- a/testdata/default/cheats/MemSafety.t.sol +++ b/testdata/default/cheats/MemSafety.t.sol @@ -413,8 +413,12 @@ 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 testFailExpectSafeMemory_MLOAD_REVERT() public { + /// forge-config: default.allow_internal_expect_revert = true + function testExpectSafeMemory_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; @@ -501,8 +505,10 @@ 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 testFailExpectSafeMemory_LOG0_REVERT() public { + /// forge-config: default.allow_internal_expect_revert = true + function testExpectSafeMemory_LOG0_REVERT() public { vm.expectSafeMemory(0x80, 0x100); + vm.expectRevert(); // This should revert. assembly { log0(0x100, 0x20) diff --git a/testdata/default/repros/Issue7457.t.sol b/testdata/default/repros/Issue7457.t.sol index e0574cbf8fdb..d95f79c4835f 100644 --- a/testdata/default/repros/Issue7457.t.sol +++ b/testdata/default/repros/Issue7457.t.sol @@ -61,8 +61,10 @@ contract Issue7457Test is DSTest, ITarget { target.emitAnonymousEventEmpty(); } - function testFailEmitEventNonIndexedReverts() public { + /// forge-config: default.allow_internal_expect_revert = true + function testEmitEventNonIndexedReverts() public { vm.expectEmit(false, false, false, true); + vm.expectRevert("use vm.expectEmitAnonymous to match anonymous events"); emit AnonymousEventNonIndexed(1); }