Add test for cli keystore path generation (#4571)

* Add test for cli keystore path generation

* Fix test

substrate/client/chain-spec/src/chain_spec.rs

@@ -228,7 +228,7 @@ impl<G, E> ChainSpec<G, E> {
 		let client_spec = ClientSpec {
 			name: name.to_owned(),
 			id: id.to_owned(),
-			boot_nodes: boot_nodes,
+			boot_nodes,
 			telemetry_endpoints,
 			protocol_id: protocol_id.map(str::to_owned),
 			properties,

substrate/client/cli/src/lib.rs

@@ -351,7 +351,10 @@ impl<'a> ParseAndPrepareBuildSpec<'a> {
 
 		if spec.boot_nodes().is_empty() && !self.params.disable_default_bootnode {
 			let base_path = base_path(&self.params.shared_params, self.version);
-			let cfg = sc_service::Configuration::<C,_,_>::default_with_spec_and_base_path(spec.clone(), Some(base_path));
+			let cfg = sc_service::Configuration::<C,_,_>::default_with_spec_and_base_path(
+				spec.clone(),
+				Some(base_path),
+			);
 			let node_key = node_key_config(
 				self.params.node_key_params,
 				&Some(cfg.in_chain_config_dir(DEFAULT_NETWORK_CONFIG_PATH).expect("We provided a base_path"))
@@ -750,26 +753,33 @@ fn input_keystore_password() -> Result<String, String> {
 }
 
 /// Fill the password field of the given config instance.
-fn fill_config_keystore_password<C, G, E>(
+fn fill_config_keystore_password_and_path<C, G, E>(
 	config: &mut sc_service::Configuration<C, G, E>,
 	cli: &RunCmd,
 ) -> Result<(), String> {
-	if let KeystoreConfig::Path { password, .. } = &mut config.keystore {
-		*password = if cli.password_interactive {
-			#[cfg(not(target_os = "unknown"))]
-			{
-				Some(input_keystore_password()?.into())
-			}
-			#[cfg(target_os = "unknown")]
-			None
-		} else if let Some(ref file) = cli.password_filename {
-			Some(fs::read_to_string(file).map_err(|e| format!("{}", e))?.into())
-		} else if let Some(ref password) = cli.password {
-			Some(password.clone().into())
-		} else {
-			None
-		};
-	}
+	let password = if cli.password_interactive {
+		#[cfg(not(target_os = "unknown"))]
+		{
+			Some(input_keystore_password()?.into())
+		}
+		#[cfg(target_os = "unknown")]
+		None
+	} else if let Some(ref file) = cli.password_filename {
+		Some(fs::read_to_string(file).map_err(|e| format!("{}", e))?.into())
+	} else if let Some(ref password) = cli.password {
+		Some(password.clone().into())
+	} else {
+		None
+	};
+
+	let path = cli.keystore_path.clone().or(
+		config.in_chain_config_dir(DEFAULT_KEYSTORE_CONFIG_PATH)
+	);
+
+	config.keystore = KeystoreConfig::Path {
+		path: path.ok_or_else(|| "No `base_path` provided to create keystore path!")?,
+		password,
+	};
 
 	Ok(())
 }
@@ -840,7 +850,7 @@ where
 {
 	let mut config = create_config_with_db_path(spec_factory, &cli.shared_params, &version)?;
 
-	fill_config_keystore_password(&mut config, &cli)?;
+	fill_config_keystore_password_and_path(&mut config, &cli)?;
 
 	let is_dev = cli.shared_params.dev;
 	let is_authority = cli.validator || cli.sentry || is_dev || cli.keyring.account.is_some();
@@ -875,12 +885,6 @@ where
 		)?
 	}
 
-	let default_keystore_path = config.in_chain_config_dir(DEFAULT_KEYSTORE_CONFIG_PATH);
-
-	if let KeystoreConfig::Path { path, ..} = &mut config.keystore {
-		*path = cli.keystore_path.or(default_keystore_path);
-	}
-
 	// set sentry mode (i.e. act as an authority but **never** actively participate)
 	config.sentry_mode = cli.sentry;
 
@@ -1205,4 +1209,48 @@ mod tests {
 		assert!(no_config_dir().is_ok());
 		assert!(some_config_dir("x".to_string()).is_ok());
 	}
+
+	#[test]
+	fn keystore_path_is_generated_correctly() {
+		let chain_spec = ChainSpec::from_genesis(
+			"test",
+			"test-id",
+			|| (),
+			Vec::new(),
+			None,
+			None,
+			None,
+			None,
+		);
+
+		let version_info = VersionInfo {
+			name: "test",
+			version: "42",
+			commit: "234234",
+			executable_name: "test",
+			description: "cool test",
+			author: "universe",
+			support_url: "com",
+		};
+
+		for keystore_path in vec![None, Some("/keystore/path")] {
+			let mut run_cmds = RunCmd::from_args();
+			run_cmds.shared_params.base_path = Some(PathBuf::from("/test/path"));
+			run_cmds.keystore_path = keystore_path.clone().map(PathBuf::from);
+
+			let node_config = create_run_node_config::<(), _, _, _>(
+				run_cmds.clone(),
+				|_| Ok(Some(chain_spec.clone())),
+				"test",
+				&version_info,
+			).unwrap();
+
+			let expected_path = match keystore_path {
+				Some(path) => PathBuf::from(path),
+				None => PathBuf::from("/test/path/chains/test-id/keystore"),
+			};
+
+			assert_eq!(expected_path, node_config.keystore.path().unwrap().to_owned());
+		}
+	}
 }

substrate/client/service/src/builder.rs

@@ -165,10 +165,11 @@ fn new_full_parts<TBl, TRtApi, TExecDisp, TCfg, TGen, TCSExt>(
 {
 	let keystore = match &config.keystore {
 		KeystoreConfig::Path { path, password } => Keystore::open(
-			path.clone().ok_or("No basepath configured")?,
+			path.clone(),
 			password.clone()
 		)?,
-		KeystoreConfig::InMemory => Keystore::new_in_memory()
+		KeystoreConfig::InMemory => Keystore::new_in_memory(),
+		KeystoreConfig::None => return Err("No keystore config provided!".into()),
 	};
 
 	let executor = NativeExecutor::<TExecDisp>::new(
@@ -289,10 +290,11 @@ where TGen: RuntimeGenesis, TCSExt: Extension {
 	>, Error> {
 		let keystore = match &config.keystore {
 			KeystoreConfig::Path { path, password } => Keystore::open(
-				path.clone().ok_or("No basepath configured")?,
+				path.clone(),
 				password.clone()
 			)?,
-			KeystoreConfig::InMemory => Keystore::new_in_memory()
+			KeystoreConfig::InMemory => Keystore::new_in_memory(),
+			KeystoreConfig::None => return Err("No keystore config provided!".into()),
 		};
 
 		let executor = NativeExecutor::<TExecDisp>::new(

substrate/client/service/src/config.rs

@@ -21,7 +21,7 @@ pub use sc_client_db::{kvdb::KeyValueDB, PruningMode};
 pub use sc_network::config::{ExtTransport, NetworkConfiguration, Roles};
 pub use sc_executor::WasmExecutionMethod;
 
-use std::{path::PathBuf, net::SocketAddr, sync::Arc};
+use std::{path::{PathBuf, Path}, net::SocketAddr, sync::Arc};
 pub use sc_transaction_pool::txpool::Options as TransactionPoolOptions;
 use sc_chain_spec::{ChainSpec, RuntimeGenesis, Extension, NoExtension};
 use sp_core::crypto::Protected;
@@ -107,10 +107,12 @@ pub struct Configuration<C, G, E = NoExtension> {
 /// Configuration of the client keystore.
 #[derive(Clone)]
 pub enum KeystoreConfig {
+	/// No config supplied.
+	None,
 	/// Keystore at a path on-disk. Recommended for native nodes.
 	Path {
-		/// The path of the keystore. Will panic if no path is specified.
-		path: Option<PathBuf>,
+		/// The path of the keystore.
+		path: PathBuf,
 		/// Node keystore's password.
 		password: Option<Protected<String>>
 	},
@@ -118,6 +120,16 @@ pub enum KeystoreConfig {
 	InMemory
 }
 
+impl KeystoreConfig {
+	/// Returns the path for the keystore.
+	pub fn path(&self) -> Option<&Path> {
+		match self {
+			Self::Path { path, .. } => Some(&path),
+			Self::None | Self::InMemory => None,
+		}
+	}
+}
+
 /// Configuration of the database of the client.
 #[derive(Clone)]
 pub enum DatabaseConfig {
@@ -150,10 +162,7 @@ impl<C, G, E> Configuration<C, G, E> where
 			roles: Roles::FULL,
 			transaction_pool: Default::default(),
 			network: Default::default(),
-			keystore: KeystoreConfig::Path {
-				path: config_dir.map(|c| c.join("keystore")),
-				password: None
-			},
+			keystore: KeystoreConfig::None,
 			database: DatabaseConfig::Path {
 				path: Default::default(),
 				cache_size: Default::default(),

substrate/client/service/test/src/lib.rs

@@ -174,7 +174,7 @@ fn node_config<G, E: Clone> (
 		transaction_pool: Default::default(),
 		network: network_config,
 		keystore: KeystoreConfig::Path {
-			path: Some(root.join("key")),
+			path: root.join("key"),
 			password: None
 		},
 		config_dir: Some(root.clone()),