-
Notifications
You must be signed in to change notification settings - Fork 94
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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]>
- Loading branch information
1 parent
7704550
commit e42acc9
Showing
8 changed files
with
374 additions
and
19 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,349 @@ | ||
# 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 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 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 such that it allows: | ||
|
||
- testing different implementations and traits. | ||
- composition of succinct tests with useful `util` methods. | ||
- 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. | ||
|
||
To achieve this, we have broken down the proposal into sub-proposals: | ||
|
||
### 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 | ||
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 implementations, we can use the IAVL Merkle store implementation from | ||
`informalsystems/basecoin-rs`. | ||
|
||
### 2. Modularize the host environment | ||
|
||
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 | ||
[`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. | ||
|
||
This `TestHost` trait should be responsible for generating blocks, headers, | ||
client, and consensus states specific to that host environment. | ||
|
||
```rs | ||
/// 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<AnyClientState> + 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) -> &Vec<Self::Block>; | ||
|
||
/// Commit a block with commitment root to the blockchain, by extending the history of blocks. | ||
fn commit_block( | ||
&mut self, | ||
commitment_root: Vec<u8>, | ||
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<u8>, | ||
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 that can be extracted from the block. | ||
type Header: TestHeader; | ||
|
||
/// The height of the block. | ||
fn height(&self) -> Height; | ||
|
||
/// The timestamp of the block. | ||
fn timestamp(&self) -> Timestamp; | ||
|
||
/// Extract the header from the block. | ||
fn into_header(self) -> Self::Header; | ||
} | ||
|
||
/// TestHeader is a trait that defines the interface for a header | ||
/// submitted by relayer from the host blockchain. | ||
pub trait TestHeader: Clone + Debug + Into<Any> { | ||
/// The type of consensus state can be extracted from the header. | ||
type ConsensusState: ConsensusState + Into<AnyConsensusState> + From<Self> + Clone + Debug; | ||
|
||
/// 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; | ||
|
||
/// Extract the consensus state from the header. | ||
fn into_consensus_state(self) -> Self::ConsensusState; | ||
} | ||
``` | ||
|
||
### 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 for than the | ||
`MockContext` itself. | ||
|
||
With this, the `MockContext` contains two decoupled parts - the host and the IBC | ||
module. | ||
|
||
### 4. Chain-like interface for `MockContext` | ||
|
||
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 real blockchain environments, such as the Cosmos-SDK. | ||
|
||
```rs | ||
impl<S, H> MockContext<S, H> | ||
where | ||
S: ProvableStore + Debug, | ||
H: TestHost, | ||
{ | ||
// Access ibc module store | ||
pub fn ibc_store_mut(&mut self) -> &mut MockIbcStore<S>; | ||
|
||
// 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); | ||
} | ||
``` | ||
|
||
### 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 | ||
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 | ||
[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: | ||
|
||
```rs | ||
pub struct MockContext<S, H> | ||
where | ||
S: ProvableStore + Debug, | ||
H: TestHost | ||
{ | ||
pub main_store: S, | ||
pub host: H, | ||
pub ibc_store: MockIbcStore<S>, | ||
} | ||
``` | ||
|
||
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. | ||
|
||
So we add an extra store in `MockIbcStore` to store the proofs from previous | ||
heights. This is similar to storing `HostConsensusState`s of previous heights. | ||
|
||
```rs | ||
#[derive(Debug)] | ||
pub struct MockIbcStore<S> | ||
where | ||
S: ProvableStore + Debug, | ||
{ | ||
... | ||
/// Map of host consensus states. | ||
pub host_consensus_states: Arc<Mutex<BTreeMap<u64, AnyConsensusState>>>, | ||
/// Map of proofs of ibc commitment prefix. | ||
pub ibc_commiment_proofs: Arc<Mutex<BTreeMap<u64, CommitmentProof>>>, | ||
} | ||
``` | ||
|
||
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`. | ||
|
||
### 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 | ||
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) | ||
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. | ||
|
||
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. | ||
|
||
## Status | ||
|
||
Proposed | ||
|
||
## Consequences | ||
|
||
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. | ||
|
||
### 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. | ||
|
||
## References | ||
|
||
This work is being tracked at | ||
[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. | ||
|
||
#### Renaming `MockContext` to `StoreGenericTestContext` | ||
|
||
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<GrowingStore<InMemoryStore>>; | ||
|
||
pub type TestContext<H> = StoreGenericTestContext<MockStore, H>; | ||
``` | ||
|
||
With this, we can now define `MockContext` which uses `MockStore` and `MockHost` | ||
and `TendermintContext` which uses `MockStore` and `TendermintHost`. | ||
|
||
```rs | ||
pub type MockContext = TestContext<MockHost>; | ||
pub type TendermintContext = TestContext<TendermintHost>; | ||
``` |
Oops, something went wrong.