Make --collator reusable and imply --validator

[JoshOrndorff]

This is my attempt at solving #368

This addresses the primary concern about the --collator flag not being equivalent to the --validator flag, as well as the part about the collator flag not being reusable. With this PR, parachain projects can import the cumulus RunCmd which includes the collator flag.

I was really hoping structopt would allow me to simply add an annotation #[structopt(implies = validator)] but it seems like there is no notion of "implies" in structopt.

[JoshOrndorff]

@cecton Feedback on my use of structopt here is very welcome.

[JoshOrndorff]

I've filled in the rest of the methods now. I'm glad you pointed it out too because there were two others that needed to be overridden. I have one more question for you, and it is about the helper function rpc_interface. I copied it from sc_cli, but I wonder if it should be made public there. What do you think?

[cecton]

I copied it from sc_cli, but I wonder if it should be made public there. What do you think?

Normally I would say it wouldn't be bad but this is so "lonely" that probably nobody would notice this is here and it is useful...

... 🤔 ... maybe if we can move it to some object it would feel better as part of the API... like an method on an instance of RpcMethods?

fn choose_rpc_interface(&self, is_external, is_unsafe, is_validator)
-> sc_cli::Result<IpAddr> { ... }

What do you think @JoshOrndorff

[JoshOrndorff]

I think making it a method is great. I would also be okay just adding pub. Would you like to try making it a method?

[JoshOrndorff]

Are we waiting on paritytech/substrate#8505 before reviewing this one? Or shall we merge this one first and refactor it after?

[cecton]

Just a small knit to fix but otherwise I think we can merge for now and we will update later if the other PR gets merged at some point

[bkchr]

Sorry for the later answer, however it took some time until I found some solution I'm happy with.

The current implementation is not making me happy, especially as you have manually overwritten some of the methods to respect collator.

I think we should do the following way:

/// The `run` command used to run a node.
#[derive(Debug, StructOpt)]
pub struct RunCmd {
	/// Our run command inherents from sc_cli's
	#[structopt(flatten)]
	pub base: sc_cli::RunCmd,
	/// Id of the parachain this collator collates for.
	#[structopt(long)]
	pub parachain_id: Option<u32>,
	/// Run node as collator.
	///
	/// Note that this is the same as running with `--validator`.
	#[structopt(long, conflicts_with = "validator")]
	pub collator: bool,
}

// as you already have it. But we don't implement `CliConfiguration` for this struct.
// CliConfiguration should be implemented for some other struct that we transform `RunCmd` into.

impl RunCmd {
    /// Find better name ;P
    fn transform(self) -> Transformed {
         self.base.validator = self.base.validator || self.collator;
         // Transformed should implement `CliConfiguration`, but this time we can forward everything to the `base` cmd.
         Transformed { self.base, self.parachain_id }
    }
}

[bkchr]

ping @JoshOrndorff

[JoshOrndorff]

Thanks for your review. I understand what you mean.

I just haven't made time to get back to this. I can take another look by next week.

[bkchr]

Ty

[JoshOrndorff]

I've taken an attempt at the design change you recommended. I'm not quite sure where you want the transformation / normalization to happen though.

I wonder if the example code you gave is really supposed to take ownership of the original RunCmd?

impl RunCmd {
    /// Find better name ;P
    fn transform(self) -> Transformed {
         self.base.validator = self.base.validator || self.collator;
         // Transformed should implement `CliConfiguration`, but this time we can forward everything to the `base` cmd.
         Transformed { self.base, self.parachain_id }
    }
}

I've tried it this way and also with only borrowing, and I've hit a different error either way. The code explains it in context.Feedback welcome.

[JoshOrndorff]

This PR now also updates the Substrate dependency by three commits to include @bkchr's work from paritytech/substrate#8731

These are the three commits. Two of them are harmless and one is the change I want, so I think this should be fine.

[bkchr]

Suggested change

	/// Transform this RunCmd into a NormalizedRunCmd which does not have a redundant `collator` field.
	pub fn normalize(&self) -> NormalizedRunCmd {
	/// Transform this RunCmd into a NormalizedRunCmd which does not have a redundant `collator` field.
	pub fn normalize(&self) -> NormalizedRunCmd {

Do you maybe have a better idea for the function name?

And the docs should also be updated

[JoshOrndorff]

Do you not like normalize? I like it because it indicates that we aren't really changing any of the data, just writing it in a more standard form. standardize would also be fine. I like both of those better than the original transform.

I'm fine with any, of them, but I can't spend too long thinking about it because I have to paint a bike shed, and haven't decided what color it should be yet ;-)

[JoshOrndorff]

I updated the docs as you requested.

[bkchr]

Fine

[bkchr]

bot merge

[ghost]

Waiting for commit status.