refactor(pagination): encode keys on refactored queries#2044
Merged
Conversation
WalkthroughImplements key-based pagination for deployment and market query handlers (Deployments, Orders, Bids, Leases): adds pagination-key decoding/encoding, multi-state iteration with resume support, reverse traversal, offset validation, Total counting, and NextKey generation; updates tests for cross-page and state-filtered pagination behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryHandler
participant StateIterator
participant Database
Client->>QueryHandler: Request with PageRequest (key, limit, offset, state filter)
QueryHandler->>QueryHandler: Decode pagination key (state, resume_pk) OR derive states
QueryHandler->>StateIterator: Init iterator for current state slice (handle reverse)
StateIterator->>Database: Fetch items from state (resume_pk/offset applied)
Database-->>StateIterator: Return items
StateIterator->>QueryHandler: Emit items until limit reached
alt limit reached
QueryHandler->>QueryHandler: Encode NextKey (state + pk + direction)
QueryHandler-->>Client: Return PageResponse (items, Total, NextKey)
else exhausted states
QueryHandler-->>Client: Return PageResponse (items, Total, nil)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
x/deployment/keeper/grpc_query_test.go (1)
304-359: Add reverse key-pagination continuation tests.Current additions cover forward traversal only. Please add a
Pagination.Reverse: truemulti-page case (including cross-state continuation) to lock down ordering and resume behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/deployment/keeper/grpc_query_test.go` around lines 304 - 359, Add a new t.Run that exercises reverse (Pagination.Reverse = true) key-based pagination for Deployments using suite.queryClient.Deployments: request a last-item-first page with &sdkquery.PageRequest{Limit:1, Reverse:true}, capture the returned Pagination.NextKey, then fetch the previous page by calling Deployments again with &sdkquery.PageRequest{Key: <prevKey>, Limit:1, Reverse:true} and assert lengths, NextKey presence/absence and that IDs differ across pages; also add a variant that combines Pagination.Reverse = true with Filters (v1beta4.DeploymentFilters{State: v1.DeploymentActive.String()}) to validate cross-state continuation/resume behavior and ensure ordering is stable when reversing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x/deployment/keeper/grpc_query.go`:
- Around line 54-57: The handler currently treats errors from
DecodePaginationKey as internal server errors; change the error returns to use
codes.InvalidArgument so malformed pagination keys are treated as client errors.
Update the error handling around calls to query.DecodePaginationKey (the
occurrences assigning to states, _, pkBytes, _, err) in grpc_query.go to return
status.Error(codes.InvalidArgument, err.Error()) instead of
status.Error(codes.Internal, ...), and do the same for the second
DecodePaginationKey call referenced in the same file.
- Around line 51-55: When req.Pagination.Key is provided, DecodePaginationKey
already returns "states" in the remaining traversal order, so do NOT reverse
"states" again when req.Pagination.Reverse is true; updating the code paths
around DecodePaginationKey (where states, pkBytes are assigned) should skip the
states reversal and avoid reassigning resumePK based on a reversed bucket.
Change logic in the blocks that call query.DecodePaginationKey (the branches
that inspect req.Pagination.Key) so you only apply states reversal when there is
NO Pagination.Key (i.e., initial query), and ensure resumePK selection and
subsequent paging use the unmodified "states" returned by DecodePaginationKey to
prevent bucket misalignment across pages.
In `@x/market/keeper/grpc_query.go`:
- Around line 51-55: Decoded pagination states from req.Pagination.Key are being
reversed again when req.Pagination.Reverse is set, which can flip state order
twice and misalign resumePK; update the handlers that call
query.DecodePaginationKey (where variables like states, pkBytes and resumePK are
used) to only reverse the decoded states when constructing resumePK if and only
if the original DecodePaginationKey did not already account for reverse
pagination—i.e., detect the intended direction from req.Pagination.Reverse and
either use states as returned or reverse it once before computing resumePK
(apply the same one-time-reverse logic to all three handlers that use
query.DecodePaginationKey to build resumePK for Orders/Bids/Leases).
- Around line 193-200: The continuation key created by reverseSearch (and
similar Bids/Leases pagination paths) drops the provider prefix so subsequent
page-only queries lose provider context; update the pagination key construction
so DecodePaginationKey(req.Pagination.Key) yields states, pkBytes, unsolicited
and you then build NextKey by re-prepending the provider/index prefix before the
state byte and pkBytes (i.e., reconstruct the full prefixed key rather than only
using the state-byte or pkBytes). Locate usages around reverseSearch and where
NextKey is assigned (references: reverseSearch, states, pkBytes, unsolicited,
req.Pagination.Key, NextKey) and change the encoding to concat(providerPrefix,
stateByte, pkBytes) so provider context is preserved across pages.
- Around line 264-269: The provider-index branches for Bids/Leases ignore
req.Pagination.Reverse and always iterate forward; update the code in the blocks
using resumePK + collections.NewPrefixedPairRange[string, keys.BidPrimaryKey]
and the calls to k.bids.Indexes.Provider.Iterate / MatchExact so that when
req.Pagination.Reverse is true you build a descending range (swap start/end
inclusivity like in the reverseSearch path) and call the index's descending
iterator variant (mirror the reverseSearch logic), and do the same fix for the
Leases provider-index branch (lines ~491-496) so pagination.Reverse is respected
consistently.
---
Nitpick comments:
In `@x/deployment/keeper/grpc_query_test.go`:
- Around line 304-359: Add a new t.Run that exercises reverse
(Pagination.Reverse = true) key-based pagination for Deployments using
suite.queryClient.Deployments: request a last-item-first page with
&sdkquery.PageRequest{Limit:1, Reverse:true}, capture the returned
Pagination.NextKey, then fetch the previous page by calling Deployments again
with &sdkquery.PageRequest{Key: <prevKey>, Limit:1, Reverse:true} and assert
lengths, NextKey presence/absence and that IDs differ across pages; also add a
variant that combines Pagination.Reverse = true with Filters
(v1beta4.DeploymentFilters{State: v1.DeploymentActive.String()}) to validate
cross-state continuation/resume behavior and ensure ordering is stable when
reversing.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
x/deployment/keeper/grpc_query.gox/deployment/keeper/grpc_query_test.gox/market/keeper/grpc_query.go
Signed-off-by: Artur Troian <troian@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
x/market/keeper/grpc_query.go (2)
188-275:⚠️ Potential issue | 🟠 Major
reverseSearchkey-based continuation may fail when filters aren't re-supplied.When a client uses key-based pagination (page 2+), the
reverseSearchflag is restored from the decoded key'sunsolicitedbyte (line 202-204). However,req.Filters.Providerat line 265 may be empty if the client doesn't re-supply filters on subsequent pages.This causes the provider-index iteration to use an empty prefix, returning incorrect or empty results.
Consider either:
- Encoding the provider in the pagination key and restoring it on decode, or
- Documenting that clients must re-supply filters when using key-based pagination.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/market/keeper/grpc_query.go` around lines 188 - 275, The pagination key currently only stores states, pk and an unsolicited byte, but the provider prefix used for Provider index iteration (k.bids.Indexes.Provider.Iterate/MatchExact) can be empty on resumed pages; modify the pagination key encoding/decoding so the provider string is included and restored: update encodeBidNextKey to pass req.Filters.Provider (as bytes) into query.EncodePaginationKey and update the DecodePaginationKey handling (where states, pkBytes, unsolicited are extracted) to also retrieve and set a local restoredProvider (or assign back to req.Filters.Provider) before using it in Provider.Iterate/MatchExact; additionally, add a validation branch so if a decoded key indicates provider-scoped iteration but no provider is present after decode, return InvalidArgument to avoid iterating with an empty prefix.
421-508:⚠️ Potential issue | 🟠 MajorSame
reverseSearchcontinuation issue applies to Leases.The Leases handler has the same issue as Bids: when
reverseSearchis restored from the pagination key (line 435-437), butreq.Filters.Provideris empty on page 2+, the provider-index iteration at line 498 will use an empty prefix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/market/keeper/grpc_query.go` around lines 421 - 508, The pagination key may set reverseSearch and provide a resume primary key (resumePK) but the code still uses req.Filters.Provider (which can be empty) when creating the provider index iteration; update the iteration logic to derive the provider prefix from the decoded resumePK when req.Filters.Provider == "" so you don't call NewPrefixedPairRange with an empty prefix. Concretely: after Decode(...) that sets resumePK, if req.Filters.Provider == "" extract provider := resumePK.Provider (from keys.LeasePrimaryKey) and use that provider variable when constructing collections.NewPrefixedPairRange[string, keys.LeasePrimaryKey](provider) for the Iterate/EndInclusive/StartInclusive/Descending branches (and likewise when deciding MatchExact vs Iterate), so the resumed iteration uses the actual provider from the pagination key.
🧹 Nitpick comments (3)
x/market/keeper/grpc_query.go (1)
98-109: Consider refactoring if-else chain to switch for clarity.Static analysis (gocritic) suggests rewriting this if-else chain as a switch statement. The same pattern appears in Bids (lines 264-275, 331-342) and Leases (lines 497-508, 564-575).
♻️ Example refactor for Orders
- if idx == 0 && resumePK != nil { - r := collections.NewPrefixedPairRange[int32, keys.OrderPrimaryKey](int32(state)).StartInclusive(*resumePK) - if req.Pagination.Reverse { - r = collections.NewPrefixedPairRange[int32, keys.OrderPrimaryKey](int32(state)).EndInclusive(*resumePK).Descending() - } - iter, err = k.orders.Indexes.State.Iterate(ctx, r) - } else if req.Pagination.Reverse { - iter, err = k.orders.Indexes.State.Iterate(ctx, - collections.NewPrefixedPairRange[int32, keys.OrderPrimaryKey](int32(state)).Descending()) - } else { - iter, err = k.orders.Indexes.State.MatchExact(ctx, int32(state)) - } + switch { + case idx == 0 && resumePK != nil: + r := collections.NewPrefixedPairRange[int32, keys.OrderPrimaryKey](int32(state)).StartInclusive(*resumePK) + if req.Pagination.Reverse { + r = collections.NewPrefixedPairRange[int32, keys.OrderPrimaryKey](int32(state)).EndInclusive(*resumePK).Descending() + } + iter, err = k.orders.Indexes.State.Iterate(ctx, r) + case req.Pagination.Reverse: + iter, err = k.orders.Indexes.State.Iterate(ctx, + collections.NewPrefixedPairRange[int32, keys.OrderPrimaryKey](int32(state)).Descending()) + default: + iter, err = k.orders.Indexes.State.MatchExact(ctx, int32(state)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/market/keeper/grpc_query.go` around lines 98 - 109, Refactor the if-else chain around idx, resumePK and req.Pagination.Reverse into a switch or type-less switch on boolean conditions to improve clarity: locate the block that currently chooses between calling k.orders.Indexes.State.Iterate(...) with a StartInclusive/EndInclusive/Descending range or k.orders.Indexes.State.MatchExact(...), and replace the nested if/else logic (using variables idx, resumePK, req.Pagination.Reverse and range builder calls like collections.NewPrefixedPairRange(...).StartInclusive/EndInclusive().Descending()) with a switch that has clear cases for "idx == 0 && resumePK != nil", "req.Pagination.Reverse", and the default case, leaving the exact calls to k.orders.Indexes.State.Iterate and MatchExact unchanged; apply the same pattern to the analogous Bids and Leases blocks referenced in the review.x/deployment/keeper/grpc_query.go (1)
98-109: Consider refactoring if-else chain to switch.Same static analysis suggestion as in
x/market/keeper/grpc_query.go- the if-else chain could be rewritten as a switch statement for improved readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/deployment/keeper/grpc_query.go` around lines 98 - 109, Refactor the if-else chain that selects the iterator into a switch for clarity: replace the current conditional block using idx, resumePK, and req.Pagination.Reverse with a switch (e.g., switch { ... }) that has three cases matching the existing branches (1: idx == 0 && resumePK != nil — create r via collections.NewPrefixedPairRange(...).StartInclusive or .EndInclusive when req.Pagination.Reverse is true, then call k.deployments.Indexes.State.Iterate with r; 2: req.Pagination.Reverse — call Iterate with collections.NewPrefixedPairRange(...).Descending(); 3: default — call k.deployments.Indexes.State.MatchExact). Ensure you keep the same uses of resumePK, int32(state), and error/iter handling as in the original (k.deployments.Indexes.State.Iterate / MatchExact).x/deployment/keeper/grpc_query_test.go (1)
283-302: Note:Totalrepresents per-page count, not global total.The tests correctly validate that
Totalequals the count of returned items on each page. However, this differs from the typical Cosmos SDK convention wherePageResponse.Totalrepresents the total count of matching records across all pages.If this is intentional (e.g., due to multi-state iteration complexity), consider documenting this behavior in API comments to avoid consumer confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/deployment/keeper/grpc_query_test.go` around lines 283 - 302, Tests currently assert that res.Pagination.Total equals the count of items on the returned page (per-page total) which deviates from Cosmos SDK’s usual PageResponse.Total meaning (global total); to address the reviewer request, update the API documentation where Deployments pagination is defined (e.g., the Deployments query handler and the QueryDeploymentsRequest/response comment near the keeper/query service definition) to explicitly state that Pagination.Total is the per-page count in this implementation and why (referencing QueryDeployments / Deployments handler and the PageResponse.Total field), so consumers are not confused; alternatively, if you want to follow Cosmos convention instead, change the Deployments pagination implementation to compute and populate the global total and adjust the tests accordingly (update assertions in grpc_query_test.go for QueryDeployments).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@x/market/keeper/grpc_query.go`:
- Around line 188-275: The pagination key currently only stores states, pk and
an unsolicited byte, but the provider prefix used for Provider index iteration
(k.bids.Indexes.Provider.Iterate/MatchExact) can be empty on resumed pages;
modify the pagination key encoding/decoding so the provider string is included
and restored: update encodeBidNextKey to pass req.Filters.Provider (as bytes)
into query.EncodePaginationKey and update the DecodePaginationKey handling
(where states, pkBytes, unsolicited are extracted) to also retrieve and set a
local restoredProvider (or assign back to req.Filters.Provider) before using it
in Provider.Iterate/MatchExact; additionally, add a validation branch so if a
decoded key indicates provider-scoped iteration but no provider is present after
decode, return InvalidArgument to avoid iterating with an empty prefix.
- Around line 421-508: The pagination key may set reverseSearch and provide a
resume primary key (resumePK) but the code still uses req.Filters.Provider
(which can be empty) when creating the provider index iteration; update the
iteration logic to derive the provider prefix from the decoded resumePK when
req.Filters.Provider == "" so you don't call NewPrefixedPairRange with an empty
prefix. Concretely: after Decode(...) that sets resumePK, if
req.Filters.Provider == "" extract provider := resumePK.Provider (from
keys.LeasePrimaryKey) and use that provider variable when constructing
collections.NewPrefixedPairRange[string, keys.LeasePrimaryKey](provider) for the
Iterate/EndInclusive/StartInclusive/Descending branches (and likewise when
deciding MatchExact vs Iterate), so the resumed iteration uses the actual
provider from the pagination key.
---
Nitpick comments:
In `@x/deployment/keeper/grpc_query_test.go`:
- Around line 283-302: Tests currently assert that res.Pagination.Total equals
the count of items on the returned page (per-page total) which deviates from
Cosmos SDK’s usual PageResponse.Total meaning (global total); to address the
reviewer request, update the API documentation where Deployments pagination is
defined (e.g., the Deployments query handler and the
QueryDeploymentsRequest/response comment near the keeper/query service
definition) to explicitly state that Pagination.Total is the per-page count in
this implementation and why (referencing QueryDeployments / Deployments handler
and the PageResponse.Total field), so consumers are not confused; alternatively,
if you want to follow Cosmos convention instead, change the Deployments
pagination implementation to compute and populate the global total and adjust
the tests accordingly (update assertions in grpc_query_test.go for
QueryDeployments).
In `@x/deployment/keeper/grpc_query.go`:
- Around line 98-109: Refactor the if-else chain that selects the iterator into
a switch for clarity: replace the current conditional block using idx, resumePK,
and req.Pagination.Reverse with a switch (e.g., switch { ... }) that has three
cases matching the existing branches (1: idx == 0 && resumePK != nil — create r
via collections.NewPrefixedPairRange(...).StartInclusive or .EndInclusive when
req.Pagination.Reverse is true, then call k.deployments.Indexes.State.Iterate
with r; 2: req.Pagination.Reverse — call Iterate with
collections.NewPrefixedPairRange(...).Descending(); 3: default — call
k.deployments.Indexes.State.MatchExact). Ensure you keep the same uses of
resumePK, int32(state), and error/iter handling as in the original
(k.deployments.Indexes.State.Iterate / MatchExact).
In `@x/market/keeper/grpc_query.go`:
- Around line 98-109: Refactor the if-else chain around idx, resumePK and
req.Pagination.Reverse into a switch or type-less switch on boolean conditions
to improve clarity: locate the block that currently chooses between calling
k.orders.Indexes.State.Iterate(...) with a
StartInclusive/EndInclusive/Descending range or
k.orders.Indexes.State.MatchExact(...), and replace the nested if/else logic
(using variables idx, resumePK, req.Pagination.Reverse and range builder calls
like
collections.NewPrefixedPairRange(...).StartInclusive/EndInclusive().Descending())
with a switch that has clear cases for "idx == 0 && resumePK != nil",
"req.Pagination.Reverse", and the default case, leaving the exact calls to
k.orders.Indexes.State.Iterate and MatchExact unchanged; apply the same pattern
to the analogous Bids and Leases blocks referenced in the review.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
x/deployment/keeper/grpc_query.gox/deployment/keeper/grpc_query_test.gox/market/keeper/grpc_query.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.md