diff --git a/x/committee/keeper/integration_test.go b/x/committee/keeper/integration_test.go index e681c6c8..29a23fcc 100644 --- a/x/committee/keeper/integration_test.go +++ b/x/committee/keeper/integration_test.go @@ -7,6 +7,12 @@ import ( "github.com/kava-labs/kava/x/committee/types" ) +// Avoid cluttering test cases with long function names +func i(in int64) sdk.Int { return sdk.NewInt(in) } +func d(str string) sdk.Dec { return sdk.MustNewDecFromStr(str) } +func c(denom string, amount int64) sdk.Coin { return sdk.NewInt64Coin(denom, amount) } +func cs(coins ...sdk.Coin) sdk.Coins { return sdk.NewCoins(coins...) } + // proposalVoteMap collects up votes into a map indexed by proposalID func getProposalVoteMap(k keeper.Keeper, ctx sdk.Context) map[uint64]([]types.Vote) { diff --git a/x/committee/keeper/keeper_test.go b/x/committee/keeper/keeper_test.go index 9144f449..3001eec0 100644 --- a/x/committee/keeper/keeper_test.go +++ b/x/committee/keeper/keeper_test.go @@ -15,8 +15,6 @@ import ( "github.com/kava-labs/kava/x/committee/types" ) -func d(s string) sdk.Dec { return sdk.MustNewDecFromStr(s) } - type KeeperTestSuite struct { suite.Suite diff --git a/x/committee/keeper/proposal.go b/x/committee/keeper/proposal.go index 66cfddb4..bbdb666f 100644 --- a/x/committee/keeper/proposal.go +++ b/x/committee/keeper/proposal.go @@ -78,6 +78,7 @@ func (k Keeper) AddVote(ctx sdk.Context, proposalID uint64, voter sdk.AccAddress } // GetProposalResult calculates if a proposal currently has enough votes to pass. +// TODO rename GetProposalTally? func (k Keeper) GetProposalResult(ctx sdk.Context, proposalID uint64) (bool, sdk.Error) { pr, found := k.GetProposal(ctx, proposalID) if !found { @@ -114,18 +115,19 @@ func (k Keeper) EnactProposal(ctx sdk.Context, proposalID uint64) sdk.Error { return types.ErrUnknownProposal(k.codespace, proposalID) } - // Run the proposal's changes through the associated handler, but using a cached version of state to ensure changes are not permanent if an error occurs. - handler := k.router.GetRoute(pr.ProposalRoute()) - cacheCtx, writeCache := ctx.CacheContext() - if err := handler(cacheCtx, pr.PubProposal); err != nil { + if err := k.ValidatePubProposal(ctx, pr.PubProposal); err != nil { return err } - // write state to the underlying multi-store - writeCache() + handler := k.router.GetRoute(pr.ProposalRoute()) + if err := handler(ctx, pr.PubProposal); err != nil { + // the handler should not error as it was checked in ValidatePubProposal + panic(fmt.Sprintf("unexpected handler error: %s", err)) + } return nil } // CloseExpiredProposals removes proposals (and associated votes) that have past their deadline. +// TODO rename to RemoveExpiredProposals? func (k Keeper) CloseExpiredProposals(ctx sdk.Context) { k.IterateProposals(ctx, func(proposal types.Proposal) bool { @@ -147,7 +149,7 @@ func (k Keeper) CloseExpiredProposals(ctx sdk.Context) { } // ValidatePubProposal checks if a pubproposal is valid. -func (k Keeper) ValidatePubProposal(ctx sdk.Context, pubProposal types.PubProposal) sdk.Error { +func (k Keeper) ValidatePubProposal(ctx sdk.Context, pubProposal types.PubProposal) (returnErr sdk.Error) { if pubProposal == nil { return types.ErrInvalidPubProposal(k.codespace, "pub proposal cannot be nil") } @@ -162,6 +164,17 @@ func (k Keeper) ValidatePubProposal(ctx sdk.Context, pubProposal types.PubPropos // Run the proposal's changes through the associated handler using a cached version of state to ensure changes are not permanent. cacheCtx, _ := ctx.CacheContext() handler := k.router.GetRoute(pubProposal.ProposalRoute()) + + // Handle an edge case where a param change proposal causes the proposal handler to panic. + // A param change proposal with a registered subspace value but unregistered key value will cause a panic in the param change proposal handler. + // This defer will catch panics and return a normal error: `recover()` gets the panic value, then the enclosing function's return value is swapped for an error. + // reference: https://stackoverflow.com/questions/33167282/how-to-return-a-value-in-a-go-function-that-panics?noredirect=1&lq=1 + defer func() { + if r := recover(); r != nil { + returnErr = types.ErrInvalidPubProposal(k.codespace, fmt.Sprintf("proposal handler panicked: %s", r)) + } + }() + if err := handler(cacheCtx, pubProposal); err != nil { return err } @@ -169,6 +182,7 @@ func (k Keeper) ValidatePubProposal(ctx sdk.Context, pubProposal types.PubPropos } // DeleteProposalAndVotes removes a proposal and its associated votes. +// TODO move to keeper.go func (k Keeper) DeleteProposalAndVotes(ctx sdk.Context, proposalID uint64) { var votes []types.Vote k.IterateVotes(ctx, proposalID, func(vote types.Vote) bool { diff --git a/x/committee/keeper/proposal_test.go b/x/committee/keeper/proposal_test.go index 7f77c847..cc6417e5 100644 --- a/x/committee/keeper/proposal_test.go +++ b/x/committee/keeper/proposal_test.go @@ -11,6 +11,7 @@ import ( abci "github.com/tendermint/tendermint/abci/types" "github.com/kava-labs/kava/app" + cdptypes "github.com/kava-labs/kava/x/cdp/types" "github.com/kava-labs/kava/x/committee" "github.com/kava-labs/kava/x/committee/types" ) @@ -33,7 +34,7 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { pubProposal types.PubProposal proposer sdk.AccAddress committeeID uint64 - expectPass bool + expectErr bool }{ { name: "normal", @@ -41,7 +42,7 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), proposer: normalCom.Members[0], committeeID: normalCom.ID, - expectPass: true, + expectErr: false, }, { name: "invalid proposal", @@ -49,7 +50,7 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { pubProposal: nil, proposer: normalCom.Members[0], committeeID: normalCom.ID, - expectPass: false, + expectErr: true, }, { name: "missing committee", @@ -57,7 +58,7 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), proposer: suite.addresses[0], committeeID: 0, - expectPass: false, + expectErr: true, }, { name: "not a member", @@ -65,7 +66,7 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), proposer: suite.addresses[4], committeeID: normalCom.ID, - expectPass: false, + expectErr: true, }, { name: "not enough permissions", @@ -73,7 +74,7 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), proposer: noPermissionsCom.Members[0], committeeID: noPermissionsCom.ID, - expectPass: false, + expectErr: true, }, } @@ -91,14 +92,14 @@ func (suite *KeeperTestSuite) TestSubmitProposal() { id, err := keeper.SubmitProposal(ctx, tc.proposer, tc.committeeID, tc.pubProposal) - if tc.expectPass { + if tc.expectErr { + suite.NotNil(err) + } else { suite.NoError(err) pr, found := keeper.GetProposal(ctx, id) suite.True(found) suite.Equal(tc.committeeID, pr.CommitteeID) suite.Equal(ctx.BlockTime().Add(tc.committee.ProposalDuration), pr.Deadline) - } else { - suite.NotNil(err) } }) } @@ -117,32 +118,32 @@ func (suite *KeeperTestSuite) TestAddVote() { proposalID uint64 voter sdk.AccAddress voteTime time.Time - expectPass bool + expectErr bool }{ { name: "normal", proposalID: types.DefaultNextProposalID, voter: normalCom.Members[0], - expectPass: true, + expectErr: false, }, { name: "nonexistent proposal", proposalID: 9999999, voter: normalCom.Members[0], - expectPass: false, + expectErr: true, }, { name: "voter not committee member", proposalID: types.DefaultNextProposalID, voter: suite.addresses[4], - expectPass: false, + expectErr: true, }, { name: "proposal expired", proposalID: types.DefaultNextProposalID, voter: normalCom.Members[0], voteTime: firstBlockTime.Add(normalCom.ProposalDuration), - expectPass: false, + expectErr: true, }, } @@ -162,12 +163,12 @@ func (suite *KeeperTestSuite) TestAddVote() { ctx = ctx.WithBlockTime(tc.voteTime) err = keeper.AddVote(ctx, tc.proposalID, tc.voter) - if tc.expectPass { + if tc.expectErr { + suite.NotNil(err) + } else { suite.NoError(err) _, found := keeper.GetVote(ctx, tc.proposalID, tc.voter) suite.True(found) - } else { - suite.NotNil(err) } }) } @@ -190,7 +191,7 @@ func (suite *KeeperTestSuite) TestGetProposalResult() { committee types.Committee votes []types.Vote proposalPasses bool - expectPass bool + expectErr bool }{ { name: "enough votes", @@ -202,7 +203,7 @@ func (suite *KeeperTestSuite) TestGetProposalResult() { {ProposalID: defaultID, Voter: suite.addresses[3]}, }, proposalPasses: true, - expectPass: true, + expectErr: false, }, { name: "not enough votes", @@ -211,7 +212,7 @@ func (suite *KeeperTestSuite) TestGetProposalResult() { {ProposalID: defaultID, Voter: suite.addresses[0]}, }, proposalPasses: false, - expectPass: true, + expectErr: false, }, } @@ -238,11 +239,11 @@ func (suite *KeeperTestSuite) TestGetProposalResult() { proposalPasses, err := keeper.GetProposalResult(ctx, defaultID) - if tc.expectPass { + if tc.expectErr { + suite.NotNil(err) + } else { suite.NoError(err) suite.Equal(tc.proposalPasses, proposalPasses) - } else { - suite.NotNil(err) } }) } @@ -272,27 +273,40 @@ func (suite *KeeperTestSuite) TestValidatePubProposal() { testcases := []struct { name string pubProposal types.PubProposal - expectPass bool + expectErr bool }{ { - name: "valid", + name: "valid (text proposal)", pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), - expectPass: true, + expectErr: false, + }, + { + name: "valid (param change proposal)", + pubProposal: params.NewParameterChangeProposal( + "Change the debt limit", + "This proposal changes the debt limit of the cdp module.", + []params.ParamChange{{ + Subspace: cdptypes.ModuleName, + Key: string(cdptypes.KeyGlobalDebtLimit), + Value: string(types.ModuleCdc.MustMarshalJSON(cs(c("usdx", 100000000000)))), + }}, + ), + expectErr: false, }, { name: "invalid (missing title)", pubProposal: gov.TextProposal{Description: "A description of this proposal."}, - expectPass: false, + expectErr: true, }, { name: "invalid (unregistered)", pubProposal: UnregisteredProposal{gov.TextProposal{Title: "A Title", Description: "A description of this proposal."}}, - expectPass: false, + expectErr: true, }, { name: "invalid (nil)", pubProposal: nil, - expectPass: false, + expectErr: true, }, { name: "invalid (proposal handler fails)", @@ -300,24 +314,35 @@ func (suite *KeeperTestSuite) TestValidatePubProposal() { "A Title", "A description of this proposal.", []params.ParamChange{{ - Subspace: "non existant", - Key: "non existant", - Value: "nonsense", + Subspace: "nonsense-subspace", + Key: "nonsense-key", + Value: "nonsense-value", }}, ), - expectPass: false, + expectErr: true, + }, + { + name: "invalid (proposal handler panics)", + pubProposal: params.NewParameterChangeProposal( + "A Title", + "A description of this proposal.", + []params.ParamChange{{ + Subspace: cdptypes.ModuleName, + Key: "nonsense-key", // a valid Subspace but invalid Key will trigger a panic in the paramchange propsal handler + Value: "nonsense-value", + }}, + ), + expectErr: true, }, - // Some proposals can cause the proposal handler to panic. - // However panics will be caught when the proposal is first submitted so should avoid making it onto the chain. } for _, tc := range testcases { suite.Run(tc.name, func() { err := suite.keeper.ValidatePubProposal(suite.ctx, tc.pubProposal) - if tc.expectPass { - suite.NoError(err) - } else { + if tc.expectErr { suite.NotNil(err) + } else { + suite.NoError(err) } }) }