From bae1fd4c154017fecbffe2a3bf698ffeb43422da Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Mon, 8 Apr 2024 15:03:33 +0100 Subject: [PATCH 1/3] Address linter findings on core/02-client --- modules/core/02-client/keeper/client_test.go | 37 +++++++++++++------ modules/core/02-client/keeper/events_test.go | 6 ++- .../core/02-client/keeper/grpc_query_test.go | 20 ++++++---- modules/core/02-client/keeper/keeper.go | 6 ++- modules/core/02-client/keeper/migrations.go | 2 +- .../02-client/migrations/v7/genesis_test.go | 5 ++- .../02-client/migrations/v7/localhost_test.go | 2 +- .../02-client/migrations/v7/store_test.go | 5 ++- modules/core/02-client/types/client.go | 2 +- modules/core/02-client/types/client_test.go | 3 +- modules/core/02-client/types/codec.go | 2 +- .../02-client/types/legacy_proposal_test.go | 3 +- 12 files changed, 61 insertions(+), 32 deletions(-) diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go index 446fe195f03..847bdee6968 100644 --- a/modules/core/02-client/keeper/client_test.go +++ b/modules/core/02-client/keeper/client_test.go @@ -216,13 +216,15 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() { suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientConsensusState(suite.chainA.GetContext(), clientID, updateHeader.GetHeight(), conflictConsState) }, true, true}, {"misbehaviour detection: monotonic time violation", func() { - clientState := path.EndpointA.GetClientState().(*ibctm.ClientState) + clientState, ok := path.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) clientID := path.EndpointA.ClientID trustedHeight := clientState.LatestHeight // store intermediate consensus state at a time greater than updateHeader time // this will break time monotonicity - incrementedClientHeight := clientState.LatestHeight.Increment().(clienttypes.Height) + incrementedClientHeight, ok := clientState.LatestHeight.Increment().(clienttypes.Height) + suite.Require().True(ok) intermediateConsState := &ibctm.ConsensusState{ Timestamp: suite.coordinator.CurrentTime.Add(2 * time.Hour), NextValidatorsHash: suite.chainB.Vals.Hash(), @@ -238,7 +240,8 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() { updateHeader = createFutureUpdateFn(trustedHeight) }, true, true}, {"client state not found", func() { - clientState := path.EndpointA.GetClientState().(*ibctm.ClientState) + clientState, ok := path.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) updateHeader = createFutureUpdateFn(clientState.LatestHeight) path.EndpointA.ClientID = ibctesting.InvalidID @@ -247,21 +250,25 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() { clientState := path.EndpointA.GetClientState() tmClient, ok := clientState.(*ibctm.ClientState) suite.Require().True(ok) - tmClient.LatestHeight = tmClient.LatestHeight.Increment().(clienttypes.Height) + tmClient.LatestHeight, ok = tmClient.LatestHeight.Increment().(clienttypes.Height) + suite.Require().True(ok) suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID, clientState) updateHeader = createFutureUpdateFn(tmClient.LatestHeight) }, false, false}, {"client is not active", func() { - clientState := path.EndpointA.GetClientState().(*ibctm.ClientState) + clientState, ok := path.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) clientState.FrozenHeight = clienttypes.NewHeight(1, 1) suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID, clientState) updateHeader = createFutureUpdateFn(clientState.LatestHeight) }, false, false}, {"invalid header", func() { - clientState := path.EndpointA.GetClientState().(*ibctm.ClientState) + clientState, ok := path.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) updateHeader = createFutureUpdateFn(clientState.LatestHeight) - updateHeader.TrustedHeight = updateHeader.TrustedHeight.Increment().(clienttypes.Height) + updateHeader.TrustedHeight, ok = updateHeader.TrustedHeight.Increment().(clienttypes.Height) + suite.Require().True(ok) }, false, false}, } @@ -275,8 +282,10 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() { tc.malleate() var clientState *ibctm.ClientState + var ok bool if tc.expPass { - clientState = path.EndpointA.GetClientState().(*ibctm.ClientState) + clientState, ok = path.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) } err := suite.chainA.App.GetIBCKeeper().ClientKeeper.UpdateClient(suite.chainA.GetContext(), path.EndpointA.ClientID, updateHeader) @@ -284,7 +293,8 @@ func (suite *KeeperTestSuite) TestUpdateClientTendermint() { if tc.expPass { suite.Require().NoError(err, err) - newClientState := path.EndpointA.GetClientState().(*ibctm.ClientState) + newClientState, ok := path.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) if tc.expFreeze { suite.Require().True(!newClientState.FrozenHeight.IsZero(), "client did not freeze after conflicting header was submitted to UpdateClient") @@ -463,7 +473,8 @@ func (suite *KeeperTestSuite) TestUpgradeClient() { path = ibctesting.NewPath(suite.chainA, suite.chainB) path.SetupClients() - clientState := path.EndpointA.GetClientState().(*ibctm.ClientState) + clientState, ok := path.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) revisionNumber := clienttypes.ParseChainID(clientState.ChainId) newChainID, err := clienttypes.SetRevisionNumber(clientState.ChainId, revisionNumber+1) @@ -589,7 +600,8 @@ func (suite *KeeperTestSuite) TestRecoverClient() { func() { tmClientState, ok := subjectClientState.(*ibctm.ClientState) suite.Require().True(ok) - tmClientState.LatestHeight = substituteClientState.(*ibctm.ClientState).LatestHeight.Increment().(clienttypes.Height) + tmClientState.LatestHeight, ok = substituteClientState.(*ibctm.ClientState).LatestHeight.Increment().(clienttypes.Height) + suite.Require().True(ok) suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), subject, tmClientState) }, clienttypes.ErrInvalidHeight, @@ -665,7 +677,8 @@ func (suite *KeeperTestSuite) TestRecoverClient() { // Assert that client status is now Active clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), subjectPath.EndpointA.ClientID) - tmClientState := subjectPath.EndpointA.GetClientState().(*ibctm.ClientState) + tmClientState, ok := subjectPath.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) suite.Require().Equal(tmClientState.Status(suite.chainA.GetContext(), clientStore, suite.chainA.App.AppCodec()), exported.Active) } else { diff --git a/modules/core/02-client/keeper/events_test.go b/modules/core/02-client/keeper/events_test.go index 45423239e98..26252de916e 100644 --- a/modules/core/02-client/keeper/events_test.go +++ b/modules/core/02-client/keeper/events_test.go @@ -18,7 +18,8 @@ func (suite *KeeperTestSuite) TestMsgCreateClientEvents() { tmConfig, ok := path.EndpointA.ClientConfig.(*ibctesting.TendermintConfig) suite.Require().True(ok) - height := path.EndpointA.Counterparty.Chain.LatestCommittedHeader.GetHeight().(clienttypes.Height) + height, ok := path.EndpointA.Counterparty.Chain.LatestCommittedHeader.GetHeight().(clienttypes.Height) + suite.Require().True(ok) clientState := ibctm.NewClientState( path.EndpointA.Counterparty.Chain.ChainID, tmConfig.TrustLevel, tmConfig.TrustingPeriod, tmConfig.UnbondingPeriod, tmConfig.MaxClockDrift, height, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) @@ -56,7 +57,8 @@ func (suite *KeeperTestSuite) TestMsgUpdateClientEvents() { suite.chainB.Coordinator.CommitBlock(suite.chainB) - clientState := path.EndpointA.GetClientState().(*ibctm.ClientState) + clientState, ok := path.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) trustedHeight := clientState.LatestHeight header, err := suite.chainB.IBCClientHeader(suite.chainB.LatestCommittedHeader, trustedHeight) suite.Require().NoError(err) diff --git a/modules/core/02-client/keeper/grpc_query_test.go b/modules/core/02-client/keeper/grpc_query_test.go index a2dc1eb3377..2a916d89c8b 100644 --- a/modules/core/02-client/keeper/grpc_query_test.go +++ b/modules/core/02-client/keeper/grpc_query_test.go @@ -344,7 +344,8 @@ func (suite *KeeperTestSuite) TestQueryConsensusStates() { path := ibctesting.NewPath(suite.chainA, suite.chainB) path.SetupClients() - height1 := path.EndpointA.GetClientLatestHeight().(types.Height) + height1, ok := path.EndpointA.GetClientLatestHeight().(types.Height) + suite.Require().True(ok) expConsensusStates = append( expConsensusStates, types.NewConsensusStateWithHeight( @@ -354,8 +355,8 @@ func (suite *KeeperTestSuite) TestQueryConsensusStates() { err := path.EndpointA.UpdateClient() suite.Require().NoError(err) - - height2 := path.EndpointA.GetClientLatestHeight().(types.Height) + height2, ok := path.EndpointA.GetClientLatestHeight().(types.Height) + suite.Require().True(ok) expConsensusStates = append( expConsensusStates, types.NewConsensusStateWithHeight( @@ -548,10 +549,12 @@ func (suite *KeeperTestSuite) TestQueryClientStatus() { func() { path := ibctesting.NewPath(suite.chainA, suite.chainB) path.SetupClients() - clientState := path.EndpointA.GetClientState().(*ibctm.ClientState) + clientState, ok := path.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) // increment latest height so no consensus state is stored - clientState.LatestHeight = clientState.LatestHeight.Increment().(types.Height) + clientState.LatestHeight, ok = clientState.LatestHeight.Increment().(types.Height) + suite.Require().True(ok) path.EndpointA.SetClientState(clientState) req = &types.QueryClientStatusRequest{ @@ -565,7 +568,8 @@ func (suite *KeeperTestSuite) TestQueryClientStatus() { func() { path := ibctesting.NewPath(suite.chainA, suite.chainB) path.SetupClients() - clientState := path.EndpointA.GetClientState().(*ibctm.ClientState) + clientState, ok := path.EndpointA.GetClientState().(*ibctm.ClientState) + suite.Require().True(ok) clientState.FrozenHeight = types.NewHeight(0, 1) path.EndpointA.SetClientState(clientState) @@ -636,7 +640,9 @@ func (suite *KeeperTestSuite) TestQueryUpgradedClientState() { suite.Require().NoError(err) suite.Require().NotNil(resp) - expClientState = clientState.(*ibctm.ClientState) + var ok bool + expClientState, ok = clientState.(*ibctm.ClientState) + suite.Require().True(ok) }, nil, }, diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index ff8a54a7f29..8c777c11fbf 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -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) + if !ok { + panic("can't convert clientModule.LatestHeight to types.Height") + } + return clientModuleHeight } // GetClientTimestampAtHeight returns the timestamp in nanoseconds of the consensus state at the given height. diff --git a/modules/core/02-client/keeper/migrations.go b/modules/core/02-client/keeper/migrations.go index 73bee079231..cefdd5da0fc 100644 --- a/modules/core/02-client/keeper/migrations.go +++ b/modules/core/02-client/keeper/migrations.go @@ -3,7 +3,7 @@ package keeper import ( sdk "github.com/cosmos/cosmos-sdk/types" - v7 "github.com/cosmos/ibc-go/v8/modules/core/02-client/migrations/v7" + "github.com/cosmos/ibc-go/v8/modules/core/02-client/migrations/v7" "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" ) diff --git a/modules/core/02-client/migrations/v7/genesis_test.go b/modules/core/02-client/migrations/v7/genesis_test.go index 944e579892f..3cdeb9c1396 100644 --- a/modules/core/02-client/migrations/v7/genesis_test.go +++ b/modules/core/02-client/migrations/v7/genesis_test.go @@ -7,7 +7,7 @@ import ( codectypes "github.com/cosmos/cosmos-sdk/codec/types" ibcclient "github.com/cosmos/ibc-go/v8/modules/core/02-client" - v7 "github.com/cosmos/ibc-go/v8/modules/core/02-client/migrations/v7" + "github.com/cosmos/ibc-go/v8/modules/core/02-client/migrations/v7" "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" host "github.com/cosmos/ibc-go/v8/modules/core/24-host" ibcexported "github.com/cosmos/ibc-go/v8/modules/core/exported" @@ -67,7 +67,8 @@ func (suite *MigrationsV7TestSuite) TestMigrateGenesisSolomachine() { // set in store for ease of determining expected genesis clientStore := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), sm.ClientID) - cdc := suite.chainA.App.AppCodec().(*codec.ProtoCodec) + cdc, ok := suite.chainA.App.AppCodec().(*codec.ProtoCodec) + suite.Require().True(ok) v7.RegisterInterfaces(cdc.InterfaceRegistry()) bz, err := cdc.MarshalInterface(legacyClientState) diff --git a/modules/core/02-client/migrations/v7/localhost_test.go b/modules/core/02-client/migrations/v7/localhost_test.go index 57b604b09cd..1a6d657477c 100644 --- a/modules/core/02-client/migrations/v7/localhost_test.go +++ b/modules/core/02-client/migrations/v7/localhost_test.go @@ -1,7 +1,7 @@ package v7_test import ( - v7 "github.com/cosmos/ibc-go/v8/modules/core/02-client/migrations/v7" + "github.com/cosmos/ibc-go/v8/modules/core/02-client/migrations/v7" host "github.com/cosmos/ibc-go/v8/modules/core/24-host" "github.com/cosmos/ibc-go/v8/modules/core/exported" ) diff --git a/modules/core/02-client/migrations/v7/store_test.go b/modules/core/02-client/migrations/v7/store_test.go index b0a2ae934e6..9ef3d995980 100644 --- a/modules/core/02-client/migrations/v7/store_test.go +++ b/modules/core/02-client/migrations/v7/store_test.go @@ -8,7 +8,7 @@ import ( "github.com/cosmos/cosmos-sdk/codec" - v7 "github.com/cosmos/ibc-go/v8/modules/core/02-client/migrations/v7" + "github.com/cosmos/ibc-go/v8/modules/core/02-client/migrations/v7" "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" host "github.com/cosmos/ibc-go/v8/modules/core/24-host" ibcexported "github.com/cosmos/ibc-go/v8/modules/core/exported" @@ -104,7 +104,8 @@ func (suite *MigrationsV7TestSuite) createSolomachineClients(solomachines []*ibc AllowUpdateAfterProposal: true, } - cdc := suite.chainA.App.AppCodec().(*codec.ProtoCodec) + cdc, ok := suite.chainA.App.AppCodec().(*codec.ProtoCodec) + suite.Require().True(ok) v7.RegisterInterfaces(cdc.InterfaceRegistry()) bz, err := cdc.MarshalInterface(legacyClientState) diff --git a/modules/core/02-client/types/client.go b/modules/core/02-client/types/client.go index 2b1cb965b85..b38dc3370ea 100644 --- a/modules/core/02-client/types/client.go +++ b/modules/core/02-client/types/client.go @@ -6,7 +6,7 @@ import ( "sort" "strings" - proto "github.com/cosmos/gogoproto/proto" + "github.com/cosmos/gogoproto/proto" errorsmod "cosmossdk.io/errors" diff --git a/modules/core/02-client/types/client_test.go b/modules/core/02-client/types/client_test.go index 960f8e785e9..19514ea7f36 100644 --- a/modules/core/02-client/types/client_test.go +++ b/modules/core/02-client/types/client_test.go @@ -27,7 +27,8 @@ func (suite *TypesTestSuite) TestMarshalConsensusStateWithHeight() { path := ibctesting.NewPath(suite.chainA, suite.chainB) path.SetupClients() - latestHeight := path.EndpointA.GetClientLatestHeight().(types.Height) + latestHeight, ok := path.EndpointA.GetClientLatestHeight().(types.Height) + suite.Require().True(ok) consensusState, ok := suite.chainA.GetConsensusState(path.EndpointA.ClientID, latestHeight) suite.Require().True(ok) diff --git a/modules/core/02-client/types/codec.go b/modules/core/02-client/types/codec.go index 835b641e7cf..e70c0682300 100644 --- a/modules/core/02-client/types/codec.go +++ b/modules/core/02-client/types/codec.go @@ -1,7 +1,7 @@ package types import ( - proto "github.com/cosmos/gogoproto/proto" + "github.com/cosmos/gogoproto/proto" errorsmod "cosmossdk.io/errors" diff --git a/modules/core/02-client/types/legacy_proposal_test.go b/modules/core/02-client/types/legacy_proposal_test.go index a2684265a4f..a3724616edd 100644 --- a/modules/core/02-client/types/legacy_proposal_test.go +++ b/modules/core/02-client/types/legacy_proposal_test.go @@ -74,7 +74,8 @@ func (suite *TypesTestSuite) TestMarshalClientUpdateProposalProposal() { cdc := codec.NewProtoCodec(ir) // marshal message - content := proposal.(*types.ClientUpdateProposal) + content, ok := proposal.(*types.ClientUpdateProposal) + suite.Require().True(ok) bz, err := cdc.MarshalJSON(content) suite.Require().NoError(err) From 16aacf3b4c8e2120107d63390b1f53e8de5c1e67 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Tue, 9 Apr 2024 09:27:40 +0100 Subject: [PATCH 2/3] Update modules/core/02-client/keeper/keeper.go Co-authored-by: Carlos Rodriguez --- modules/core/02-client/keeper/keeper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index 8c777c11fbf..3a0a0ce66be 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -424,7 +424,7 @@ func (k Keeper) GetClientLatestHeight(ctx sdk.Context, clientID string) types.He clientModuleHeight, ok := clientModule.LatestHeight(ctx, clientID).(types.Height) if !ok { - panic("can't convert clientModule.LatestHeight to types.Height") + panic(fmt.Errorf("cannot convert %T to %T", clientModule.LatestHeight, &types.Height") } return clientModuleHeight } From bce654d44755fb232592856c7af09affa11fe3e4 Mon Sep 17 00:00:00 2001 From: Nikolas De Giorgis Date: Tue, 9 Apr 2024 09:29:49 +0100 Subject: [PATCH 3/3] Better panic messages. --- modules/core/02-client/keeper/keeper.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index 3a0a0ce66be..6d33183b72b 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -422,11 +422,12 @@ func (k Keeper) GetClientLatestHeight(ctx sdk.Context, clientID string) types.He return types.ZeroHeight() } - clientModuleHeight, ok := clientModule.LatestHeight(ctx, clientID).(types.Height) + var latestHeight types.Height + latestHeight, ok := clientModule.LatestHeight(ctx, clientID).(types.Height) if !ok { - panic(fmt.Errorf("cannot convert %T to %T", clientModule.LatestHeight, &types.Height") + panic(fmt.Errorf("cannot convert %T to %T", clientModule.LatestHeight, latestHeight)) } - return clientModuleHeight + return latestHeight } // GetClientTimestampAtHeight returns the timestamp in nanoseconds of the consensus state at the given height.