From 5911e648b7ab7e37fc0d3ccb34be5e65a56190d5 Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Wed, 11 Mar 2020 19:52:25 +0000 Subject: [PATCH] improve code organisation --- x/committee/keeper/keeper.go | 146 +++-------------- x/committee/keeper/keeper_test.go | 231 -------------------------- x/committee/keeper/proposal.go | 117 ++++++++++++++ x/committee/keeper/proposal_test.go | 241 ++++++++++++++++++++++++++++ x/committee/types/keys.go | 6 - x/committee/types/types.go | 23 ++- 6 files changed, 388 insertions(+), 376 deletions(-) create mode 100644 x/committee/keeper/proposal.go create mode 100644 x/committee/keeper/proposal_test.go diff --git a/x/committee/keeper/keeper.go b/x/committee/keeper/keeper.go index 033deebd..523ba417 100644 --- a/x/committee/keeper/keeper.go +++ b/x/committee/keeper/keeper.go @@ -33,115 +33,7 @@ func NewKeeper(cdc *codec.Codec, storeKey sdk.StoreKey, router govtypes.Router) } } -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) - if !found { - return 0, sdk.ErrInternal("committee doesn't exist") - } - if !com.HasMember(proposer) { - return 0, sdk.ErrInternal("only member can propose proposals") - } - - // Check committee has permissions to enact proposal. - if !com.HasPermissionsFor(pubProposal) { - return 0, sdk.ErrInternal("committee does not have permissions to enact proposal") - } - - // Check proposal is valid - // TODO what if it's not valid now but will be in the future? - // TODO does this need to be before permission check? - if err := k.ValidatePubProposal(ctx, pubProposal); err != nil { - return 0, err - } - - // Get a new ID and store the proposal - return k.StoreNewProposal(ctx, committeeID, pubProposal) -} - -func (k Keeper) AddVote(ctx sdk.Context, proposalID uint64, voter sdk.AccAddress) sdk.Error { - // Validate - pr, found := k.GetProposal(ctx, proposalID) - if !found { - return sdk.ErrInternal("proposal not found") - } - com, found := k.GetCommittee(ctx, pr.CommitteeID) - if !found { - return sdk.ErrInternal("committee disbanded") - } - if !com.HasMember(voter) { - return sdk.ErrInternal("not authorized to vote on proposal") - } - - // Store vote, overwriting any prior vote - k.SetVote(ctx, types.Vote{ProposalID: proposalID, Voter: voter}) - - return nil -} - -func (k Keeper) CloseOutProposal(ctx sdk.Context, proposalID uint64) sdk.Error { - pr, found := k.GetProposal(ctx, proposalID) - if !found { - return sdk.ErrInternal("proposal not found") - } - com, found := k.GetCommittee(ctx, pr.CommitteeID) - if !found { - return sdk.ErrInternal("committee disbanded") - } - - var votes []types.Vote - k.IterateVotes(ctx, proposalID, func(vote types.Vote) bool { - votes = append(votes, vote) - return false - }) - if sdk.NewDec(int64(len(votes))).GTE(types.VoteThreshold.MulInt64(int64(len(com.Members)))) { // TODO move vote counting stuff to committee methods // TODO add timeout check here - close if expired regardless of 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 - - // delete proposal and votes - k.DeleteProposal(ctx, proposalID) - for _, v := range votes { - k.DeleteVote(ctx, v.ProposalID, v.Voter) - } - } else { - return sdk.ErrInternal("note enough votes to close proposal") - } - return nil -} - -func (k Keeper) ValidatePubProposal(ctx sdk.Context, pubProposal types.PubProposal) sdk.Error { - // TODO not sure if the basic validation is required - should be run in msg.ValidateBasic - if pubProposal == nil { - return sdk.ErrInternal("proposal is empty") - } - if err := pubProposal.ValidateBasic(); err != nil { - return err - } - - if !k.router.HasRoute(pubProposal.ProposalRoute()) { - 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. - cacheCtx, _ := ctx.CacheContext() - handler := k.router.GetRoute(pubProposal.ProposalRoute()) - if err := handler(cacheCtx, pubProposal); err != nil { - return err - } - - return nil -} +// ---------- Committees ---------- // GetCommittee gets a committee from the store. func (k Keeper) GetCommittee(ctx sdk.Context, committeeID uint64) (types.Committee, bool) { @@ -168,6 +60,8 @@ func (k Keeper) DeleteCommittee(ctx sdk.Context, committeeID uint64) { store.Delete(types.GetKeyFromID(committeeID)) } +// ---------- Proposals ---------- + // SetNextProposalID stores an ID to be used for the next created proposal func (k Keeper) SetNextProposalID(ctx sdk.Context, id uint64) { store := ctx.KVStore(k.storeKey) @@ -240,22 +134,7 @@ func (k Keeper) DeleteProposal(ctx sdk.Context, proposalID uint64) { store.Delete(types.GetKeyFromID(proposalID)) } -// IterateVotes provides an iterator over all stored votes for a given proposal. -// For each vote, cb will be called. If cb returns true, the iterator will close and stop. -func (k Keeper) IterateVotes(ctx sdk.Context, proposalID uint64, cb func(vote types.Vote) (stop bool)) { - // iterate over the section of the votes store that has all votes for a particular proposal - iterator := sdk.KVStorePrefixIterator(ctx.KVStore(k.storeKey), append(types.VoteKeyPrefix, types.GetKeyFromID(proposalID)...)) - - defer iterator.Close() - for ; iterator.Valid(); iterator.Next() { - var vote types.Vote - k.cdc.MustUnmarshalBinaryLengthPrefixed(iterator.Value(), &vote) - - if cb(vote) { - break - } - } -} +// ---------- Votes ---------- // GetVote gets a vote from the store. func (k Keeper) GetVote(ctx sdk.Context, proposalID uint64, voter sdk.AccAddress) (types.Vote, bool) { @@ -281,3 +160,20 @@ func (k Keeper) DeleteVote(ctx sdk.Context, proposalID uint64, voter sdk.AccAddr store := prefix.NewStore(ctx.KVStore(k.storeKey), types.VoteKeyPrefix) store.Delete(types.GetVoteKey(proposalID, voter)) } + +// IterateVotes provides an iterator over all stored votes for a given proposal. +// For each vote, cb will be called. If cb returns true, the iterator will close and stop. +func (k Keeper) IterateVotes(ctx sdk.Context, proposalID uint64, cb func(vote types.Vote) (stop bool)) { + // iterate over the section of the votes store that has all votes for a particular proposal + iterator := sdk.KVStorePrefixIterator(ctx.KVStore(k.storeKey), append(types.VoteKeyPrefix, types.GetKeyFromID(proposalID)...)) + + defer iterator.Close() + for ; iterator.Valid(); iterator.Next() { + var vote types.Vote + k.cdc.MustUnmarshalBinaryLengthPrefixed(iterator.Value(), &vote) + + if cb(vote) { + break + } + } +} diff --git a/x/committee/keeper/keeper_test.go b/x/committee/keeper/keeper_test.go index 5605ce99..f57e380a 100644 --- a/x/committee/keeper/keeper_test.go +++ b/x/committee/keeper/keeper_test.go @@ -1,13 +1,11 @@ package keeper_test import ( - "reflect" "testing" "github.com/stretchr/testify/suite" 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" @@ -32,235 +30,6 @@ func (suite *KeeperTestSuite) SetupTest() { _, suite.addresses = app.GeneratePrivKeyAddressPairs(5) } -func (suite *KeeperTestSuite) TestSubmitProposal() { - normalCom := types.Committee{ - ID: 12, - Members: suite.addresses[:2], - Permissions: []types.Permission{types.GodPermission{}}, - } - noPermissionsCom := types.Committee{ - ID: 12, - Members: suite.addresses[:2], - Permissions: []types.Permission{}, - } - - testcases := []struct { - name string - committee types.Committee - pubProposal types.PubProposal - proposer sdk.AccAddress - committeeID uint64 - expectPass bool - }{ - { - name: "normal", - committee: normalCom, - pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), - proposer: normalCom.Members[0], - committeeID: normalCom.ID, - expectPass: true, - }, - { - name: "invalid proposal", - committee: normalCom, - pubProposal: nil, - proposer: normalCom.Members[0], - committeeID: normalCom.ID, - expectPass: false, - }, - { - name: "missing committee", - // no committee - pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), - proposer: suite.addresses[0], - committeeID: 0, - expectPass: false, - }, - { - name: "not a member", - committee: normalCom, - pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), - proposer: suite.addresses[4], - committeeID: normalCom.ID, - expectPass: false, - }, - { - name: "not enough permissions", - committee: noPermissionsCom, - pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), - proposer: noPermissionsCom.Members[0], - committeeID: noPermissionsCom.ID, - expectPass: false, - }, - } - - 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{}) - tApp.InitializeFromGenesisStates() - // setup committee (if required) - if !(reflect.DeepEqual(tc.committee, types.Committee{})) { - keeper.SetCommittee(ctx, tc.committee) - } - - id, err := keeper.SubmitProposal(ctx, tc.proposer, tc.committeeID, tc.pubProposal) - - if tc.expectPass { - suite.NoError(err) - _, found := keeper.GetProposal(ctx, id) - suite.True(found) - } else { - suite.NotNil(err) - } - }) - } -} - -func (suite *KeeperTestSuite) TestAddVote() { - normalCom := types.Committee{ - ID: 12, - Members: suite.addresses[:2], - Permissions: []types.Permission{types.GodPermission{}}, - } - - testcases := []struct { - name string - proposalID uint64 - voter sdk.AccAddress - expectPass bool - }{ - { - name: "normal", - proposalID: types.DefaultNextProposalID, - voter: normalCom.Members[0], - expectPass: true, - }, - { - name: "nonexistent proposal", - proposalID: 9999999, - voter: normalCom.Members[0], - expectPass: false, - }, - { - name: "voter not committee member", - proposalID: types.DefaultNextProposalID, - voter: suite.addresses[4], - expectPass: false, - }, - } - - 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{}) - tApp.InitializeFromGenesisStates() - - // setup the committee and proposal - keeper.SetCommittee(ctx, normalCom) - _, err := keeper.SubmitProposal(ctx, normalCom.Members[0], normalCom.ID, gov.NewTextProposal("A Title", "A description of this proposal.")) - suite.NoError(err) - - err = keeper.AddVote(ctx, tc.proposalID, tc.voter) - - if tc.expectPass { - suite.NoError(err) - _, found := keeper.GetVote(ctx, tc.proposalID, tc.voter) - suite.True(found) - } else { - suite.NotNil(err) - } - }) - } -} - -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{}}, - } - 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) - - // 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 - }) - -} - -type UnregisteredProposal struct { - gov.TextProposal -} - -func (UnregisteredProposal) ProposalRoute() string { return "unregistered" } -func (UnregisteredProposal) ProposalType() string { return "unregistered" } - -var _ types.PubProposal = UnregisteredProposal{} - -func (suite *KeeperTestSuite) TestValidatePubProposal() { - - testcases := []struct { - name string - pubProposal types.PubProposal - expectPass bool - }{ - { - name: "valid", - pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), - expectPass: true, - }, - { - name: "invalid (missing title)", - pubProposal: gov.TextProposal{Description: "A description of this proposal."}, - expectPass: false, - }, - { - name: "invalid (unregistered)", - pubProposal: UnregisteredProposal{gov.TextProposal{Title: "A Title", Description: "A description of this proposal."}}, - expectPass: false, - }, - { - name: "invalid (nil)", - pubProposal: nil, - expectPass: false, - }, - // TODO test case when the handler fails - } - - for _, tc := range testcases { - suite.Run(tc.name, func() { - err := suite.keeper.ValidatePubProposal(suite.ctx, tc.pubProposal) - if tc.expectPass { - suite.NoError(err) - } else { - suite.NotNil(err) - } - }) - } -} - func (suite *KeeperTestSuite) TestGetSetDeleteCommittee() { // setup test com := types.Committee{ diff --git a/x/committee/keeper/proposal.go b/x/committee/keeper/proposal.go new file mode 100644 index 00000000..acb604ba --- /dev/null +++ b/x/committee/keeper/proposal.go @@ -0,0 +1,117 @@ +package keeper + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/kava-labs/kava/x/committee/types" +) + +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) + if !found { + return 0, sdk.ErrInternal("committee doesn't exist") + } + if !com.HasMember(proposer) { + return 0, sdk.ErrInternal("only member can propose proposals") + } + + // Check committee has permissions to enact proposal. + if !com.HasPermissionsFor(pubProposal) { + return 0, sdk.ErrInternal("committee does not have permissions to enact proposal") + } + + // Check proposal is valid + // TODO what if it's not valid now but will be in the future? + // TODO does this need to be before permission check? + if err := k.ValidatePubProposal(ctx, pubProposal); err != nil { + return 0, err + } + + // Get a new ID and store the proposal + return k.StoreNewProposal(ctx, committeeID, pubProposal) +} + +func (k Keeper) AddVote(ctx sdk.Context, proposalID uint64, voter sdk.AccAddress) sdk.Error { + // Validate + pr, found := k.GetProposal(ctx, proposalID) + if !found { + return sdk.ErrInternal("proposal not found") + } + com, found := k.GetCommittee(ctx, pr.CommitteeID) + if !found { + return sdk.ErrInternal("committee disbanded") + } + if !com.HasMember(voter) { + return sdk.ErrInternal("not authorized to vote on proposal") + } + + // Store vote, overwriting any prior vote + k.SetVote(ctx, types.Vote{ProposalID: proposalID, Voter: voter}) + + return nil +} + +func (k Keeper) CloseOutProposal(ctx sdk.Context, proposalID uint64) sdk.Error { + pr, found := k.GetProposal(ctx, proposalID) + if !found { + return sdk.ErrInternal("proposal not found") + } + com, found := k.GetCommittee(ctx, pr.CommitteeID) + if !found { + return sdk.ErrInternal("committee disbanded") + } + + var votes []types.Vote + k.IterateVotes(ctx, proposalID, func(vote types.Vote) bool { + votes = append(votes, vote) + return false + }) + if sdk.NewDec(int64(len(votes))).GTE(types.VoteThreshold.MulInt64(int64(len(com.Members)))) { // TODO move vote counting stuff to committee methods // TODO add timeout check here - close if expired regardless of 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 + + // delete proposal and votes + k.DeleteProposal(ctx, proposalID) + for _, v := range votes { + k.DeleteVote(ctx, v.ProposalID, v.Voter) + } + } else { + return sdk.ErrInternal("note enough votes to close proposal") + } + return nil +} + +func (k Keeper) ValidatePubProposal(ctx sdk.Context, pubProposal types.PubProposal) sdk.Error { + // TODO not sure if the basic validation is required - should be run in msg.ValidateBasic + if pubProposal == nil { + return sdk.ErrInternal("proposal is empty") + } + if err := pubProposal.ValidateBasic(); err != nil { + return err + } + + if !k.router.HasRoute(pubProposal.ProposalRoute()) { + 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. + cacheCtx, _ := ctx.CacheContext() + handler := k.router.GetRoute(pubProposal.ProposalRoute()) + if err := handler(cacheCtx, pubProposal); err != nil { + return err + } + + return nil +} diff --git a/x/committee/keeper/proposal_test.go b/x/committee/keeper/proposal_test.go new file mode 100644 index 00000000..fe5ca6c7 --- /dev/null +++ b/x/committee/keeper/proposal_test.go @@ -0,0 +1,241 @@ +package keeper_test + +import ( + "reflect" + + 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/types" +) + +func (suite *KeeperTestSuite) TestSubmitProposal() { + normalCom := types.Committee{ + ID: 12, + Members: suite.addresses[:2], + Permissions: []types.Permission{types.GodPermission{}}, + } + noPermissionsCom := types.Committee{ + ID: 12, + Members: suite.addresses[:2], + Permissions: []types.Permission{}, + } + + testcases := []struct { + name string + committee types.Committee + pubProposal types.PubProposal + proposer sdk.AccAddress + committeeID uint64 + expectPass bool + }{ + { + name: "normal", + committee: normalCom, + pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), + proposer: normalCom.Members[0], + committeeID: normalCom.ID, + expectPass: true, + }, + { + name: "invalid proposal", + committee: normalCom, + pubProposal: nil, + proposer: normalCom.Members[0], + committeeID: normalCom.ID, + expectPass: false, + }, + { + name: "missing committee", + // no committee + pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), + proposer: suite.addresses[0], + committeeID: 0, + expectPass: false, + }, + { + name: "not a member", + committee: normalCom, + pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), + proposer: suite.addresses[4], + committeeID: normalCom.ID, + expectPass: false, + }, + { + name: "not enough permissions", + committee: noPermissionsCom, + pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), + proposer: noPermissionsCom.Members[0], + committeeID: noPermissionsCom.ID, + expectPass: false, + }, + } + + 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{}) + tApp.InitializeFromGenesisStates() + // setup committee (if required) + if !(reflect.DeepEqual(tc.committee, types.Committee{})) { + keeper.SetCommittee(ctx, tc.committee) + } + + id, err := keeper.SubmitProposal(ctx, tc.proposer, tc.committeeID, tc.pubProposal) + + if tc.expectPass { + suite.NoError(err) + _, found := keeper.GetProposal(ctx, id) + suite.True(found) + } else { + suite.NotNil(err) + } + }) + } +} + +func (suite *KeeperTestSuite) TestAddVote() { + normalCom := types.Committee{ + ID: 12, + Members: suite.addresses[:2], + Permissions: []types.Permission{types.GodPermission{}}, + } + + testcases := []struct { + name string + proposalID uint64 + voter sdk.AccAddress + expectPass bool + }{ + { + name: "normal", + proposalID: types.DefaultNextProposalID, + voter: normalCom.Members[0], + expectPass: true, + }, + { + name: "nonexistent proposal", + proposalID: 9999999, + voter: normalCom.Members[0], + expectPass: false, + }, + { + name: "voter not committee member", + proposalID: types.DefaultNextProposalID, + voter: suite.addresses[4], + expectPass: false, + }, + } + + 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{}) + tApp.InitializeFromGenesisStates() + + // setup the committee and proposal + keeper.SetCommittee(ctx, normalCom) + _, err := keeper.SubmitProposal(ctx, normalCom.Members[0], normalCom.ID, gov.NewTextProposal("A Title", "A description of this proposal.")) + suite.NoError(err) + + err = keeper.AddVote(ctx, tc.proposalID, tc.voter) + + if tc.expectPass { + suite.NoError(err) + _, found := keeper.GetVote(ctx, tc.proposalID, tc.voter) + suite.True(found) + } else { + suite.NotNil(err) + } + }) + } +} + +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{}}, + } + 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) + + // 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 + }) + +} + +type UnregisteredProposal struct { + gov.TextProposal +} + +func (UnregisteredProposal) ProposalRoute() string { return "unregistered" } +func (UnregisteredProposal) ProposalType() string { return "unregistered" } + +var _ types.PubProposal = UnregisteredProposal{} + +func (suite *KeeperTestSuite) TestValidatePubProposal() { + + testcases := []struct { + name string + pubProposal types.PubProposal + expectPass bool + }{ + { + name: "valid", + pubProposal: gov.NewTextProposal("A Title", "A description of this proposal."), + expectPass: true, + }, + { + name: "invalid (missing title)", + pubProposal: gov.TextProposal{Description: "A description of this proposal."}, + expectPass: false, + }, + { + name: "invalid (unregistered)", + pubProposal: UnregisteredProposal{gov.TextProposal{Title: "A Title", Description: "A description of this proposal."}}, + expectPass: false, + }, + { + name: "invalid (nil)", + pubProposal: nil, + expectPass: false, + }, + // TODO test case when the handler fails + } + + for _, tc := range testcases { + suite.Run(tc.name, func() { + err := suite.keeper.ValidatePubProposal(suite.ctx, tc.pubProposal) + if tc.expectPass { + suite.NoError(err) + } else { + suite.NotNil(err) + } + }) + } +} diff --git a/x/committee/types/keys.go b/x/committee/types/keys.go index 7ad1731d..6b0a8c4d 100644 --- a/x/committee/types/keys.go +++ b/x/committee/types/keys.go @@ -30,7 +30,6 @@ var ( CommitteeKeyPrefix = []byte{0x00} // prefix for keys that store committees ProposalKeyPrefix = []byte{0x01} // prefix for keys that store proposals VoteKeyPrefix = []byte{0x02} // prefix for keys that store votes - //AuctionByTimeKeyPrefix = []byte{0x01} // prefix for keys that are part of the auctionsByTime index NextProposalIDKey = []byte{0x03} // key for the next proposal id ) @@ -44,11 +43,6 @@ func GetVoteKey(proposalID uint64, voter sdk.AccAddress) []byte { return append(GetKeyFromID(proposalID), voter.Bytes()...) } -// // GetAuctionByTimeKey returns the key for iterating auctions by time -// func GetAuctionByTimeKey(endTime time.Time, auctionID uint64) []byte { -// return append(sdk.FormatTimeBytes(endTime), Uint64ToBytes(auctionID)...) -// } - // Uint64ToBytes converts a uint64 into fixed length bytes for use in store keys. func uint64ToBytes(id uint64) []byte { bz := make([]byte, 8) diff --git a/x/committee/types/types.go b/x/committee/types/types.go index 08ff1db3..2930bd37 100644 --- a/x/committee/types/types.go +++ b/x/committee/types/types.go @@ -5,6 +5,8 @@ import ( "github.com/cosmos/cosmos-sdk/x/gov" ) +// -------- Committees -------- + var VoteThreshold sdk.Dec = sdk.MustNewDecFromStr("0.75") // A Committee is a collection of addresses that are allowed to vote and enact any governance proposal that passes their permissions. @@ -23,8 +25,9 @@ func (c Committee) HasMember(addr sdk.AccAddress) bool { return false } -// As long as one permission allows the proposal then it goes through. Its the OR of all permissions. -func (c Committee) HasPermissionsFor(proposal gov.Content) bool { +// HasPermissionsFor returns whether the committee is authorized to enact a proposal. +// As long as one permission allows the proposal then it goes through. Its the OR of all permissions. +func (c Committee) HasPermissionsFor(proposal PubProposal) bool { for _, p := range c.Permissions { if p.Allows(proposal) { return true @@ -35,22 +38,18 @@ func (c Committee) HasPermissionsFor(proposal gov.Content) bool { // Permission is anything with a method that validates whether a proposal is allowed by it or not. type Permission interface { - Allows(gov.Content) bool + Allows(PubProposal) bool } -// GOV STUFF -------------------------- -// Should be much the same as in gov module, except Proposals are linked to a committee ID. +// -------- Proposals -------- -// TODO not needed? var _ gov.Content = Proposal{} - -type PubProposal = gov.Content // TODO better name +// PubProposal is an interface that all gov proposals defined in other modules must satisfy. +type PubProposal = gov.Content // TODO find a better name type Proposal struct { PubProposal ID uint64 CommitteeID uint64 - // TODO - // could store votes on the proposal object } type Vote struct { @@ -58,7 +57,3 @@ type Vote struct { Voter sdk.AccAddress // Option byte // TODO for now don't need more than just a yes as options } - -// Genesis ------------------- -// Ok just to dump everything to json and reload - if time involved then begin blocker will take care of closing expired proposals. And it won't enact proposals because they would've been immediately enacted before the halt if they passed. -// committee, proposals, votes