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

figure out failing emits with latest foundry #814

Closed
holic opened this issue May 14, 2023 · 3 comments · Fixed by #815
Closed

figure out failing emits with latest foundry #814

holic opened this issue May 14, 2023 · 3 comments · Fixed by #815

Comments

@holic
Copy link
Member

holic commented May 14, 2023

main branch's tests started failing. I think it might be this: foundry-rs/foundry#4920

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 a little unclear by the above "nested in the CALL subtree" description if this should succeed or fail in our case, but I noticed that StoreCore.pushToField is doing more calls internally before emitting, so maybe it's not matching on the new criteria for expectEmit?

This feels like a bummer constraint if so, and wonder if there are ways around this or alternative vm helpers.

I asked about this in the Foundry Support Telegram group. If there's not a quick/clear way to test for emits in the same way with the latest foundry version, we may need to pin our version for now.

Another option is to use Vulcan test utils, which has some nicer ergonomics: https://github.com/nomoixyz/vulcan

mc.doSomethingElse();

// Check event emissions
expect(address(mc).lastCall()).toHaveEmitted(
    "SomeEvent(address,bytes32,uint256)",
    [address(1).topic(), any()], // Event topics
    abi.encode(123) // Event data
);

(I also have an open issue and they have a pending PR for better gas reporting)

@holic
Copy link
Member Author

holic commented May 14, 2023

I tried out Vulcan but it had some internal reverts that I couldn't track down, so I gave up (flight is about to land).

I did, however, find a way around the foundry issue using vm.recordLogs() and vm.getRecordedLogs(), but I need to write a helper to clean it up. Will report back with some code later!

@alvrs
Copy link
Member

alvrs commented May 14, 2023

Another simple work around for this is to call the functions on a wrapper contract instead of the library (see #815). I'm actually confused why only two of the store tests are failing after the foundry update and not all of them - in my mind none of them should match the criteria of "expect an event in the next CALL" because the event is emitted via a library and therefore not inside a CALL.

@holic
Copy link
Member Author

holic commented May 15, 2023

Another simple work around for this is to call the functions on a wrapper contract instead of the library (see #815). I'm actually confused why only two of the store tests are failing after the foundry update and not all of them - in my mind none of them should match the criteria of "expect an event in the next CALL" because the event is emitted via a library and therefore not inside a CALL.

Weird! Is this something reproducible and worth bubbling up to Foundry team?

Glad it was a much easier fix than I was anticipating.

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 a pull request may close this issue.

2 participants