diff --git a/CHANGELOG.md b/CHANGELOG.md index 4058ea0b62..626e8ec8d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,7 @@ increased significantly due to modular `AnteHandler` support. Increase GasLimit ### Features +* (cli) [\#5212](https://github.com/cosmos/cosmos-sdk/issues/5212) The `q gov proposals` command now supports pagination. * (store) [\#4724](https://github.com/cosmos/cosmos-sdk/issues/4724) Multistore supports substore migrations upon load. New `rootmulti.Store.LoadLatestVersionAndUpgrade` method in `Baseapp` supports `StoreLoader` to enable various upgrade strategies. It no longer panics if the store to load contains substores that we didn't explicitly mount. @@ -180,6 +181,7 @@ to detail this new feature and how state transitions occur. * (keys) Fix ledger custom coin type support bug * (x/gov) [\#5107](https://github.com/cosmos/cosmos-sdk/pull/5107) Sum validator operator's all voting power when tally votes * (baseapp) [\#5200](https://github.com/cosmos/cosmos-sdk/issues/5200) Remove duplicate events from previous messages. +* (rest) [\#5212](https://github.com/cosmos/cosmos-sdk/issues/5212) Fix pagination in the `/gov/proposals` handler. ## [v0.37.2] - 2019-10-10 diff --git a/x/gov/client/cli/query.go b/x/gov/client/cli/query.go index ad87bceebb..47a0c223d0 100644 --- a/x/gov/client/cli/query.go +++ b/x/gov/client/cli/query.go @@ -87,27 +87,29 @@ func GetCmdQueryProposals(queryRoute string, cdc *codec.Codec) *cobra.Command { Use: "proposals", Short: "Query proposals with optional filters", Long: strings.TrimSpace( - fmt.Sprintf(`Query for a all proposals. You can filter the returns with the following flags. + fmt.Sprintf(`Query for a all paginated proposals that match optional filters: Example: $ %s query gov proposals --depositor cosmos1skjwj5whet0lpe65qaq4rpq03hjxlwd9nf39lk $ %s query gov proposals --voter cosmos1skjwj5whet0lpe65qaq4rpq03hjxlwd9nf39lk $ %s query gov proposals --status (DepositPeriod|VotingPeriod|Passed|Rejected) +$ %s query gov proposals --page=2 --limit=100 `, - version.ClientName, version.ClientName, version.ClientName, + version.ClientName, version.ClientName, version.ClientName, version.ClientName, ), ), RunE: func(cmd *cobra.Command, args []string) error { bechDepositorAddr := viper.GetString(flagDepositor) bechVoterAddr := viper.GetString(flagVoter) strProposalStatus := viper.GetString(flagStatus) - numLimit := uint64(viper.GetInt64(flagNumLimit)) + page := viper.GetInt(flagPage) + limit := viper.GetInt(flagNumLimit) var depositorAddr sdk.AccAddress var voterAddr sdk.AccAddress var proposalStatus types.ProposalStatus - params := types.NewQueryProposalsParams(proposalStatus, numLimit, voterAddr, depositorAddr) + params := types.NewQueryProposalsParams(page, limit, proposalStatus, voterAddr, depositorAddr) if len(bechDepositorAddr) != 0 { depositorAddr, err := sdk.AccAddressFromBech32(bechDepositorAddr) @@ -159,7 +161,8 @@ $ %s query gov proposals --status (DepositPeriod|VotingPeriod|Passed|Rejected) }, } - cmd.Flags().String(flagNumLimit, "", "(optional) limit to latest [number] proposals. Defaults to all proposals") + cmd.Flags().Int(flagPage, 1, "pagination page of proposals to to query for") + cmd.Flags().Int(flagNumLimit, 100, "pagination limit of proposals to query for") cmd.Flags().String(flagDepositor, "", "(optional) filter by proposals deposited on by depositor") cmd.Flags().String(flagVoter, "", "(optional) filter by proposals voted on by voted") cmd.Flags().String(flagStatus, "", "(optional) filter proposals by proposal status, status: deposit_period/voting_period/passed/rejected") diff --git a/x/gov/client/cli/tx.go b/x/gov/client/cli/tx.go index 137003c519..8b6deecb8a 100644 --- a/x/gov/client/cli/tx.go +++ b/x/gov/client/cli/tx.go @@ -28,6 +28,7 @@ const ( flagDepositor = "depositor" flagStatus = "status" flagNumLimit = "limit" + flagPage = "page" FlagProposal = "proposal" ) diff --git a/x/gov/client/rest/query.go b/x/gov/client/rest/query.go index b67f4c3590..a3ccb926cf 100644 --- a/x/gov/client/rest/query.go +++ b/x/gov/client/rest/query.go @@ -389,48 +389,13 @@ func queryVotesOnProposalHandlerFn(cliCtx context.CLIContext) http.HandlerFunc { } } -// todo: Split this functionality into helper functions to remove the above +// HTTP request handler to query list of governance proposals func queryProposalsWithParameterFn(cliCtx context.CLIContext) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - bechVoterAddr := r.URL.Query().Get(RestVoter) - bechDepositorAddr := r.URL.Query().Get(RestDepositor) - strProposalStatus := r.URL.Query().Get(RestProposalStatus) - strNumLimit := r.URL.Query().Get(RestNumLimit) - - params := types.QueryProposalsParams{} - - if len(bechVoterAddr) != 0 { - voterAddr, err := sdk.AccAddressFromBech32(bechVoterAddr) - if err != nil { - rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) - return - } - params.Voter = voterAddr - } - - if len(bechDepositorAddr) != 0 { - depositorAddr, err := sdk.AccAddressFromBech32(bechDepositorAddr) - if err != nil { - rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) - return - } - params.Depositor = depositorAddr - } - - if len(strProposalStatus) != 0 { - proposalStatus, err := types.ProposalStatusFromString(gcutils.NormalizeProposalStatus(strProposalStatus)) - if err != nil { - rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) - return - } - params.ProposalStatus = proposalStatus - } - if len(strNumLimit) != 0 { - numLimit, ok := rest.ParseUint64OrReturnBadRequest(w, strNumLimit) - if !ok { - return - } - params.Limit = numLimit + _, page, limit, err := rest.ParseHTTPArgsWithLimit(r, 0) + if err != nil { + rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) + return } cliCtx, ok := rest.ParseQueryHeightOrReturnBadRequest(w, cliCtx, r) @@ -438,13 +403,45 @@ func queryProposalsWithParameterFn(cliCtx context.CLIContext) http.HandlerFunc { return } + var ( + voterAddr sdk.AccAddress + depositorAddr sdk.AccAddress + proposalStatus types.ProposalStatus + ) + + if v := r.URL.Query().Get(RestVoter); len(v) != 0 { + voterAddr, err = sdk.AccAddressFromBech32(v) + if err != nil { + rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) + return + } + } + + if v := r.URL.Query().Get(RestDepositor); len(v) != 0 { + depositorAddr, err = sdk.AccAddressFromBech32(v) + if err != nil { + rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) + return + } + } + + if v := r.URL.Query().Get(RestProposalStatus); len(v) != 0 { + proposalStatus, err = types.ProposalStatusFromString(gcutils.NormalizeProposalStatus(v)) + if err != nil { + rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) + return + } + } + + params := types.NewQueryProposalsParams(page, limit, proposalStatus, voterAddr, depositorAddr) bz, err := cliCtx.Codec.MarshalJSON(params) if err != nil { rest.WriteErrorResponse(w, http.StatusBadRequest, err.Error()) return } - res, height, err := cliCtx.QueryWithData("custom/gov/proposals", bz) + route := fmt.Sprintf("custom/%s/%s", types.QuerierRoute, types.QueryProposals) + res, height, err := cliCtx.QueryWithData(route, bz) if err != nil { rest.WriteErrorResponse(w, http.StatusInternalServerError, err.Error()) return diff --git a/x/gov/genesis.go b/x/gov/genesis.go index 7283e32588..0eb019fc2d 100644 --- a/x/gov/genesis.go +++ b/x/gov/genesis.go @@ -56,8 +56,7 @@ func ExportGenesis(ctx sdk.Context, k Keeper) GenesisState { depositParams := k.GetDepositParams(ctx) votingParams := k.GetVotingParams(ctx) tallyParams := k.GetTallyParams(ctx) - - proposals := k.GetProposalsFiltered(ctx, nil, nil, StatusNil, 0) + proposals := k.GetProposals(ctx) var proposalsDeposits Deposits var proposalsVotes Votes diff --git a/x/gov/keeper/proposal.go b/x/gov/keeper/proposal.go index 4ad9ea8b9b..2580565d38 100644 --- a/x/gov/keeper/proposal.go +++ b/x/gov/keeper/proposal.go @@ -3,6 +3,7 @@ package keeper import ( "fmt" + "github.com/cosmos/cosmos-sdk/client" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/gov/types" ) @@ -101,51 +102,50 @@ func (keeper Keeper) GetProposals(ctx sdk.Context) (proposals types.Proposals) { return } -// GetProposalsFiltered get Proposals from store by ProposalID -// voterAddr will filter proposals by whether or not that address has voted on them -// depositorAddr will filter proposals by whether or not that address has deposited to them -// status will filter proposals by status -// numLatest will fetch a specified number of the most recent proposals, or 0 for all proposals -func (keeper Keeper) GetProposalsFiltered(ctx sdk.Context, voterAddr sdk.AccAddress, depositorAddr sdk.AccAddress, status types.ProposalStatus, numLatest uint64) []types.Proposal { +// GetProposalsFiltered retrieves proposals filtered by a given set of params which +// include pagination parameters along with voter and depositor addresses and a +// proposal status. The voter address will filter proposals by whether or not +// that address has voted on proposals. The depositor address will filter proposals +// by whether or not that address has deposited to them. Finally, status will filter +// proposals by status. +// +// NOTE: If no filters are provided, all proposals will be returned in paginated +// form. +func (keeper Keeper) GetProposalsFiltered(ctx sdk.Context, params types.QueryProposalsParams) []types.Proposal { + proposals := keeper.GetProposals(ctx) + filteredProposals := make([]types.Proposal, 0, len(proposals)) - maxProposalID, err := keeper.GetProposalID(ctx) - if err != nil { - return []types.Proposal{} + for _, p := range proposals { + matchVoter, matchDepositor, matchStatus := true, true, true + + // match status (if supplied/valid) + if types.ValidProposalStatus(params.ProposalStatus) { + matchStatus = p.Status == params.ProposalStatus + } + + // match voter address (if supplied) + if len(params.Voter) > 0 { + _, matchVoter = keeper.GetVote(ctx, p.ProposalID, params.Voter) + } + + // match depositor (if supplied) + if len(params.Depositor) > 0 { + _, matchDepositor = keeper.GetDeposit(ctx, p.ProposalID, params.Depositor) + } + + if matchVoter && matchDepositor && matchStatus { + filteredProposals = append(filteredProposals, p) + } } - matchingProposals := []types.Proposal{} - - if numLatest == 0 { - numLatest = maxProposalID + start, end := client.Paginate(len(filteredProposals), params.Page, params.Limit, 100) + if start < 0 || end < 0 { + filteredProposals = []types.Proposal{} + } else { + filteredProposals = filteredProposals[start:end] } - for proposalID := maxProposalID - numLatest; proposalID < maxProposalID; proposalID++ { - if voterAddr != nil && len(voterAddr) != 0 { - _, found := keeper.GetVote(ctx, proposalID, voterAddr) - if !found { - continue - } - } - - if depositorAddr != nil && len(depositorAddr) != 0 { - _, found := keeper.GetDeposit(ctx, proposalID, depositorAddr) - if !found { - continue - } - } - - proposal, ok := keeper.GetProposal(ctx, proposalID) - if !ok { - continue - } - - if types.ValidProposalStatus(status) && proposal.Status != status { - continue - } - - matchingProposals = append(matchingProposals, proposal) - } - return matchingProposals + return filteredProposals } // GetProposalID gets the highest proposal ID diff --git a/x/gov/keeper/proposal_test.go b/x/gov/keeper/proposal_test.go index f36b9d8ab5..c359f1e5e1 100644 --- a/x/gov/keeper/proposal_test.go +++ b/x/gov/keeper/proposal_test.go @@ -120,3 +120,57 @@ func TestSubmitProposal(t *testing.T) { require.Equal(t, tc.expectedErr, err, "unexpected type of error: %s", err) } } + +func TestGetProposalsFiltered(t *testing.T) { + proposalID := uint64(1) + ctx, _, keeper, _, _ := createTestInput(t, false, 100) + status := []types.ProposalStatus{types.StatusDepositPeriod, types.StatusVotingPeriod} + + addr1 := sdk.AccAddress("foo") + + for _, s := range status { + for i := 0; i < 50; i++ { + p := types.NewProposal(TestProposal, proposalID, time.Now(), time.Now()) + p.Status = s + + if i%2 == 0 { + d := types.NewDeposit(proposalID, addr1, nil) + v := types.NewVote(proposalID, addr1, types.OptionYes) + keeper.SetDeposit(ctx, d) + keeper.SetVote(ctx, v) + } + + keeper.SetProposal(ctx, p) + proposalID++ + } + } + + testCases := []struct { + params types.QueryProposalsParams + expectedNumResults int + }{ + {types.NewQueryProposalsParams(1, 50, types.StatusNil, nil, nil), 50}, + {types.NewQueryProposalsParams(1, 50, types.StatusDepositPeriod, nil, nil), 50}, + {types.NewQueryProposalsParams(1, 50, types.StatusVotingPeriod, nil, nil), 50}, + {types.NewQueryProposalsParams(1, 25, types.StatusNil, nil, nil), 25}, + {types.NewQueryProposalsParams(2, 25, types.StatusNil, nil, nil), 25}, + {types.NewQueryProposalsParams(1, 50, types.StatusRejected, nil, nil), 0}, + {types.NewQueryProposalsParams(1, 50, types.StatusNil, addr1, nil), 50}, + {types.NewQueryProposalsParams(1, 50, types.StatusNil, nil, addr1), 50}, + {types.NewQueryProposalsParams(1, 50, types.StatusNil, addr1, addr1), 50}, + {types.NewQueryProposalsParams(1, 50, types.StatusDepositPeriod, addr1, addr1), 25}, + {types.NewQueryProposalsParams(1, 50, types.StatusDepositPeriod, nil, nil), 50}, + {types.NewQueryProposalsParams(1, 50, types.StatusVotingPeriod, nil, nil), 50}, + } + + for _, tc := range testCases { + proposals := keeper.GetProposalsFiltered(ctx, tc.params) + require.Len(t, proposals, tc.expectedNumResults) + + for _, p := range proposals { + if len(tc.params.ProposalStatus.String()) != 0 { + require.Equal(t, tc.params.ProposalStatus, p.Status) + } + } + } +} diff --git a/x/gov/keeper/querier.go b/x/gov/keeper/querier.go index bb6a63256a..4ff43af6b9 100644 --- a/x/gov/keeper/querier.go +++ b/x/gov/keeper/querier.go @@ -16,20 +16,28 @@ func NewQuerier(keeper Keeper) sdk.Querier { switch path[0] { case types.QueryParams: return queryParams(ctx, path[1:], req, keeper) + case types.QueryProposals: return queryProposals(ctx, path[1:], req, keeper) + case types.QueryProposal: return queryProposal(ctx, path[1:], req, keeper) + case types.QueryDeposits: return queryDeposits(ctx, path[1:], req, keeper) + case types.QueryDeposit: return queryDeposit(ctx, path[1:], req, keeper) + case types.QueryVotes: return queryVotes(ctx, path[1:], req, keeper) + case types.QueryVote: return queryVote(ctx, path[1:], req, keeper) + case types.QueryTally: return queryTally(ctx, path[1:], req, keeper) + default: return nil, sdk.ErrUnknownRequest("unknown gov query endpoint") } @@ -182,19 +190,18 @@ func queryVotes(ctx sdk.Context, path []string, req abci.RequestQuery, keeper Ke return bz, nil } -// nolint: unparam -func queryProposals(ctx sdk.Context, path []string, req abci.RequestQuery, keeper Keeper) ([]byte, sdk.Error) { +func queryProposals(ctx sdk.Context, _ []string, req abci.RequestQuery, keeper Keeper) ([]byte, sdk.Error) { var params types.QueryProposalsParams + err := keeper.cdc.UnmarshalJSON(req.Data, ¶ms) if err != nil { - return nil, sdk.ErrUnknownRequest(sdk.AppendMsgToErr("incorrectly formatted request data", err.Error())) + return nil, sdk.ErrUnknownRequest(sdk.AppendMsgToErr("failed to parse params", err.Error())) } - proposals := keeper.GetProposalsFiltered(ctx, params.Voter, params.Depositor, params.ProposalStatus, params.Limit) - - bz, err := codec.MarshalJSONIndent(keeper.cdc, proposals) + bz, err := codec.MarshalJSONIndent(keeper.cdc, keeper.GetProposalsFiltered(ctx, params)) if err != nil { - return nil, sdk.ErrInternal(sdk.AppendMsgToErr("could not marshal result to JSON", err.Error())) + return nil, sdk.ErrInternal(sdk.AppendMsgToErr("failed to JSON marshal result: %s", err.Error())) } + return bz, nil } diff --git a/x/gov/keeper/querier_test.go b/x/gov/keeper/querier_test.go index 18fd794902..827741bea3 100644 --- a/x/gov/keeper/querier_test.go +++ b/x/gov/keeper/querier_test.go @@ -54,10 +54,14 @@ func getQueriedParams(t *testing.T, ctx sdk.Context, cdc *codec.Codec, querier s return depositParams, votingParams, tallyParams } -func getQueriedProposals(t *testing.T, ctx sdk.Context, cdc *codec.Codec, querier sdk.Querier, depositor, voter sdk.AccAddress, status types.ProposalStatus, limit uint64) []types.Proposal { +func getQueriedProposals( + t *testing.T, ctx sdk.Context, cdc *codec.Codec, querier sdk.Querier, + depositor, voter sdk.AccAddress, status types.ProposalStatus, page, limit int, +) []types.Proposal { + query := abci.RequestQuery{ Path: strings.Join([]string{custom, types.QuerierRoute, types.QueryProposals}, "/"), - Data: cdc.MustMarshalJSON(types.NewQueryProposalsParams(status, limit, voter, depositor)), + Data: cdc.MustMarshalJSON(types.NewQueryProposalsParams(page, limit, status, voter, depositor)), } bz, err := querier(ctx, []string{types.QueryProposals}, query) @@ -214,12 +218,12 @@ func TestQueries(t *testing.T) { require.Equal(t, deposit5, deposit) // Only proposal #1 should be in types.Deposit Period - proposals := getQueriedProposals(t, ctx, keeper.cdc, querier, nil, nil, types.StatusDepositPeriod, 0) + proposals := getQueriedProposals(t, ctx, keeper.cdc, querier, nil, nil, types.StatusDepositPeriod, 1, 0) require.Len(t, proposals, 1) require.Equal(t, proposal1, proposals[0]) // Only proposals #2 and #3 should be in Voting Period - proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, nil, nil, types.StatusVotingPeriod, 0) + proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, nil, nil, types.StatusVotingPeriod, 1, 0) require.Len(t, proposals, 2) require.Equal(t, proposal2, proposals[0]) require.Equal(t, proposal3, proposals[1]) @@ -235,7 +239,7 @@ func TestQueries(t *testing.T) { keeper.SetVote(ctx, vote3) // Test query voted by TestAddrs[0] - proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, nil, TestAddrs[0], types.StatusNil, 0) + proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, nil, TestAddrs[0], types.StatusNil, 1, 0) require.Equal(t, proposal2, proposals[0]) require.Equal(t, proposal3, proposals[1]) @@ -254,25 +258,25 @@ func TestQueries(t *testing.T) { require.Equal(t, vote3, votes[1]) // Test query all proposals - proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, nil, nil, types.StatusNil, 0) + proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, nil, nil, types.StatusNil, 1, 0) require.Equal(t, proposal1, proposals[0]) require.Equal(t, proposal2, proposals[1]) require.Equal(t, proposal3, proposals[2]) // Test query voted by TestAddrs[1] - proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, nil, TestAddrs[1], types.StatusNil, 0) + proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, nil, TestAddrs[1], types.StatusNil, 1, 0) require.Equal(t, proposal3.ProposalID, proposals[0].ProposalID) // Test query deposited by TestAddrs[0] - proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, TestAddrs[0], nil, types.StatusNil, 0) + proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, TestAddrs[0], nil, types.StatusNil, 1, 0) require.Equal(t, proposal1.ProposalID, proposals[0].ProposalID) // Test query deposited by addr2 - proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, TestAddrs[1], nil, types.StatusNil, 0) + proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, TestAddrs[1], nil, types.StatusNil, 1, 0) require.Equal(t, proposal2.ProposalID, proposals[0].ProposalID) require.Equal(t, proposal3.ProposalID, proposals[1].ProposalID) // Test query voted AND deposited by addr1 - proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, TestAddrs[0], TestAddrs[0], types.StatusNil, 0) + proposals = getQueriedProposals(t, ctx, keeper.cdc, querier, TestAddrs[0], TestAddrs[0], types.StatusNil, 1, 0) require.Equal(t, proposal2.ProposalID, proposals[0].ProposalID) } diff --git a/x/gov/types/querier.go b/x/gov/types/querier.go index 2b14592eeb..fe97356b5c 100644 --- a/x/gov/types/querier.go +++ b/x/gov/types/querier.go @@ -68,18 +68,20 @@ func NewQueryVoteParams(proposalID uint64, voter sdk.AccAddress) QueryVoteParams // QueryProposalsParams Params for query 'custom/gov/proposals' type QueryProposalsParams struct { + Page int + Limit int Voter sdk.AccAddress Depositor sdk.AccAddress ProposalStatus ProposalStatus - Limit uint64 } // NewQueryProposalsParams creates a new instance of QueryProposalsParams -func NewQueryProposalsParams(status ProposalStatus, limit uint64, voter, depositor sdk.AccAddress) QueryProposalsParams { +func NewQueryProposalsParams(page, limit int, status ProposalStatus, voter, depositor sdk.AccAddress) QueryProposalsParams { return QueryProposalsParams{ + Page: page, + Limit: limit, Voter: voter, Depositor: depositor, ProposalStatus: status, - Limit: limit, } }