chore: pull out get light client module to helper methods

[gjermundgaraba]

Description

Start of #6019
In order to keep the PRs for this task a little more digestible, I wanted to do this small refactor first.

And I wanted to ask if I should also use the helper method in other places where we currently do not do any if !k.GetParams(ctx).IsAllowedClient(clientType) checks.

The places I have identified that currently gets the route, but does not run IsAllowedClient:

• ibc-go/modules/core/02-client/keeper/keeper.go

  Lines 307 to 314 in dd23cf9

  	func (k *Keeper) GetLatestClientConsensusState(ctx sdk.Context, clientID string) (exported.ConsensusState, bool) {
  	clientModule, found := k.router.GetRoute(clientID)
  	if !found {
  	return nil, false
  	}
  	return k.GetClientConsensusState(ctx, clientID, clientModule.LatestHeight(ctx, clientID))
  	}

• ibc-go/modules/core/02-client/keeper/keeper.go

  Lines 395 to 411 in dd23cf9

  	func (k *Keeper) GetClientStatus(ctx sdk.Context, clientID string) exported.Status {
  	clientType, _, err := types.ParseClientIdentifier(clientID)
  	if err != nil {
  	return exported.Unauthorized
  	}
  	if !k.GetParams(ctx).IsAllowedClient(clientType) {
  	return exported.Unauthorized
  	}
  	clientModule, found := k.router.GetRoute(clientID)
  	if !found {
  	return exported.Unauthorized
  	}
  	return clientModule.Status(ctx, clientID)
  	}

• ibc-go/modules/core/02-client/keeper/keeper.go

  Lines 415 to 436 in dd23cf9

  	func (k *Keeper) GetClientLatestHeight(ctx sdk.Context, clientID string) types.Height {
  	clientType, _, err := types.ParseClientIdentifier(clientID)
  	if err != nil {
  	return types.ZeroHeight()
  	}
  	if !k.GetParams(ctx).IsAllowedClient(clientType) {
  	return types.ZeroHeight()
  	}
  	clientModule, found := k.router.GetRoute(clientID)
  	if !found {
  	return types.ZeroHeight()
  	}
  	var latestHeight types.Height
  	latestHeight, ok := clientModule.LatestHeight(ctx, clientID).(types.Height)
  	if !ok {
  	panic(fmt.Errorf("cannot convert %T to %T", clientModule.LatestHeight, latestHeight))
  	}
  	return latestHeight
  	}

• ibc-go/modules/core/02-client/keeper/client.go

  Lines 63 to 121 in dd23cf9

  	func (k *Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exported.ClientMessage) error {
  	if status := k.GetClientStatus(ctx, clientID); status != exported.Active {
  	return errorsmod.Wrapf(types.ErrClientNotActive, "cannot update client (%s) with status %s", clientID, status)
  	}
  	clientType, _, err := types.ParseClientIdentifier(clientID)
  	if err != nil {
  	return errorsmod.Wrapf(err, "unable to parse client identifier %s", clientID)
  	}
  	clientModule, found := k.router.GetRoute(clientID)
  	if !found {
  	return errorsmod.Wrap(types.ErrRouteNotFound, clientID)
  	}
  	if err := clientModule.VerifyClientMessage(ctx, clientID, clientMsg); err != nil {
  	return err
  	}
  	foundMisbehaviour := clientModule.CheckForMisbehaviour(ctx, clientID, clientMsg)
  	if foundMisbehaviour {
  	clientModule.UpdateStateOnMisbehaviour(ctx, clientID, clientMsg)
  	k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID)
  	defer telemetry.IncrCounterWithLabels(
  	[]string{"ibc", "client", "misbehaviour"},
  	1,
  	[]metrics.Label{
  	telemetry.NewLabel(types.LabelClientType, clientType),
  	telemetry.NewLabel(types.LabelClientID, clientID),
  	telemetry.NewLabel(types.LabelMsgType, "update"),
  	},
  	)
  	emitSubmitMisbehaviourEvent(ctx, clientID, clientType)
  	return nil
  	}
  	consensusHeights := clientModule.UpdateState(ctx, clientID, clientMsg)
  	k.Logger(ctx).Info("client state updated", "client-id", clientID, "heights", consensusHeights)
  	defer telemetry.IncrCounterWithLabels(
  	[]string{"ibc", "client", "update"},
  	1,
  	[]metrics.Label{
  	telemetry.NewLabel(types.LabelClientType, clientType),
  	telemetry.NewLabel(types.LabelClientID, clientID),
  	telemetry.NewLabel(types.LabelUpdateType, "msg"),
  	},
  	)
  	// emitting events in the keeper emits for both begin block and handler client updates
  	emitUpdateClientEvent(ctx, clientID, clientType, consensusHeights, k.cdc, clientMsg)
  	return nil
  	}

Should they also do this?

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against the correct branch (see CONTRIBUTING.md).
• Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
• Code follows the module structure standards and Go style guide.
• Wrote unit and integration tests.
• Updated relevant documentation (docs/).
• Added relevant godoc comments.
• Provide a conventional commit message to follow the repository standards.
• Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
• Re-reviewed Files changed in the GitHub PR explorer.
• Review SonarCloud Report in the comment section below once CI passes.

[crodriguezvega]

I was thinking that it might be worth to have feature branch for this, if it's going to span multiple PRs. We have still the benefit of reviewing smaller PRs, but at the same time we also have another opportunity to review how each piece fits together when reviewing the feature branch. What do you think?

[gjermundgaraba]

I was thinking that it might be worth to have feature branch for this, if it's going to span multiple PRs. We have still the benefit of reviewing smaller PRs, but at the same time we also have another opportunity to review how each piece fits together when reviewing the feature branch. What do you think?

Yeah, happy to do that! I will create a feature branch and target that one instead :)

Edit: done! feat/refactor-client-route-to-verify

[crodriguezvega]

I'd say that it makes sense to also do the IsAllowedClient check in the other functions...

[sonarcloud]

Quality Gate passed for 'ibc-go'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[crodriguezvega]

Trying to continue the work of Gjermund while he is away...

[crodriguezvega]

Linter was complaining about this.

[crodriguezvega]

I had to leave the parsing of the client ID above, because the client type is used in the telemetry below. Same thing in UpdateClient. If somebody has a proposal to get rid of it (since it is also done in getLightClientModule), I'm all ears.

[chatton]

LGTM!

I also think it makes sense to include the IsAllowed check anywhere we are fetching the client module.