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

Model-based tests for ICS02 #601

Merged
merged 57 commits into from
Feb 12, 2021
Merged

Model-based tests for ICS02 #601

merged 57 commits into from
Feb 12, 2021

Conversation

vitorenesduarte
Copy link
Contributor

@vitorenesduarte vitorenesduarte commented Feb 2, 2021 •
edited
Loading

Towards: cosmos/ibc-rs#30

Description

This PR is the first step towards cosmos/ibc-rs#30. For ease of reviewing, we plan to introduce one ICS at a time, starting now with ICS02.

For some details on MBT and current limitations, please check modules/tests/README.md (which steals ideas from the light-client one).

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.

romac
romac reviewed Feb 3, 2021
View reviewed changes
modules/Cargo.toml Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 3, 2021 •
edited
Loading

Codecov Report

Merging #601 (3a991e7) into master (b1b37f5) will increase coverage by 33.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    informalsystems/hermes#601      +/-   ##
=========================================
+ Coverage    13.6%   46.8%   +33.1%     
=========================================
  Files          69     150      +81     
  Lines        3752   10091    +6339     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    4725    +4212     
- Misses       2618    5366    +2748     
+ 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 258 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 b654af3...3a991e7. Read the comment docs.

@vitorenesduarte vitorenesduarte changed the title MBT for ICS02 MBT for ICS02 and ICS03 Feb 4, 2021
@vitorenesduarte vitorenesduarte marked this pull request as ready for review February 9, 2021 09:51
adizere
adizere requested changes Feb 9, 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.

Impressive work!

My main worry is that we're introducing new TLA+ specs here, instead of leveraging (part of) what we already had. I'm curious what @ancazamfir, @andrey-kuprianov, and @istoilkovska think about this: would it be possible, at least eventually, to reuse Ilina's TLA+ specs for MBT?

modules/tests/modelator.rs Outdated Show resolved Hide resolved
modules/src/ics02_client/msgs/create_client.rs Show resolved Hide resolved
modules/tests/model_based.rs Outdated Show resolved Hide resolved
modules/tests/model_based.rs Outdated Show resolved Hide resolved
modules/tests/modelator.rs Outdated Show resolved Hide resolved
modules/tests/support/model_based/IBC.cfg Outdated Show resolved Hide resolved
modules/tests/support/model_based/IBCTests.cfg Show resolved Hide resolved
modules/tests/support/model_based/README.md Outdated Show resolved Hide resolved
@vitorenesduarte
Copy link
Contributor Author

Thanks @adizere and @romac for the review! I just finished addressing all your comments.

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 good after a fist pass. Entered a few comments, will take another look.

modules/tests/model_based.rs Show resolved Hide resolved
modules/tests/model_based.rs Show resolved Hide resolved
modules/tests/model_based.rs Show resolved Hide resolved
modules/tests/step.rs Show resolved Hide resolved
This was referenced Feb 9, 2021
Closed
Closed
Merged
@vitorenesduarte vitorenesduarte mentioned this pull request Feb 10, 2021
Closed
5 tasks
@ancazamfir
Copy link
Collaborator

Let's merge this one and address any pending comments in #633.

adizere
adizere approved these changes Feb 12, 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.

Good work Vitor!

@adizere adizere merged commit 214681c into master Feb 12, 2021
@adizere adizere deleted the vitor/ics02_mbt branch February 12, 2021 09:07
@adizere adizere mentioned this pull request Feb 12, 2021
Closed
5 tasks
@vitorenesduarte vitorenesduarte mentioned this pull request Feb 12, 2021
Merged
5 tasks
@vitorenesduarte vitorenesduarte mentioned this pull request Feb 22, 2021
Merged
5 tasks
@adizere adizere mentioned this pull request Sep 28, 2022
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrey-kuprianov andrey-kuprianov andrey-kuprianov left review comments

@romac romac romac left review comments

@ancazamfir ancazamfir ancazamfir approved these changes

@adizere adizere adizere approved these changes

@cezarad cezarad Awaiting requested review from cezarad

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

Successfully merging this pull request may close these issues.
6 participants
@vitorenesduarte @codecov-io @ancazamfir @romac @adizere @andrey-kuprianov