Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[WIP] Replace the service builder with an experimental builder trait #6600

Closed
wants to merge 22 commits into from

Conversation

expenses
Copy link
Contributor

@expenses expenses commented Jul 7, 2020

I mentioned here: #6557 (comment) that I had an idea of how we could build the service in a different way. This PR is my progress so far on this idea. The basic idea is that we have a sc_service::builder::Builder trait, which we implement for each chain, e.g. node-template. This trait consists of a lot of type definitions:

use sc_service::builder::{self, *, Builder as _};

pub struct Builder;

impl builder::Builder<Block, RuntimeApi, Executor> for Builder {
	type TransactionPoolBuilder = BasicPoolBuilder;
	type BlockImportBuilder = GrandpaBlockImportBuilder;
	type ImportQueueBuilder = AuraImportQueueBuilder<Self::BlockImportBuilder>;
	type FinalityProofProviderBuilder = GrandpaFinalityProofProviderBuilder;
	type SelectChainBuilder = LongestChainBuilder;
}

Builder has 3 provided functions: build_light, build_full, build_ops. These use the type definitions to build the service components.

The biggest advantage of a system like this is that if you're happy with the provided builders, the amount of code you have to write is very small, but if you're not, you only have to re-write the parts you're interested in.

@expenses expenses requested review from cecton and bkchr July 7, 2020 15:26
let (block_import, grandpa_link) =
import_setup.take()
.expect("Link Half and Block Import are present for Full Services or setup failed before. qed");
pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand the idea correctly. It means you can move the content of that function to fn build_full(config) in the Builder impl below, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very cool imo. Nice API! Maybe Basti will have more insights on this but I think this is what we need.

@@ -68,8 +69,7 @@ pub fn run() -> sc_cli::Result<()> {
Some(subcommand) => {
let runner = cli.create_runner(subcommand)?;
runner.run_subcommand(subcommand, |config| {
let (builder, _, _) = new_full_start!(config);
Ok(builder.to_chain_ops_parts())
crate::service::Builder::build_ops(config)
})
}
None => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then here you can call Builder::new_light and Builder::new_full instead of having functions, right?

Comment on lines 71 to +72
runner.run_subcommand(subcommand, |config| {
let (builder, _, _) = new_full_start!(config);
Ok(builder.to_chain_ops_parts())
crate::service::Builder::build_ops(config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize now that subcommand and ops are actually tight together and should probably share a name. Maybe we should rename run_subcommand to run_ops.

More context/info and thought: "ops" or something else, it doesn't matter much but it must clearly identifies the builtin operations we can do on the chain. The word "subcommand" is more generic and cli-oriented. run_subcommand is not actually used to run "any" subcommand, it's only for ops subcommands. Extra subcommands use run_sync and run_async

@expenses
Copy link
Contributor Author

Closing for the time being due to #4587 (comment).

@expenses expenses closed this Jul 20, 2020
@cecton
Copy link
Contributor

cecton commented Jul 22, 2020

(If you plan to re-open you can use the tag "onice" instead)

@expenses expenses deleted the ashley-service-builder-experimental branch August 2, 2020 15:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants