fix(tokenfactory): add pagination to DenomsFromCreator query (PLT-410)#3524
fix(tokenfactory): add pagination to DenomsFromCreator query (PLT-410)#3524amir-deris wants to merge 9 commits into
Conversation
PR SummaryMedium Risk Overview The proto request/response gain Wasm queries without pagination (or with Reviewed by Cursor Bugbot for commit 8a7754c. Bugbot is set up for automated code reviews on this repo. Configure here. |
amir-deris
changed the title
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3524 +/- ##
==========================================
- Coverage 59.22% 58.35% -0.88%
==========================================
Files 2214 2140 -74
Lines 183402 174868 -8534
==========================================
- Hits 108619 102042 -6577
+ Misses 64994 63733 -1261
+ Partials 9789 9093 -696
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@claude review this pr |
| // defaultDenomsFromCreatorLimit is the wasm query default, higher than the gRPC DefaultLimit of 100 | ||
| // for backwards compatibility with contracts that expect all denoms in one response, but still | ||
| // bounded to limit DoS risk since denom creation has no fee. | ||
| const defaultDenomsFromCreatorLimit = 2000 |
There was a problem hiding this comment.
Does this value make sense given #3494 is open?
2K will be rejected once that PR lands. What are your thoughts in terms of coordinating the two PRs?
There was a problem hiding this comment.
Thanks for great feedback, that is a great point. I will modify this to reference the query.MaxLimit instead which should still work and not conflict with PR 3494
| // DenomsFromCreator gRPC query. | ||
| message QueryDenomsFromCreatorResponse { | ||
| repeated string denoms = 1 [(gogoproto.moretags) = "yaml:\"denoms\""]; | ||
| cosmos.base.query.v1beta1.PageResponse pagination = 2; |
There was a problem hiding this comment.
I think this would be an apphash breaking change but the PR is labeled as non-apphash breaking.
There was a problem hiding this comment.
Thanks for feedback. It seems that is not the case regarding app hash breaking change, since this is a protocol buffer query and results are not part of the state. Do we have any test that would fail if this was an app hash breaking change and incorrectly labeled non-app-hash-breaking?
The app hash is the Merkle root of committed state — it only changes when something alters what gets written to the KV store during block execution (DeliverTx, BeginBlock, EndBlock, InitChain). The changes here
are entirely in the query plane:
- QueryDenomsFromCreatorRequest and QueryDenomsFromCreatorResponse are only used by the gRPC/REST query handler — they never touch state, never go through consensus, and are never committed.
- query.pb.go / query.pb.gw.go regeneration only affects how query messages are serialized over the wire; it has no effect on the state machine.
- x/tokenfactory/keeper/creators.go now calls query.Paginate instead of a raw iterator, but getDenomsFromCreator is a read-only keeper method — it doesn't write anything.
The only thing that could make a query-proto change app-hash-breaking is if the query result were hashed into state somewhere (e.g. stored as a module param or emitted as a deterministic event used in consensus),
which isn't the case here.```
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8a7754c. Configure here.
| req.Pagination = &query.PageRequest{Limit: defaultDenomsFromCreatorLimit} | ||
| } else if req.Pagination.Limit == 0 { | ||
| req.Pagination.Limit = defaultDenomsFromCreatorLimit | ||
| } |
There was a problem hiding this comment.
Wasm offset zero limit breaks
Medium Severity
When a wasm DenomsFromCreator query sets pagination.offset but leaves limit at zero, the handler replaces limit with query.MaxLimit. In query.Paginate, end is computed as offset + limit, which overflows for a large limit and an non-zero offset, so no denoms match the window and the response can be empty instead of the expected slice.
Reviewed by Cursor Bugbot for commit 8a7754c. Configure here.
2 participants
Summary
DenomsFromCreatorperformed an unbounded full prefix scan with no limit, making it unsafe.paginationfield toQueryDenomsFromCreatorRequestandQueryDenomsFromCreatorResponse, routing the handler throughquery.Paginatewhich enforces the MaxLimit/MaxOffset bounds being added in PLT-361 (fix(sei-cosmos): harden paginated RPC queries against DoS via limit, offset, and count_total caps (PLT-361) #3494)Changes
proto/tokenfactory/query.proto— addedPageRequest/PageResponsepagination fields toDenomsFromCreatorrequest and responsex/tokenfactory/types/query.pb.go— regenerated viascripts/protoc.shx/tokenfactory/types/query.pb.gw.go— regenerated; REST gateway now reads?pagination.*query parametersx/tokenfactory/keeper/creators.go— replaced raw unbounded iterator withquery.Paginatex/tokenfactory/keeper/grpc_query.go— threads pagination through the handlerx/tokenfactory/client/cli/query.go— exposes--page-*flags on thedenoms-from-creatorcommandx/tokenfactory/keeper/grpc_query_test.go— added pagination tests (no pagination, limit, key-based cursor, offset)wasmbinding/test/query_test.go— fixed assertions to compare onlyDenomsfield sincePaginationis now always non-nil in the responseNotes
DefaultLimit = 100resultsquery.Paginateonce PLT-361 lands; this PR depends on that for the cap to be in effect🤖 Generated with Claude Code