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.

Sign up for GitHub

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

Jump to bottom

Add missing Channel Open/Close handlers #618

Merged
merged 10 commits into from
Feb 10, 2021
Merged

Add missing Channel Open/Close handlers #618

merged 10 commits into from
Feb 10, 2021

Conversation

cezarad
Copy link
Contributor

@cezarad cezarad commented Feb 8, 2021 •
edited by adizere
Loading

Closes: cosmos/ibc-rs#104

Description

  • channel open ack
  • channel open confirm
  • channel close init
  • channel close confirm

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.

@cezarad cezarad requested a review from adizere February 8, 2021 07:21
@romac romac changed the title Channel handles Add missing Channel Open/Close handlers Feb 8, 2021
@ancazamfir
Copy link
Collaborator

Would it be possible to fix cosmos/ibc-rs#92 here also?

romac
romac requested changes Feb 8, 2021
View reviewed changes
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.

Few minor stylistic comments already, will look into semantics a bit later.

modules/src/ics03_connection/connection.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/error.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/error.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/handler/chan_close_confirm.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/handler/chan_close_confirm.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/msgs/chan_close_confirm.rs Show resolved Hide resolved
modules/src/ics04_channel/msgs/chan_close_init.rs Show resolved Hide resolved
modules/src/ics04_channel/msgs/chan_open_ack.rs Show resolved Hide resolved
modules/src/ics04_channel/msgs/chan_open_confirm.rs Show resolved Hide resolved
modules/src/ics26_routing/handler.rs Outdated Show resolved Hide resolved
ancazamfir
ancazamfir reviewed Feb 9, 2021
View reviewed changes
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.

Looks great!! A few minor comments. Also, could you merge master here?

modules/src/ics04_channel/handler/chan_open_confirm.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/handler/chan_open_confirm.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/handler/chan_close_confirm.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/handler/chan_close_confirm.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/handler/chan_close_confirm.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/handler/chan_close_init.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/handler/chan_open_ack.rs Outdated Show resolved Hide resolved
@romac
Copy link
Member

romac commented Feb 9, 2021

@cezarad Great stuff! Can you also please update the CHANGELOG.md file with a link to https://github.com/informalsystems/ibc-rs/issues/316?

@codecov-io
Copy link

codecov-io commented Feb 9, 2021 •
edited
Loading

Codecov Report

Merging #618 (cbb9f89) into master (b1b37f5) will increase coverage by 32.8%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    informalsystems/hermes#618      +/-   ##
=========================================
+ Coverage    13.6%   46.5%   +32.8%     
=========================================
  Files          69     148      +79     
  Lines        3752    9954    +6202     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    4631    +4118     
- Misses       2618    5323    +2705     
+ Partials      621       0     -621
Impacted Files Coverage Δ
modules/src/address.rs 100.0% <ø> (ø)
...application/ics20_fungible_token_transfer/error.rs 0.0% <ø> (ø)
...pplication/ics20_fungible_token_transfer/events.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 0.0% <ø> (ø)
modules/src/events.rs 0.0% <ø> (ø)
modules/src/handler.rs 100.0% <ø> (ø)
modules/src/ics02_client/client_def.rs 48.3% <ø> (ø)
modules/src/ics02_client/client_type.rs 79.1% <ø> (+31.5%) ⬆️
modules/src/ics02_client/context.rs 100.0% <ø> (ø)
modules/src/ics02_client/error.rs 100.0% <ø> (ø)
... and 254 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 2bf5ce0...cbb9f89. Read the comment docs.

@cezarad
Copy link
Contributor Author

cezarad commented Feb 9, 2021

@cezarad Great stuff! Can you also please update the CHANGELOG.md file with a link to cosmos/ibc-rs#104?

Done.

@adizere adizere mentioned this pull request Feb 9, 2021
Closed
@vitorenesduarte vitorenesduarte mentioned this pull request Sep 29, 2022
Closed
5 tasks
ancazamfir
ancazamfir approved these changes Feb 10, 2021
View reviewed changes
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.

Great work Cezara! Thanks!

@cezarad cezarad merged commit b654af3 into master Feb 10, 2021
@cezarad cezarad deleted the chanAck branch February 10, 2021 12:41
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@cezarad @romac
Add missing Channel Open/Close handlers (informalsystems#618)
58fe170 
* ack

* test ics26

* chan confirm

* cargo tests

* Update conn_open_try.rs

* updates reviews

* Update chan_open_ack.rs

* Update changelog

Co-authored-by: Romain Ruetschi <[email protected]>
@ancazamfir ancazamfir mentioned this pull request Sep 29, 2022
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitorenesduarte vitorenesduarte vitorenesduarte left review comments

@ancazamfir ancazamfir ancazamfir approved these changes

@romac romac romac approved these changes

@adizere adizere Awaiting requested review from adizere

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cezarad @ancazamfir @romac @codecov-io @vitorenesduarte