Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Decouple & move default ports to CliConfiguration #6250

Closed
cecton wants to merge 13 commits into from
Closed

Decouple & move default ports to CliConfiguration #6250

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6384bc4
Initial commit
cecton Jun 4, 2020
1ef6fa8
Move default ports to CliConfiguration
cecton Jun 4, 2020
e48ccf8
replace options by value directly
cecton Jun 4, 2020
c7f9851
Remove &self and Result for default_ methods
cecton Jun 4, 2020
c3028d5
move all the things to SubstrateCli
cecton Jun 4, 2020
5acaa63
Update client/cli/src/commands/mod.rs
cecton Jun 8, 2020
b401b8d
Update client/cli/src/commands/mod.rs
cecton Jun 8, 2020
be8de48
Merge commit feb334d87773331c23ef6560a55f43fcb5ef62a8 (no conflict)
cecton Jun 9, 2020
df58587
Revert "move all the things to SubstrateCli"
cecton Jun 10, 2020
e07b9a4
Revert "Remove &self and Result for default_ methods"
cecton Jun 10, 2020
1b325b8
Merge commit f06d5f0ff1b94c0e66b7bda871274e9cba2fc5d6 (no conflict)
cecton Jun 10, 2020
e77365a
Added changelog
cecton Jun 10, 2020
90b4f44
Update CHANGELOG.md
cecton Jun 11, 2020
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
Prev Previous commit
Next Next commit
move all the things to SubstrateCli
  • Loading branch information
@cecton
cecton committed Jun 4, 2020
commit c3028d58780a6d0e40b047eaad89396241b0aed9
27 changes: 3 additions & 24 deletions client/cli/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,27 +259,13 @@ pub trait CliConfiguration: Sized {
Ok(Default::default())
}

/// Get the default RPC HTTP port.
///
/// By default this is 9933.
fn default_rpc_http_port() -> u16 {
9933
}

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

/// Get the default RPC websocket port.
///
/// By default this is 9944.
fn default_rpc_ws_port() -> u16 {
9944
}

/// Returns the RPC method set to expose.
///
/// By default this is `RpcMethods::Auto` (unsafe RPCs are denied iff
Expand Down Expand Up @@ -309,13 +295,6 @@ pub trait CliConfiguration: Sized {
Ok(Default::default())
}

/// Get the default prometheus port
///
/// By default this 9615.
fn default_prometheus_port() -> u16 {
9615
}

/// Get the telemetry endpoints (if any)
///
/// By default this is retrieved from the chain spec loaded by `load_spec`.
Expand Down Expand Up @@ -466,12 +445,12 @@ pub trait CliConfiguration: Sized {
pruning: self.pruning(unsafe_pruning, &role)?,
wasm_method: self.wasm_method()?,
execution_strategies: self.execution_strategies(is_dev)?,
rpc_http: self.rpc_http(Self::default_rpc_http_port())?,
rpc_ws: self.rpc_ws(Self::default_rpc_ws_port())?,
rpc_http: self.rpc_http(C::default_rpc_http_port())?,
rpc_ws: self.rpc_ws(C::default_rpc_ws_port())?,
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(Self::default_prometheus_port())?,
prometheus_config: self.prometheus_config(C::default_prometheus_port())?,
telemetry_endpoints: self.telemetry_endpoints(&chain_spec)?,
telemetry_external_transport: self.telemetry_external_transport()?,
default_heap_pages: self.default_heap_pages()?,
Expand Down
21 changes: 21 additions & 0 deletions client/cli/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,27 @@ pub trait SubstrateCli: Sized {
command.init::<Self>()?;
Runner::new(self, command)
}

/// Default RPC HTTP port.
///
/// By default this is 9933.
fn default_rpc_http_port() -> u16 {
Copy link
Member

@bkchr bkchr Jun 8, 2020

Choose a reason for hiding this comment

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

I don't really understand why he have these ports in this trait here?

Copy link
Contributor Author

@cecton cecton Jun 8, 2020

Choose a reason for hiding this comment

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

I explained in detail here: #6250 (comment)

TL;DR: This is to make sure the default value is the same everywhere (on all subcommands). SubstrateCli has kind of "static"/"constant"-like stuff while ConfigurationCli know how to transform things for a subcommand.

Copy link
Member

@bkchr bkchr Jun 8, 2020

Choose a reason for hiding this comment

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

I'm not entirely sold on this solution. This distinction between these traits seems rather random. And what is with other default values? Why are they not override-able?

Copy link
Contributor Author

@cecton cecton Jun 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Me neither 😅 initially when I did the refactoring I wanted to have only one trait but it wasn't possible.

Some things work using the parameters provided by the user (CliConfiguration) like: getting the database parameters.

Some other things work using kinda global values (SubstrateCli) like: impl name, impl version.

Some are a bit of both like: getting the chain spec value from user input but process the input the same way for the entire Cli.

In CliConfiguration there are no function that return what should be the default value of something. They are all kinda getter to the user input. Here what we override is the default port of something.

Well, either way I'm out of argument so I will let you decide what is best among those options and I will adapt the code accordingly:

  1. Move the default port methods to CliConfiguration and use Result and &self (what I did originally)
  2. Move the default port methods to CliConfiguration and not use Result but use &self (what @expenses proposed)
  3. Move the default port methods to SubstrateCli and not use Result nor &self (what is it right now)

(Note: there is no option for using CliConfiguration without Result and without &self because &self is required to dispatch to the right subcommand if used in CliConfiguration. This is not the case for SubstrateCli because the trait is kinda global to the entire Cli app.)

🔝 ☝️ cc @seunlanlege (because related to encapsulation OCD)

The alternative is to have these methods on CliConfiguration and use proc macro magic to override functions

I have been asked to avoid proc macros in the API as much as possible, so this won't even be an option for me.

9933
}

/// Default RPC websocket port.
///
/// By default this is 9944.
fn default_rpc_ws_port() -> u16 {
9944
}

/// Default prometheus port.
///
/// By default this 9615.
fn default_prometheus_port() -> u16 {
9615
}
}

/// Initialize the logger
Expand Down