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

Merged
merged 8 commits into from
Mar 30, 2022
Merged

chore: update 07-tendermint GetConsensusState to return bool over error #1180

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
edcbf5f
updating 07-tendermint GetConsensusState to return bool over error
damiannolan Mar 28, 2022
bfccea5
panic on unmarshal and type assertion failure, update tests
damiannolan Mar 28, 2022
75af0dc
updating to use MustUnmarshalConsensusState directly
damiannolan Mar 29, 2022
3878189
Merge branch '02-client-refactor' into damian/1140-update-get-consens…
damiannolan Mar 29, 2022
5ef2cee
Update modules/light-clients/07-tendermint/types/misbehaviour_handle.go
damiannolan Mar 29, 2022
6e18aca
Update modules/light-clients/07-tendermint/types/misbehaviour_handle.go
damiannolan Mar 29, 2022
120912a
removing comments and updating godoc
damiannolan Mar 29, 2022
5d9ca38
Merge branch '02-client-refactor' into damian/1140-update-get-consens…
damiannolan Mar 30, 2022
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
10 changes: 5 additions & 5 deletions modules/light-clients/07-tendermint/types/client_state.go
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,15 @@ func (cs ClientState) CheckMisbehaviourAndUpdateState(
// and unmarshal from clientStore

// Get consensus bytes from clientStore
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
tmConsensusState1, err := GetConsensusState(clientStore, cdc, tmMisbehaviour.Header1.TrustedHeight)
if err != nil {
return nil, sdkerrors.Wrapf(err, "could not get trusted consensus state from clientStore for Header1 at TrustedHeight: %s", tmMisbehaviour.Header1)
tmConsensusState1, found := GetConsensusState(clientStore, cdc, tmMisbehaviour.Header1.TrustedHeight)
if !found {
return nil, sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "could not get trusted consensus state from clientStore for Header1 at TrustedHeight: %s", tmMisbehaviour.Header1)
}

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

// 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
View file
Open in desktop
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
25 changes: 6 additions & 19 deletions modules/light-clients/07-tendermint/types/store.go
View file
Open in desktop
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 @@ -59,29 +58,18 @@ func SetConsensusState(clientStore sdk.KVStore, cdc codec.BinaryCodec, consensus

// GetConsensusState retrieves the consensus state from the client prefixed
// store. An error is returned if the consensus state does not exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

godoc needs to be updated. Should we make this function private?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot

Copy link
Member Author

Choose a reason for hiding this comment

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

Making private makes a bit more awkward for testing. We would actually need to remove the test function for this.
getConsensusState will still get coverage from a number of other tests but testing the panic scenarios then isn't really possible.

What do you think? I don't really have any strong feelings about making it public/private

Copy link
Contributor

Choose a reason for hiding this comment

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

lets leave for now

func GetConsensusState(store sdk.KVStore, cdc codec.BinaryCodec, height exported.Height) (*ConsensusState, error) {
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,
)
return nil, false
}

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{},
)
panic(err)
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
}

return consensusState, nil
return consensusStateI.(*ConsensusState), true
}

// deleteConsensusState deletes the consensus state at the given height
Expand Down Expand Up @@ -299,9 +287,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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,10 @@ func (cs ClientState) CheckHeaderAndUpdateState(
}

// get consensus state from clientStore
trustedConsState, err := GetConsensusState(clientStore, cdc, tmHeader.TrustedHeight)
if err != nil {
trustedConsState, found := GetConsensusState(clientStore, cdc, tmHeader.TrustedHeight)
if !found {
return nil, nil, sdkerrors.Wrapf(
err, "could not get consensus state from clientstore at TrustedHeight: %s", tmHeader.TrustedHeight,
clienttypes.ErrConsensusStateNotFound, "could not get consensus state from clientstore at TrustedHeight: %s", tmHeader.TrustedHeight,
)
}

Expand Down Expand Up @@ -259,10 +259,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
View file
Open in desktop
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