Add vm.expectRevert overloads that take an address arg, to distinguish between error selectors that are identical but in different contracts.

[simplyoptimistic]

Component

Forge

Describe the feature you would like

As described in this issue in the foundry book:

If you have two contracts with identical custom errors:

contract A {
  error NotValid();
  // contract A specific code here...
}
contract B {
  error NotValid();
  // contract A specific code here...
}

then: A.NotValid.selector == B.NotValid.selector.

If you have a test that checks for a revert from A.NotValid.selector and the error is thrown by B in the test, the test will still pass as the error selector comparisons will pass.

By including the address of the contract in expectRevert, you can specify with greater granularity which error you wish to test for.

As it stands, it is not practical to refactor common errors into a shared interface as there is no way of checking the source of a revert.

Additional context

No response

[xeno097]

If no one is working on this I'd like to 😄

[Evalir]

@xeno097 just assigned you!

@mds1 quickly looping u in to further nail UX on this

[mds1]

We currently have these 3 cheats:

// Expects an error on next call
function expectRevert(bytes calldata revertData) external;
function expectRevert(bytes4 revertData) external;
function expectRevert() external;

Similar to the vm.expectEmit overloads, we should add 3 new cheats with the "address that's expected to revert" as the last arg:

// Expects an error on next call
function expectRevert(bytes calldata revertData) external;
function expectRevert(bytes4 revertData) external;
function expectRevert() external;

// Expects an error on next call from the given address. If the revert data matches but the
// address that originated the revert does not, that is considered a failure.
function expectRevert(bytes calldata revertData, address reverter) external;
function expectRevert(bytes4 revertData, address reverter) external;
function expectRevert(address reverter) external;

One consideration when implementing is how to handling bubbling up of reverts. Say we're expecting address A to revert with 0x1234

• If address A was the first contract in the call stack to revert with 0x1234, that's a success
• If address B was the first contract in the call stack to revert with 0x1234, then it bubbled up to A which therefore reverted with that same error, that's a failure because A did not originate with the revert
• If address B was the first contract in the call stack to revert with 0x1234, then it bubbled up to A which caught the error and re-threw it, that would be a success because A originated the error, and could have originated any revert data there (or could have continued execution)

@Evalir @xeno097 do you agree with that behavior? I'm not sure how feasible that third bullet is though. 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

[xeno097]

Yep, I agree. I'll see how feasible it is to achieve the third bullet point and if not, then I'll stick to plan A.

[mds1]

if not, then I'll stick to plan A.

sorry what is "plan A", is that referring to the below plan?

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

[xeno097]

if not, then I'll stick to plan A.

sorry what is "plan A", is that referring to the below plan?

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

Yep sorry for not being clear 😅

[zerosnacks]

Implemented in #8770