Splitting relaychain and parachain command-line arguments #42
Conversation
|
It looks like @cecton signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
test/parachain/src/cli.rs
Outdated
| config.in_chain_config_dir("polkadot"), | ||
| ) | ||
| .map_err(|e| e.to_string())? | ||
| .expect("not a run command?"); |
There was a problem hiding this comment.
in Substrate and Polkadot codebases, we have a stricter policy on panickers than that - please provide a logical proof (premise -> conclusion) that this does not fail. If you can't think of one, find a way to handle the failure case, probably by performing a more graceful error.
There was a problem hiding this comment.
I don't think I can proof anything here. The user can send other commands than RunCmd. But I think we expect only RunCmd here, right? @bkchr
There was a problem hiding this comment.
If you can't prove anything, then you can get rid of the expect and exit gracefully. There's no reason to panic - panic is for unrecoverable errors.
There was a problem hiding this comment.
I just checked because I changed the code and I forgot exactly what was the issue. But I left that "?" expressly for someone to check that at the review.
This is an unwrap on an Option. This option is always Some unless the command was a custom command. In the current context, it is impossible that it is a custom command because the parse_and_prepare is ran with NoCustom. So this won't happen.
Nevertheless... the subcommand can be anything: it can be a RunCmd (what we expect) but also an import-blocks, export-blocks and everything else that is supposed. This makes no sense and I'm afraid at the comment there isn't much I can't do to prevent this.
There was a problem hiding this comment.
This is an unwrap on an Option. This option is always Some unless the command was a custom command. In the current context, it is impossible that it is a custom command because the parse_and_prepare is ran with NoCustom. So this won't happen.
It seems like you are misunderstanding what I'm trying to communicate.
The point is to put this logic in the code so that if it triggers later we can see exactly what assumption was broken. It also allows the reviewer to see exactly what assumption you are making. I am not saying to never use expect, only to use it with these guidelines, which exist to make the codebase more robust.
Here is an example (although I haven't fact-checked your logic):
expect("can only fail when this is not a RunCmd. Running parse_and_prepare with NoCustom can only return a RunCmd; therefore this is a RunCmd; qed")
I agree with you that the API could be improved, so that this property is enforced by the type system.
There was a problem hiding this comment.
@rphmeier I adapted the message consequently but I think you misunderstood what I said.
Whatever I do, the user CAN use import-blocks, export-blocks and the other subcommands in the relay chain arguments. There is nothing I can do to prevent that unless I do quite big modifications on substrate's cli.
I believe the current behavior is acceptable for now but I plan to refactor substrate's cli and I will fix this in a near future.
(The current behavior is: whatever subcommand you use in the relay chain arguments, it will run the collator)
There was a problem hiding this comment.
Whatever I do, the user CAN use import-blocks, export-blocks and the other subcommands in the relay chain arguments. There is nothing I can do to prevent that unless I do quite big modifications on substrate's cli.
I followed you this far in the previous comment, and I agree that it would be a large refactoring. Can you clarify: the expect call you added cannot be triggered by user input?
There was a problem hiding this comment.
No it's impossible. into_configuration returns None only in case of a custom command but parse_and_prepare (the second one, not the first one) is ran without custom command (NoCustom)
There was a problem hiding this comment.
I have already started the refactoring and it will makes some things (like this) much easier: paritytech/substrate#4692
test/parachain/src/cli.rs
Outdated
| config.in_chain_config_dir("polkadot"), | ||
| ) | ||
| .map_err(|e| e.to_string())? | ||
| .expect("not a run command?"); |
.editorconfig
Outdated
| max_line_length=100 | ||
| insert_final_newline=true | ||
|
|
||
| [*.yml] |
There was a problem hiding this comment.
I shamefully copied the file from substrate 😁
56111b0 to
4919c27
Compare
This reverts commit 9eee632.
It changes the way we extended the CLI functionalities of substrate to allow more flexibility. (If this was not clear, here is another version: it changes the `sc_cli` API to allow more flexibility).
This touches a few important things:
- the startup of the async task with tokei:
This was in node and node-template and I moved it to substrate. The idea is to have 1 time the code that handles unix signals (SIGTERM and SIGINT) properly. It is however possible to make this more generic to wait for a future instead and provide only a helper for the basic handling of SIGTERM and SIGINT.
- increased the version of structopt and tokei
- no more use of structopt internal's API
- less use of generics
Related to #4643 and paritytech/cumulus#42: the implementation of "into_configuration" and "get_config" are similar but with better flexibility so it is now possible in cumulus to have the command-line arguments only of the run command for polkadot if we want
Related to paritytech/cumulus#24 and paritytech/cumulus#34 : it will now be possible to make a configuration struct for polkadot with some overrides of the default parameters much more easily.
Fixes #34