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

ICS 4 Domain Types for channel handshakes #391

Merged
merged 15 commits into from
Nov 12, 2020
Merged

Conversation

adizere
Copy link
Member

@adizere adizere commented Nov 10, 2020

Closes: cosmos/ibc-rs#105

Description


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.

@adizere adizere changed the title ICS 4 types ICS 4: Domain Types Nov 11, 2020
@adizere adizere changed the base branch from master to adi/315_ics4_types_split November 11, 2020 07:16
@adizere adizere mentioned this pull request Nov 11, 2020
5 tasks
Base automatically changed from adi/315_ics4_types_split to master November 11, 2020 11:07
@adizere adizere mentioned this pull request Nov 11, 2020
5 tasks
@codecov-io
Copy link

codecov-io commented Nov 11, 2020

Codecov Report

Merging #391 (a1e9188) into master (b1b37f5) will increase coverage by 22.1%.
The diff coverage is 64.2%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    informalsystems/hermes#391      +/-   ##
=========================================
+ Coverage    13.6%   35.8%   +22.1%     
=========================================
  Files          69     150      +81     
  Lines        3752    9499    +5747     
  Branches     1374    3424    +2050     
=========================================
+ Hits          513    3403    +2890     
- Misses       2618    5860    +3242     
+ Partials      621     236     -385     
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 279 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 24db51d...a45c401. Read the comment docs.

@adizere adizere changed the title ICS 4: Domain Types ICS 4 Channel lifecycle Domain Types Nov 11, 2020
@adizere adizere changed the title ICS 4 Channel lifecycle Domain Types ICS 4 Domain Types for channel handshakes Nov 11, 2020
@adizere adizere marked this pull request as ready for review November 11, 2020 14:44
@ancazamfir
Copy link
Collaborator

ancazamfir commented Nov 12, 2020

Looking at this now and wondering why we need impl DomainType<RawMyType> for <MyType> for many of these. Just the TryFrom/From would be enough. The code compiles and runs fine if we remove all except the cases where RawMyType is not a prost message but wire bytes. For example the client state that is stuffed into an Any type as Vec, or merkle proofs.

(Note - the question is general not only for this PR. We have this pattern everywhere in the code)

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 good, thanks Adi! Let's merge this and we'll make more changes as we implement the handlers (making use of the deserializer/ TryFrom part) and the relayer (used the serializer/ From part).

@adizere adizere merged commit 3abb563 into master Nov 12, 2020
@adizere adizere deleted the adi/315_ics4_types branch November 12, 2020 10:20
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* First step towards #315

* Domain type & tests for chan_open_init.

* Fix FMT

* WIP ChannelOpenTry Domain type & tests

* Re-adding ConnectionMsgType (accidentally removed).

* Done with OpenTry msg

* Finished ChanOpenAck domain type + tests; also fmt fix.

* Finished ChanOpenConfirm message

* ChanCloseInit done

* FMT fix

* Removed unnecessary derives. Close Confirm done

* Rename packet into RecvPacket. Almost done.

* Updated changelog
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.

3 participants