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

ADR for unified approach for CLI arguments for Hermes #2306

Merged
merged 14 commits into from
Jun 17, 2022
Merged
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
182 changes: 182 additions & 0 deletions docs/architecture/adr-010-unified-cli-arguments-hermes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
# ADR 010: Unified approach for CLI arguments for Hermes v1.0.0

## Changelog
* 15.06.2022: Proposed.

## Context

In this ADR we provide recommendations and intuitions to using flags for all the arguments of the Hermes commands.

The problem we are trying to solve is a unified approach to CLI arguments for Hermes v1.0.0.

## Decision

To avoid confusion, all the parameters should take long flags. The following general scenarios should be applied:

* Only long flags are used in order to avoid having nonintuitive flags or conflicting flags.
* Any parameter ending with `_id` should have the `_id` removed from the flag to shorten it. For example the flag for `chain_id` should only be `chain`.
* Flags which can be shorten and still be meaningful should be shorten. This is done for `connection`, `channel` and `sequence`, which become respectively `conn`, `chan` and `seq`.
* In cases where there are undirectional parameters for a same object, the flags end with the suffix `-a` and `-b`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are "unidirectional" parameters?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also we have src_chain but chain_a. In the code we use src_chain and a_chain. The qualifier should be either prefix or suffix but not both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My choice of word is confusing sorry. My interpretation of nondirectional parameters are the ones where using parameters --a A and --b B will result the same as --a B and --b A.

Let's use create connection as an example. There are two chains ibc-0 and ibc-1. There will have be an internal difference between --chain-a ibc-0 and --chain-b ibc-1 and --chain-a ibc-1 and --chain-b ibc-0, which is the chain that initialises the handshake. But the created connection is the same.

This is the reason why src and dst seemed confusing if used for all the cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My choice of word is confusing sorry. My interpretation of nondirectional parameters are the ones where using parameters --a A and --b B will result the same as --a B and --b A.

But the result is the same. I propose we do not use unidirectional. We can say that A and B identify entities (chains, clients, connections, ports) on the two ends of a connection or channel. We can just mention that OpenInit is sent to chain A.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also we have src_chain but chain_a. In the code we use src_chain and a_chain. The qualifier should be either prefix or suffix but not both.

I'm a bit confused by this as there is a mix of both chain_a and a_chain in the code. From a quick search I only see a_chain in the relayer create. But chain_a is in both the relayer and relayer-cli crates. If only prefix or suffix should be used, I find the prefix better, mostly for source and destination.

My choice of word is confusing sorry. My interpretation of nondirectional parameters are the ones where using parameters --a A and --b B will result the same as --a B and --b A.

But the result is the same. I propose we do not use unidirectional. We can say that A and B identify entities (chains, clients, connections, ports) on the two ends of a connection or channel. We can just mention that OpenInit is sent to chain A.

I will change my phrasing to: "In cases where there are two separate parameters for a same object type" would that seem better ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good! @ljoss17 could you please make the changes and we can see how they read? I think we are pretty close to merging this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put them to suffixes, let me know if this is preferred.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i read the doc again and not super happy with how chain-host and chain-reference read. The latter for example reads "the reference of the chain". My order of preference now is:

  1. a-chain, host-chain, reference-chain
  2. chain-a, host-chain, reference-chain ...even though i think it's not consistent i now prefer it slightly over 3.
  3. chain-a, chain-host, chain-reference

@adizere, @mircea-c et al. wdyt?

I will let you all decide, in the end I am ok with any of the above and will be much better than what we have today.

Choose a reason for hiding this comment

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

I am partial to the first option. Only thing I don't like is the suffix for the unidirectional. Could just be chain without a suffix

Copy link
Member

Choose a reason for hiding this comment

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

First option sounds good also to me!

Only thing I don't like is the suffix for the unidirectional. Could just be chain without a suffix

Not sure what you mean.


The following commands are implemented, with the binary name `hermes` omitted:

### Hermes global flags

* `hermes --config <CONFIG> <COMMAND>`

* `hermes --json <COMMAND>`

### Commands for clients

* `create client --host-chain <HOST_CHAIN_ID> --reference-chain <REFERENCE_CHAIN_ID>`
* Optional: `[--clock-drift <CLOCK_DRIFT>] [--trust-threshold <TRUST_THRESHOLD>] [--trusting-period <TRUSTING_PERDIOD>]`

* `update client --chain <CHAIN_ID> --client <CLIENT_ID>`
* Optional: `[--target-height <TARGET_HEIGHT>] [--trusted-height <TRUSTED_HEIGHT>]`

* `upgrade client --chain <CHAIN_ID> --client <CLIENT_ID>`

* `upgrade clients --chain <CHAIN_ID>`
ljoss17 marked this conversation as resolved.
Show resolved Hide resolved

### Create a connection

* `create connection --chain-a <CHAIN_A_ID> --chain-b <CHAIN_B_ID>`
* Optional: `[--delay <DELAY>]`

* `create connection --chain-a <CHAIN_A_ID> --client-a <CLIENT_A_ID> --client-b <CLIENT_B_ID>`
* Optional: `[--delay <DELAY>]`

### Create a channel

* `create channel --chain-a <CHAIN_A_ID> --chain-b <CHAIN_B_ID> --port-a <PORT_A_ID> --port-b <PORT_B_ID>`
* Optional: `[--chan-version <VERSION>] [--new-client-conn] [--order <ORDER>]`

* `create channel --chain-a <CHAIN_A_ID> --conn-a <CONNECTION_A_ID> --port-a <PORT_A_ID> --port-b <PORT_B_ID>`
* Optional: `[--chan-version <VERSION>] [--new-client-conn] [--order <ORDER>]`
ljoss17 marked this conversation as resolved.
Show resolved Hide resolved

### Commands for keys

* `keys add --chain <CHAIN_ID> --key-file <KEY_FILE> --mnemonic-file <MNEMONIC_FILE>`
* Optional: `[--hd-path <HD_PATH>] [--key-name <KEY_NAME>]`

* `keys balance --chain <CHAIN_ID>`
* Optional: `[--key-name <KEY_NAME>]`

* `keys delete --chain <CHAIN_ID> --all`

* `keys delete --chain <CHAIN_ID> --key-name <KEY_NAME>`

* `keys list --chain <CHAIN_ID>`

### Listen

* `listen --chain <CHAIN_ID>`
* Optional: `[--event <EVENT>]`

### Misbehaviour

* `misbehaviour --chain <CHAIN_ID> --client <CLIENT_ID>`

### Start the relayer in multi-chain mode

* `start`
* Optional: `[--full-scan]`

### Clear objects

* `clear packets --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID>`

### Queries
ancazamfir marked this conversation as resolved.
Show resolved Hide resolved

__Channel__

* `query channel client --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID>`

* `query channel end --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID>`
* Optional: `[--height <HEIGHT>]`

* `query channel ends --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID>`
plafer marked this conversation as resolved.
Show resolved Hide resolved
* Optional: `[--height <HEIGHT>] [--verbose]`

* `query channels --host-chain <HOST_CHAIN_ID>`
* Optional: `[--reference-chain <REFERENCE_CHAIN_ID>] [--verbose]`
ljoss17 marked this conversation as resolved.
Show resolved Hide resolved

__Client__

* `query client connections --chain <CHAIN_ID> --client <CLIENT_ID>`
* Optional: `[--height <HEIGHT>]`

* `query client consensus --chain <CHAIN_ID> --client <CLIENT_ID>`
* Optional: `[--consensus-height <CONSENSUS_HEIGHT>] [--height <HEIGHT>] [--heights-only]`

* `query client header --chain <CHAIN_ID> --client <CLIENT_ID> --consensus-height <CONSENSUS_HEIGHT>`
* Optional: `[--height <HEIGHT>]`

* `query client state --chain <CHAIN_ID> --client <CLIENT_ID>`
* Optional: `[--height <HEIGHT>]`

* `query clients --host-chain <HOST_CHAIN_ID>`
* Optional: `[--omit-chain-ids] [--reference-chain <REFERENCE_CHAIN_ID>]`

__Connection__

* `query connection channels --chain <CHAIN_ID> --conn <CONNECTION_ID>`

* `query connection end --chain <CHAIN_ID> --conn <CONNECTION_ID>`
* Optional: `[--height <HEIGHT>]`

* `query connections --chain <CHAIN_ID>`
ancazamfir marked this conversation as resolved.
Show resolved Hide resolved

__Packet__

* `query packet ack --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID> --seq <SEQUENCE>`
* Optional: `[--height <HEIGHT>]`

* `query packet acks --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID>`

* `query packet commitment --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID> --seq <SEQUENCE>`
* Optional: `[--height <HEIGHT>]`

* `query packet commitments --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID>`

* `query packet pending --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID>`

* `query packet unreceived-acks --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID>`

* `query packet unreceived-packets --chain <CHAIN_ID> --port <PORT_ID> --chan <CHANNEL_ID>`

__Tx__

* `query tx events --chain <CHAIN_ID> --hash <HASH>`

### Shell completion

* `completions --shell <SHELL>`

### Validate configuration file

* `config validate`

### Health check

* `health-check`

plafer marked this conversation as resolved.
Show resolved Hide resolved
## Status

Proposed

## Consequences

### Positive

* Clear parameters for Hermes commands

### Negative

* Breaking changes which will require updating anything using Hermes

### Neutral

## References

* Proposal in issue: [#2239](https://github.com/informalsystems/ibc-rs/issues/2239)