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

hermes query channels --chain chain_id to output the counterparty chain_id #2429

Closed
5 tasks
AlianBenabdallah opened this issue Jul 19, 2022 · 7 comments · Fixed by #2463
Closed
5 tasks

hermes query channels --chain chain_id to output the counterparty chain_id #2429

AlianBenabdallah opened this issue Jul 19, 2022 · 7 comments · Fixed by #2463
Labels
I: CLI Internal: related to the relayer's CLI O: new-feature Objective: cause to add a new feature or support O: usability Objective: cause to improve the user experience (UX) and ease using the product
Milestone

Comments

@AlianBenabdallah
Copy link
Contributor

Summary

hermes query channels --chain chain_id currently retrieves all the channel_ids and associated ports.

To this information, I think adding the counterparty chain's id would be helpful.

Problem Definition

In order to transfer packets from A to B, if the user does not know which channel should be used, he need to retrieve all the channels on A then for every channel use hermes query channel --chain --channel --port to retrieve information about the counterparty.
In a situation with many chains, it can be exhausting.
I propose to add information about the counterparty in hermes query channels

Proposal

hermes query channels --chain chain_id would produce an output of this format :

Success: [
    PortChannelId {
        channel_id: ChannelId(
            "channel-0",
        ),
        port_id: PortId(
            "transfer",
        ),
        counterparty_id: ChainId(
            "ibc-0",
        ),
    },
    PortChannelId {
        channel_id: ChannelId(
            "channel-1",
        ),
        port_id: PortId(
            "transfer",
        ),
        counterparty_id: ChainId(
            "ibc-1",
        ),
    },
]

Instead of :

Success: [
    PortChannelId {
        channel_id: ChannelId(
            "channel-0",
        ),
        port_id: PortId(
            "transfer",
        ),
    },
    PortChannelId {
        channel_id: ChannelId(
            "channel-1",
        ),
        port_id: PortId(
            "transfer",
        ),
    },
]

Acceptance Criteria

hermes query channels --chain ibc-0 gives all the channels and the counterparty chain's id.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@AlianBenabdallah AlianBenabdallah added O: new-feature Objective: cause to add a new feature or support I: CLI Internal: related to the relayer's CLI O: usability Objective: cause to improve the user experience (UX) and ease using the product labels Jul 19, 2022
@ljoss17
Copy link
Contributor

ljoss17 commented Jul 19, 2022

Hi @AlianBenabdallah I am wondering if this change is necessary as there are two optional flags which allow to retrieve the information required.

  • The --counterparty-chain would allow to filter all the channels going from A to B
  • The --verbose would allow to have a more detailed output

What do you think ?

@AlianBenabdallah
Copy link
Contributor Author

Hi @ljoss17,

I think --verbose produces a very large output which is not always convenient.

I did not know about --counterparty-chain. Could you please share an example of how to use it ? hermes query channels --counterparty-chain ibc-1 --chain ibc-0 outputs an error and I don't find any mention of this flag with hermes query channels --help

@ljoss17
Copy link
Contributor

ljoss17 commented Jul 19, 2022

Indeed the verbose is not ideal for this scenario.

In order to test the --counterparty-chain you need to pull the latest master, this flag will be introduced in the next release. Once you have the version with --counterparty-chain, can run the following commands:

With this gm.toml:

[global]
  add_to_hermes = true
  auto_maintain_config = true
  extra_wallets = 1
  gaiad_binary = "???"
  hdpath = ""
  home_dir = "???"
  ports_start_at = 27040
  validator_mnemonic = ""
  wallet_mnemonic = ""

  [global.hermes]
    binary = "???"
    config = "???"
    log_level = "debug"
    telemetry_enabled = true
    telemetry_host = "127.0.0.1"
    telemetry_port = 3001

[ibc-0]
  ports_start_at = 27000

[ibc-1]
  ports_start_at = 27010

[ibc-2]
  ports_start_at = 27020

Don't forget to setup the correct global gaiad_binary and home_dir, Hermes binary and config.

  1. gm start
  2. gm hermes config
  3. gm hermes keys
  4. Create the channels between the three chains (e.g. gm hermes cc --exec or hermes create channel)

Then if you run hermes query channels --chain ibc-0 you should get:

Success: [
    PortChannelId {
        channel_id: ChannelId(
            "channel-0",
        ),
        port_id: PortId(
            "transfer",
        ),
    },
    PortChannelId {
        channel_id: ChannelId(
            "channel-1",
        ),
        port_id: PortId(
            "transfer",
        ),
    },
]

And if you run hermes query channels --chain ibc-0 --counterparty-chain ibc-1 you will only get:

Success: [
    PortChannelId {
        channel_id: ChannelId(
            "channel-0",
        ),
        port_id: PortId(
            "transfer",
        ),
    },
]

@AlianBenabdallah
Copy link
Contributor Author

Indeed that's convenient. However, this flag applies filtering and I think that usually, filtering should be done on a visible attribute. Also, in some sense, using the command without any flag is not very useful.

@adizere, you have any opinion regarding the output of hermes query channels --chain chain_id ?

@adizere
Copy link
Member

adizere commented Jul 20, 2022

I think the current default behavior of hermes query channels --chain chain_id is fine. Agree that the filtering we do via --counterparty-chain ibc-1 on a non-visible attribute is not ideal, but this is likely not the best place to fix that and not a priority.

For the issue raised here specifically -- about introducing the counterparty chain identifier in each output element -- maybe we can do it by introducing an additional option to this CLI, say --show-counterparty-chain or similar. Please consult ADR 010 to make sure we're consistent with the principles laid in there.

So we would end-up with 3 possible invocations of this CLIm, first two described by Luca above:

  • The --counterparty-chain would allow to filter all the channels going from A to B
  • The --verbose would allow to have a more detailed output

The third:

  • The --show-counterparty-chain option complements the PortChannelId output element by adding another field (called counterparty_chain_id). Similar to your suggestion Ali, we would go from
PortChannelId {
        channel_id: ChannelId(
            "channel-2",
        ),
        port_id: PortId(
            "transfer",
        ),
    },

to

PortChannelCounterpartyId {
        channel_id: ChannelId(
            "channel-2",
        ),
        port_id: PortId(
            "transfer",
        ),
        counterparty_chain_id: ChainId(
            "ibc-1",
        ),
    },

@AlianBenabdallah
Copy link
Contributor Author

This new flag would be a great idea.
In IBC-go, they have a command that shows every channel in a clear and concise way. (rly path list)
I had to use multiple chains to recreate a bug and I wish I had a single command that gives the whole topology of the network.

@romac
Copy link
Member

romac commented Jul 25, 2022

@AlianBenabdallah Do you want to take a stab at it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: CLI Internal: related to the relayer's CLI O: new-feature Objective: cause to add a new feature or support O: usability Objective: cause to improve the user experience (UX) and ease using the product
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants