-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: Feature gate traits inside storage-api that still depend on db-api #14647
base: main
Are you sure you want to change the base?
feat: Feature gate traits inside storage-api that still depend on db-api #14647
Conversation
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.
ty, a few suggestions
#[cfg(feature = "db-api")] | ||
use reth_db_api::table::Table; | ||
use reth_storage_errors::provider::ProviderResult; | ||
|
||
/// The trait for fetching provider statistics. | ||
#[cfg(feature = "db-api")] |
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.
these we don't need if we instead put the feature above mod stats;
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.
will fix, leftover from iterations.
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.
done
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.
this should be compatible?
so we dont need a feature here?
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.
BlockNumberAddress
is declared inside of dp-api, and the StorageChangeSetReader
trait uses that, hence the need for the feature over that trait alone
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.
right, actually we should move these to db-models
opening an issue for this
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.
Will take it up. That particular type is indeed supposed to live in db-models.
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.
I think here we want to group all of the db-api traits into its own module, like
#[cfg("db-api)]
pub use db::*;
#[cfg("db-api)]
mod db {
....
}
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.
noted, cleaner too. Will fix
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.
done
Feature gate traits inside storage-api that still depend on db-api, updates reth-provider features flag for storage-api dependency to include new db-api feature