Skip to content

Commit 284b0fc

Browse files
grandizzygrandizzy
committed
fix(cheatcode): expect revert only for calls with greater depth
1 parent 59f354c commit 284b0fc

File tree

11 files changed

+132
-48
lines changed

11 files changed

+132
-48
lines changed

‎crates/cheatcodes/src/inspector.rs

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ use revm::{
5454
};
5555
use serde_json::Value;
5656
use std::{
57+
cmp::max,
5758
collections::{BTreeMap, VecDeque},
5859
fs::File,
5960
io::BufReader,
@@ -1176,6 +1177,11 @@ impl Inspector<&mut dyn DatabaseExt> for Cheatcodes {
11761177
if self.gas_metering.paused {
11771178
self.gas_metering.paused_frames.push(interpreter.gas);
11781179
}
1180+
1181+
// `expectRevert`: track the max call depth during `expectRevert`
1182+
if let Some(ref mut expected) = self.expected_revert {
1183+
expected.reverted_depth = max(ecx.journaled_state.depth(), expected.reverted_depth);
1184+
}
11791185
}
11801186

11811187
#[inline]
@@ -1302,21 +1308,17 @@ impl Inspector<&mut dyn DatabaseExt> for Cheatcodes {
13021308

13031309
// Handle expected reverts.
13041310
if let Some(expected_revert) = &mut self.expected_revert {
1305-
// Record current reverter address before processing the expect revert if call reverted,
1306-
// expect revert is set with expected reverter address and no actual reverter set yet.
1307-
if outcome.result.is_revert() &&
1308-
expected_revert.reverter.is_some() &&
1309-
expected_revert.reverted_by.is_none()
1310-
{
1311-
expected_revert.reverted_by = Some(call.target_address);
1312-
} else if outcome.result.is_revert() &&
1313-
expected_revert.reverter.is_some() &&
1314-
expected_revert.reverted_by.is_some() &&
1315-
expected_revert.count > 1
1316-
{
1317-
// If we're expecting more than one revert, we need to reset the reverted_by address
1318-
// to latest reverter.
1319-
expected_revert.reverted_by = Some(call.target_address);
1311+
// Record current reverter address and call scheme before processing the expect revert
1312+
// if call reverted.
1313+
if outcome.result.is_revert() {
1314+
// Record current reverter address if expect revert is set with expected reverter
1315+
// address and no actual reverter was set yet or if we're expecting more than one
1316+
// revert.
1317+
if expected_revert.reverter.is_some() &&
1318+
(expected_revert.reverted_by.is_none() || expected_revert.count > 1)
1319+
{
1320+
expected_revert.reverted_by = Some(call.target_address);
1321+
}
13201322
}
13211323

13221324
if ecx.journaled_state.depth() <= expected_revert.depth {

‎crates/cheatcodes/src/test/expect.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,8 @@ pub struct ExpectedRevert {
8787
pub reverter: Option<Address>,
8888
/// Actual reverter of the call.
8989
pub reverted_by: Option<Address>,
90+
/// Max depth at which call reverted. Filled in by inspector during next call execution.
91+
pub reverted_depth: u64,
9092
/// Number of times this revert is expected.
9193
pub count: u64,
9294
/// Actual number of times this revert has been seen.
@@ -774,6 +776,7 @@ fn expect_revert(
774776
partial_match,
775777
reverter,
776778
reverted_by: None,
779+
reverted_depth: 0,
777780
count,
778781
actual_count: 0,
779782
});
@@ -806,6 +809,13 @@ pub(crate) fn handle_expect_revert(
806809
hex::encode_prefixed(data)
807810
};
808811

812+
if !is_cheatcode {
813+
ensure!(
814+
expected_revert.reverted_depth > expected_revert.depth,
815+
"call didn't revert at a lower depth than cheatcode call depth"
816+
);
817+
}
818+
809819
if expected_revert.count == 0 {
810820
if expected_revert.reverter.is_none() && expected_revert.reason.is_none() {
811821
ensure!(

‎crates/forge/tests/it/repros.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -389,3 +389,6 @@ test_repro!(8971; |config| {
389389

390390
// https://github.com/foundry-rs/foundry/issues/8639
391391
test_repro!(8639);
392+
393+
// https://github.com/foundry-rs/foundry/issues/7238
394+
test_repro!(7238);

‎testdata/default/cheats/AttachDelegation.t.sol

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ contract AttachDelegationTest is DSTest {
8686
assertEq(token.balanceOf(bob), 200);
8787
}
8888

89-
function testAttachDelegationRevertInvalidSignature() public {
89+
function testFailAttachDelegationRevertInvalidSignature() public {
9090
Vm.SignedDelegation memory signedDelegation = vm.signDelegation(address(implementation), alice_pk);
9191
// change v from 1 to 0
9292
signedDelegation.v = (signedDelegation.v + 1) % 2;
@@ -98,7 +98,6 @@ contract AttachDelegationTest is DSTest {
9898

9999
vm.broadcast(alice_pk);
100100
// empty revert because no bytecode was set to Alice's account
101-
vm.expectRevert();
102101
SimpleDelegateContract(alice).execute(calls);
103102
}
104103

@@ -109,7 +108,7 @@ contract AttachDelegationTest is DSTest {
109108
// send tx to increment alice's nonce
110109
token.mint(1, bob);
111110

112-
vm.expectRevert("vm.attachDelegation: invalid nonce");
111+
vm._expectCheatcodeRevert("vm.attachDelegation: invalid nonce");
113112
vm.attachDelegation(signedDelegation);
114113
}
115114

‎testdata/default/cheats/BroadcastRawTransaction.t.sol

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,8 +117,8 @@ contract BroadcastRawTransactionTest is DSTest {
117117
assertEq(address(from).balance, balance - (gasPrice * 21_000) - amountSent);
118118
assertEq(address(to).balance, amountSent);
119119

120-
vm.expectRevert();
121-
assert(3 == 4);
120+
vm._expectCheatcodeRevert();
121+
vm.assertFalse(true);
122122
}
123123

124124
function test_execute_multiple_signed_tx() public {

‎testdata/default/cheats/MemSafety.t.sol

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -413,11 +413,8 @@ contract MemSafetyTest is DSTest {
413413

414414
/// @dev Tests that expanding memory outside of the range given to `expectSafeMemory`
415415
/// will cause the test to fail while using the `MLOAD` opcode.
416-
function testExpectSafeMemory_MLOAD_REVERT() public {
416+
function testFailExpectSafeMemory_MLOAD_REVERT() public {
417417
vm.expectSafeMemory(0x80, 0x100);
418-
419-
vm.expectRevert();
420-
421418
// This should revert. Ugly hack to make sure the mload isn't optimized
422419
// out.
423420
uint256 a;
@@ -504,9 +501,8 @@ contract MemSafetyTest is DSTest {
504501

505502
/// @dev Tests that expanding memory outside of the range given to `expectSafeMemory`
506503
/// will cause the test to fail while using the `LOG0` opcode.
507-
function testExpectSafeMemory_LOG0_REVERT() public {
504+
function testFailExpectSafeMemory_LOG0_REVERT() public {
508505
vm.expectSafeMemory(0x80, 0x100);
509-
vm.expectRevert();
510506
// This should revert.
511507
assembly {
512508
log0(0x100, 0x20)

‎testdata/default/cheats/MockCall.t.sol

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -201,8 +201,11 @@ contract MockCallRevertTest is DSTest {
201201

202202
// post-mock
203203
assertEq(target.numberA(), 1);
204-
vm.expectRevert();
205-
target.numberB();
204+
try target.numberB() {
205+
revert();
206+
} catch (bytes memory err) {
207+
require(keccak256(err) == keccak256(ERROR_MESSAGE));
208+
}
206209
}
207210

208211
function testMockRevertWithCustomError() public {
@@ -216,8 +219,11 @@ contract MockCallRevertTest is DSTest {
216219
vm.mockCallRevert(address(target), abi.encodeWithSelector(target.numberB.selector), customError);
217220

218221
assertEq(target.numberA(), 1);
219-
vm.expectRevert(customError);
220-
target.numberB();
222+
try target.numberB() {
223+
revert();
224+
} catch (bytes memory err) {
225+
require(keccak256(err) == keccak256(customError));
226+
}
221227
}
222228

223229
function testMockNestedRevert() public {
@@ -228,8 +234,11 @@ contract MockCallRevertTest is DSTest {
228234

229235
vm.mockCallRevert(address(inner), abi.encodeWithSelector(inner.numberB.selector), ERROR_MESSAGE);
230236

231-
vm.expectRevert(ERROR_MESSAGE);
232-
target.sum();
237+
try target.sum() {
238+
revert();
239+
} catch (bytes memory err) {
240+
require(keccak256(err) == keccak256(ERROR_MESSAGE));
241+
}
233242
}
234243

235244
function testMockCalldataRevert() public {
@@ -241,8 +250,11 @@ contract MockCallRevertTest is DSTest {
241250

242251
assertEq(target.add(6, 4), 10);
243252

244-
vm.expectRevert(ERROR_MESSAGE);
245-
target.add(5, 5);
253+
try target.add(5, 5) {
254+
revert();
255+
} catch (bytes memory err) {
256+
require(keccak256(err) == keccak256(ERROR_MESSAGE));
257+
}
246258
}
247259

248260
function testClearMockRevertedCalls() public {
@@ -263,8 +275,11 @@ contract MockCallRevertTest is DSTest {
263275

264276
assertEq(mock.add(1, 2), 3);
265277

266-
vm.expectRevert(ERROR_MESSAGE);
267-
mock.add(2, 3);
278+
try mock.add(2, 3) {
279+
revert();
280+
} catch (bytes memory err) {
281+
require(keccak256(err) == keccak256(ERROR_MESSAGE));
282+
}
268283
}
269284

270285
function testMockCallRevertWithValue() public {
@@ -275,8 +290,11 @@ contract MockCallRevertTest is DSTest {
275290
assertEq(mock.pay(1), 1);
276291
assertEq(mock.pay(2), 2);
277292

278-
vm.expectRevert(ERROR_MESSAGE);
279-
mock.pay{value: 10}(1);
293+
try mock.pay{value: 10}(1) {
294+
revert();
295+
} catch (bytes memory err) {
296+
require(keccak256(err) == keccak256(ERROR_MESSAGE));
297+
}
280298
}
281299

282300
function testMockCallResetsMockCallRevert() public {
@@ -296,8 +314,11 @@ contract MockCallRevertTest is DSTest {
296314

297315
vm.mockCallRevert(address(mock), abi.encodeWithSelector(mock.add.selector), ERROR_MESSAGE);
298316

299-
vm.expectRevert(ERROR_MESSAGE);
300-
mock.add(2, 3);
317+
try mock.add(2, 3) {
318+
revert();
319+
} catch (bytes memory err) {
320+
require(keccak256(err) == keccak256(ERROR_MESSAGE));
321+
}
301322
}
302323

303324
function testMockCallRevertWithCall() public {
@@ -317,7 +338,10 @@ contract MockCallRevertTest is DSTest {
317338

318339
vm.mockCallRevert(address(mock), abi.encodeWithSelector(mock.add.selector), ERROR_MESSAGE);
319340

320-
vm.expectRevert(ERROR_MESSAGE);
321-
mock.add(1, 2);
341+
try mock.add(2, 3) {
342+
revert();
343+
} catch (bytes memory err) {
344+
require(keccak256(err) == keccak256(ERROR_MESSAGE));
345+
}
322346
}
323347
}

‎testdata/default/cheats/RandomCheatcodes.t.sol

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ contract RandomCheatcodesTest is DSTest {
1111
int128 constant max = 170141183460469231731687303715884105727;
1212

1313
function test_int128() public {
14-
vm.expectRevert("vm.randomInt: number of bits cannot exceed 256");
14+
vm._expectCheatcodeRevert("vm.randomInt: number of bits cannot exceed 256");
1515
int256 val = vm.randomInt(type(uint256).max);
1616

1717
val = vm.randomInt(128);
@@ -31,7 +31,7 @@ contract RandomCheatcodesTest is DSTest {
3131
}
3232

3333
function test_randomUintLimit() public {
34-
vm.expectRevert("vm.randomUint: number of bits cannot exceed 256");
34+
vm._expectCheatcodeRevert("vm.randomUint: number of bits cannot exceed 256");
3535
uint256 val = vm.randomUint(type(uint256).max);
3636
}
3737

@@ -67,7 +67,7 @@ contract RandomBytesTest is DSTest {
6767
}
6868

6969
function test_symbolic_bytes_revert() public {
70-
vm.expectRevert();
70+
vm._expectCheatcodeRevert();
7171
bytes memory val = vm.randomBytes(type(uint256).max);
7272
}
7373

‎testdata/default/repros/Issue7238.t.sol

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// SPDX-License-Identifier: MIT OR Apache-2.0
2+
pragma solidity ^0.8.18;
3+
4+
import "ds-test/test.sol";
5+
import "cheats/Vm.sol";
6+
7+
contract Reverter {
8+
function doNotRevert() public {}
9+
10+
function revertWithMessage(string calldata message) public {
11+
revert(message);
12+
}
13+
}
14+
15+
// https://github.com/foundry-rs/foundry/issues/7238
16+
contract Issue7238Test is DSTest {
17+
Vm constant vm = Vm(HEVM_ADDRESS);
18+
19+
function testExpectRevertString() public {
20+
Reverter reverter = new Reverter();
21+
vm.expectRevert("revert");
22+
reverter.revertWithMessage("revert");
23+
}
24+
25+
// FAIL
26+
function testFailRevertNotOnImmediateNextCall() public {
27+
Reverter reverter = new Reverter();
28+
// expectRevert should only work for the next call. However,
29+
// we do not inmediately revert, so,
30+
// we fail.
31+
vm.expectRevert("revert");
32+
reverter.doNotRevert();
33+
reverter.revertWithMessage("revert");
34+
}
35+
36+
// FAIL
37+
function testFailCheatcodeRevert() public {
38+
// This expectRevert is hanging, as the next cheatcode call is ignored.
39+
vm.expectRevert();
40+
vm.fsMetadata("something/something"); // try to go to some non-existent path to cause a revert
41+
}
42+
43+
function testFailEarlyRevert() public {
44+
vm.expectRevert();
45+
rever();
46+
}
47+
48+
function rever() internal {
49+
revert();
50+
}
51+
}

‎testdata/default/repros/Issue7457.t.sol

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,8 @@ contract Issue7457Test is DSTest, ITarget {
6161
target.emitAnonymousEventEmpty();
6262
}
6363

64-
function testEmitEventNonIndexedReverts() public {
64+
function testFailEmitEventNonIndexedReverts() public {
6565
vm.expectEmit(false, false, false, true);
66-
vm.expectRevert("use vm.expectEmitAnonymous to match anonymous events");
6766
emit AnonymousEventNonIndexed(1);
6867
}
6968

0 commit comments

Comments
 (0)