chore: Address linter findings in testing/ #6150
Conversation
bznein
requested review from
colin-axner,
AdityaSripal,
crodriguezvega,
charleenfei,
damiannolan,
chatton and
DimitrisJim
as code owners
WalkthroughThe changes encompass improving error handling, enhancing type assertion checks, and adjusting import paths and initialization processes across various testing files to bolster the resilience and adaptability of the testing framework. 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 (
|
testing/simapp/app.go
Outdated
| transfer "github.com/cosmos/ibc-go/v8/modules/apps/transfer" | ||
| ibctransferkeeper "github.com/cosmos/ibc-go/v8/modules/apps/transfer/keeper" | ||
| "github.com/cosmos/ibc-go/v8/modules/apps/transfer" | ||
| ibctransferkeeper "github.com/cosmos/ibc-go/v8/modules/apps/transfer/keeper" //nolint |
There was a problem hiding this comment.
This is an interesting one that left me stumped!
If I remove the type alias, goimports will remove the import saying it is unused (?!) and it won't build)
If I replace it with _, the code will not build because ibctransferkeeper is not found
If I don't do anything, the linter will complain ¯\_(ツ)_/¯
So I would be very happy if anyone would like to help me understand what is going on :)
There was a problem hiding this comment.
hm, with these imports it works fine locally:
"github.com/cosmos/ibc-go/v8/modules/apps/transfer"
ibctransferkeeper "github.com/cosmos/ibc-go/v8/modules/apps/transfer/keeper"you mean goimports would straight up remove the first import if this is the case?
There was a problem hiding this comment.
Well, this is weird. I tried to repro the issue (which happened at line 121) but... it's not happening anymore. No idea what was happening, but thank you!
There was a problem hiding this comment.
the code gods works in mysterious ways 🕯️
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -15,7 +15,7 @@ import ( | |||
| paramstypes "github.com/cosmos/cosmos-sdk/x/params/types" | |||
|
|
|||
| capabilitykeeper "github.com/cosmos/ibc-go/modules/capability/keeper" | |||
| v6 "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/migrations/v6" | |||
| "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/migrations/v6" | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed that the old import path github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/migrations/v6 is still being used in two files: testing/simapp/upgrades/upgrades.go and modules/apps/27-interchain-accounts/controller/migrations/v6/migrations_test.go. This indicates that the import path update has not been fully propagated throughout the codebase, which could lead to inconsistencies or import errors.
Given this finding, it is recommended to update all references to the old import path across the entire codebase to ensure consistency and prevent potential issues related to outdated imports.
Analysis chain
The import path for the migration module related to interchain accounts has been updated to remove the specific version reference v6. This change likely aims to simplify the import structure or prepare for future upgrades. Ensure that this change is reflected across all files that might use this module to avoid import errors.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any files that might still be using the old import path.
rg --type go 'github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/migrations/v6'
Length of output: 403
| height, ok := endpoint.Counterparty.Chain.LatestCommittedHeader.GetHeight().(clienttypes.Height) | ||
| require.True(endpoint.Chain.TB, ok) |
There was a problem hiding this comment.
Ensure proper error handling for type assertion failures.
- require.True(endpoint.Chain.TB, ok)
+ require.True(endpoint.Chain.TB, ok, "failed to assert type as clienttypes.Height")Adding a descriptive error message will help in debugging if the type assertion fails.
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.
| height, ok := endpoint.Counterparty.Chain.LatestCommittedHeader.GetHeight().(clienttypes.Height) | |
| require.True(endpoint.Chain.TB, ok) | |
| height, ok := endpoint.Counterparty.Chain.LatestCommittedHeader.GetHeight().(clienttypes.Height) | |
| require.True(endpoint.Chain.TB, ok, "failed to assert type as clienttypes.Height") |
| trustedHeight, ok := endpoint.GetClientLatestHeight().(clienttypes.Height) | ||
| require.True(endpoint.Chain.TB, ok) |
There was a problem hiding this comment.
Ensure proper error handling for type assertion failures.
- require.True(endpoint.Chain.TB, ok)
+ require.True(endpoint.Chain.TB, ok, "failed to assert type as clienttypes.Height")Adding a descriptive error message will help in debugging if the type assertion fails.
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.
| trustedHeight, ok := endpoint.GetClientLatestHeight().(clienttypes.Height) | |
| require.True(endpoint.Chain.TB, ok) | |
| trustedHeight, ok := endpoint.GetClientLatestHeight().(clienttypes.Height) | |
| require.True(endpoint.Chain.TB, ok, "failed to assert type as clienttypes.Height") |
| tmClientState, ok := clientState.(*ibctm.ClientState) | ||
| require.True(endpoint.Chain.TB, ok) |
There was a problem hiding this comment.
Ensure proper error handling for type assertion failures.
- require.True(endpoint.Chain.TB, ok)
+ require.True(endpoint.Chain.TB, ok, "failed to assert type as *ibctm.ClientState")Adding a descriptive error message will help in debugging if the type assertion fails.
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.
| tmClientState, ok := clientState.(*ibctm.ClientState) | |
| require.True(endpoint.Chain.TB, ok) | |
| tmClientState, ok := clientState.(*ibctm.ClientState) | |
| require.True(endpoint.Chain.TB, ok, "failed to assert type as *ibctm.ClientState") |
| var ok bool | ||
| app.ICAAuthModule, ok = icaControllerStack.(ibcmock.IBCModule) | ||
| if !ok { | ||
| panic(fmt.Errorf("cannot convert %T into %T", icaControllerStack, app.ICAAuthModule)) | ||
| } |
There was a problem hiding this comment.
Type assertion for app.ICAAuthModule is correctly implemented with appropriate error handling. Consider adding a unit test to ensure that the type assertion does not fail during normal operations.
Would you like me to help with writing the unit test for this type assertion?
| var ok bool | ||
| consensusHeight, ok = endpoint.Counterparty.GetClientLatestHeight().(clienttypes.Height) | ||
| require.True(endpoint.Chain.TB, ok) |
There was a problem hiding this comment.
Ensure proper error handling for type assertion failures.
- require.True(endpoint.Chain.TB, ok)
+ require.True(endpoint.Chain.TB, ok, "failed to assert type as clienttypes.Height")Adding a descriptive error message will help in debugging if the type assertion fails.
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.
| var ok bool | |
| consensusHeight, ok = endpoint.Counterparty.GetClientLatestHeight().(clienttypes.Height) | |
| require.True(endpoint.Chain.TB, ok) | |
| var ok bool | |
| consensusHeight, ok = endpoint.Counterparty.GetClientLatestHeight().(clienttypes.Height) | |
| require.True(endpoint.Chain.TB, ok, "failed to assert type as clienttypes.Height") |
Description
Addresses all linting issues that would be found intesting/ by upgrading golanci-lint version (see linked issue)
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
Bug Fixes
chain.goandendpoint.go.Refactor
require.Trueand checking for type assertions usingokvariables.app.ICAAuthModuleto handle type conversion.