refactor(x/marker): cleanup decode/encode for pagination keys #2005
Conversation
WalkthroughRefactors pagination in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant G as gRPC Handler
participant K as Keeper (Orders/Bids/Leases)
participant E as Pagination Encoder
C->>G: Query with pagination params
G->>K: Fetch and iterate store states
rect rgba(200,220,255,0.18)
loop iterate states until limit
K->>K: collect result, increment total, set idx
alt reached limit
K-->>K: break loop
end
end
end
alt pageRes != nil
K->>E: EncodePaginationKey(states[idx:], searchPrefix, pageRes.NextKey, unsolicited?)
alt encode succeeds
E-->>K: NextKey
K-->>G: Response {results, total, nextKey}
else encode fails
E-->>K: Error
K-->>G: Partial response + Internal error
end
else pageRes == nil
K-->>G: Response {results} (no pagination)
end
G-->>C: Reply
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/market/keeper/grpc_query.go (1)
188-195: Fix out-of-bounds read on unsolicited flag in Bids decode.Indexing unsolicited[1] panics when len(unsolicited) == 1. Should be unsolicited[0].
Apply this diff:
- if len(unsolicited) != 1 { + if len(unsolicited) != 1 { return nil, status.Error(codes.InvalidArgument, "invalid pagination key") } req.Pagination.Key = key - if unsolicited[1] == 1 { + if unsolicited[0] == 1 { reverseSearch = true }
🧹 Nitpick comments (4)
x/market/keeper/grpc_query.go (4)
285-303: Encode NextKey direction byte consistently (Bids).Good use of a single-byte unsolicited vector to persist reverseSearch. Consider extracting a small helper (e.g., encodeNextKeyWithDirection) to de‑duplicate this pattern across Bids/Leases.
If you want, I can draft a minimal helper and its unit tests.
255-263: Prefer consistent ID accessor for escrow account lookup.Elsewhere in this file you use bid.ID(). For consistency and readability, use the same here.
Apply this diff:
- acct, err := k.ekeeper.GetAccount(ctx, types.EscrowAccountForBid(bid.BidID)) + acct, err := k.ekeeper.GetAccount(ctx, types.EscrowAccountForBid(bid.ID()))
285-287: Confirm PageResponse.Total semantics.You now set pageRes.Total = total unconditionally. If clients rely on Cosmos SDK’s CountTotal flag semantics, this may differ from expectations. Please confirm intended behavior or gate behind req.Pagination.CountTotal.
395-399: Nit: unify variable naming for the paginated store.searchedStore → searchStore for consistency with Orders/Bids.
Apply this diff:
- searchedStore := prefix.NewStore(ctx.KVStore(k.skey), searchPrefix) + searchStore := prefix.NewStore(ctx.KVStore(k.skey), searchPrefix) - pageRes, err = sdkquery.FilteredPaginate(searchedStore, req.Pagination, func(key []byte, value []byte, accumulate bool) (bool, error) { + pageRes, err = sdkquery.FilteredPaginate(searchStore, req.Pagination, func(key []byte, value []byte, accumulate bool) (bool, error) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/market/keeper/grpc_query.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
x/market/keeper/grpc_query.go (2)
util/query/pagination.go (1)
EncodePaginationKey(91-143)x/market/query/types.go (3)
Orders(13-13)Bids(18-18)Leases(23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: lint
- GitHub Check: coverage
- GitHub Check: release-dry-run
- GitHub Check: build-bins
- GitHub Check: tests
- GitHub Check: release-dry-run
- GitHub Check: lint
- GitHub Check: build-bins
- GitHub Check: tests
- GitHub Check: coverage
🔇 Additional comments (2)
x/market/keeper/grpc_query.go (2)
83-87: Pagination refactor LGTM; post-loop NextKey handling is cleaner and consistent.Using idx from the for-range and encoding NextKey with states[idx:] after the loop is correct and avoids per-iteration re-encoding. The pageRes nil check is also good.
Please add/extend tests that cover:
- continuation within the same state (NextKey present),
- transition to the next state when the current state is exhausted (NextKey empty),
- decode/encode round‑trip of pagination keys across state boundaries.
Also applies to: 137-149
372-440: Leases pagination refactor LGTM; mirrors Bids correctly.Direction flag handling uses unsolicited[0] (correct). Post-loop NextKey encoding is consistent.
Also applies to: 442-461
Signed-off-by: Artur Troian <troian@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
x/market/keeper/grpc_query.go (5)
137-149: Gate Total on CountTotal; add safety on states slice before encoding.
- PageResponse.Total should typically be set only when PageRequest.CountTotal is true; unconditionally setting it may change API behavior.
- Encode slice with a defensive check to avoid a potential panic if idx is out of range (edge regressions or future edits).
Apply this diff:
- if pageRes != nil { - pageRes.Total = total + if pageRes != nil { + if req.Pagination != nil && req.Pagination.CountTotal { + pageRes.Total = total + } if len(pageRes.NextKey) > 0 { - pageRes.NextKey, err = query.EncodePaginationKey(states[idx:], searchPrefix, pageRes.NextKey, nil) + if idx < 0 || idx >= len(states) { + return &types.QueryOrdersResponse{Orders: orders, Pagination: pageRes}, status.Error(codes.Internal, "invalid pagination state index") + } + pageRes.NextKey, err = query.EncodePaginationKey(states[idx:], searchPrefix, pageRes.NextKey, nil) if err != nil { return &types.QueryOrdersResponse{ Orders: orders, Pagination: pageRes, }, status.Error(codes.Internal, err.Error()) } } }
193-196: Replace magic number with named flag for reverseSearch.Use a named const and construct the byte in one place for readability and to avoid 0/1 magic values.
Apply this diff:
- if unsolicited[0] == 1 { + const unsolicitedReverse byte = 1 + if unsolicited[0] == unsolicitedReverse { reverseSearch = true }
285-303: Factor NextKey encoding into a small helper to remove duplication and centralize checks.Orders/Bids/Leases now share identical post-loop blocks. A helper will reduce risk of future divergence and make unit testing easier.
Example inline refactor within this block:
- pageRes.Total = total - if len(pageRes.NextKey) > 0 { - unsolicited := make([]byte, 1) - unsolicited[0] = 0 - if reverseSearch { - unsolicited[0] = 1 - } - pageRes.NextKey, err = query.EncodePaginationKey(states[idx:], searchPrefix, pageRes.NextKey, unsolicited) - if err != nil { - pageRes.Total = total - return &types.QueryBidsResponse{ - Bids: bids, - Pagination: pageRes, - }, status.Error(codes.Internal, err.Error()) - } - } + if req.Pagination != nil && req.Pagination.CountTotal { + pageRes.Total = total + } + if err := encodeNextKey(&pageRes.NextKey, states, idx, searchPrefix, reverseSearch); err != nil { + if req.Pagination != nil && req.Pagination.CountTotal { + pageRes.Total = total + } + return &types.QueryBidsResponse{Bids: bids, Pagination: pageRes}, status.Error(codes.Internal, err.Error()) + }And add (once) in this file:
func encodeNextKey(dst *[]byte, states []byte, idx int, prefix []byte, reverse bool) error { if dst == nil || len(*dst) == 0 { return nil } if idx < 0 || idx >= len(states) { return fmt.Errorf("invalid pagination state index") } var flag byte if reverse { flag = 1 } next, err := query.EncodePaginationKey(states[idx:], prefix, *dst, []byte{flag}) if err != nil { return err } *dst = next return nil }
442-460: Mirror the Orders/Bids fixes: gate Total on CountTotal and add idx bounds check.Keep behavior uniform across handlers and prevent accidental slice panics.
Apply this diff:
- if pageRes != nil { - pageRes.Total = total - if len(pageRes.NextKey) > 0 { - unsolicited := make([]byte, 1) - unsolicited[0] = 0 - if reverseSearch { - unsolicited[0] = 1 - } - pageRes.NextKey, err = query.EncodePaginationKey(states[idx:], searchPrefix, pageRes.NextKey, unsolicited) - if err != nil { - pageRes.Total = total - return &types.QueryLeasesResponse{ - Leases: leases, - Pagination: pageRes, - }, status.Error(codes.Internal, err.Error()) - } - } - } + if pageRes != nil { + if req.Pagination != nil && req.Pagination.CountTotal { + pageRes.Total = total + } + if err := encodeNextKey(&pageRes.NextKey, states, idx, searchPrefix, reverseSearch); err != nil { + if req.Pagination != nil && req.Pagination.CountTotal { + pageRes.Total = total + } + return &types.QueryLeasesResponse{Leases: leases, Pagination: pageRes}, status.Error(codes.Internal, err.Error()) + } + }
395-399: Nit: variable name consistency.Consider renaming searchedStore -> searchStore to match the other handlers.
- searchedStore := prefix.NewStore(ctx.KVStore(k.skey), searchPrefix) + searchStore := prefix.NewStore(ctx.KVStore(k.skey), searchPrefix)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/market/keeper/grpc_query.go(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
x/market/keeper/grpc_query.go (1)
util/query/pagination.go (1)
EncodePaginationKey(91-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: lint
- GitHub Check: release-dry-run
- GitHub Check: tests
- GitHub Check: build-bins
- GitHub Check: coverage
- GitHub Check: tests
- GitHub Check: release-dry-run
- GitHub Check: coverage
- GitHub Check: lint
- GitHub Check: build-bins
🔇 Additional comments (4)
x/market/keeper/grpc_query.go (4)
83-99: Good: explicit idx and deferring NextKey work.Using an explicit loop index and moving NextKey handling post-loop improves consistency across states and avoids per-iteration re-encoding. Resetting Pagination.Key on subsequent states is correct.
217-221: Good: shared idx and err simplify post-loop encoding.
372-375: Good: aligns Leases with Orders/Bids for idx/err scope.
437-440: LGTM: early break on limit preserves remaining states for NextKey encoding.
| if pageRes != nil { | ||
| pageRes.Total = total | ||
|
|
||
| if len(pageRes.NextKey) > 0 { | ||
| pageRes.NextKey, err = query.EncodePaginationKey(states[idx:], searchPrefix, pageRes.NextKey, nil) | ||
| if err != nil { | ||
| return &types.QueryOrdersResponse{ | ||
| Orders: orders, | ||
| Pagination: pageRes, | ||
| }, status.Error(codes.Internal, err.Error()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify backward compatibility of pagination keys and Total semantics.
Please confirm:
- Old NextKey values still decode under the new scheme and lead to identical result sets.
- Total is unchanged for clients when CountTotal is false.
Run:
Also applies to: 285-303, 442-460
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# 1) Find all Encode/DecodePaginationKey call sites
rg -nP --type=go 'EncodePaginationKey\(|DecodePaginationKey\(' -C2
# 2) Check for callers that rely on PageResponse.Total without CountTotal
rg -nP --type=go 'PageRequest\{[^}]*CountTotal:\s*false' -C3 || true
rg -nP --type=go 'Pagination:\s*&sdkquery\.PageRequest\{[^}]*CountTotal:\s*false' -C3 || true
# 3) Grep tests for pagination behavior
rg -nP --type=go '(NextKey|CountTotal|PageResponse\.Total)' -g '!**/vendor/**' -C3Length of output: 7895
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== util/query/pagination.go =="
sed -n '1,240p' util/query/pagination.go || true
echo
echo "== util/query/pagination_test.go =="
sed -n '1,400p' util/query/pagination_test.go || true
echo
echo "== x/market/keeper/grpc_query.go (1..520) =="
sed -n '1,520p' x/market/keeper/grpc_query.go || true
echo
echo "== x/cert/keeper/grpc_query.go (1..240) =="
sed -n '1,240p' x/cert/keeper/grpc_query.go || true
echo
echo "== x/deployment/keeper/grpc_query.go (1..320) =="
sed -n '1,320p' x/deployment/keeper/grpc_query.go || true
echo
echo "== rg CountTotal occurrences =="
rg -n "CountTotal" || true
echo
echo "== rg pageRes.Total occurrences =="
rg -n "pageRes.Total" || true
echo
echo "== rg EncodePaginationKey / DecodePaginationKey definitions =="
rg -n "func (EncodePaginationKey|DecodePaginationKey|EncodePaginationKey|DecodePaginationKey)" util -S || trueLength of output: 34601
Critical: pagination key encoding breaks legacy NextKey and CountTotal semantics
-
Legacy NextKey incompatible — util/query/pagination.go::DecodePaginationKey requires the new checksum+structure and will return an error for raw NextKey values returned by older FilteredPaginate. Affected handlers: x/market/keeper/grpc_query.go (Decode at line ~55; Encode at ~141/294/452), x/cert/keeper/grpc_query.go (Decode at ~50; Encode at ~126), x/deployment/keeper/grpc_query.go (Decode at ~58; Encode at ~148). Action: add a backward-compatible fallback (e.g. on ErrInvalidPaginationKey treat the incoming key as legacy raw key and continue, or implement versioned keys).
-
CountTotal semantics violated — handlers set pageRes.Total = total unconditionally (x/market/... lines ~138/286/296/443/454; x/cert/... ~141; x/deployment/... ~162), so clients with CountTotal=false still receive totals. Action: only populate pageRes.Total when req.Pagination.CountTotal == true (or otherwise preserve legacy behavior).
🤖 Prompt for AI Agents
In x/market/keeper/grpc_query.go around lines 137 to 149, the pagination
handling breaks compatibility: EncodePaginationKey produces a structured key
that DecodePaginationKey will reject if callers send legacy raw NextKey values,
and pageRes.Total is always set which violates CountTotal semantics. Fix by
catching the pagination decode/encode error (specifically treat
ErrInvalidPaginationKey as a legacy raw key and continue using the raw key bytes
instead of failing), and only populate pageRes.Total when req.Pagination != nil
&& req.Pagination.CountTotal == true (leave it unset otherwise) so
CountTotal=false preserves the legacy behavior.
No description provided.