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

fix(forge): Fix cheatcodes not cooperating with vm.transact #3970

Merged
merged 7 commits into from
Dec 28, 2022

Conversation

AdithyaNarayan
Copy link
Contributor

Motivation

Closes #3797

Solution

  • Add optional inspector to commit_transaction
  • Add implementation for RollFork3/vm.rollFork(uint256 forkId, bytes32 transaction) (which was not implemented until now for some particular reason?)
  • Add tests for transact and rollFork cooperating with cheatcodes

It is evident that the Cheatcodes inspector doesn't inspect the transaction that is executed in transact since not only does vm.recordLogs not work as expected but even other cheatcodes like vm.expectCall doesn't work as expected.
I added an optional inspector to inspect the new EVM instance that the transact is run on. However, since the Cheatcodes inspector needs the Database to also implement the DatabaseExt trait, I create a new Backend database with the fork's database and pass it to the EVM.

Note that, this means that the DatabaseExt trait now depends on Backend which implements DatabaseExt itself. It is a circular dependency. Would this design be prefered or shall I move the responsibility of passing a DatabaseExt all the way up to the caller of transact?

Also, I've noticed that vm.transact doesn't revert when the transaction is supposed to revert, shall I create a separate issue for this?

@mattsse
Copy link
Member

mattsse commented Dec 27, 2022

thanks for looking into this

Note that, this means that the DatabaseExt trait now depends on Backend which implements DatabaseExt itself. It is a circular dependency.

While this seems to be working, I'm hesitant to move forward with this because this will eventually cause all sorts of problems.

it looks like we only need the Cheatcodes inspector though?

and since the cheatcodes triggers the transact cheatcode call, it'd be perfectly fine to pass this as inspector instead?

@AdithyaNarayan
Copy link
Contributor Author

Yep, that makes sense! I thought that down the road other inspectors might be needed but didn't realise that it is only being called inside the Cheatcodes inspector. Changed it.

Should this inspector inspect the roll_fork_to_transaction transactions as well? Or only transact?

@mattsse
Copy link
Member

mattsse commented Dec 27, 2022

Should this inspector inspect the roll_fork_to_transaction transactions as well? Or only transact?

hmm not entirely sure, but I think we can start by adding it to transact only.

evm/src/executor/backend/mod.rs Outdated Show resolved Hide resolved
evm/src/executor/backend/mod.rs Show resolved Hide resolved
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.

lgtm, tysm

@mattsse mattsse merged commit cae951f into foundry-rs:master Dec 28, 2022
@mattsse mattsse added T-feature Type: feature A-cheatcodes Area: cheatcodes labels Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vm.recordLogs does not work with vm.transact
2 participants