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 CLI for upgrading client #723

Merged
merged 33 commits into from
Mar 23, 2021
Merged

Hermes CLI for upgrading client #723

merged 33 commits into from
Mar 23, 2021

Conversation

adizere
Copy link
Member

@adizere adizere commented Mar 3, 2021

Closes: #357
Closes: #734

Description

  • added a new CLI
    • hermes tx raw upgrade-client ibc-1 ibc-0 07-tendermint-1
  • testing instructions for the CLI can be found in this guide

Additional notes:

  • added the corresponding proto file: proto/src/prost/cosmos.upgrade.v1beta1.rs
  • also added the proto-compiler Cargo.lock file (note: this file account for over 1K new lines of code, so the PR is not as big as it seems)
    • also mentioned in proto-compiler that we should use the locked flag so that the binary is identical every time we re-generate these files;
    • this is important to avoid having aesthetic differences in the generated proto files (for example pub log: std::string::String, vs pub log: ::prost::alloc::string::String,)
  • added some stubs for the upgrade client handler, but the rest will be handled in a separate issue (Handler for upgrade client #722)
  • also cleaned up some rust doc

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 changed the title Herms CLI for upgrading client Hermes CLI for upgrading client Mar 3, 2021
@adizere adizere marked this pull request as ready for review March 9, 2021 13:36
@@ -63,9 +63,7 @@ mkdir -p "$GAIA_DATA" && cd "$GAIA_DATA" && cd ../
ONE_CHAIN="$(dirname "$0")/one-chain"

CHAIN_0_RPC_PORT=26657
CHAIN_0_RPC_ADDR="localhost:$CHAIN_0_RPC_PORT"
Copy link
Member Author

Choose a reason for hiding this comment

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

Was not used.

@@ -133,5 +133,7 @@ $BINARY --home $CHAIN_DIR/$CHAIN_ID start --pruning=nothing --grpc.address="0.0.
# Show validator's and user's balance
sleep 3
RPC_ADDR="tcp://localhost:$RPC_PORT"
echo "Balances for validator '$VALIDATOR' @ '$RPC_ADDR'"
Copy link
Member Author

Choose a reason for hiding this comment

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

Made it more explicit as it wasn't clear from output which was user <> validator.

@codecov-io
Copy link

codecov-io commented Mar 10, 2021

Codecov Report

Merging #723 (4187c40) into master (b1b37f5) will increase coverage by 31.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #723      +/-   ##
=========================================
+ Coverage    13.6%   44.8%   +31.1%     
=========================================
  Files          69     166      +97     
  Lines        3752   11318    +7566     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    5078    +4565     
- Misses       2618    6240    +3622     
+ Partials      621       0     -621     
Impacted Files Coverage Δ
...application/ics20_fungible_token_transfer/error.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 22.2% <ø> (ø)
..._transfer/relay_application_logic/send_transfer.rs 85.7% <ø> (ø)
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 32.3% <ø> (ø)
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 90.4% <ø> (ø)
... and 266 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 c2ba84b...4187c40. Read the comment docs.

guide/src/upgrade_test.md Outdated Show resolved Hide resolved
modules/src/ics02_client/events.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/events.rs Show resolved Hide resolved
modules/src/ics24_host/path.rs Outdated Show resolved Hide resolved
scripts/init-clients Show resolved Hide resolved
@adizere
Copy link
Member Author

adizere commented Mar 16, 2021

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

@ancazamfir
Copy link
Collaborator

FYI, I added a new command upgrade-chain in 42b326a to help with the client upgrade testing. Also removed the patches and dependencies on the Go relayer. Updated the instructions.

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.

Wow, thanks Adi, we are almost done with this! I have a couple of comments on the CLI and UX.

required,
help = "identifier of the client to be upgraded on destination chain"
)]
dst_client_id: ClientId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should make dst_client_id optional and if not entered to upgrade all clients of src_chain_id present on dst_chain_id. The way it is, with a lot of clients, the user needs to query all clients and issue the upgrade for each client.
Also, we should have a hermes client upgrade.. command in place of or in addition to this command.

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to do this in this PR or a follow-up one? If the latter, then we should open an issue to track this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Follow up PR, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

@ancazamfir ancazamfir requested a review from romac March 23, 2021 18:16
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Awesome, pushed a couple commits for nitpicks and left a comment about a TODO, but otherwise it's all good!

relayer/src/upgrade_chain.rs Outdated Show resolved Hide resolved
@romac romac merged commit 5b1f409 into master Mar 23, 2021
@romac romac deleted the adi/357_upgrade branch March 23, 2021 20:19
@adizere adizere mentioned this pull request Mar 24, 2021
8 tasks
@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 domain type def.

Also cleaned-up the documentation, which looked very sloppy.
https://docs.rs/ibc/0.1.1/ibc/ics02_client/msgs/index.html

* Added partial handler & command

* Added upgrade proto files. Added Cargo.lock for proto-compiler

* Prep for query_upgraded_client_state

* Method & support for querying ugpraded client state

* Support for querying the upgraded consensus state

* Minor dev scripts enhancements & bugs

* Added guide. Uses patched Go relayer

* Proto conversion for upgrade msg. Refactored upgrade() impl

* Fix missing signer bug

* Update msg bf. upgrade. Event parsing

* Changelog. Revised guide

* Aesthetic nits based on file review

* Clarifications in the test instructions

* Documented Go relayer version in testing instructions

* Possible fix for informalsystems#734

* changelog & method documentation

* Apply suggestions from code review

Co-authored-by: Romain Ruetschi <romain@informal.systems>

* Added more derived trait bounds on module events

* Adapt to newer Ics02 structure.

* Added Protobuf impl for MsgUpgradeAnyClient

* FMT

* Added upgrade-chain CLI and updated instructions

* Remove a clone and turn zero_custom_fields into a static method

* Whitespace and nitpick

* Remove obsolete TODO

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Co-authored-by: Anca Zamfir <zamfiranca@gmail.com>
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.

Client update fails if the client is already up-to-date Relayer CLI for client upgrade message
4 participants