Repository: chainx-org/ChainX (language: rust)

Issue:
Impl DefaultConfigurationValues for Cli to override the default port numbers defined in Substrate
Currently, we use a workaround by forcibly injecting the ChainX port numbers into the command line arguments (https://github.com/chainx-org/ChainX/blob/8b3dce1fac/cli/src/config.rs#L113-L119) for overriding the default port numbers in substrate, which is ugly and can be directly resolved by implementing the trait `DefaultConfigurationValues` in chainx `Cli`, example: https://github.com/paritytech/statemint/blob/1ea95abe6d/node/src/command.rs#L324-L340.


Discussion / hints:

Fix patch:
diff --git a/cli/src/command.rs b/cli/src/command.rs
index c0a5d642d..5fcb4bc57 100644
--- a/cli/src/command.rs
+++ b/cli/src/command.rs
@@ -1,13 +1,127 @@
 // Copyright 2019-2020 ChainX Project Authors. Licensed under GPL-3.0.
 
-use sc_cli::{ChainSpec, Role, RuntimeVersion, SubstrateCli};
-use sc_service::PartialComponents;
+use std::net::SocketAddr;
+
+use sc_cli::{
+    ChainSpec, CliConfiguration, DefaultConfigurationValues, ImportParams, KeystoreParams,
+    NetworkParams, Role, RuntimeVersion, SharedParams, SubstrateCli,
+};
+use sc_service::{config::PrometheusConfig, PartialComponents};
 
 use chainx_service::{self as service, new_partial};
 
 use crate::chain_spec;
 use crate::cli::{Cli, Subcommand};
 
+impl DefaultConfigurationValues for Cli {
+    fn p2p_listen_port() -> u16 {
+        20222
+    }
+
+    fn rpc_ws_listen_port() -> u16 {
+        8087
+    }
+
+    fn rpc_http_listen_port() -> u16 {
+        8086
+    }
+
+    fn prometheus_listen_port() -> u16 {
+        9615
+    }
+}
+
+impl CliConfiguration<Self> for Cli {
+    fn shared_params(&self) -> &SharedParams {
+        self.run.base.shared_params()
+    }
+
+    fn import_params(&self) -> Option<&ImportParams> {
+        self.run.base.import_params()
+    }
+
+    fn keystore_params(&self) -> Option<&KeystoreParams> {
+        self.run.base.keystore_params()
+    }
+
+    fn network_params(&self) -> Option<&NetworkParams> {
+        self.run.base.network_params()
+    }
+
+    fn role(&self, is_dev: bool) -> sc_cli::Result<sc_service::Role> {
+        self.run.base.role(is_dev)
+    }
+
+    fn transaction_pool(&self) -> sc_cli::Result<sc_service::config::TransactionPoolOptions> {
+        self.run.base.transaction_pool()
+    }
+
+    fn state_cache_child_ratio(&self) -> sc_cli::Result<Option<usize>> {
+        self.run.base.state_cache_child_ratio()
+    }
+
+    fn rpc_http(&self, default_listen_port: u16) -> sc_cli::Result<Option<SocketAddr>> {
+        self.run.base.rpc_http(default_listen_port)
+    }
+
+    fn rpc_ipc(&self) -> sc_cli::Result<Option<String>> {
+        self.run.base.rpc_ipc()
+    }
+
+    fn rpc_ws(&self, default_listen_port: u16) -> sc_cli::Result<Option<SocketAddr>> {
+        self.run.base.rpc_ws(default_listen_port)
+    }
+
+    fn rpc_methods(&self) -> sc_cli::Result<sc_service::config::RpcMethods> {
+        self.run.base.rpc_methods()
+    }
+
+    fn rpc_ws_max_connections(&self) -> sc_cli::Result<Option<usize>> {
+        self.run.base.rpc_ws_max_connections()
+    }
+
+    fn rpc_cors(&self, is_dev: bool) -> sc_cli::Result<Option<Vec<String>>> {
+        self.run.base.rpc_cors(is_dev)
+    }
+
+    fn prometheus_config(
+        &self,
+        default_listen_port: u16,
+    ) -> sc_cli::Result<Option<PrometheusConfig>> {
+        self.run.base.prometheus_config(default_listen_port)
+    }
+
+    fn telemetry_external_transport(
+        &self,
+    ) -> sc_cli::Result<Option<sc_service::config::ExtTransport>> {
+        self.run.base.telemetry_external_transport()
+    }
+
+    fn default_heap_pages(&self) -> sc_cli::Result<Option<u64>> {
+        self.run.base.default_heap_pages()
+    }
+
+    fn force_authoring(&self) -> sc_cli::Result<bool> {
+        self.run.base.force_authoring()
+    }
+
+    fn disable_grandpa(&self) -> sc_cli::Result<bool> {
+        self.run.base.disable_grandpa()
+    }
+
+    fn max_runtime_instances(&self) -> sc_cli::Result<Option<usize>> {
+        self.run.base.max_runtime_instances()
+    }
+
+    fn announce_block(&self) -> sc_cli::Result<bool> {
+        self.run.base.announce_block()
+    }
+
+    fn init<C: SubstrateCli>(&self) -> sc_cli::Result<()> {
+        unreachable!("ChainX is never initialized; qed");
+    }
+}
+
 impl SubstrateCli for Cli {
     fn impl_name() -> String {
         "ChainX".into()
@@ -96,6 +210,10 @@ pub fn run() -> sc_cli::Result<()> {
             set_default_ss58_version(chain_spec);
 
             runner.run_node_until_exit(|config| async move {
+                let config =
+                    SubstrateCli::create_configuration(&cli, &cli, config.task_executor.clone())
+                        .map_err(|err| format!("chain argument error: {:?}", err))?;
+
                 match config.role {
                     Role::Light => service::build_light(config),
                     _ => service::build_full(config),
diff --git a/cli/src/config.rs b/cli/src/config.rs
index d667e25c6..87cad070f 100644
--- a/cli/src/config.rs
+++ b/cli/src/config.rs
@@ -18,29 +18,12 @@ fn read_config_file(path: &Path) -> Result<HashMap<String, Value>, Box<dyn std::
     }))
 }
 
-const SUB_COMMANDS: [&str; 13] = [
-    "benchmark",
-    "build-spec",
-    "check-block",
-    "export-blocks",
-    "export-state",
-    "help",
-    "import-blocks",
-    "key",
-    "purge-chain",
-    "revert",
-    "sign",
-    "vanity",
-    "verify",
-];
-
 /// Extends the origin cli arg list with the options from the config file.
 ///
 /// Only the options that do not appear in the command line will be appended.
 fn extend_cli_args(
     cli_args: Vec<String>,
     path: Option<&Path>,
-    default_opts: HashMap<String, String>,
 ) -> Result<Vec<String>, Box<dyn std::error::Error>> {
     // Gather all the FLAGS/OPTIONS passed from the command line.
     let cli_opts = cli_args
@@ -50,18 +33,9 @@ fn extend_cli_args(
         .collect::<Vec<_>>();
 
     let mut config_opts = Vec::new();
-    let mut default_opts = default_opts
-        .into_iter()
-        .filter(|(k, _)| !cli_opts.contains(&format!("--{}", k).as_ref()))
-        .collect::<HashMap<_, _>>();
 
     if let Some(path) = path {
         for (key, value) in read_config_file(path)?.into_iter() {
-            // Remove the option that has been configured in the config file.
-            if default_opts.contains_key(key.as_str()) {
-                default_opts.remove(key.as_str());
-            }
-
             let opt = format!("--{}", key);
             match value {
                 Value::Bool(b) => {
@@ -96,28 +70,11 @@ fn extend_cli_args(
         }
     }
 
-    if let Some(sub_command) = cli_args.get(1) {
-        // Injecting `default_opts()` only makes sense in the context of no specified subcommands.
-        if !SUB_COMMANDS.contains(&sub_command.as_str()) {
-            for (key, value) in default_opts {
-                config_opts.push(format!("--{}={}", key, value));
-            }
-        }
-    }
-
     let mut args = cli_args;
     args.extend(config_opts);
     Ok(args)
 }
 
-fn default_opts() -> HashMap<String, String> {
-    let mut map = HashMap::new();
-    map.insert("port".into(), "20222".to_string());
-    map.insert("rpc-port".into(), "8086".to_string());
-    map.insert("ws-port".into(), "8087".to_string());
-    map
-}
-
 /// Try to inject the options from the config file.
 pub fn preprocess_cli_args(cli_args: Vec<String>) -> Vec<String> {
     let mut config_path: Option<String> = None;
@@ -137,7 +94,7 @@ pub fn preprocess_cli_args(cli_args: Vec<String>) -> Vec<String> {
     }
 
     let path: Option<&Path> = config_path.as_ref().map(Path::new);
-    match extend_cli_args(cli_args, path, default_opts()) {
+    match extend_cli_args(cli_args, path) {
         Ok(args) => args,
         Err(e) => panic!(e.to_string()),
     }


Test patch: