cosmos / ibc-rs

Rust implementation of the Inter-Blockchain Communication (IBC) protocol.
Apache License 2.0
182 stars 74 forks source link

feat(ibc-testkit): revamp `MockContext` with chain capabilities #1135

Closed rnbguy closed 3 months ago

rnbguy commented 3 months ago

Closes: #1045

Description


PR author checklist:

Reviewer checklist:

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 89.28571% with 69 lines in your changes are missing coverage. Please review.

Project coverage is 67.76%. Comparing base (cb6ca8b) to head (72c2a0e).

Files Patch % Lines
ibc-testkit/src/context.rs 92.93% 25 Missing :warning:
ibc-testkit/src/testapp/ibc/core/core_ctx.rs 57.14% 21 Missing :warning:
ibc-testkit/src/hosts/mock.rs 71.42% 6 Missing :warning:
ibc-testkit/src/hosts/tendermint.rs 86.04% 6 Missing :warning:
ibc-testkit/src/hosts/mod.rs 91.11% 4 Missing :warning:
ibc-testkit/src/testapp/ibc/core/types.rs 89.74% 4 Missing :warning:
ibc-testkit/src/fixtures/core/context.rs 94.44% 2 Missing :warning:
ibc-testkit/src/testapp/ibc/clients/mod.rs 91.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## feat/refactor-testkit #1135 +/- ## ========================================================= + Coverage 66.71% 67.76% +1.04% ========================================================= Files 213 213 Lines 20800 20108 -692 ========================================================= - Hits 13877 13626 -251 + Misses 6923 6482 -441 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rnbguy commented 3 months ago

I managed to write a minimal proof verification test with Tendermint light client. For now, it uses the ProofSpecs of basecoin_store.

To keep the proof retrieval minimal, I had to modify the ProvableContext::get_proof of MockIbcStore. Before it returned the encoded bytes of a singleCommitmentProof. Now it returns the encoded bytes of MerkleProofs, which is essentially a list of CommitmentProofs corresponding to a MerklePath.

Two shortcomings of basecoin_store:


Ignore this comment - as the changes related to this will be processed in a separated PR.

rnbguy commented 3 months ago

That was a good suggestion. While doing that, I realized that the chain ID (and revision number) are not that relevant for our test cases. We can actually totally remove the chain_ids and use fixed zero as a default revision number. It is only needed to test client upgrades.

I still kept it as some of the tests are using Height::new(1, <height>). Let's refactor the following in future PR: