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

Ics04 packet recv write_ack ack #737

Merged
merged 21 commits into from
Mar 22, 2021
Merged

Ics04 packet recv write_ack ack #737

merged 21 commits into from
Mar 22, 2021

Conversation

cezarad
Copy link
Contributor

@cezarad cezarad commented Mar 11, 2021 •
edited by adizere
Loading

Closes: cosmos/ibc-rs#85

Description

  • implementation for receive packet, write acknowledgement and ack packet handlers
  • unit test for the handlers
  • update of the Mock accordingly
  • unit tests in ICS26 for send packet and receive packet

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 March 11, 2021 08:48
@adizere adizere mentioned this pull request Mar 11, 2021
Merged
5 tasks
@codecov-io
Copy link

codecov-io commented Mar 11, 2021 •
edited
Loading

Codecov Report

Merging #737 (83cbc9d) into master (b1b37f5) will increase coverage by 32.4%.
The diff coverage is n/a.

❗ Current head 83cbc9d differs from pull request most recent head 62db47b. Consider uploading reports for the commit 62db47b to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           master    informalsystems/hermes#737      +/-   ##
=========================================
+ Coverage    13.6%   46.1%   +32.4%     
=========================================
  Files          69     162      +93     
  Lines        3752   11007    +7255     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    5075    +4562     
- Misses       2618    5932    +3314     
+ Partials      621       0     -621
Impacted Files Coverage Δ
...application/ics20_fungible_token_transfer/error.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 23.0% <ø> (ø)
..._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 100.0% <ø> (ø)
... and 261 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 9e68484...62db47b. Read the comment docs.

romac
romac requested changes Mar 12, 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.

Pushed some minor changes and left a couple comments and a suggestion.

modules/src/ics04_channel/context.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/context.rs Outdated Show resolved Hide resolved
modules/src/ics04_channel/context.rs Show resolved Hide resolved
@adizere adizere removed the modules label Mar 16, 2021
@devashishdxt devashishdxt mentioned this pull request Sep 28, 2022
Closed
5 tasks
adizere added 4 commits March 18, 2021 09:47
@adizere
Added missing timeout_timestamp fields in converstion methods.
5bf6813
@adizere
Typos and extra spaces
d4e21a2
@adizere
Cleanup & made some method names in Ics04 more accurate.
e940c13 
- Renamed Ics4Msg into Ics4ChannelMsg
- Renamed verify_proofs into verify_channel_proofs
- Renamed verify_packet_proofs into verify_packet_recv_proofs
- Renamed Ics04 dispatch method into channel_dispatch
- Optimized imports in several modules of Ics04
@adizere
Added impl for with_height. Fixed recv_packet test
62db47b
adizere
adizere approved these changes Mar 18, 2021
View reviewed changes
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 work Cezara! I just added a few commits with some improvements. I think the PR is ready.

modules/src/ics26_routing/handler.rs
want_pass: true,
},
Test {
name: "Re-Receive packet".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

@adizere adizere requested a review from romac March 18, 2021 10:14
romac
romac approved these changes Mar 18, 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.

Looks good, especially the tests :) I only have a little comment, but we can follow-up on this in a later PR if you want. Approved preemptively just in case.

modules/src/ics04_channel/context.rs
fn host_height(&self) -> Height;

/// Returns the current timestamp of the local chain.
fn host_timestamp(&self) -> u64;
Copy link
Member

@romac romac Mar 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We could introduce a newtype wrapper for the timestamp, here and in other places where we currently use a u64, eg. a pub struct Timestamp { seconds: u64 } in the host module.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/informalsystems/ibc-rs/issues/758

@adizere adizere mentioned this pull request Sep 29, 2022
Closed
5 tasks
@adizere adizere merged commit ab0a9c0 into master Mar 22, 2021
@adizere adizere deleted the ics04_packet_recv branch March 22, 2021 10:13
@livelybug livelybug mentioned this pull request Sep 28, 2022
Closed
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@cezarad @romac @adizere
Ics04 packet recv write_ack ack (informalsystems#737)
32ecc29 
* recv_packet file

* Update client_def.rs

* updates

* packet

* write ack

* update recv host

* receive packet

* acknowledgement

* receipt type

* dispatch message update

* Update acknowledgement.rs

* Remove a few clones and fix some clippy warnings

* Remove a few more clones

* Changelog

* Minor improvements

* Added missing timeout_timestamp fields in converstion methods.

* Typos and extra spaces

* Cleanup & made some method names in Ics04 more accurate.

- Renamed Ics4Msg into Ics4ChannelMsg
- Renamed verify_proofs into verify_channel_proofs
- Renamed verify_packet_proofs into verify_packet_recv_proofs
- Renamed Ics04 dispatch method into channel_dispatch
- Optimized imports in several modules of Ics04

* Added impl for with_height. Fixed recv_packet test

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
Reviewers

@adizere adizere adizere approved these changes

@romac romac romac approved these changes

@ancazamfir ancazamfir Awaiting requested review from ancazamfir

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

Successfully merging this pull request may close these issues.
4 participants
@cezarad @codecov-io @romac @adizere