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 channel create CLI #732

Merged
merged 14 commits into from
Mar 25, 2021
Merged

Hermes channel create CLI #732

merged 14 commits into from
Mar 25, 2021

Conversation

adizere
Copy link
Member

@adizere adizere commented Mar 10, 2021

Closes: #715

Description


For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@adizere adizere mentioned this pull request Mar 10, 2021
5 tasks
@adizere adizere marked this pull request as ready for review March 10, 2021 13:20
@codecov-io
Copy link

codecov-io commented Mar 11, 2021

Codecov Report

Merging #732 (62b0a26) into master (b1b37f5) will increase coverage by 29.9%.
The diff coverage is n/a.

❗ Current head 62b0a26 differs from pull request most recent head b61fb53. Consider uploading reports for the commit b61fb53 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           master    #732      +/-   ##
=========================================
+ Coverage    13.6%   43.5%   +29.9%     
=========================================
  Files          69     159      +90     
  Lines        3752   10488    +6736     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    4572    +4059     
- Misses       2618    5916    +3298     
+ Partials      621       0     -621     
Impacted Files Coverage Δ
...application/ics20_fungible_token_transfer/error.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 0.0% <ø> (ø)
..._transfer/relay_application_logic/send_transfer.rs 0.0% <ø> (ø)
modules/src/events.rs 0.0% <ø> (ø)
modules/src/handler.rs 100.0% <ø> (ø)
modules/src/ics02_client/client_consensus.rs 55.8% <ø> (ø)
modules/src/ics02_client/client_def.rs 33.0% <ø> (ø)
modules/src/ics02_client/client_state.rs 65.1% <ø> (ø)
modules/src/ics02_client/client_type.rs 79.1% <ø> (+31.5%) ⬆️
modules/src/ics02_client/context.rs 100.0% <ø> (ø)
... and 255 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b1f409...b61fb53. Read the comment docs.

modules/src/ics03_connection/connection.rs Outdated Show resolved Hide resolved
modules/src/ics03_connection/connection.rs Show resolved Hide resolved
relayer-cli/src/commands/create/channel.rs Show resolved Hide resolved
relayer-cli/src/commands/create/channel.rs Show resolved Hide resolved
relayer/src/connection.rs Outdated Show resolved Hide resolved
relayer/src/connection.rs Show resolved Hide resolved
relayer/src/connection.rs Outdated Show resolved Hide resolved
relayer/src/connection.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/create/channel.rs Show resolved Hide resolved
relayer-cli/src/commands/create/channel.rs Outdated Show resolved Hide resolved
@adizere
Copy link
Member Author

adizere commented Mar 16, 2021

@ancazamfir @cezarad can you please have a look over this?

Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

The command that is re-using connection (and therefore clients) doesn't work with different client IDs on the two chains. To test you can try to start the two chains then run e2e/run.py -c ... first. This creates client 07-tendermint-0 on ibc-0 and 07-tendermint-1 on ibc-1. It also creates a connection with connection-0 on ibc-0 and connection-1 on ibc-1.
Then you can try hermes create channel ibc-0 --connection-a connection-0 --port-a transfer --port-b transfer to see the failure.
I proposed some changes (see comments) in the obvious place that I found. Seems to run fine with these changes.

let c = Connection {
delay_period: 0, // TODO: Unclear if we should add mechanism to check the delay period
a_side: ConnectionSide {
chain: a_client.src_chain.clone(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
chain: a_client.src_chain.clone(),
chain: b_client.src_chain.clone(),

Copy link
Member Author

Choose a reason for hiding this comment

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

The bug was that I confused the "source" and "destination" chains for a client.

Instead of your solution proposed here where the clients become reversed, I chose a different one where the clients on each side remain the same (side "a" has client "a") and we associate the correct chain to that side (which is the "destination" chain of the client). 659fc43

relayer/src/connection.rs Outdated Show resolved Hide resolved
@romac romac merged commit 1e5ba02 into master Mar 25, 2021
@romac romac deleted the adi/715_channel branch March 25, 2021 13:58
@romac romac mentioned this pull request Apr 6, 2021
15 tasks
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Added channel handshake basic skeleton

* Added alternative invocation

* Better error diagnostic when connection is non-existing

* Changelog

* Retired the 'channel handshake' command

* FMT

* Extended trait bounds in modules. Better eq check

* FMT

* Added a conclude output

* Switched to lower-case help

* Fixed reversed chains in connection side constructor.
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.

Channel create CLI
4 participants