Skip to content

feat: CheatcodesExecutor + vm.deployCode #8181

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

Merged
merged 30 commits into from
Jun 26, 2024
Merged

feat: CheatcodesExecutor + vm.deployCode #8181

merged 30 commits into from
Jun 26, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Jun 17, 2024

Motivation

Some of the cheatcodes require creating and executing separate EVM frames. Examples of such cheatcodes are deployCode and assertEqCall from forge-std and vm.transact.

Current structure does not allow us to correctly execute such frames because we don't have access to complete Inspector instance. Because of that, for example, vm.transact does not produce any traces at the moment.

Solution

This PR adds CheatcodesExecutor trait which is passed to call hook on Cheatcodes. Its implementors should implement get_inspector method which accepts &mut Cheatcodes and returns Inspector instance.

InspectorStack is divided into InspectorStackInner and Cheatcodes to allow separate impl CheatcodesExecutor for InspectorStackInner. Cheatcodes impl of Inspector was inlined and call hook is modified to accept CheatcodesExecutor.

This PR adjusts DatabaseExt trait to allow usage of dyn DatabaseExt as DB: DatabaseExt and changes transact_inner and other methods creating inner EVMs to using &mut dyn DB instead of &mut DB to allow DatabaseExt::transact to accept Inspector<Backend>.

The only limitation currently is that we can't pass ContextPrecompiles<DB> to inner context. ContextPrecompiles<DB> can't be used as ContextPrecompiles<&mut dyn DB> and ContextPrecompiles<&mut DB> doesn't work because it's invariant over &mut DB. This might be an issue if we decide to implement stateful precompiles which should be persisted when entering inner EVM context.

Marking as WIP for now, however, deployCode impl does work already

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.

I see,

this removes the inspector impl from cheatcodes and adds the functions natively, so that we can also pass the CheatcodesExecutor (StackInner), this way we can invoke the evm from the Cheatcodes and keep inspecting with the stack.

so this introduces an additional layer in the stack.
I think this should work, and isn't too bad if we document this properly, the downside is additional complexity.

I thought about alternative approaches, but I couldn't come up with anything.

the stateful precompiles limitation might become an issue down the road

I don't think this has significant performance impacts

wdyt @DaniPopes

@klkvr
Copy link
Member Author

klkvr commented Jun 19, 2024

Added two deployCode cheatcodes and a testcase for native Vyper tests using it

@klkvr klkvr marked this pull request as ready for review June 23, 2024 05:57
@klkvr klkvr requested a review from Evalir as a code owner June 23, 2024 05:57
@klkvr klkvr changed the title [wip] feat: CheatcodesExecutor feat: CheatcodesExecutor + vm.deployCode Jun 23, 2024
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.

I think this is a great solution.

I think we could add back the inspector impl to Cheatcodes be calling its call function with a noop Cheatcodesexecutor or an empty stack?

pending @DaniPopes

@klkvr
Copy link
Member Author

klkvr commented Jun 26, 2024

Added TransparentCheatcodesExecutor which just reuses Cheatcodes itself as an inspector

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

nits

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.

3 participants