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 x/staking queries #6163

Merged
merged 24 commits into from
Jun 6, 2020
Merged

Conversation

jgimeno
Copy link
Contributor

@jgimeno jgimeno commented May 7, 2020

Description

closes: #6109

@auto-comment
Copy link

auto-comment bot commented May 7, 2020

👋 Thanks for creating a PR!

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.


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)

Thank you for your contribution to the Cosmos-SDK! 🚀

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #6163 into master will decrease coverage by 0.00%.
The diff coverage is 43.47%.

@@            Coverage Diff             @@
##           master    #6163      +/-   ##
==========================================
- Coverage   55.75%   55.75%   -0.01%     
==========================================
  Files         450      450              
  Lines       27055    27073      +18     
==========================================
+ Hits        15084    15094      +10     
- Misses      10888    10896       +8     
  Partials     1083     1083              

@aaronc
Copy link
Member

aaronc commented May 11, 2020

@aaronc that is something that called my attention too!

This is something that I can add here for this specific case, or create another issue for all paginations, because all paginated results as far as I know use this approach which is not optimal.

I mean if it's not hard to do my preference would be to not put more code into production using an approach we know we're going to change...

Either way I'm opening an issue now to document the proposed approach.

@aaronc
Copy link
Member

aaronc commented May 11, 2020

@jgimeno see #6191

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

utACK

@aaronc
Copy link
Member

aaronc commented May 12, 2020

So is the decision not to address #6191 in this PR? And instead wait for when we get to gRPC? That's fine by me, just checking

@alessio
Copy link
Contributor

alessio commented May 12, 2020

BTW the client.Paginate function smells a bit bad, i.e. try pass a negative page value. We may want to address that in a separate PR though (cc'ing @jgimeno)

@alexanderbez
Copy link
Contributor

So is the decision not to address #6191 in this PR? And instead wait for when we get to gRPC? That's fine by me, just checking

It's not being done in this PR but not necessarily blocked by gRPC either. We can do it before gRPC and in one fell swoop.

BTW the client.Paginate function smells a bit bad, i.e. try pass a negative page value. We may want to address that in a separate PR though (cc'ing @jgimeno)

Yes, the check should be page <= 0.

@jgimeno
Copy link
Contributor Author

jgimeno commented May 12, 2020

I prefer using another PR for the changes documented in #6191

@aaronc
Copy link
Member

aaronc commented May 12, 2020

I prefer using another PR for the changes documented in #6191

That's fine by me

@aaronc
Copy link
Member

aaronc commented May 12, 2020

So is the decision not to address #6191 in this PR? And instead wait for when we get to gRPC? That's fine by me, just checking

It's not being done in this PR but not necessarily blocked by gRPC either. We can do it before gRPC and in one fell swoop.

I actually think it'd be better to only do it for gRPC so that the existing REST endpoints don't change... I think that will allow clients a more graceful transition period

@alexanderbez
Copy link
Contributor

@aaronc I'd advise that existing REST endpoints do change. We never stated REST endpoints are frozen or not subject to change. We should have the same correct pagination logic for gRPC as well as the existing REST handlers. However, since we'll eventually sunset the existing REST handlers, it might not even be worth it.

@tac0turtle
Copy link
Member

whats the status with this pr? good to merge or close?

@aaronc
Copy link
Member

aaronc commented Jun 5, 2020

My thought is that it's fine to merge this as is and we'll do better pagination in the future (#6191)

@tac0turtle
Copy link
Member

@jgimeno can you resolve the conflicts please

@tac0turtle tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 6, 2020
@jgimeno
Copy link
Contributor Author

jgimeno commented Jun 6, 2020

Ready @marbar3778 !!

@tac0turtle
Copy link
Member

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2020

Command refresh: success

@mergify mergify bot merged commit 626f9b6 into master Jun 6, 2020
@mergify mergify bot deleted the jonathan/6109-pagination-validator branch June 6, 2020 17:32
@alessio alessio changed the title add pagination add pagination to x/staking queries Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:x/staking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REST client breaks on high load
5 participants