feat(cheatcodes): add ability to ignore (multiple) specific and partial reverts in fuzz and invariant tests

[emo-eth]

Motivation

As raised in #4271, sometimes the fuzzer dictionary will uncover real but irrelevant errors in contract code, especially when testing against stateful forks. Runs that encounter anticipated errors should be thrown out and count towards the global assume counter.

Closes #4271.

Solution

This PR adds several overloaded and related function definitions to vm.assumeNoRevert, introduced in #8780, as follows:

    /// Discard this run's fuzz inputs and generate new ones if next call reverted with data starting with the given revert data. Call more than once to add multiple reasons.
    #[cheatcode(group = Testing, safety = Safe)]
    function assumeNoRevert(bytes4 revertData) external pure;

    /// Discard this run's fuzz inputs and generate new ones if next call reverted with the given revert data. Call more than once to add multiple reasons.
    #[cheatcode(group = Testing, safety = Safe)]
    function assumeNoRevert(bytes calldata revertData) external pure;

    /// Discard this run's fuzz inputs and generate new ones if next call reverted with data starting with the given revert data. Call more than once to add multiple reasons.
    #[cheatcode(group = Testing, safety = Safe)]
    function assumeNoRevert(bytes4 revertData, address reverter) external pure;

    /// Discard this run's fuzz inputs and generate new ones if next call reverted with the given revert data. Call more than once to add multiple reasons.
    #[cheatcode(group = Testing, safety = Safe)]
    function assumeNoRevert(bytes calldata revertData, address reverter) external pure;

The reverter param ensures a revert comes from a specific called contract, per #8770, and the PartialRevert methods allow matching on selectors with arbitrary extra data, per #8763.

Behavior

• it should be possible to to reject any specific error without data (vm.assumeNoRevert(bytes4))
• it should be possible to reject any specific error with arbitrary data (vm.assumeNoRevert(bytes))
• it should be possible to reject any specific of error regardless of extra data (vm.assumeNoPartialRevert(bytes4))
• it should be possible to reject multiple errors that a single call may produce (multiple subsequent calls to any of the vm.assumeNoRevert definitions that accept params)
• it should be possible to restrict these rejections to a specific reverter (the optional overloaded address reverter param)
• anticipated reverts should reset after next non-cheatcode external call
• combining a catch-all vm.assumeNoRevert() with a vm.assumeNoRevert(args) call should result in an error (currently, the inverse correctly errors, but the ordering provided here will result in a \vm.assume` rejected too many inputserror due to how cheatcode errors are encoded, see note aboutvm.expectRevert` below)

Questions

• Should these functions be pure and Safe? I see that expectRevert is not pure, and is marked as Unsafe, but assumeNoRevert() is marked pure and Safe.
• Is the behavior of calling vm.assumeNoRevert multiple times to build up anticipated reverts per-call acceptable, or should it be forced to take an array of structs with revert parameters, since I think aggregating parameters from separate calls is new behavior (correct me if I'm wrong).
• I've also left a few TODOs in the code with questions, which I will try to surface via comments here.
• expectRevert() without args seems to not play strangely with cheatcode errors – eg, vm.expectRevert() will swallow a cheatcode revert, including those thrown by vm.assumeNoRevert and subsequent calls to vm.expectRevert(). This seems like a separate bug that I can log if we agree there.

[grandizzy]

looks good! left couple of comments, going to move back in draft until fix added

[grandizzy]

@mds1 would be great to have your thoughts re new proposed cheatcodes. Thanks!

[emo-eth]

@grandizzy should be in a good state now – apologies, had a hectic week. let me know if anything else looks off.

[jenpaff]

@grandizzy should be in a good state now – apologies, had a hectic week. let me know if anything else looks off.

moving PR to ready for review

[grandizzy]

thank you, left couple of comments.

[grandizzy]

Thank you, looks good! I pushed a change to simplify test and have one more nit re assume_no_revert fn. @zerosnacks @yash-atreya @klkvr pls chime in when time permits. thank you!

[grandizzy]

Lgtm, thank you! Pending additional review before merging

[mds1]

In general UX LGTM! One thing is let's make sure the behavior between this and the various expectRevert cheats is consistent, e.g. if you pass a 4byte selector does it only match if the revert data is exact, or do partials match also? Given that this cheat is assumeNoRevert for partial match I think it's inconsistent as spec'd in the PR description

https://github.com/foundry-rs/forge-std/blob/da591f56d8884c5824c0c1b3103fbcfd81123c4c/src/Vm.sol#L2102-L2124

[emo-eth]

In general UX LGTM! One thing is let's make sure the behavior between this and the various expectRevert cheats is consistent, e.g. if you pass a 4byte selector does it only match if the revert data is exact, or do partials match also? Given that this cheat is assumeNoRevert for partial match I think it's inconsistent as spec'd in the PR description

https://github.com/foundry-rs/forge-std/blob/da591f56d8884c5824c0c1b3103fbcfd81123c4c/src/Vm.sol#L2102-L2124

@mds1 apologies, i can update the description (pending final decision) – @grandizzy suggested having the default behavior to accept partial matches.

having the granularity of assumeNoRevert vs assumeNoPartialRevert would be useful in the scenario of colliding selectors – but that's pretty rare unless intentional. i am having a hard time thinking of cases where a test might be pulling in (arbitrary) external or random revert bytes that might have collisions – but there are possibly some, and it might be worth having the disambiguation.

i do appreciate the explicitness, though i do think assumeNoRevert is already kind of a counter-intuitive name, and assumeNoPartialRevert adds onto that complexity. eg assumeNoRevertPartial is maybe slightly less confusing, but breaks with the expectPartialRevert naming convention...

happy to add back in if there's consensus

[mds1]

Thanks for those details, so my thoughts are:

• We already have expectRevert(bytes4) and expectPartialRevert(bytes4). To this point—"would be useful in the scenario of colliding selectors – but that's pretty rare unless intentional"—I'm unsure of the motivation for this split
• The least surprising assume behavior would therefore be analogous meaning assumeNoRevert(bytes4) and assumeNoPartialRevert(bytes4)
• It sounds like there are some breaking change considerations here, which I am not fully up to speed on, in which case that is suitable rationale for having inconsistent behavior between the expect and assume cheats
• Therefore I'll defer to you and @grandizzy to decide
• But, we should track this and unify / simplify for foundry v1

[grandizzy]

thanks both for comments! the expectPartialRevert was added when expectRevert was already in place and used see #3725 (comment) and Paul's comment

This may warrant a new VM cheatcode, to ensure backwards compatibility and avoid triggering false positives for people who assume the current vm.expectRevert cheatcode to look for an exact ABI match.

For assumeNoRevert there's no breaking change consideration (as we don't have them yet) that's why I proposed to avoid adding partial one but consistency is a good point too. Would like to hear more opinions, @zerosnacks maybe you could share your thoughts?

[zerosnacks]

Thanks @emo-eth for your PR!

Is the behavior of calling vm.assumeNoRevert multiple times to build up anticipated reverts per-call acceptable, or should it be forced to take an array of structs with revert parameters, since I think aggregating parameters from separate calls is new behavior (correct me if I'm wrong).

I would prefer we use an array parameter to express the intent of checking against multiple cases in line
with existing behavior (mockCalls, makePersistent, revokePersistent, etc..). Stateful stacking as proposed is, as mentioned, a new kind of behavior I think we should avoid.

I agree that expectPartialRevert exists for a backwards compatibility reason, no need to introduce a parallel assumeNoPartialRevert in my opinion. This can always be added later on in a non-breaking way if there is a specific request for it.

Should these functions be pure and Safe? I see that expectRevert is not pure, and is marked as Unsafe, but assumeNoRevert() is marked pure and Safe.

Unsafe generally refers to either it persistently touching EVM settings / state or writing to a file. Do you have an opinion in this regard @DaniPopes?