Skip to content

Conversation

@Philogy
Copy link
Contributor

@Philogy Philogy commented Oct 23, 2025

Hey, was trying out the library and noticed that build_trevm was fallible when it didn't need to be. Let me know if this change is desired.

@Philogy
Copy link
Contributor Author

Philogy commented Oct 23, 2025

Thinking about it TrevmBuilder only has 4 fields, does that need to be a builder at all? Could just be a small set of functions on Trevm, no?

@prestwich
Copy link
Member

I like that Trevm always corresponds to a useable Evm<..> in some state, and the revm Evm can't be partially constructed. They tend to use more of a factory-trait, like this.

I do like the bringing the builder in line with the typestate API tho

@Philogy
Copy link
Contributor Author

Philogy commented Oct 23, 2025

Yeah actually trying to write it out, even for "just 4" fields, Builder already seems cleaner than providing 16 variants based on what you want the default for, or even just 8 is a lot already if you assume they'll always manually chose the db.

In terms of the builder API is it ok how I did it? I noticed for Trevm you put the type states behind a "sealed" module, is there a specific reason for that? Would you like me to do it like that, too?

@Philogy
Copy link
Contributor Author

Philogy commented Oct 23, 2025

Since db is required a simpler way for the builder might be to just make db an input to new:

impl<Db: Database> TrevmBuilder<NoOpInspector, Db> {
    /// Create a new builder with the default database and inspector.
    #[allow(clippy::new_without_default)] // default would make bad devex :(
    pub const fn new(db: Db) -> Self {
        Self { insp: NoOpInspector, spec: SpecId::PRAGUE, precompiles: None, db }
    }
}

Like this you have the same type safety of a db always being set but without the extra typestate types & methods.

@prestwich
Copy link
Member

this is more of a patterns question. I think a builder should start "empty" and then be populated piece by piece. I always like it when a bulider can be Bulider::new(). This also means that the builder could take a DbConnect or a Db or something else

there's also a bit of tension in that we have both TrevmBuilder and the EvmFactory trait, but they're both useful in different situations >.>

@prestwich prestwich merged commit 2504622 into init4tech:main Nov 3, 2025
6 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