improve proposal tests

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) {
 

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
 

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.
+	if err := k.ValidatePubProposal(ctx, pr.PubProposal); err != nil {
-	handler := k.router.GetRoute(pr.ProposalRoute())
-	cacheCtx, writeCache := ctx.CacheContext()
-	if err := handler(cacheCtx, pr.PubProposal); err != nil {
 		return err
 	}
-	// write state to the underlying multi-store
+	handler := k.router.GetRoute(pr.ProposalRoute())
-	writeCache()
+	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 {

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",
+					Subspace: "nonsense-subspace",
-					Key:      "non existant",
+					Key:      "nonsense-key",
-					Value:    "nonsense",
+					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 {
+			if tc.expectErr {
-				suite.NoError(err)
-			} else {
 				suite.NotNil(err)
+			} else {
+				suite.NoError(err)
 			}
 		})
 	}