Make --collator reusable and imply --validator (#380)

* Reusable RunCmd struct

* wire new run command through service

* Fill in the rest of the methods

* attempt normalization

* Settle on the borrowing approach

* add the normalize call

* bump substrate

* Update client/cli/src/lib.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Update docs

* Update client/cli/src/lib.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

cumulus/client/cli/src/lib.rs

@@ -19,11 +19,17 @@
 #![warn(missing_docs)]
 
 use sc_cli;
+use sc_service::{
+	BasePath,
+	config::{TelemetryEndpoints, PrometheusConfig},
+	TransactionPoolOptions,
+};
 use std::{
 	fs,
 	io::{self, Write},
 };
 use structopt::StructOpt;
+use std::net::SocketAddr;
 
 /// The `purge-chain` command used to remove the whole chain: the parachain and the relaychain.
 #[derive(Debug, StructOpt)]
@@ -112,3 +118,134 @@ impl sc_cli::CliConfiguration for PurgeChainCmd {
 		Some(&self.base.database_params)
 	}
 }
+
+/// The `run` command used to run a node.
+#[derive(Debug, StructOpt)]
+pub struct RunCmd {
+	/// The cumulus RunCmd inherents from sc_cli's
+	#[structopt(flatten)]
+	pub base: sc_cli::RunCmd,
+
+	/// Id of the parachain this collator collates for.
+	#[structopt(long)]
+	pub parachain_id: Option<u32>,
+
+	/// Run node as collator.
+	///
+	/// Note that this is the same as running with `--validator`.
+	#[structopt(long, conflicts_with = "validator")]
+	pub collator: bool,
+}
+
+/// A non-redundant version of the `RunCmd` that sets the `validator` field when the
+/// original `RunCmd` had the `colaltor` field.
+/// This is how we make `--collator` imply `--validator`.
+pub struct NormalizedRunCmd {
+	/// The cumulus RunCmd inherents from sc_cli's
+	pub base: sc_cli::RunCmd,
+	/// Id of the parachain this collator collates for.
+	pub parachain_id: Option<u32>,
+}
+
+impl RunCmd {
+	/// Create a [`NormalizedRunCmd`] which merges the `collator` cli argument into `validator` to have only one.
+	pub fn normalize(&self) -> NormalizedRunCmd {
+		let mut new_base = self.base.clone();
+
+		 new_base.validator = self.base.validator || self.collator;
+
+		 NormalizedRunCmd { 
+			 base: new_base,
+			 parachain_id: self.parachain_id,
+		}
+	}
+}
+
+impl sc_cli::CliConfiguration for NormalizedRunCmd {
+	fn shared_params(&self) -> &sc_cli::SharedParams {
+		self.base.shared_params()
+	}
+
+	fn import_params(&self) -> Option<&sc_cli::ImportParams> {
+		self.base.import_params()
+	}
+
+	fn network_params(&self) -> Option<&sc_cli::NetworkParams> {
+		self.base.network_params()
+	}
+
+	fn keystore_params(&self) -> Option<&sc_cli::KeystoreParams> {
+		self.base.keystore_params()
+	}
+
+	fn offchain_worker_params(&self) -> Option<&sc_cli::OffchainWorkerParams> {
+		self.base.offchain_worker_params()
+	}
+
+	fn node_name(&self) -> sc_cli::Result<String> {
+		self.base.node_name()
+	}
+
+	fn dev_key_seed(&self, is_dev: bool) -> sc_cli::Result<Option<String>> {
+		self.base.dev_key_seed(is_dev)
+	}
+
+	fn telemetry_endpoints(
+		&self,
+		chain_spec: &Box<dyn sc_cli::ChainSpec>,
+	) -> sc_cli::Result<Option<TelemetryEndpoints>> {
+		self.base.telemetry_endpoints(chain_spec)
+	}
+
+	fn role(&self, is_dev: bool) -> sc_cli::Result<sc_cli::Role> {
+		self.base.role(is_dev)
+	}
+
+	fn force_authoring(&self) -> sc_cli::Result<bool> {
+		self.base.force_authoring()
+	}
+
+	fn prometheus_config(&self, default_listen_port: u16) -> sc_cli::Result<Option<PrometheusConfig>> {
+		self.base.prometheus_config(default_listen_port)
+	}
+
+	fn disable_grandpa(&self) -> sc_cli::Result<bool> {
+		self.base.disable_grandpa()
+	}
+
+	fn rpc_ws_max_connections(&self) -> sc_cli::Result<Option<usize>> {
+		self.base.rpc_ws_max_connections()
+	}
+
+	fn rpc_cors(&self, is_dev: bool) -> sc_cli::Result<Option<Vec<String>>> {
+		self.base.rpc_cors(is_dev)
+	}
+
+	fn rpc_http(&self, default_listen_port: u16) -> sc_cli::Result<Option<SocketAddr>> {
+		self.base.rpc_http(default_listen_port)
+	}
+
+	fn rpc_ipc(&self) -> sc_cli::Result<Option<String>> {
+		self.base.rpc_ipc()
+	}
+
+	fn rpc_ws(&self, default_listen_port: u16) -> sc_cli::Result<Option<SocketAddr>> {
+		self.base.rpc_ws(default_listen_port)
+	}
+
+	fn rpc_methods(&self) -> sc_cli::Result<sc_service::config::RpcMethods> {
+		self.base.rpc_methods()
+	}
+
+	fn transaction_pool(&self) -> sc_cli::Result<TransactionPoolOptions> {
+		self.base.transaction_pool()
+	}
+
+	fn max_runtime_instances(&self) -> sc_cli::Result<Option<usize>> {
+		self.base.max_runtime_instances()
+	}
+
+	fn base_path(&self) -> sc_cli::Result<Option<BasePath>> {
+		self.base.base_path()
+	}
+}

cumulus/rococo-parachains/src/cli.rs

@@ -90,24 +90,6 @@ pub struct ExportGenesisWasmCommand {
 	pub chain: Option<String>,
 }
 
-#[derive(Debug, StructOpt)]
-pub struct RunCmd {
-	#[structopt(flatten)]
-	pub base: sc_cli::RunCmd,
-
-	/// Id of the parachain this collator collates for.
-	#[structopt(long)]
-	pub parachain_id: Option<u32>,
-}
-
-impl std::ops::Deref for RunCmd {
-	type Target = sc_cli::RunCmd;
-
-	fn deref(&self) -> &Self::Target {
-		&self.base
-	}
-}
-
 #[derive(Debug, StructOpt)]
 #[structopt(settings = &[
 	structopt::clap::AppSettings::GlobalVersion,
@@ -119,13 +101,7 @@ pub struct Cli {
 	pub subcommand: Option<Subcommand>,
 
 	#[structopt(flatten)]
-	pub run: RunCmd,
-
-	/// Run node as collator.
-	///
-	/// Note that this is the same as running with `--validator`.
-	#[structopt(long, conflicts_with = "validator")]
-	pub collator: bool,
+	pub run: cumulus_client_cli::RunCmd,
 
 	/// Relaychain arguments
 	#[structopt(raw = true)]

cumulus/rococo-parachains/src/command.rs

@@ -262,7 +262,7 @@ pub fn run() -> Result<()> {
 			Ok(())
 		}
 		None => {
-			let runner = cli.create_runner(&*cli.run)?;
+			let runner = cli.create_runner(&cli.run.normalize())?;
 			let use_shell = use_shell_runtime(&runner.config().chain_spec);
 
 			runner.run_node_until_exit(|config| async move {
@@ -295,20 +295,19 @@ pub fn run() -> Result<()> {
 					task_executor,
 				)
 				.map_err(|err| format!("Relay chain argument error: {}", err))?;
-				let collator = cli.run.base.validator || cli.collator;
 
 				info!("Parachain id: {:?}", id);
 				info!("Parachain Account: {}", parachain_account);
 				info!("Parachain genesis state: {}", genesis_state);
-				info!("Is collating: {}", if collator { "yes" } else { "no" });
+				info!("Is collating: {}", if config.role.is_authority() { "yes" } else { "no" });
 
 				if use_shell {
-					crate::service::start_shell_node(config, key, polkadot_config, id, collator)
+					crate::service::start_shell_node(config, key, polkadot_config, id)
 						.await
 						.map(|r| r.0)
 						.map_err(Into::into)
 				} else {
-					crate::service::start_node(config, key, polkadot_config, id, collator)
+					crate::service::start_node(config, key, polkadot_config, id)
 						.await
 						.map(|r| r.0)
 						.map_err(Into::into)

cumulus/rococo-parachains/src/service.rs

@@ -146,7 +146,6 @@ async fn start_node_impl<RuntimeApi, Executor, RB>(
 	collator_key: CollatorPair,
 	polkadot_config: Configuration,
 	id: ParaId,
-	validator: bool,
 	rpc_ext_builder: RB,
 ) -> sc_service::error::Result<(TaskManager, Arc<TFullClient<Block, RuntimeApi, Executor>>)>
 where
@@ -198,6 +197,7 @@ where
 		polkadot_full_node.backend.clone(),
 	);
 
+	let validator = parachain_config.role.is_authority();
 	let prometheus_registry = parachain_config.prometheus_registry().cloned();
 	let transaction_pool = params.transaction_pool.clone();
 	let mut task_manager = params.task_manager;
@@ -295,17 +295,14 @@ pub async fn start_node(
 	collator_key: CollatorPair,
 	polkadot_config: Configuration,
 	id: ParaId,
-	validator: bool,
-) -> sc_service::error::Result<(
-	TaskManager,
-	Arc<TFullClient<Block, parachain_runtime::RuntimeApi, RuntimeExecutor>>,
-)> {
+) -> sc_service::error::Result<
+	(TaskManager, Arc<TFullClient<Block, parachain_runtime::RuntimeApi, RuntimeExecutor>>)
+> {
 	start_node_impl::<parachain_runtime::RuntimeApi, RuntimeExecutor, _>(
 		parachain_config,
 		collator_key,
 		polkadot_config,
 		id,
-		validator,
 		|_| Default::default(),
 	)
 	.await
@@ -317,17 +314,14 @@ pub async fn start_shell_node(
 	collator_key: CollatorPair,
 	polkadot_config: Configuration,
 	id: ParaId,
-	validator: bool,
-) -> sc_service::error::Result<(
-	TaskManager,
-	Arc<TFullClient<Block, shell_runtime::RuntimeApi, ShellRuntimeExecutor>>,
-)> {
+) -> sc_service::error::Result<
+	(TaskManager, Arc<TFullClient<Block, shell_runtime::RuntimeApi, ShellRuntimeExecutor>>)
+> {
 	start_node_impl::<shell_runtime::RuntimeApi, ShellRuntimeExecutor, _>(
 		parachain_config,
 		collator_key,
 		polkadot_config,
 		id,
-		validator,
 		|_| Default::default(),
 	)
 	.await