reth icon indicating copy to clipboard operation
reth copied to clipboard

Revise `NodeComponentsBuilder` trait

Open emhane opened this issue 7 months ago • 3 comments

Describe the feature

Revise NodeComponentsBuilder, with goal of peeling away abstraction layers + increasing generality.

Design suggestions:

  • Specifying the type we are building from the start, so long as we aren't storing the built type as a trait object later - having the builder output type accessible as AT makes life easier! now we specify the literal type NodeAdapter everywhere, which isn't clearly associated to its builder. We don't really care what the builder type is...it is only relevant on bootstrap...but all other types in the program are constrained by its output type, i.e. the built type....
  • Using all builders as trait objects, these are only called once at start of program, so the extra overhead from dynamic dispatch isn't a problem.
  • Instantiate the builders as late as possible, using trait like new BuilderProvider<N: FullNodeComponents> trait on AT of the type being built. So reversed to first suggestion, let the output type provide its builder as opposed to the builder specifying its output. This way, we don't have to care about the builder being Send + Sync + Clone + Default + ..., just the built type - which probably has those trait bounds imposed on it by how we use that type during the entire execution of the program anyway, i.e. we need to satisfy those anyway for that type.
  • Not having NodeComponents be a strict subset of FullNodeComponents has been painful. It's impossible to reference the transition from a generic NodeComponents to a generic FullNodeComponents type atm. Alt, an intermediary trait that specifies the BlockchainTree type could be added for transit between the NodeComponents and FullNodeComponents traits, see (https://github.com/paradigmxyz/reth/pull/9243).
  • Not having all components returned by getters in FullNodeComponents, defined as AT, has made the code less flexible. Thinking in particular of Network and Tasks generics in reth-rpc-builder, which has now had to be hardcoded to types NetworkHandle and TaskExecutor in several places. Would make way for sense if those types could be referenced via FullNodeComponents, just like Provider or Pool.
  • As little trait bounds on AT definitions as possible - in general. We should add most trait bounds later on impl bodies, where we use that behvaiour, instead of prematurely on the AT definitions of FullNodeTypes, NodeTypes, etc. Delaying adding trait bounds, until the scope where that behaviour is used, makes adding new tests way less effort since we won't have to implement many trait bounds on some MockType to make it AT, just to make a MockNodeBuilder where Self: NodeComponentsBuilder type.
  • Favour ATs over traits with generics. Blanket implementations for T where T: NodeComponents<N> only becomes possible for traits that also have the generic N, which is a shame, since blanket impls of our traits on each other could save us a lot of lines of code. This is also favourable since we can ref the AT from opaque types, but we can't ref the generic.

Additional context

No response

emhane avatar Jul 16 '24 14:07 emhane