From 4298564096855d06bd93856ed98c86bc7c506636 Mon Sep 17 00:00:00 2001 From: rhuairahrighairigh Date: Mon, 30 Mar 2020 14:06:31 +0100 Subject: [PATCH] address minor TODOs --- x/committee/client/cli/tx.go | 2 -- x/committee/client/rest/query.go | 33 ++++++++--------------------- x/committee/client/rest/tx.go | 5 ++--- x/committee/keeper/keeper.go | 12 ++++++++--- x/committee/keeper/proposal_test.go | 24 ++++++++++++++++----- x/committee/keeper/querier.go | 16 ++++++++++---- x/committee/types/msg.go | 8 ++----- x/committee/types/permissions.go | 2 +- x/committee/types/types.go | 14 +++++++++--- 9 files changed, 65 insertions(+), 51 deletions(-) diff --git a/x/committee/client/cli/tx.go b/x/committee/client/cli/tx.go index 2261a9c1..2e142e8d 100644 --- a/x/committee/client/cli/tx.go +++ b/x/committee/client/cli/tx.go @@ -126,8 +126,6 @@ func GetCmdVote(cdc *codec.Codec) *cobra.Command { } } -// TODO This could replace the whole gov submit-proposal cmd. It would align how it works with how submiting proposal to committees works. -// Requires removing and replacing the gov cmd in kvcli main.go // GetGovCmdSubmitProposal returns a command to submit a proposal to the gov module. It is passed to the gov module for use on its command subtree. func GetGovCmdSubmitProposal(cdc *codec.Codec) *cobra.Command { cmd := &cobra.Command{ diff --git a/x/committee/client/rest/query.go b/x/committee/client/rest/query.go index 75034884..da80ec5c 100644 --- a/x/committee/client/rest/query.go +++ b/x/committee/client/rest/query.go @@ -24,7 +24,9 @@ func registerQueryRoutes(cliCtx context.CLIContext, r *mux.Router) { r.HandleFunc(fmt.Sprintf("/%s/proposals/{%s}/votes", types.ModuleName, RestProposalID), queryVotesOnProposalHandlerFn(cliCtx)).Methods("GET") } -// ---------- Committees ---------- +// ------------------------------------------ +// Committees +// ------------------------------------------ func queryCommitteesHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -85,7 +87,9 @@ func queryCommitteeHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { } } -// ---------- Proposals ---------- +// ------------------------------------------ +// Proposals +// ------------------------------------------ func queryProposalsHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -190,7 +194,9 @@ func queryProposerHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { } } -// ---------- Votes ---------- +// ------------------------------------------ +// Votes +// ------------------------------------------ func queryVotesOnProposalHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { @@ -224,27 +230,6 @@ func queryVotesOnProposalHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { return } - // TODO should add this feature back - // var proposal types.Proposal - // if err := cliCtx.Codec.UnmarshalJSON(res, &proposal); err != nil { - // rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error()) - // return - // } - - // // For inactive proposals we must query the txs directly to get the votes - // // as they're no longer in state. - // propStatus := proposal.Status - // if !(propStatus == types.StatusVotingPeriod || propStatus == types.StatusDepositPeriod) { - // res, err = gcutils.QueryVotesByTxQuery(cliCtx, params) - // } else { - // res, _, err = cliCtx.QueryWithData("custom/gov/votes", bz) - // } - - // if err != nil { - // rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error()) - // return - // } - // Write response cliCtx = cliCtx.WithHeight(height) rest.PostProcessResponse(w, cliCtx, res) diff --git a/x/committee/client/rest/tx.go b/x/committee/client/rest/tx.go index 01e1b7c4..46447123 100644 --- a/x/committee/client/rest/tx.go +++ b/x/committee/client/rest/tx.go @@ -106,11 +106,10 @@ func postVoteHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { } } -// -------- -------- -// TODO this could replace the POST gov/proposals endpoint, would need to overwrite routes in kvcli main, hacky +// This is a rest handler for for the gov module, that handles committee change/delete proposals. type PostGovProposalReq struct { BaseReq rest.BaseReq `json:"base_req" yaml:"base_req"` - Content govtypes.Content `json:"content" yaml:"content"` //TODO use same PubProposal name? + Content govtypes.Content `json:"content" yaml:"content"` Proposer sdk.AccAddress `json:"proposer" yaml:"proposer"` Deposit sdk.Coins `json:"deposit" yaml:"deposit"` } diff --git a/x/committee/keeper/keeper.go b/x/committee/keeper/keeper.go index 327b653a..6b2f9f7c 100644 --- a/x/committee/keeper/keeper.go +++ b/x/committee/keeper/keeper.go @@ -33,7 +33,9 @@ func NewKeeper(cdc *codec.Codec, storeKey sdk.StoreKey, router govtypes.Router, } } -// ---------- Committees ---------- +// ------------------------------------------ +// Committees +// ------------------------------------------ // GetCommittee gets a committee from the store. func (k Keeper) GetCommittee(ctx sdk.Context, committeeID uint64) (types.Committee, bool) { @@ -76,7 +78,9 @@ func (k Keeper) IterateCommittees(ctx sdk.Context, cb func(committee types.Commi } } -// ---------- Proposals ---------- +// ------------------------------------------ +// Proposals +// ------------------------------------------ // SetNextProposalID stores an ID to be used for the next created proposal func (k Keeper) SetNextProposalID(ctx sdk.Context, id uint64) { @@ -167,7 +171,9 @@ func (k Keeper) IterateProposals(ctx sdk.Context, cb func(proposal types.Proposa } } -// ---------- Votes ---------- +// ------------------------------------------ +// Votes +// ------------------------------------------ // GetVote gets a vote from the store. func (k Keeper) GetVote(ctx sdk.Context, proposalID uint64, voter sdk.AccAddress) (types.Vote, bool) { diff --git a/x/committee/keeper/proposal_test.go b/x/committee/keeper/proposal_test.go index 6bf59141..2ff91cdc 100644 --- a/x/committee/keeper/proposal_test.go +++ b/x/committee/keeper/proposal_test.go @@ -7,6 +7,7 @@ import ( "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/gov" + "github.com/cosmos/cosmos-sdk/x/params" abci "github.com/tendermint/tendermint/abci/types" "github.com/kava-labs/kava/app" @@ -78,8 +79,7 @@ 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 tApp := app.NewTestApp() keeper := tApp.GetCommitteeKeeper() ctx := tApp.NewContext(true, abci.Header{}) @@ -148,7 +148,7 @@ func (suite *KeeperTestSuite) TestAddVote() { 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 tApp := app.NewTestApp() keeper := tApp.GetCommitteeKeeper() ctx := tApp.NewContext(true, abci.Header{Height: 1, Time: firstBlockTime}) @@ -217,7 +217,7 @@ func (suite *KeeperTestSuite) TestGetProposalResult() { 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 tApp := app.NewTestApp() keeper := tApp.GetCommitteeKeeper() ctx := tApp.NewContext(true, abci.Header{Height: 1, Time: firstBlockTime}) @@ -294,7 +294,21 @@ func (suite *KeeperTestSuite) TestValidatePubProposal() { pubProposal: nil, expectPass: false, }, - // TODO test case when the handler fails + { + name: "invalid (proposal handler fails)", + pubProposal: params.NewParameterChangeProposal( + "A Title", + "A description of this proposal.", + []params.ParamChange{{ + Subspace: "non existant", + Key: "non existant", + Value: "nonsense", + }}, + ), + expectPass: false, + }, + // 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 { diff --git a/x/committee/keeper/querier.go b/x/committee/keeper/querier.go index 54ede8b6..5981f945 100644 --- a/x/committee/keeper/querier.go +++ b/x/committee/keeper/querier.go @@ -36,7 +36,9 @@ func NewQuerier(keeper Keeper) sdk.Querier { } } -// ---------- Committees ---------- +// ------------------------------------------ +// Committees +// ------------------------------------------ func queryCommittees(ctx sdk.Context, path []string, _ abci.RequestQuery, keeper Keeper) ([]byte, sdk.Error) { @@ -72,7 +74,9 @@ func queryCommittee(ctx sdk.Context, path []string, req abci.RequestQuery, keepe return bz, nil } -// ---------- Proposals ---------- +// ------------------------------------------ +// Proposals +// ------------------------------------------ func queryProposals(ctx sdk.Context, path []string, req abci.RequestQuery, keeper Keeper) ([]byte, sdk.Error) { var params types.QueryCommitteeParams @@ -115,7 +119,9 @@ func queryProposal(ctx sdk.Context, path []string, req abci.RequestQuery, keeper return bz, nil } -// ---------- Votes ---------- +// ------------------------------------------ +// Votes +// ------------------------------------------ func queryVotes(ctx sdk.Context, path []string, req abci.RequestQuery, keeper Keeper) ([]byte, sdk.Error) { var params types.QueryProposalParams @@ -157,7 +163,9 @@ func queryVote(ctx sdk.Context, path []string, req abci.RequestQuery, keeper Kee return bz, nil } -// ---------- Tally ---------- +// ------------------------------------------ +// Tally +// ------------------------------------------ func queryTally(ctx sdk.Context, path []string, req abci.RequestQuery, keeper Keeper) ([]byte, sdk.Error) { var params types.QueryProposalParams diff --git a/x/committee/types/msg.go b/x/committee/types/msg.go index 173d9564..5e2b138b 100644 --- a/x/committee/types/msg.go +++ b/x/committee/types/msg.go @@ -5,8 +5,8 @@ import ( ) const ( - TypeMsgSubmitProposal = "submit_proposal" // TODO these are the same as the gov module, will there be collisions? - TypeMsgVote = "vote" + TypeMsgSubmitProposal = "commmittee_submit_proposal" // 'committee' prefix appended to avoid potential conflicts with gov msg types + TypeMsgVote = "committee_vote" ) var _, _ sdk.Msg = MsgSubmitProposal{}, MsgVote{} @@ -41,10 +41,6 @@ func (msg MsgSubmitProposal) ValidateBasic() sdk.Error { if msg.Proposer.Empty() { return sdk.ErrInvalidAddress(msg.Proposer.String()) } - // TODO - // if !IsValidProposalType(msg.Content.ProposalType()) { - // return ErrInvalidProposalType(DefaultCodespace, msg.Content.ProposalType()) - // } return msg.PubProposal.ValidateBasic() } diff --git a/x/committee/types/permissions.go b/x/committee/types/permissions.go index 2481587e..28a1d534 100644 --- a/x/committee/types/permissions.go +++ b/x/committee/types/permissions.go @@ -110,6 +110,6 @@ func (perm ShutdownPermission) MarshalYAML() (interface{}, error) { } // TODO add more permissions? -// - limit parameter changes to bew withing small ranges or fixed sets +// - limit parameter changes to be within small ranges // - allow community spend proposals // - allow committee change proposals diff --git a/x/committee/types/types.go b/x/committee/types/types.go index a575848e..b5e46211 100644 --- a/x/committee/types/types.go +++ b/x/committee/types/types.go @@ -11,7 +11,9 @@ import ( const MaxCommitteeDescriptionLength int = 5000 -// -------- Committees -------- +// ------------------------------------------ +// Committees +// ------------------------------------------ // A Committee is a collection of addresses that are allowed to vote and enact any governance proposal that passes their permissions. type Committee struct { @@ -94,10 +96,12 @@ type Permission interface { Allows(PubProposal) bool } -// -------- Proposals -------- +// ------------------------------------------ +// Proposals +// ------------------------------------------ // PubProposal is an interface that all gov proposals defined in other modules must satisfy. -type PubProposal = gov.Content // TODO find a better name +type PubProposal = gov.Content type Proposal struct { PubProposal `json:"pub_proposal" yaml:"pub_proposal"` @@ -118,6 +122,10 @@ func (p Proposal) String() string { return string(bz) } +// ------------------------------------------ +// Votes +// ------------------------------------------ + type Vote struct { ProposalID uint64 `json:"proposal_id" yaml:"proposal_id"` Voter sdk.AccAddress `json:"voter" yaml:"voter"`