x/gov: Queries to /gov/proposals/{proposalID}/votes support pagination #5405

dshulyak · 2019-12-13T10:34:08Z

Changes:

votes for active proposals are paginated by applying page limits to results stored in keeper
for inactive proposals we paginate what we receive from tendermint.TxSearch

tendermint.TxSearch paginates transactions, not messages. I noticed that current gov application allows to submit only one msg per transaction. But transaction structure suggests that there could be many messages in a single transaction. Hence I load transactions in batches (30), unpacking them and once we collected enough votes - query is terminated. Or it will be terminated if we run out off transactions.

This approach gives clients what they want, but also adds unnecessary load. So if relying on application semantics is error-proof, let me know and i will simplify this part.

Other changes:

added separate type for proposal votes queries
extended cli for query gov votes with '--page' and '--limit' flags

Closes: #5344

Description

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

dshulyak · 2019-12-13T11:13:12Z

There was a note that TxSearch doesn't support pagination. I assume it is outdated, right? Based on https://github.com/tendermint/tendermint/blob/master/rpc/core/tx.go#L79-L85

codecov · 2019-12-13T11:42:46Z

Codecov Report

Merging #5405 into master will decrease coverage by 0.22%.
The diff coverage is 91.89%.

@@            Coverage Diff             @@
##           master    #5405      +/-   ##
==========================================
- Coverage   54.59%   54.37%   -0.23%     
==========================================
  Files         315      313       -2     
  Lines       18921    18601     -320     
==========================================
- Hits        10330    10114     -216     
+ Misses       7810     7712      -98     
+ Partials      781      775       -6

Impacted Files	Coverage Δ
x/gov/keeper/querier.go	`45.18% <85.71%> (+1.77%)`	⬆️
x/gov/client/utils/query.go	`24.36% <93.33%> (ø)`
x/params/subspace/subspace.go	`0% <0%> (-77.36%)`	⬇️
x/params/subspace/table.go	`74.35% <0%> (-25.65%)`	⬇️
x/bank/handler.go	`69.04% <0%> (-16.67%)`	⬇️
types/dec_coin.go	`63.21% <0%> (-15.39%)`	⬇️
x/params/proposal_handler.go	`62.96% <0%> (-10.73%)`	⬇️
crypto/keys/keybase_base.go	`64.7% <0%> (-3.72%)`	⬇️
client/keys/utils.go	`45.45% <0%> (-3.16%)`	⬇️
crypto/keys/keyring.go	`43.65% <0%> (-1.9%)`	⬇️
... and 58 more

x/gov/client/utils/query_test.go

golangcibot · 2019-12-13T14:10:38Z

x/gov/client/utils/query_test.go

+	} {
+		t.Run(tc.description, func(t *testing.T) {
+			params := types.NewQueryProposalVotesParams(0, tc.page, tc.limit)
+			votes, err := getPaginatedVotes(context.CLIContext{}, params, makeQuerier(tc.txs))


Using the variable on range scope tc in function literal (from scopelint)

golangcibot · 2019-12-13T14:10:38Z

x/gov/client/utils/query_test.go

+		t.Run(tc.description, func(t *testing.T) {
+			params := types.NewQueryProposalVotesParams(0, tc.page, tc.limit)
+			votes, err := getPaginatedVotes(context.CLIContext{}, params, makeQuerier(tc.txs))
+			if tc.err != nil {


Using the variable on range scope tc in function literal (from scopelint)

dshulyak · 2019-12-16T11:06:34Z

Will be great if someone will be able to review this change. I noticed golangcibot warnings, and will fix them with other suggestions. Also coverage seems to report irrelevant info, reports coverage decrease in not related modules and files.

alexanderbez

Amazing! Thanks for the contribution @dshulyak -- I left some feedback, but overall this is the correct idea!

x/gov/client/cli/query.go

alexanderbez · 2019-12-17T15:44:30Z

x/gov/client/utils/query.go

@@ -72,41 +73,57 @@ func QueryDepositsByTxQuery(cliCtx context.CLIContext, params types.QueryProposa
 	return cliCtx.Codec.MarshalJSON(deposits)
 }

+type txQuerier func(cliCtx context.CLIContext, events []string, page, limit int) (*sdk.SearchTxsResult, error)


Not sure why the function was broken up this way? It's a bit harder to grok for me.

Supporting pagination for this one-off case can be achieved with minimal changes to QueryVotesByTxQuery. Let me know what you think.

func QueryVotesByTxQuery(cliCtx context.CLIContext, params types.QueryProposalParams) ([]byte, error) { events := []string{ fmt.Sprintf("%s.%s='%s'", sdk.EventTypeMessage, sdk.AttributeKeyAction, types.TypeMsgVote), fmt.Sprintf("%s.%s='%s'", types.EventTypeProposalVote, types.AttributeKeyProposalID, []byte(fmt.Sprintf("%d", params.ProposalID))), } var ( votes []types.Vote searchResult *sdk.SearchTxsResult err error ) page := 1 searchResult, err = utils.QueryTxsByEvents(cliCtx, events, page, 100) if err != nil { return nil, err } // Search for votes until the tx results are exhausted or we've reached the // pagination limit. for searchResult.Count != 0 || len(votes) < params.Limit { for _, info := range searchResult.Txs { for _, msg := range info.Tx.GetMsgs() { if msg.Type() == types.TypeMsgVote { voteMsg := msg.(types.MsgVote) votes = append(votes, types.Vote{ Voter: voteMsg.Voter, ProposalID: params.ProposalID, Option: voteMsg.Option, }) } } } page++ searchResult, err = utils.QueryTxsByEvents(cliCtx, events, page, 100) if err != nil { return nil, err } } start, end := client.Paginate(len(votes), params.Page, params.Limit, 100) if start < 0 || end < 0 { votes = []types.Vote{} } else { votes = votes[start:end] } if cliCtx.Indent { return cliCtx.Codec.MarshalJSONIndent(votes, "", " ") } return cliCtx.Codec.MarshalJSON(votes) }

i splitted whole function so that i can actually test pagination logic (see query_test.go).
otherwise, body of the function is similar to yours, with one distinction - if the previous tx query returned less then full chunk we already know that tx results are exhaused and query terminated earlier

Let's keep the logic in QueryVotesByTxQuery because unit testing private functions is not always an option -- the end result is the same, you're still unit testing the same business logic.

You can update the above snippet to handle the case I missed 👍

in order to unit test QueryVotesByTxQuery i need to mock calls to SearchTx, i do it with passing custom querier function. am i missing something that can be used for this purpose?

Yes, you're right! We can add the function as a parameter to QueryVotesByTxQuery?

imo bad api. but not worth arguing, if you feel that this is ok i will pass as param

Thanks @dshulyak!

x/gov/client/utils/query_test.go

alexanderbez · 2019-12-17T15:46:50Z

x/gov/keeper/vote.go

@@ -53,6 +54,15 @@ func (keeper Keeper) GetVotes(ctx sdk.Context, proposalID uint64) (votes types.V
 	return
 }

+func (keeper Keeper) GetVotesPaginated(ctx sdk.Context, proposalID uint64, page, limit int) (votes types.Votes) {


I would remove this method and move the pagination logic within the querier handler like other modules do.

i checked proposals method when i was adding this one - https://github.com/cosmos/cosmos-sdk/blob/master/x/gov/keeper/proposal.go#L114-L148

I'm not sure I follow? This method is not needed. Pagination can simply be done in queryVotes.

i mean that when i was adding this method i intentionally made it consistent with that one (e.g. kept pagination in keeper)

I see. Yeah, but this isn't necessary in this case.

x/gov/keeper/querier.go

dshulyak

@alexanderbez thank you for review. i responded to your comments, please take a look.
in gov module there are also deposits, do you think that it makes sense to add pagination for them as well?

x/gov/client/cli/query.go

x/gov/client/utils/query_test.go

dshulyak · 2019-12-17T18:14:07Z

x/gov/keeper/vote.go

@@ -53,6 +54,15 @@ func (keeper Keeper) GetVotes(ctx sdk.Context, proposalID uint64) (votes types.V
 	return
 }

+func (keeper Keeper) GetVotesPaginated(ctx sdk.Context, proposalID uint64, page, limit int) (votes types.Votes) {


i checked proposals method when i was adding this one - https://github.com/cosmos/cosmos-sdk/blob/master/x/gov/keeper/proposal.go#L114-L148

dshulyak · 2019-12-17T18:18:28Z

x/gov/client/utils/query.go

@@ -72,41 +73,57 @@ func QueryDepositsByTxQuery(cliCtx context.CLIContext, params types.QueryProposa
 	return cliCtx.Codec.MarshalJSON(deposits)
 }

+type txQuerier func(cliCtx context.CLIContext, events []string, page, limit int) (*sdk.SearchTxsResult, error)


i splitted whole function so that i can actually test pagination logic (see query_test.go).
otherwise, body of the function is similar to yours, with one distinction - if the previous tx query returned less then full chunk we already know that tx results are exhaused and query terminated earlier

Changes: - votes for active proposals are paginated by applying page limits to results stored in keeper - for inactive proposals we paginate what we receive from tendermint.TxSearch tendermint.TxSearch paginates transactions, not messages. I noticed that current gov application allows to submit only one msg per tx. But tx structure suggests that there could be many messages in a single transaction. Hence we load transactions in batches (30), unpacking them and once we collected enough votes - query is terminted. Or it will be terminted if we run out off transactions. Other changes: - add separate type for proposal votes queries - extended cli to query gov vote with '--page' and '--limit' flags Signed-off-by: dmitry <yashulyak@gmail.com>

dshulyak · 2019-12-18T05:51:27Z

removed pagination from keeper
and QueryVotesByTxQuery now accepts TxQuerier

@alexanderbez let me know if you have other concerns

dshulyak · 2019-12-18T05:52:18Z

btw GolangCI chocked on downloading tendermint, would be great if someone can restart

Running error: context loading failed: failed to load program with go/packages: -: go: downloading github.com/tendermint/tendermint v0.32.8

alexanderbez

Looks good @dshulyak. Only thing remaining is to not return an error in one case.

alexanderbez · 2019-12-18T13:34:55Z

x/gov/client/utils/query.go

-
+	start, end := client.Paginate(len(votes), params.Page, params.Limit, 100)
+	if start < 0 || end < 0 {
+		return nil, fmt.Errorf("invalid page (%d) or range is out of bounds (limit %d)", params.Page, params.Limit)


Suggested change

return nil, fmt.Errorf("invalid page (%d) or range is out of bounds (limit %d)", params.Page, params.Limit)

votes = []type.Vote{}

x/gov/client/utils/query_test.go

Signed-off-by: dmitry <yashulyak@gmail.com>

alexanderbez

ACK 🎉

dshulyak requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners December 13, 2019 10:34

dshulyak force-pushed the votes-pagination branch from 4d31499 to 1603b60 Compare December 13, 2019 14:07

golangcibot reviewed Dec 13, 2019

View reviewed changes

alexanderbez added R4R C:x/gov labels Dec 16, 2019

alexanderbez suggested changes Dec 17, 2019

View reviewed changes

dshulyak commented Dec 17, 2019

View reviewed changes

dshulyak force-pushed the votes-pagination branch from 1603b60 to aa56aa9 Compare December 17, 2019 18:46

dshulyak added 2 commits December 18, 2019 07:22

x/gov: Pass TxQuerier to QueryVotesByTxQuery directly

0af476a

x/gov: Remove votes pagination from keeper

8a6ded8

dshulyak changed the title ~~x/gov: Queries to /gob/proposals/{proposalID}/votes support pagination~~ x/gov: Queries to /gov/proposals/{proposalID}/votes support pagination Dec 18, 2019

alexanderbez suggested changes Dec 18, 2019

View reviewed changes

x/gov: Dont return an error if pagination parameters are wrong

1caad87

Signed-off-by: dmitry <yashulyak@gmail.com>

dshulyak force-pushed the votes-pagination branch from 1eb3fab to 1caad87 Compare December 18, 2019 14:56

alexanderbez approved these changes Dec 18, 2019

View reviewed changes

Merge branch 'master' into votes-pagination

fd6208b

alexanderbez merged commit cf46452 into cosmos:master Dec 18, 2019

dshulyak mentioned this pull request Dec 24, 2019

Mock rpcclient in tests for votes pagination #5445

Merged

11 tasks

antho1404 mentioned this pull request Feb 26, 2020

Implementation page-like system on rest api mesg-foundation/engine#1679

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/gov: Queries to /gov/proposals/{proposalID}/votes support pagination #5405

x/gov: Queries to /gov/proposals/{proposalID}/votes support pagination #5405

dshulyak commented Dec 13, 2019 •

edited

Loading

dshulyak commented Dec 13, 2019 •

edited

Loading

codecov bot commented Dec 13, 2019 •

edited

Loading

golangcibot Dec 13, 2019

golangcibot Dec 13, 2019

dshulyak commented Dec 16, 2019 •

edited

Loading

alexanderbez left a comment

alexanderbez Dec 17, 2019

dshulyak Dec 17, 2019

alexanderbez Dec 17, 2019 •

edited

Loading

dshulyak Dec 17, 2019

alexanderbez Dec 17, 2019

dshulyak Dec 17, 2019

alexanderbez Dec 17, 2019

alexanderbez Dec 17, 2019

dshulyak Dec 17, 2019

alexanderbez Dec 17, 2019 •

edited

Loading

dshulyak Dec 17, 2019

alexanderbez Dec 17, 2019

dshulyak left a comment

dshulyak Dec 17, 2019

dshulyak Dec 17, 2019

dshulyak commented Dec 18, 2019

dshulyak commented Dec 18, 2019

alexanderbez left a comment

alexanderbez Dec 18, 2019

alexanderbez left a comment

	return nil, fmt.Errorf("invalid page (%d) or range is out of bounds (limit %d)", params.Page, params.Limit)
	votes = []type.Vote{}

x/gov: Queries to /gov/proposals/{proposalID}/votes support pagination #5405

x/gov: Queries to /gov/proposals/{proposalID}/votes support pagination #5405

Conversation

dshulyak commented Dec 13, 2019 • edited Loading

Description

dshulyak commented Dec 13, 2019 • edited Loading

codecov bot commented Dec 13, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshulyak commented Dec 16, 2019 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshulyak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshulyak commented Dec 18, 2019

dshulyak commented Dec 18, 2019

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

dshulyak commented Dec 13, 2019 •

edited

Loading

dshulyak commented Dec 13, 2019 •

edited

Loading

codecov bot commented Dec 13, 2019 •

edited

Loading

dshulyak commented Dec 16, 2019 •

edited

Loading

alexanderbez Dec 17, 2019 •

edited

Loading

alexanderbez Dec 17, 2019 •

edited

Loading