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

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

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.

modules/Cargo.toml Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 3, 2021

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
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/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/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.

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
@ancazamfir ancazamfir requested a review from adizere February 12, 2021 08:20
@ancazamfir
Copy link
Collaborator

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

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
@vitorenesduarte vitorenesduarte mentioned this pull request Feb 22, 2021
5 tasks
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.

6 participants