Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 27 additions & 15 deletions client/cli/src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -189,17 +188,24 @@ macro_rules! substrate_cli_subcommands {

fn network_config(
&self,
chain_spec: &::std::boxed::Box<dyn ::sc_service::ChainSpec>,
chain_spec: &std::boxed::Box<dyn sc_service::ChainSpec>,
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this parameter actually a default value or is this something that is being configured by the user somehow and then actually used to construct an instance of some type?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default ports are hardcoded in sc-cli but during runtime the user can override them. Cumulus has a third way to this approach: it overrides the default ports so the user can still override at runtime but the default value would be different.

This is because cumulus has 2 nodes running.

) -> $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,
)
),*
}
Expand Down Expand Up @@ -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<std::option::Option<std::net::SocketAddr>> {
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<std::option::Option<std::net::SocketAddr>> {
match self {
$($enum::$variant(cmd) => cmd.rpc_ws()),*
$($enum::$variant(cmd) => cmd.rpc_ws(default_listen_port)),*
}
}

Expand All @@ -316,23 +328,23 @@ macro_rules! substrate_cli_subcommands {
}

fn rpc_cors(&self, is_dev: bool)
-> $crate::Result<::std::option::Option<::std::vec::Vec<String>>> {
-> $crate::Result<std::option::Option<std::vec::Vec<String>>> {
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<std::option::Option<sc_service::config::PrometheusConfig>> {
match self {
$($enum::$variant(cmd) => cmd.prometheus_config()),*
$($enum::$variant(cmd) => cmd.prometheus_config(default_listen_port)),*
}
}

fn telemetry_endpoints(
&self,
chain_spec: &Box<dyn ::sc_service::ChainSpec>,
) -> $crate::Result<::std::option::Option<::sc_service::config::TelemetryEndpoints>> {
chain_spec: &Box<dyn sc_service::ChainSpec>,
) -> $crate::Result<std::option::Option<sc_service::config::TelemetryEndpoints>> {
match self {
$($enum::$variant(cmd) => cmd.telemetry_endpoints(chain_spec)),*
}
Expand Down
17 changes: 10 additions & 7 deletions client/cli/src/commands/run_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ impl CliConfiguration for RunCmd {
Ok(self.shared_params.dev || self.force_authoring)
}

fn prometheus_config(&self) -> Result<Option<PrometheusConfig>> {
fn prometheus_config(&self, default_listen_port: u16) -> Result<Option<PrometheusConfig>> {
Ok(if self.no_prometheus {
None
} else {
Expand All @@ -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),
)
))
})
}
Expand Down Expand Up @@ -427,30 +430,30 @@ impl CliConfiguration for RunCmd {
.into())
}

fn rpc_http(&self) -> Result<Option<SocketAddr>> {
fn rpc_http(&self, default_listen_port: u16) -> Result<Option<SocketAddr>> {
let interface = rpc_interface(
self.rpc_external,
self.unsafe_rpc_external,
self.rpc_methods,
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<Option<String>> {
Ok(self.ipc_path.clone())
}

fn rpc_ws(&self) -> Result<Option<SocketAddr>> {
fn rpc_ws(&self, default_listen_port: u16) -> Result<Option<SocketAddr>> {
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<sc_service::config::RpcMethods> {
Expand Down
69 changes: 54 additions & 15 deletions client/cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danforbes any ideas of what we could call this instead? 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How's about CliConfigurationDefaults?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually better 👍

/// 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<DCV: DefaultConfigurationValues = ()>: Sized {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
pub trait CliConfiguration<Default: DefaultConfigurationValues = ()>: Sized {

/// Get the SharedParams for this object
fn shared_params(&self) -> &SharedParams;

Expand Down Expand Up @@ -122,6 +158,7 @@ pub trait CliConfiguration: Sized {
client_id: &str,
node_name: &str,
node_key: NodeKeyConfig,
default_listen_port: u16,
) -> Result<NetworkConfiguration> {
Ok(if let Some(network_params) = self.network_params() {
network_params.network_config(
Expand All @@ -131,6 +168,7 @@ pub trait CliConfiguration: Sized {
client_id,
node_name,
node_key,
default_listen_port,
)
} else {
NetworkConfiguration::new(
Expand Down Expand Up @@ -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<Option<SocketAddr>> {
Ok(Default::default())
fn rpc_http(&self, _default_listen_port: u16) -> Result<Option<SocketAddr>> {
Ok(None)
}

/// Get the RPC IPC path (`None` if disabled).
///
/// By default this is `None`.
fn rpc_ipc(&self) -> Result<Option<String>> {
Ok(Default::default())
Ok(None)
}

/// Get the RPC websocket address (`None` if disabled).
///
/// By default this is `None`.
fn rpc_ws(&self) -> Result<Option<SocketAddr>> {
Ok(Default::default())
fn rpc_ws(&self, _default_listen_port: u16) -> Result<Option<SocketAddr>> {
Ok(None)
}

/// Returns the RPC method set to expose.
Expand All @@ -287,21 +325,21 @@ pub trait CliConfiguration: Sized {
///
/// By default this is `None`.
fn rpc_ws_max_connections(&self) -> Result<Option<usize>> {
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<Option<Vec<String>>> {
Ok(Some(Vec::new()))
}

/// Get the prometheus configuration (`None` if disabled)
///
/// By default this is `None`.
fn prometheus_config(&self) -> Result<Option<PrometheusConfig>> {
Ok(Default::default())
fn prometheus_config(&self, _default_listen_port: u16) -> Result<Option<PrometheusConfig>> {
Ok(None)
}

/// Get the telemetry endpoints (if any)
Expand All @@ -318,14 +356,14 @@ pub trait CliConfiguration: Sized {
///
/// By default this is `None`.
fn telemetry_external_transport(&self) -> Result<Option<ExtTransport>> {
Ok(Default::default())
Ok(None)
}

/// Get the default value for heap pages
///
/// By default this is `None`.
fn default_heap_pages(&self) -> Result<Option<u64>> {
Ok(Default::default())
Ok(None)
}

/// Returns an offchain worker config wrapped in `Ok(_)`
Expand Down Expand Up @@ -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)?,
Expand All @@ -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()?,
Expand Down
2 changes: 1 addition & 1 deletion client/cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ pub trait SubstrateCli: Sized {
}

/// Only create a Configuration for the command provided in argument
fn create_configuration<T: CliConfiguration>(
fn create_configuration<T: CliConfiguration<DVC>, DVC: DefaultConfigurationValues>(
&self,
command: &T,
task_executor: TaskExecutor,
Expand Down
3 changes: 2 additions & 1 deletion client/cli/src/params/network_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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![
Expand Down