book icon indicating copy to clipboard operation
book copied to clipboard

Explicit documentation for `expectRevert` when errors are identical.

Open simplyoptimistic opened this issue 2 years ago • 1 comments

If you have the same error e.g. NotValid() in different contracts / interfaces, forge doesn't distinguish between the two. So if you have a test that expects A.NotValid.selector to be thrown, but B.NotValid.selector is thrown instead, the test will still pass.

I think this is fine as it is doing a comparison on the selector and it makes sense if you think about it for a little bit but I think it would be nice if the docs for expectRevert made this explicit so that developers do not accidentally trip on this.

simplyoptimistic avatar Jul 04 '23 09:07 simplyoptimistic

If you have this:

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, where the selector is computed the same as function signatures, i.e. bytes4(keccak256("NotValid()")) == 0xf1640ae1. Therefore you cannot distinguish them by the thrown error alone.

This is a good point though, I'd suggest creating an issue in the foundry repo to add a series of vm.expectRevert overloads that take an address arg, where the address input is the contract you expect to throw the initial revert

mds1 avatar Jul 04 '23 14:07 mds1