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

Fix the flexible conn id for connection messages #334

Merged
merged 13 commits into from
Oct 22, 2020
Merged

Conversation

ancazamfir
Copy link
Collaborator

Closes: #332

Description

While the message handlers are correct, the encoding for the new connection id fields in Try and Ack connection message domain types should be Option<ConnectionId>
Focused code review and more tests will be done as part https://github.com/informalsystems/ibc-rs/issues/306.


For contributor use:

  • Unit tests written
  • Added test to CI if applicable
  • Updated CHANGELOG_PENDING.md
  • 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 Oct 21, 2020
@ancazamfir ancazamfir added this to the v0.0.5 milestone Oct 21, 2020
@codecov-io
Copy link

codecov-io commented Oct 21, 2020

Codecov Report

Merging #334 into master will increase coverage by 22.9%.
The diff coverage is 64.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    informalsystems/hermes#334      +/-   ##
=========================================
+ Coverage    13.6%   36.6%   +22.9%     
=========================================
  Files          69     127      +58     
  Lines        3752    8200    +4448     
  Branches     1374    2842    +1468     
=========================================
+ Hits          513    3007    +2494     
- Misses       2618    5017    +2399     
+ Partials      621     176     -445     
Impacted Files Coverage Δ
modules/src/events.rs 0.0% <0.0%> (ø)
modules/src/ics02_client/events.rs 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%> (ø)
modules/src/ics26_routing/error.rs 75.0% <0.0%> (ø)
... and 233 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 e3e28b5...96fb534. Read the comment docs.

ancazamfir and others added 4 commits October 22, 2020 13:12
Co-authored-by: Romain Ruetschi <romain@informal.systems>
Co-authored-by: Romain Ruetschi <romain@informal.systems>
@ancazamfir ancazamfir requested a review from romac October 22, 2020 12:52
modules/src/ics03_connection/connection.rs Outdated Show resolved Hide resolved
@ancazamfir ancazamfir requested a review from romac October 22, 2020 13:53
@ancazamfir ancazamfir merged commit bcbefaa into master Oct 22, 2020
@ancazamfir ancazamfir deleted the anca/flexible_id branch October 22, 2020 14:15
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Fix the flexible conn id in Try and Ack for connection

* Avoid cloning in MsgConnectionOpenAck::counterparty_connection_id

* Avoid a (safe) use of unwrap

* Add check for counterparty_connection_id new field in Try handler.

Allow None in Counterparty structure.

* fix fmt

* Make the dest conn id optional in the tx raw cli

* Update changelog

* Update modules/src/ics03_connection/msgs/conn_open_try.rs

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

* Update modules/src/ics03_connection/msgs/conn_open_ack.rs

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

* fix conversions

* simplify more

* review comments

Co-authored-by: Romain Ruetschi <romain@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.

Empty connection id may result in correct result - ConnOpenAck
3 participants