From 43cea1020eecb0f70d99a6fc94b83990d69c660f Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 4 Mar 2024 17:51:03 +0100 Subject: [PATCH 01/16] draft adr 009 --- docs/architecture/adr-009-revamp-testkit.md | 167 ++++++++++++++++++++ 1 file changed, 167 insertions(+) create mode 100644 docs/architecture/adr-009-revamp-testkit.md diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md new file mode 100644 index 000000000..15f8115c8 --- /dev/null +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -0,0 +1,167 @@ +# ADR 009: Revamp IBC integration test framework + +## Changelog + +- 04-03-2024: Initial draft + +## Context + +The current framework in the IBC testkit uses +[existing types and injects state or dependency manually][1]. Sometimes, it uses +[semantically wrong data as mock data][2]. Because of this, tests with +customizable steps and fixed fixtures became messy and unmaintainable. + +To overcome this, we need to create a proper integration test framework that +allows: + +- writing and maintaining tests easily. +- yet covering various success and failure scenarios. + +[1]: https://github.com/cosmos/ibc-rs/blob/65d84464842b3620f0bd66a07af30705bfa37761/ibc-testkit/tests/core/ics02_client/update_client.rs#L572-L576 +[2]: https://github.com/cosmos/ibc-rs/blob/65d84464842b3620f0bd66a07af30705bfa37761/ibc-testkit/src/testapp/ibc/core/types.rs#L320 + +## Decision + +### Principle + +The main goal of this proposal is that writing and maintaining tests should be +easier. The happy tests should be succinct and readable. The happy steps may be +reused as fixtures - but not the sad steps. Because the sad tests should be +descriptive and contain comments about the test steps. (Also, if there are more +than 10 steps, it should be broken down into smaller tests or reuse fixtures) + +### Testing traits + +Moreover, the test framework should allow testing the implementations of the +traits. Namely, there are two sets of crucial traits - one for different kinds +of clients and the other for different IBC apps. + +Testing different implementations of clients is possible using a general `Host` +and implementing its corresponding client traits. This is already done by +[`Mock` and `SyntheticTendermint`][3]. + +[3]: https://github.com/cosmos/ibc-rs/blob/65d84464842b3620f0bd66a07af30705bfa37761/ibc-testkit/src/hosts/block.rs#L32-L36 + +In the Tendermint client, we still don't test membership checks as we are not +using a Merkle tree in our test framework. In the Mock client, we return success +for these checks. This allowed us to write tests for Connection, Channel and +Packets but clearly, they don't cover the failure cases. + +For the IBC apps, we are using existing `ics-20` and `ics-721`. But we may add a +mock one to cover more scenarios. + +### Architecture + +To revamp the test framework, we need to unify the existing methods, traits and +structs using a clear structure. The following is the proposed structure: + +```rs +struct Chain { + // host meta data + host: H, + + // host chain history + blocks: Vec, + + // chain storage with commitment vector support + store: S, + + // deployed ibc module or contract + ibc: Ibc +} + +impl Chain { + fn new(chain_id: H::ChainId) -> Self; + + fn dispatch(msg: MsgEnvelope) -> Result<(), Error> { + dispatch(self.ibc.context, self.ibc.router, msg) + } + ... + + fn advance_height(&self) { + self.store.commit(); + let root_hash = self.store.root_hash(); + self.blocks.push(self.host.generate_block(root_hash)); + } +} + +struct Relayer; + +impl Relayer { + fn bootstrap_client(ctx_a: Chain, ctx_b: Chain); + + fn bootstrap_connection(ctx: Chain, client_id: String); + fn bootstrap_channel(ctx: Chain, connection_id: String); + + + fn client_create_msg(ctx: Chain, height: Height) -> MsgEnvelope; + fn client_update_msg(ctx: Chain, height: Height) -> MsgEnvelope; + .. + + fn connection_open_init_msg(ctx: Chain, client_id: String) -> MsgEnvelope; + // uses IBC events + fn connection_open_try_msg(ctx_a: Chain, ctx_b: Chain) -> MsgEnvelope; + .. + + // similarly for channels and packets +} + +struct Ibc { + context: IbcStore, + router: R +} + +pub trait Host { + type Block; + fn chain_id(&self) -> String; + fn generate_block(&self, hash: Vec) -> Self::Block; + fn generate_client_state(&self) -> Self::Block; + fn generate_consensus_state(&self) -> Self::Block; +} + +pub trait Store { + type Error; + fn get(&self, key: &[u8]) -> Result, Self::Error>; + fn set(&mut self, key: &[u8], value: Vec) -> Result, Self::Error> + fn commit(&mut self) -> Result<(), Self::Error>; + fn root_hash(&self) -> Result, Self::Error>; + fn get_proof(&self, key: &[u8]) -> Result, Self::Error>; +} +``` + +The idea is to maintain multiple `Chain` instances and use `Relayer` to +facilitate the IBC among them. The `Chain` uses `blocks` and `store` to model +chain history and storage. `Chain::advance_height` is used to commit changes to +store and generate a new block with a new height and commitment root. +`Chain::dispatch` is used to submit messages to the IBC module and make changes +to the storage. + +We should use emitted IBC events and log messages in our tests too. We should +have utility functions to parse and assert these in our tests. + +## Status + +Proposed + +## Consequences + +This ADR pays the technical debt of the existing testing framework. + +### Positive + +The future tests will be more readable and maintainable. The test framework +becomes modular and heavily leverages Rust's trait system. Even the `ibc-rs` +users may benefit from this framework to test their implementations of `ibc-rs` +traits. + +### Negative + +This requires a significant refactoring of the existing tests. Since this may +take some time, the parallel development on `main` branch may conflict with this +work. + +## Future + +The IBC standard is being implemented in different languages. We should +investigate a DSL to write the IBC test scenarios. This way we add portability +to our tests and check compatibility with other implementations, such as ibc-go. From 6152e82881052210d16b5f91cf01d06b08341dad Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Tue, 2 Apr 2024 15:05:01 +0200 Subject: [PATCH 02/16] update adr-009 --- docs/architecture/adr-009-revamp-testkit.md | 310 +++++++++++++------- 1 file changed, 211 insertions(+), 99 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index 15f8115c8..91d221333 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -7,137 +7,250 @@ ## Context The current framework in the IBC testkit uses -[existing types and injects state or dependency manually][1]. Sometimes, it uses -[semantically wrong data as mock data][2]. Because of this, tests with -customizable steps and fixed fixtures became messy and unmaintainable. +[existing types and injects state or dependency +manually](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/tests/core/ics02_client/update_client.rs#L574-L578). +Sometimes, it uses +[semantically wrong data as mock data](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L320). +Because of this, tests with customizable steps and fixed fixtures became ad-hoc +and unmaintainable. -To overcome this, we need to create a proper integration test framework that -allows: +To overcome this, we need to improve our test framework that allows: -- writing and maintaining tests easily. -- yet covering various success and failure scenarios. - -[1]: https://github.com/cosmos/ibc-rs/blob/65d84464842b3620f0bd66a07af30705bfa37761/ibc-testkit/tests/core/ics02_client/update_client.rs#L572-L576 -[2]: https://github.com/cosmos/ibc-rs/blob/65d84464842b3620f0bd66a07af30705bfa37761/ibc-testkit/src/testapp/ibc/core/types.rs#L320 +- testing different implementations (traits). +- succinct tests (useful `util` methods). +- improving test coverage (Merkle proof generation). +- integration tests exercising the IBC workflow (relayer-like interface) ## Decision -### Principle - -The main goal of this proposal is that writing and maintaining tests should be -easier. The happy tests should be succinct and readable. The happy steps may be -reused as fixtures - but not the sad steps. Because the sad tests should be -descriptive and contain comments about the test steps. (Also, if there are more -than 10 steps, it should be broken down into smaller tests or reuse fixtures) +The main goal of this proposal is to create a test framework that is modular and +closer to the real blockchain environment. This should also make the existing +tests succinct and readable. Instead of bootstrapping the mock data, we should +use valid steps to generate it - so that we know the exact steps to reach a +state to reproduce in a real environment. -### Testing traits +To achieve this, we have broken down the proposal into sub-proposals. -Moreover, the test framework should allow testing the implementations of the -traits. Namely, there are two sets of crucial traits - one for different kinds -of clients and the other for different IBC apps. +### Adopt a Merkle store for the test framework -Testing different implementations of clients is possible using a general `Host` -and implementing its corresponding client traits. This is already done by -[`Mock` and `SyntheticTendermint`][3]. +The current framework uses `HashMap` and `HashSet` to store data. This works for +many test scenarios, but it fails to test proof-sensitive scenarios. Because of +this, we don't have any connection, channel handshake or packet relay tests for +Tendermint light client. -[3]: https://github.com/cosmos/ibc-rs/blob/65d84464842b3620f0bd66a07af30705bfa37761/ibc-testkit/src/hosts/block.rs#L32-L36 +We generalize +[`MockContext`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L103) +to use a Merkle store which is used for IBC Context's store. For concrete or +default implementation, we can use the IAVL Merkle store implementation from +`informalsystems/basecoin-rs`. -In the Tendermint client, we still don't test membership checks as we are not -using a Merkle tree in our test framework. In the Mock client, we return success -for these checks. This allowed us to write tests for Connection, Channel and -Packets but clearly, they don't cover the failure cases. +### Modularize the host environment -For the IBC apps, we are using existing `ics-20` and `ics-721`. But we may add a -mock one to cover more scenarios. +Currently, we are using `Mock` and `SyntheticTendermint` +[variants](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L33-L36) +as the host environments. To manage these two different environments, we also +introduced +[`HostBlocks`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L72-75) +for corresponding host variants. -### Architecture +This creates friction if we have to add a new host variant. It creates the same +problem as why we have `ibc-derive` crate for `ClientState` and +`ConsensusState`. This should be refactored by a generic `TestHost` trait that +maintains its corresponding types e.g. `Block` types, via associated types. +Finally, we generalize the `MockContext` once more to use this `TestHost` trait +instead of a concrete enum variant. -To revamp the test framework, we need to unify the existing methods, traits and -structs using a clear structure. The following is the proposed structure: +This `TestHost` trait should be responsible for generating blocks, headers, +client and consensus states specific to that host environment. ```rs -struct Chain { - // host meta data - host: H, +/// TestHost is a trait that defines the interface for a host blockchain. +pub trait TestHost: Default + Debug + Sized { + /// The type of block produced by the host. + type Block: TestBlock; + + /// The type of client state produced by the host. + type ClientState: Into + Debug; + + /// The type of block parameters to produce a block + type BlockParams: Debug + Default; + + /// The type of light client parameters to produce a light client state + type LightClientParams: Debug + Default; + + /// The history of blocks produced by the host chain. + fn history(&self) -> &VecDeque; + + /// Triggers the advancing of the host chain, by extending the history of blocks (or headers). + fn advance_block( + &mut self, + commitment_root: Vec, + block_time: Duration, + params: &Self::BlockParams, + ); + + /// Generate a block at the given height and timestamp, using the provided parameters. + fn generate_block( + &self, + commitment_root: Vec, + height: u64, + timestamp: Timestamp, + params: &Self::BlockParams, + ) -> Self::Block; + + /// Generate a client state using the block at the given height and the provided parameters. + fn generate_client_state( + &self, + latest_height: &Height, + params: &Self::LightClientParams, + ) -> Self::ClientState; +} + +/// TestBlock is a trait that defines the interface for a block produced by a host blockchain. +pub trait TestBlock: Clone + Debug { + /// The type of header can be extracted from the block. + type Header: TestHeader; - // host chain history - blocks: Vec, + /// The height of the block. + fn height(&self) -> Height; - // chain storage with commitment vector support - store: S, + /// The timestamp of the block. + fn timestamp(&self) -> Timestamp; - // deployed ibc module or contract - ibc: Ibc + /// Extract the header from the block. + fn into_header(self) -> Self::Header; } -impl Chain { - fn new(chain_id: H::ChainId) -> Self; +/// TestHeader is a trait that defines the interface for a header produced by a host blockchain. +pub trait TestHeader: Clone + Debug + Into { + /// The type of consensus state can be extracted from the header. + type ConsensusState: ConsensusState + Into + From + Clone + Debug; - fn dispatch(msg: MsgEnvelope) -> Result<(), Error> { - dispatch(self.ibc.context, self.ibc.router, msg) - } - ... + /// The height of the block, as recorded in the header. + fn height(&self) -> Height; + + /// The timestamp of the block, as recorded in the header. + fn timestamp(&self) -> Timestamp; - fn advance_height(&self) { - self.store.commit(); - let root_hash = self.store.root_hash(); - self.blocks.push(self.host.generate_block(root_hash)); - } + /// Extract the consensus state from the header. + fn into_consensus_state(self) -> Self::ConsensusState; } +``` -struct Relayer; +### Decoupling IbcContext and Host environment -impl Relayer { - fn bootstrap_client(ctx_a: Chain, ctx_b: Chain); +Currently, `MockContext` implements the top validation and execution context of +`ibc-rs`. It contains other host-specific data e.g. `host_chain_id`, +`block_time` - that are irrelevant to the IBC context directly. If we think +`MockContext` as a real blockchain context, the `MockContext` represents the top +runtime - which contains the IBC module. So we implement the validation and +execution context on `MockIbcStore`, instead of `MockContext`. - fn bootstrap_connection(ctx: Chain, client_id: String); - fn bootstrap_channel(ctx: Chain, connection_id: String); +With this, the `MockContext` contains two decoupled parts - the host and the IBC +module. +### Chain-like interface for `MockContext` - fn client_create_msg(ctx: Chain, height: Height) -> MsgEnvelope; - fn client_update_msg(ctx: Chain, height: Height) -> MsgEnvelope; - .. +With the above changes, we can refactor the `MockContext` to have +blockchain-like interfaces. - fn connection_open_init_msg(ctx: Chain, client_id: String) -> MsgEnvelope; - // uses IBC events - fn connection_open_try_msg(ctx_a: Chain, ctx_b: Chain) -> MsgEnvelope; - .. +The `MockContext` should have `end_block`, `produce_block` and `begin_block` to +mimic the real blockchain environment such as Cosmos-SDK. - // similarly for channels and packets +```rs +impl MockContext +where + S: ProvableStore + Debug, + H: TestHost, +{ + pub fn ibc_store_mut(&mut self) -> &mut MockIbcStore; + pub fn host_mut(&mut self) -> &mut H; + + pub fn generate_genesis_block(&mut self, genesis_time: Timestamp); + pub fn begin_block(&mut self); + pub fn end_block(&mut self); + pub fn produce_block(&mut self, block_time: Duration); } +``` -struct Ibc { - context: IbcStore, - router: R -} +### ICS23 compatible proof generation + +With the new ability of proof generation, we can now test the Tendermint light +clients. But we need our proofs to be ICS23 compatible. ICS23 expects the IBC +store root to be committed at a commitment prefix at a top store in the host +environment. + +For this, we add an extra store in `MockContext` where the `MockIbcStore` +commits its storage root at its +[commitment prefix](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/core_ctx.rs#L127-L129) +key. + +So the `MockContext` is finalized as: -pub trait Host { - type Block; - fn chain_id(&self) -> String; - fn generate_block(&self, hash: Vec) -> Self::Block; - fn generate_client_state(&self) -> Self::Block; - fn generate_consensus_state(&self) -> Self::Block; +```rs +pub struct MockGenericContext +where + S: ProvableStore + Debug, + H: TestHost +{ + pub main_store: S, + pub host: H, + pub ibc_store: MockIbcStore, } +``` + +Now the `MockIbcStore` can generate proofs that contain the proofs in its store +and commitment prefix. But it has to know the proofs of its commitment prefix of +the previous heights. -pub trait Store { - type Error; - fn get(&self, key: &[u8]) -> Result, Self::Error>; - fn set(&mut self, key: &[u8], value: Vec) -> Result, Self::Error> - fn commit(&mut self) -> Result<(), Self::Error>; - fn root_hash(&self) -> Result, Self::Error>; - fn get_proof(&self, key: &[u8]) -> Result, Self::Error>; +So we add an extra store in `MockIbcStore` to store the proofs from previous +heights. This is similar to storing `HostConsensusState` of previous heights. + +```rs +#[derive(Debug)] +pub struct MockIbcStore +where + S: ProvableStore + Debug, +{ + ... + /// Map of host consensus states + pub host_consensus_states: Arc>>, + /// Map of proofs of ibc commitment prefix + pub ibc_commiment_proofs: Arc>>, } ``` -The idea is to maintain multiple `Chain` instances and use `Relayer` to -facilitate the IBC among them. The `Chain` uses `blocks` and `store` to model -chain history and storage. `Chain::advance_height` is used to commit changes to -store and generate a new block with a new height and commitment root. -`Chain::dispatch` is used to submit messages to the IBC module and make changes -to the storage. +The storing of the IBC store root at the IBC commitment prefix happens in the +end block. The storing of proofs and host consensus states happens in the +`begin_block` of the `MockContext`. + +### Integration Test via `RelayerContext` + +With all the above changes, we can now write an integration test that tests the +IBC workflow - client creation, connection handshake, channel handshake and +packet relay for any host environment that implements `TestHost`. + +This can be done by reading the +[IBC events](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L95) +from `MockIbcStore` and creating and sending the IBC messages via +[`MockContext::deliver`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L696). + +### Miscellaneous + +To achieve blockchain-like interfaces, we removed `max_history_size` and +`host_chain_id` from `MockContext`. + +- `max_history_size`: We generate all the blocks till a block height. This gives + us reproducibility. If we need to prune some older block data, we use a + dedicated `prune_block_till` to prune older blocks. This makes our tests more + descriptive about the assumption of the test scenarios. +- `host_chain_id`: The IBC runtime does not depend on `host_chain_id` directly. + The `TestHost` trait implementation is responsible for generating the blocks + with the necessary data. -We should use emitted IBC events and log messages in our tests too. We should -have utility functions to parse and assert these in our tests. +Also to minimize verbosity while writing tests (as Rust doesn't support default +arguments to function parameters), we want to use some parameter builders. For +that, we can use [`TypedBuilder`](https://crates.io/crates/typed-builder) crate. ## Status @@ -150,9 +263,9 @@ This ADR pays the technical debt of the existing testing framework. ### Positive The future tests will be more readable and maintainable. The test framework -becomes modular and heavily leverages Rust's trait system. Even the `ibc-rs` -users may benefit from this framework to test their implementations of `ibc-rs` -traits. +becomes modular and leverages Rust's trait system. Even the `ibc-rs` users may +benefit from this framework to test their implementations of `ibc-rs` +components. ### Negative @@ -160,8 +273,7 @@ This requires a significant refactoring of the existing tests. Since this may take some time, the parallel development on `main` branch may conflict with this work. -## Future +## References -The IBC standard is being implemented in different languages. We should -investigate a DSL to write the IBC test scenarios. This way we add portability -to our tests and check compatibility with other implementations, such as ibc-go. +This work is being tracked at +[cosmos/ibc-rs#1109](https://github.com/cosmos/ibc-rs/pull/1109) From 009947e17922d6445f46f57b0e2570d4ec69eb65 Mon Sep 17 00:00:00 2001 From: Rano | Ranadeep Date: Mon, 8 Apr 2024 04:56:39 -0400 Subject: [PATCH 03/16] apply suggestions from pr review Co-authored-by: Sean Chen Signed-off-by: Rano | Ranadeep --- docs/architecture/adr-009-revamp-testkit.md | 62 ++++++++++----------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index 91d221333..b4be37cbc 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -18,7 +18,7 @@ To overcome this, we need to improve our test framework that allows: - testing different implementations (traits). - succinct tests (useful `util` methods). -- improving test coverage (Merkle proof generation). +- improving test coverage (i.e. validating Merkle proof generation). - integration tests exercising the IBC workflow (relayer-like interface) ## Decision @@ -29,31 +29,31 @@ tests succinct and readable. Instead of bootstrapping the mock data, we should use valid steps to generate it - so that we know the exact steps to reach a state to reproduce in a real environment. -To achieve this, we have broken down the proposal into sub-proposals. +To achieve this, we have broken down the proposal into sub-proposals: ### Adopt a Merkle store for the test framework The current framework uses `HashMap` and `HashSet` to store data. This works for many test scenarios, but it fails to test proof-sensitive scenarios. Because of -this, we don't have any connection, channel handshake or packet relay tests for -Tendermint light client. +this, we don't have any connection, channel handshake, or packet relay tests that +cover the Tendermint light client. We generalize [`MockContext`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L103) to use a Merkle store which is used for IBC Context's store. For concrete or -default implementation, we can use the IAVL Merkle store implementation from +default implementations, we can use the IAVL Merkle store implementation from `informalsystems/basecoin-rs`. ### Modularize the host environment Currently, we are using `Mock` and `SyntheticTendermint` [variants](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L33-L36) -as the host environments. To manage these two different environments, we also +as host environments. To manage these two different environments, we also introduced [`HostBlocks`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L72-75) -for corresponding host variants. +for encapsulating the possible host variants that need to be covered by `ibc-testkit`. -This creates friction if we have to add a new host variant. It creates the same +However, this creates friction in the case when we need to add new host variants. It creates the same problem as why we have `ibc-derive` crate for `ClientState` and `ConsensusState`. This should be refactored by a generic `TestHost` trait that maintains its corresponding types e.g. `Block` types, via associated types. @@ -61,7 +61,7 @@ Finally, we generalize the `MockContext` once more to use this `TestHost` trait instead of a concrete enum variant. This `TestHost` trait should be responsible for generating blocks, headers, -client and consensus states specific to that host environment. +client, and consensus states specific to that host environment. ```rs /// TestHost is a trait that defines the interface for a host blockchain. @@ -72,16 +72,16 @@ pub trait TestHost: Default + Debug + Sized { /// The type of client state produced by the host. type ClientState: Into + Debug; - /// The type of block parameters to produce a block + /// The type of block parameters to produce a block. type BlockParams: Debug + Default; - /// The type of light client parameters to produce a light client state + /// The type of light client parameters to produce a light client state. type LightClientParams: Debug + Default; /// The history of blocks produced by the host chain. fn history(&self) -> &VecDeque; - /// Triggers the advancing of the host chain, by extending the history of blocks (or headers). + /// Triggers the advancing of the host chain by extending the history of blocks (or headers). fn advance_block( &mut self, commitment_root: Vec, @@ -108,7 +108,7 @@ pub trait TestHost: Default + Debug + Sized { /// TestBlock is a trait that defines the interface for a block produced by a host blockchain. pub trait TestBlock: Clone + Debug { - /// The type of header can be extracted from the block. + /// The type of header that can be extracted from the block. type Header: TestHeader; /// The height of the block. @@ -141,10 +141,9 @@ pub trait TestHeader: Clone + Debug + Into { Currently, `MockContext` implements the top validation and execution context of `ibc-rs`. It contains other host-specific data e.g. `host_chain_id`, -`block_time` - that are irrelevant to the IBC context directly. If we think -`MockContext` as a real blockchain context, the `MockContext` represents the top -runtime - which contains the IBC module. So we implement the validation and -execution context on `MockIbcStore`, instead of `MockContext`. +`block_time` - that are not directly relevant to the IBC context. If we think +of `MockContext` as a real blockchain context, the `MockContext` represents the top- +level runtime; it contains `MockIbcStore`, which is a more appropriate candidate to implement the validation and execution contexts than the `MockContext` itself. With this, the `MockContext` contains two decoupled parts - the host and the IBC module. @@ -154,8 +153,8 @@ module. With the above changes, we can refactor the `MockContext` to have blockchain-like interfaces. -The `MockContext` should have `end_block`, `produce_block` and `begin_block` to -mimic the real blockchain environment such as Cosmos-SDK. +The `MockContext` should have `end_block`, `produce_block`, and `begin_block` to +mimic real blockchain environments, such as the Cosmos-SDK. ```rs impl MockContext @@ -175,9 +174,9 @@ where ### ICS23 compatible proof generation -With the new ability of proof generation, we can now test the Tendermint light +With the new proof generation capabilities, we can now test the Tendermint light clients. But we need our proofs to be ICS23 compatible. ICS23 expects the IBC -store root to be committed at a commitment prefix at a top store in the host +store root to be committed at a commitment prefix at a top-level store in the host environment. For this, we add an extra store in `MockContext` where the `MockIbcStore` @@ -204,7 +203,7 @@ and commitment prefix. But it has to know the proofs of its commitment prefix of the previous heights. So we add an extra store in `MockIbcStore` to store the proofs from previous -heights. This is similar to storing `HostConsensusState` of previous heights. +heights. This is similar to storing `HostConsensusState`s of previous heights. ```rs #[derive(Debug)] @@ -213,9 +212,9 @@ where S: ProvableStore + Debug, { ... - /// Map of host consensus states + /// Map of host consensus states. pub host_consensus_states: Arc>>, - /// Map of proofs of ibc commitment prefix + /// Map of proofs of ibc commitment prefix. pub ibc_commiment_proofs: Arc>>, } ``` @@ -227,8 +226,8 @@ end block. The storing of proofs and host consensus states happens in the ### Integration Test via `RelayerContext` With all the above changes, we can now write an integration test that tests the -IBC workflow - client creation, connection handshake, channel handshake and -packet relay for any host environment that implements `TestHost`. +IBC workflow - client creation, connection handshake, channel handshake, and +packet relaying for any host environment that implements `TestHost`. This can be done by reading the [IBC events](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L95) @@ -250,7 +249,7 @@ To achieve blockchain-like interfaces, we removed `max_history_size` and Also to minimize verbosity while writing tests (as Rust doesn't support default arguments to function parameters), we want to use some parameter builders. For -that, we can use [`TypedBuilder`](https://crates.io/crates/typed-builder) crate. +that, we can use the [`TypedBuilder`](https://crates.io/crates/typed-builder) crate. ## Status @@ -262,15 +261,14 @@ This ADR pays the technical debt of the existing testing framework. ### Positive -The future tests will be more readable and maintainable. The test framework -becomes modular and leverages Rust's trait system. Even the `ibc-rs` users may -benefit from this framework to test their implementations of `ibc-rs` -components. +Future tests will be more readable and maintainable. The test framework +becomes modular and leverages Rust's trait system. `ibc-rs` users may +benefit from this framework, which allows them to test their host implementations of `ibc-rs` components. ### Negative This requires a significant refactoring of the existing tests. Since this may -take some time, the parallel development on `main` branch may conflict with this +take some time, the parallel development on the `main` branch may conflict with this work. ## References From b96d805a07aba43063f60790d41a794c25c3df8d Mon Sep 17 00:00:00 2001 From: Rano | Ranadeep Date: Mon, 8 Apr 2024 05:13:13 -0400 Subject: [PATCH 04/16] use numbers list for sub-proposal titles Co-authored-by: Sean Chen Signed-off-by: Rano | Ranadeep --- docs/architecture/adr-009-revamp-testkit.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index b4be37cbc..a20f0f186 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -31,7 +31,7 @@ state to reproduce in a real environment. To achieve this, we have broken down the proposal into sub-proposals: -### Adopt a Merkle store for the test framework +### 1. Adopt a Merkle store for the test framework The current framework uses `HashMap` and `HashSet` to store data. This works for many test scenarios, but it fails to test proof-sensitive scenarios. Because of @@ -44,7 +44,7 @@ to use a Merkle store which is used for IBC Context's store. For concrete or default implementations, we can use the IAVL Merkle store implementation from `informalsystems/basecoin-rs`. -### Modularize the host environment +### 2. Modularize the host environment Currently, we are using `Mock` and `SyntheticTendermint` [variants](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L33-L36) @@ -137,7 +137,7 @@ pub trait TestHeader: Clone + Debug + Into { } ``` -### Decoupling IbcContext and Host environment +### 3. Decoupling IbcContext and Host environment Currently, `MockContext` implements the top validation and execution context of `ibc-rs`. It contains other host-specific data e.g. `host_chain_id`, @@ -148,7 +148,7 @@ level runtime; it contains `MockIbcStore`, which is a more appropriate candidate With this, the `MockContext` contains two decoupled parts - the host and the IBC module. -### Chain-like interface for `MockContext` +### 4. Chain-like interface for `MockContext` With the above changes, we can refactor the `MockContext` to have blockchain-like interfaces. @@ -172,7 +172,7 @@ where } ``` -### ICS23 compatible proof generation +### 5. ICS23 compatible proof generation With the new proof generation capabilities, we can now test the Tendermint light clients. But we need our proofs to be ICS23 compatible. ICS23 expects the IBC @@ -223,7 +223,7 @@ The storing of the IBC store root at the IBC commitment prefix happens in the end block. The storing of proofs and host consensus states happens in the `begin_block` of the `MockContext`. -### Integration Test via `RelayerContext` +### 6. Integration Tests via `RelayerContext` With all the above changes, we can now write an integration test that tests the IBC workflow - client creation, connection handshake, channel handshake, and From 81eb8f314a8ceda2bea5a8ca7641de687c8af162 Mon Sep 17 00:00:00 2001 From: Rano | Ranadeep Date: Mon, 8 Apr 2024 05:22:55 -0400 Subject: [PATCH 05/16] links to validation and execute context Co-authored-by: Sean Chen Signed-off-by: Rano | Ranadeep --- docs/architecture/adr-009-revamp-testkit.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index a20f0f186..fe56ce8d0 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -139,8 +139,8 @@ pub trait TestHeader: Clone + Debug + Into { ### 3. Decoupling IbcContext and Host environment -Currently, `MockContext` implements the top validation and execution context of -`ibc-rs`. It contains other host-specific data e.g. `host_chain_id`, +Currently, `MockContext` implements the top-level [validation](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-core/ics25-handler/src/entrypoint.rs#L45) and [execution](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-core/ics25-handler/src/entrypoint.rs#L112) contexts of +`ibc-rs`, as opposed to the more granular contexts of each of the individual handlers. It contains other host-specific data e.g. `host_chain_id`, `block_time` - that are not directly relevant to the IBC context. If we think of `MockContext` as a real blockchain context, the `MockContext` represents the top- level runtime; it contains `MockIbcStore`, which is a more appropriate candidate to implement the validation and execution contexts than the `MockContext` itself. From bad46c91f9727912abf0679fabafd97f5198d47e Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 8 Apr 2024 11:44:51 +0200 Subject: [PATCH 06/16] apply suggestion from pr review --- docs/architecture/adr-009-revamp-testkit.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index fe56ce8d0..9bc542eb2 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -46,16 +46,18 @@ default implementations, we can use the IAVL Merkle store implementation from ### 2. Modularize the host environment -Currently, we are using `Mock` and `SyntheticTendermint` -[variants](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L33-L36) -as host environments. To manage these two different environments, we also +Currently, we are using `Mock` and `SyntheticTendermint` variants of +[`HostType`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L33-L36) +enum as host environments. To manage these two different environments, we also introduced [`HostBlocks`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L72-75) for encapsulating the possible host variants that need to be covered by `ibc-testkit`. However, this creates friction in the case when we need to add new host variants. It creates the same -problem as why we have `ibc-derive` crate for `ClientState` and -`ConsensusState`. This should be refactored by a generic `TestHost` trait that +problem that the `ibc-derive` crate was designed to solve for `ClientState` and +`ConsensusState` types, namely: dispatching methods to underlying variants of a top-level enum. +But, a concrete `MockContext` will always have a unique `HostType` and its corresponding `HostBlocks`. +So we can refactor `HostTypes` and `HockBlocks` with a generic `TestHost` trait that maintains its corresponding types e.g. `Block` types, via associated types. Finally, we generalize the `MockContext` once more to use this `TestHost` trait instead of a concrete enum variant. From cdd5dcfe56bed1bb6516911f8aeceb4d1a6ca6e7 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Tue, 9 Apr 2024 16:01:46 +0200 Subject: [PATCH 07/16] add links to impls --- docs/architecture/adr-009-revamp-testkit.md | 32 ++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index 9bc542eb2..24be678ae 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -276,4 +276,34 @@ work. ## References This work is being tracked at -[cosmos/ibc-rs#1109](https://github.com/cosmos/ibc-rs/pull/1109) +[cosmos/ibc-rs#1109](https://github.com/cosmos/ibc-rs/pull/1109). + +The following provides the concrete implementations of the proposed changes: + +#### MockIbcStore + +The modified `MockIbcStore` with Merkle store lives at +[`testapp/ibc/core/types.rs`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/testapp/ibc/core/types.rs#L43-L96). + +#### TestHost + +The Rust trait lives at +[`hosts/mod.rs`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/hosts/mod.rs#L27). +The `Mock` and `Tendermint` host implementations live in +[`hosts/mock.rs`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/hosts/mock.rs#L30) +and +[`hosts/tendermint.rs`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/hosts/tendermint.rs#L42) +respectively. + +#### MockGenericContext + +[`MockGenericContext`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/context.rs#L34-L52) +is actually what is described as `MockContext` in the ADR. For simplicity, we +defined `MockContext` to +[have a concrete store](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/context.rs#L54-L55) +implementation. + +```rs +pub type MockStore = RevertibleStore>; +pub type MockContext = MockGenericContext; +``` From d9f5c9e9ece364875d302246cc522bf6a810c704 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Tue, 30 Apr 2024 22:24:38 +0200 Subject: [PATCH 08/16] update comment --- docs/architecture/adr-009-revamp-testkit.md | 7 ++++--- ibc-testkit/src/hosts/mod.rs | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index 24be678ae..439b26173 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -35,8 +35,8 @@ To achieve this, we have broken down the proposal into sub-proposals: The current framework uses `HashMap` and `HashSet` to store data. This works for many test scenarios, but it fails to test proof-sensitive scenarios. Because of -this, we don't have any connection, channel handshake, or packet relay tests that -cover the Tendermint light client. +this, we don't have any connection, channel handshake, or packet relay tests +that cover the Tendermint light client. We generalize [`MockContext`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L103) @@ -123,7 +123,8 @@ pub trait TestBlock: Clone + Debug { fn into_header(self) -> Self::Header; } -/// TestHeader is a trait that defines the interface for a header produced by a host blockchain. +/// TestHeader is a trait that defines the interface for a header +/// submitted by relayer from the host blockchain. pub trait TestHeader: Clone + Debug + Into { /// The type of consensus state can be extracted from the header. type ConsensusState: ConsensusState + Into + From + Clone + Debug; diff --git a/ibc-testkit/src/hosts/mod.rs b/ibc-testkit/src/hosts/mod.rs index 3a4183401..426b69066 100644 --- a/ibc-testkit/src/hosts/mod.rs +++ b/ibc-testkit/src/hosts/mod.rs @@ -138,7 +138,8 @@ pub trait TestBlock: Clone + Debug { } } -/// TestHeader is a trait that defines the interface for a header corresponding to a host blockchain. +/// TestHeader is a trait that defines the interface for a header +/// submitted by relayer from the host blockchain. pub trait TestHeader: Clone + Debug + Into { /// The type of consensus state can be extracted from the header. type ConsensusState: ConsensusState + Into + From + Clone + Debug; From 6235136fe4e65d6dd149dedd90944bffcfc02ee8 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Tue, 30 Apr 2024 22:25:45 +0200 Subject: [PATCH 09/16] markdown format --- docs/architecture/adr-009-revamp-testkit.md | 56 ++++++++++++--------- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index 439b26173..2338b6742 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -51,16 +51,18 @@ Currently, we are using `Mock` and `SyntheticTendermint` variants of enum as host environments. To manage these two different environments, we also introduced [`HostBlocks`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L72-75) -for encapsulating the possible host variants that need to be covered by `ibc-testkit`. - -However, this creates friction in the case when we need to add new host variants. It creates the same -problem that the `ibc-derive` crate was designed to solve for `ClientState` and -`ConsensusState` types, namely: dispatching methods to underlying variants of a top-level enum. -But, a concrete `MockContext` will always have a unique `HostType` and its corresponding `HostBlocks`. -So we can refactor `HostTypes` and `HockBlocks` with a generic `TestHost` trait that -maintains its corresponding types e.g. `Block` types, via associated types. -Finally, we generalize the `MockContext` once more to use this `TestHost` trait -instead of a concrete enum variant. +for encapsulating the possible host variants that need to be covered by +`ibc-testkit`. + +However, this creates friction in the case when we need to add new host +variants. It creates the same problem that the `ibc-derive` crate was designed +to solve for `ClientState` and `ConsensusState` types, namely: dispatching +methods to underlying variants of a top-level enum. But, a concrete +`MockContext` will always have a unique `HostType` and its corresponding +`HostBlocks`. So we can refactor `HostTypes` and `HockBlocks` with a generic +`TestHost` trait that maintains its corresponding types e.g. `Block` types, via +associated types. Finally, we generalize the `MockContext` once more to use this +`TestHost` trait instead of a concrete enum variant. This `TestHost` trait should be responsible for generating blocks, headers, client, and consensus states specific to that host environment. @@ -142,11 +144,17 @@ pub trait TestHeader: Clone + Debug + Into { ### 3. Decoupling IbcContext and Host environment -Currently, `MockContext` implements the top-level [validation](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-core/ics25-handler/src/entrypoint.rs#L45) and [execution](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-core/ics25-handler/src/entrypoint.rs#L112) contexts of -`ibc-rs`, as opposed to the more granular contexts of each of the individual handlers. It contains other host-specific data e.g. `host_chain_id`, -`block_time` - that are not directly relevant to the IBC context. If we think -of `MockContext` as a real blockchain context, the `MockContext` represents the top- -level runtime; it contains `MockIbcStore`, which is a more appropriate candidate to implement the validation and execution contexts than the `MockContext` itself. +Currently, `MockContext` implements the top-level +[validation](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-core/ics25-handler/src/entrypoint.rs#L45) +and +[execution](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-core/ics25-handler/src/entrypoint.rs#L112) +contexts of `ibc-rs`, as opposed to the more granular contexts of each of the +individual handlers. It contains other host-specific data e.g. `host_chain_id`, +`block_time` - that are not directly relevant to the IBC context. If we think of +`MockContext` as a real blockchain context, the `MockContext` represents the +top- level runtime; it contains `MockIbcStore`, which is a more appropriate +candidate to implement the validation and execution contexts than the +`MockContext` itself. With this, the `MockContext` contains two decoupled parts - the host and the IBC module. @@ -179,8 +187,8 @@ where With the new proof generation capabilities, we can now test the Tendermint light clients. But we need our proofs to be ICS23 compatible. ICS23 expects the IBC -store root to be committed at a commitment prefix at a top-level store in the host -environment. +store root to be committed at a commitment prefix at a top-level store in the +host environment. For this, we add an extra store in `MockContext` where the `MockIbcStore` commits its storage root at its @@ -252,7 +260,8 @@ To achieve blockchain-like interfaces, we removed `max_history_size` and Also to minimize verbosity while writing tests (as Rust doesn't support default arguments to function parameters), we want to use some parameter builders. For -that, we can use the [`TypedBuilder`](https://crates.io/crates/typed-builder) crate. +that, we can use the [`TypedBuilder`](https://crates.io/crates/typed-builder) +crate. ## Status @@ -264,15 +273,16 @@ This ADR pays the technical debt of the existing testing framework. ### Positive -Future tests will be more readable and maintainable. The test framework -becomes modular and leverages Rust's trait system. `ibc-rs` users may -benefit from this framework, which allows them to test their host implementations of `ibc-rs` components. +Future tests will be more readable and maintainable. The test framework becomes +modular and leverages Rust's trait system. `ibc-rs` users may benefit from this +framework, which allows them to test their host implementations of `ibc-rs` +components. ### Negative This requires a significant refactoring of the existing tests. Since this may -take some time, the parallel development on the `main` branch may conflict with this -work. +take some time, the parallel development on the `main` branch may conflict with +this work. ## References From c383c40c0d83c78e2a478ecaeeec51ed77a24892 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Tue, 30 Apr 2024 22:30:47 +0200 Subject: [PATCH 10/16] update adr --- docs/architecture/adr-009-revamp-testkit.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index 2338b6742..4cd6f5f28 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -83,10 +83,10 @@ pub trait TestHost: Default + Debug + Sized { type LightClientParams: Debug + Default; /// The history of blocks produced by the host chain. - fn history(&self) -> &VecDeque; + fn history(&self) -> &Vec; - /// Triggers the advancing of the host chain by extending the history of blocks (or headers). - fn advance_block( + /// Commit a block with commitment root to the blockchain, by extending the history of blocks. + fn commit_block( &mut self, commitment_root: Vec, block_time: Duration, From f5c0dbdcb0ddf1060a6158b5c005b7e5f0177a05 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Tue, 30 Apr 2024 22:35:22 +0200 Subject: [PATCH 11/16] use TestContext --- docs/architecture/adr-009-revamp-testkit.md | 35 ++++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index 4cd6f5f28..c33f4528a 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -198,7 +198,7 @@ key. So the `MockContext` is finalized as: ```rs -pub struct MockGenericContext +pub struct MockContext where S: ProvableStore + Debug, H: TestHost @@ -306,15 +306,34 @@ and [`hosts/tendermint.rs`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/hosts/tendermint.rs#L42) respectively. -#### MockGenericContext +#### Renaming `MockContext` to `StoreGenericTestContext` -[`MockGenericContext`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/context.rs#L34-L52) -is actually what is described as `MockContext` in the ADR. For simplicity, we -defined `MockContext` to -[have a concrete store](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/context.rs#L54-L55) -implementation. +There was confusion about what is a _Test_ component and what is a _Mock_ +component. We have `MockContext` with `MockClient` and `TendermintClient`. + +To avoid this confusion, we renamed `MockContext` to `StoreGenericTestContext`. +This means that going forward all our general frameworks and traits should have +`Test` in their name. But a dummy concrete implementation of these traits may +have `Mock` in their name. + +#### StoreGenericTestContext + +[`StoreGenericTestContext`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/context.rs#L34-L52) +is actually what is described as `MockContext` in the ADR. For convenience, we +defined `TestContext` to have a concrete store implementation - +[`MockStore`](https://github.com/cosmos/ibc-rs/blob/feat/refactor-testkit/ibc-testkit/src/context.rs#L55-L56). ```rs +// A mock store type using basecoin-storage implementations. pub type MockStore = RevertibleStore>; -pub type MockContext = MockGenericContext; + +pub type TestContext = StoreGenericTestContext; +``` + +With this, we can now define `MockContext` which uses `MockStore` and `MockHost` +and `TendermintContext` which uses `MockStore` and `TendermintHost`. + +```rs +pub type MockContext = TestContext; +pub type TendermintContext = TestContext; ``` From 8bc5d5bb831dce5385dc5b27a8322fa882e5cbb7 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 2 May 2024 10:47:03 +0200 Subject: [PATCH 12/16] update comment --- ibc-testkit/src/context.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ibc-testkit/src/context.rs b/ibc-testkit/src/context.rs index 846182e8c..35e4b9d81 100644 --- a/ibc-testkit/src/context.rs +++ b/ibc-testkit/src/context.rs @@ -166,8 +166,9 @@ where /// Begin a new block on the context. /// - /// This method book keeps the data from last block, - /// and prepares the context for the next block. + /// This method commits the required metadata from the last block generation + /// and consensus, and prepares the context for the next block. This includes + /// the latest consensus state and the latest IBC commitment proof. pub fn begin_block(&mut self) { let consensus_state = self .host From b3c447ebf79aa9b42e7673e8673bbab03f4dd319 Mon Sep 17 00:00:00 2001 From: Rano | Ranadeep Date: Thu, 2 May 2024 04:51:51 -0400 Subject: [PATCH 13/16] apply suggestions from code review Co-authored-by: Sean Chen Signed-off-by: Rano | Ranadeep --- docs/architecture/adr-009-revamp-testkit.md | 24 ++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index c33f4528a..09ebd830d 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -7,25 +7,25 @@ ## Context The current framework in the IBC testkit uses -[existing types and injects state or dependency +[existing types and injects state and/or dependencies manually](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/tests/core/ics02_client/update_client.rs#L574-L578). Sometimes, it uses -[semantically wrong data as mock data](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L320). +[semantically incorrect data as mock data](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/testapp/ibc/core/types.rs#L320). Because of this, tests with customizable steps and fixed fixtures became ad-hoc and unmaintainable. -To overcome this, we need to improve our test framework that allows: +To overcome this, we need to improve our test framework such that it allows: -- testing different implementations (traits). -- succinct tests (useful `util` methods). -- improving test coverage (i.e. validating Merkle proof generation). -- integration tests exercising the IBC workflow (relayer-like interface) +- testing different implementations and traits. +- composition of succinct tests with useful `util` methods. +- validating of code paths that were not easily testable before, i.e. Merkle proof generation. +- simulating a more realistic IBC workflow, using real IBC and relayer interfaces. ## Decision The main goal of this proposal is to create a test framework that is modular and -closer to the real blockchain environment. This should also make the existing -tests succinct and readable. Instead of bootstrapping the mock data, we should +closer to a real blockchain environment. This should also make the existing +tests more succinct and readable. Instead of bootstrapping the mock data that tests use, we should use valid steps to generate it - so that we know the exact steps to reach a state to reproduce in a real environment. @@ -46,7 +46,7 @@ default implementations, we can use the IAVL Merkle store implementation from ### 2. Modularize the host environment -Currently, we are using `Mock` and `SyntheticTendermint` variants of +Currently, we are using `Mock` and `SyntheticTendermint` variants of the [`HostType`](https://github.com/cosmos/ibc-rs/blob/v0.51.0/ibc-testkit/src/hosts/block.rs#L33-L36) enum as host environments. To manage these two different environments, we also introduced @@ -153,7 +153,7 @@ individual handlers. It contains other host-specific data e.g. `host_chain_id`, `block_time` - that are not directly relevant to the IBC context. If we think of `MockContext` as a real blockchain context, the `MockContext` represents the top- level runtime; it contains `MockIbcStore`, which is a more appropriate -candidate to implement the validation and execution contexts than the +candidate to implement the validation and execution contexts for than the `MockContext` itself. With this, the `MockContext` contains two decoupled parts - the host and the IBC @@ -231,7 +231,7 @@ where ``` The storing of the IBC store root at the IBC commitment prefix happens in the -end block. The storing of proofs and host consensus states happens in the +`end_block` procedure. `produce_block` commits the main store, produces a block with its latest root, and pushes the block to the blockchain. The storing of proofs and host consensus states happens in the `begin_block` of the `MockContext`. ### 6. Integration Tests via `RelayerContext` From 802de69794afd2677ffb39844139b5296c5b5212 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 2 May 2024 13:35:54 +0200 Subject: [PATCH 14/16] markdown format --- docs/architecture/adr-009-revamp-testkit.md | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index 09ebd830d..e82c9bb28 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -18,16 +18,18 @@ To overcome this, we need to improve our test framework such that it allows: - testing different implementations and traits. - composition of succinct tests with useful `util` methods. -- validating of code paths that were not easily testable before, i.e. Merkle proof generation. -- simulating a more realistic IBC workflow, using real IBC and relayer interfaces. +- validating code paths that were not easily testable before, i.e. Merkle proof + generation. +- simulating a more realistic IBC workflow, using real IBC and relayer + interfaces. ## Decision The main goal of this proposal is to create a test framework that is modular and closer to a real blockchain environment. This should also make the existing -tests more succinct and readable. Instead of bootstrapping the mock data that tests use, we should -use valid steps to generate it - so that we know the exact steps to reach a -state to reproduce in a real environment. +tests more succinct and readable. Instead of bootstrapping the mock data that +tests use, we should use valid steps to generate it - so that we know the exact +steps to reach a state to reproduce in a real environment. To achieve this, we have broken down the proposal into sub-proposals: @@ -152,7 +154,7 @@ contexts of `ibc-rs`, as opposed to the more granular contexts of each of the individual handlers. It contains other host-specific data e.g. `host_chain_id`, `block_time` - that are not directly relevant to the IBC context. If we think of `MockContext` as a real blockchain context, the `MockContext` represents the -top- level runtime; it contains `MockIbcStore`, which is a more appropriate +top-level runtime; it contains `MockIbcStore`, which is a more appropriate candidate to implement the validation and execution contexts for than the `MockContext` itself. @@ -231,8 +233,10 @@ where ``` The storing of the IBC store root at the IBC commitment prefix happens in the -`end_block` procedure. `produce_block` commits the main store, produces a block with its latest root, and pushes the block to the blockchain. The storing of proofs and host consensus states happens in the -`begin_block` of the `MockContext`. +`end_block` procedure. `produce_block` commits the main store, produces a block +with its latest root, and pushes the block to the blockchain. The storing of +proofs and host consensus states happens in the `begin_block` of the +`MockContext`. ### 6. Integration Tests via `RelayerContext` From 557c17a0ccbf377cb57ef8651ae6561bb851961f Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 2 May 2024 18:10:47 +0200 Subject: [PATCH 15/16] update method names --- docs/architecture/adr-009-revamp-testkit.md | 12 +++++++++--- ibc-testkit/src/context.rs | 16 ++++++++++------ ibc-testkit/src/fixtures/core/context.rs | 2 +- ibc-testkit/src/relayer/integration.rs | 2 +- ibc-testkit/src/relayer/utils.rs | 2 +- .../tests/core/ics02_client/recover_client.rs | 2 +- .../tests/core/ics02_client/update_client.rs | 8 ++++---- 7 files changed, 27 insertions(+), 17 deletions(-) diff --git a/docs/architecture/adr-009-revamp-testkit.md b/docs/architecture/adr-009-revamp-testkit.md index e82c9bb28..8bf5af3e2 100644 --- a/docs/architecture/adr-009-revamp-testkit.md +++ b/docs/architecture/adr-009-revamp-testkit.md @@ -175,13 +175,19 @@ where S: ProvableStore + Debug, H: TestHost, { + // Access ibc module store pub fn ibc_store_mut(&mut self) -> &mut MockIbcStore; - pub fn host_mut(&mut self) -> &mut H; - pub fn generate_genesis_block(&mut self, genesis_time: Timestamp); + // Advance the first block height. + pub fn advance_genesis_height(&mut self, genesis_time: Timestamp); + // Routine procedures at the beginning of a block + // Just after committing last block state. pub fn begin_block(&mut self); + // Advance the current block height. + pub fn advance_block_height(&mut self, block_time: Duration); + // Routine procedures at the end of a block + // Just before committing current block state. pub fn end_block(&mut self); - pub fn produce_block(&mut self, block_time: Duration); } ``` diff --git a/ibc-testkit/src/context.rs b/ibc-testkit/src/context.rs index 35e4b9d81..8ff2596c9 100644 --- a/ibc-testkit/src/context.rs +++ b/ibc-testkit/src/context.rs @@ -131,7 +131,7 @@ where } else { // Repeatedly advance the host chain height till we hit the desired height while self.host.latest_height().revision_height() < target_height.revision_height() { - self.advance_height() + self.advance_block_height() } } self @@ -231,15 +231,19 @@ where /// Advances the host chain height by ending the current block, producing a new block, and /// beginning the next block. - pub fn advance_height_with_params(&mut self, block_time: Duration, params: &H::BlockParams) { + pub fn advance_block_height_with_params( + &mut self, + block_time: Duration, + params: &H::BlockParams, + ) { self.end_block(); self.commit_state_to_host(block_time, params); self.begin_block(); } /// Convenience method to advance the host chain height using default parameters. - pub fn advance_height(&mut self) { - self.advance_height_with_params( + pub fn advance_block_height(&mut self) { + self.advance_block_height_with_params( Duration::from_secs(DEFAULT_BLOCK_TIME_SECS), &Default::default(), ) @@ -480,7 +484,7 @@ where self.dispatch(msg) .map_err(RelayerError::TransactionFailed)?; // Create a new block. - self.advance_height(); + self.advance_block_height(); Ok(()) } @@ -571,7 +575,7 @@ mod tests { let current_height = test.ctx.latest_height(); // After advancing the chain's height, the context should still be valid. - test.ctx.advance_height(); + test.ctx.advance_block_height(); assert!( test.ctx.host.validate().is_ok(), "failed in test [{}] {} while validating context {:?}", diff --git a/ibc-testkit/src/fixtures/core/context.rs b/ibc-testkit/src/fixtures/core/context.rs index 7810c6b77..99304f0e0 100644 --- a/ibc-testkit/src/fixtures/core/context.rs +++ b/ibc-testkit/src/fixtures/core/context.rs @@ -80,7 +80,7 @@ where ); for block_params in params.block_params_history { - context.advance_height_with_params(params.block_time, &block_params); + context.advance_block_height_with_params(params.block_time, &block_params); } assert_eq!( diff --git a/ibc-testkit/src/relayer/integration.rs b/ibc-testkit/src/relayer/integration.rs index 766a21f75..240609f63 100644 --- a/ibc-testkit/src/relayer/integration.rs +++ b/ibc-testkit/src/relayer/integration.rs @@ -120,7 +120,7 @@ where .expect("successfully created send_packet"); // send_packet wasn't committed, hence produce a block - relayer.get_ctx_a_mut().advance_height(); + relayer.get_ctx_a_mut().advance_block_height(); // retrieve the send_packet event let Some(IbcEvent::SendPacket(send_packet_event)) = relayer diff --git a/ibc-testkit/src/relayer/utils.rs b/ibc-testkit/src/relayer/utils.rs index af93f6085..9d4c96bd6 100644 --- a/ibc-testkit/src/relayer/utils.rs +++ b/ibc-testkit/src/relayer/utils.rs @@ -103,7 +103,7 @@ where /// Advances the block height on `A` until it catches up with the latest timestamp on `B`. pub fn sync_clock_on_a(ctx_a: &mut TestContext, ctx_b: &TestContext) { while ctx_b.latest_timestamp() > ctx_a.latest_timestamp() { - ctx_a.advance_height(); + ctx_a.advance_block_height(); } } diff --git a/ibc-testkit/tests/core/ics02_client/recover_client.rs b/ibc-testkit/tests/core/ics02_client/recover_client.rs index 8f6f804b0..28f1de076 100644 --- a/ibc-testkit/tests/core/ics02_client/recover_client.rs +++ b/ibc-testkit/tests/core/ics02_client/recover_client.rs @@ -100,7 +100,7 @@ fn setup_client_recovery_fixture( // Let the subject client state expire. while ctx.latest_timestamp() <= subject_client_trusted_timestamp { - ctx.advance_height(); + ctx.advance_block_height(); } // at this point, the subject client should be expired. diff --git a/ibc-testkit/tests/core/ics02_client/update_client.rs b/ibc-testkit/tests/core/ics02_client/update_client.rs index e74f10fa9..9dbc49030 100644 --- a/ibc-testkit/tests/core/ics02_client/update_client.rs +++ b/ibc-testkit/tests/core/ics02_client/update_client.rs @@ -258,7 +258,7 @@ fn test_consensus_state_pruning() { let update_height = ctx.latest_height(); - ctx.advance_height(); + ctx.advance_block_height(); let block = ctx.host_block(&update_height).unwrap().clone(); let mut block = block.into_header(); @@ -1433,7 +1433,7 @@ fn test_expired_client() { while ctx.ibc_store.host_timestamp().expect("no error") < (timestamp + trusting_period).expect("no error") { - ctx.advance_height(); + ctx.advance_block_height(); } let client_state = ctx.ibc_store.client_state(&client_id).unwrap(); @@ -1488,11 +1488,11 @@ fn test_client_update_max_clock_drift() { while ctx_b.ibc_store.host_timestamp().expect("no error") < (ctx_a.ibc_store.host_timestamp().expect("no error") + max_clock_drift).expect("no error") { - ctx_b.advance_height(); + ctx_b.advance_block_height(); } // include current block - ctx_b.advance_height(); + ctx_b.advance_block_height(); let update_height = ctx_b.latest_height(); From cb8e706707999c714995f079c4559b57a2c1f54f Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 2 May 2024 18:57:53 +0200 Subject: [PATCH 16/16] fix cargo doc error --- ibc-testkit/src/context.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ibc-testkit/src/context.rs b/ibc-testkit/src/context.rs index 8ff2596c9..74d141f2e 100644 --- a/ibc-testkit/src/context.rs +++ b/ibc-testkit/src/context.rs @@ -143,8 +143,8 @@ where /// But it bootstraps the genesis block by height 1 and `genesis_time`. /// /// The method starts and ends with [`Self::end_block`] and [`Self::begin_block`], just - /// like the [`Self::advance_height_with_params`], so that it can advance to next height - /// i.e. height 2 - just by calling [`Self::advance_height_with_params`]. + /// like the [`Self::advance_block_height_with_params`], so that it can advance to next height + /// i.e. height 2 - just by calling [`Self::advance_block_height_with_params`]. pub fn advance_genesis_height(&mut self, genesis_time: Timestamp, params: &H::BlockParams) { self.end_block();