-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
👋 Thanks for creating a PR! Before we can merge this PR, please make sure that all the following items have been For contributor use:
For admin use:
Thank you for your contribution to the Cosmos-SDK! 🚀 |
Codecov Report
@@ 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 |
…s/cosmos-sdk into jonathan/6109-pagination-validator
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
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 |
BTW the |
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.
Yes, the check should be |
I prefer using another PR for the changes documented in #6191 |
That's fine by me |
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 |
@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. |
whats the status with this pr? good to merge or close? |
My thought is that it's fine to merge this as is and we'll do better pagination in the future (#6191) |
@jgimeno can you resolve the conflicts please |
Ready @marbar3778 !! |
@Mergifyio refresh |
Command |
Description
closes: #6109