How to use `ibc_testkit`

[zvolin]

Hey, how should one use the ibc_testkit in tests? I thought I should implement TestHost for my blockchain and then I could use the fixtures like

ibc_integration_test::<MyHost, TendermintHost>()

But I cannot implement it because TestHost::ClientState: Into<AnyClientState> and there is no way to extend AnyClientState

[rnbguy]

Thanks for taking interest in ibc-testkit.

Currently, our MockIbcStore works only with AnyClientState - hence this restriction. The immediate solution is to fork ibc-testkit to add a new variant to AnyClientState and AnyConsensusState.

The ideal solution is to generalize MockIbcStore<Storage> with MockIbcStore<Storage, AnyClientState, AnyConsensuState> where AnyClientState and AnyConsensusState are generics. Please don't hesitate to open a PR if that works for you.

[zvolin]

Thanks, I'll take a look at what's needed there

[zvolin]

The ideal solution is to generalize MockIbcStore with MockIbcStore<Storage, AnyClientState, AnyConsensuState> where AnyClientState and AnyConsensusState are generics.

I tried to implement that and I'm kind of stuck. I have some wip changes here, however I'm hitting the recursive generics specification between Host and MockIbcStore. The way I see this is that the Store should be generic over ClientState and ConsensusState which should be specified by Host. However Host::ClientState at some point needs to have ValidationContext and ExecutionContext specified, which are implemented by MockIbcStore.

This is not exactly valid code but quite gives an example:

    /// An object that stores all IBC related data.
    pub ibc_store: MockIbcStore<S, H::ClientState<MockIbcStore<...(recursion)>, MockIbcStore<...>>, HostConsensusState<H, S>>,

Do you maybe have any idea/tips how to approach that? Previously Host and MockIbcStore were kinda disjoint things, and I'm getting feeling that you need a dedicated Contexts per Host, I don't see a way to have both Host and Store generic at the same time...

[rnbguy]

Hey, thanks for reporting this! Makes sense. I will take a dig at this and re-evaluate our generics and associated type bounds.

Meanwhile, I can only suggest forking and modifying AnyClientState and AnyConsensusState.

[zvolin]

actually I'm still looking into this. I've got some first success where this happily compiles.

ibc_integration_test::<TendermintHost, TendermintHost>();

I've tried many different approaches before and I'm not quite happy with current angle but it's still very wip so maybe I'll manage to make it clean

[rnbguy]

Wow ! Great :+1:

Just giving my opinion after going over your changes. I would like to not add ClientValidationContext and ClientExecutionContext in TestHost. Maybe you can add those bounds where we use TestHost explicitly. I agree, this may become too verbose. But let's try to keep things almost identical without any huge dependency changes.

Nonetheless, nice work ! :sparkles:

[zvolin]

I would like to not add ClientValidationContext and ClientExecutionContext in TestHost

I'll see how it plays. Out of curiosity, why? is there a case where TestHost::ClientState doesn't need to implement ClientState?

[rnbguy]

is there a case where TestHost::ClientState doesn't need to implement ClientState?

Not at all. You're right. But I am trying to decouple the circular dependencies between TestHost and MockIbcStore.

Anyway, I am still looking into it. I started a branch rano/feat/generialize-mockibcstore - but it's not finished.

[zvolin]

I had two approaches trying to remove the generics, they both fail on the cyclic requirements. MockIbcStore<S, H> and MockIbcStore<S, AnyClientState, AnyConsensusState>. So far the only way that worked was with those generics but I still couldn't implement the TestHost for TendermintHost genericly, but only for specific MockIbcStore

[zvolin]

I'll try to proceed with this generics on TestHost to see how much cleaner it could get

[zvolin]

Actually this on it's own is enough to give compiler a head ache

impl<S, AnyClientState, AnyConsensusState> ClientExecutionContext
    for MockIbcStore<S, AnyClientState, AnyConsensusState>
where
    S: ProvableStore + Debug,
    AnyClientState: ClientStateExecution<Self> + Clone + Debug + Into<Any> + TryFrom<Any>,
    AnyConsensusState: ConsensusState + Clone + Debug + Into<Any> + TryFrom<Any>,
{
    type ClientStateMut = AnyClientState;

use ibc::clients::tendermint::client_state::ClientState;
use ibc::clients::tendermint::consensus_state::ConsensusState;
use ibc::core::client::context::ClientExecutionContext;

struct A<B: ClientExecutionContext> {
    _b: std::marker::PhantomData<B>,
}

fn get_a() -> A<MockIbcStore<MockStore, ClientState, ConsensusState>> {
    todo!()
}

    = note: required for `MockIbcStore<S, ibc::clients::tendermint::client_state::ClientState, ibc::clients::tendermint::consensus_state::ConsensusState>` to implement `ExtClientExecutionCont
ext`
    = note: required for `ibc::clients::tendermint::client_state::ClientState` to implement `ibc::core::client::context::client_state::ClientStateExecution<MockIbcStore<S, ibc::clients::tende
rmint::client_state::ClientState, ibc::clients::tendermint::consensus_state::ConsensusState>>`
note: required for `MockIbcStore<S, ibc::clients::tendermint::client_state::ClientState, ibc::clients::tendermint::consensus_state::ConsensusState>` to implement `ibc::core::client::context::
ClientExecutionContext`

[zvolin]

I've noticed that even if we make MockIbcStore generic, it will only allow for testing with custom clients, but there will still be no way of testing custom ibc implementation with ibc-testkit. Maybe we could make it generic over both client state and context? This way, we would no longer need to make MockIbcStore itself generic and use it as a context only for currently implemented hosts, but allow defining new ones with custom context.

[rnbguy]

Hey. Appreciate that you looked into this. Since, it is getting complicated - wait for us to wrap up the fix at our side.

we make MockIbcStore generic, it will only allow for testing with custom clients, but there will still be no way of testing custom ibc implementation with ibc-testkit.

This is a good point. We did this way so that we can test the ibc-rs stack by mocking different hosts and their light client implementation. We can think about your suggestion as a separate issue later.

I am curious, since I know, you're working on IBC integration in Sovereign - what kind of custom IBC implementation you're using? If you follow our latest work, we don't have any changes to IBC-rs. And our integration tests are here: https://github.com/informalsystems/sovereign-ibc/tree/main/crates/test/sov-ibc-mocks

[zvolin]

Maybe I wasn't exactly clear about my wording there. I would like to be able to test the ibc implementation with the storage native to the blockchain that will use it, so here is where MockIbcStore gets in the way.

I'm aware of the mocks you linked, I just thought that the purpose of ibc-testkit is to provide a set of pre-defined testcases to test custom clients and ibc applications. I got that impression because the test functions are publicly exposed.

Thanks for all the context and support

[rnbguy]

hey @zvolin I am closing your issue in favor of the tracking issue. Going ahead, let's discuss at the tracking issue. I listed down the list of changes that needs to be done after discussing with you :pray: If I missed anything or if you come across anything else - please let me know.