Move config path generation into the service config for reusability (#3978)

* Move config path generation into the service config for reusability

* Make NoCostum Default and fix tests

* Apply suggestions from code review

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

* remove function not used anymore

* Make path into an option

* remove database_path function and call it directly

* remove helper functions, use consts

substrate/core/cli/src/lib.rs

@@ -65,6 +65,13 @@ use lazy_static::lazy_static;
 use futures::Future;
 use substrate_telemetry::TelemetryEndpoints;
 
+/// default sub directory to store network config
+const DEFAULT_NETWORK_CONFIG_PATH : &'static str = "network";
+/// default sub directory to store database
+const DEFAULT_DB_CONFIG_PATH : &'static str = "db";
+/// default sub directory for the key store
+const DEFAULT_KEYSTORE_CONFIG_PATH : &'static str =  "keystore";
+
 /// The maximum number of characters for a node name.
 const NODE_NAME_MAX_LENGTH: usize = 32;
 
@@ -307,18 +314,36 @@ pub struct ParseAndPrepareBuildSpec<'a> {
 
 impl<'a> ParseAndPrepareBuildSpec<'a> {
 	/// Runs the command and build the chain specs.
-	pub fn run<G, S, E>(
+	pub fn run<C, G, S, E>(
 		self,
 		spec_factory: S
 	) -> error::Result<()> where
 		S: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
+		C: Default,
 		G: RuntimeGenesis,
 		E: ChainSpecExtension,
 	{
 		info!("Building chain spec");
 		let raw_output = self.params.raw;
 		let mut spec = load_spec(&self.params.shared_params, spec_factory)?;
-		with_default_boot_node(&mut spec, self.params, self.version)?;
+
+		if spec.boot_nodes().is_empty() && !self.params.disable_default_bootnode {
+			let base_path = base_path(&self.params.shared_params, self.version);
+			let cfg = 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"))
+			)?;
+			let keys = node_key.into_keypair()?;
+			let peer_id = keys.public().into_peer_id();
+			let addr = build_multiaddr![
+				Ip4([127, 0, 0, 1]),
+				Tcp(30333u16),
+				P2p(peer_id)
+			];
+			spec.add_boot_node(addr)
+		}
+		
 		let json = service::chain_ops::build_spec(spec, raw_output)?;
 
 		print!("{}", json);
@@ -558,16 +583,13 @@ fn fill_transaction_pool_configuration<C, G, E>(
 /// Fill the given `NetworkConfiguration` by looking at the cli parameters.
 fn fill_network_configuration(
 	cli: NetworkConfigurationParams,
-	base_path: &Path,
+	config_path: PathBuf,
-	chain_spec_id: &str,
 	config: &mut NetworkConfiguration,
 	client_id: String,
 	is_dev: bool,
 ) -> error::Result<()> {
 	config.boot_nodes.extend(cli.bootnodes.into_iter());
-	config.config_path = Some(
+	config.config_path = Some(config_path.to_string_lossy().into());
-		network_path(&base_path, chain_spec_id).to_string_lossy().into()
-	);
 	config.net_config_path = config.config_path.clone();
 	config.reserved_nodes.extend(cli.reserved_nodes.into_iter());
 
@@ -642,7 +664,8 @@ where
 	S: FnOnce(&str) -> Result<Option<ChainSpec<G, E>>, String>,
 {
 	let spec = load_spec(&cli.shared_params, spec_factory)?;
-	let mut config = service::Configuration::default_with_spec(spec.clone());
+	let base_path = base_path(&cli.shared_params, &version);
+	let mut config = service::Configuration::default_with_spec_and_base_path(spec.clone(), Some(base_path));
 
 	fill_config_keystore_password(&mut config, &cli)?;
 
@@ -666,14 +689,10 @@ where
 		)?
 	}
 
-	let base_path = base_path(&cli.shared_params, version);
+	config.keystore_path = cli.keystore_path.or_else(|| config.in_chain_config_dir(DEFAULT_KEYSTORE_CONFIG_PATH));
-
-	config.keystore_path = cli.keystore_path.unwrap_or_else(
-		|| keystore_path(&base_path, config.chain_spec.id())
-	);
 
 	config.database = DatabaseConfig::Path {
-		path: db_path(&base_path, config.chain_spec.id()),
+		path: config.in_chain_config_dir(DEFAULT_DB_CONFIG_PATH).expect("We provided a base_path."),
 		cache_size: cli.database_cache_size,
 	};
 	config.state_cache_size = cli.state_cache_size;
@@ -740,8 +759,7 @@ where
 	let client_id = config.client_id();
 	fill_network_configuration(
 		cli.network_config,
-		&base_path,
+		config.in_chain_config_dir(DEFAULT_NETWORK_CONFIG_PATH).expect("We provided a basepath"),
-		spec.id(),
 		&mut config.network,
 		client_id,
 		is_dev,
@@ -792,39 +810,6 @@ where
 	Ok(config)
 }
 
-//
-// IANA unassigned port ranges that we could use:
-// 6717-6766		Unassigned
-// 8504-8553		Unassigned
-// 9556-9591		Unassigned
-// 9803-9874		Unassigned
-// 9926-9949		Unassigned
-
-fn with_default_boot_node<G, E>(
-	spec: &mut ChainSpec<G, E>,
-	cli: BuildSpecCmd,
-	version: &VersionInfo,
-) -> error::Result<()>
-where
-	G: RuntimeGenesis,
-	E: ChainSpecExtension,
-{
-	if spec.boot_nodes().is_empty() && !cli.disable_default_bootnode {
-		let base_path = base_path(&cli.shared_params, version);
-		let storage_path = network_path(&base_path, spec.id());
-		let node_key = node_key_config(cli.node_key_params, &Some(storage_path))?;
-		let keys = node_key.into_keypair()?;
-		let peer_id = keys.public().into_peer_id();
-		let addr = build_multiaddr![
-			Ip4([127, 0, 0, 1]),
-			Tcp(30333u16),
-			P2p(peer_id)
-		];
-		spec.add_boot_node(addr)
-	}
-	Ok(())
-}
-
 /// Creates a configuration including the database path.
 pub fn create_config_with_db_path<C, G, E, S>(
 	spec_factory: S, cli: &SharedParams, version: &VersionInfo,
@@ -838,9 +823,9 @@ where
 	let spec = load_spec(cli, spec_factory)?;
 	let base_path = base_path(cli, version);
 
-	let mut config = service::Configuration::default_with_spec(spec.clone());
+	let mut config = service::Configuration::default_with_spec_and_base_path(spec.clone(), Some(base_path));
 	config.database = DatabaseConfig::Path {
-		path: db_path(&base_path, spec.id()),
+		path: config.in_chain_config_dir(DEFAULT_DB_CONFIG_PATH).expect("We provided a base_path."),
 		cache_size: None,
 	};
 
@@ -866,30 +851,6 @@ fn parse_address(
 	Ok(address)
 }
 
-fn keystore_path(base_path: &Path, chain_id: &str) -> PathBuf {
-	let mut path = base_path.to_owned();
-	path.push("chains");
-	path.push(chain_id);
-	path.push("keystore");
-	path
-}
-
-fn db_path(base_path: &Path, chain_id: &str) -> PathBuf {
-	let mut path = base_path.to_owned();
-	path.push("chains");
-	path.push(chain_id);
-	path.push("db");
-	path
-}
-
-fn network_path(base_path: &Path, chain_id: &str) -> PathBuf {
-	let mut path = base_path.to_owned();
-	path.push("chains");
-	path.push(chain_id);
-	path.push("network");
-	path
-}
-
 fn init_logger(pattern: &str) {
 	use ansi_term::Colour;
 

substrate/core/cli/src/params.rs

@@ -854,7 +854,7 @@ impl<CC, RP> GetLogFilter for CoreParams<CC, RP> where CC: GetLogFilter {
 
 /// A special commandline parameter that expands to nothing.
 /// Should be used as custom subcommand/run arguments if no custom values are required.
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Default)]
 pub struct NoCustom {}
 
 impl StructOpt for NoCustom {

substrate/core/service/src/builder.rs

@@ -155,7 +155,10 @@ where TGen: RuntimeGenesis, TCSExt: Extension {
 		(),
 		TFullBackend<TBl>,
 	>, Error> {
-		let keystore = Keystore::open(config.keystore_path.clone(), config.keystore_password.clone())?;
+		let keystore = Keystore::open(
+			config.keystore_path.clone().ok_or("No basepath configured")?,
+			config.keystore_password.clone()
+		)?;
 
 		let executor = NativeExecutor::<TExecDisp>::new(
 			config.wasm_method,
@@ -236,7 +239,10 @@ where TGen: RuntimeGenesis, TCSExt: Extension {
 		(),
 		TLightBackend<TBl>,
 	>, Error> {
-		let keystore = Keystore::open(config.keystore_path.clone(), config.keystore_password.clone())?;
+		let keystore = Keystore::open(
+			config.keystore_path.clone().ok_or("No basepath configured")?,
+			config.keystore_password.clone()
+		)?;
 
 		let executor = NativeExecutor::<TExecDisp>::new(
 			config.wasm_method,

substrate/core/service/src/config.rs

@@ -43,8 +43,10 @@ pub struct Configuration<C, G, E = NoExtension> {
 	pub transaction_pool: transaction_pool::txpool::Options,
 	/// Network configuration.
 	pub network: NetworkConfiguration,
+	/// Path to the base configuration directory.
+	pub config_dir: Option<PathBuf>,
 	/// Path to key files.
-	pub keystore_path: PathBuf,
+	pub keystore_path: Option<PathBuf>,
 	/// Configuration for the database.
 	pub database: DatabaseConfig,
 	/// Size of internal state cache in Bytes
@@ -118,18 +120,19 @@ impl<C, G, E> Configuration<C, G, E> where
 	G: RuntimeGenesis,
 	E: Extension,
 {
-	/// Create default config for given chain spec.
+	/// Create a default config for given chain spec and path to configuration dir
-	pub fn default_with_spec(chain_spec: ChainSpec<G, E>) -> Self {
+	pub fn default_with_spec_and_base_path(chain_spec: ChainSpec<G, E>, config_dir: Option<PathBuf>) -> Self {
 		let mut configuration = Configuration {
 			impl_name: "parity-substrate",
 			impl_version: "0.0.0",
 			impl_commit: "",
 			chain_spec,
+			config_dir: config_dir.clone(),
 			name: Default::default(),
 			roles: Roles::FULL,
 			transaction_pool: Default::default(),
 			network: Default::default(),
-			keystore_path: Default::default(),
+			keystore_path: config_dir.map(|c| c.join("keystore")),
 			database: DatabaseConfig::Path {
 				path: Default::default(),
 				cache_size: Default::default(),
@@ -161,6 +164,9 @@ impl<C, G, E> Configuration<C, G, E> where
 		configuration
 	}
 
+}
+
+impl<C, G, E> Configuration<C, G, E> {
 	/// Returns full version string of this configuration.
 	pub fn full_version(&self) -> String {
 		full_version_from_strs(self.impl_version, self.impl_commit)
@@ -170,6 +176,17 @@ impl<C, G, E> Configuration<C, G, E> where
 	pub fn client_id(&self) -> String {
 		format!("{}/v{}", self.impl_name, self.full_version())
 	}
+
+	/// Generate a PathBuf to sub in the chain configuration directory
+	/// if given
+	pub fn in_chain_config_dir(&self, sub: &str) -> Option<PathBuf> {
+		self.config_dir.clone().map(|mut path| {
+			path.push("chains");
+			path.push(self.chain_spec.id());
+			path.push(sub);
+			path
+		})
+	}
 }
 
 /// Returns platform info

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

@@ -169,8 +169,9 @@ fn node_config<G, E: Clone> (
 		roles: role,
 		transaction_pool: Default::default(),
 		network: network_config,
-		keystore_path: root.join("key"),
+		keystore_path: Some(root.join("key")),
 		keystore_password: None,
+		config_dir: Some(root.clone()),
 		database: DatabaseConfig::Path {
 			path: root.join("db"),
 			cache_size: None

substrate/node-template/src/cli.rs

@@ -39,7 +39,7 @@ pub fn run<I, T, E>(args: I, exit: E, version: VersionInfo) -> error::Result<()>
 				),
 			}
 		}),
-		ParseAndPrepare::BuildSpec(cmd) => cmd.run(load_spec),
+		ParseAndPrepare::BuildSpec(cmd) => cmd.run::<NoCustom, _, _, _>(load_spec),
 		ParseAndPrepare::ExportBlocks(cmd) => cmd.run_with_builder(|config: Config<_>|
 			Ok(new_full_start!(config).0), load_spec, exit),
 		ParseAndPrepare::ImportBlocks(cmd) => cmd.run_with_builder(|config: Config<_>|

substrate/node/cli/src/browser.rs

@@ -39,7 +39,7 @@ fn start_inner(wasm_ext: wasm_ext::ffi::Transport) -> Result<Client, Box<dyn std
 	let config = {
 		let wasm_ext = wasm_ext::ExtTransport::new(wasm_ext);
 		let chain_spec = ChainSpec::FlamingFir.load().map_err(|e| format!("{:?}", e))?;
-		let mut config = Configuration::<(), _, _>::default_with_spec(chain_spec);
+		let mut config = Configuration::<(), _, _>::default_with_spec_and_base_path(chain_spec, None);
 		config.network.transport = network::config::TransportConfig::Normal {
 			wasm_external_transport: Some(wasm_ext.clone()),
 			enable_mdns: false,

substrate/node/cli/src/cli.rs

@@ -131,7 +131,7 @@ pub fn run<I, T, E>(args: I, exit: E, version: substrate_cli::VersionInfo) -> er
 				),
 			}
 		}),
-		ParseAndPrepare::BuildSpec(cmd) => cmd.run(load_spec),
+		ParseAndPrepare::BuildSpec(cmd) => cmd.run::<NoCustom, _, _, _>(load_spec),
 		ParseAndPrepare::ExportBlocks(cmd) => cmd.run_with_builder(|config: Config<_, _>|
 			Ok(new_full_start!(config).0), load_spec, exit),
 		ParseAndPrepare::ImportBlocks(cmd) => cmd.run_with_builder(|config: Config<_, _>|