Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Pagination to Pool With Filter Query #3563

Merged
merged 9 commits into from
Nov 30, 2022

Conversation

mattverse
Copy link
Member

Closes: #XXX

What is the purpose of the change

This PR adds pagination to pools with filter query.
As of right now, pagination is not included in the query for pools with filter, whist pagination is necessary as front end query heavily depends on pagination.

Brief Changelog

  • Add pagination to PoolsWithFilter query in gamm

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@mattverse mattverse requested a review from a team November 29, 2022 15:58
@github-actions github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Nov 29, 2022
@@ -184,14 +175,14 @@ func (q Querier) PoolsWithFilter(ctx context.Context, req *types.QueryPoolsWithF
checks_needed++
}

for _, p := range pools {
var response = []*codectypes.Any{}
pageRes, err := query.FilteredPaginate(poolStore, req.Pagination, func(_, value []byte, accumulate bool) (bool, error) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using FilteredPaginate instead of Paginate method allows us to iterate until pagination count has been reached

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cosmos/cosmos-sdk#12242

We can backport the fix for that to clean this up a little if we want.
(Or just copy/paste cosmos/cosmos-sdk#12253 into osmoutils)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also checking generic pagination as well! Sad that we don't have it yet. I say we refactor this to use generic pagination later on when it gets backported and for now leave as is

var pool types.PoolI

err := q.cdc.UnpackAny(p, &pool)
pool, err := q.Keeper.UnmarshalPool(value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does this value as the input to UnmarshalPool come from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It comes from the FilteredPagination method, where it iterates over the given kv store!

@mattverse mattverse added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v13.x backport patches to v13.x branch labels Nov 29, 2022
return false, err
}

if accumulate {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required for offset in pagination

@mattverse mattverse added the A:backport/v12.x backport patches to v12.x branch label Nov 29, 2022
@ValarDragon ValarDragon merged commit 1dedc28 into main Nov 30, 2022
@ValarDragon ValarDragon deleted the mattverse/fix-filtered-pool-query branch November 30, 2022 05:21
mergify bot pushed a commit that referenced this pull request Nov 30, 2022
* Add Pagination to filter pool query

* Use Filtered Pagination

* Add logic for offset in pagination

* Return false on response

* Bug fix

* Refactors for clean code

* Remove snake cases

* Fix typos

* Clean more code

(cherry picked from commit 1dedc28)

# Conflicts:
#	proto/osmosis/gamm/v1beta1/query.proto
#	x/gamm/keeper/grpc_query.go
#	x/gamm/types/query.pb.go
mergify bot pushed a commit that referenced this pull request Nov 30, 2022
* Add Pagination to filter pool query

* Use Filtered Pagination

* Add logic for offset in pagination

* Return false on response

* Bug fix

* Refactors for clean code

* Remove snake cases

* Fix typos

* Clean more code

(cherry picked from commit 1dedc28)
ValarDragon pushed a commit that referenced this pull request Nov 30, 2022
* Add Pagination to filter pool query

* Use Filtered Pagination

* Add logic for offset in pagination

* Return false on response

* Bug fix

* Refactors for clean code

* Remove snake cases

* Fix typos

* Clean more code

(cherry picked from commit 1dedc28)

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v12.x backport patches to v12.x branch A:backport/v13.x backport patches to v13.x branch C:x/gamm Changes, features and bugs related to the gamm module. V:state/compatible/backport State machine compatible PR, should be backported
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants