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.

Sign up for GitHub

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

chore: Address linter findings in testing/ #6150

Merged
merged 6 commits into from
Apr 15, 2024
Merged

chore: Address linter findings in testing/ #6150

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f33576b
Address lint findings in testing/
bznein Apr 12, 2024
d8e729d
Merge branch 'cosmos:main' into main
bznein Apr 12, 2024
c2b728a
Remove nolint directive
bznein Apr 12, 2024
76ec265
Merge branch 'main' into main
bznein Apr 12, 2024
aa4fff2
Merge branch 'cosmos:main' into main
bznein Apr 15, 2024
8c9695d
Merge branch 'main' into main
DimitrisJim Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion testing/chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,8 @@ func (chain *TestChain) QueryUpgradeProof(key []byte, height uint64) ([]byte, cl
// QueryConsensusStateProof performs an abci query for a consensus state
// stored on the given clientID. The proof and consensusHeight are returned.
func (chain *TestChain) QueryConsensusStateProof(clientID string) ([]byte, clienttypes.Height) {
consensusHeight := chain.GetClientLatestHeight(clientID).(clienttypes.Height)
consensusHeight, ok := chain.GetClientLatestHeight(clientID).(clienttypes.Height)
require.True(chain.TB, ok)
consensusKey := host.FullConsensusStateKey(clientID, consensusHeight)
consensusProof, _ := chain.QueryProof(consensusKey)

Expand Down
13 changes: 9 additions & 4 deletions testing/endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ func (endpoint *Endpoint) CreateClient() (err error) {
tmConfig, ok := endpoint.ClientConfig.(*TendermintConfig)
require.True(endpoint.Chain.TB, ok)

height := endpoint.Counterparty.Chain.LatestCommittedHeader.GetHeight().(clienttypes.Height)
height, ok := endpoint.Counterparty.Chain.LatestCommittedHeader.GetHeight().(clienttypes.Height)
require.True(endpoint.Chain.TB, ok)
Comment on lines +98 to +99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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")

clientState = ibctm.NewClientState(
endpoint.Counterparty.Chain.ChainID, tmConfig.TrustLevel, tmConfig.TrustingPeriod, tmConfig.UnbondingPeriod, tmConfig.MaxClockDrift,
height, commitmenttypes.GetSDKSpecs(), UpgradePath)
Expand Down Expand Up @@ -138,7 +139,8 @@ func (endpoint *Endpoint) UpdateClient() (err error) {

switch endpoint.ClientConfig.GetClientType() {
case exported.Tendermint:
trustedHeight := endpoint.GetClientLatestHeight().(clienttypes.Height)
trustedHeight, ok := endpoint.GetClientLatestHeight().(clienttypes.Height)
require.True(endpoint.Chain.TB, ok)
Comment on lines +142 to +143
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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")

header, err = endpoint.Counterparty.Chain.IBCClientHeader(endpoint.Counterparty.Chain.LatestCommittedHeader, trustedHeight)
default:
err = fmt.Errorf("client type %s is not supported", endpoint.ClientConfig.GetClientType())
Expand Down Expand Up @@ -168,7 +170,8 @@ func (endpoint *Endpoint) UpgradeChain() error {
}

clientState := endpoint.Counterparty.GetClientState()
tmClientState := clientState.(*ibctm.ClientState)
tmClientState, ok := clientState.(*ibctm.ClientState)
require.True(endpoint.Chain.TB, ok)
Comment on lines +173 to +174
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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")


// increment revision number in chainID
oldChainID := tmClientState.ChainId
Expand Down Expand Up @@ -306,7 +309,9 @@ func (endpoint *Endpoint) QueryConnectionHandshakeProof() (
clientKey := host.FullClientStateKey(endpoint.Counterparty.ClientID)
clientProof, proofHeight = endpoint.Counterparty.QueryProof(clientKey)

consensusHeight = endpoint.Counterparty.GetClientLatestHeight().(clienttypes.Height)
var ok bool
consensusHeight, ok = endpoint.Counterparty.GetClientLatestHeight().(clienttypes.Height)
require.True(endpoint.Chain.TB, ok)
Comment on lines +312 to +314
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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")

// query proof for the consensus state on the counterparty
consensusKey := host.FullConsensusStateKey(endpoint.Counterparty.ClientID, consensusHeight)
consensusProof, _ = endpoint.Counterparty.QueryProofAtHeight(consensusKey, proofHeight.GetRevisionHeight())
Expand Down
12 changes: 8 additions & 4 deletions testing/simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ import (
"github.com/cosmos/cosmos-sdk/x/bank"
bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
consensus "github.com/cosmos/cosmos-sdk/x/consensus"
"github.com/cosmos/cosmos-sdk/x/consensus"
consensusparamkeeper "github.com/cosmos/cosmos-sdk/x/consensus/keeper"
consensusparamtypes "github.com/cosmos/cosmos-sdk/x/consensus/types"
"github.com/cosmos/cosmos-sdk/x/crisis"
Expand Down Expand Up @@ -117,8 +117,8 @@ import (
ibcfee "github.com/cosmos/ibc-go/v8/modules/apps/29-fee"
ibcfeekeeper "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/keeper"
ibcfeetypes "github.com/cosmos/ibc-go/v8/modules/apps/29-fee/types"
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
Copy link
Contributor Author

@bznein bznein Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code gods works in mysterious ways 🕯️

ibctransfertypes "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
ibc "github.com/cosmos/ibc-go/v8/modules/core"
ibcclient "github.com/cosmos/ibc-go/v8/modules/core/02-client"
Expand Down Expand Up @@ -523,7 +523,11 @@ func NewSimApp(
// initialize ICA module with mock module as the authentication module on the controller side
var icaControllerStack porttypes.IBCModule
icaControllerStack = ibcmock.NewIBCModule(&mockModule, ibcmock.NewIBCApp("", scopedICAMockKeeper))
app.ICAAuthModule = icaControllerStack.(ibcmock.IBCModule)
var ok bool
app.ICAAuthModule, ok = icaControllerStack.(ibcmock.IBCModule)
if !ok {
panic(fmt.Errorf("cannot convert %T into %T", icaControllerStack, app.ICAAuthModule))
}
Comment on lines +526 to +530
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

icaControllerStack = icacontroller.NewIBCMiddleware(icaControllerStack, app.ICAControllerKeeper)
icaControllerStack = ibcfee.NewIBCMiddleware(icaControllerStack, app.IBCFeeKeeper)

Expand Down
2 changes: 1 addition & 1 deletion testing/simapp/upgrades/upgrades.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

clientkeeper "github.com/cosmos/ibc-go/v8/modules/core/02-client/keeper"
"github.com/cosmos/ibc-go/v8/modules/core/exported"
ibctmmigrations "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint/migrations"
Expand Down
Loading