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): Make expectEmit only work for the next call #4920

Merged
merged 15 commits into from
May 12, 2023

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented May 10, 2023

This is a big breaking change.

Partially closes #1745. expectCall is left and needs #4912 to be merged before it can be reworked to support this.

Makes expectEmit only work for the immediate next call after the cheatcode has been called.

Behavior

expectEmit remains additive. Calling expectEmit N times will set a lower bound of N events that must be seen for the cheatcode to pass successfully.

The current behavior of expectEmit allows to:

  • declare the cheatcode before all events are emitted.
  • "fill" the expected emits before the test ends.
  • match events after the tests ends.

The intended behavior is that it only works for the next immediate call. This new behavior only allows to:

  • Call the cheatcode AND fill the expected emit right after calling the cheatcode. Declaring the cheatcode N times and then filling N events will fail.
  • match events after the next CALL is triggered. Events not triggered by a call will be ignored. Events nested in the triggered CALL subtree will be matched. If the events defined are not matched, the execution will revert.

It's important to note the restriction imposed by this, namely that after the cheatcode is declared, no CALLs other than the one the cheatcode is intended to "match" must happen. This means that #1214 is now illegal behavior. Specifically, this means that this is now illegal:

function testEmitMyEvent_v2() external {
    cheatcodes.expectEmit(true, true, true, true);
    // myContract.myVar() is a call made after the cheatcode was called. This is illegal.
    // To fix this, move the call to before the cheatcode and use a variable.
    emit MyEvent(myContract.myVar(), address(this), address(this), block.timestamp);
    myContract.emitMyEvent();
}

Why are now expect emits being pushed without subtracting 1 from the depth?

#1215 is a great read to reason about depth. After this, the reason is that currently, depth - 1 is being used is so that expectEmit checks the events after the call that invoked the cheatcode ends. However, we now wanna check the events after the next CALL (which will have more depth) ends, which means that we need to check for depth, not depth - 1.

When we're happy with this and merge it we should definitely prepare a telegram message & a tweetstorm for this change, as this might confuse people.

@Evalir Evalir requested review from mds1, gakonst and mattsse May 10, 2023 23:14
@Evalir Evalir marked this pull request as draft May 10, 2023 23:22
@Evalir Evalir marked this pull request as ready for review May 11, 2023 00:27
@Evalir Evalir removed the request for review from gakonst May 11, 2023 00:36
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-wise this looks good, but defer to @mds1 if this is the intended behavior now

@mds1
Copy link
Collaborator

mds1 commented May 11, 2023

This means that #1214 is now illegal behavior. Specifically, this means that this is now illegal:

function testEmitMyEvent_v2() external {
    cheatcodes.expectEmit(true, true, true, true);
    // myContract.myVar() is a call made after the cheatcode was called. This is illegal.
    // To fix this, move the call to before the cheatcode and use a variable.
    emit MyEvent(myContract.myVar(), address(this), address(this), block.timestamp);
    myContract.emitMyEvent();
}

Is this change required if myContract.myVar() is a staticcall? You cannot emit events in staticcalls / view methods, so perhaps they should be ignored automatically? Similar to how staticcalls are ignored when you're in a startBroadcast context

Aside from that this seems great and I agree with this behavior. This is a decent sizes breaking change though, and since we may have some other breaking changes, perhaps we should start feature flagging them? We could just check the value of CARGO_PKG_VERSION_MAJOR to disable certain features if that value is < 1, then delete the old code after v1 is released?

Will defer to you all on if we should do that or not. Either way we should maintain somewhere a list of the recent breaking changes we can continually point people to. Could be a changelog file, discussion in the repo, or a meta-issue

@Evalir
Copy link
Member Author

Evalir commented May 11, 2023

Is this change required if myContract.myVar() is a staticcall? You cannot emit events in staticcalls / view methods, so perhaps they should be ignored automatically? Similar to how staticcalls are ignored when you're in a startBroadcast` context

Great point—I'll push a modification so we ignore STATICCALLs, it's an easy one.

On the feature flagging, that's a great idea, and i'm not too opposed to it (my only grip is obviously just the need to keep extra code for some more time/having to modify stuff to keep the old behavior working). Very curious on what @gakonst / @mattsse think about this

@Evalir
Copy link
Member Author

Evalir commented May 11, 2023

Also, I believe this behavior will also break some forge-std tests (some seem to be failing), so we might need to follow up there

@Evalir
Copy link
Member Author

Evalir commented May 11, 2023

@mds1 great catch on forge-std—i had missed this. With 420d814 we now have this behavior (will also update the main description):

A function emits events: A, B, C, D, E, F, F, G.

expectEmit will be able to match ranges with and without skipping events in between:

  • [A, B, C] is valid.
  • [B, D, F] is valid.
  • [G] or any other single event combination is valid.
  • [B, A] or similar out-of-order combinations are invalid (events must be in order).
  • [C, F, F] is valid.
  • [F, F, C] is invalid (out of order).

Which means we should be able to test forge-std with this change and not need to change it

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.

smol nit, otherwise this looks sounds.

tests look good.

// First, we can return early if all events have been matched.
// This allows a contract to arbitrarily emit more events than expected (additive behavior),
// as long as all the previous events were matched in the order they were expected to be.
if !state.expected_emits.iter().any(|expected| !expected.found) {
Copy link
Member

Choose a reason for hiding this comment

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

this is the same as if state.expected_emits.iter().all(|expected| expected.found), right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep indeed

Copy link
Member Author

Choose a reason for hiding this comment

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

done in d72a935 , waiting for ci

@bowd
Copy link

bowd commented May 22, 2023

Unless I'm missing something, there's one case that's impossible to fully cover with the given syntax:
Say we have a function on ContractA that calls ContractB which emits EventB and then ContractA emits EventA.
We can't ensure that events originate from the contract we intend. I can do:

vm.expectEmit(true, true, true, true);
emit EventB(...);
emit EventA(...);
contractA.example();

But this would match irrespective of which contract emitted the events. This isn't allowed:

vm.expectEmit(true, true, true, true, address(contractB));
emit EventB(...);
vm.expectEmit(true, true, true, true, address(contractA));
emit EventA(...);

So essentially if I understand correctly, you can only match the contract address if you don't intend to match any other events emitted by other contracts. Also, you can't mix and match matcher configs. For example, if you want to match data on one event but not match it on another you can't do that. This also can introduce some weird situations where if you want to match data on one event you have to compute/simulate all data values for other events.

@mds1
Copy link
Collaborator

mds1 commented May 22, 2023

@bowd can you open a separate issue so we can track this more easily? Your example seems like it should work to me so if it's currently not supported we can discuss in that issue with @Evalir

@Evalir Evalir mentioned this pull request Jun 8, 2023
2 tasks
Evalir added a commit that referenced this pull request Jun 12, 2023
Evalir added a commit that referenced this pull request Jun 12, 2023
…#5135)

* chore: add edge-cases

* chore: add edge case covering #4920 (comment)
Evalir added a commit that referenced this pull request Jun 26, 2023
* feat(`cheatcodes`): Make expectCall only work for the next call's subcalls (#5032)

* chore: make expect call only work for the next call

* chore: make expectCall actually check only the next call's subcalls

* chore: fmt

* chore: introduce checks at the main call level, not at the subcall level

* chore: handle dangling expected calls gracefully

* chore: fix tests

* chore: fmt

* chore: forge fmt

* chore: actually exclude depth the cheatcode was called from

* chore: tests

* chore: better docs

* chore: comment out impossible to check condition on expectCall

* chore: remove unused check

* fix(cheatcodes): Correct `expectRevert` behavior (#4945)

* chore: add repro test to pass

* chore: strictly check for the depth expectRevert was called in, instead of being able to peek at function end

* chore: tests

* chore: add more repro tests

* chore: fmt

* chore: clippy

* chore: fixup problematic tests, mark them as not working properly

* chore: forge fmt

* chore: forge fmt

* Update evm/src/executor/inspector/cheatcodes/mod.rs

* chore: add more info to changelog

* chore: fmt

* chore(tests): add more cases for `expectEmit` (#5076)

* chore(tests): add more extreme cases for expectEmit

* chore(tests): add next call fail case for expectEmit

* chore(`cheatcodes`): add more edge case tests on `expect*` cheatcodes (#5135)

* chore: add edge-cases

* chore: add edge case covering #4920 (comment)

* feat(`cheatcodes`): disallow usage of `expectRevert` with `expectCall` and `expectEmit`  (#5144)

* feat(cheatcodes): disallow usage of expectCall/Emit with expectRevert

* chore: add tests

* chore: fmt

* chore: fmt
Evalir added a commit that referenced this pull request Jul 10, 2023
* feat(`cheatcodes`): `1.0` cheatcode changes (#5045)

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

* chore: make expect call only work for the next call

* chore: make expectCall actually check only the next call's subcalls

* chore: fmt

* chore: introduce checks at the main call level, not at the subcall level

* chore: handle dangling expected calls gracefully

* chore: fix tests

* chore: fmt

* chore: forge fmt

* chore: actually exclude depth the cheatcode was called from

* chore: tests

* chore: better docs

* chore: comment out impossible to check condition on expectCall

* chore: remove unused check

* fix(cheatcodes): Correct `expectRevert` behavior (#4945)

* chore: add repro test to pass

* chore: strictly check for the depth expectRevert was called in, instead of being able to peek at function end

* chore: tests

* chore: add more repro tests

* chore: fmt

* chore: clippy

* chore: fixup problematic tests, mark them as not working properly

* chore: forge fmt

* chore: forge fmt

* Update evm/src/executor/inspector/cheatcodes/mod.rs

* chore: add more info to changelog

* chore: fmt

* chore(tests): add more cases for `expectEmit` (#5076)

* chore(tests): add more extreme cases for expectEmit

* chore(tests): add next call fail case for expectEmit

* chore(`cheatcodes`): add more edge case tests on `expect*` cheatcodes (#5135)

* chore: add edge-cases

* chore: add edge case covering #4920 (comment)

* feat(`cheatcodes`): disallow usage of `expectRevert` with `expectCall` and `expectEmit`  (#5144)

* feat(cheatcodes): disallow usage of expectCall/Emit with expectRevert

* chore: add tests

* chore: fmt

* chore: fmt

* `foundryup`: v1 changes (#5158)

* feat(foundryup): look for v1 tag instead of nightly for normal foundryup

* feat(foundryup): add ability to download legacy nightly binary with -L flag

* feat: use latest release for figuring out the tag name

* chore(foundryup): slightly improve stable release detection

* chore: use proper repo

* make fns async

* chore: remove prb math from integration tests

* chore: forge fmt

* chore: fix some merge leftovers

* chore: last test fixes

* chore: forge fmt

* chore: uncomment etch test

* feat(docs): add `RELEASE_PROCESS.md` (#5269)

* feat(docs): add RELEASE_PROCESS.md

* chore: not include changelog changes in step

* chore: bump crates to 1.0.0 (#5346)
Evalir added a commit that referenced this pull request Jul 10, 2023
* feat(`cheatcodes`): `1.0` cheatcode changes (#5045)

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

* chore: make expect call only work for the next call

* chore: make expectCall actually check only the next call's subcalls

* chore: fmt

* chore: introduce checks at the main call level, not at the subcall level

* chore: handle dangling expected calls gracefully

* chore: fix tests

* chore: fmt

* chore: forge fmt

* chore: actually exclude depth the cheatcode was called from

* chore: tests

* chore: better docs

* chore: comment out impossible to check condition on expectCall

* chore: remove unused check

* fix(cheatcodes): Correct `expectRevert` behavior (#4945)

* chore: add repro test to pass

* chore: strictly check for the depth expectRevert was called in, instead of being able to peek at function end

* chore: tests

* chore: add more repro tests

* chore: fmt

* chore: clippy

* chore: fixup problematic tests, mark them as not working properly

* chore: forge fmt

* chore: forge fmt

* Update evm/src/executor/inspector/cheatcodes/mod.rs

* chore: add more info to changelog

* chore: fmt

* chore(tests): add more cases for `expectEmit` (#5076)

* chore(tests): add more extreme cases for expectEmit

* chore(tests): add next call fail case for expectEmit

* chore(`cheatcodes`): add more edge case tests on `expect*` cheatcodes (#5135)

* chore: add edge-cases

* chore: add edge case covering #4920 (comment)

* feat(`cheatcodes`): disallow usage of `expectRevert` with `expectCall` and `expectEmit`  (#5144)

* feat(cheatcodes): disallow usage of expectCall/Emit with expectRevert

* chore: add tests

* chore: fmt

* chore: fmt

* `foundryup`: v1 changes (#5158)

* feat(foundryup): look for v1 tag instead of nightly for normal foundryup

* feat(foundryup): add ability to download legacy nightly binary with -L flag

* feat: use latest release for figuring out the tag name

* chore(foundryup): slightly improve stable release detection

* chore: use proper repo

* make fns async

* chore: remove prb math from integration tests

* chore: forge fmt

* chore: fix some merge leftovers

* chore: last test fixes

* chore: forge fmt

* chore: uncomment etch test

* feat(docs): add `RELEASE_PROCESS.md` (#5269)

* feat(docs): add RELEASE_PROCESS.md

* chore: not include changelog changes in step

* chore: bump crates to 1.0.0 (#5346)
excalq pushed a commit to thrackle-io/foundry that referenced this pull request Oct 1, 2024
* feat(`cheatcodes`): `1.0` cheatcode changes (#5045)

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

* chore: make expect call only work for the next call

* chore: make expectCall actually check only the next call's subcalls

* chore: fmt

* chore: introduce checks at the main call level, not at the subcall level

* chore: handle dangling expected calls gracefully

* chore: fix tests

* chore: fmt

* chore: forge fmt

* chore: actually exclude depth the cheatcode was called from

* chore: tests

* chore: better docs

* chore: comment out impossible to check condition on expectCall

* chore: remove unused check

* fix(cheatcodes): Correct `expectRevert` behavior (#4945)

* chore: add repro test to pass

* chore: strictly check for the depth expectRevert was called in, instead of being able to peek at function end

* chore: tests

* chore: add more repro tests

* chore: fmt

* chore: clippy

* chore: fixup problematic tests, mark them as not working properly

* chore: forge fmt

* chore: forge fmt

* Update evm/src/executor/inspector/cheatcodes/mod.rs

* chore: add more info to changelog

* chore: fmt

* chore(tests): add more cases for `expectEmit` (#5076)

* chore(tests): add more extreme cases for expectEmit

* chore(tests): add next call fail case for expectEmit

* chore(`cheatcodes`): add more edge case tests on `expect*` cheatcodes (#5135)

* chore: add edge-cases

* chore: add edge case covering foundry-rs/foundry#4920 (comment)

* feat(`cheatcodes`): disallow usage of `expectRevert` with `expectCall` and `expectEmit`  (#5144)

* feat(cheatcodes): disallow usage of expectCall/Emit with expectRevert

* chore: add tests

* chore: fmt

* chore: fmt

* `foundryup`: v1 changes (#5158)

* feat(foundryup): look for v1 tag instead of nightly for normal foundryup

* feat(foundryup): add ability to download legacy nightly binary with -L flag

* feat: use latest release for figuring out the tag name

* chore(foundryup): slightly improve stable release detection

* chore: use proper repo

* make fns async

* chore: remove prb math from integration tests

* chore: forge fmt

* chore: fix some merge leftovers

* chore: last test fixes

* chore: forge fmt

* chore: uncomment etch test

* feat(docs): add `RELEASE_PROCESS.md` (#5269)

* feat(docs): add RELEASE_PROCESS.md

* chore: not include changelog changes in step

* chore: bump crates to 1.0.0 (#5346)
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.

bug: expectEmit and expectCall don't only check the next call
4 participants