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 expectCall behavior #7237

Open
Tracked by #7613
Evalir opened this issue Feb 25, 2024 · 0 comments
Open
Tracked by #7613

feat(cheatcodes): V1 expectCall behavior #7237

Evalir opened this issue Feb 25, 2024 · 0 comments
Labels
A-cheatcodes Area: cheatcodes T-likely-breaking Type: requires changes that can be breaking T-to-discuss Type: requires discussion

Comments

@Evalir
Copy link
Member

Evalir commented Feb 25, 2024

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 with the specified selector, it will pass. While this is the current accepted behavior, it was not the intended original one. For V1, we propose actually introducing the intended behavior.

Current 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 in the baz body. This counts as 2 of two calls expected. expectCall is fulfilled.
}

While this looks fine in this example, this is not the intended behavior, as the expectCall should only work for the next call. The current behavior can lead to footguns where you expect a certain call to a function to happen during the next call made, but it can instead happen later in the test and still pass. An example:

// 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.
  //
  // More calls throughout the test body... expectCall is still not filled.
  //
  target.baz(); // Finally call bar in the baz body. This counts as 2 of two calls expected. expectCall is fulfilled, after executing multiple separate calls.
}

New behavior

The proposed behavior is the following: expectCall should match calls that are strictly subcalls of the next call.

Example of proposed functionality for V1:

// 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.
  target.baz(); // This still leads to a failure, as expectCall will only process the next call and all its subcalls.
}

// 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 
  }
} 
@Evalir Evalir added the A-cheatcodes Area: cheatcodes label Feb 25, 2024
@Evalir Evalir added this to the v1.0.0 milestone Feb 25, 2024
@zerosnacks zerosnacks added the T-likely-breaking Type: requires changes that can be breaking label Jul 11, 2024
@zerosnacks zerosnacks added the T-to-discuss Type: requires discussion label Jul 31, 2024
@zerosnacks zerosnacks removed this from the v1.0.0 milestone Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes T-likely-breaking Type: requires changes that can be breaking T-to-discuss Type: requires discussion
Projects
None yet
Development

No branches or pull requests

2 participants