chore: Address linter findings on core/02-client #6119
Conversation
bznein
requested review from
colin-axner,
AdityaSripal,
damiannolan and
chatton
as code owners
WalkthroughThis update focuses on enhancing the robustness, type safety, and maintainability of the Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (12)
- modules/core/02-client/keeper/client_test.go (7 hunks)
- modules/core/02-client/keeper/events_test.go (2 hunks)
- modules/core/02-client/keeper/grpc_query_test.go (5 hunks)
- modules/core/02-client/keeper/keeper.go (1 hunks)
- modules/core/02-client/keeper/migrations.go (1 hunks)
- modules/core/02-client/migrations/v7/genesis_test.go (2 hunks)
- modules/core/02-client/migrations/v7/localhost_test.go (1 hunks)
- modules/core/02-client/migrations/v7/store_test.go (2 hunks)
- modules/core/02-client/types/client.go (1 hunks)
- modules/core/02-client/types/client_test.go (1 hunks)
- modules/core/02-client/types/codec.go (1 hunks)
- modules/core/02-client/types/legacy_proposal_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- modules/core/02-client/types/codec.go
Additional comments not posted (13)
modules/core/02-client/migrations/v7/localhost_test.go (1)
4-4: Updated import path aligns with the new module version. This change is necessary for correct module referencing.modules/core/02-client/types/client_test.go (1)
30-31: Adding type assertion with error handling forlatestHeightimproves the robustness of the test by ensuring the type conversion is successful before proceeding.modules/core/02-client/types/legacy_proposal_test.go (1)
77-78: Adding type assertion with error handling forproposalenhances the test's safety by ensuring the conversion totypes.ClientUpdateProposalis successful before marshaling.modules/core/02-client/keeper/events_test.go (2)
21-22: Adding type assertion with error handling forheightensures the conversion is successful before proceeding, enhancing the test's reliability.
60-61: Adding type assertion with error handling forclientStateensures the conversion is successful before proceeding, enhancing the test's reliability.modules/core/02-client/types/client.go (1)
9-9: The import path update for theprotopackage is correct and necessary for proper package referencing.modules/core/02-client/migrations/v7/genesis_test.go (2)
10-10: Updated import path aligns with the new module version. This change is necessary for correct module referencing.
70-71: Adding a codec type assertion with error handling ensures the codec is of the expected type before proceeding with interface registration, enhancing code safety.modules/core/02-client/migrations/v7/store_test.go (2)
11-11: Updated import path aligns with the new module version. This change is necessary for correct module referencing.
107-108: Adding a codec type assertion with error handling ensures the codec is of the expected type before proceeding with interface registration, enhancing code safety.modules/core/02-client/keeper/grpc_query_test.go (3)
344-351: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [347-359]
LGTM! Adding type assertions and checks in tests enhances reliability by verifying type conversions before usage.
552-557: LGTM! Properly handling type assertions in tests ensures that the test logic is based on correct assumptions.
571-572: LGTM! The use of type assertions followed by checks in tests is a good practice for ensuring type safety.
| clientModuleHeight, ok := clientModule.LatestHeight(ctx, clientID).(types.Height) | ||
| if !ok { | ||
| panic("can't convert clientModule.LatestHeight to types.Height") | ||
| } |
There was a problem hiding this comment.
Consider returning an error instead of panicking for type assertion failure.
Panicking can lead to abrupt termination of applications using this library. It's generally more robust to return an error and allow the caller to decide how to handle it.
- panic("can't convert clientModule.LatestHeight to types.Height")
+ return types.ZeroHeight(), errors.New("can't convert clientModule.LatestHeight to types.Height")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| clientModuleHeight, ok := clientModule.LatestHeight(ctx, clientID).(types.Height) | |
| if !ok { | |
| panic("can't convert clientModule.LatestHeight to types.Height") | |
| } | |
| clientModuleHeight, ok := clientModule.LatestHeight(ctx, clientID).(types.Height) | |
| if !ok { | |
| return types.ZeroHeight(), errors.New("can't convert clientModule.LatestHeight to types.Height") | |
| } |
| @@ -422,7 +422,11 @@ func (k Keeper) GetClientLatestHeight(ctx sdk.Context, clientID string) types.He | |||
| return types.ZeroHeight() | |||
| } | |||
|
|
|||
| return clientModule.LatestHeight(ctx, clientID).(types.Height) | |||
| clientModuleHeight, ok := clientModule.LatestHeight(ctx, clientID).(types.Height) | |||
There was a problem hiding this comment.
| clientModuleHeight, ok := clientModule.LatestHeight(ctx, clientID).(types.Height) | |
| latestHeight, ok := clientModule.LatestHeight(ctx, clientID).(types.Height) |
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Description
Addresses linting issues (mostly unchecked type assertions and redundant imports) in core/02-client.
ref: #6086
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.Summary by CodeRabbit