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

Implement ability to select specific wallet for packet clearing CLI #2252

Merged

Conversation

seanchen1991
Copy link
Contributor

@seanchen1991 seanchen1991 commented May 31, 2022

Closes: #2111

Description

Add --key <KEY> and --counterparty-key <KEY> options for clear packets CLI, just like Hermes provides one for tx ft-transfer -key <KEY>.


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/). Add a definition of what keys are in the "keys" chapter of the Hermes guide.

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

It looks like as part of ADR 10 it was decided that CLI commands that require being able to specify parameters on both the source and destination chains should do so using -host and -reference suffixes.

@adizere pointed out that the original design of this feature comes up a bit short because the clear packet flow sends messages to both a source and destination chain; specifying a single key via the flag doesn't make it clear which chain the key is associated with.

The most straightforward way to address this is to split the proposed key flag into two flags that follow the convention laid out in the ADR. However, --key-host and --key-reference are confusing; --host-key and --reference-key are more appropriate. Though to be honest, "reference key" can also be confusing.

Curious what folks thoughts are on how this question might be best resolved.

@adizere
Copy link
Member

adizere commented Jun 20, 2022

The most straightforward way to address this is to split the proposed key flag into two flags that follow the convention laid out in the ADR. However, --key-host and --key-reference are confusing; --host-key and --reference-key are more appropriate. Though to be honest, "reference key" can also be confusing.

I agree this is not ideal. The "host" + "reference" terminology seems to be useful for client-related queries only.

Just to understand the current situation: We're refactoring (cf ADR 10) the clear packets CLI as follows:

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

This refactor is underway here #2275. Note that whatever changes we introduce in the present PR will likely conflict with that refactor, so we should aim to minimize merge conflicts, but that's a separate topic.

I looked again through the ADR and found that the specifiers we typically use as of ADR 10 are as follows:

Example of specifiers we currently use are host, reference, a, b and counterparty.

If we want to have a specifier to a --key option, the specifier should probably be one of the above, to avoid introducing new ones. So we are left with two options from what I can tell:

  1. --a-key + --b-key or
  2. --key + --counterparty-key.

I doubt option 1 would be ideal here since clear packets CLI has no "a" or "b" options.

Additionally, we should also be consistent with the key name specifiers from ADR 010. So it's more like

  1. --a-key-name + --b-key-name or
  2. --key-name + --counterparty-key-name.

PS. should we update ADR 010 with whichever decision we take? to clearly specify how clear packets looks like.

@seanchen1991
Copy link
Contributor Author

I agree that a and b specifiers in this case are not very helpful. It sounds like going with --key-name and --counterparty-key-name is the only viable option then.

should we update ADR 010 with whichever decision we take? to clearly specify how clear packets looks like.

Agree that the ADR should be updated.

@seanchen1991 seanchen1991 marked this pull request as ready for review June 28, 2022 16:11
@romac romac merged commit 3cb297e into informalsystems:master Jun 29, 2022
@seanchen1991 seanchen1991 deleted the select-specific-wallet-packet-clearing branch June 29, 2022 16:49
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…nformalsystems#2252)

* Pull in upstream changes

* Add `-k` key flag to clear packets command

* Implement Override<Config> for ClearPacketsCmd

* Add changelog entry

* Update guide

* Remove unnecessary import

* Remove short flag option

* Add clap option for `counterparty_key_name` flag

* Attempting to fetch counterparty chain config

* Overwrite dst chain's `key_name` in clear packet flow

* Change a word in a comment

* Make language in guide consistent with CLI output

* Update ADR 010 to reflect changes to `clear packet` command

Co-authored-by: Romain Ruetschi <romain@informal.systems>
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.

Add support for selecting specific wallet for packet clearing CLI
3 participants