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

Add flag to enable interactive prompt in create channel CLI #2014

Merged
merged 29 commits into from
Apr 8, 2022

Conversation

seanchen1991
Copy link
Contributor

Closes: #1421

Description

Changes the flow of the create channel CLI command. The default invocation has been changed from

hermes create channel --port-a <PORT-ID> --port-b <PORT-ID> <CHAIN-A-ID> <CHAIN-B-ID>

to

hermes create channel --port-a <PORT-ID> --port-b <PORT-ID> <CHAIN-A-ID> <CONNECTION-A-ID>

If users want to create new clients and connections using this invocation, it is now required that --new-client-connections flag be passed along:

hermes create channel --port-a <PORT-ID> --port-b <PORT-ID> <CHAIN-A-ID> <CHAIN-B-ID> --new-client-connections

This brings up an interactive prompt that the user must agree to, letting them know that they are creating new clients / connections as part of the invocation.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@seanchen1991
Copy link
Contributor Author

There's an open issue in the dialoguer crate, which is used for the interactive prompt functionality, where the prompt is re-rendered to the terminal upon user interaction.

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Neat work! Left some suggestions below.

It seems that the 'usage' message is not very clear:

$ ./target/debug/hermes create channel
error: The following required arguments were not provided:
    --port-a <PORT_A>
    --port-b <PORT_B>
    <CHAIN_A_ID>

USAGE:
    hermes create channel [OPTIONS] --port-a <PORT_A> --port-b <PORT_B> <CHAIN_A_ID> [CHAIN_B_ID]

I think we should make it so that the 'usage' output here presents the user with the default invocation, which has a connection ID (and has no chain-b-ID). I suspect to do that we need to:

  • make the chain_b_id option to be non-positional, i.e., it should have the clap pragma config including short, long

-make the connection_a option to be positional (remove the clap pragmas config short, long.

I would also suggest we rename top-level message that create channel --help provides. It is currently:

Create a new channel between two chains

maybe would be more accurate to have

Create a new channel using an existing connection, or alternatively using a new client and connection.

relayer-cli/src/commands/create/channel.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/create/channel.rs Outdated Show resolved Hide resolved
@adizere adizere self-requested a review March 30, 2022 10:58
@adizere adizere dismissed their stale review March 30, 2022 10:59

Stale review

@adizere
Copy link
Member

adizere commented Mar 31, 2022

@mzabaluev when you get a chance have a look over & review this please.

Copy link
Contributor

@mzabaluev mzabaluev left a comment

Choose a reason for hiding this comment

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

The changes look good, though I'm still uncomfortable with bringing interactivity into the otherwise script-friendly CLI command. What's the failure mode when the input is /dev/null?

Should the guide be updated now that create channel does not automatically create a connection and clients by omission?

relayer-cli/src/commands/create/channel.rs Outdated Show resolved Hide resolved
@seanchen1991
Copy link
Contributor Author

Should the guide be updated now that create channel does not automatically create a connection and clients by omission?

Yes, I'll update the guide 👍

@romac
Copy link
Member

romac commented Apr 4, 2022

Can we update this PR with a more descriptive title?

@seanchen1991 seanchen1991 changed the title Create channel cli Add feature flag to enable interactive prompt in create channel CLI Apr 4, 2022
@romac romac changed the title Add feature flag to enable interactive prompt in create channel CLI Add flag to enable interactive prompt in create channel CLI Apr 5, 2022
Copy link
Contributor

@mzabaluev mzabaluev left a comment

Choose a reason for hiding this comment

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

Renaming looks good, just needs to be completed everywhere and the guide updated accordingly.-

guide/src/commands/path-setup/channels.md Outdated Show resolved Hide resolved
guide/src/commands/path-setup/channels.md Outdated Show resolved Hide resolved
relayer-cli/src/commands/create/channel.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/create/channel.rs Outdated Show resolved Hide resolved
@seanchen1991
Copy link
Contributor Author

The guide has been updated accordingly. @romac are there any other changes you think need to be made before we merge this?

@romac
Copy link
Member

romac commented Apr 7, 2022

Ah my bad, sorry about that. Can you please split out the guide changes into a separate PR so that we can merge it once we release v0.14? Otherwise the guide will be updated before we actually release v0.14, which may confuse users.

@romac
Copy link
Member

romac commented Apr 8, 2022

Perfect 👍

@romac romac merged commit ab11ccb into informalsystems:master Apr 8, 2022
@adizere adizere mentioned this pull request Apr 25, 2022
7 tasks
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
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.

CLI create channel can fail and spam with unused clients/connections
4 participants