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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/database/interface/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ serde = { version = "1.0", default-features = false, features = [
], optional = true }

# asyncdb
tokio = { version = "1.40", optional = true }
tokio = { version = "1.40", optional = true, features = ["rt", "rt-multi-thread"] }


[dev-dependencies]
Expand Down
4 changes: 2 additions & 2 deletions crates/database/interface/src/async_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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


/// Get basic account information.
fn basic_async(
Expand Down Expand Up @@ -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


/// Get basic account information.
fn basic_async_ref(
Expand Down
4 changes: 2 additions & 2 deletions crates/database/interface/src/empty_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ impl<E> EmptyDBTyped<E> {
}
}

impl<E> Database for EmptyDBTyped<E> {
impl<E: core::error::Error> Database for EmptyDBTyped<E> {
type Error = E;

#[inline]
Expand All @@ -76,7 +76,7 @@ impl<E> Database for EmptyDBTyped<E> {
}
}

impl<E> DatabaseRef for EmptyDBTyped<E> {
impl<E: core::error::Error> DatabaseRef for EmptyDBTyped<E> {
type Error = E;

#[inline]
Expand Down
6 changes: 3 additions & 3 deletions crates/database/interface/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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


/// Get basic account information.
fn basic(&mut self, address: Address) -> Result<Option<AccountInfo>, Self::Error>;
Expand All @@ -38,7 +38,7 @@ pub trait Database {

/// EVM database commit interface.
#[auto_impl(&mut, Box)]
pub trait DatabaseCommit {
pub trait DatabaseCommit: Database {
/// Commit changes to the database.
fn commit(&mut self, changes: HashMap<Address, Account>);
}
Expand All @@ -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


/// Get basic account information.
fn basic_ref(&self, address: Address) -> Result<Option<AccountInfo>, Self::Error>;
Expand Down
2 changes: 1 addition & 1 deletion crates/database/src/in_memory_db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl<ExtDB: DatabaseRef> CacheDB<ExtDB> {
}
}

impl<ExtDB> DatabaseCommit for CacheDB<ExtDB> {
impl<ExtDB: DatabaseRef> DatabaseCommit for CacheDB<ExtDB> {
fn commit(&mut self, changes: HashMap<Address, Account>) {
for (address, mut account) in changes {
if !account.is_touched() {
Expand Down
2 changes: 1 addition & 1 deletion crates/database/src/states/state_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ impl<DB: Database> StateBuilder<DB> {
}

/// With boxed version of database.
pub fn with_database_boxed<Error>(
pub fn with_database_boxed<Error: core::error::Error>(
self,
database: DBBox<'_, Error>,
) -> StateBuilder<DBBox<'_, Error>> {
Expand Down
4 changes: 2 additions & 2 deletions examples/database_components/src/block_hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ use std::sync::Arc;

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

/// Get block hash by block number
fn block_hash(&mut self, number: u64) -> Result<B256, Self::Error>;
}

#[auto_impl(&, &mut, Box, Rc, Arc)]
pub trait BlockHashRef {
type Error;
type Error: core::error::Error + 'static;

/// Get block hash by block number
fn block_hash(&self, number: u64) -> Result<B256, Self::Error>;
Expand Down
31 changes: 28 additions & 3 deletions examples/database_components/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
//! Optimism-specific constants, types, and helpers.
#![cfg_attr(not(test), warn(unused_crate_dependencies))]

//! Database that is split on State and BlockHash traits.
Expand Down Expand Up @@ -26,7 +25,33 @@ pub enum DatabaseComponentError<SE, BHE> {
BlockHash(BHE),
}

impl<S: State, BH: BlockHash> Database for DatabaseComponents<S, BH> {
impl<SE, BHE> core::fmt::Display for DatabaseComponentError<SE, BHE>
where
SE: core::fmt::Display,
BHE: core::fmt::Display,
{
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
match self {
DatabaseComponentError::State(e) => write!(f, "State error: {}", e),
DatabaseComponentError::BlockHash(e) => write!(f, "BlockHash error: {}", e),
}
}
}

impl<SE, BHE> core::error::Error for DatabaseComponentError<SE, BHE>
where
SE: core::error::Error + 'static,
BHE: core::error::Error + 'static,
{
fn source(&self) -> Option<&(dyn core::error::Error + 'static)> {
match self {
DatabaseComponentError::State(e) => Some(e),
DatabaseComponentError::BlockHash(e) => Some(e),
}
}
}

impl<S: State, BH: BlockHashRef> Database for DatabaseComponents<S, BH> {
type Error = DatabaseComponentError<S::Error, BH::Error>;

fn basic(&mut self, address: Address) -> Result<Option<AccountInfo>, Self::Error> {
Expand Down Expand Up @@ -78,7 +103,7 @@ impl<S: StateRef, BH: BlockHashRef> DatabaseRef for DatabaseComponents<S, BH> {
}
}

impl<S: DatabaseCommit, BH: BlockHashRef> DatabaseCommit for DatabaseComponents<S, BH> {
impl<S: State + DatabaseCommit, BH: BlockHashRef> DatabaseCommit for DatabaseComponents<S, BH> {
fn commit(&mut self, changes: HashMap<Address, Account>) {
self.state.commit(changes);
}
Expand Down
4 changes: 2 additions & 2 deletions examples/database_components/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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


/// Get basic account information.
fn basic(&mut self, address: Address) -> Result<Option<AccountInfo>, Self::Error>;
Expand All @@ -24,7 +24,7 @@ pub trait State {

#[auto_impl(&, &mut, Box, Rc, Arc)]
pub trait StateRef {
type Error;
type Error: core::error::Error + 'static;

/// Get basic account information.
fn basic(&self, address: Address) -> Result<Option<AccountInfo>, Self::Error>;
Expand Down
Loading