From c50f6bc9fa3119e34c6363ff4b8f885f6b46fbb4 Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Thu, 26 Mar 2020 20:17:49 +0000 Subject: [PATCH] refactor out vote tallying --- x/committee/abci.go | 5 +- x/committee/handler.go | 18 +++-- x/committee/keeper/proposal.go | 65 +++++++++------- x/committee/keeper/proposal_test.go | 110 +++++++++++++++++++++------- x/committee/keeper/querier.go | 17 +---- x/committee/keeper/querier_test.go | 19 ++--- 6 files changed, 141 insertions(+), 93 deletions(-) diff --git a/x/committee/abci.go b/x/committee/abci.go index 5ab25598..7b56d99e 100644 --- a/x/committee/abci.go +++ b/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 { - panic(err) // if an expired proposal does not close then something has gone very wrong - } + k.DeleteProposalAndVotes(ctx, proposal.ID) } return false }) diff --git a/x/committee/handler.go b/x/committee/handler.go index bbc0530c..e52989f7 100644 --- a/x/committee/handler.go +++ b/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 - _ = k.CloseOutProposal(ctx, msg.ProposalID) - // if err.Error() == "note enough votes to close proposal" { // TODO - // return nil // This is not a reason to error - // } - // if err != nil { - // return err - // } + // Enact a proposal if it has enough votes + passes, err := k.GetProposalResult(ctx, msg.ProposalID) + if err != nil { + return err.Result() + } + if passes { + _ = k.EnactProposal(ctx, msg.ProposalID) + // log err + k.DeleteProposalAndVotes(ctx, msg.ProposalID) + } ctx.EventManager().EmitEvent( sdk.NewEvent( diff --git a/x/committee/keeper/proposal.go b/x/committee/keeper/proposal.go index f42be0f3..6aeefeca 100644 --- a/x/committee/keeper/proposal.go +++ b/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 { - // 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") + return int64(len(votes)) } +// 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 - // actual parameter changes before the proposal proceeds through the - // governance process. State is not persisted. + // 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()) 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 { diff --git a/x/committee/keeper/proposal_test.go b/x/committee/keeper/proposal_test.go index 0d9edcc2..c61c3c54 100644 --- a/x/committee/keeper/proposal_test.go +++ b/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() { - // setup test - suite.app.InitializeFromGenesisStates() - // TODO replace below with genesis state - normalCom := types.Committee{ - ID: 12, - Members: suite.addresses[:2], - Permissions: []types.Permission{types.GodPermission{}}, +func (suite *KeeperTestSuite) TestGetProposalResult() { + var defaultID uint64 = 1 + firstBlockTime := time.Date(1998, time.January, 1, 1, 0, 0, 0, time.UTC) + + 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[: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 - err = suite.keeper.CloseOutProposal(suite.ctx, id) + 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. + tApp := app.NewTestApp() + keeper := tApp.GetCommitteeKeeper() + ctx := tApp.NewContext(true, abci.Header{Height: 1, Time: firstBlockTime}) - // check - suite.NoError(err) - _, found := suite.keeper.GetProposal(suite.ctx, id) - suite.False(found) - suite.keeper.IterateVotes(suite.ctx, id, func(v types.Vote) bool { - suite.Fail("found vote when none should exist") - return false - }) + 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) + suite.Equal(tc.proposalPasses, proposalPasses) + } else { + suite.NotNil(err) + } + }) + } +} + +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 { diff --git a/x/committee/keeper/querier.go b/x/committee/keeper/querier.go index 44dfa877..c378d750 100644 --- a/x/committee/keeper/querier.go +++ b/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 - pr, found := keeper.GetProposal(ctx, params.ProposalID) + _, found := keeper.GetProposal(ctx, params.ProposalID) if !found { return nil, sdk.ErrInternal("proposal not found") } - com, found := keeper.GetCommittee(ctx, pr.CommitteeID) - 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 + numVotes := keeper.TallyVotes(ctx, params.ProposalID) - 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())) } diff --git a/x/committee/keeper/querier_test.go b/x/committee/keeper/querier_test.go index a2d8fd7e..2d532d23 100644 --- a/x/committee/keeper/querier_test.go +++ b/x/committee/keeper/querier_test.go @@ -30,17 +30,13 @@ type QuerierTestSuite struct { querier sdk.Querier - addresses []sdk.AccAddress - committees []types.Committee - proposals []types.Proposal - votes map[uint64]([]types.Vote) - expectedTallyForTheFirstProposal bool // TODO replace once tallying has been refactored + addresses []sdk.AccAddress + committees []types.Committee + proposals []types.Proposal + votes map[uint64]([]types.Vote) } 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(expectedTally, tally) + suite.Equal(int64(len(suite.votes[propID])), tally) } func TestQuerierTestSuite(t *testing.T) { suite.Run(t, new(QuerierTestSuite))