fix(forge) expectRevert for cheatcodes

[klkvr]

Motivation

Currently expectRevert isn't working correctly for reverts coming from cheatcodes invocations and may cause false-positives.
For example, such test will pass at the moment:

function test_parseJsonKeys() public {
    string memory jsonString = '{"a": "b"}';
    
    vm.expectRevert('JSON value at ".a" is not an object');
    vm.parseJsonKeys(jsonString, ".a"); 
    
    revert("failed"); // Won't be reached
}

This is happening because call_end hook in Cheatcodes inspector always exists early for calls to cheatcodes address, forwarding reverts from cheatcodes further. Because of that, test is reverting, but not failing because revert is being processed as an expected revert in the next call_end invocation.

Solution

1. Move expected reverts processing logic so it always run disregarding the call target.
2. Add special internal pending flag which is tracking that we've exited vm.expectRevert call context, because otherwise we would always expect it to revert which won't happen.
3. Update one cheatcode test which was giving false-positives due to this

[mattsse]

I see, this simply moves revert handling above the cheatcode check and thereby makes it possible to use revert handling on cheatcodes?

[mattsse]

I think I understand this, but would love to have a bit more context here

[mattsse]

can we add some docs here?

[klkvr]

@mattsse it seems that failing CI is caused by FsTest::testReadDir being another false positive before the fix, but I can't reproduce this locally, can it be platform-dependent?

[klkvr]

oh and it breaks some external codebases as well

for example, snekmate tests are failing right now because of such patterns:

vm.expectRevert(bytes("Ownable: caller is not the owner"));
ERC721Extended.set_minter(makeAddr(minter), true);

makeAddr calls vm.addr under the hood, so we are expecting the wrong call to revert

not sure if this should be merged now

[Evalir]

yeah i think the fs failures are windows-related, so we'll need a windows box to properly deal with those

I think making expect* cheatcodes compatible with other cheatcodes is something we probably do want later on, this will break existing codebases as expect* not interacting with other cheatcodes is sort of an undocumented-but-known foundry quirk. I think we should hold off from merging this now but proposing this as a breaking change later, if we gain something from this

[klkvr]

expect* not interacting with other cheatcodes is sort of an undocumented-but-known foundry quirk

I see, what's the best way to write negative tests for cheatcodes then? initially I've discovered this while writing tests for #6803, so not sure how to proceed with it

[mds1]

Just noting that I agree with @Evalir that supporting expectRevert for cheatcode reverts would break many codebases, and I don't think it's even a feature we want (I have not seen demand for it). Cheatcodes often ignore other cheatcodes and that consistency makes the dynamic easy to understand, for example vm.expectEmit would not expect an event to be emitted by a subsequent vm.prank

[klkvr]

yeah, I think we shouldn't merge that, can't imagine usecase when someone would want to use expect* on cheatcode invocations

I didn't realize initially that it will be breaking as we've already used expectReverts in tests with cheatcodes

Now it seems that some of those tests are incorrect, and imo it makes sense to rewrite them anyway

[klkvr]

Closed in favor of #6841