Cleanup the cli interface (#1194)

Parameters like `--dev`, `--base-path` or `--chain` were replicated
for each subcommand and were also present in the "top". This pr removes
these parameters from the subcommands.
The subcommands also ignored stuff like `--dev` as it was only read in
the top matcher, that did not look into the subcommand matchers.
Now, if you specify one of the parameters for a subcommand, you will get
an error instead of silently ignoring it.

substrate/core/cli/src/lib.rs

@@ -78,7 +78,7 @@ use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use names::{Generator, Name};
 use regex::Regex;
-use structopt::StructOpt;   
+use structopt::StructOpt;
 pub use params::{CoreParams, CoreCommands, ExecutionStrategy};
 
 use futures::Future;
@@ -116,7 +116,9 @@ pub trait IntoExit {
 fn load_spec<F, G>(matches: &clap::ArgMatches, factory: F) -> Result<ChainSpec<G>, String>
 	where G: RuntimeGenesis, F: FnOnce(&str) -> Result<Option<ChainSpec<G>>, String>,
 {
-	let chain_key = matches.value_of("chain").unwrap_or_else(|| if matches.is_present("dev") { "dev" } else { "" });
+	let chain_key = matches.value_of("chain").unwrap_or_else(
+		|| if matches.is_present("dev") { "dev" } else { "" }
+	);
 	let spec = match factory(chain_key)? {
 		Some(spec) => spec,
 		None => ChainSpec::from_json_file(PathBuf::from(chain_key))?
@@ -316,7 +318,7 @@ where
 	Ok((spec, config))
 }
 
-// 
+//
 // IANA unassigned port ranges that we could use:
 // 6717-6766		Unassigned
 // 8504-8553		Unassigned
@@ -342,28 +344,23 @@ where
 	init_logger(log_pattern);
 	fdlimit::raise_fd_limit();
 
+	let base_path = base_path(matches);
+	let db_path = db_path(&base_path, spec.id());
+
 	if let Some(matches) = matches.subcommand_matches("build-spec") {
 		build_spec::<F>(matches, spec, config)?;
 		return Ok(Action::ExecutedInternally);
-	}
-
-	if let Some(matches) = matches.subcommand_matches("export-blocks") {
-		export_blocks::<F, _>(matches, spec, exit.into_exit())?;
+	} else if let Some(matches) = matches.subcommand_matches("export-blocks") {
+		export_blocks::<F, _>(db_path, matches, spec, exit.into_exit())?;
 		return Ok(Action::ExecutedInternally);
-	}
-
-	if let Some(matches) = matches.subcommand_matches("import-blocks") {
-		import_blocks::<F, _>(matches, spec, exit.into_exit())?;
+	} else if let Some(matches) = matches.subcommand_matches("import-blocks") {
+		import_blocks::<F, _>(db_path, matches, spec, exit.into_exit())?;
 		return Ok(Action::ExecutedInternally);
-	}
-
-	if let Some(matches) = matches.subcommand_matches("revert") {
-		revert_chain::<F>(matches, spec)?;
+	} else if let Some(matches) = matches.subcommand_matches("revert") {
+		revert_chain::<F>(db_path, matches, spec)?;
 		return Ok(Action::ExecutedInternally);
-	}
-
-	if let Some(matches) = matches.subcommand_matches("purge-chain") {
-		purge_chain::<F>(matches, spec)?;
+	} else if let Some(matches) = matches.subcommand_matches("purge-chain") {
+		purge_chain::<F>(db_path)?;
 		return Ok(Action::ExecutedInternally);
 	}
 
@@ -409,12 +406,16 @@ where
 	Ok(())
 }
 
-fn export_blocks<F, E>(matches: &clap::ArgMatches, spec: ChainSpec<FactoryGenesis<F>>, exit: E) -> error::Result<()>
+fn export_blocks<F, E>(
+	db_path: PathBuf,
+	matches: &clap::ArgMatches,
+	spec: ChainSpec<FactoryGenesis<F>>,
+	exit: E
+) -> error::Result<()>
 	where F: ServiceFactory, E: Future<Item=(),Error=()> + Send + 'static,
 {
-	let base_path = base_path(matches);
 	let mut config = service::Configuration::default_with_spec(spec);
-	config.database_path = db_path(&base_path, config.chain_spec.id()).to_string_lossy().into();
+	config.database_path = db_path.to_string_lossy().into();
 	info!("DB path: {}", config.database_path);
 	let from: u64 = match matches.value_of("from") {
 		Some(v) => v.parse().map_err(|_| "Invalid --from argument")?,
@@ -435,12 +436,16 @@ fn export_blocks<F, E>(matches: &clap::ArgMatches, spec: ChainSpec<FactoryGenesi
 	Ok(service::chain_ops::export_blocks::<F, _, _>(config, exit, file, As::sa(from), to.map(As::sa), json)?)
 }
 
-fn import_blocks<F, E>(matches: &clap::ArgMatches, spec: ChainSpec<FactoryGenesis<F>>, exit: E) -> error::Result<()>
+fn import_blocks<F, E>(
+	db_path: PathBuf,
+	matches: &clap::ArgMatches,
+	spec: ChainSpec<FactoryGenesis<F>>,
+	exit: E
+) -> error::Result<()>
 	where F: ServiceFactory, E: Future<Item=(),Error=()> + Send + 'static,
 {
-	let base_path = base_path(matches);
 	let mut config = service::Configuration::default_with_spec(spec);
-	config.database_path = db_path(&base_path, config.chain_spec.id()).to_string_lossy().into();
+	config.database_path = db_path.to_string_lossy().into();
 
 	if let Some(s) = matches.value_of("execution") {
 		config.block_execution_strategy = match s {
@@ -468,12 +473,15 @@ fn import_blocks<F, E>(matches: &clap::ArgMatches, spec: ChainSpec<FactoryGenesi
 	Ok(service::chain_ops::import_blocks::<F, _, _>(config, exit, file)?)
 }
 
-fn revert_chain<F>(matches: &clap::ArgMatches, spec: ChainSpec<FactoryGenesis<F>>) -> error::Result<()>
+fn revert_chain<F>(
+	db_path: PathBuf,
+	matches: &clap::ArgMatches,
+	spec: ChainSpec<FactoryGenesis<F>>
+) -> error::Result<()>
 	where F: ServiceFactory,
 {
-	let base_path = base_path(matches);
 	let mut config = service::Configuration::default_with_spec(spec);
-	config.database_path = db_path(&base_path, config.chain_spec.id()).to_string_lossy().into();
+	config.database_path = db_path.to_string_lossy().into();
 
 	let blocks = match matches.value_of("num") {
 		Some(v) => v.parse().map_err(|_| "Invalid block count specified")?,
@@ -483,13 +491,12 @@ fn revert_chain<F>(matches: &clap::ArgMatches, spec: ChainSpec<FactoryGenesis<F>
 	Ok(service::chain_ops::revert_chain::<F>(config, As::sa(blocks))?)
 }
 
-fn purge_chain<F>(matches: &clap::ArgMatches, spec: ChainSpec<FactoryGenesis<F>>) -> error::Result<()>
+fn purge_chain<F>(
+	db_path: PathBuf,
+) -> error::Result<()>
 	where F: ServiceFactory,
 {
-	let base_path = base_path(matches);
-	let database_path = db_path(&base_path, spec.id());
-
-	print!("Are you sure to remove {:?}? (y/n)", &database_path);
+	print!("Are you sure to remove {:?}? (y/n)", &db_path);
 	stdout().flush().expect("failed to flush stdout");
 
 	let mut input = String::new();
@@ -498,8 +505,8 @@ fn purge_chain<F>(matches: &clap::ArgMatches, spec: ChainSpec<FactoryGenesis<F>>
 
 	match input.chars().nth(0) {
 		Some('y') | Some('Y') => {
-			fs::remove_dir_all(&database_path)?;
-			println!("{:?} removed.", &database_path);
+			fs::remove_dir_all(&db_path)?;
+			println!("{:?} removed.", &db_path);
 		},
 		_ => println!("Aborted"),
 	}

substrate/core/cli/src/params.rs

@@ -40,7 +40,7 @@ pub struct CoreParams {
     /// Specify node secret key (64-character hex string)
     #[structopt(long = "node-key", value_name = "KEY")]
     node_key: Option<String>,
-    
+
     /// Enable validator mode
     #[structopt(long = "validator")]
     validator: bool,
@@ -48,71 +48,71 @@ pub struct CoreParams {
     /// Run in light client mode
     #[structopt(long = "light")]
     light: bool,
-  
+
     /// Run in development mode; implies --chain=dev --validator --key Alice
     #[structopt(long = "dev")]
     dev: bool,
-  
+
     /// Listen on this multiaddress
     #[structopt(long = "listen-addr", value_name = "LISTEN_ADDR")]
     listen_addr: Vec<String>,
-  
+
     /// Specify p2p protocol TCP port. Only used if --listen-addr is not specified.
     #[structopt(long = "port", value_name = "PORT")]
     port: Option<u32>,
-  
+
     /// Listen to all RPC interfaces (default is local)
     #[structopt(long = "rpc-external")]
     rpc_external: bool,
-  
+
     /// Listen to all Websocket interfaces (default is local)
     #[structopt(long = "ws-external")]
     ws_external: bool,
-  
+
     /// Specify HTTP RPC server TCP port
     #[structopt(long = "rpc-port", value_name = "PORT")]
     rpc_port: Option<u32>,
-  
+
     /// Specify WebSockets RPC server TCP port
     #[structopt(long = "ws-port", value_name = "PORT")]
     ws_port: Option<u32>,
-  
+
     /// Specify a list of bootnodes
     #[structopt(long = "bootnodes", value_name = "URL")]
     bootnodes: Vec<String>,
-  
+
     /// Specify a list of reserved node addresses
     #[structopt(long = "reserved-nodes", value_name = "URL")]
     reserved_nodes: Vec<String>,
-  
+
     /// Specify the number of outgoing connections we're trying to maintain
     #[structopt(long = "out-peers", value_name = "OUT_PEERS")]
     out_peers: Option<u8>,
-  
+
     /// Specify the maximum number of incoming connections we're accepting
     #[structopt(long = "in-peers", value_name = "IN_PEERS")]
     in_peers: Option<u8>,
-  
+
     /// Specify the chain specification (one of dev, local or staging)
     #[structopt(long = "chain", value_name = "CHAIN_SPEC")]
     chain: Option<String>,
-  
+
     /// Specify the pruning mode, a number of blocks to keep or 'archive'. Default is 256.
     #[structopt(long = "pruning", value_name = "PRUNING_MODE")]
     pruning: Option<u32>,
-  
+
     /// The human-readable name for this node, as reported to the telemetry server, if enabled
     #[structopt(long = "name", value_name = "NAME")]
     name: Option<String>,
-  
+
     /// Should connect to the Substrate telemetry server (telemetry is off by default on local chains)
     #[structopt(short = "t", long = "telemetry")]
     telemetry: bool,
-  
+
     /// Should not connect to the Substrate telemetry server (telemetry is on by default on global chains)
     #[structopt(long = "no-telemetry")]
     no_telemetry: bool,
-  
+
     /// The URL of the telemetry server. Implies --telemetry
     #[structopt(long = "telemetry-url", value_name = "TELEMETRY_URL")]
     telemetry_url: Option<String>,
@@ -164,14 +164,6 @@ pub enum CoreCommands {
         /// Force raw genesis storage output.
         #[structopt(long = "raw")]
         raw: bool,
-        
-        /// Specify the chain specification (one of dev, local or staging)
-        #[structopt(long = "chain", value_name = "CHAIN_SPEC")]
-        chain: Option<String>,
-        
-        /// Specify the development chain
-        #[structopt(long = "dev")]
-        dev: bool,
     },
 
     /// Export blocks to a file
@@ -180,27 +172,15 @@ pub enum CoreCommands {
         /// Output file name or stdout if unspecified.
         #[structopt(parse(from_os_str))]
         output: Option<PathBuf>,
-        
-        /// Specify the chain specification.
-        #[structopt(long = "chain", value_name = "CHAIN_SPEC")]
-        chain: Option<String>,
-        
-        /// Specify the development chain
-        #[structopt(long = "dev")]
-        dev: bool,
-        
-        /// Specify custom base path.
-        #[structopt(long = "base-path", short = "d", value_name = "PATH")]
-        base_path: Option<String>,
-        
+
         /// Specify starting block number. 1 by default.
         #[structopt(long = "from", value_name = "BLOCK")]
         from: Option<u128>,
-        
+
         /// Specify last block number. Best block by default.
         #[structopt(long = "to", value_name = "BLOCK")]
         to: Option<u128>,
-        
+
         /// Use JSON output rather than binary.
         #[structopt(long = "json")]
         json: bool,
@@ -212,27 +192,15 @@ pub enum CoreCommands {
         /// Input file or stdin if unspecified.
         #[structopt(parse(from_os_str))]
         input: Option<PathBuf>,
-        
-        /// Specify the chain specification.
-        #[structopt(long = "chain", value_name = "CHAIN_SPEC")]
-        chain: Option<String>,
-          
-        /// Specify the development chain
-        #[structopt(long = "dev")]
-        dev: bool,
-        
-        /// Specify custom base path.
-        #[structopt(long = "base-path", short = "d", value_name = "PATH", parse(from_os_str))]
-        base_path: Option<PathBuf>,
-        
+
         /// The means of execution used when executing blocks. Can be either wasm, native or both.
         #[structopt(long = "execution", value_name = "STRATEGY")]
         execution: ExecutionStrategy,
-        
+
         /// The means of execution used when calling into the runtime. Can be either wasm, native or both.
         #[structopt(long = "api-execution", value_name = "STRATEGY")]
         api_execution: ExecutionStrategy,
-        
+
         /// The maximum number of 64KB pages to ever allocate for Wasm execution. Don't alter this unless you know what you're doing.
         #[structopt(long = "max-heap-pages", value_name = "COUNT")]
         max_heap_pages: Option<u32>,
@@ -243,33 +211,9 @@ pub enum CoreCommands {
     Revert {
         /// Number of blocks to revert. Default is 256.
         num: Option<u32>,
-        
-        /// Specify the chain specification.
-        #[structopt(long = "chain", value_name = "CHAIN_SPEC")]
-        chain: Option<String>,
-        
-        /// Specify the development chain
-        #[structopt(long = "dev")]
-        dev: bool,
-        
-        /// Specify custom base path.
-        #[structopt(long = "base-path", short = "d", value_name = "PATH", parse(from_os_str))]
-        base_path: Option<PathBuf>,
     },
 
     /// Remove the whole chain data.
     #[structopt(name = "purge-chain")]
-    PurgeChain {
-        /// Specify the chain specification.
-        #[structopt(long = "chain", value_name = "CHAIN_SPEC")]
-        chain: Option<String>,
-        
-        /// Specify the development chain
-        #[structopt(long = "dev")]
-        dev: bool,
-        
-        /// Specify custom base path.
-        #[structopt(long = "base-path", short = "d", value_name = "PATH", parse(from_os_str))]
-        base_path: Option<PathBuf>
-    }
+    PurgeChain {},
 }

substrate/node/cli/src/lib.rs

@@ -122,7 +122,9 @@ pub fn run<I, T, E>(args: I, exit: E, version: cli::VersionInfo) -> error::Resul
 			Err(e) => e.exit(),
 		};
 
-	let (spec, mut config) = cli::parse_matches::<service::Factory, _>(load_spec, version, "substrate-node", &matches)?;
+	let (spec, mut config) = cli::parse_matches::<service::Factory, _>(
+		load_spec, version, "substrate-node", &matches
+	)?;
 
 	if matches.is_present("grandpa_authority_only") {
 		config.custom.grandpa_authority = true;