-
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: integrate NodeTypesWithDB
#10698
Conversation
26ee4fd
to
acc020e
Compare
@@ -63,14 +64,14 @@ pub type PipelineWithResult<DB> = (Pipeline<DB>, Result<ControlFlow, PipelineErr | |||
/// # Defaults | |||
/// | |||
/// The [`DefaultStages`](crate::sets::DefaultStages) are used to fully sync reth. | |||
pub struct Pipeline<DB: Database> { | |||
pub struct Pipeline<N: NodeTypesWithDB> { |
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.
not clear to me why we need the full node types 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.
basically it's needed because pipeline currently depends on concrete ProviderFactory
which needs chainspec AT
we can indeed avoid this by doing <DB, Provider: DatabaseProviderFactory<DB> + ...>
, but given that we're going to make providers generic over primitive types this DB generic would at some point become NodeTypesWithDB
or equivalent anyway
c5f3f83
to
12b5d45
Compare
Co-authored-by: joshieDo <[email protected]>
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.
very nice,
granted this introduces additional verbosity, but this is necessary if we want to integrate type generics on provider/db level
there are def some simplifications we can when it comes to the NodeWith trait, but imo it's nice to group db+types in on trait and the caller is responsible to instantiate this
the nice benefit of this is that we can get rid of a ton of DB generics, which is nice.
we also only can use the DB together with types so it makes sense to group those imo.
followup simplifications we can try are
- stacked trait bounds for engine, this requires some nodebuilder changes which we def need to do
- move away from
ChainSpec
as the default type
NodeTypesWithDB
NodeTypesWithDB
following #10683 integrates
NodeTypesWithDB
for use in providers and to types depending on them.This introcuces a lot of
ChainSpec = ChainSpec
bounds comming from concrete provider implementations. I've created a helper traitProviderNodeTypes
reth/crates/storage/provider/src/providers/mod.rs
Line 64 in 12b5d45
this trait can be removed/relaxed once we figure out requirements for abstracted chainspec