Skip to content

Commit

Permalink
fix(cheatcode): expect revert only for calls with greater depth
Browse files Browse the repository at this point in the history
  • Loading branch information
grandizzy committed Dec 11, 2024
1 parent 59f354c commit 93aec03
Show file tree
Hide file tree
Showing 11 changed files with 132 additions and 48 deletions.
32 changes: 17 additions & 15 deletions crates/cheatcodes/src/inspector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ use revm::{
};
use serde_json::Value;
use std::{
cmp::max,
collections::{BTreeMap, VecDeque},
fs::File,
io::BufReader,
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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 {
Expand Down
10 changes: 10 additions & 0 deletions crates/cheatcodes/src/test/expect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ pub struct ExpectedRevert {
pub reverter: Option<Address>,
/// Actual reverter of the call.
pub reverted_by: Option<Address>,
/// 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.
Expand Down Expand Up @@ -774,6 +776,7 @@ fn expect_revert(
partial_match,
reverter,
reverted_by: None,
max_depth: depth,
count,
actual_count: 0,
});
Expand Down Expand Up @@ -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!(
Expand Down
3 changes: 3 additions & 0 deletions crates/forge/tests/it/repros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
5 changes: 2 additions & 3 deletions testdata/default/cheats/AttachDelegation.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
}

Expand All @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions testdata/default/cheats/BroadcastRawTransaction.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 2 additions & 6 deletions testdata/default/cheats/MemSafety.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down
56 changes: 40 additions & 16 deletions testdata/default/cheats/MockCall.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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));
}
}
}
6 changes: 3 additions & 3 deletions testdata/default/cheats/RandomCheatcodes.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
}

Expand Down
51 changes: 51 additions & 0 deletions testdata/default/repros/Issue7238.t.sol
Original file line number Diff line number Diff line change
@@ -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();
}
}
3 changes: 1 addition & 2 deletions testdata/default/repros/Issue7457.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion testdata/paris/cheats/LastCallGas.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ abstract contract LastCallGasFixture is DSTest {
}

function testRevertNoCachedLastCallGas() public {
vm.expectRevert();
vm._expectCheatcodeRevert();
vm.lastCallGas();
}

Expand Down

0 comments on commit 93aec03

Please sign in to comment.