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 client update and fix client create CLIs #355

Merged
merged 19 commits into from
Nov 5, 2020

Conversation

ancazamfir
Copy link
Collaborator

@ancazamfir ancazamfir commented Nov 2, 2020

Closes: #277 informalsystems/ibc-rs#103

Description

Here is a summary of the changes:

  • pointing to branch greg/research-json where the tendermint changes for stargate-4 are and tendermint domain types are implemented for the IBC dependencies (validator set, consensus params, etc)
  • removed unneeded serde (de)serializers
  • refactored a bit the conversion between accountId and String. We should change the signer type to String in handlers as any validation and use of this happens before the hanlers are invoked. Code will be simpler and we could remove the address.rs
  • defined domain types for AnyHeader, MsgUpdateAnyClient, ics07 header,
  • update tendermint client_state with stargate-4 required fields and impls
  • implement get_sign_bytes() in Msg trait
  • add update client CLI
  • refactor the Tx CLIs to take typed input, remove Option from positional params.
  • add client message builder to chain impl of cosmos
  • add connection init message builder to chain trait
  • some refactor of chain methods. We should sit down and review what goes in Chain vs impl Chain for CosmosSDK vs impl CosmosSDK. I guess these would move to the ChainHandle but the approach should be the same.
  • some changes in tests and refactor of the different test utils.

TODO:


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.

    Cleanup and tests

    Add query_consensus_params() and fix query_header_at_height() in cosmos
    chain

    Refactor a bit the accnt seq and key-seed code

    Update signed_header.json with valid signed header

    Hardcode cosmos ics23 specs for create client msg

    Remove ics07 messages

    Fix create client params and filling

    Impl domain type for MsgUpdateAnyClient

    Add update client CLI

    Impl domain type for ics02 AnyHeader and ics07 Header

    Add type-url constants

    Make changes to compile

    Use local tendermint-rs based on Greg's branch
@ancazamfir ancazamfir requested a review from romac as a code owner November 2, 2020 11:47
@ancazamfir ancazamfir requested review from adizere and andynog November 2, 2020 11:47
@codecov-io
Copy link

codecov-io commented Nov 3, 2020

Codecov Report

Merging #355 into master will increase coverage by 20.9%.
The diff coverage is 64.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    informalsystems/hermes#355      +/-   ##
=========================================
+ Coverage    13.6%   34.6%   +20.9%     
=========================================
  Files          69     143      +74     
  Lines        3752    9048    +5296     
  Branches     1374    3053    +1679     
=========================================
+ Hits          513    3132    +2619     
- Misses       2618    5676    +3058     
+ Partials      621     240     -381     
Impacted Files Coverage Δ
modules/src/events.rs 0.0% <0.0%> (ø)
modules/src/ics02_client/events.rs 0.0% <ø> (ø)
modules/src/ics02_client/msgs.rs 0.0% <0.0%> (ø)
modules/src/ics02_client/raw.rs 0.0% <0.0%> (ø)
modules/src/ics03_connection/error.rs 13.6% <0.0%> (-19.7%) ⬇️
modules/src/ics04_channel/error.rs 75.0% <ø> (+50.0%) ⬆️
modules/src/ics04_channel/packet.rs 0.0% <0.0%> (ø)
modules/src/ics07_tendermint/client_def.rs 0.0% <0.0%> (ø)
modules/src/ics07_tendermint/error.rs 75.0% <ø> (+75.0%) ⬆️
modules/src/ics18_relayer/error.rs 0.0% <0.0%> (ø)
... and 264 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 5f314e5...50dae06. Read the comment docs.

modules/src/ics02_client/msgs.rs Outdated Show resolved Hide resolved
modules/src/tx_msg.rs Show resolved Hide resolved
relayer/src/chain.rs Outdated Show resolved Hide resolved
relayer/src/tx/client.rs Outdated Show resolved Hide resolved
relayer/src/tx/client.rs Outdated Show resolved Hide resolved
@romac
Copy link
Member

romac commented Nov 3, 2020

@ancazamfir Amazing job by the way :)

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Cool stuff!

Left a few ideas on how we can continue this work + do a bit of cleanup, but this is almost ready.

modules/src/address.rs Show resolved Hide resolved
modules/src/ics03_connection/connection.rs Show resolved Hide resolved
modules/src/ics03_connection/msgs.rs Show resolved Hide resolved
modules/src/ics07_tendermint/client_state.rs Show resolved Hide resolved
modules/src/ics07_tendermint/client_state.rs Outdated Show resolved Hide resolved
modules/src/ics24_host/identifier.rs Show resolved Hide resolved
relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
relayer/src/tx/connection.rs Outdated Show resolved Hide resolved
Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Thanks Anca for the replies & updates. Looks good!

@adizere adizere merged commit 12cb1d6 into master Nov 5, 2020
@adizere adizere deleted the anca/client_clis_new branch November 5, 2020 18:58
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Initial commit of client txs

    Cleanup and tests

    Add query_consensus_params() and fix query_header_at_height() in cosmos
    chain

    Refactor a bit the accnt seq and key-seed code

    Update signed_header.json with valid signed header

    Hardcode cosmos ics23 specs for create client msg

    Remove ics07 messages

    Fix create client params and filling

    Impl domain type for MsgUpdateAnyClient

    Add update client CLI

    Impl domain type for ics02 AnyHeader and ics07 Header

    Add type-url constants

    Make changes to compile

    Use local tendermint-rs based on Greg's branch

* Switch to tendermint repo branch

* restructure and cleanup

* move client message builder to chain

* move conn init message builder to chain

* Implement get_sign_bytes() in Msg trait, add to_any() and type_url()

* cleanup

* add forgotten test_utils

* Better error handling in ibc::address

* Formatting

* Add missing Eq bounds where possible

* rearrange the conn-init positional params and the chain handling

* review comments

* Move the client msg builder out of chain

* Split client msgs

* Move unbonding_period to impl CosmosSDK

* Updated changelog

* Fix for upstream changes (optional version in consensus::Params).

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

Rename parts to part_set_header Relayer CLIs for client update message
4 participants