From 7bf838f225c12c9d79e3902f703802a8e389b2e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 19 Aug 2020 17:30:56 +0200 Subject: [PATCH] Combine default values used at initialization in a trait (#6857) This moves default values used in the Substrate code base when initializing a service into a common trait. Currently this trait only contains listen ports, but this could be extended in the future. Essentially this will make overriding these values much easier for Cumulus, where we have 2 nodes running in one binary. --- substrate/client/cli/src/commands/mod.rs | 42 +++++++---- substrate/client/cli/src/commands/run_cmd.rs | 17 +++-- substrate/client/cli/src/config.rs | 69 +++++++++++++++---- substrate/client/cli/src/lib.rs | 2 +- .../client/cli/src/params/network_params.rs | 3 +- 5 files changed, 94 insertions(+), 39 deletions(-) diff --git a/substrate/client/cli/src/commands/mod.rs b/substrate/client/cli/src/commands/mod.rs index 04cce66bef..5d4f4fe18d 100644 --- a/substrate/client/cli/src/commands/mod.rs +++ b/substrate/client/cli/src/commands/mod.rs @@ -64,7 +64,6 @@ pub enum Subcommand { ExportState(ExportStateCmd), } -// TODO: move to config.rs? /// Macro that helps implement CliConfiguration on an enum of subcommand automatically /// /// # Example @@ -189,17 +188,24 @@ macro_rules! substrate_cli_subcommands { fn network_config( &self, - chain_spec: &::std::boxed::Box, + chain_spec: &std::boxed::Box, is_dev: bool, - net_config_dir: ::std::path::PathBuf, + net_config_dir: std::path::PathBuf, client_id: &str, node_name: &str, - node_key: ::sc_service::config::NodeKeyConfig, + node_key: sc_service::config::NodeKeyConfig, + default_listen_port: u16, ) -> $crate::Result<::sc_service::config::NetworkConfiguration> { match self { $( $enum::$variant(cmd) => cmd.network_config( - chain_spec, is_dev, net_config_dir, client_id, node_name, node_key + chain_spec, + is_dev, + net_config_dir, + client_id, + node_name, + node_key, + default_listen_port, ) ),* } @@ -291,15 +297,21 @@ macro_rules! substrate_cli_subcommands { } } - fn rpc_http(&self) -> $crate::Result<::std::option::Option<::std::net::SocketAddr>> { + fn rpc_http( + &self, + default_listen_port: u16, + ) -> $crate::Result> { match self { - $($enum::$variant(cmd) => cmd.rpc_http()),* + $($enum::$variant(cmd) => cmd.rpc_http(default_listen_port)),* } } - fn rpc_ws(&self) -> $crate::Result<::std::option::Option<::std::net::SocketAddr>> { + fn rpc_ws( + &self, + default_listen_port: u16, + ) -> $crate::Result> { match self { - $($enum::$variant(cmd) => cmd.rpc_ws()),* + $($enum::$variant(cmd) => cmd.rpc_ws(default_listen_port)),* } } @@ -316,23 +328,23 @@ macro_rules! substrate_cli_subcommands { } fn rpc_cors(&self, is_dev: bool) - -> $crate::Result<::std::option::Option<::std::vec::Vec>> { + -> $crate::Result>> { match self { $($enum::$variant(cmd) => cmd.rpc_cors(is_dev)),* } } - fn prometheus_config(&self) - -> $crate::Result<::std::option::Option<::sc_service::config::PrometheusConfig>> { + fn prometheus_config(&self, default_listen_port: u16) + -> $crate::Result> { match self { - $($enum::$variant(cmd) => cmd.prometheus_config()),* + $($enum::$variant(cmd) => cmd.prometheus_config(default_listen_port)),* } } fn telemetry_endpoints( &self, - chain_spec: &Box, - ) -> $crate::Result<::std::option::Option<::sc_service::config::TelemetryEndpoints>> { + chain_spec: &Box, + ) -> $crate::Result> { match self { $($enum::$variant(cmd) => cmd.telemetry_endpoints(chain_spec)),* } diff --git a/substrate/client/cli/src/commands/run_cmd.rs b/substrate/client/cli/src/commands/run_cmd.rs index de5589196f..019b760e5b 100644 --- a/substrate/client/cli/src/commands/run_cmd.rs +++ b/substrate/client/cli/src/commands/run_cmd.rs @@ -382,7 +382,7 @@ impl CliConfiguration for RunCmd { Ok(self.shared_params.dev || self.force_authoring) } - fn prometheus_config(&self) -> Result> { + fn prometheus_config(&self, default_listen_port: u16) -> Result> { Ok(if self.no_prometheus { None } else { @@ -393,7 +393,10 @@ impl CliConfiguration for RunCmd { }; Some(PrometheusConfig::new_with_default_registry( - SocketAddr::new(interface.into(), self.prometheus_port.unwrap_or(9615)) + SocketAddr::new( + interface.into(), + self.prometheus_port.unwrap_or(default_listen_port), + ) )) }) } @@ -427,7 +430,7 @@ impl CliConfiguration for RunCmd { .into()) } - fn rpc_http(&self) -> Result> { + fn rpc_http(&self, default_listen_port: u16) -> Result> { let interface = rpc_interface( self.rpc_external, self.unsafe_rpc_external, @@ -435,22 +438,22 @@ impl CliConfiguration for RunCmd { self.validator )?; - Ok(Some(SocketAddr::new(interface, self.rpc_port.unwrap_or(9933)))) + Ok(Some(SocketAddr::new(interface, self.rpc_port.unwrap_or(default_listen_port)))) } fn rpc_ipc(&self) -> Result> { Ok(self.ipc_path.clone()) } - fn rpc_ws(&self) -> Result> { + fn rpc_ws(&self, default_listen_port: u16) -> Result> { let interface = rpc_interface( self.ws_external, self.unsafe_ws_external, self.rpc_methods, - self.validator + self.validator, )?; - Ok(Some(SocketAddr::new(interface, self.ws_port.unwrap_or(9944)))) + Ok(Some(SocketAddr::new(interface, self.ws_port.unwrap_or(default_listen_port)))) } fn rpc_methods(&self) -> Result { diff --git a/substrate/client/cli/src/config.rs b/substrate/client/cli/src/config.rs index efda45a0ec..ff0222216c 100644 --- a/substrate/client/cli/src/config.rs +++ b/substrate/client/cli/src/config.rs @@ -41,8 +41,44 @@ pub(crate) const NODE_NAME_MAX_LENGTH: usize = 64; /// default sub directory to store network config pub(crate) const DEFAULT_NETWORK_CONFIG_PATH: &'static str = "network"; +/// Default configuration values used by Substrate +/// +/// These values will be used by [`CliConfiguritation`] to set +/// default values for e.g. the listen port or the RPC port. +pub trait DefaultConfigurationValues { + /// The port Substrate should listen on for p2p connections. + /// + /// By default this is `30333`. + fn p2p_listen_port() -> u16 { + 30333 + } + + /// The port Substrate should listen on for websocket connections. + /// + /// By default this is `9944`. + fn rpc_ws_listen_port() -> u16 { + 9944 + } + + /// The port Substrate should listen on for http connections. + /// + /// By default this is `9933`. + fn rpc_http_listen_port() -> u16 { + 9933 + } + + /// The port Substrate should listen on for prometheus connections. + /// + /// By default this is `9615`. + fn prometheus_listen_port() -> u16 { + 9615 + } +} + +impl DefaultConfigurationValues for () {} + /// A trait that allows converting an object to a Configuration -pub trait CliConfiguration: Sized { +pub trait CliConfiguration: Sized { /// Get the SharedParams for this object fn shared_params(&self) -> &SharedParams; @@ -122,6 +158,7 @@ pub trait CliConfiguration: Sized { client_id: &str, node_name: &str, node_key: NodeKeyConfig, + default_listen_port: u16, ) -> Result { Ok(if let Some(network_params) = self.network_params() { network_params.network_config( @@ -131,6 +168,7 @@ pub trait CliConfiguration: Sized { client_id, node_name, node_key, + default_listen_port, ) } else { NetworkConfiguration::new( @@ -257,22 +295,22 @@ pub trait CliConfiguration: Sized { /// Get the RPC HTTP address (`None` if disabled). /// /// By default this is `None`. - fn rpc_http(&self) -> Result> { - Ok(Default::default()) + fn rpc_http(&self, _default_listen_port: u16) -> Result> { + Ok(None) } /// Get the RPC IPC path (`None` if disabled). /// /// By default this is `None`. fn rpc_ipc(&self) -> Result> { - Ok(Default::default()) + Ok(None) } /// Get the RPC websocket address (`None` if disabled). /// /// By default this is `None`. - fn rpc_ws(&self) -> Result> { - Ok(Default::default()) + fn rpc_ws(&self, _default_listen_port: u16) -> Result> { + Ok(None) } /// Returns the RPC method set to expose. @@ -287,12 +325,12 @@ pub trait CliConfiguration: Sized { /// /// By default this is `None`. fn rpc_ws_max_connections(&self) -> Result> { - Ok(Default::default()) + Ok(None) } /// Get the RPC cors (`None` if disabled) /// - /// By default this is `None`. + /// By default this is `Some(Vec::new())`. fn rpc_cors(&self, _is_dev: bool) -> Result>> { Ok(Some(Vec::new())) } @@ -300,8 +338,8 @@ pub trait CliConfiguration: Sized { /// Get the prometheus configuration (`None` if disabled) /// /// By default this is `None`. - fn prometheus_config(&self) -> Result> { - Ok(Default::default()) + fn prometheus_config(&self, _default_listen_port: u16) -> Result> { + Ok(None) } /// Get the telemetry endpoints (if any) @@ -318,14 +356,14 @@ pub trait CliConfiguration: Sized { /// /// By default this is `None`. fn telemetry_external_transport(&self) -> Result> { - Ok(Default::default()) + Ok(None) } /// Get the default value for heap pages /// /// By default this is `None`. fn default_heap_pages(&self) -> Result> { - Ok(Default::default()) + Ok(None) } /// Returns an offchain worker config wrapped in `Ok(_)` @@ -445,6 +483,7 @@ pub trait CliConfiguration: Sized { client_id.as_str(), self.node_name()?.as_str(), node_key, + DCV::p2p_listen_port(), )?, keystore: self.keystore_config(&config_dir)?, database: self.database_config(&config_dir, database_cache_size, database)?, @@ -453,13 +492,13 @@ pub trait CliConfiguration: Sized { pruning: self.pruning(unsafe_pruning, &role)?, wasm_method: self.wasm_method()?, execution_strategies: self.execution_strategies(is_dev, is_validator)?, - rpc_http: self.rpc_http()?, - rpc_ws: self.rpc_ws()?, + rpc_http: self.rpc_http(DCV::rpc_http_listen_port())?, + rpc_ws: self.rpc_ws(DCV::rpc_ws_listen_port())?, rpc_ipc: self.rpc_ipc()?, rpc_methods: self.rpc_methods()?, rpc_ws_max_connections: self.rpc_ws_max_connections()?, rpc_cors: self.rpc_cors(is_dev)?, - prometheus_config: self.prometheus_config()?, + prometheus_config: self.prometheus_config(DCV::prometheus_listen_port())?, telemetry_endpoints: self.telemetry_endpoints(&chain_spec)?, telemetry_external_transport: self.telemetry_external_transport()?, default_heap_pages: self.default_heap_pages()?, diff --git a/substrate/client/cli/src/lib.rs b/substrate/client/cli/src/lib.rs index f940ab0b95..021f349aaf 100644 --- a/substrate/client/cli/src/lib.rs +++ b/substrate/client/cli/src/lib.rs @@ -209,7 +209,7 @@ pub trait SubstrateCli: Sized { } /// Only create a Configuration for the command provided in argument - fn create_configuration( + fn create_configuration, DVC: DefaultConfigurationValues>( &self, command: &T, task_executor: TaskExecutor, diff --git a/substrate/client/cli/src/params/network_params.rs b/substrate/client/cli/src/params/network_params.rs index 253585544d..4a33644e89 100644 --- a/substrate/client/cli/src/params/network_params.rs +++ b/substrate/client/cli/src/params/network_params.rs @@ -114,8 +114,9 @@ impl NetworkParams { client_id: &str, node_name: &str, node_key: NodeKeyConfig, + default_listen_port: u16, ) -> NetworkConfiguration { - let port = self.port.unwrap_or(30333); + let port = self.port.unwrap_or(default_listen_port); let listen_addresses = if self.listen_addr.is_empty() { vec![