Combine default values used at initialization in a trait

[bkchr]

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.

I'm open for any ideas on how to improve the name of the trait, because it sounds awful.

[bkchr]

See https://github.com/paritytech/cumulus/pull/189/files#diff-843c1b140143c42356bf6758355171b5

For how it will look like in Cumulus.

[cecton]

I feel like we are going in circle 😓 (no offense!)

Right now we have 2 ways:

1. the default values come from CliConfiguration
2. the default values come from SubstrateCli through CliConfiguration

Here you are introducing a third way but it feels very much like a mix of both options: it comes from a third trait through CliConfiguration.

I don't think it's bad but I don't see much the value compared to having the defaults directly in CliConfiguration (or the other). Do you feel it's better than the others?

In your implementation though I find the usage a bit special. Having to put Self in the type argument seems weird.

About the naming:

The only reason why there is this distinction between CliConfiguration and SubstrateCli is because SubstrateCli kinda holds "global" variables (node iml name, version, ...) while CliConfiguration holds behavior specific to a command (every command implements its CliConfiguration).

I'm also noticing that the only defaults we have at the moments are for ports. Therefore, only the RunCmd is interested in it. Though that could potentially change.

(DefaultConfigurationValues is a fine name imo)

Here is another solution I have in mind

Use a new struct for the defaults in sc-cli (let's call it "Ports" for the sake of the example) which would have a Default impl (which gives the ports we know). Then we change the runner when we run a node to take an extra argument ("ports"). This argument could be an Option that we unwrap_or_default() (so it becomes Ports' defaults). The user could change the default ports like this: Some(Ports { rpc_http: 9933, rpc_ws: 9944, ..Default::default() }).

Another advantage of this implementation is that the user can code a runtime behavior for the default ports.

[bkchr]

Right now we have 2 ways:

1. the default values come from CliConfiguration

2. the default values come from SubstrateCli through CliConfiguration

I honestly don't understand this code and it is always very complicated to read. The current CliConfiguration overriding we have done in Cumulus is just a huge hack, because we copied the full function implementation which is also not synchronized anymore with the one in Substrate.

I don't think it's bad but I don't see much the value compared to having the defaults directly in CliConfiguration (or the other). Do you feel it's better than the others?

Yes, because it will enable us to drop the custom RelayChainCli struct in the future. We will be able to just use the default Cli struct from Polkadot.

In your implementation though I find the usage a bit special. Having to put Self in the type argument seems weird.

Not sure why this is weird, but I just have done this to get it working. This is normal Rust code.

Use a new struct for the defaults in sc-cli (let's call it "Ports" for the sake of the example) which would have a Default impl (which gives the ports we know). Then we change the runner when we run a node to take an extra argument ("ports"). This argument could be an Option that we unwrap_or_default() (so it becomes Ports' defaults). The user could change the default ports like this: Some(Ports { rpc_http: 9933, rpc_ws: 9944, ..Default::default() }).

While I somewhat like the idea, I don't like that we will add another parameter that will be ignored by 90% of all runners.

[expenses]

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

[danforbes]

How's about CliConfigurationDefaults?

[cecton]

That's actually better 👍

[danforbes]

Suggested change

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

[danforbes]

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?

[cecton]

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.

[cecton]

Sorry for the delay. I will come back to it very soon.

In the meantime I bumped into this: https://crates.io/crates/structopt-toml maybe something we can inspire from (or use). No idea, I didn't really check yet.

[cecton]

Yes, because it will enable us to drop the custom RelayChainCli struct in the future. We will be able to just use the default Cli struct from Polkadot.

I think I finally get your idea. I made a playground here to demonstrate it. In this playground, CliConfiguration is not implemented for RelayCli;

BUT 😁 As you can see in this playground, if we put the fn default_* to SubstrateCli, we do have the same perk without introducing a new trait and a new type argument.

So my question is: what's wrong with using SubstrateCli to put our defaults? Example here

Not sure why this is weird, but I just have done this to get it working. This is normal Rust code.

I have never seen such pattern in a library. I would have preferred a more stupid simple way with less typing. But I reckon this is still a big improvement.

I'm NOT saying in any way that the current SubstrateCli + CliConfiguration traits are great. It certainly isn't but they have minimal complexity. Adding a new trait and a new type argument is an added complexity. I would like to hear a good reason to do so.

I think it would already improve a lot to properly name those traits.

• rename SubstrateCli to NodeConfiguration (because this very much about the node and has little to do with the cli, it's only used by the cli) (I'm not even sure if this trait should be in sc-cli at all, maybe it's more a sc-service thingy)
• rename either SubstrateCli::create_configuration either CliConfiguration::create_configuration. The name conflicts because they are mostly implemented on the same object.

So for me the ONLY question that matters is: why does it bother you to have the defaults on SubstrateCli.

cc @bkchr @danforbes

[cecton]

I don't think it's worth blocking the PR so feel free to merge if you weren't convinced by my points