More versatile expectRevert

[adhusson]

Component

Forge

Describe the feature you would like

It would be useful to have more control over which call expectRevert applies to. Sometimes I don't want to expect the next call to revert but one a little later. And it's not that simple to insert my expectRevert between the upcoming call and the call I expect to revert.

Possible solutions

1. New cheat code: vm.wontRevert(), which tells forge to ignore the next call for the purpose of expectRevert.

2. Enrich expectRevert: vm.expectRevert(bytes|bytes4 input, msg) where you expect a revert on the next call where the first argument matches the calldata, eg vm.expectRevert(ERC20.transferFrom.selector,"lowAllowance").

Additional context

No response

[gakonst]

It would be useful to have more control over which call expectRevert applies to. Sometimes I don't want to expect the next call to revert but one a little later. And it's not that simple to insert my expectRevert between the upcoming call and the call I expect to revert.

Can you give a specific example of the scenario?

[adhusson]

Sure, in my case I have a contract that self-calls at some point. I'd like to see that self-call fail.

[onbjerg]

I don't understand - you have something like:

A -> (1) B -> (2) B and you would like to test if (2) reverts?

[adhusson]

Sorry I don't think I get your diagram. I have something like

contract A is Something { 
  function f() public { 
    try this.g() { ... } catch { ... } 
  } 
}
contract ATest is Test { 
  function test_f() public {
    // here I'd like to expect a revert from the g() call
    (new A()).f();
  }
}

I ended up testing other behaviors around the revert, and you could even say I shouldn't test a nested revert, but it would be nice.

[onbjerg]

Yeah that's essentially what I meant - I don't think this is super feasible because of e.g. #864 (which this is sort of a dupe of?) and #432. Both of them are really hard to implement, and likely to break in many situations for very opaque reasons. Imo, since you catch the revert, it should be sufficient to test the state/output in the case it doesn't revert, and test the opposite as well (in the case it does revert)

Another thing is that under the hood expectRevert actually "inverts" the status of the call in order to allow for the test to continue execution. That is, when you perform a call (static, delegate, normal etc.) it returns a status of either 0 or 1, which Forge inverts. For internal calls, there is no status, so it's unclear how this would work - we'd essentially need to coax the execution to jump out to another point in the code, which might not be feasible with static analysis alone

[adhusson]

AFAICT my use case does not involve "internal calls" but actual CALL opcodes -- this.g() compiles to a CALL, not a JUMP. So implementation-wise it should just be a matter of waiting until the CALL meets some conditions about (from, to, calldata) before inverting the status. It may not be wise to implement it though :p

[onbjerg]

Yeah, as mentioned it works in some cases, but I have definitely seen cases where jumps were used, and it's not entirely clear when that happens and the support (bug fixing, answering questions/helping people) would probably be a lot since the error isn't necessarily immediately clear :)

[adhusson]

I don't think a this.(...) ever compiles to a jump per this but I may be misreading.

Regardless, the feature I'm talking about here would strictly be about being able to qualify which next CALL to invert. For instance vm.expectRevert(bytes4 sig) would invert the result of the next CALL where the first 4 bytes of calldata match sig.

[adhusson]

I have another use case right now. I'm testing contract Callee. The context is that Callee will be called (after a sequence of state-changing operations) by a contract Caller (which I'm not testing). So the order is tx.origin -> Caller -> Callee.

I'd like to check that some transaction makes Callee revert, and I'd rather test that directly than test it through Caller's behavior. So something like this would be useful:

vm.expectRevert(address(callee),"revert reason");
caller.fn();

[onbjerg]

That's true, however the important bit is this - if you just do g() (which is valid), this is no longer always the case, ref #432 about mockCall which works similarly (structurally) to expectRevert. Same for internal calls

[adhusson]

Agreed, managing internal calls seems difficult. Doing something similar to expectEmit but for the sequence of CALLs that follow an expectRevert seems less hard?

[mds1]

A lot has been discussed, and I think:

• Some pars of the discussion have workarounds
• Others are covered by supporting JUMPs which will be tracked in meta(cheatcodes): tracking issue for cheatcode improvements #4439

So going to close this optimistically, but if there's a feature request here we still want to track feel free to create a new issue describing the scope + sample UX

[adhusson]

I'm not sure how this PR is addressed by supporting JUMPs nor what workarounds there are are the moment. I'll try to clarify: for regular reverts, from call frame to call frame, you sometimes want to specify that a revert happens deep in the call frame stack.

For instance :

// test contract would like to expect a revert from c2
| test contract
| - c1.fun(); 
     | - ...<code>...   
     | - c2.fun();
         | - revert("reason");

The only potential workaround here is to do vm.prank(address(c1)); c2.fun();, but this stops working very quickly if the c2.fun() call inside c1 is surrounded by state-changing context that you'd have to replicate in the test code.

The simplest UX I can think of (it doesn't handle all use cases but a lot) is to enrich expectRevert:

// expect next call with given calldata input to revert, optionally with reason
vm.expectRevert(bytes|bytes4 input, [reason])
// expect next call to address to revert, optionally with calldata input, optionally with reason
vm.expectRevert(address,[bytes|bytes4 input, [reason]])

This is useful to pseudo-integration tests where you are testing either c1 or c2, and you want to check that their interaction results in a revert under some conditions.

[mds1]

I'm not sure how this PR is addressed by supporting JUMPs

Sorry, saw some discussion above about reverts for internal function calls (which are JUMPs), but I see that's not the idea here

nor what workarounds there are are the moment.

One I had in mind was the vm.prank(address(c1)); c2.fun(); workaround which you mentioned. Additionally, you can already catch reverts in cases like that:

import "forge-std/Test.sol";

contract Foo {
  function foo() public {
    revert("oops");
  }
}

contract Bar {
  Foo foo;
  constructor() {
    foo = new Foo();
  }

  function bar() public {
    foo.foo();
  }
}

contract MyTest is Test {
  // This test passes.
  function testCounter() public {
    Bar bar = new Bar();
    vm.expectRevert(bytes("oops"));
    bar.bar();
  }
}

This is useful to pseudo-integration tests where you are testing either c1 or c2, and you want to check that their interaction results in a revert under some conditions.

I think I'm not following the use case and what you can't do currently. What does the above example—where a call to bar results in a revert in foo, which we test for and catch—not handle? Based on your cheat syntaxes, it seems you can just move the expectRevert call later in your test method, so it expects the revert on the next call, instead of having expectRevert earlier in the test and waiting for the target or calldata conditions to be met