Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(cheatcodes): v1 expect* changes #5728

Closed
wants to merge 1 commit into from
Closed

feat(cheatcodes): v1 expect* changes #5728

wants to merge 1 commit into from

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented Aug 25, 2023

expect* cheatcode changes

This is the working feature branch for the 1.0 expect* cheatcodes-related changes.

expectCall

It will now only work for the next call's subcalls. See detailed description:

Details

Motivation

Right now, expectCall works at the "test" level—as long as there are enough calls to the target contract at any depth level in the test, it will pass. This will make it so that the calls are only counted if they happen in an external call, meaning calls performed at the "test" ("root") level will not count.

New behavior

The current behavior allows using expectCall this way:

// PASS
function testThings() {
  Contract inner = new Contract();
  NestedContract target = new NestedContract(inner);
  vm.expectCall(address(inner), abi.encodeWithSelector(inner.bar.selector), 2);
  inner.bar(); // call bar directly. Counts as 1 of two calls expected.
  target.baz(); // will call bar internally. This counts as 2 of two calls expected. expectCall is fulfilled.
}

This is not the intended behavior, due to this reason:

  • expect* cheatcodes should only work for the next call.

This means that expectCall should actually expect to match calls that are "subcalls" of the next call.

This PR introduces the following behavior

// FAIL
function testOne() {
  Contract inner = new Contract();
  NestedContract target = new NestedContract(inner);
  vm.expectCall(address(inner), abi.encodeWithSelector(inner.bar.selector), 1);
  inner.bar(); // call bar directly. This does not get counted, as this is a test-level call. It should be abstracted into another call.
}

// PASS
function testOne() {
  Contract inner = new Contract();
  NestedContract target = new NestedContract(inner);
  vm.expectCall(address(inner), abi.encodeWithSelector(inner.bar.selector), 1);
  target.baz(); // will call bar internally, therefore works.
}

contract NestedContract {
  Contract public inner;

  constructor(Contract _inner) {
    inner = _inner;
  }
  function baz() public {
    inner.bar(); // call bar 
  }
} 

expectRevert

Will now only catch reverts coming from the next call, instead of at any level in the test function, including at the "test level" itself. Detailed description:

Details

** This is a big breaking change**

Motivation

Closes #3723 , Closes #3437 , Closes #4832 .

expectRevert currently has incorrect behavior: it matches reverts at the test level, not at the next call level. This means that reverting cheatcodes were being incorrectly catched by expectRevert (we're supposed to bypass checks for cheatcode calls) and also let users use the cheatcode incorrectly as illustrated in #3723.

Solution and Behavior

expectRevert now only works on the next CALL. Cheatcode calls are now ignored properly, even if those cheatcode calls revert. To illustrate, these are examples of now legal/illegal behavior:

// OK
function testExpectRevertString() public {
    Reverter reverter = new Reverter();
    cheats.expectRevert("revert");
    reverter.revertWithMessage("revert");
}

// FAIL
function testFailRevertNotOnImmediateNextCall() public {
    Reverter reverter = new Reverter();
    // expectRevert should only work for the next call. However,
    // we do not inmediately revert, so,
    // we fail.
    cheats.expectRevert("revert");
    reverter.doNotRevert();
    reverter.revertWithMessage("revert");
}

// FAIL
function testFailCheatcodeRevert() public {
  // This expectRevert is hanging, as the next cheatcode call is ignored.
  vm.expectRevert();
  vm.fsMetadata(fakePath) // try to go to some non-existent path to cause a revert
}

Please note that CI is expected to fail due to solady and other repos not being upgraded to V1's expect behavior.

@Evalir Evalir changed the base branch from master to v1 September 18, 2023 18:50
@Evalir Evalir marked this pull request as ready for review September 18, 2023 18:50
@Evalir Evalir requested a review from mattsse September 18, 2023 18:50
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks fine, can we add a test that demonstrates the nuances?

wdyt @mds1 ?

@Evalir
Copy link
Member Author

Evalir commented Nov 7, 2023

Needs a rebase (along with the v1) branch to accomodate the new cheatcode changes.

@Evalir
Copy link
Member Author

Evalir commented Dec 20, 2023

Will be reopened as an issue containing a Spec, so the community can comment on the changes. Not worth it to keep rebasing this branch.

@Evalir Evalir closed this Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants