Skip to content

feat(cheatcodes): Make expectCall only work for the next call's subcalls#4986

Merged
mattsse merged 15 commits intomasterfrom
evalir/fix_expect_call
May 24, 2023
Merged

feat(cheatcodes): Make expectCall only work for the next call's subcalls#4986
mattsse merged 15 commits intomasterfrom
evalir/fix_expect_call

Conversation

@Evalir
Copy link
Copy Markdown
Member

@Evalir Evalir commented May 19, 2023

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 
  }
} 

TODO:

  • Book update

@Evalir Evalir requested review from mattsse and mds1 May 19, 2023 20:38
@Evalir Evalir marked this pull request as ready for review May 19, 2023 20:38
@Evalir
Copy link
Copy Markdown
Member Author

Evalir commented May 19, 2023

the ci failure looks a tad strange @mattsse — this shouldn't be making revm randomly fail to check the journaled state? we're not accessing it on places it might be undefined i believe, but it seems like the usage of depth in the call hook might be making it fail?

Copy link
Copy Markdown
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.

seems like only the transact_fork test is affected,
could also be an rpc provider issue with that test, but it's weird that this panics

@Evalir Evalir requested a review from mattsse May 23, 2023 18:28
Copy link
Copy Markdown
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.

nice,
tests lgtm!

Comment on lines +896 to +897
// If the depth is 0, then this is the root call terminating
if data.journaled_state.depth() == 0 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@mattsse mattsse merged commit 3a82b48 into master May 24, 2023
@mattsse mattsse deleted the evalir/fix_expect_call branch May 24, 2023 09:30
@Evalir Evalir restored the evalir/fix_expect_call branch May 24, 2023 17:03
@Evalir
Copy link
Copy Markdown
Member Author

Evalir commented May 24, 2023

Reverting this to merge into a feature branch

mattsse added a commit that referenced this pull request May 24, 2023
mattsse added a commit that referenced this pull request May 24, 2023
@Evalir Evalir deleted the evalir/fix_expect_call branch May 24, 2023 18:04
@Evalir Evalir restored the evalir/fix_expect_call branch May 24, 2023 18:04
@Evalir Evalir deleted the evalir/fix_expect_call branch May 25, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants