imp(ibc-testkit): generalize `Host` for `MockContext` #1107

rnbguy · 2024-02-29T16:50:17Z

Closes: #1044

Description

This PR introduces TestHost that generalizes Host for MockContext.

An important change in configuring contexts with light-client states is that - now a context is needed to populate the light client data.

Before

let ctx = MockContext::default().with_client_config(
  MockClientConfig::builder()
    .latest_height(client_height)
    .build(),
);

Now

let ctx = MockContext::<MockHost>::default().with_light_client(
  ClientId::default(),
  LightClientState:with_height(client_height)
);

// or

ctx_b =  MockClientConfig::builder()
  .chain_id(chain_id)
  .latest_height(client_height)
  .build();

ctx_a = ctx_a.with_light_client(
    &client_on_a_for_b,
    LightClientBuilder::init()
        .context(&ctx_b)
        .consensus_heights([client_on_a_for_b_height])
        .params(params)
        .build()
)

This way, if we have two contexts ctx_a and ctx_b - we can reuse the blocks/headers from these contexts to generate their corresponding light clients.

TODO:

Add comments in new structs and traits
Review the Host interface and its namings
Rename field/variable names (e.g. host_chain_type)
~~Remove redundant field/variable (e.g. max_history_size)~~
Merge Host with MockContext.
Light client manipulation under RelayerContext trait

Extras:

bda48c2 fixes a failing test and incorrect logic introduced in prev PR
af49053 optimizes the logic introduced in prev PR
0a66c3b revert the change in prev PR

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

codecov · 2024-02-29T17:06:46Z

Codecov Report

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

❗ No coverage uploaded for pull request base (feat/refactor-testkit@3d11dfc). Click here to learn what that means.

❗ Current head 645ad73 differs from pull request most recent head 531c3c9. Consider uploading reports for the commit 531c3c9 to get more accurate results

Files	Patch %	Lines
ibc-testkit/src/testapp/ibc/core/core_ctx.rs	42.19%	311 Missing ⚠️
ibc-testkit/src/testapp/ibc/core/types.rs	94.42%	14 Missing ⚠️
ibc-testkit/src/testapp/ibc/core/client_ctx.rs	95.67%	8 Missing ⚠️
ibc-testkit/src/hosts/tenderminthost.rs	95.94%	6 Missing ⚠️
...stkit/src/testapp/ibc/clients/mock/client_state.rs	78.26%	5 Missing ⚠️
ibc-testkit/src/hosts/mockhost.rs	92.50%	3 Missing ⚠️
ibc-testkit/src/testapp/ibc/clients/mock/header.rs	66.66%	3 Missing ⚠️
ibc-testkit/src/fixtures/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##             feat/refactor-testkit    #1107   +/-   ##
========================================================
  Coverage                         ?   66.67%           
========================================================
  Files                            ?      211           
  Lines                            ?    20818           
  Branches                         ?        0           
========================================================
  Hits                             ?    13881           
  Misses                           ?     6937           
  Partials                         ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rnbguy · 2024-03-01T16:39:57Z

How does this look to bootstrap clients in a context?

ctx_a = BootstrappedContext::init()
    .local_ctx(ctx_a)
    .client_id(&client_on_a_for_b)
    .remote_ctx(&ctx_b)
    .heights(vec![client_on_a_for_b_height])
    .client_params(&params)
    .bootstrap();

of course, only local_ctx and remote_ctx is required - the rest are optional.

rnbguy · 2024-03-05T22:07:55Z

We went for this version,

ctx_a = ctx_a.with_light_client(
    &client_on_a_for_b,
    LightClientBuilder::init()
        .context(&ctx_b)
        .consensus_heights([client_on_a_for_b_height])
        .params(params)
        .build()
)

.consensus_heights() and .params() are optional. .context is not optional. If you don't want to create a new context but want to use a default light client, use the LightClientState method.

LightClientState::<MockHost>::default(); // default light client for MockHost
LightClientState::<MockHost>::with_latest_height(ctx_b_height); // default light client for MockHost with a height

Farhad-Shabani

Great work, Rano! 🚀
I've left a few questions and suggestions, primarily centered around further simplifying abstractions. Though, your PR has already made significant strides.

Farhad-Shabani · 2024-03-07T05:58:18Z

ibc-testkit/src/hosts/tendermint.rs

+
+    fn generate_client_state(
+        &self,
+        latest_block: &Self::Block,


Do we need to pass the entire block info here? Should it specifically be the latest?
or would simply including the target_height as an argument would suffice?

Nice! Note MockHost and TendermintHost don't contain the block data. They are just string Host(String). Rather MockContext<H> wraps around TestHost and passes the block data.

But your suggestion totally makes sense if I implement TestHost on MockContext directly. Let's do that in the next PR when I implement the high-level contexts.

Alright, that makes sense. Can we have a concise list of TODOs? So that we avoid leaving any unfinished work before merging to the main. Appreciate that.

Farhad-Shabani · 2024-03-07T06:00:12Z

ibc-testkit/src/hosts/mod.rs

+    /// The height of the block.
+    fn height(&self) -> Height;
+
+    /// The timestamp of the block.
+    fn timestamp(&self) -> Timestamp;


Could default implementations for these two methods be provided using the Header?
It even might be worthwhile to explore whether merging TestBlock and TestHeader under a single trait. Uncertain if such granularity is necessary.

I wanted to differentiate between TestBlock and TestHeader in the Tendermint context. The Tendermint light client header contains trusted height metadata. Let me think more about this.

Farhad-Shabani · 2024-03-07T06:05:06Z

ibc-testkit/src/testapp/ibc/core/types.rs

    /// Maximum size for the history of the host chain. Any block older than this is pruned.
    pub max_history_size: u64,

    /// The chain of blocks underlying this context. A vector of size up to `max_history_size`
    /// blocks, ascending order by their height (latest block is on the last position).
-    pub history: Vec<HostBlock>,
+    pub history: Vec<H::Block>,

    /// Average time duration between blocks
    pub block_time: Duration,


Should we include these fields (max_history_size, history, block_time) under the host in this PR, or is it planned to address this in a subsequent update?

Great idea! You already suggested above. Let's tackle this in the next PR. In this PR, I focused on generalizing the HostType/HostBlock enum using traits.

Farhad-Shabani · 2024-03-07T06:08:24Z

ibc-testkit/src/testapp/ibc/core/types.rs

-    pub fn with_client_config(mut self, client: MockClientConfig) -> Self {
-        let cs_heights = if client.consensus_state_heights.is_empty() {
-            vec![client.latest_height]
+    pub fn generate_light_client(


Would it be beneficial to add a docstring for this. Does this method generate a client of the counterparty context within the current one, or is it the other way around?

Make sense. Here I create the light client of the local context. This should go under RelayerContext. Let's tackle this in the next PR.

ibc-testkit/src/testapp/ibc/core/types.rs

ibc-testkit/src/utils/mod.rs

ibc-testkit/src/testapp/ibc/core/types.rs

Farhad-Shabani · 2024-03-07T06:26:02Z

ibc-testkit/src/hosts/tendermint.rs

+        &self,
+        height: u64,
+        timestamp: Timestamp,
+        params: &Self::BlockParams,


Suggestion: What if we introduce getter/setter methods for the "params" instead of passing it as an argument? This would entail adding the relevant fields to the Host struct as well. Likewise for the LightClientParams.

Totally see your point. This should be part of Host (or MockContext).

Farhad-Shabani

Let's keep up the good work in the next PR! 🙏🏻

* imp(testkit): Mock IBC context with `basecoin-store` types (#1068) * non empty default CommitmentPrefix * update ibc mock client types * fix tests for updated mock types * generalize MockContextConfig::into * rm clone from MockContext * add with_{client,consensus}_state * add blocks_since in utils.rs * client takes host timestamp by default * refactor relayer context test * fix few tests * add ibc-query and basecoin-store deps * add basecoin-store in mock ibc context * refactor for updated mock ibc context * imp timeout test * public git commit as dep source * fix spelling * update MockClient types * fix failing tests * rm unused utils and deps * fix cargo doc lint * use ibc host paths in mock context * rm redundant curly brackets --------- Co-authored-by: Farhad Shabani <[email protected]> * imp(ibc-testkit): generalize `Host` for `MockContext` (#1107) * fix semantic conflict * happy clippy * fix next_consensus_state impl * update prev_consensus_state impl * use self.block_time over const * use constant timestamp over dynamic now * add new host impls * changes to ibc-testkit * rm deprecated methods * refactor tests * public fields for tm block params * forged client header update test * add todo comment * minor refactor * refactor tests * builder type for light client state * minor refactor * refactor tests * code opt * rename host structs * renamings * refactor tendermint host * mv year_2023 to utils * refactor the new tests * test malicious adjacent header update * fix tests * add changelog * rm unused chain_revision_number * add doc string for year_2023 * rename to query_latest_block * feat(ibc-testkit): revamp `MockContext` with chain capabilities (#1135) * imp: impl ctxs on MockIbcStore * fix: set update_meta whenever build light client * imp: generate_client_state should take latest_height * imp: remove unnecessary latest_client_states method * refactor: migrate host relevant fields/methods * fix: get back validate_self_client * imp: bring context types up under src * rm default for CommitmentPrefix * rm default height and timestamp impl for TestBlock * default MockStore * AnyClient and AnyConsensus state in MockIbcStore * at least one consensus state when bootstrapping light client * revision number in MockIbcStore * advance height in MockIbcStore * sync host and ibc store advance in conext * add host params to build host * update host trait * convenient type generics for host associated types * update MockHost * return existing block header with correct timestamp * update TendermintHost * update MockGenericContext impl * update mock context building * add implied trait bounds * refactor method * rm redundant imports and impl * call advance_block on context * update MockContext tests * rm ClientStateCommon * use HostParams to build a host * rm using max_history_size * update few tests * ignore failing tests * clippy::use_self * clippy::flat_map_option * clippy::cloned_instead_of_copied * clippy::redundant_clone * clippy::redundant_type_annotations * clippy::as_underscore * disable slow testcase * prune old host consensus state * prune host consensus history in fixture * enable ignored test * return client_id in fixture * fix and enable failing test * avoid Arc and Mutex * fix doc build * refactor host trait and impls * into over Self::from * fix msrv * explicit relative or global path for pub use * clippy::map_identity * clippy::inconsistent_struct_constructor * clippy::std_instead_of_core * use commitment_root for block generation * rename consensus_states to host_consensus_states * use basecoin proof specs * update tests * imp: add history() under TestHost * chore: add docstring for some of methods under TestHost * update TestHost trait * update MockHost and TendermintHost * add MockGenericContext::generate_genesis_block * update MockContextConfig * update tests * update remaining tests * rm comments and rename test * rm HostParams --------- Co-authored-by: Farhad Shabani <[email protected]> * imp(ibc-testkit): Tendermint proof verifications via integration test (#1146) * test tmclient proof verification * use ClientStateConfig directly * rm MockClientConfig * use basecoin store proofspec as default * update tests * use merkle storage in MockContext * fix ProofSpecs size * refactor MockIbcStore to perform begin_block and end_block * simpler proof verification test * use ValidationContext::commitment_prefix() * nits * refactor host related trait method * tendermint host client integration test with proof verification * rm raw test * use typed relayer * add todo for channel integration test * core over std * be semantically correct * add comment for TypedRelayer * integration test for all pairs * fix semantic bug * renames * add channel management * channel creation in RelayerContext * add channel creation in integration test * add test for channel close * query client_id from connection and channel * ibc_store_mut * utils functions for packet relay * add packet relay integration test * add comments * optimize integration utils functions * serde feature for integration tests * rm redundant chain_id * sync clock only on a * add comment * imp: place router under MockGenericContext * nit: add docstring for router * nits * rm redundant lint filters * imp: ditch RelayerContext * nit: simplify build_client_update_datagram * refactor integration tests * add doc strings for TypedRelayerOps * doc strings for RelayerContext * mv client_update_ping_pong to tests dir * rename main_store to multi_store * update TestHost trait * update mock and tendermint hosts * update relayer functions * nits * renames and comments * add comments for return values in relayer ops * imp: simplify into_header --------- Co-authored-by: Farhad Shabani <[email protected]> * rm prune_block_till * apply suggestions from code review Co-authored-by: Sean Chen <[email protected]> Signed-off-by: Rano | Ranadeep <[email protected]> * rename context structs * add MockContext and TendermintContext * update with MockContext and TendermintContext * MockContextConfig to TestContextConfig * update comments with new namings * add util methods for StoreGenericTestContext * fix failing client recovery tests * increase default trusting period of MockClientState * fallible ProofSpecs conversion * update with new ClientStateConfig * use ExtClientValidationContext * fix links in doc comments * fix import order * fix semantic conflicts * Convertible over ConsensusStateConverter * rm ConsensusStateConverter * ibc-query as workspace deps * update Convertible * fix failing test * update changelog entry * add comments * rm absolute import * use expect over comment for safety * infallible try_into * update method names * add unused variable names * rm redundant TendermintBlock wrapper type * add comments * update docstrings * rm should never fail comment * update adr * fix compilation * fix failing test * docs: ADR-009 to revamp testing framework (#1157) * draft adr 009 * update adr-009 * apply suggestions from pr review Co-authored-by: Sean Chen <[email protected]> Signed-off-by: Rano | Ranadeep <[email protected]> * use numbers list for sub-proposal titles Co-authored-by: Sean Chen <[email protected]> Signed-off-by: Rano | Ranadeep <[email protected]> * links to validation and execute context Co-authored-by: Sean Chen <[email protected]> Signed-off-by: Rano | Ranadeep <[email protected]> * apply suggestion from pr review * add links to impls * update comment * markdown format * update adr * use TestContext * update comment * apply suggestions from code review Co-authored-by: Sean Chen <[email protected]> Signed-off-by: Rano | Ranadeep <[email protected]> * markdown format * update method names * fix cargo doc error --------- Signed-off-by: Rano | Ranadeep <[email protected]> Co-authored-by: Sean Chen <[email protected]> --------- Signed-off-by: Rano | Ranadeep <[email protected]> Signed-off-by: Rano | Ranadeep <[email protected]> Co-authored-by: Farhad Shabani <[email protected]> Co-authored-by: Sean Chen <[email protected]>

rnbguy mentioned this pull request Feb 29, 2024

Revamp mock testing artifacts in the ibc-testkit #677

Closed

rnbguy force-pushed the rano/testkit/chain-interface branch from ef8201b to feb139a Compare February 29, 2024 17:03

rnbguy requested a review from Farhad-Shabani February 29, 2024 17:14

rnbguy added 10 commits February 29, 2024 18:59

fix semantic conflict

1c8dcc6

happy clippy

fedf772

fix next_consensus_state impl

bda48c2

update prev_consensus_state impl

af49053

use self.block_time over const

0a66c3b

use constant timestamp over dynamic now

18e2005

add new host impls

4a15b79

changes to ibc-testkit

dd4f6ef

rm deprecated methods

f9be068

refactor tests

a2bf476

rnbguy force-pushed the rano/testkit/chain-interface branch from 645ad73 to a2bf476 Compare February 29, 2024 17:59

rnbguy mentioned this pull request Feb 29, 2024

imp(ibc-testkit): revamp mock testing framework #1109

Merged

7 tasks

rnbguy added 4 commits February 29, 2024 21:22

public fields for tm block params

ba5935a

forged client header update test

af2b52a

add todo comment

3f92f17

minor refactor

805f7f1

rnbguy added 4 commits March 1, 2024 18:10

refactor tests

3480c97

builder type for light client state

25c7bb5

minor refactor

720a714

refactor tests

47db1bc

rnbguy added 4 commits March 5, 2024 23:40

code opt

29386bd

rename host structs

efffab0

renamings

9bf774f

refactor tendermint host

e7c1bb3

rnbguy added 4 commits March 6, 2024 18:33

mv year_2023 to utils

fbcb2a5

refactor the new tests

a678b6f

test malicious adjacent header update

79104a1

fix tests

7b46404

rnbguy marked this pull request as ready for review March 6, 2024 18:13

add changelog

4fda8a5

Farhad-Shabani reviewed Mar 7, 2024

View reviewed changes

rnbguy added 3 commits March 7, 2024 11:43

rm unused chain_revision_number

c1b03d9

add doc string for year_2023

1561834

rename to query_latest_block

531c3c9

Farhad-Shabani approved these changes Mar 8, 2024

View reviewed changes

Farhad-Shabani merged commit 74203fb into feat/refactor-testkit Mar 8, 2024
10 checks passed

Farhad-Shabani deleted the rano/testkit/chain-interface branch March 8, 2024 04:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imp(ibc-testkit): generalize `Host` for `MockContext` #1107

imp(ibc-testkit): generalize `Host` for `MockContext` #1107

rnbguy commented Feb 29, 2024 •

edited

Loading

codecov bot commented Feb 29, 2024 •

edited

Loading

rnbguy commented Mar 1, 2024

rnbguy commented Mar 5, 2024 •

edited

Loading

Farhad-Shabani left a comment

Farhad-Shabani Mar 7, 2024

rnbguy Mar 7, 2024

Farhad-Shabani Mar 7, 2024

Farhad-Shabani Mar 7, 2024

rnbguy Mar 7, 2024

Farhad-Shabani Mar 7, 2024

rnbguy Mar 7, 2024

Farhad-Shabani Mar 7, 2024

rnbguy Mar 7, 2024

Farhad-Shabani Mar 7, 2024

rnbguy Mar 7, 2024

Farhad-Shabani left a comment

imp(ibc-testkit): generalize Host for MockContext #1107

imp(ibc-testkit): generalize Host for MockContext #1107

Conversation

rnbguy commented Feb 29, 2024 • edited Loading

Description

PR author checklist:

Reviewer checklist:

codecov bot commented Feb 29, 2024 • edited Loading

Codecov Report

rnbguy commented Mar 1, 2024

rnbguy commented Mar 5, 2024 • edited Loading

Farhad-Shabani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Farhad-Shabani left a comment

Choose a reason for hiding this comment

imp(ibc-testkit): generalize `Host` for `MockContext` #1107

imp(ibc-testkit): generalize `Host` for `MockContext` #1107

rnbguy commented Feb 29, 2024 •

edited

Loading

codecov bot commented Feb 29, 2024 •

edited

Loading

rnbguy commented Mar 5, 2024 •

edited

Loading