Skip to content
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 8 commits into from
Feb 16, 2021
Merged

Rework relay CLI a bit #734

merged 8 commits into from
Feb 16, 2021

Conversation

tomusdrw
Copy link
Contributor

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:

$ ./target/release/substrate-relay --help
substrate-relay 0.1.0
Substrate-to-Substrate relay

USAGE:
    substrate-relay <SUBCOMMAND>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

SUBCOMMANDS:
    help                                          Prints this message or the help of the given subcommand(s)
    initialize-millau-headers-bridge-in-rialto    Initialize Millau headers bridge in Rialto
    initialize-rialto-headers-bridge-in-millau    Initialize Rialto headers bridge in Millau
    millau-headers-to-rialto                      Relay Millau headers to Rialto
    millau-messages-to-rialto                     Serve given lane of Millau -> Rialto messages
    rialto-headers-to-millau                      Relay Rialto headers to Millau
    rialto-messages-to-millau                     Serve given lane of Rialto -> Millau messages
    submit-millau-to-rialto-message               Submit message to given Millau -> Rialto lane
    submit-rialto-to-millau-message               Submit message to given Rialto -> Millau lane

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:

$ ./target/release/substrate-relay --help
substrate-relay 0.1.0
Substrate-to-Substrate relay

USAGE:
    substrate-relay <SUBCOMMAND>

FLAGS:
    -h, --help       
            Prints help information

    -V, --version    
            Prints version information


SUBCOMMANDS:
    help              Prints this message or the help of the given subcommand(s)
    init-bridge       Initialize on-chain bridge pallet with current header data
    relay-headers     Start headers relay between two chains
    relay-messages    Start messages relay between two chains
    send-message      Send custom message over the bridge

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.

Copy link
Contributor

@adoerr adoerr left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

ses uses

@adoerr adoerr merged commit 78b843d into master Feb 16, 2021
@adoerr adoerr deleted the td-relay-cli branch February 16, 2021 15:26
@tomusdrw tomusdrw mentioned this pull request Mar 30, 2021
5 tasks
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants