Companion PR for Move subcommands from sc-cli to nodes #6948

[expenses]

paritytech/substrate#6948

[cecton]

It would be nice if those comments could be moved to sc-cli instead so people don't need to duplicate them on every project.

(I don't know if this is possible!)

[expenses]

Yeah, not sure. If I don't comment these lines there's still some text for them in the --help display, but it's for some of the options for the command, not the command itself. Weird.

[cecton]

I'm not 100% sure but since we do have to repeat ourselves a lot, we'd better make it easier by using the same pattern everywhere. Is it possible to use async_run everywhere? Even if the code is blocking, it doesn't really matter that it in a future or not (little bit of overhead, that's ok).

[expenses]

You need a TaskManager to pass into async_run (unless we make that an Option<TaskManager>?), and it seems like a waste to build one just for that.

[cecton]

TaskManager?? 😁 That was not my idea when I made it. The run_node_until_exit was meant to use a task manager but sync_run and async_run where meant to run without task manager. Apparently it has been changed in paritytech/substrate#6543

On the bright side: async_run seems not to be used at all anywhere except in this PR. Not even here in polkadot. So we can still change the course if we want.

It might be worth considering really only using a future in async_run and let the code inside handle any task manager they night have (and call clean_shutdown when the command needs to stop the node for example).

[seunlanlege]

Would it be possible for us to have an fn that does all this?

and we call it so

async_run(&cli, cmd)

where cmd is a generic trait with the run method

and it internally does all these runtime checks?

[expenses]

So I tried, this and came up with:

fn run_cmd<CMD, FN, B>(cli: &Cli, cmd: &CMD, func: FN) -> Result<()>
	where
		FN: FnOnce(sc_cli::Runner<Cli>, &CMD) -> Result<()>,
		CMD: sc_cli::CliConfiguration + sc_cli::SubstrateCli,
{
	let runner = cli.create_runner(cmd)?;
	let chain_spec = &runner.config().chain_spec;

	set_default_ss58_version(chain_spec);

	if chain_spec.is_kusama() {
		func::<service::kusama_runtime::Block, service::KusamaExecutor>(runner, cmd)
	} else if chain_spec.is_westend() {
		func::<service::westend_runtime::Block, service::WestendExecutor>(runner, cmd)
	} else {
		func::<service::polkadot_runtime::Block, service::PolkadotExecutor>(runner, cmd)
	}
}

Unfortunately, rust does not allow you to put generics on a FnOnce like this:

error[E0109]: type arguments are not allowed for this type
   --> cli/src/command.rs:366:10
    |
366 |         func::<service::kusama_runtime::Block, service::KusamaExecutor>(runner, cmd)
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type argument not allowed

error[E0109]: type arguments are not allowed for this type
   --> cli/src/command.rs:368:10
    |
368 |         func::<service::westend_runtime::Block, service::WestendExecutor>(runner, cmd)
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type argument not allowed

error[E0109]: type arguments are not allowed for this type
   --> cli/src/command.rs:370:10
    |
370 |         func::<service::polkadot_runtime::Block, service::PolkadotExecutor>(runner, cmd)
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type argument not allowed

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0109`.
error: could not compile `polkadot-cli`.

You get the same problem with a macro as well.

[seunlanlege]

What about:

fn run_cmd<CMD, B>(cli: &Cli, cmd: &CMD) -> Result<()>
	where
		CMD: sc_cli::CliConfiguration + sc_cli::SubstrateCli,
{
	let runner = cli.create_runner(cmd)?;
	let chain_spec = &runner.config().chain_spec;

	set_default_ss58_version(chain_spec);

	if chain_spec.is_kusama() {
    runner.async_run(|mut config| {
		  let (client, _, import_queue, task_manager) = service::new_chain_ops::<
			  service::kusama_runtime::RuntimeApi,
			  service::KusamaExecutor,
		  >(&mut config)?;
		  Ok((cmd.run(client, import_queue), task_manager))
	  })
	} else if chain_spec.is_westend() {
    runner.async_run(|mut config| {
		  let (client, _, import_queue, task_manager) = service::new_chain_ops::<
			  service::westend_runtime::RuntimeApi,
			  service::WestEndExecutor,
		  >(&mut config)?;
		  Ok((cmd.run(client, import_queue), task_manager))
	  })
	} else {
    runner.async_run(|mut config| {
		  let (client, _, import_queue, task_manager) = service::new_chain_ops::<
			  service::polkadot_runtime::RuntimeApi,
			  service::PolkadotExecutor,
		  >(&mut config)?;
		  Ok((cmd.run(client, import_queue), task_manager))
	  })
	}
}

[expenses]

The different subcommands have different inputs unfortunately.

[seunlanlege]

In that case i think it's fine for now

[cecton]

I think you can achieve this if you use the Client enum that Basti introduced recently: https://github.com/paritytech/polkadot/blob/master/service/src/client.rs#L138

You would need to make the cmd.run accepts that Client enum.

Ideally we should move the if kusama { ... } else if westend { ... } else ... { ... } pattern to polkadot service instead of the binary.

[expenses]

Yep, that works.

[seunlanlege]

So much better 👏🏾

[ghost]

Waiting for commit status.

[ghost]

Checks failed; merge aborted.

[expenses]

bot merge

[ghost]

Missing process info; check that the PR belongs to a project column.

Merge can be attempted if:

• The PR has approval from two core-devs (or one if the PR is labelled insubstantial).
• The PR has approval from a member of substrateteamleads.
• The PR is attached to a project column and has approval from the project owner.

See https://github.com/paritytech/parity-processbot#faq