fix: honour status query param in list transactions endpoint#792
fix: honour status query param in list transactions endpoint#792shaunwackerly wants to merge 1 commit into
Conversation
The GET /relayers/{id}/transactions endpoint accepted a ?status= query
parameter but silently discarded it. PaginationQuery only contained page
and per_page, so every call returned all transactions for the relayer
regardless of the requested status.
Introduce TransactionListQuery (pagination.rs) which adds an optional
status field and a From<TransactionListQuery> impl for PaginationQuery.
Update the route to deserialise TransactionListQuery and the controller
to branch on the presence of a status filter:
- status present: delegate to the existing find_by_status_paginated,
which uses per-status sorted sets for efficient indexed lookups.
- status absent: delegate to find_by_relayer_id as before.
No repository changes are required; both methods already exist on the
TransactionRepository trait.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR extends the transaction listing endpoint to support optional status filtering. A new ChangesTransaction List Status Filtering
Sequence DiagramsequenceDiagram
participant Client
participant list_transactions as list_transactions handler
participant repo as Transaction repository
Client->>list_transactions: GET with query (page, per_page, status?)
alt status is Some
list_transactions->>repo: list_transactions_by_status(status, pagination)
else status is None
list_transactions->>repo: list_transactions_by_relayer_id(relayer_id, pagination)
end
repo-->>list_transactions: paginated transactions
list_transactions-->>Client: transaction list response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you for your contribution to OpenZeppelin Relayer. Before being able to integrate those changes, we would like you to sign our Contributor License Agreement. You can sign the CLA by just posting a Pull Request Comment with the sentence below. Thanks. I confirm that I have read and hereby agree to the OpenZeppelin Contributor License Agreement You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
There was a problem hiding this comment.
Pull request overview
This PR fixes the relayer transactions listing endpoint so that the status query parameter is actually honored, enabling clients to retrieve only transactions in a specific TransactionStatus while retaining pagination.
Changes:
- Introduces
TransactionListQuery(pagination + optionalstatus) for query deserialization. - Updates the relayer routes/controllers to accept the new query type.
- Adds branching logic in the controller to call
find_by_status_paginatedwhen a status filter is present.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/models/pagination.rs |
Adds TransactionListQuery and a conversion to PaginationQuery to support optional status filtering alongside pagination. |
src/api/routes/relayer.rs |
Switches the list-transactions route extractor to TransactionListQuery and updates the route docstring to mention status filtering. |
src/api/controllers/relayer.rs |
Implements status-aware branching: filtered queries use find_by_status_paginated, otherwise the existing find_by_relayer_id path is used. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[serde(default = "default_per_page")] | ||
| pub per_page: u32, | ||
| pub status: Option<TransactionStatus>, | ||
| } |
| let pagination = PaginationQuery::from(query.clone()); | ||
| let transactions = match query.status { | ||
| Some(status) => { | ||
| state | ||
| .transaction_repository | ||
| .find_by_status_paginated(&relayer_id, &[status], pagination, false) | ||
| .await? | ||
| } | ||
| None => { | ||
| state | ||
| .transaction_repository | ||
| .find_by_relayer_id(&relayer_id, pagination) | ||
| .await? | ||
| } | ||
| }; |
| let transactions = match query.status { | ||
| Some(status) => { | ||
| state | ||
| .transaction_repository | ||
| .find_by_status_paginated(&relayer_id, &[status], pagination, false) |
| /// Lists all transactions for a specific relayer with pagination and optional status filter. | ||
| #[get("/relayers/{relayer_id}/transactions")] | ||
| async fn list_transactions( | ||
| relayer_id: web::Path<String>, | ||
| query: web::Query<PaginationQuery>, | ||
| query: web::Query<TransactionListQuery>, |
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
PR looks good!
Few things to wrap it up:
- Update desc, only lowercase statuses should be accepted(per deserialization)
- Add tests for newly added logic
- Update src/api/routes/docs/relayer_docs.rs: docs to include status param for openapi spec generation
- Accept CLA per #792 (comment)
Summary
GET /relayers/{id}/transactions?status=Sent(and any other status value) was silently ignored — the endpoint always returned all transactions for the relayer regardless of the requested status.PaginationQueryonly hadpage/per_pagefields. The?status=parameter was unknown to the deserializer and dropped. The controller calledfind_by_relayer_id, which applies no status filter.Changes
src/models/pagination.rsTransactionListQuery— a copy ofPaginationQueryextended with an optionalstatus: Option<TransactionStatus>field.From<TransactionListQuery> for PaginationQueryfor ergonomic conversion.src/api/routes/relayer.rslist_transactionsroute now deserialisesTransactionListQueryinstead ofPaginationQuery.src/api/controllers/relayer.rslist_transactionsacceptsTransactionListQueryand branches on the presence of a status filter:find_by_status_paginated, which uses the per-status sorted sets already maintained by the repository for efficient indexed lookups.find_by_relayer_idas before.No repository changes are required; both methods already exist on
TransactionRepository.Test plan
cargo test --lib).GET /relayers/{id}/transactions?status=Sentreturns onlySenttransactions.GET /relayers/{id}/transactions(no filter) still returns all transactions.GET /relayers/{id}/transactions?status=Failedreturns onlyFailedtransactions.page/per_page) works correctly in both filtered and unfiltered modes.🤖 Generated with Claude Code
Summary by CodeRabbit