feat(cheatcodes): expectRevert with specific reverter address

[grandizzy]

Motivation

Closes #5299
Closes foundry-rs/book#945
#5299 (comment)

Solution

• added expectRevert cheatcodes with reverter (expects an error on next call from the given address)

function expectRevert(bytes calldata revertData, address reverter) external;
function expectRevert(bytes4 revertData, address reverter) external;
function expectRevert(address reverter) external;
function expectPartialRevert(address reverter) external;

• when reverter arg passed then process expected revert for next call, at any depth. If address of contract that reverted the call (call's target address) is different than the expected reverter, then fail test with Reason: Reverter != expected reverter reason.
• code refactoring: pass ExpectedRevert directly to handle_expect_revert
• added tests

[mds1]

Continuing from #4762:

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

Personally I am not a fan of this behavior, and I think expectRevert(reverterAddress) should expect the revert on the next call. The main reason here is this is consistent with how expectEmit(emitter) behaves—it always applies to the next call, and consistent is important for UX.

Additionally, I think "the next call to reverterAddress" is not always obvious, because calls to a different top level address may end up making subcalls to reverterAddress

[grandizzy]

Personally I am not a fan of this behavior, and I think expectRevert(reverterAddress) should expect the revert on the next call. The main reason here is this is consistent with how expectEmit(emitter) behaves—it always applies to the next call, and consistent is important for UX.

Agree, consistency makes sense. However not clear how the bubbling revert described in #5299 (comment) could be achieved if we expect the revert on the next call, I could be missing something

[mds1]

So it might be better to just consider all 3 of those bullets a success, i.e. we're expecting that A throws that error anywhere in the call stack, and does not have to be the originator

I think this is probably the simplest solution, just ignore whether or not it bubbled up at all, and as long as reverterAddress reverts with the expected error it's a success

[grandizzy]

So it might be better to just consider all 3 of those bullets a success, i.e. we're expecting that A throws that error anywhere in the call stack, and does not have to be the originator

I think this is probably the simplest solution, just ignore whether or not it bubbled up at all, and as long as reverterAddress reverts with the expected error it's a success

👍 will do so then

[mattsse]

lgtm, pending @klkvr

[klkvr]

is this basically expectRevert + assert the target of the next call is reverterAddress?

[grandizzy]

is this basically expectRevert + assert the target of the next call is reverterAddress?

yeah, pls check #5299 for identical selectors but in different contracts

[grandizzy]

@klkvr ptal and lmk if any things you'd like to be changed before merging. Thank you!

[klkvr]

the approach LGTM, though I don't think we're actually resolving #5299 usecase here? eg there's no way to assert B reverting without calling it directly.

IMO having a way to assert revert of the given contract at any subcall of the next call might be useful. such reverts might not even end up bubbled up, and this should still be fine — try-catch blocks are not used that much, and it's arguably useful to have a way to test them being entered as well.

I also think that this would be consistent, because expectEmit(address emitter) catches emits of any depth as well

[grandizzy]

the approach LGTM, though I don't think we're actually resolving #5299 usecase here? eg there's no way to assert B reverting without calling it directly.

IMO having a way to assert revert of the given contract at any subcall of the next call might be useful. such reverts might not even end up bubbled up, and this should still be fine — try-catch blocks are not used that much, and it's arguably useful to have a way to test them being entered as well.

I also think that this would be consistent, because expectEmit(address emitter) catches emits of any depth as well

@mds1 would you concur with @klkvr comment? Imo makes sense but needs to be able to check subcalls of next call. So kind of a mix of how was implemented initially but only for the next call

[mds1]

is this basically expectRevert + assert the target of the next call is reverterAddress?

yeah, pls check #5299 for identical selectors but in different contracts

Oh, I thought this was: expectRevert + assert the revert came from reverterAddress, regardless of the target of the next call? I agree with @klkvr and think that is actually what we want to resolve #5299

[grandizzy]

IMO having a way to assert revert of the given contract at any subcall of the next call might be useful. such reverts might not even end up bubbled up, and this should still be fine — try-catch blocks are not used that much, and it's arguably useful to have a way to test them being entered as well.

I also think that this would be consistent, because expectEmit(address emitter) catches emits of any depth as well

@klkvr please recheck. thank you!

dismissing as several other changes were done since this review

[klkvr]

LGTM