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

refactor: make stricter database trait bounds for user ergonomics #1840

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

prestwich
Copy link
Contributor

@prestwich prestwich commented Oct 25, 2024

Two quick ergonomics changes for downstream users. Intended to simplify bounds writing

This is a breaking change, as downstream implementors of these traits may need to be updated


Error bounds for database errors

Adds an error bound for Database::Error and DatabaseRef::Error, and the async versions. This closes #1674. This prevents propagation of complex bounds for upstream wrapper structs, particularly error types that might wrap EVM output.

motivation:
simplify bounds writing for users by ensuring they don't need to manually write out error bounds. This is particulartly a problem when holding an instance of EVMError<Db>

Before:

use revm::{primitives::EVMError, Database};

pub struct Error<Db: Database<Error: core::error::Error>>(EVMError<Db::Error>);

impl<Db: Database<Error: core::error::Error>> From<EVMError<Db::Error>> for Error<Db>
{
    fn from(e: EVMError<Db::Error>) -> Self {
        Self(e)
    }
}

impl<Db: Database<Error: core::error::Error>> core::error::Error for Error<Db> {
    fn source(&self) -> Option<&(dyn Error + 'static)> {
        Some(&self.0)
    }
}

After:

use revm::{primitives::EVMError, Database};

pub struct Error<Db: Database>(EVMError<Db::Error>);

impl<Db> From<EVMError<Db::Error>> for Error<Db>
where
    Db: Database,
{
    fn from(e: EVMError<Db::Error>) -> Self {
        Self(e)
    }
}

impl<Db: Database> core::error::Error for Error<Db> {
    fn source(&self) -> Option<&(dyn Error + 'static)> {
        Some(&self.0)
    }
}

Add Database as a supertrait of DatabaseCommit

This does not close a current issue, but I'm happy to make one. This causes some bounds changes in the database_components examples

motivation: simplify bounds writing for users by allowing them to avoid specifying Database

alternate approach: a trait CommitableDatabase: Database + DatabaseCommit with a blanket impl<T> CommittableDatabase for T where T: Database + DatabaseCommit

Code before:

use revm::{Database, DatabaseCommit};
pub fn make_writable_evm<Db: Database + DatabaseCommit>(db: Db) -> Evm<'static, (), Db> { ... } 

Code after:

use revm::{DatabaseCommit};
pub fn make_writable_evm<Db: DatabaseCommit>(db: Db) -> Evm<'static, (), Db> { ... } 

Add WrapBlockHashRef to example

Add a wrapper struct like WrapDatabaseRef to BlockHashRef to allow relaxing the bounds on the database components in the database_components example


Drive-by:
The asyncdb is cfged out when running cargo c --all-features. And running cargo c -p revm-database-interface --features asyncdb produced compilation errors due to disabled tokio features. I have enabled the necessary features. When running with --workspace it appears some other package enabled the features, and covered up the dep specification issue

@prestwich
Copy link
Contributor Author

prestwich commented Oct 25, 2024

hmmmm. for some reason it seems not all packages build when running cargo t on my local. so this may be more invasive than it appeared 🤔

punting to draft

@prestwich prestwich marked this pull request as draft October 25, 2024 14:50
@prestwich prestwich marked this pull request as ready for review October 25, 2024 15:04
@prestwich
Copy link
Contributor Author

prestwich commented Oct 25, 2024

Ok it seems to be mostly an example that was broken, so I've updated that :)

I think this is ready for consideration. CI failure appears to be unrelated

@@ -10,7 +10,7 @@ use std::sync::Arc;

#[auto_impl(&mut, Box)]
pub trait State {
type Error;
type Error: core::error::Error + 'static;
Copy link
Member

Choose a reason for hiding this comment

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

Why 'static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this case, it's required by the impl :Error for DatabaseComponentError<T, U> here. The bound could be removed here, but it would require all downstream users to propagate 2 bounds every time they want to use that impl Error for DatabaseComponentError (which is pretty often in practice)

Given that we don't have any examples of errors borrowing (i.e. all our current databases would meet this bound) it seems pretty reasonble to put it here too. This is just an example lib, and I don't feel strongly about its ergonomics tho

I would consider adding a + 'static bound to the Error assoc type in trait Database tho, for the same reason. We have no clear use case for errors that capture data instead of own it, and it causes a lot of downstream complexity


/// Wraps a [`BlockHashRef`] to provide a [`BlockHash`] implementation.
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct WrapBlockHashRef<T: BlockHashRef>(pub T);
Copy link
Member

Choose a reason for hiding this comment

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

hm, i can't remember it but would this code

impl<T: BlockHashRef> BlockHash for T {

solve all of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was unnecessary anyway so i removed it

@@ -21,7 +21,7 @@ pub use empty_db::{EmptyDB, EmptyDBTyped};
#[auto_impl(&mut, Box)]
pub trait Database {
/// The database error type.
type Error;
type Error: core::error::Error;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would consider adding a 'static bound here, as described in https://github.com/bluealloy/revm/pull/1840/files#r1819000225

@@ -52,7 +52,7 @@ pub trait DatabaseCommit {
#[auto_impl(&, &mut, Box, Rc, Arc)]
pub trait DatabaseRef {
/// The database error type.
type Error;
type Error: core::error::Error;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would consider adding a 'static bound here, as described in https://github.com/bluealloy/revm/pull/1840/files#r1819000225

@@ -13,7 +13,7 @@ use crate::{Database, DatabaseRef};
/// Use [WrapDatabaseAsync] to provide [Database] implementation for a type that only implements this trait.
pub trait DatabaseAsync {
/// The database error type.
type Error: Send;
type Error: core::error::Error + Send;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would consider adding a 'static bound here, as described in https://github.com/bluealloy/revm/pull/1840/files#r1819000225

@@ -48,7 +48,7 @@ pub trait DatabaseAsync {
/// Use [WrapDatabaseAsync] to provide [DatabaseRef] implementation for a type that only implements this trait.
pub trait DatabaseAsyncRef {
/// The database error type.
type Error: Send;
type Error: core::error::Error + Send;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would consider adding a 'static bound here, as described in https://github.com/bluealloy/revm/pull/1840/files#r1819000225

@prestwich prestwich force-pushed the prestwich/stricter-trait-bounds branch from a14a377 to c44063b Compare November 4, 2024 15:58
@prestwich
Copy link
Contributor Author

CI failure seems to be related to #1844

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.

Feature Request: Add Debug bound to Error types for Database trait
2 participants