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

MsgConnOpenTry builder and CLI #388

Merged
merged 15 commits into from
Nov 11, 2020
Merged

MsgConnOpenTry builder and CLI #388

merged 15 commits into from
Nov 11, 2020

Conversation

ancazamfir
Copy link
Collaborator

@ancazamfir ancazamfir commented Nov 9, 2020

Closes: #358

Description

  • new the raw tx conn-try CLI
  • version handling, validation, (de)serialization)
  • merkle proof (de)serialization
  • basic functions to create the ConnTry message that can be used with minimal changes from the relayer
  • changed ConnInit and the client messages builders to allow usage from the relayer
  • add proven_.. queries that return proofs
  • changed some queries to use ICSHeight instead of tendermint defined Height

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.

@ancazamfir ancazamfir self-assigned this Nov 9, 2020
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.

This is great stuff!

Left a few comments, and also ideas to potentially fix later.

modules/src/ics23_commitment/merkle.rs Outdated Show resolved Hide resolved
modules/src/ics23_commitment/merkle.rs Outdated Show resolved Hide resolved
proto/src/prost/tendermint.types.rs Outdated Show resolved Hide resolved
relayer/src/chain/cosmos.rs Outdated Show resolved Hide resolved
relayer/src/error.rs Outdated Show resolved Hide resolved
relayer/src/tx/connection.rs Show resolved Hide resolved
Move temporarily to broadcast_tx_commit() to help with testing.
@codecov-io
Copy link

codecov-io commented Nov 10, 2020

Codecov Report

Merging #388 (d0d5779) into master (b1b37f5) will increase coverage by 21.5%.
The diff coverage is 64.7%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    informalsystems/hermes#388      +/-   ##
=========================================
+ Coverage    13.6%   35.1%   +21.5%     
=========================================
  Files          69     150      +81     
  Lines        3752    9251    +5499     
  Branches     1374    3318    +1944     
=========================================
+ Hits          513    3254    +2741     
- Misses       2618    5771    +3153     
+ Partials      621     226     -395     
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/msgs/acknowledgement.rs 0.0% <0.0%> (ø)
modules/src/ics04_channel/msgs/packet.rs 0.0% <0.0%> (ø)
modules/src/ics04_channel/msgs/timeout.rs 0.0% <0.0%> (ø)
modules/src/ics04_channel/packet.rs 0.0% <0.0%> (ø)
... and 277 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 49129e5...d0d5779. Read the comment docs.

@@ -1,7 +1,6 @@
#![allow(clippy::too_many_arguments)]
use super::channel::{ChannelEnd, Counterparty, Order};

use crate::ics03_connection::connection::validate_version;
Copy link
Member

Choose a reason for hiding this comment

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

Following the recent PR , there may be major conflicts in file modules/src/ics04_channel/msgs.rs, preventing the merge. From what I can tell, the modifications here are no longer relevant, so it's safe to choose the quick fix of leaving this file as it is on the master currently: https://github.com/informalsystems/ibc-rs/blob/master/modules/src/ics04_channel/msgs.rs (choose merging strategy theirs I think).

PS: The changes here are small: basically you're removing any use of the ICS03::validate_version method from what I can tell, and I'm doing the same changes in #391, so we will not lose these fixes.

@ancazamfir ancazamfir merged commit 24db51d into master Nov 11, 2020
@ancazamfir ancazamfir deleted the anca/conn_open_try_tx branch November 11, 2020 12:06
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Initial commit for open try, updates to merkle and version, etc

* add grpc for unbonding period

* Added more version code and tests

* cleanup

* cargo fmt

* Correct checks for destination connection.

Move temporarily to broadcast_tx_commit() to help with testing.

* Remove the tendermint generated files

* review comments

* Just use RawMerkleProof for now and remove the conversion shortcuts

* moved CommitementProof deser/ser

* fix status info and match in build proofs

* forgotten changes after merge
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.

Relayer CLI for MsgConnectionOpenTry
3 participants