imp: remove Route method in favour of Verify(Non)Membership on client keeper
#6760
Conversation
* chore: pull out get light client module to helper methods * another place to use getLightClientModuleRoute * use getLightClientModule in other 02-client functions * lint --------- Co-authored-by: Carlos Rodriguez <[email protected]>
crodriguezvega
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
# Conflicts: # modules/core/03-connection/keeper/verify.go # modules/light-clients/09-localhost/client_state.go
| @@ -16,7 +16,7 @@ type ClientKeeper interface { | |||
| GetClientConsensusState(ctx sdk.Context, clientID string, height exported.Height) (exported.ConsensusState, bool) | |||
| GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) | |||
| ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error | |||
| VerifyMembershipp(ctx sdk.Context, clientID string, height exported.Height, delayTimePeriod uint64, delayBlockPeriod uint64, proof []byte, path exported.Path, value []byte) error | |||
| VerifyMembershipProof(ctx sdk.Context, clientID string, height exported.Height, delayTimePeriod uint64, delayBlockPeriod uint64, proof []byte, path exported.Path, value []byte) error | |||
| VerifyNonMembership(ctx sdk.Context, clientID string, height exported.Height, delayTimePeriod uint64, delayBlockPeriod uint64, proof []byte, path exported.Path) error | |||
There was a problem hiding this comment.
Should we call this VerifyNonMembershipProof for now so that the names align?
There was a problem hiding this comment.
Since we will (hopefully) rename the other one back to VerifyMembership before we release, I thought of keeping it just like this.
|
assigned ya too Damian, feel free to remove if no bandwidth |
| if !k.GetParams(ctx).IsAllowedClient(clientType) { | ||
| return nil, errorsmod.Wrapf( | ||
| types.ErrInvalidClientType, | ||
| "client (%s) type %s is not in the allowed client list", clientID, clientType, | ||
| ) | ||
| } | ||
|
|
||
| clientModule, found := k.router.GetRoute(clientID) | ||
| if !found { | ||
| return nil, errorsmod.Wrap(types.ErrRouteNotFound, clientID) | ||
| } |
There was a problem hiding this comment.
I feel like this params check could be moved into router.GetRoute().
We already parse the clientID in there, so the changes would be to accept a context and return and error rather than a boolean. I'd be in favour of doing this in a following PR. I can create an issue if others are onboard
There was a problem hiding this comment.
how would this jive with registering them in app.go? Wouldn't you need to run extra logic when updating params just to register the newly allowed light client module?
Co-authored-by: DimitrisJim <[email protected]>
|
Description
closes: #6019
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.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.