Skip to content

Feat: evm module with custom EvmFactory trait #554

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 6 commits into from
Apr 21, 2025

Conversation

julio4
Copy link
Contributor

@julio4 julio4 commented Apr 11, 2025

💡 Motivation and Context

This PR brings small code cleanup for EvmFactory. I have created a new EvmFactory trait in rbuilder because the Evm trait from alloy_revm has a lot of associated types and trait bounds.
These are now defined in the custom EvmFactory trait and resolved in the implementation for EthCachedEvmFactory, so this removes the need for extra trait bounds outside of the evm module (such as in the execute_evm function in order_commit).

📝 Summary

  • The EthCachedEvmFactory was moved to new module evm under building (rbuilder::building::evm), IMO this makes the overall code structure better as the separation of concerns is cleaner (more obvious)
  • EthCachedEvmFactory::Context is defined to EthEvmContext<DB>, which is an alias of Context<BlockEnv, TxEnv, CfgEnv, DB>
  • EvmFactory custom trait, which is a more concrete version than alloy_revm::EvmFactory
  • Remove the dependency on EthEvmFactory in EthCachedEvmFactory struct, but still use it's default implementation under the hood (we can also consider completely remove it and use Context instead)

✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@Copilot Copilot AI review requested due to automatic review settings April 11, 2025 08:10
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

@dvush dvush requested a review from efbig April 11, 2025 08:31
@ZanCorDX
Copy link
Contributor

Please add comments on trait EvmFactory (reason of existing, basically what you explain here!).

@ZanCorDX
Copy link
Contributor

Changing the dependency on EthEvmFactory and calling EthEvmFactory::default() seems dangerous, you are trusting the implementation of EthEvmFactory will not change. If in the future EthEvmFactory::default() gets super expensive and stores some internal data we'll get a performance penalty.

@julio4
Copy link
Contributor Author

julio4 commented Apr 14, 2025

Changing the dependency on EthEvmFactory and calling EthEvmFactory::default() seems dangerous, you are trusting the implementation of EthEvmFactory will not change. If in the future EthEvmFactory::default() gets super expensive and stores some internal data we'll get a performance penalty.

I see, because before it was calling EthEvmFactory::default() at BlockBuildingContext not at each create_evm calls.
I can use Context instead of EthEvmFactory::default(), by reverting this commit.

Please add comments on trait EvmFactory (reason of existing, basically what you explain here!).

Done.

@julio4 julio4 force-pushed the jules/evm-factory-trait branch from 65e8a34 to 13083e9 Compare April 14, 2025 03:02
@ZanCorDX
Copy link
Contributor

Changing the dependency on EthEvmFactory and calling EthEvmFactory::default() seems dangerous, you are trusting the implementation of EthEvmFactory will not change. If in the future EthEvmFactory::default() gets super expensive and stores some internal data we'll get a performance penalty.

I see, because before it was calling EthEvmFactory::default() at BlockBuildingContext not at each create_evm calls. I can use Context instead of EthEvmFactory::default(), by reverting this commit.

Please add comments on trait EvmFactory (reason of existing, basically what you explain here!).

Done.

You are copy/pasting the code of EthEvmFactory. Unless you can show me something we win with the copy/paste, can we go back to having a EthEvmFactory inside EthCachedEvmFactory?

@julio4
Copy link
Contributor Author

julio4 commented Apr 15, 2025

You are copy/pasting the code of EthEvmFactory. Unless you can show me something we win with the copy/paste, can we go back to having a EthEvmFactory inside EthCachedEvmFactory?

Yes this is done one purpose to have the same logic as EthEvmFactory without having to trust that its implementation will not change in the future.
If you think this is not a good idea, I can still revert to having the evm_factory field in EthCachedEvmFactory, and only call EthEvmFactory::Default() within EthCachedEvmFactory::Default().

@ZanCorDX
Copy link
Contributor

You are copy/pasting the code of EthEvmFactory. Unless you can show me something we win with the copy/paste, can we go back to having a EthEvmFactory inside EthCachedEvmFactory?

Yes this is done one purpose to have the same logic as EthEvmFactory without having to trust that its implementation will not change in the future. If you think this is not a good idea, I can still revert to having the evm_factory field in EthCachedEvmFactory, and only call EthEvmFactory::Default() within EthCachedEvmFactory::Default().

Yes please 🙏 I trust reth people, if they decide to change EthEvmFactory it will be for good reasons.

@julio4 julio4 force-pushed the jules/evm-factory-trait branch from 13083e9 to 9a75082 Compare April 16, 2025 07:39
@julio4
Copy link
Contributor Author

julio4 commented Apr 16, 2025

Yes please 🙏 I trust reth people, if they decide to change EthEvmFactory it will be for good reasons.

Done. Thanks for the valuable comments.

@ZanCorDX ZanCorDX merged commit 162a0cf into flashbots:develop Apr 21, 2025
4 checks passed
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