-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework relay CLI a bit #734
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adoerr
approved these changes
Feb 16, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
relays/substrate/src/cli.rs
Outdated
@@ -181,6 +217,9 @@ pub enum ToMillauMessage { | |||
arg_enum! { | |||
#[derive(Debug)] | |||
/// The origin to use when dispatching the message on the target chain. | |||
/// | |||
/// - `Target` uses account existing on the target chain (requires target private key). | |||
/// - `Origin` ses account derived from the source-chain account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ses
uses
adoerr
approved these changes
Feb 16, 2021
serban300
pushed a commit
to serban300/parity-bridges-common
that referenced
this pull request
Mar 27, 2024
* Change CLI UX. * De-duplicate main. * De-duplicate send message. * Add more docs and extract functions. * Fix scripts. * cargo fmt --all * Add missing 'u'.
serban300
pushed a commit
to serban300/parity-bridges-common
that referenced
this pull request
Apr 8, 2024
* Change CLI UX. * De-duplicate main. * De-duplicate send message. * Add more docs and extract functions. * Fix scripts. * cargo fmt --all * Add missing 'u'.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR extracts some common parts of the CLI commands to avoid duplication and also changes the way CLI is structured a bit.
Currently the CLI is flat:
IMHO it becomes already a bit difficult to parse long strings, and since I'm planning to add even more commands (see #607, #608, #609) I've changed it to have a bit more structure:
Initially I tried to split the CLI into network related commands (like
$ relay millau to-rialto headers
), but this quickly get's out of hand and is not user friendly.Instead in the top level you will get different functions of the relayer binary first and then you see networks.
I wanted to avoid doing too much changes, so the commands are still in a single module, but I think the next step might be extracting commands/networks/bridges into separate modules.
Another thing worth trying in the future is avoiding duplication via macro, but while it will remove duplication I think it won't make the code more readable/maintainable, hence after a couple of failed attempts I didn't follow that direction further.