Implement GET /v1/trades/{address} endpoint#51
Merged
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds TradesDataSource::get_trades_for_owner and Raindex-backed implementation, wires get_trades_by_address to call processing logic that applies pagination and time filters, maps Raindex trades into API responses with pagination metadata, adds unit/route tests, and updates AGENTS.md with a Submodules note. ChangesTrade Listing by Address
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
This was referenced Apr 20, 2026
19 tasks
findolor
force-pushed
the
implement-trades-address
branch
from
10c59f1 to
0a05197
Compare
Collaborator
Author
How to use the Graphite Merge QueueAdd the label add-to-gt-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
4 tasks
findolor
force-pushed
the
implement-trades-address
branch
from
0a05197 to
10c59f1
Compare
findolor
force-pushed
the
implement-trades-tx-hash
branch
from
39f65e4 to
69db573
Compare
findolor
force-pushed
the
implement-trades-address
branch
from
10c59f1 to
e72d88e
Compare
findolor
force-pushed
the
implement-trades-tx-hash
branch
from
69db573 to
f7ecb76
Compare
findolor
force-pushed
the
implement-trades-address
branch
from
e72d88e to
249f984
Compare
JuaniRios
approved these changes
May 7, 2026
0xgleb
approved these changes
May 7, 2026
findolor
force-pushed
the
implement-trades-address
branch
from
249f984 to
3d47dad
Compare
findolor
force-pushed
the
implement-trades-tx-hash
branch
from
f7ecb76 to
6029bce
Compare
graphite-app
Bot
changed the base branch from
implement-trades-tx-hash
to
graphite-base/51
Merge activity
|
findolor
force-pushed
the
implement-trades-address
branch
from
3d47dad to
cded42d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/routes/trades/get_by_address.rs (1)
93-93: 💤 Low valueSilent
.ok()onorder_hashparsing discards errors without any observability.
FixedBytes::<32>::from_str(&trade.order_hash().to_string()).ok()will silently produceNoneif the string representation can't be re-parsed. Iforder_hash()already returns aB256/FixedBytes<32>, a direct.into()would be cleaner and infallible. If the string path is unavoidable, at least emit atracing::warnon failure so unexpectednullorder hashes are visible in logs.♻️ Option A — direct conversion (if order_hash() returns B256)
-let order_hash = FixedBytes::<32>::from_str(&trade.order_hash().to_string()).ok(); +let order_hash = Some(trade.order_hash());♻️ Option B — keep string path but add tracing on failure
-let order_hash = FixedBytes::<32>::from_str(&trade.order_hash().to_string()).ok(); +let order_hash = FixedBytes::<32>::from_str(&trade.order_hash().to_string()) + .inspect_err(|e| tracing::warn!(error = %e, "failed to parse order hash")) + .ok();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/trades/get_by_address.rs` at line 93, The current call FixedBytes::<32>::from_str(&trade.order_hash().to_string()).ok() swallows parse errors; change it to a safe, observable conversion: if trade.order_hash() already returns a B256/FixedBytes<32>, replace the whole expression with trade.order_hash().into() (infallible), otherwise keep the string path but handle the Result instead of calling .ok() — match or map_err to log a tracing::warn including the original string and the parse error and propagate or skip accordingly; update the variable where order_hash is used so it reflects the Result/Option handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/trades/get_by_address.rs`:
- Around line 59-72: Add an explicit check for page_size == 0 on params before
constructing PaginationParams so we return ApiError::BadRequest for zero page
sizes instead of forwarding Some(0) to the data source; specifically, in the
handler that reads params.page_size and builds PaginationParams (where page_size
is assigned and PaginationParams.page_size is set), validate
params.page_size.unwrap_or(20) (or the raw params.page_size if present) and if
it equals 0 return ApiError::BadRequest("page_size must be > 0"), otherwise
proceed to the existing try_into() conversion and construction of
PaginationParams.page_size.
---
Nitpick comments:
In `@src/routes/trades/get_by_address.rs`:
- Line 93: The current call
FixedBytes::<32>::from_str(&trade.order_hash().to_string()).ok() swallows parse
errors; change it to a safe, observable conversion: if trade.order_hash()
already returns a B256/FixedBytes<32>, replace the whole expression with
trade.order_hash().into() (infallible), otherwise keep the string path but
handle the Result instead of calling .ok() — match or map_err to log a
tracing::warn including the original string and the parse error and propagate or
skip accordingly; update the variable where order_hash is used so it reflects
the Result/Option handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08807d7c-44a5-499b-a305-abbc54a22159
📒 Files selected for processing (4)
AGENTS.mdsrc/routes/trades/get_by_address.rssrc/routes/trades/get_by_tx.rssrc/routes/trades/mod.rs
Comment on lines
+59
to
+72
| let page = params.page.unwrap_or(1); | ||
| let page_size = params.page_size.unwrap_or(20); | ||
|
|
||
| let pagination = PaginationParams { | ||
| page: Some( | ||
| page.try_into() | ||
| .map_err(|_| ApiError::BadRequest("page value too large".into()))?, | ||
| ), | ||
| page_size: Some( | ||
| page_size | ||
| .try_into() | ||
| .map_err(|_| ApiError::BadRequest("page_size value too large".into()))?, | ||
| ), | ||
| }; |
There was a problem hiding this comment.
Add explicit page_size == 0 validation before calling the data source.
If a caller sends page_size=0 the try_into() overflow check passes (0 fits any target integer type), so PaginationParams { page_size: Some(0) } is forwarded to the data source. The total_pages guard on line 127 prevents a divide-by-zero in this file, but the subgraph query itself may return an error or unexpected data, yielding a 500 instead of the correct 400.
🛡️ Proposed fix
let page = params.page.unwrap_or(1);
let page_size = params.page_size.unwrap_or(20);
+if page_size == 0 {
+ return Err(ApiError::BadRequest("page_size must be greater than 0".into()));
+}
+
let pagination = PaginationParams {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let page = params.page.unwrap_or(1); | |
| let page_size = params.page_size.unwrap_or(20); | |
| let pagination = PaginationParams { | |
| page: Some( | |
| page.try_into() | |
| .map_err(|_| ApiError::BadRequest("page value too large".into()))?, | |
| ), | |
| page_size: Some( | |
| page_size | |
| .try_into() | |
| .map_err(|_| ApiError::BadRequest("page_size value too large".into()))?, | |
| ), | |
| }; | |
| let page = params.page.unwrap_or(1); | |
| let page_size = params.page_size.unwrap_or(20); | |
| if page_size == 0 { | |
| return Err(ApiError::BadRequest("page_size must be greater than 0".into())); | |
| } | |
| let pagination = PaginationParams { | |
| page: Some( | |
| page.try_into() | |
| .map_err(|_| ApiError::BadRequest("page value too large".into()))?, | |
| ), | |
| page_size: Some( | |
| page_size | |
| .try_into() | |
| .map_err(|_| ApiError::BadRequest("page_size value too large".into()))?, | |
| ), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/trades/get_by_address.rs` around lines 59 - 72, Add an explicit
check for page_size == 0 on params before constructing PaginationParams so we
return ApiError::BadRequest for zero page sizes instead of forwarding Some(0) to
the data source; specifically, in the handler that reads params.page_size and
builds PaginationParams (where page_size is assigned and
PaginationParams.page_size is set), validate params.page_size.unwrap_or(20) (or
the raw params.page_size if present) and if it equals 0 return
ApiError::BadRequest("page_size must be > 0"), otherwise proceed to the existing
try_into() conversion and construction of PaginationParams.page_size.
## Chained PRs - #50 ## Motivation See issues: - #29 - #30 Add a paginated endpoint to retrieve all trades for a given owner address across all configured orderbooks, with optional time filtering. Replaces #40, which was based on the old chain (#39) and couldn't merge cleanly. This branch is rebased on top of `implement-trades-tx-hash` (PR #50). ## Solution - Implement `GET /v1/trades/{address}` with pagination (`page`, `pageSize`) and optional time filters (`startTime`, `endTime`) - Extend `TradesDataSource` trait with `get_trades_for_owner` method that iterates all configured orderbooks and aggregates trades and total counts - Wire the route handler to use `RaindexTradesDataSource` via `run_with_client`, converting `TradesPaginationParams` into `PaginationParams` + `TimeFilter` with overflow validation - Map each `RaindexTrade` into `TradeByAddress` response type (tx_hash, formatted amounts, `TokenRef` for input/output tokens, order_hash, timestamp, block_number) - Compute pagination metadata (total_pages via `div_ceil`, has_more) - Update `rain.orderbook` submodule (`ff9578c` → `a612a4f`) to bring in `get_trades_for_owner()`, `RaindexTradesListResult`, `PaginationParams`, `TimeFilter`, `OrderbookIdentifierParams`, and address casing fix - Update `get_by_tx.rs` mock to implement the new trait method - Add AGENTS.md submodule rule: never modify submodule code directly - Unit tests for success (with amount/token assertions), empty results, query failure, 401 without auth, and 500 on bad config ## Checks By submitting this for review, I'm confirming I've done the following: - [x] made this PR as small as possible - [x] unit-tested any new functionality - [x] linked any relevant issues or PRs - [ ] included screenshots (if this involves a front-end change) fix #29 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Implemented the trade lookup endpoint for wallet addresses with pagination and time-range filtering capabilities. * **Documentation** * Added guidelines for managing external code dependencies. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
Bot
force-pushed
the
implement-trades-address
branch
from
cded42d to
196377c
Compare
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
3 participants
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.
Chained PRs
Motivation
See issues:
Add a paginated endpoint to retrieve all trades for a given owner address across all configured orderbooks, with optional time filtering. Replaces #40, which was based on the old chain (#39) and couldn't merge cleanly. This branch is rebased on top of
implement-trades-tx-hash(PR #50).Solution
GET /v1/trades/{address}with pagination (page,pageSize) and optional time filters (startTime,endTime)TradesDataSourcetrait withget_trades_for_ownermethod that iterates all configured orderbooks and aggregates trades and total countsRaindexTradesDataSourceviarun_with_client, convertingTradesPaginationParamsintoPaginationParams+TimeFilterwith overflow validationRaindexTradeintoTradeByAddressresponse type (tx_hash, formatted amounts,TokenReffor input/output tokens, order_hash, timestamp, block_number)div_ceil, has_more)rain.orderbooksubmodule (ff9578c→a612a4f) to bring inget_trades_for_owner(),RaindexTradesListResult,PaginationParams,TimeFilter,OrderbookIdentifierParams, and address casing fixget_by_tx.rsmock to implement the new trait methodChecks
By submitting this for review, I'm confirming I've done the following:
fix #29
Summary by CodeRabbit
New Features
Documentation
Tests