x/ibc Make 02-client more modular

[colin-axner]

Summary

x/ibc/02-client should be as modular as possible and not rely on needing to know a ClientType in order to execute custom logic or it should at least provide some default type which is modular enough to use for the majority of use cases. Requiring every new client to update code in 02-client is not practical and limiting to use cases. Any IBC application should be able to define their own client without needing to modify 02-client code in a similar fashion that any SDK application can define their own custom ante handler logic without needing to modify the SDK ante handler code.

Problem Definition

Currently, when constructing a client that isn't TM based or a localhost, code needs to be added to 02-client to define a ClientType and add custom logic to handle that client type. This may be fine for the first few clients, but what if you have 5, 10, 15 different clients? In this case, you might as well have all clients live as a subpkg in 02-client.

Proposal

From my limited understanding, a lot of the custom logic can be packaged into a single interface call and the responsibility pushed onto the client developer. For example, the switch in x/ibc/02-client/handler.go is essentially requiring custom logic to create a new client state based on the client type. This could be modified into an interface call by adding

GetClientState(chainID string, height uint64) ClientState

to MsgCreateClient

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[fedekunze]

I'm in favor of this refactor. Just note that the clients would need to be initialized in GetClientState instead of the subpkg. 👍

[colin-axner]

After looking into this a bit more I don't think the proposed solution will reasonably work. In order to create Localhost type client, read access to the client store for that client id is required as seen here. My worry with trying to create a interface function call to fulfill the roll of initializing a client and updating a client is providing access to the right amount of information. It would probably be hard to create an open yet restrictive amount of provided arguments to fulfill this role.

Perhaps it would be better to allow special client types with special needs to have corresponding case handling, but also provide a generic client type with a general interface call that can be implemented by outside developers. I imagine access to sdk.Context would probably suffice for initializing a client and access to sdk.Context, Header and clientState would suffice for updating the client state.

I'm not sure how useful this would be since it is hard to predict what sort of clients would be implemented outside of those proposed in the ICS. I'm just worried about the bottleneck that is 02-client and pre 1.0 release is a good time period to make significant refactors such as this.

However, introduction of generic type does seem like a non-breaking change that could be introduced down the line if demand arises.

[cwgoes]

I think we will need to choose a unified interface for all client types. If that means that arguments are provided which some client types do not use, that is unfortunate, but paying that cost is worth standardisation - both on the spec & implementation side.

[colin-axner]

I don't know how discouraged it is to pass around the ctx, but I think it would allow enough info to be passed around for a large number of clients to be made.

Interface func additions:

type MsgCreateClient interface {
    InitializeClientState(ctx sdk.Context) clientexported.ClientState
}

type ClientState interface {
    CheckValidityAndUpdateState(ctx sdk.Context, header Header) (ClientState, ConsensusState, error)
    CheckMisbehaviourAndUpdateState(ctx sdk.Context, consensusState ConsensusState, misbehaviour Misbehaviour) (ClientState, error)
}

[fedekunze]

Why do you need to pass a context to the MsgCreateClient?

[colin-axner]

Localhost needs the chain-id and block height. But I do agree it is probably unnecessary for all other cases. Chain-ID seems like it could moved to a message field provided by the client, but I'm not sure about block height

[fedekunze]

what about passing them as parameters? I checked ICS6 (solo machine client) and ICS10 (GRANDPA client) and they seem to be the only two common fields required from the context

InitializeClientState(chainID string, height uint64) clientexported.ClientState

[AdityaSripal]

Is anyone looking into this @colin-axner @fedekunze ? I can take this on if not

[fedekunze]

@AdityaSripal go ahead

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.