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

Make evm.db not optional #713

Conversation

alessandromazza98
Copy link
Contributor

Closes #697

I removed the Option<DB> inside the EVM struct to just leave DB.

Let me know if it's ok.
Thanks

Copy link
Collaborator

@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.

This is great, just a few things

crates/revm/src/evm.rs Outdated Show resolved Hide resolved
crates/revm/src/evm.rs Outdated Show resolved Hide resolved
crates/revm/src/evm.rs Outdated Show resolved Hide resolved
crates/revm/src/evm.rs Outdated Show resolved Hide resolved
crates/revm/src/evm.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

lgtm, nits you can ignore

let mut evm = revm::new();
evm.database(&mut state);
let mut evm = revm::new(Default::default());
*evm.db() = state;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can move this into the new call above

let mut evm = revm::new();
evm.database(BenchmarkDB::new_bytecode(Bytecode::new()));
let mut evm = revm::new(Default::default());
*evm.db() = BenchmarkDB::new_bytecode(Bytecode::new());
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here and in this file

}

pub fn new<DB>() -> EVM<DB> {
EVM::new()
pub fn new<DB>(db: DB) -> EVM<DB> {
Copy link
Member

Choose a reason for hiding this comment

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

Would love to refresh this in the future, so to not break API two times can you revert this?

Comment on lines -179 to 160
pub fn new() -> Self {
Self::with_env(Default::default())
pub fn new(db: DB) -> Self {
Self::with_env(Default::default(), db)
}

/// Creates a new [EVM] instance with the given environment.
pub fn with_env(env: Env) -> Self {
Self { env, db: None }
}

pub fn database(&mut self, db: DB) {
self.db = Some(db);
}

pub fn db(&mut self) -> Option<&mut DB> {
self.db.as_mut()
pub fn with_env(env: Env, db: DB) -> Self {
Self { env, db }
}

pub fn take_db(&mut self) -> DB {
core::mem::take(&mut self.db).unwrap()
pub fn db(&mut self) -> &mut DB {
&mut self.db
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert this so as to not break functions

pub fn new() -> Self { should init with EmptyDB this would remove a need to have Option and would make code nicer

Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

Code looks a lot cleaner without Option, but I would restrain from breaking current functions as this is something I want to rework in near future so it does not make sense to break it twice

@alessandromazza98
Copy link
Contributor Author

alessandromazza98 commented Sep 14, 2023

Code looks a lot cleaner without Option, but I would restrain from breaking current functions as this is something I want to rework in near future so it does not make sense to break it twice

Ok I can revert those changes, but I am now struggling because without breaking those functions is not easy for me.

In particular I have the problem that, if I delete the Option, then I have problems in with_env and take_db functions.

impl<DB> EVM<DB> {
    /// Creates a new [EVM] instance with the default environment,
    pub fn new() -> Self {
        Self::with_env(Default::default())
    }

    /// Creates a new [EVM] instance with the given environment.
    pub fn with_env(env: Env) -> Self {
        Self { env, db: None }
    }                      ^---> here 

    pub fn database(&mut self, db: DB) {
        self.db = db;
    }

    pub fn db(&mut self) -> &mut DB {
        &mut self.db
    }

    pub fn take_db(&mut self) -> DB {
        core::mem::take(&mut self.db)
    }                       ^---> here
}

One way to solve those errors is to add DB: Default trait in the impl and do like this:

impl<DB: Default> EVM<DB> {
    /// Creates a new [EVM] instance with the default environment,
    pub fn new() -> Self {
        Self::with_env(Default::default())
    }

    /// Creates a new [EVM] instance with the given environment.
    pub fn with_env(env: Env) -> Self {
        Self { env, db: Default::default() }
    }                              

    pub fn database(&mut self, db: DB) {
        self.db = db;
    }

    pub fn db(&mut self) -> &mut DB {
        &mut self.db
    }

    pub fn take_db(&mut self) -> DB {
        core::mem::take(&mut self.db)
    }                               
}

But then I have to put the Default constrain also to the new function and also in the impl of the Default trait for EVM.

So I tried do do as you suggested: init with an EmptyDB db, doing something like this:

impl<DB> EVM<DB> {
    /// Creates a new [EVM] instance with the default environment,
    pub fn new() -> Self {
        Self::with_env(Default::default())
    }

    /// Creates a new [EVM] instance with the given environment.
    pub fn with_env(env: Env) -> Self {
        Self { env, db: EmptyDB::default() }
    }                                     ^--> here I put EmptyDB

    pub fn database(&mut self, db: DB) {
        self.db = db;
    }

    pub fn db(&mut self) -> &mut DB {
        &mut self.db
    }

    pub fn take_db(&mut self) -> DB {
        core::mem::take(&mut self.db)
    }
}

But of course this is not good since the expected type is DB and not EmptyDB.

If you could give me some pointers about how to start tackling this problem I can try and solve it.

Thank you very much

@rakita
Copy link
Member

rakita commented Sep 19, 2023

@alessandromazza98 you are right, let us move this to later after we create new revm version

@rakita
Copy link
Member

rakita commented Dec 5, 2023

This is getting changed a lot inside EvmBuilder #888

@rakita rakita closed this Dec 5, 2023
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.

Make EVM.db not optional
3 participants