feat(forge): exclude certain contracts from being eavesdropped on by vm.expectRevert

[PaulRBerg]

Component

Forge

Describe the feature you would like

Take the following test:

function test_RevertWhen_BatchSizeZero() external {
    Batch.CancelMultiple[] memory batch = new Batch.CancelMultiple[](0);
    vm.expectRevert(Errors.SablierV2ProxyTarget_BatchSizeZero.selector);
    bytes memory data = abi.encodeCall(target.batchCancelMultiple, (batch, defaults.assets()));
    proxy.execute(address(target), data);
}

The test does not pass because Forge expects a revert in the defaults.assets() call, which precedes the proxy.execute call. The catch is that defaults is a testing-only contract containing default values used throughout the tests.

contract Defaults {
    uint256 public constant BATCH_SIZE = 10;
    UD60x18 public constant BROKER_FEE = UD60x18.wrap(0);
    uint40 public constant CLIFF_DURATION = 2500 seconds;
    
    // --- snip --- //
}

This has to be a contract (rather than a library) because I need to calculate certain values dynamically in the constructor.

I know I could easily address the problem in my code above by flipping the order of operations. Still, I wanted to open this issue to suggest a feature to improve the developer experience when this scenario occurs.

It would be nice to have the ability to exclude certain contracts from being eavesdropped on by vm.expectRevert. This functionality would be analogous to excludeContract for invariants.

[PaulRBerg]

I offer a $200 bounty for implementing this feature. DM me on Telegram once you have the PR.

[mds1]

I think this needs to be spec'd out before a PR is opened :)

First it's worth noting this concept can apply to all expect* cheats, not just expectRevert, so we should consider that when deciding on behavior/UX/syntax.

Then there are a lot of questions about the behavior/UX/syntax:

• Should all expect* cheats have their own overloads that take arrays of addresses? This greatly increases the size of the cheatcode interface by adding 13 cheats
• Or instead of arrays, for better UX there can be overloads for between e.g. 1 and 10 addresses, which further increases the size of the cheatcode interface
• Maybe a generic vm.excludeFromExpectations method that takes an array of addresses (or many overloads, again) and excludes those from all expect* checks
• Should the exclusion apply just to the next expect* call, or to all calls until a vm.stopExclusionFromExpectations method is called?
• Should subsequent calls to the cheat be additive (i.e. append to the array of addresses), overwrite prior calls, or cause a revert?

[mds1]

Another idea is a cheat to enable disable expect* checks for staticcalls, because really the use case here most of the time is going to be skipping staticcalls (h/t @brockelmore)

[PaulRBerg]

All good points/ questions, thanks @mds1.

Should all expect* cheats have their own overloads that take arrays of addresses?

No. The contracts meant to be excluded are (at least in my projects) dummy testing contracts that are never meant to be tested, for anything.

take arrays of addresses?

That + a single address would be a good starting point.

for better UX there can be overloads for between e.g. 1 and 10 addresses

This is an optimization we can consider in future iterations of the feature/ PR.

Maybe a generic vm.excludeFromExpectations method

Sounds great!

Should the exclusion apply just to the next expect* call

All calls, as per my explanation in two paragraphs above.

Should subsequent calls to the cheat be additive

Additive, yes.

---

Two final notes:

• I think that the behavior of this exclusion functionality should mimic that of excludeContract in StdInvariant.
• As you pointed out, @brockelmore suggested an alternative way of implementing this feature (that would cover some use cases, at least) by providing a vm.expectRevert(data,skipStatic) overload that disregards all static calls in expectRevert.

[PaulRBerg]

The lack of this functionality has continued to be a pain.

CC @mattsse, @gakonst

[PaulRBerg]

As evidenced by this StackExchange Q&A, people keep bumping into hard-to-debug scenarios that could be prevented with a feature that would allow users to exclude certain contracts from being eavesdropped by vm.expectRevert.

[Zartaj0]

Thanks for mentioning the issue here @PaulRBerg.

[Zartaj0]

Another idea is a cheat to enable disable expect* checks for staticcalls, because really the use case here most of the time is going to be skipping staticcalls (h/t @brockelmore)

I think this would be the best solution.

[zerosnacks]

Supportive, also on this note

I think that the behavior of this exclusion functionality should mimic that of excludeContract in StdInvariant.

[grandizzy]

@PaulRBerg would proposed cheatcodes here #5299 (comment) accomplish desired behavior? thank you!

[PaulRBerg]

@grandizzy I don't think they would. From Matt's code:

If the revert data matches but the address that originated the revert does not, that is considered a failure.

I would like the test to not fail if the revert address does not match. This is what I mean by excluding certain contracts from being eavesdropped.

[grandizzy]

@grandizzy I don't think they would. From Matt's code:

If the revert data matches but the address that originated the revert does not, that is considered a failure.

I would like the test to not fail if the revert address does not match. This is what I mean by excluding certain contracts from being eavesdropped.

@PaulRBerg I actually think it helps, tested the following scenario using PR #8770 for 2 contracts like

contract CounterWithoutRevert {
    uint256 public number;
    function count() public {
        number += 1;
    }
}

contract CounterWithRevert {
    error RandomError();
    uint256 public number;
    function count() public pure {
        revert RandomError();
    }
}

a test as

function test_count_with_revert() public {
    CounterWithoutRevert countNoRevert = new CounterWithoutRevert();
    CounterWithRevert countRevert = new CounterWithRevert();
    vm.expectRevert(address(countRevert));
    countNoRevert.count();
    countNoRevert.count();
    countNoRevert.count();
    countRevert.count();
}

it's a pass. That is because when reverter address is specified vm.expectRevert(address(countRevert)); we're not looking for the next call to revert but rather for the next reverted call to be done by address(countRevert), I included this test in PR too, wdyt?

[PaulRBerg]

That is because when reverter address is specified vm.expectRevert(address(countRevert)); we're not looking for the next call to revert but rather for the next reverted call to be done by address(countRevert)

This wasn't clear from Matt's comment. Or I might have misunderstood it.

Anyway, in this case, yes this new feature accomplishes the desired behavior. Thank you!

[mds1]

it's a pass. That is because when reverter address is specified vm.expectRevert(address(countRevert)); we're not looking for the next call to revert but rather for the next reverted call to be done by address(countRevert), I included this test in PR too, wdyt?

@grandizzy Has this always been the case for expextRevert or is that new to this version? I thought, like other expect* cheats, that expectRevert wants the revert to happen on the next call? I think it makes more sense to keep that behavior consistent for all cheats and expect it to be the next call.

[grandizzy]

it's a pass. That is because when reverter address is specified vm.expectRevert(address(countRevert)); we're not looking for the next call to revert but rather for the next reverted call to be done by address(countRevert), I included this test in PR too, wdyt?

@grandizzy Has this always been the case for expextRevert or is that new to this version? I thought, like other expect* cheats, that expectRevert wants the revert to happen on the next call? I think it makes more sense to keep that behavior consistent for all cheats and expect it to be the next call.

@mds1 that's only with PR #8770 in review and with cheatcodes specifying reverter addresses as per #5299 (comment) in order to satisfy bubbling reverts.

I thought, like other expect* cheats, that expectRevert wants the revert to happen on the next call?

That still holds true but it should be read as expectRevert(reverterAddress) wants the revert to happen on the next call to reverterAddress

[mds1]

Does that mean the next top level to reverterAddress (i.e. call from the test contract), or a call at any depth? See comments below for example:

function test_count_with_revert() public {
    CounterWithoutRevert countNoRevert = new CounterWithoutRevert();
    CounterWithRevert countRevert = new CounterWithRevert();
    vm.expectRevert(address(countRevert));
    countNoRevert.count(); // If this call makes a subcall to `countRevert` and it doesn't revert, does test fail?
    countNoRevert.count();
    countNoRevert.count();
    countRevert.count();
}

I think that nuance is why I'd prefer expectRevert(reverterAddress) to always apply to the next call regardless: it's simpler, and it's the same as how expectEmit(emitter) always applies to the next call and doesn't wait for the next call to emitter

I know this doesn't meet @PaulRBerg requirements, but the use case of the feature was really just to skip listening for reverts from staticcalls, and that same feature would also apply to events as well. So I don't think this proposed behavior of "expectRevert(reverterAddress) waits for call to reverterAddress" is the optimal solution, compared to something like an overload of those methods with a skipStatic arg

[grandizzy]

countNoRevert.count(); // If this call makes a subcall to `countRevert` and it doesn't revert, does test fail?

yes, that's correct, in this case the test will fail right away

I think that nuance is why I'd prefer expectRevert(reverterAddress) to always apply to the next call regardless

Agree, open to suggestions if that's not desired behavior, maybe let's move convo in PR #8770?

[mds1]

Sounds good, will move my thoughts there now