-
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: introduce ProviderFactoryBuilder #13989
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.
Cool, given the number of types we need to configure for a builder here a typestate builder for this makes sense. I have some initial questions about fn usage
|
||
impl<N> ProviderFactoryBuilder<N> { | ||
/// Maps the [`NodeTypes`] of this builder. | ||
pub fn types<T>(self) -> ProviderFactoryBuilder<T> { |
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.
How would this fn be used? Would it be used in combination with eg a constructor method on ProviderFactoryBuilder<()>
So the pattern would be something like
// Hopefully don't have to specify N for Default if planning on configuring types with the fn
let builder = ProviderFactoryBuilder::default()
.types(types)
.db(db)
.chain_spec(spec)
.static_files(sf)
.build()
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.
yep,
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.
Nice, this makes provider factory init super simple
/// Settings for how to open the database and static files. | ||
/// | ||
/// The default derivation from a path assumes the path is the datadir: | ||
/// [`ReadOnlyConfig::from_datadir`] | ||
#[derive(Debug, Clone)] | ||
pub struct ReadOnlyConfig { | ||
/// The path to the database directory. | ||
pub db_dir: PathBuf, | ||
/// How to open the database | ||
pub db_args: DatabaseArguments, | ||
/// The path to the static file dir | ||
pub static_files_dir: PathBuf, | ||
/// Whether the static files should be watched for changes. | ||
pub watch_static_files: bool, | ||
} | ||
|
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 like this
towards #13988
this is far from done, hence this doesn't update all
ProviderFactory::new
instances.This is introduces a builder helper type for the ProviderFactory.
it is intended that this will be instantiated via the node type (
EthereumNode::provider_factory_builder
).This Pr introduces various builder stage types,
TypesAndX
that could potentially be reused for additional builders.their purpose is to attach additional values and steer the user via appropriately named functions.
As followup, I'll look at making db+static file instantiation easier and make this a single builder call.
this pr is the first step to keep changes small