-
Notifications
You must be signed in to change notification settings - Fork 110
refactor(l1,levm): simplify vm database trait. #4855
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Benchmark Results ComparisonBenchmark Results: MstoreBench
Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
crates/blockchain/Cargo.toml
Outdated
ethrex-common.workspace = true | ||
ethrex-storage.workspace = true | ||
ethrex-vm.workspace = true | ||
ethrex-levm.workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we have to add this here, then the vm crate doesn't really have a reason to exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out. I moved the StoreVmDatabase
to the vm crate so that it lives there with the other databases that implement the LevmDatabase
trait. I think it's tidier this way but I don't know if you think it's a good idea to have it there or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the VM database trait architecture by removing the VmDatabase
trait abstraction and directly implementing the LevmDatabase
trait on storage structures. This simplifies the codebase by eliminating an unnecessary abstraction layer.
Key changes:
- Removed
VmDatabase
trait andDynVmDatabase
type alias - Implemented
LevmDatabase
trait directly onStoreVmDatabase
,GuestProgramStateWrapper
, andDatabaseLogger
- Updated method signatures to return non-optional values with defaults instead of
Option<T>
- Changed database wrapper types from
Box<dyn VmDatabase>
toArc<dyn LevmDatabase>
Reviewed Changes
Copilot reviewed 18 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
crates/vm/lib.rs | Removed db module and VmDatabase exports, moved GuestProgramStateWrapper export |
crates/vm/backends/mod.rs | Removed new_for_l1 and new_for_l2 constructor methods |
crates/vm/backends/levm/db/mod.rs | Created new module file to export database implementations |
crates/vm/backends/levm/db/store.rs | Implemented LevmDatabase trait, changed return types to non-optional with defaults |
crates/vm/backends/levm/db/witness.rs | Implemented LevmDatabase trait, updated error types to DatabaseError |
crates/vm/backends/levm/db/logger.rs | Removed DynVmDatabase LevmDatabase implementation |
crates/vm/Cargo.toml | Removed dyn-clone dependency, added ethrex-storage dependency |
crates/blockchain/blockchain.rs | Updated to use Arc<dyn LevmDatabase> and new constructor methods |
crates/blockchain/payload.rs | Updated constructor calls to use new_from_db_for_l1/l2 |
crates/blockchain/tracing.rs | Updated import path for StoreVmDatabase |
crates/l2/common/src/state_diff.rs | Updated to use LevmDatabase trait instead of VmDatabase |
crates/l2/prover/src/guest_program/src/execution.rs | Updated constructor calls and removed VmDatabase import |
tooling/ef_tests/state/utils.rs | Changed from Box to Arc for database wrapper |
tooling/ef_tests/state/runner/revm_db.rs | Implemented direct LevmDatabase usage with AccountState |
tooling/ef_tests/state_v2/src/modules/utils.rs | Changed from Box to Arc for database wrapper |
crates/vm/levm/runner/src/main.rs | Changed from Box to Arc for database wrapper |
crates/vm/levm/bench/revm_comparison/src/levm_bench.rs | Changed from Box to Arc for database wrapper |
crates/l2/common/Cargo.toml | Added ethrex-levm dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the core changes, the databases that implement Levm trait are now in the db folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in this file are correct but unimportant because we don't care about the revm runner of eftests anymore. I suggest ignoring them
Motivation
Description