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.

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: update 07-tendermint GetConsensusState to return bool over error #1180

Merged
10 changes: 5 additions & 5 deletions modules/light-clients/07-tendermint/types/client_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ func (cs ClientState) Status(
}

// get latest consensus state from clientStore to check for expiry
consState, err := GetConsensusState(clientStore, cdc, cs.GetLatestHeight())
if err != nil {
consState, found := GetConsensusState(clientStore, cdc, cs.GetLatestHeight())
if !found {
// if the client state does not have an associated consensus state for its latest height
// then it must be expired
return exported.Expired
Expand Down Expand Up @@ -574,9 +574,9 @@ func produceVerificationArgs(
return commitmenttypes.MerkleProof{}, nil, sdkerrors.Wrap(commitmenttypes.ErrInvalidProof, "failed to unmarshal proof into commitment merkle proof")
}

consensusState, err = GetConsensusState(store, cdc, height)
if err != nil {
return commitmenttypes.MerkleProof{}, nil, sdkerrors.Wrap(err, "please ensure the proof was constructed against a height that exists on the client")
consensusState, found := GetConsensusState(store, cdc, height)
if !found {
return commitmenttypes.MerkleProof{}, nil, sdkerrors.Wrap(clienttypes.ErrConsensusStateNotFound, "please ensure the proof was constructed against a height that exists on the client")
}

return merkleProof, consensusState, nil
Expand Down
12 changes: 6 additions & 6 deletions modules/light-clients/07-tendermint/types/misbehaviour_handle.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,14 @@ func (cs *ClientState) verifyMisbehaviour(ctx sdk.Context, clientStore sdk.KVSto
// Regardless of the type of misbehaviour, ensure that both headers are valid and would have been accepted by light-client

// Retrieve trusted consensus states for each Header in misbehaviour
tmConsensusState1, err := GetConsensusState(clientStore, cdc, misbehaviour.Header1.TrustedHeight)
if err != nil {
return sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header1 at TrustedHeight: %s", misbehaviour.Header1.TrustedHeight)
tmConsensusState1, found := GetConsensusState(clientStore, cdc, misbehaviour.Header1.TrustedHeight)
if !found {
return sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "could not get trusted consensus state from clientStore for Header1 at TrustedHeight: %s", misbehaviour.Header1.TrustedHeight)
}

tmConsensusState2, err := GetConsensusState(clientStore, cdc, misbehaviour.Header2.TrustedHeight)
if err != nil {
return sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header2 at TrustedHeight: %s", misbehaviour.Header2.TrustedHeight)
tmConsensusState2, found := GetConsensusState(clientStore, cdc, misbehaviour.Header2.TrustedHeight)
if !found {
return sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "could not get trusted consensus state from clientStore for Header2 at TrustedHeight: %s", misbehaviour.Header2.TrustedHeight)
}

// Check the validity of the two conflicting headers against their respective
Expand Down
6 changes: 3 additions & 3 deletions modules/light-clients/07-tendermint/types/proposal_handle.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ func (cs ClientState) CheckSubstituteAndUpdateState(
// starting from initial height and ending on the latest height (inclusive)
height := substituteClientState.GetLatestHeight()

consensusState, err := GetConsensusState(substituteClientStore, cdc, height)
if err != nil {
return nil, sdkerrors.Wrap(err, "unable to retrieve latest consensus state for substitute client")
consensusState, found := GetConsensusState(substituteClientStore, cdc, height)
if !found {
return nil, sdkerrors.Wrap(clienttypes.ErrConsensusStateNotFound, "unable to retrieve latest consensus state for substitute client")
}

SetConsensusState(subjectClientStore, cdc, consensusState, height)
Expand Down
33 changes: 8 additions & 25 deletions modules/light-clients/07-tendermint/types/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/store/prefix"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
host "github.com/cosmos/ibc-go/v3/modules/core/24-host"
Expand Down Expand Up @@ -57,31 +56,16 @@ func SetConsensusState(clientStore sdk.KVStore, cdc codec.BinaryCodec, consensus
clientStore.Set(key, val)
}

// GetConsensusState retrieves the consensus state from the client prefixed
// store. An error is returned if the consensus state does not exist.
func GetConsensusState(store sdk.KVStore, cdc codec.BinaryCodec, height exported.Height) (*ConsensusState, error) {
// GetConsensusState retrieves the consensus state from the client prefixed store.
// If the ConsensusState does not exist in state for the provided height a nil value and false boolean flag is returned
func GetConsensusState(store sdk.KVStore, cdc codec.BinaryCodec, height exported.Height) (*ConsensusState, bool) {
bz := store.Get(host.ConsensusStateKey(height))
if bz == nil {
return nil, sdkerrors.Wrapf(
clienttypes.ErrConsensusStateNotFound,
"consensus state does not exist for height %s", height,
)
}

consensusStateI, err := clienttypes.UnmarshalConsensusState(cdc, bz)
if err != nil {
return nil, sdkerrors.Wrapf(clienttypes.ErrInvalidConsensus, "unmarshal error: %v", err)
}

consensusState, ok := consensusStateI.(*ConsensusState)
if !ok {
return nil, sdkerrors.Wrapf(
clienttypes.ErrInvalidConsensus,
"invalid consensus type %T, expected %T", consensusState, &ConsensusState{},
)
return nil, false
}

return consensusState, nil
consensusStateI := clienttypes.MustUnmarshalConsensusState(cdc, bz)
return consensusStateI.(*ConsensusState), true
}

// deleteConsensusState deletes the consensus state at the given height
Expand Down Expand Up @@ -299,9 +283,8 @@ func PruneAllExpiredConsensusStates(
var heights []exported.Height

pruneCb := func(height exported.Height) bool {
consState, err := GetConsensusState(clientStore, cdc, height)
// this error should never occur
if err != nil {
consState, found := GetConsensusState(clientStore, cdc, height)
if !found { // consensus state should always be found
return true
}

Expand Down
25 changes: 18 additions & 7 deletions modules/light-clients/07-tendermint/types/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,31 +23,32 @@ func (suite *TendermintTestSuite) TestGetConsensusState() {
name string
malleate func()
expPass bool
expPanic bool
}{
{
"success", func() {}, true,
"success", func() {}, true, false,
},
{
"consensus state not found", func() {
// use height with no consensus state set
height = height.(clienttypes.Height).Increment()
}, false,
}, false, false,
},
{
"not a consensus state interface", func() {
// marshal an empty client state and set as consensus state
store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID)
clientStateBz := suite.chainA.App.GetIBCKeeper().ClientKeeper.MustMarshalClientState(&types.ClientState{})
store.Set(host.ConsensusStateKey(height), clientStateBz)
}, false,
}, false, true,
},
{
"invalid consensus state (solomachine)", func() {
// marshal and set solomachine consensus state
store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID)
consensusStateBz := suite.chainA.App.GetIBCKeeper().ClientKeeper.MustMarshalConsensusState(&solomachinetypes.ConsensusState{})
store.Set(host.ConsensusStateKey(height), consensusStateBz)
}, false,
}, false, true,
},
}

Expand All @@ -64,16 +65,26 @@ func (suite *TendermintTestSuite) TestGetConsensusState() {

tc.malleate() // change vars as necessary

if tc.expPanic {
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
suite.Require().Panics(func() {
store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID)
types.GetConsensusState(store, suite.chainA.Codec, height)
})

return
}

store := suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.chainA.GetContext(), path.EndpointA.ClientID)
consensusState, err := types.GetConsensusState(store, suite.chainA.Codec, height)
consensusState, found := types.GetConsensusState(store, suite.chainA.Codec, height)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().True(found)

expConsensusState, found := suite.chainA.GetConsensusState(path.EndpointA.ClientID, height)
suite.Require().True(found)
suite.Require().Equal(expConsensusState, consensusState)
} else {
suite.Require().Error(err)
suite.Require().False(found)
suite.Require().Nil(consensusState)
}
})
Expand Down
12 changes: 6 additions & 6 deletions modules/light-clients/07-tendermint/types/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,9 @@ func (cs *ClientState) verifyHeader(
currentTimestamp := ctx.BlockTime()

// Retrieve trusted consensus states for each Header in misbehaviour
consState, err := GetConsensusState(clientStore, cdc, header.TrustedHeight)
if err != nil {
return sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header at TrustedHeight: %s", header.TrustedHeight)
consState, found := GetConsensusState(clientStore, cdc, header.TrustedHeight)
if !found {
return sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "could not get trusted consensus state from clientStore for Header at TrustedHeight: %s", header.TrustedHeight)
}

if err := checkTrustedHeader(header, consState); err != nil {
Expand Down Expand Up @@ -280,10 +280,10 @@ func (cs ClientState) pruneOldestConsensusState(ctx sdk.Context, cdc codec.Binar
)

pruneCb := func(height exported.Height) bool {
consState, err := GetConsensusState(clientStore, cdc, height)
consState, found := GetConsensusState(clientStore, cdc, height)
// this error should never occur
if err != nil {
pruneError = err
if !found {
pruneError = sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "failed to retrieve consensus state at height: %s", height)
return true
}

Expand Down
6 changes: 3 additions & 3 deletions modules/light-clients/07-tendermint/types/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ func (cs ClientState) VerifyUpgradeAndUpdateState(
// Must prove against latest consensus state to ensure we are verifying against latest upgrade plan
// This verifies that upgrade is intended for the provided revision, since committed client must exist
// at this consensus state
consState, err := GetConsensusState(clientStore, cdc, lastHeight)
if err != nil {
return sdkerrors.Wrap(err, "could not retrieve consensus state for lastHeight")
consState, found := GetConsensusState(clientStore, cdc, lastHeight)
if !found {
return sdkerrors.Wrap(clienttypes.ErrConsensusStateNotFound, "could not retrieve consensus state for lastHeight")
}

// Verify client proof
Expand Down