From fd69c7a6de6eb1f6277779179344d9eaf7bfcfa6 Mon Sep 17 00:00:00 2001 From: technicallyty <48813565+tytech3@users.noreply.github.com> Date: Thu, 11 Nov 2021 17:50:45 -0800 Subject: [PATCH 1/5] refactor: return error instead of bool from GetSelfConsensusState --- modules/core/02-client/keeper/keeper.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index 12d360e4053..ec2b0d06296 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -241,19 +241,19 @@ func (k Keeper) GetLatestClientConsensusState(ctx sdk.Context, clientID string) // GetSelfConsensusState introspects the (self) past historical info at a given height // and returns the expected consensus state at that height. // For now, can only retrieve self consensus states for the current revision -func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, bool) { +func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { selfHeight, ok := height.(types.Height) if !ok { - return nil, false + return nil, sdkerrors.ErrInvalidType.Wrapf("cannot cast %T to %T", height, types.Height{}) } // check that height revision matches chainID revision revision := types.ParseChainID(ctx.ChainID()) if revision != height.GetRevisionNumber() { - return nil, false + return nil, sdkerrors.ErrInvalidHeight.Wrapf("height revision %d does not match chainID revision %d", revision, height.GetRevisionNumber()) } histInfo, found := k.stakingKeeper.GetHistoricalInfo(ctx, int64(selfHeight.RevisionHeight)) if !found { - return nil, false + return nil, sdkerrors.ErrNotFound.Wrapf("no historical info found at height %d", selfHeight.RevisionHeight) } consensusState := &ibctmtypes.ConsensusState{ @@ -261,7 +261,7 @@ func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) ( Root: commitmenttypes.NewMerkleRoot(histInfo.Header.GetAppHash()), NextValidatorsHash: histInfo.Header.NextValidatorsHash, } - return consensusState, true + return consensusState, nil } // ValidateSelfClient validates the client parameters for a client of the running chain From ae5b01ef145ebc33ae432d2e89069c869f1304aa Mon Sep 17 00:00:00 2001 From: technicallyty <48813565+tytech3@users.noreply.github.com> Date: Thu, 11 Nov 2021 18:04:12 -0800 Subject: [PATCH 2/5] fix: tests and interface signature --- modules/core/02-client/keeper/keeper_test.go | 6 +++--- modules/core/03-connection/keeper/handshake.go | 12 ++++++------ modules/core/03-connection/keeper/verify_test.go | 6 +++--- modules/core/03-connection/types/expected_keepers.go | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/modules/core/02-client/keeper/keeper_test.go b/modules/core/02-client/keeper/keeper_test.go index 54df1c15013..b3520dbe996 100644 --- a/modules/core/02-client/keeper/keeper_test.go +++ b/modules/core/02-client/keeper/keeper_test.go @@ -306,12 +306,12 @@ func (suite KeeperTestSuite) TestGetConsensusState() { for i, tc := range cases { tc := tc - cs, found := suite.keeper.GetSelfConsensusState(suite.ctx, tc.height) + cs, err := suite.keeper.GetSelfConsensusState(suite.ctx, tc.height) if tc.expPass { - suite.Require().True(found, "Case %d should have passed: %s", i, tc.name) + suite.Require().NoError(err, "Case %d should have passed: %s", i, tc.name) suite.Require().NotNil(cs, "Case %d should have passed: %s", i, tc.name) } else { - suite.Require().False(found, "Case %d should have failed: %s", i, tc.name) + suite.Require().Error(err, "Case %d should have failed: %s", i, tc.name) suite.Require().Nil(cs, "Case %d should have failed: %s", i, tc.name) } } diff --git a/modules/core/03-connection/keeper/handshake.go b/modules/core/03-connection/keeper/handshake.go index e3a8ac242f7..b9772508d38 100644 --- a/modules/core/03-connection/keeper/handshake.go +++ b/modules/core/03-connection/keeper/handshake.go @@ -125,9 +125,9 @@ func (k Keeper) ConnOpenTry( return "", err } - expectedConsensusState, found := k.clientKeeper.GetSelfConsensusState(ctx, consensusHeight) - if !found { - return "", sdkerrors.Wrap(clienttypes.ErrSelfConsensusStateNotFound, consensusHeight.String()) + expectedConsensusState, err := k.clientKeeper.GetSelfConsensusState(ctx, consensusHeight) + if err != nil{ + return "", clienttypes.ErrSelfConsensusStateNotFound.Wrapf(err.Error(), consensusHeight.String()) } // expectedConnection defines Chain A's ConnectionEnd @@ -250,9 +250,9 @@ func (k Keeper) ConnOpenAck( } // Retrieve chainA's consensus state at consensusheight - expectedConsensusState, found := k.clientKeeper.GetSelfConsensusState(ctx, consensusHeight) - if !found { - return clienttypes.ErrSelfConsensusStateNotFound + expectedConsensusState, err := k.clientKeeper.GetSelfConsensusState(ctx, consensusHeight) + if err != nil { + return clienttypes.ErrSelfConsensusStateNotFound.Wrap(err.Error()) } prefix := k.GetCommitmentPrefix() diff --git a/modules/core/03-connection/keeper/verify_test.go b/modules/core/03-connection/keeper/verify_test.go index 6ef0cc78dca..b3fb7d49c0f 100644 --- a/modules/core/03-connection/keeper/verify_test.go +++ b/modules/core/03-connection/keeper/verify_test.go @@ -139,10 +139,10 @@ func (suite *KeeperTestSuite) TestVerifyClientConsensusState() { proof, consensusHeight := suite.chainB.QueryConsensusStateProof(path.EndpointB.ClientID) proofHeight := clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()-1)) - consensusState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetSelfConsensusState(suite.chainA.GetContext(), consensusHeight) - suite.Require().True(found) + consensusState, err := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetSelfConsensusState(suite.chainA.GetContext(), consensusHeight) + suite.Require().NoError(err) - err := suite.chainA.App.GetIBCKeeper().ConnectionKeeper.VerifyClientConsensusState( + err = suite.chainA.App.GetIBCKeeper().ConnectionKeeper.VerifyClientConsensusState( suite.chainA.GetContext(), connection, malleateHeight(proofHeight, heightDiff), consensusHeight, proof, consensusState, ) diff --git a/modules/core/03-connection/types/expected_keepers.go b/modules/core/03-connection/types/expected_keepers.go index e378adbed2a..a7627c531f7 100644 --- a/modules/core/03-connection/types/expected_keepers.go +++ b/modules/core/03-connection/types/expected_keepers.go @@ -9,7 +9,7 @@ import ( type ClientKeeper interface { GetClientState(ctx sdk.Context, clientID string) (exported.ClientState, bool) GetClientConsensusState(ctx sdk.Context, clientID string, height exported.Height) (exported.ConsensusState, bool) - GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, bool) + GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) ValidateSelfClient(ctx sdk.Context, clientState exported.ClientState) error IterateClients(ctx sdk.Context, cb func(string, exported.ClientState) bool) ClientStore(ctx sdk.Context, clientID string) sdk.KVStore From 5e869e4154723afebefc017500c0c0f13be404e5 Mon Sep 17 00:00:00 2001 From: technicallyty <48813565+tytech3@users.noreply.github.com> Date: Thu, 11 Nov 2021 18:04:56 -0800 Subject: [PATCH 3/5] chore: error wording --- 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 ec2b0d06296..3bf988e3e37 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -244,7 +244,7 @@ func (k Keeper) GetLatestClientConsensusState(ctx sdk.Context, clientID string) func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { selfHeight, ok := height.(types.Height) if !ok { - return nil, sdkerrors.ErrInvalidType.Wrapf("cannot cast %T to %T", height, types.Height{}) + return nil, sdkerrors.ErrInvalidType.Wrapf("unable cast %T to %T", height, types.Height{}) } // check that height revision matches chainID revision revision := types.ParseChainID(ctx.ChainID()) From ae8dc9e0f088a5bae79d3d5ad7dc25ea725187e7 Mon Sep 17 00:00:00 2001 From: technicallyty <48813565+tytech3@users.noreply.github.com> Date: Fri, 12 Nov 2021 07:15:05 -0800 Subject: [PATCH 4/5] chore: apply changes from review --- modules/core/02-client/keeper/keeper.go | 4 ++-- modules/core/03-connection/keeper/handshake.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index 3bf988e3e37..ed637511a65 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -249,11 +249,11 @@ func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) ( // check that height revision matches chainID revision revision := types.ParseChainID(ctx.ChainID()) if revision != height.GetRevisionNumber() { - return nil, sdkerrors.ErrInvalidHeight.Wrapf("height revision %d does not match chainID revision %d", revision, height.GetRevisionNumber()) + return nil, sdkerrors.Wrapf(types.ErrInvalidHeight, "chainID revision number does not match height revision number: expected %d, got %d", revision, height.GetRevisionNumber()) } histInfo, found := k.stakingKeeper.GetHistoricalInfo(ctx, int64(selfHeight.RevisionHeight)) if !found { - return nil, sdkerrors.ErrNotFound.Wrapf("no historical info found at height %d", selfHeight.RevisionHeight) + return nil, sdkerrors.Wrapf(sdkerrors.ErrNotFound, "no historical info found at height %d", selfHeight.RevisionHeight) } consensusState := &ibctmtypes.ConsensusState{ diff --git a/modules/core/03-connection/keeper/handshake.go b/modules/core/03-connection/keeper/handshake.go index b9772508d38..1dab8821ae5 100644 --- a/modules/core/03-connection/keeper/handshake.go +++ b/modules/core/03-connection/keeper/handshake.go @@ -126,8 +126,8 @@ func (k Keeper) ConnOpenTry( } expectedConsensusState, err := k.clientKeeper.GetSelfConsensusState(ctx, consensusHeight) - if err != nil{ - return "", clienttypes.ErrSelfConsensusStateNotFound.Wrapf(err.Error(), consensusHeight.String()) + if err != nil { + return "", sdkerrors.Wrapf(err, "self consensus state not found for height %s", consensusHeight.String()) } // expectedConnection defines Chain A's ConnectionEnd @@ -252,7 +252,7 @@ func (k Keeper) ConnOpenAck( // Retrieve chainA's consensus state at consensusheight expectedConsensusState, err := k.clientKeeper.GetSelfConsensusState(ctx, consensusHeight) if err != nil { - return clienttypes.ErrSelfConsensusStateNotFound.Wrap(err.Error()) + return sdkerrors.Wrapf(err, "self consensus state not found for height %s", consensusHeight.String()) } prefix := k.GetCommitmentPrefix() From 5fc40dea6951db98a378c8b404a7bbbf43c54bb7 Mon Sep 17 00:00:00 2001 From: technicallyty <48813565+tytech3@users.noreply.github.com> Date: Wed, 17 Nov 2021 09:34:51 -0800 Subject: [PATCH 5/5] chore: nit+changelog --- CHANGELOG.md | 1 + modules/core/02-client/keeper/keeper.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cba9a60da0..054abb5db9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### API Breaking * (transfer) [\#517](https://github.com/cosmos/ibc-go/pull/517) Separates the ICS 26 callback functions from `AppModule` into a new type `IBCModule` for ICS 20 transfer. +* (modules/core/02-client) [\#536](https://github.com/cosmos/ibc-go/pull/536) GetSelfConsensusState return type changed from bool to error. ### State Machine Breaking diff --git a/modules/core/02-client/keeper/keeper.go b/modules/core/02-client/keeper/keeper.go index ed637511a65..18766130304 100644 --- a/modules/core/02-client/keeper/keeper.go +++ b/modules/core/02-client/keeper/keeper.go @@ -244,7 +244,7 @@ func (k Keeper) GetLatestClientConsensusState(ctx sdk.Context, clientID string) func (k Keeper) GetSelfConsensusState(ctx sdk.Context, height exported.Height) (exported.ConsensusState, error) { selfHeight, ok := height.(types.Height) if !ok { - return nil, sdkerrors.ErrInvalidType.Wrapf("unable cast %T to %T", height, types.Height{}) + return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", types.Height{}, height) } // check that height revision matches chainID revision revision := types.ParseChainID(ctx.ChainID())