refactor out vote tallying

x/committee/abci.go

@@ -11,12 +11,9 @@ import (
 func BeginBlocker(ctx sdk.Context, _ abci.RequestBeginBlock, k Keeper) {
 
 	// Close all expired proposals
-	// TODO optimize by using an index to avoid iterating over non expired proposals
 	k.IterateProposals(ctx, func(proposal types.Proposal) bool {
 		if proposal.HasExpiredBy(ctx.BlockTime()) {
-			if err := k.CloseOutProposal(ctx, proposal.ID); err != nil {
+			k.DeleteProposalAndVotes(ctx, proposal.ID)
-				panic(err) // if an expired proposal does not close then something has gone very wrong
-			}
 		}
 		return false
 	})

x/committee/handler.go

@@ -52,14 +52,16 @@ func handleMsgVote(ctx sdk.Context, k keeper.Keeper, msg types.MsgVote) sdk.Resu
 		return err.Result()
 	}
 
-	// Try closing proposal in case enough votes have been cast
+	// Enact a proposal if it has enough votes
-	_ = k.CloseOutProposal(ctx, msg.ProposalID)
+	passes, err := k.GetProposalResult(ctx, msg.ProposalID)
-	// if err.Error() == "note enough votes to close proposal" { // TODO
+	if err != nil {
-	// 	return nil // This is not a reason to error
+		return err.Result()
-	// }
+	}
-	// if err != nil {
+	if passes {
-	// 	return err
+		_ = k.EnactProposal(ctx, msg.ProposalID)
-	// }
+		// log err
+		k.DeleteProposalAndVotes(ctx, msg.ProposalID)
+	}
 
 	ctx.EventManager().EmitEvent(
 		sdk.NewEvent(

x/committee/keeper/proposal.go

@@ -8,6 +8,7 @@ import (
 	"github.com/kava-labs/kava/x/committee/types"
 )
 
+// SubmitProposal adds a proposal to a committee so that it can be voted on.
 func (k Keeper) SubmitProposal(ctx sdk.Context, proposer sdk.AccAddress, committeeID uint64, pubProposal types.PubProposal) (uint64, sdk.Error) {
 	// Limit proposals to only be submitted by committee members
 	com, found := k.GetCommittee(ctx, committeeID)
@@ -44,6 +45,7 @@ func (k Keeper) SubmitProposal(ctx sdk.Context, proposer sdk.AccAddress, committ
 	return proposalID, nil
 }
 
+// AddVote submits a vote on a proposal.
 func (k Keeper) AddVote(ctx sdk.Context, proposalID uint64, voter sdk.AccAddress) sdk.Error {
 	// Validate
 	pr, found := k.GetProposal(ctx, proposalID)
@@ -67,44 +69,55 @@ func (k Keeper) AddVote(ctx sdk.Context, proposalID uint64, voter sdk.AccAddress
 	return nil
 }
 
-func (k Keeper) CloseOutProposal(ctx sdk.Context, proposalID uint64) sdk.Error {
+// GetProposalResult calculates if a proposal currently has enough votes to pass.
+func (k Keeper) GetProposalResult(ctx sdk.Context, proposalID uint64) (bool, sdk.Error) {
 	pr, found := k.GetProposal(ctx, proposalID)
 	if !found {
-		return sdk.ErrInternal("proposal not found")
+		return false, sdk.ErrInternal("proposal not found")
 	}
 	com, found := k.GetCommittee(ctx, pr.CommitteeID)
 	if !found {
-		return sdk.ErrInternal("committee disbanded")
+		return false, sdk.ErrInternal("committee disbanded")
 	}
 
+	numVotes := k.TallyVotes(ctx, proposalID)
+
+	proposalResult := sdk.NewDec(numVotes).GTE(types.VoteThreshold.MulInt64(int64(len(com.Members))))
+
+	return proposalResult, nil
+}
+
+// TallyVotes counts all the votes on a proposal
+func (k Keeper) TallyVotes(ctx sdk.Context, proposalID uint64) int64 {
+
 	var votes []types.Vote
 	k.IterateVotes(ctx, proposalID, func(vote types.Vote) bool {
 		votes = append(votes, vote)
 		return false
 	})
-	proposalPasses := sdk.NewDec(int64(len(votes))).GTE(types.VoteThreshold.MulInt64(int64(len(com.Members))))
 
-	if proposalPasses {
+	return int64(len(votes))
-		// eneact vote
-		// The proposal handler may execute state mutating logic depending
-		// on the proposal content. If the handler fails, no state mutation
-		// is written and the error message is logged.
-		handler := k.router.GetRoute(pr.ProposalRoute())
-		cacheCtx, writeCache := ctx.CacheContext()
-		err := handler(cacheCtx, pr.PubProposal) // need to pass pubProposal as the handlers type assert it into the concrete types
-		if err == nil {
-			// write state to the underlying multi-store
-			writeCache()
-		} // if handler returns error, then still delete the proposal - it's still over, but send an event
-	}
-	if proposalPasses || pr.HasExpiredBy(ctx.BlockTime()) {
-
-		k.DeleteProposalAndVotes(ctx, proposalID)
-		return nil
-	}
-	return sdk.ErrInternal("note enough votes to close proposal")
 }
 
+// EnactProposal makes the changes proposed in a proposal.
+func (k Keeper) EnactProposal(ctx sdk.Context, proposalID uint64) sdk.Error {
+	pr, found := k.GetProposal(ctx, proposalID)
+	if !found {
+		return sdk.ErrInternal("proposal not found")
+	}
+
+	// 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 {
+		return err
+	}
+	// write state to the underlying multi-store
+	writeCache()
+	return nil
+}
+
+// ValidatePubProposal checks if a pubproposal is valid.
 func (k Keeper) ValidatePubProposal(ctx sdk.Context, pubProposal types.PubProposal) sdk.Error {
 	if pubProposal == nil {
 		return sdk.ErrInternal("proposal is empty")
@@ -117,18 +130,16 @@ func (k Keeper) ValidatePubProposal(ctx sdk.Context, pubProposal types.PubPropos
 		return sdk.ErrInternal("no handler found for proposal")
 	}
 
-	// Execute the proposal content in a cache-wrapped context to validate the
+	// Run the proposal's changes through the associated handler using a cached version of state to ensure changes are not permanent.
-	// actual parameter changes before the proposal proceeds through the
-	// governance process. State is not persisted.
 	cacheCtx, _ := ctx.CacheContext()
 	handler := k.router.GetRoute(pubProposal.ProposalRoute())
 	if err := handler(cacheCtx, pubProposal); err != nil {
 		return err
 	}
-
 	return nil
 }
 
+// DeleteProposalAndVotes removes a proposal and its associated votes.
 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

@@ -4,11 +4,13 @@ import (
 	"reflect"
 	"time"
 
+	"github.com/cosmos/cosmos-sdk/codec"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/gov"
 	abci "github.com/tendermint/tendermint/abci/types"
 
 	"github.com/kava-labs/kava/app"
+	"github.com/kava-labs/kava/x/committee"
 	"github.com/kava-labs/kava/x/committee/types"
 )
 
@@ -76,7 +78,8 @@ func (suite *KeeperTestSuite) TestSubmitProposal() {
 
 	for _, tc := range testcases {
 		suite.Run(tc.name, func() {
-			// Create local testApp because suite doesn't run the SetupTest function for subtests, which would mean the app state is not be reset between subtests.
+			// Create local testApp because suite doesn't run the SetupTest function for subtests,
+			// which would mean the app state is not be reset between subtests.
 			tApp := app.NewTestApp()
 			keeper := tApp.GetCommitteeKeeper()
 			ctx := tApp.NewContext(true, abci.Header{})
@@ -170,36 +173,89 @@ func (suite *KeeperTestSuite) TestAddVote() {
 	}
 }
 
-func (suite *KeeperTestSuite) TestCloseOutProposal() {
+func (suite *KeeperTestSuite) TestGetProposalResult() {
-	// setup test
+	var defaultID uint64 = 1
-	suite.app.InitializeFromGenesisStates()
+	firstBlockTime := time.Date(1998, time.January, 1, 1, 0, 0, 0, time.UTC)
-	// TODO replace below with genesis state
+
-	normalCom := types.Committee{
+	testcases := []struct {
+		name           string
+		committee      types.Committee
+		votes          []types.Vote
+		proposalPasses bool
+		expectPass     bool
+	}{
+		{
+			name: "enough votes",
+			committee: types.Committee{
 				ID:          12,
-		Members:     suite.addresses[:2],
+				Members:     suite.addresses[:5],
 				Permissions: []types.Permission{types.GodPermission{}},
+			},
+			votes: []types.Vote{
+				{ProposalID: defaultID, Voter: suite.addresses[0]},
+				{ProposalID: defaultID, Voter: suite.addresses[1]},
+				{ProposalID: defaultID, Voter: suite.addresses[2]},
+				{ProposalID: defaultID, Voter: suite.addresses[3]},
+			},
+			proposalPasses: true,
+			expectPass:     true,
+		},
+		{
+			name: "not enough votes",
+			committee: types.Committee{
+				ID:          12,
+				Members:     suite.addresses[:5],
+				Permissions: []types.Permission{types.GodPermission{}},
+			},
+			votes: []types.Vote{
+				{ProposalID: defaultID, Voter: suite.addresses[0]},
+			},
+			proposalPasses: false,
+			expectPass:     true,
+		},
 	}
-	suite.keeper.SetCommittee(suite.ctx, normalCom)
-	pprop := gov.NewTextProposal("A Title", "A description of this proposal.")
-	id, err := suite.keeper.SubmitProposal(suite.ctx, normalCom.Members[0], normalCom.ID, pprop)
-	suite.NoError(err)
-	err = suite.keeper.AddVote(suite.ctx, id, normalCom.Members[0])
-	suite.NoError(err)
-	err = suite.keeper.AddVote(suite.ctx, id, normalCom.Members[1])
-	suite.NoError(err)
 
-	// run test
+	for _, tc := range testcases {
-	err = suite.keeper.CloseOutProposal(suite.ctx, id)
+		suite.Run(tc.name, func() {
+			// Create local testApp because suite doesn't run the SetupTest function for subtests, which would mean the app state is not be reset between subtests.
+			tApp := app.NewTestApp()
+			keeper := tApp.GetCommitteeKeeper()
+			ctx := tApp.NewContext(true, abci.Header{Height: 1, Time: firstBlockTime})
 
-	// check
+			tApp.InitializeFromGenesisStates(
+				committeeGenState(
+					tApp.Codec(),
+					[]types.Committee{tc.committee},
+					[]types.Proposal{{
+						PubProposal: gov.NewTextProposal("A Title", "A description of this proposal."),
+						ID:          defaultID,
+						CommitteeID: tc.committee.ID,
+						Deadline:    firstBlockTime.Add(time.Hour * 24 * 7),
+					}},
+					tc.votes,
+				),
+			)
+
+			proposalPasses, err := keeper.GetProposalResult(ctx, defaultID)
+
+			if tc.expectPass {
 				suite.NoError(err)
-	_, found := suite.keeper.GetProposal(suite.ctx, id)
+				suite.Equal(tc.proposalPasses, proposalPasses)
-	suite.False(found)
+			} else {
-	suite.keeper.IterateVotes(suite.ctx, id, func(v types.Vote) bool {
+				suite.NotNil(err)
-		suite.Fail("found vote when none should exist")
+			}
-		return false
 		})
+	}
+}
 
+func committeeGenState(cdc *codec.Codec, committees []types.Committee, proposals []types.Proposal, votes []types.Vote) app.GenesisState {
+	gs := types.NewGenesisState(
+		uint64(len(proposals)+1),
+		committees,
+		proposals,
+		votes,
+	)
+	return app.GenesisState{committee.ModuleName: cdc.MustMarshalJSON(gs)}
 }
 
 type UnregisteredProposal struct {

x/committee/keeper/querier.go

@@ -168,24 +168,13 @@ func queryTally(ctx sdk.Context, path []string, req abci.RequestQuery, keeper Ke
 		return nil, sdk.ErrUnknownRequest(sdk.AppendMsgToErr("incorrectly formatted request data", err.Error()))
 	}
 
-	// TODO split tally and process result logic so tally logic can be used here
+	_, found := keeper.GetProposal(ctx, params.ProposalID)
-	pr, found := keeper.GetProposal(ctx, params.ProposalID)
 	if !found {
 		return nil, sdk.ErrInternal("proposal not found")
 	}
-	com, found := keeper.GetCommittee(ctx, pr.CommitteeID)
+	numVotes := keeper.TallyVotes(ctx, params.ProposalID)
-	if !found {
-		return nil, sdk.ErrInternal("committee disbanded")
-	}
-	votes := []types.Vote{}
-	keeper.IterateVotes(ctx, params.ProposalID, func(vote types.Vote) bool {
-		votes = append(votes, vote)
-		return false
-	})
-	proposalPasses := sdk.NewDec(int64(len(votes))).GTE(types.VoteThreshold.MulInt64(int64(len(com.Members))))
-	// TODO return some kind of tally object, rather than just a bool
 
-	bz, err := codec.MarshalJSONIndent(keeper.cdc, proposalPasses)
+	bz, err := codec.MarshalJSONIndent(keeper.cdc, numVotes)
 	if err != nil {
 		return nil, sdk.ErrInternal(sdk.AppendMsgToErr("could not marshal result to JSON", err.Error()))
 	}

x/committee/keeper/querier_test.go

@@ -34,13 +34,9 @@ type QuerierTestSuite struct {
 	committees []types.Committee
 	proposals  []types.Proposal
 	votes      map[uint64]([]types.Vote)
-	expectedTallyForTheFirstProposal bool // TODO replace once tallying has been refactored
 }
 
 func (suite *QuerierTestSuite) SetupTest() {
-	// SetupTest function runs before every test, but a new suite is not created every time.
-	// So be careful about modifying data on suite as data from previous tests will still be there.
-	// For example, don't append proposal to suite.proposals, initialize a new slice value.
 	suite.app = app.NewTestApp()
 	suite.keeper = suite.app.GetCommitteeKeeper()
 	suite.ctx = suite.app.NewContext(true, abci.Header{})
@@ -87,8 +83,6 @@ func (suite *QuerierTestSuite) SetupTest() {
 		})
 		return false
 	})
-	suite.expectedTallyForTheFirstProposal = true // TODO replace once tallying has been refactored
-
 }
 
 func (suite *QuerierTestSuite) TestQueryCommittees() {
@@ -240,12 +234,11 @@ func (suite *QuerierTestSuite) TestQueryTally() {
 	suite.NotNil(bz)
 
 	// Unmarshal the bytes
-	var tally bool
+	var tally int64
 	suite.NoError(suite.cdc.UnmarshalJSON(bz, &tally))
 
 	// Check
-	expectedTally := suite.expectedTallyForTheFirstProposal
+	suite.Equal(int64(len(suite.votes[propID])), tally)
-	suite.Equal(expectedTally, tally)
 }
 func TestQuerierTestSuite(t *testing.T) {
 	suite.Run(t, new(QuerierTestSuite))