Paginate supply queries #8798

sahith-narahari · 2021-03-05T14:27:38Z

Description

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

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
Review Codecov Report in the comment section below once CI passes

…inate-supply

lgtm-com · 2021-03-05T14:52:24Z

This pull request introduces 1 alert when merging baef976 into 6520997 - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

amaury1093 · 2021-03-17T16:21:30Z

Hey @sahith-narahari, any progress on this PR? Is this r4r (even in draft)?

…inate-supply

orijbot · 2021-03-19T08:57:13Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=7c5e50db17a14f90ad450c503db5a33b

orijbot · 2021-03-19T08:57:23Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=97c9c4403580fd2777b8d65ba76b3258cc76ed18

orijbot · 2021-03-19T12:16:28Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=e352f58ac45ac3b7707e7c74261532da7d6972ac

orijbot · 2021-03-19T12:24:49Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=5efecf3e96c13733884ee7f79c24cbf20b3fd985

orijbot · 2021-03-19T12:43:23Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=5340037a9938f06249e74ce861ecff504321290f

sahith-narahari · 2021-03-19T13:58:24Z

x/bank/keeper/invariants.go

@@ -50,7 +51,11 @@ func NonnegativeBalanceInvariant(k ViewKeeper) sdk.Invariant {
 func TotalSupply(k Keeper) sdk.Invariant {
 	return func(ctx sdk.Context) (string, bool) {
 		expectedTotal := sdk.Coins{}
-		supply := k.GetTotalSupply(ctx)
+		supply, _, err := k.GetPaginatedTotalSupply(ctx, &query.PageRequest{})


@AmauryM do you think we should have empty page request or use alternative defaults

i think we can keep calling GetTotalSupply, no?

I feel it's better not to expose the method, not a strong opinion though. wdyt?

I feel it's better not to expose the method

Not sure I understand. GetTotalSupply still exists in the keeper. Should we actually remove it?

yes, I'm in favour of removing it.

amaury1093

lgtm overall!

amaury1093 · 2021-03-22T16:48:03Z

x/bank/keeper/invariants.go

@@ -50,7 +51,11 @@ func NonnegativeBalanceInvariant(k ViewKeeper) sdk.Invariant {
 func TotalSupply(k Keeper) sdk.Invariant {
 	return func(ctx sdk.Context) (string, bool) {
 		expectedTotal := sdk.Coins{}
-		supply := k.GetTotalSupply(ctx)
+		supply, _, err := k.GetPaginatedTotalSupply(ctx, &query.PageRequest{})


i think we can keep calling GetTotalSupply, no?

fdymylja

lgtm, changelog is missing

sahith-narahari · 2021-03-29T18:13:46Z

@aaronc if we use paginations for internal queries, I feel we're increasing complexity as the number of iterations for the pagination increase. Whereas using GetTotalSupply fetches values in one interation over the store. Is there any particular case you see where pagination helps even for internal functions?

cc @jgimeno @AmauryM

orijbot · 2021-03-29T23:11:37Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=be7b307d49a84526b4a17e6165e7a6a9

amaury1093

if we use paginations for internal queries, I feel we're increasing complexity as the number of iterations for the pagination increase. Whereas using GetTotalSupply fetches values in one interation over the store.

From my understanding, GetTotalSupply == GetPaginatedTotalSupply(PageRequest{Limit: max_uint64}). If that's correct, then I think we can keep GetTotalSupply deleted.

Blocking because @jgimeno's comment needs to be fixed #8798 (comment)

… into sahith/paginate-supply

…inate-supply

fedekunze · 2021-04-03T15:40:27Z

x/bank/keeper/keeper.go

+	supply := sdk.NewCoins()
+
+	pageRes, err := query.Paginate(supplyStore, pagination, func(key, value []byte) error {
+		amount, ok := sdk.NewIntFromString(string(value))


?? why the amount is stored as a string??

fwiu it's because sdk.Int doesn't implement proto message. @jgimeno should know better

but it has Bytes() tho

true, opened a pr for this #9051

types/query/pagination.go

amaury1093

LGTM, except that we should probably wait for #9051, right? (ref: #8798 (comment))

…inate-supply

amaury1093

nice 🎉

sahith-narahari added 2 commits March 5, 2021 19:56

paginate grpc query

653ce30

Merge branch 'master' of github.com:cosmos/cosmos-sdk into sahith/pag…

baef976

…inate-supply

sahith-narahari added 3 commits March 19, 2021 14:19

Merge branch 'master' of github.com:cosmos/cosmos-sdk into sahith/pag…

38aae39

…inate-supply

fix lint

9232a85

update tests

97c9c44

remove GetTotalSupply

e352f58

Merge branch 'master' into sahith/paginate-supply

5efecf3

fix test

5340037

sahith-narahari marked this pull request as ready for review March 19, 2021 13:56

sahith-narahari requested review from aaronc, alessio and alexanderbez as code owners March 19, 2021 13:56

sahith-narahari requested review from robert-zaremba, amaury1093, fdymylja, jgimeno and gsora March 19, 2021 13:57

sahith-narahari commented Mar 19, 2021

View reviewed changes

clevinson assigned amaury1093 Mar 19, 2021

amaury1093 reviewed Mar 22, 2021

View reviewed changes

amaury1093 approved these changes Mar 22, 2021

View reviewed changes

fdymylja approved these changes Mar 22, 2021

View reviewed changes

clevinson linked an issue Mar 22, 2021 that may be closed by this pull request

Paginate supply and balances methods #8761

Closed

6 tasks

Merge branch 'master' into sahith/paginate-supply

77e4022

cyberbono3 self-requested a review March 31, 2021 00:23

cyberbono3 approved these changes Mar 31, 2021

View reviewed changes

amaury1093 suggested changes Mar 31, 2021

View reviewed changes

clevinson mentioned this pull request Apr 2, 2021

Paginate supply and balances methods #8761

Closed

6 tasks

sahith-narahari added 4 commits April 3, 2021 13:06

Merge branch 'sahith/paginate-supply' of github.com:cosmos/cosmos-sdk…

adf4c4a

… into sahith/paginate-supply

Merge branch 'master' of github.com:cosmos/cosmos-sdk into sahith/pag…

36c9e6f

…inate-supply

update limit

3a0607d

update genesis

ba44b06

sahith-narahari requested review from jgimeno and amaury1093 April 3, 2021 08:43

fedekunze reviewed Apr 3, 2021

View reviewed changes

sahith-narahari added 2 commits April 5, 2021 19:34

add max limit to query

d125062

fix lint

ec14729

sahith-narahari commented Apr 5, 2021

View reviewed changes

types/query/pagination.go Outdated Show resolved Hide resolved

sahith-narahari added 3 commits April 5, 2021 22:21

go imports

ce1e5ef

Update types/query/pagination.go

a228af9

Merge branch 'master' into sahith/paginate-supply

66ca8ab

amaury1093 mentioned this pull request Apr 6, 2021

Store supply as Int bytes rather than string bytes #9051

Merged

9 tasks

amaury1093 reviewed Apr 6, 2021

View reviewed changes

sahith-narahari added 3 commits April 6, 2021 19:10

Merge branch 'master' into sahith/paginate-supply

8c9f665

Merge branch 'master' of github.com:cosmos/cosmos-sdk into sahith/pag…

d89cfc0

…inate-supply

update supply

b64c77b

sahith-narahari requested a review from amaury1093 April 6, 2021 14:08

amaury1093 approved these changes Apr 6, 2021

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 6, 2021

mergify bot merged commit 0effd3a into master Apr 6, 2021

mergify bot deleted the sahith/paginate-supply branch April 6, 2021 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paginate supply queries #8798

Paginate supply queries #8798

sahith-narahari commented Mar 5, 2021 •

edited by amaury1093

Loading

lgtm-com bot commented Mar 5, 2021

amaury1093 commented Mar 17, 2021 •

edited

Loading

orijbot commented Mar 19, 2021

orijbot commented Mar 19, 2021

orijbot commented Mar 19, 2021

orijbot commented Mar 19, 2021

orijbot commented Mar 19, 2021

sahith-narahari Mar 19, 2021

amaury1093 Mar 22, 2021

sahith-narahari Mar 23, 2021

amaury1093 Mar 23, 2021

sahith-narahari Mar 23, 2021

amaury1093 left a comment •

edited

Loading

amaury1093 Mar 22, 2021

fdymylja left a comment

sahith-narahari commented Mar 29, 2021

orijbot commented Mar 29, 2021

amaury1093 left a comment

fedekunze Apr 3, 2021

sahith-narahari Apr 5, 2021

fedekunze Apr 5, 2021

sahith-narahari Apr 6, 2021

amaury1093 left a comment •

edited

Loading

amaury1093 left a comment

Paginate supply queries #8798

Paginate supply queries #8798

Conversation

sahith-narahari commented Mar 5, 2021 • edited by amaury1093 Loading

Description

lgtm-com bot commented Mar 5, 2021

amaury1093 commented Mar 17, 2021 • edited Loading

orijbot commented Mar 19, 2021

orijbot commented Mar 19, 2021

orijbot commented Mar 19, 2021

orijbot commented Mar 19, 2021

orijbot commented Mar 19, 2021

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

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdymylja left a comment

Choose a reason for hiding this comment

sahith-narahari commented Mar 29, 2021

orijbot commented Mar 29, 2021

amaury1093 left a 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

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

sahith-narahari commented Mar 5, 2021 •

edited by amaury1093

Loading

amaury1093 commented Mar 17, 2021 •

edited

Loading

amaury1093 left a comment •

edited

Loading

amaury1093 left a comment •

edited

Loading