Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PoC for v2 02-client routing #5565

Closed
3 tasks
colin-axner opened this issue Jan 10, 2024 · 0 comments · Fixed by #5806
Closed
3 tasks

PoC for v2 02-client routing #5565

colin-axner opened this issue Jan 10, 2024 · 0 comments · Fixed by #5806
Assignees
Labels
02-client type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.

Comments

@colin-axner
Copy link
Contributor

Summary

In order to close #5084, I propose we start with a PoC which sets the general design for v2 routing within 02-client. Tests can be commented out and the design can be reviewed together on a synchronous call, this can then be used as the base of the feature branch to close #5084.

There are 2 breaking change considerations to make when resturcturing 02-client routing

  1. Breaking changes for light client developers
  2. Breaking changes for relayers

As we are introducing a v2 API, both light client developers and relayers will eventually need to update to the changes, but we can do our best to limit disruption when possible.

If we attempt to provide backwards compatible support for light client developers, the approach is quite straight forward within 02-client:

  lightClientModule, found := k.router.GetRoute(clientState.ClientType())
  if !found {
    // v1 routing
    if err := clientState.Initialize(ctx, k.cdc, clientStore, consensusState); err != nil {
        return "", err
    }
  } else {
    // v2 routing
    if err := lightClientModule.Initialize(ctx, clientState, consensusState); err != nil {
        return "", err
    }
  }

Outside of 02-client it becomes a little trickier as we won't have direct access to the router. This brings up the general question of how to handle requests to the 02-client from external modules. See my issue comment.

It's possible to use an initial approach which creates a public GetLightClientModule public function so that the 03-connection can follow a similar pattern to above. Another approach is to standardize light client module interface functions into the 02-client msg server and grpc query server. This way external modules could request 02-client to pass through a msg request based on the client ID in the msg and the type of msg provided. So a MsgVerifyMembership would be delivered to the light client module associated with the clientID provided in the message. There's a minor roadblock over the discussion of whether VerifyMembership should be a msg service action or query only as only the solo machine requires access to state changes in that function.

  lightClientModule, found := k.clientKeeper.GetLightClientModule(clientID)
  if !found {
    // v1 routing
    if err := clientState.VerifyMembership(
        ctx, clientStore, k.cdc, height,
        0, 0, // skip delay period checks for non-packet processing verification
        proof, merklePath, bz,
    ); err != nil {
        return errorsmod.Wrapf(err, "failed connection state verification for client (%s)", clientID)
    }
  } else {
    // v2 routing
    if err := lightClientModule.VerifyMembership(ctx, clientID, 0, 0, proof, height, merklePath, bz); err != nil {
        return err
    }
  }
  // alternative approach
  msgVerifyMembership := clienttypes.MsgVerifyMembership{
    ClientID:    clientID,
    Proof:       proof,
    ProofHeight: height,
    Path:        path,
    Value:       bz,
    BlockDelay:  0,
    TimeDelay:   0,
  }

  err := k.clientKeeper.VerifyMembership(ctx, msgVerifyMembership)
  if errorsmod.ErrorIs(ErrUnsupported) {
    // v1 routing
    if err := clientState.VerifyMembership(
        ctx, clientStore, k.cdc, height,
        0, 0, // skip delay period checks for non-packet processing verification
        proof, merklePath, bz,
    ); err != nil {
        return errorsmod.Wrapf(err, "failed connection state verification for client (%s)", clientID)
    }
  }

  return err

As we see, this begins to become quite cumbersome, particularly because we tend to reference calls to light client modules in the middle of handlers and sometimes multiple times in the same handler. We could duplicate all these functions or create many if/else statements, but given the difficulty in cleanly implementing backwards compatible support, it might be better to question whether light client modules are okay with breaking changes.

As it stands today, there are not many standalone light client modules, in fact the majority are likely hosted in ibc-go. Given that the proposed changes increase flexibility for light client developers, there is little reason to not update to v2 aside from not needing the new changes in the immediate term. An alternative approach is to break the light client interface, by forcing light client modules to adopt v2 routing support. This would imply that light client developers should begin by copy/pasting the glue code which allows them to implement the v2 API while calling into their old API. This would likely be a light_client_module.go file which implements each interface function by calling into the client and consensus states respectively. Once the developers have a moment, they could readjust their code to take advantage of the new flexibility as they wish (thus extra glue code). Essentially, if light client developers wish to use the old API, they may host the glue code as it is an unnecessary burden on core IBC.

I propose we do this for all our light clients immediately by breaking the light client interface, providing a quick guide for external devs who wish to limit disruption. We can then refactor only our light clients which immediately benefit from v2 which is primarily the 08-wasm client.

In order to limit disruption to relayers, we will need to add the v2 msgs alongside v1. v1 msgs should be capable of calling into the v2 light client module API. The v2 msgs are simply removing unnecessary logic for calling into the v2 API. Once we have confirmation that relayers have switched to the v2 msg types, we may remove the v1 msg types and any remaining logic with them.

To achieve this, we will need to leave the ClientState interface with a single function: ClientType(), this is because on CreateClient, the v2 msg type will include the necessary client type which cannot be obtained from the client state once the client state interface is removed. It should be possible to remove the consensus state interface.

I propose the PoC be developed as follows:

  1. Copy over the existing client state and consensus state interfaces into the light client module interface. Certain functions will now need to be provided the client ID and the client state, consensus state. Remove unnecessary arguments from the interface (light client modules should be initialized with their own prefixed store key, they can use the clientID to access it). The codec can be removed.

  2. Add basic router access to 02-client (simplified 05-port router). A later pr can try to turn the 05-port router into a generic router.

  3. Implement the new light client module interface for each of the client implementations

  4. Replace v1 routing with v2 routing, using the clientState.ClientType() on create client, and client ID on other handlers

    • remove interface functions from client/consensus states/client message when possible
  5. Add v2 msg server. Modify so that v1 and v2 call into same handlers. Create client sub handler can take in client type, client state (bytes), consensus state (bytes). Legacy msg handler uses client state interface to get client type

(PoC ends here, upon agreement via walkthrough call begin writing tests/docs)

  1. Remove v1 msg server once relayers are updated to handle new API. Remove client state interface and codec registrations as necessary.
Bonus consideration

Create new go.mod for new API so we don't have breakage for every new ibc-go version?


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@crodriguezvega crodriguezvega moved this to Todo in ibc-go Jan 10, 2024
@damiannolan damiannolan moved this from Todo to In progress in ibc-go Jan 29, 2024
@crodriguezvega crodriguezvega added the type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. label Feb 8, 2024
@github-project-automation github-project-automation bot moved this from In progress 👷 to Done 🥳 in ibc-go Mar 14, 2024
@crodriguezvega crodriguezvega mentioned this issue Jun 5, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02-client type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants