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

Introduce LightBlock support for MockContext #390

Merged
merged 18 commits into from
Nov 19, 2020
Merged

Conversation

adizere
Copy link
Member

@adizere adizere commented Nov 9, 2020

Overview

Closes cosmos/ibc-rs#100

Description

This PR introduces two important changes:

1. Module mock_context.rs was expanded

This module was broken down into two separate modules: context.rs with an implementation of MockContext

https://github.com/informalsystems/ibc-rs/blob/110a3d81dc811820a73879fe43ce8a384f435c4a/modules/src/mock/context.rs#L31

and host.rs with various host chain-specific types, e.g., HostBlock

https://github.com/informalsystems/ibc-rs/blob/b9b4667699a2b7f3b2e95d1421389fea800b93c0/modules/src/mock/host.rs#L30-L33

Note that two types of blocks are now possible: mocks or synthetically generated Tendermint blocks (based on testgen).

2. The ibc crate now exports a mocks feature

https://github.com/informalsystems/ibc-rs/blob/b9b4667699a2b7f3b2e95d1421389fea800b93c0/modules/Cargo.toml#L17-L20

This feature grants the relayer access to mock structures, so the relayer can build an in-process mock chain based on MockContext.


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 mentioned this pull request Nov 10, 2020
8 tasks
@codecov-io
Copy link

codecov-io commented Nov 12, 2020

Codecov Report

Merging #390 (7fc7fcb) into master (b1b37f5) will increase coverage by 20.9%.
The diff coverage is 66.6%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    informalsystems/hermes#390      +/-   ##
=========================================
+ Coverage    13.6%   34.6%   +20.9%     
=========================================
  Files          69     155      +86     
  Lines        3752   10456    +6704     
  Branches     1374    3852    +2478     
=========================================
+ Hits          513    3621    +3108     
- Misses       2618    6213    +3595     
- Partials      621     622       +1     
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/chan_open_try.rs 66.8% <ø> (ø)
modules/src/ics04_channel/msgs/recv_packet.rs 0.0% <ø> (ø)
modules/src/ics04_channel/msgs/timeout.rs 0.0% <ø> (ø)
... and 288 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 0409703...7fc7fcb. Read the comment docs.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
.with_client(&client_on_a_for_b, client_on_a_for_b_height);
let mut ctx_b = MockContext::new(
ChainId::new("mockgaia", 1).unwrap(),
HostType::Mock,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
HostType::Mock,
HostType::SynteticTendermint,

It would be good to create different types of contexts in the test. I think there are a few places where the code still needs to be updated, e.g. with_client_parametrized() or store_client_state().

Copy link
Member Author

Choose a reason for hiding this comment

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

Modified context ctx_b to rely on a HostType::SynteticTendermint chain and updated the relevant parts.

Note that some verification functions (like this one) are incomplete, but will add more checks in there as we go along and write new tests.

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.

Very cool! The first time that we have different chains and client types

modules/src/ics18_relayer/utils.rs Outdated Show resolved Hide resolved
modules/src/ics18_relayer/utils.rs Outdated Show resolved Hide resolved
Applied Anca's comments

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
@adizere adizere merged commit 24c784d into master Nov 19, 2020
@adizere adizere deleted the adi/158_tm_mock branch November 19, 2020 14:56
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Bringing changes from the older PR informalsystems#289, first pass.

* Second pass

* Third pass done, finished bringing old changes.

* Updated changelog

* Removed useless test conditional compilation.

* Clippy exceptions to disable dead code warning

* Removing dep on the dev branch

* Update tendermint-testgen dependency

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Prep context to store multiple types of clients. Deleted mock error.

* Moved module mock_client into mock

* Generalized MockClientRecord

* with_client_parametrized done

* Added basic implementation for TM::check_header_and_update_state.

* Apply suggestions from code review

Applied Anca's comments

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

Co-authored-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: Anca Zamfir <zamfiranca@gmail.com>
Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
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.

4 participants