From e8dd1205eebcbb499c5d20214037a1638b2710fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <bkchr@users.noreply.github.com>
Date: Sat, 11 Jan 2020 18:47:26 +0100
Subject: [PATCH] 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 | 2 +-
substrate/client/cli/src/lib.rs | 98 ++++++--
substrate/client/service/src/builder.rs | 10 +-
substrate/client/service/src/config.rs | 23 +-
substrate/client/service/test/src/lib.rs | 2 +-
substrate/primitives/runtime/src/lib.rs | 10 +
substrate/utils/browser/src/lib.rs | 236 +++++++++---------
7 files changed, 225 insertions(+), 156 deletions(-)
diff --git a/substrate/client/chain-spec/src/chain_spec.rs b/substrate/client/chain-spec/src/chain_spec.rs
index ac6070fdd68..6e58083e8cc 100644
--- a/substrate/client/chain-spec/src/chain_spec.rs
+++ b/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,
diff --git a/substrate/client/cli/src/lib.rs b/substrate/client/cli/src/lib.rs
index 18ba347f6ec..620a90c514d 100644
--- a/substrate/client/cli/src/lib.rs
+++ b/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());
+ }
+ }
}
diff --git a/substrate/client/service/src/builder.rs b/substrate/client/service/src/builder.rs
index 3a3702bc194..6eb0259b792 100644
--- a/substrate/client/service/src/builder.rs
+++ b/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(
diff --git a/substrate/client/service/src/config.rs b/substrate/client/service/src/config.rs
index 94872129dff..75c62682147 100644
--- a/substrate/client/service/src/config.rs
+++ b/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(),
diff --git a/substrate/client/service/test/src/lib.rs b/substrate/client/service/test/src/lib.rs
index d76064300c5..06a1edd189a 100644
--- a/substrate/client/service/test/src/lib.rs
+++ b/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()),
diff --git a/substrate/primitives/runtime/src/lib.rs b/substrate/primitives/runtime/src/lib.rs
index 198f55869e9..c010ea0456d 100644
--- a/substrate/primitives/runtime/src/lib.rs
+++ b/substrate/primitives/runtime/src/lib.rs
@@ -165,6 +165,16 @@ impl BuildStorage for sp_core::storage::Storage {
}
}
+#[cfg(feature = "std")]
+impl BuildStorage for () {
+ fn assimilate_storage(
+ &self,
+ _: &mut sp_core::storage::Storage,
+ )-> Result<(), String> {
+ Err("`assimilate_storage` not implemented for `()`".into())
+ }
+}
+
/// Consensus engine unique ID.
pub type ConsensusEngineId = [u8; 4];
diff --git a/substrate/utils/browser/src/lib.rs b/substrate/utils/browser/src/lib.rs
index fd4ad9f69e1..e404c661290 100644
--- a/substrate/utils/browser/src/lib.rs
+++ b/substrate/utils/browser/src/lib.rs
@@ -1,4 +1,4 @@
-// Copyright 2019 Parity Technologies (UK) Ltd.
+// Copyright 2019-2020 Parity Technologies (UK) Ltd.
// This file is part of Substrate.
// Substrate is free software: you can redistribute it and/or modify
@@ -18,13 +18,13 @@ use futures01::sync::mpsc as mpsc01;
use log::{debug, info};
use std::sync::Arc;
use service::{
- AbstractService, RpcSession, Roles, Configuration, config::{DatabaseConfig, KeystoreConfig},
- ChainSpec, RuntimeGenesis
+ AbstractService, RpcSession, Roles, Configuration, config::{DatabaseConfig, KeystoreConfig},
+ ChainSpec, RuntimeGenesis
};
use wasm_bindgen::prelude::*;
use futures::{
- TryFutureExt as _, FutureExt as _, Stream as _, Future as _, TryStreamExt as _,
- channel::{oneshot, mpsc}, future::{poll_fn, ok}, compat::*,
+ TryFutureExt as _, FutureExt as _, Stream as _, Future as _, TryStreamExt as _,
+ channel::{oneshot, mpsc}, future::{poll_fn, ok}, compat::*,
};
use std::task::Poll;
use std::pin::Pin;
@@ -38,136 +38,136 @@ pub use console_log::init_with_level as init_console_log;
///
/// This configuration contains good defaults for a browser light client.
pub async fn browser_configuration<C, G, E>(
- transport: Transport,
- chain_spec: ChainSpec<G, E>,
+ transport: Transport,
+ chain_spec: ChainSpec<G, E>,
) -> Result<Configuration<C, G, E>, Box<dyn std::error::Error>>
where
- C: Default,
- G: RuntimeGenesis,
- E: Extension,
+ C: Default,
+ G: RuntimeGenesis,
+ E: Extension,
{
- let name = chain_spec.name().to_string();
-
- let transport = ExtTransport::new(transport);
- let mut config = Configuration::default_with_spec_and_base_path(chain_spec, None);
- config.network.transport = network::config::TransportConfig::Normal {
- wasm_external_transport: Some(transport.clone()),
- allow_private_ipv4: true,
- enable_mdns: false,
- };
- config.telemetry_external_transport = Some(transport);
- config.roles = Roles::LIGHT;
- config.name = format!("{} (Browser)", name);
- config.database = {
- info!("Opening Indexed DB database '{}'...", name);
- let db = kvdb_web::Database::open(name, 10)
- .await?;
- DatabaseConfig::Custom(Arc::new(db))
- };
- config.keystore = KeystoreConfig::InMemory;
-
- Ok(config)
+ let name = chain_spec.name().to_string();
+
+ let transport = ExtTransport::new(transport);
+ let mut config = Configuration::default_with_spec_and_base_path(chain_spec, None);
+ config.network.transport = network::config::TransportConfig::Normal {
+ wasm_external_transport: Some(transport.clone()),
+ allow_private_ipv4: true,
+ enable_mdns: false,
+ };
+ config.telemetry_external_transport = Some(transport);
+ config.roles = Roles::LIGHT;
+ config.name = format!("{} (Browser)", name);
+ config.database = {
+ info!("Opening Indexed DB database '{}'...", name);
+ let db = kvdb_web::Database::open(name, 10)
+ .await?;
+ DatabaseConfig::Custom(Arc::new(db))
+ };
+ config.keystore = KeystoreConfig::InMemory;
+
+ Ok(config)
}
/// A running client.
#[wasm_bindgen]
pub struct Client {
- rpc_send_tx: mpsc::UnboundedSender<RpcMessage>,
+ rpc_send_tx: mpsc::UnboundedSender<RpcMessage>,
}
struct RpcMessage {
- rpc_json: String,
- session: RpcSession,
- send_back: oneshot::Sender<Pin<Box<dyn futures::Future<Output = Option<String>> + Send>>>,
+ rpc_json: String,
+ session: RpcSession,
+ send_back: oneshot::Sender<Pin<Box<dyn futures::Future<Output = Option<String>> + Send>>>,
}
/// Create a Client object that connects to a service.
pub fn start_client(service: impl AbstractService) -> Client {
- let mut service = service.compat();
- // We dispatch a background task responsible for processing the service.
- //
- // The main action performed by the code below consists in polling the service with
- // `service.poll()`.
- // The rest consists in handling RPC requests.
- let (rpc_send_tx, mut rpc_send_rx) = mpsc::unbounded::<RpcMessage>();
- wasm_bindgen_futures::spawn_local(poll_fn(move |cx| {
- loop {
- match Pin::new(&mut rpc_send_rx).poll_next(cx) {
- Poll::Ready(Some(message)) => {
- let fut = service.get_ref()
- .rpc_query(&message.session, &message.rpc_json)
- .compat()
- .unwrap_or_else(|_| None)
- .boxed();
- let _ = message.send_back.send(fut);
- },
- Poll::Pending => break,
- Poll::Ready(None) => return Poll::Ready(()),
- }
- }
-
- Pin::new(&mut service)
- .poll(cx)
- .map(drop)
- }));
-
- Client {
- rpc_send_tx,
- }
+ let mut service = service.compat();
+ // We dispatch a background task responsible for processing the service.
+ //
+ // The main action performed by the code below consists in polling the service with
+ // `service.poll()`.
+ // The rest consists in handling RPC requests.
+ let (rpc_send_tx, mut rpc_send_rx) = mpsc::unbounded::<RpcMessage>();
+ wasm_bindgen_futures::spawn_local(poll_fn(move |cx| {
+ loop {
+ match Pin::new(&mut rpc_send_rx).poll_next(cx) {
+ Poll::Ready(Some(message)) => {
+ let fut = service.get_ref()
+ .rpc_query(&message.session, &message.rpc_json)
+ .compat()
+ .unwrap_or_else(|_| None)
+ .boxed();
+ let _ = message.send_back.send(fut);
+ },
+ Poll::Pending => break,
+ Poll::Ready(None) => return Poll::Ready(()),
+ }
+ }
+
+ Pin::new(&mut service)
+ .poll(cx)
+ .map(drop)
+ }));
+
+ Client {
+ rpc_send_tx,
+ }
}
#[wasm_bindgen]
impl Client {
- /// Allows starting an RPC request. Returns a `Promise` containing the result of that request.
- #[wasm_bindgen(js_name = "rpcSend")]
- pub fn rpc_send(&mut self, rpc: &str) -> js_sys::Promise {
- let rpc_session = RpcSession::new(mpsc01::channel(1).0);
- let (tx, rx) = oneshot::channel();
- let _ = self.rpc_send_tx.unbounded_send(RpcMessage {
- rpc_json: rpc.to_owned(),
- session: rpc_session,
- send_back: tx,
- });
- wasm_bindgen_futures::future_to_promise(async {
- match rx.await {
- Ok(fut) => {
- fut.await
- .map(|s| JsValue::from_str(&s))
- .ok_or_else(|| JsValue::NULL)
- },
- Err(_) => Err(JsValue::NULL)
- }
- })
- }
-
- /// Subscribes to an RPC pubsub endpoint.
- #[wasm_bindgen(js_name = "rpcSubscribe")]
- pub fn rpc_subscribe(&mut self, rpc: &str, callback: js_sys::Function) {
- let (tx, rx) = mpsc01::channel(4);
- let rpc_session = RpcSession::new(tx);
- let (fut_tx, fut_rx) = oneshot::channel();
- let _ = self.rpc_send_tx.unbounded_send(RpcMessage {
- rpc_json: rpc.to_owned(),
- session: rpc_session.clone(),
- send_back: fut_tx,
- });
- wasm_bindgen_futures::spawn_local(async {
- if let Ok(fut) = fut_rx.await {
- fut.await;
- }
- });
-
- wasm_bindgen_futures::spawn_local(async move {
- let _ = rx.compat()
- .try_for_each(|s| {
- let _ = callback.call1(&callback, &JsValue::from_str(&s));
- ok(())
- })
- .await;
-
- // We need to keep `rpc_session` alive.
- debug!("RPC subscription has ended");
- drop(rpc_session);
- });
- }
+ /// Allows starting an RPC request. Returns a `Promise` containing the result of that request.
+ #[wasm_bindgen(js_name = "rpcSend")]
+ pub fn rpc_send(&mut self, rpc: &str) -> js_sys::Promise {
+ let rpc_session = RpcSession::new(mpsc01::channel(1).0);
+ let (tx, rx) = oneshot::channel();
+ let _ = self.rpc_send_tx.unbounded_send(RpcMessage {
+ rpc_json: rpc.to_owned(),
+ session: rpc_session,
+ send_back: tx,
+ });
+ wasm_bindgen_futures::future_to_promise(async {
+ match rx.await {
+ Ok(fut) => {
+ fut.await
+ .map(|s| JsValue::from_str(&s))
+ .ok_or_else(|| JsValue::NULL)
+ },
+ Err(_) => Err(JsValue::NULL)
+ }
+ })
+ }
+
+ /// Subscribes to an RPC pubsub endpoint.
+ #[wasm_bindgen(js_name = "rpcSubscribe")]
+ pub fn rpc_subscribe(&mut self, rpc: &str, callback: js_sys::Function) {
+ let (tx, rx) = mpsc01::channel(4);
+ let rpc_session = RpcSession::new(tx);
+ let (fut_tx, fut_rx) = oneshot::channel();
+ let _ = self.rpc_send_tx.unbounded_send(RpcMessage {
+ rpc_json: rpc.to_owned(),
+ session: rpc_session.clone(),
+ send_back: fut_tx,
+ });
+ wasm_bindgen_futures::spawn_local(async {
+ if let Ok(fut) = fut_rx.await {
+ fut.await;
+ }
+ });
+
+ wasm_bindgen_futures::spawn_local(async move {
+ let _ = rx.compat()
+ .try_for_each(|s| {
+ let _ = callback.call1(&callback, &JsValue::from_str(&s));
+ ok(())
+ })
+ .await;
+
+ // We need to keep `rpc_session` alive.
+ debug!("RPC subscription has ended");
+ drop(rpc_session);
+ });
+ }
}
--
GitLab