feat: add token filters to getTrades#2570
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds unified trade retrieval across local SQLite and subgraph sources, including SQL and statement builders, async local-db query wrappers, subgraph trade query types and clients, a merged WASM ChangesFetch Trades Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 |
Collaborator
Author
How to use the Graphite Merge QueueAdd the label Raindex-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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/subgraph/src/multi_orderbook_client.rs (1)
245-322: ⚡ Quick winAdd test parity for new
trades_list/trades_countfan-out paths.These new methods would benefit from the same scenario coverage already present for orders/vaults (no subgraphs, partial failure, all failure, aggregation/summing). That will lock in behavior and prevent regressions in multi-subgraph orchestration.
🤖 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 `@crates/subgraph/src/multi_orderbook_client.rs` around lines 245 - 322, Add unit/integration tests for the new fan-out methods trades_list and trades_count mirroring existing orders/vaults test coverage: create scenarios for (1) no subgraphs configured, (2) partial failures where some subgraphs return errors and others succeed (assert that trades_list returns aggregated successes and trades_count sums successes), (3) all subgraphs failing (assert trades_list returns the last error and trades_count returns an error), and (4) successful aggregation/summing across multiple subgraphs (verify SgTradeWithSubgraphName wrapping for trades_list and correct u32 sum for trades_count); use the same test harness/fixtures and mocks used by the orders/vaults tests to simulate subgraph responses and errors, and reference trades_list, trades_count, and SgTradeWithSubgraphName when locating the code to test.
🤖 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 `@crates/common/src/raindex_client/trades/get_all.rs`:
- Around line 195-254: The code paginates each source independently
(fetch_trades with PaginationParams and client.trades_list with
SgPaginationArgs) then concatenates/sorts, which yields wrong cross-source page
boundaries; change to fetch counts per-source as you do (fetch_trades_count /
client.trades_count) but remove per-source pagination when fetching rows: call
fetch_trades and client.trades_list without page/page_size (or with a combined
limit like page_number * page_size) so you retrieve the full candidate set, then
merge/convert (RaindexTrade::try_from_local_db_trade), sort, and only after
merging apply global pagination using page_number and page_size to slice the
merged all_trades; keep total_count as the sum of per-source counts (total_count
and count_rows logic) and ensure variables referenced (fetch_trades,
fetch_trades_count, client.trades_list, client.trades_count,
get_multi_subgraph_args, total_count, all_trades, page_number, page_size) are
updated accordingly.
- Around line 121-146: Normalize and compare token sets rather than raw vectors:
before checking tokens.inputs == tokens.outputs, create normalized (sorted and
deduplicated) versions of tokens.inputs and tokens.outputs, compare those
normalized lists, and use the normalized list(s) when constructing the OR branch
filters (the SgTradeVaultTokenFilter token_in) so the equality check is order-
and duplicate-insensitive; leave the existing per-branch
token_filter(tokens.inputs) / token_filter(tokens.outputs) behavior for the
single-side cases.
In `@crates/subgraph/src/orderbook_client/order_trade.rs`:
- Around line 119-134: Add a rustdoc comment to the public function trades_list
describing its purpose, parameters and return value; document that it takes
SgTradesListQueryFilters and SgPaginationArgs, calls parse_pagination_args and
the GraphQL query via query::<SgTradesListQuery, SgTradesListQueryVariables>,
and returns Result<Vec<SgTrade>, OrderbookSubgraphClientError> (including any
error behavior and pagination semantics) so consumers understand usage.
- Around line 164-169: Add a rustdoc comment for the public async method
trades_count that explains what it does (returns the total number of trades
matching the provided SgTradesListQueryFilters), its parameters (filters:
SgTradesListQueryFilters), its return value (Result<u32,
OrderbookSubgraphClientError>), and any error conditions (propagates errors from
fetch_all_trades_pages). Place the doc comment directly above the pub async fn
trades_count declaration and mention that it internally calls
fetch_all_trades_pages to compute the count.
---
Nitpick comments:
In `@crates/subgraph/src/multi_orderbook_client.rs`:
- Around line 245-322: Add unit/integration tests for the new fan-out methods
trades_list and trades_count mirroring existing orders/vaults test coverage:
create scenarios for (1) no subgraphs configured, (2) partial failures where
some subgraphs return errors and others succeed (assert that trades_list returns
aggregated successes and trades_count sums successes), (3) all subgraphs failing
(assert trades_list returns the last error and trades_count returns an error),
and (4) successful aggregation/summing across multiple subgraphs (verify
SgTradeWithSubgraphName wrapping for trades_list and correct u32 sum for
trades_count); use the same test harness/fixtures and mocks used by the
orders/vaults tests to simulate subgraph responses and errors, and reference
trades_list, trades_count, and SgTradeWithSubgraphName when locating the code to
test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3b0626f-5859-432f-a895-559479c9d242
📒 Files selected for processing (12)
crates/common/src/local_db/query/fetch_trades/mod.rscrates/common/src/local_db/query/fetch_trades/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/query/fetch_trades.rscrates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/raindex_client/trades/get_all.rscrates/common/src/raindex_client/trades/mod.rscrates/subgraph/src/multi_orderbook_client.rscrates/subgraph/src/orderbook_client/mod.rscrates/subgraph/src/orderbook_client/order_trade.rscrates/subgraph/src/types/common.rscrates/subgraph/src/types/order_trade.rs
findolor
force-pushed
the
arda/get-trades-token-filter
branch
from
fa36f79 to
987590f
Compare
findolor
force-pushed
the
arda/get-trades-token-filter
branch
2 times, most recently
from
27e2e58 to
4597cf4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/common/src/raindex_client/trades/get_all.rs`:
- Around line 232-245: The code currently calls client.trades_count(...) when
sg_token_filter.is_none() and then separately calls client.trades_list_all(...),
which can lead to mismatched counts; instead remove the call to
client.trades_count and derive total_count from the actual fetched rows: after
obtaining sg_trades via client.trades_list_all(...).await? compute total_count
+= sg_trades.len() as u64 in the branch where sg_token_filter is None (the
filtered branch already updates total_count from filtered_trades), ensuring you
only count the rows you actually received and avoid the extra trades_count
request.
In `@crates/subgraph/src/multi_orderbook_client.rs`:
- Around line 273-300: Merged trade vectors are returned in subgraph iteration
order, causing unstable ordering; after collecting items into all_trades in the
method in multi_orderbook_client (the block that builds results, sets
any_success, last_error and returns Ok(all_trades)), sort all_trades using the
same comparator used by orders_list (i.e., the same fields and ordering
direction used there) before returning so the combined list is deterministically
ordered; apply the same change to the other analogous merge block referenced
(lines ~331-358) so both merge points perform the same final sort.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5bde6ee0-59e3-4aed-8442-8901808c3034
📒 Files selected for processing (12)
crates/common/src/local_db/query/fetch_trades/mod.rscrates/common/src/local_db/query/fetch_trades/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/query/fetch_trades.rscrates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/raindex_client/trades/get_all.rscrates/common/src/raindex_client/trades/mod.rscrates/subgraph/src/multi_orderbook_client.rscrates/subgraph/src/orderbook_client/mod.rscrates/subgraph/src/orderbook_client/order_trade.rscrates/subgraph/src/types/common.rscrates/subgraph/src/types/order_trade.rs
✅ Files skipped from review due to trivial changes (3)
- crates/subgraph/src/orderbook_client/mod.rs
- crates/common/src/local_db/query/mod.rs
- crates/common/src/raindex_client/local_db/query/mod.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/common/src/raindex_client/trades/mod.rs
- crates/subgraph/src/types/order_trade.rs
- crates/common/src/raindex_client/local_db/query/fetch_trades.rs
- crates/subgraph/src/orderbook_client/order_trade.rs
- crates/subgraph/src/types/common.rs
- crates/common/src/local_db/query/fetch_trades/query.sql
- crates/common/src/local_db/query/fetch_trades/mod.rs
findolor
force-pushed
the
arda/get-trades-token-filter
branch
6 times, most recently
from
75abd5e to
bc7e6a8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/common/src/raindex_client/trades/get_all.rs (1)
197-222:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't paginate the local DB before the cross-source merge.
When subgraphs are involved, this still fetches only one local DB page and then paginates again after concatenating with the full subgraph result. Any local trades that fall outside that first local slice never make it into the merged candidate set, so combined pages and boundaries are wrong once both sources have data. Fetch the local side unpaginated, or at least up to
page_number * page_size, wheneversg_idsis non-empty and paginate only after the merged sort.Also applies to: 352-359
🤖 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 `@crates/common/src/raindex_client/trades/get_all.rs` around lines 197 - 222, The local DB branch currently applies PaginationParams (local_pagination) before merging with subgraph results, causing missing local trades when sg_ids is non-empty; change the logic in the local_db block that builds FetchTradesArgs for fetch_trades so that when sg_ids is non-empty you pass no pagination (or set page_size = page_number * page_size to fetch the full candidate window) instead of the current local_pagination, and only apply pagination after concatenating and sorting merged results; update the local_query_paginated flag usage accordingly (references: local_db, local_pagination, local_query_paginated, fetch_trades, FetchTradesArgs, sg_ids).
🤖 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 `@crates/common/src/local_db/query/fetch_trades/query.sql`:
- Around line 278-279: The pagination instability comes from identical key
fields for the two sides in matching_clears (clear_alice vs clear_bob) so
trade_id and ORDER BY are not unique or stable; fix by propagating a side
discriminator (e.g., 'side' with values 'alice'/'bob') or carry order_hash
through the CTE chain and include that discriminator/order_hash in both the
trade_id computation and the ORDER BY clauses (instead of relying only on
transaction_hash/log_index), updating the matching_clears CTE and any downstream
CTEs that compute trade_id and the final ORDER BY to use the new field to
produce stable, unique IDs and deterministic sort order.
In `@crates/common/src/raindex_client/trades/get_all.rs`:
- Around line 27-40: GetTradesFilters currently requires owners which breaks
deserialization of partial filter objects; update the struct so the owners field
is optional by adding #[serde(default)] and #[tsify(optional, type =
"Address[]")] to the owners field (the GetTradesFilters struct and its owners
field) so omitted owners deserialize to an empty Vec or undefined on the
TypeScript side and partial filters like tokens-only or orderbook-only succeed.
---
Duplicate comments:
In `@crates/common/src/raindex_client/trades/get_all.rs`:
- Around line 197-222: The local DB branch currently applies PaginationParams
(local_pagination) before merging with subgraph results, causing missing local
trades when sg_ids is non-empty; change the logic in the local_db block that
builds FetchTradesArgs for fetch_trades so that when sg_ids is non-empty you
pass no pagination (or set page_size = page_number * page_size to fetch the full
candidate window) instead of the current local_pagination, and only apply
pagination after concatenating and sorting merged results; update the
local_query_paginated flag usage accordingly (references: local_db,
local_pagination, local_query_paginated, fetch_trades, FetchTradesArgs, sg_ids).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6182478a-f3ed-4756-8ae5-12271c11c7e9
📒 Files selected for processing (13)
crates/common/src/local_db/query/fetch_trades/count_query.sqlcrates/common/src/local_db/query/fetch_trades/mod.rscrates/common/src/local_db/query/fetch_trades/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/query/fetch_trades.rscrates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/raindex_client/trades/get_all.rscrates/common/src/raindex_client/trades/mod.rscrates/subgraph/src/multi_orderbook_client.rscrates/subgraph/src/orderbook_client/mod.rscrates/subgraph/src/orderbook_client/order_trade.rscrates/subgraph/src/types/common.rscrates/subgraph/src/types/order_trade.rs
✅ Files skipped from review due to trivial changes (3)
- crates/common/src/local_db/query/mod.rs
- crates/subgraph/src/orderbook_client/mod.rs
- crates/common/src/raindex_client/local_db/query/mod.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/common/src/raindex_client/local_db/query/fetch_trades.rs
- crates/common/src/raindex_client/trades/mod.rs
- crates/subgraph/src/orderbook_client/order_trade.rs
- crates/subgraph/src/multi_orderbook_client.rs
- crates/subgraph/src/types/common.rs
- crates/subgraph/src/types/order_trade.rs
findolor
force-pushed
the
arda/get-trades-token-filter
branch
from
bc7e6a8 to
5552903
Compare
findolor
force-pushed
the
arda/get-trades-token-filter
branch
from
5552903 to
380a687
Compare
4 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/common/src/utils/timing.rs (1)
27-34: ⚡ Quick winUse
performance.now()instead ofDate::now()for monotonic elapsed timing on WASM.Line 29 uses
Date::now()(wall clock), which can jump backward if the system clock is adjusted, causing elapsed timing to be inaccurate or negative. For measuring elapsed duration, useperformance.now()via web-sys, which is a high-resolution monotonic clock unaffected by system time adjustments:web_sys::window() .and_then(|w| w.performance()) .map(|p| p.now())Apply the same fix to line 17 where the initial timestamp is captured.
🤖 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 `@crates/common/src/utils/timing.rs` around lines 27 - 34, Replace use of Date::now() with the browser performance API so elapsed timing is monotonic: when capturing the start timestamp in the constructor/initializer (the place setting self.started_at_ms) and when computing elapsed in the WASM branch inside the elapsed method, call web_sys::window().and_then(|w| w.performance()).map(|p| p.now()) and fall back to 0.0 or previous behavior if performance is unavailable; update the types/casts so started_at_ms and the elapsed computation use the performance.now() value consistently (e.g., f64 stored and converted to u64 when returning) in the functions/methods that set started_at_ms and compute elapsed.
🤖 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 `@crates/common/src/local_db/query/fetch_trades/query.sql`:
- Around line 379-380: The ORDER BY clause in the query (the clause starting
with "ORDER BY tws.block_timestamp DESC, tws.block_number DESC, tws.log_index
DESC, tws.trade_kind, tws.trade_side") is not total across chains; add
tws.chain_id as an additional tiebreaker (e.g., after trade_side) to ensure
deterministic pagination across multiple chain_ids and optionally append a final
unique key (such as tws.tx_hash or a primary id column) as the last tie-breaker
to guarantee uniqueness.
---
Nitpick comments:
In `@crates/common/src/utils/timing.rs`:
- Around line 27-34: Replace use of Date::now() with the browser performance API
so elapsed timing is monotonic: when capturing the start timestamp in the
constructor/initializer (the place setting self.started_at_ms) and when
computing elapsed in the WASM branch inside the elapsed method, call
web_sys::window().and_then(|w| w.performance()).map(|p| p.now()) and fall back
to 0.0 or previous behavior if performance is unavailable; update the
types/casts so started_at_ms and the elapsed computation use the
performance.now() value consistently (e.g., f64 stored and converted to u64 when
returning) in the functions/methods that set started_at_ms and compute elapsed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fbcd19a-8213-4afb-b7ef-c03e399e9457
📒 Files selected for processing (14)
crates/common/src/local_db/query/fetch_trades/mod.rscrates/common/src/local_db/query/fetch_trades/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/query/fetch_trades.rscrates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/raindex_client/trades/get_all.rscrates/common/src/raindex_client/trades/mod.rscrates/common/src/utils/mod.rscrates/common/src/utils/timing.rscrates/subgraph/src/multi_orderbook_client.rscrates/subgraph/src/orderbook_client/mod.rscrates/subgraph/src/orderbook_client/order_trade.rscrates/subgraph/src/types/common.rscrates/subgraph/src/types/order_trade.rs
✅ Files skipped from review due to trivial changes (4)
- crates/common/src/local_db/query/mod.rs
- crates/subgraph/src/orderbook_client/mod.rs
- crates/common/src/raindex_client/local_db/query/mod.rs
- crates/common/src/raindex_client/trades/mod.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/subgraph/src/types/order_trade.rs
- crates/subgraph/src/types/common.rs
- crates/subgraph/src/multi_orderbook_client.rs
- crates/common/src/raindex_client/local_db/query/fetch_trades.rs
- crates/common/src/raindex_client/trades/get_all.rs
- crates/subgraph/src/orderbook_client/order_trade.rs
Comment on lines
+379
to
+380
| ORDER BY tws.block_timestamp DESC, tws.block_number DESC, tws.log_index DESC, tws.trade_kind, tws.trade_side | ||
| /*PAGINATION_CLAUSE*/; |
Contributor
There was a problem hiding this comment.
Make the pagination sort key total across chains.
Line 379 is still unstable for multi-chain queries: two trades from different chain_ids can share block_timestamp, block_number, log_index, trade_kind, and trade_side, so LIMIT/OFFSET pagination can skip or repeat rows between pages. Add tws.chain_id as a tiebreaker here (and keep a final unique key if you want extra safety).
Suggested fix
-ORDER BY tws.block_timestamp DESC, tws.block_number DESC, tws.log_index DESC, tws.trade_kind, tws.trade_side
+ORDER BY
+ tws.block_timestamp DESC,
+ tws.block_number DESC,
+ tws.log_index DESC,
+ tws.trade_kind,
+ tws.trade_side,
+ tws.chain_id DESC📝 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
| ORDER BY tws.block_timestamp DESC, tws.block_number DESC, tws.log_index DESC, tws.trade_kind, tws.trade_side | |
| /*PAGINATION_CLAUSE*/; | |
| ORDER BY | |
| tws.block_timestamp DESC, | |
| tws.block_number DESC, | |
| tws.log_index DESC, | |
| tws.trade_kind, | |
| tws.trade_side, | |
| tws.chain_id DESC | |
| /*PAGINATION_CLAUSE*/; |
🤖 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 `@crates/common/src/local_db/query/fetch_trades/query.sql` around lines 379 -
380, The ORDER BY clause in the query (the clause starting with "ORDER BY
tws.block_timestamp DESC, tws.block_number DESC, tws.log_index DESC,
tws.trade_kind, tws.trade_side") is not total across chains; add tws.chain_id as
an additional tiebreaker (e.g., after trade_side) to ensure deterministic
pagination across multiple chain_ids and optionally append a final unique key
(such as tws.tx_hash or a primary id column) as the last tie-breaker to
guarantee uniqueness.
4 tasks
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+197
to
+359
| if let Some(db) = local_db { | ||
| let local_pagination = PaginationParams { | ||
| page: Some(page_number), | ||
| page_size: Some(page_size), | ||
| }; | ||
| local_query_paginated = true; | ||
| let local_tokens = filters | ||
| .tokens | ||
| .clone() | ||
| .map(|tokens| FetchTradesTokensFilter { | ||
| inputs: tokens.inputs.unwrap_or_default(), | ||
| outputs: tokens.outputs.unwrap_or_default(), | ||
| }) | ||
| .unwrap_or_default(); | ||
| let local_fetch_started = Timing::now(); | ||
| let local_trades = match fetch_trades( | ||
| &db, | ||
| FetchTradesArgs { | ||
| chain_ids: local_ids.clone(), | ||
| orderbook_addresses: filters.orderbook_addresses.clone().unwrap_or_default(), | ||
| owners: filters.owners.clone(), | ||
| order_hash: filters.order_hash, | ||
| tokens: local_tokens.clone(), | ||
| time_filter: filters.time_filter.clone().unwrap_or_default(), | ||
| pagination: local_pagination, | ||
| }, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(trades) => trades, | ||
| Err(err) => { | ||
| tracing::error!( | ||
| elapsed_ms = local_fetch_started.elapsed_ms(), | ||
| local_chain_ids_count, | ||
| error = %err, | ||
| "getTrades local DB fetch failed" | ||
| ); | ||
| return Err(err.into()); | ||
| } | ||
| }; | ||
| local_fetch_ms = Some(local_fetch_started.elapsed_ms()); | ||
| local_rows = local_trades.len(); | ||
|
|
||
| let local_count_started = Timing::now(); | ||
| let count_rows = match fetch_trades_count( | ||
| &db, | ||
| FetchTradesArgs { | ||
| chain_ids: local_ids, | ||
| orderbook_addresses: filters.orderbook_addresses.clone().unwrap_or_default(), | ||
| owners: filters.owners.clone(), | ||
| order_hash: filters.order_hash, | ||
| tokens: local_tokens, | ||
| time_filter: filters.time_filter.clone().unwrap_or_default(), | ||
| pagination: PaginationParams::default(), | ||
| }, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(count_rows) => count_rows, | ||
| Err(err) => { | ||
| tracing::error!( | ||
| elapsed_ms = local_count_started.elapsed_ms(), | ||
| local_chain_ids_count, | ||
| error = %err, | ||
| "getTrades local DB count failed" | ||
| ); | ||
| return Err(err.into()); | ||
| } | ||
| }; | ||
| local_count_ms = Some(local_count_started.elapsed_ms()); | ||
| local_total_count = extract_trades_count(&count_rows); | ||
| total_count += local_total_count; | ||
| all_trades.extend( | ||
| local_trades | ||
| .into_iter() | ||
| .map(RaindexTrade::try_from_local_db_trade) | ||
| .collect::<Result<Vec<_>, _>>()?, | ||
| ); | ||
| tracing::debug!( | ||
| local_chain_ids_count, | ||
| rows = local_rows, | ||
| total_count = local_total_count, | ||
| fetch_ms = local_fetch_ms, | ||
| count_ms = local_count_ms, | ||
| "getTrades local DB completed" | ||
| ); | ||
| } | ||
|
|
||
| if !sg_ids.is_empty() { | ||
| let multi_subgraph_args = self.get_multi_subgraph_args(Some(sg_ids))?; | ||
| let sg_filters: SgTradesListQueryFilters = filters.clone().into(); | ||
| let sg_token_filter = filters | ||
| .tokens | ||
| .clone() | ||
| .and_then(Option::<SgTradesTokensFilterArgs>::from) | ||
| .map(normalize_trade_tokens); | ||
| let name_to_chain_id: std::collections::HashMap<&str, u32> = multi_subgraph_args | ||
| .iter() | ||
| .flat_map(|(chain_id, args)| args.iter().map(|arg| (arg.name.as_str(), *chain_id))) | ||
| .collect(); | ||
| let client = MultiOrderbookSubgraphClient::new( | ||
| multi_subgraph_args.values().flatten().cloned().collect(), | ||
| ); | ||
| let subgraph_fetch_started = Timing::now(); | ||
| let sg_trades = match client.trades_list_all(sg_filters).await { | ||
| Ok(trades) => trades, | ||
| Err(err) => { | ||
| tracing::error!( | ||
| elapsed_ms = subgraph_fetch_started.elapsed_ms(), | ||
| subgraph_chain_ids_count, | ||
| error = %err, | ||
| "getTrades subgraph fetch failed" | ||
| ); | ||
| return Err(err.into()); | ||
| } | ||
| }; | ||
| subgraph_fetch_ms = Some(subgraph_fetch_started.elapsed_ms()); | ||
| subgraph_rows_before_token_filter = sg_trades.len(); | ||
| let sg_trades: Vec<_> = if let Some(tokens) = sg_token_filter { | ||
| let filtered_trades: Vec<_> = sg_trades | ||
| .into_iter() | ||
| .filter(|trade| sg_trade_matches_token_filter(&trade.trade, &tokens)) | ||
| .collect(); | ||
| total_count += filtered_trades.len() as u64; | ||
| subgraph_rows_after_token_filter = filtered_trades.len(); | ||
| filtered_trades | ||
| } else { | ||
| total_count += sg_trades.len() as u64; | ||
| subgraph_rows_after_token_filter = sg_trades.len(); | ||
| sg_trades | ||
| }; | ||
| for trade_with_name in sg_trades { | ||
| let chain_id = name_to_chain_id | ||
| .get(trade_with_name.subgraph_name.as_str()) | ||
| .copied() | ||
| .ok_or(RaindexError::SubgraphNotFound( | ||
| trade_with_name.subgraph_name.clone(), | ||
| trade_with_name.trade.id.0.clone(), | ||
| ))?; | ||
| all_trades.push(RaindexTrade::try_from_sg_trade( | ||
| chain_id, | ||
| trade_with_name.trade, | ||
| )?); | ||
| } | ||
| tracing::debug!( | ||
| subgraph_chain_ids_count, | ||
| rows_before_token_filter = subgraph_rows_before_token_filter, | ||
| rows_after_token_filter = subgraph_rows_after_token_filter, | ||
| fetch_ms = subgraph_fetch_ms, | ||
| "getTrades subgraph completed" | ||
| ); | ||
| } | ||
|
|
||
| let merge_started = Timing::now(); | ||
| all_trades.sort_by(|a, b| b.timestamp.cmp(&a.timestamp).then_with(|| a.id.cmp(&b.id))); | ||
| let offset = if local_query_paginated && subgraph_chain_ids_count == 0 { | ||
| 0 | ||
| } else { | ||
| ((page_number - 1) as usize) * (page_size as usize) | ||
| }; | ||
| let limit = page_size as usize; | ||
| let merged_rows_before_pagination = all_trades.len(); | ||
| let trades: Vec<_> = all_trades.into_iter().skip(offset).take(limit).collect(); |
There was a problem hiding this comment.
Critical pagination bug when mixing local DB and subgraph results. The local DB query is paginated (lines 198-201) but the subgraph query fetches ALL trades via trades_list_all (line 301). After merging and sorting (line 351), the code applies pagination again (lines 352-359). This causes incorrect results because:
- Local DB returns only page N (e.g., trades 11-20 for page 2)
- Subgraph returns ALL matching trades (e.g., 1-100)
- After merging and sorting, the sort order is corrupted since local DB is missing trades outside its page
- Re-applying pagination to the merged list returns wrong results
Example: If a user requests page 2 with page_size 10, and local DB has 50 trades while subgraph has 30 trades:
- Local DB returns trades 11-20 (missing 1-10, 21-50)
- Subgraph returns all 30 trades
- Merged list has 40 trades, incorrectly sorted since local DB trades 1-10 and 21-50 are missing
- Final pagination skips 10 and takes 10, returning wrong trades
Fix: Either (1) fetch ALL trades from local DB when subgraph is involved, or (2) only query subgraph if local DB is not being used. The current approach of mixing pre-paginated and unpaginated results before sorting is fundamentally broken.
Suggested change
| if let Some(db) = local_db { | |
| let local_pagination = PaginationParams { | |
| page: Some(page_number), | |
| page_size: Some(page_size), | |
| }; | |
| local_query_paginated = true; | |
| let local_tokens = filters | |
| .tokens | |
| .clone() | |
| .map(|tokens| FetchTradesTokensFilter { | |
| inputs: tokens.inputs.unwrap_or_default(), | |
| outputs: tokens.outputs.unwrap_or_default(), | |
| }) | |
| .unwrap_or_default(); | |
| let local_fetch_started = Timing::now(); | |
| let local_trades = match fetch_trades( | |
| &db, | |
| FetchTradesArgs { | |
| chain_ids: local_ids.clone(), | |
| orderbook_addresses: filters.orderbook_addresses.clone().unwrap_or_default(), | |
| owners: filters.owners.clone(), | |
| order_hash: filters.order_hash, | |
| tokens: local_tokens.clone(), | |
| time_filter: filters.time_filter.clone().unwrap_or_default(), | |
| pagination: local_pagination, | |
| }, | |
| ) | |
| .await | |
| { | |
| Ok(trades) => trades, | |
| Err(err) => { | |
| tracing::error!( | |
| elapsed_ms = local_fetch_started.elapsed_ms(), | |
| local_chain_ids_count, | |
| error = %err, | |
| "getTrades local DB fetch failed" | |
| ); | |
| return Err(err.into()); | |
| } | |
| }; | |
| local_fetch_ms = Some(local_fetch_started.elapsed_ms()); | |
| local_rows = local_trades.len(); | |
| let local_count_started = Timing::now(); | |
| let count_rows = match fetch_trades_count( | |
| &db, | |
| FetchTradesArgs { | |
| chain_ids: local_ids, | |
| orderbook_addresses: filters.orderbook_addresses.clone().unwrap_or_default(), | |
| owners: filters.owners.clone(), | |
| order_hash: filters.order_hash, | |
| tokens: local_tokens, | |
| time_filter: filters.time_filter.clone().unwrap_or_default(), | |
| pagination: PaginationParams::default(), | |
| }, | |
| ) | |
| .await | |
| { | |
| Ok(count_rows) => count_rows, | |
| Err(err) => { | |
| tracing::error!( | |
| elapsed_ms = local_count_started.elapsed_ms(), | |
| local_chain_ids_count, | |
| error = %err, | |
| "getTrades local DB count failed" | |
| ); | |
| return Err(err.into()); | |
| } | |
| }; | |
| local_count_ms = Some(local_count_started.elapsed_ms()); | |
| local_total_count = extract_trades_count(&count_rows); | |
| total_count += local_total_count; | |
| all_trades.extend( | |
| local_trades | |
| .into_iter() | |
| .map(RaindexTrade::try_from_local_db_trade) | |
| .collect::<Result<Vec<_>, _>>()?, | |
| ); | |
| tracing::debug!( | |
| local_chain_ids_count, | |
| rows = local_rows, | |
| total_count = local_total_count, | |
| fetch_ms = local_fetch_ms, | |
| count_ms = local_count_ms, | |
| "getTrades local DB completed" | |
| ); | |
| } | |
| if !sg_ids.is_empty() { | |
| let multi_subgraph_args = self.get_multi_subgraph_args(Some(sg_ids))?; | |
| let sg_filters: SgTradesListQueryFilters = filters.clone().into(); | |
| let sg_token_filter = filters | |
| .tokens | |
| .clone() | |
| .and_then(Option::<SgTradesTokensFilterArgs>::from) | |
| .map(normalize_trade_tokens); | |
| let name_to_chain_id: std::collections::HashMap<&str, u32> = multi_subgraph_args | |
| .iter() | |
| .flat_map(|(chain_id, args)| args.iter().map(|arg| (arg.name.as_str(), *chain_id))) | |
| .collect(); | |
| let client = MultiOrderbookSubgraphClient::new( | |
| multi_subgraph_args.values().flatten().cloned().collect(), | |
| ); | |
| let subgraph_fetch_started = Timing::now(); | |
| let sg_trades = match client.trades_list_all(sg_filters).await { | |
| Ok(trades) => trades, | |
| Err(err) => { | |
| tracing::error!( | |
| elapsed_ms = subgraph_fetch_started.elapsed_ms(), | |
| subgraph_chain_ids_count, | |
| error = %err, | |
| "getTrades subgraph fetch failed" | |
| ); | |
| return Err(err.into()); | |
| } | |
| }; | |
| subgraph_fetch_ms = Some(subgraph_fetch_started.elapsed_ms()); | |
| subgraph_rows_before_token_filter = sg_trades.len(); | |
| let sg_trades: Vec<_> = if let Some(tokens) = sg_token_filter { | |
| let filtered_trades: Vec<_> = sg_trades | |
| .into_iter() | |
| .filter(|trade| sg_trade_matches_token_filter(&trade.trade, &tokens)) | |
| .collect(); | |
| total_count += filtered_trades.len() as u64; | |
| subgraph_rows_after_token_filter = filtered_trades.len(); | |
| filtered_trades | |
| } else { | |
| total_count += sg_trades.len() as u64; | |
| subgraph_rows_after_token_filter = sg_trades.len(); | |
| sg_trades | |
| }; | |
| for trade_with_name in sg_trades { | |
| let chain_id = name_to_chain_id | |
| .get(trade_with_name.subgraph_name.as_str()) | |
| .copied() | |
| .ok_or(RaindexError::SubgraphNotFound( | |
| trade_with_name.subgraph_name.clone(), | |
| trade_with_name.trade.id.0.clone(), | |
| ))?; | |
| all_trades.push(RaindexTrade::try_from_sg_trade( | |
| chain_id, | |
| trade_with_name.trade, | |
| )?); | |
| } | |
| tracing::debug!( | |
| subgraph_chain_ids_count, | |
| rows_before_token_filter = subgraph_rows_before_token_filter, | |
| rows_after_token_filter = subgraph_rows_after_token_filter, | |
| fetch_ms = subgraph_fetch_ms, | |
| "getTrades subgraph completed" | |
| ); | |
| } | |
| let merge_started = Timing::now(); | |
| all_trades.sort_by(|a, b| b.timestamp.cmp(&a.timestamp).then_with(|| a.id.cmp(&b.id))); | |
| let offset = if local_query_paginated && subgraph_chain_ids_count == 0 { | |
| 0 | |
| } else { | |
| ((page_number - 1) as usize) * (page_size as usize) | |
| }; | |
| let limit = page_size as usize; | |
| let merged_rows_before_pagination = all_trades.len(); | |
| let trades: Vec<_> = all_trades.into_iter().skip(offset).take(limit).collect(); | |
| if let Some(db) = local_db { | |
| // Only paginate the local DB query if we are NOT also querying subgraphs. | |
| // When subgraph results are involved, we need all local DB trades so that | |
| // the merged sort + pagination step produces correct results. | |
| let local_pagination = if sg_ids.is_empty() { | |
| local_query_paginated = true; | |
| PaginationParams { | |
| page: Some(page_number), | |
| page_size: Some(page_size), | |
| } | |
| } else { | |
| PaginationParams::default() | |
| }; | |
| let local_tokens = filters | |
| .tokens | |
| .clone() | |
| .map(|tokens| FetchTradesTokensFilter { | |
| inputs: tokens.inputs.unwrap_or_default(), | |
| outputs: tokens.outputs.unwrap_or_default(), | |
| }) | |
| .unwrap_or_default(); | |
| let local_fetch_started = Timing::now(); | |
| let local_trades = match fetch_trades( | |
| &db, | |
| FetchTradesArgs { | |
| chain_ids: local_ids.clone(), | |
| orderbook_addresses: filters.orderbook_addresses.clone().unwrap_or_default(), | |
| owners: filters.owners.clone(), | |
| order_hash: filters.order_hash, | |
| tokens: local_tokens.clone(), | |
| time_filter: filters.time_filter.clone().unwrap_or_default(), | |
| pagination: local_pagination, | |
| }, | |
| ) | |
| .await | |
| { | |
| Ok(trades) => trades, | |
| Err(err) => { | |
| tracing::error!( | |
| elapsed_ms = local_fetch_started.elapsed_ms(), | |
| local_chain_ids_count, | |
| error = %err, | |
| "getTrades local DB fetch failed" | |
| ); | |
| return Err(err.into()); | |
| } | |
| }; | |
| local_fetch_ms = Some(local_fetch_started.elapsed_ms()); | |
| local_rows = local_trades.len(); | |
| let local_count_started = Timing::now(); | |
| let count_rows = match fetch_trades_count( | |
| &db, | |
| FetchTradesArgs { | |
| chain_ids: local_ids, | |
| orderbook_addresses: filters.orderbook_addresses.clone().unwrap_or_default(), | |
| owners: filters.owners.clone(), | |
| order_hash: filters.order_hash, | |
| tokens: local_tokens, | |
| time_filter: filters.time_filter.clone().unwrap_or_default(), | |
| pagination: PaginationParams::default(), | |
| }, | |
| ) | |
| .await | |
| { | |
| Ok(count_rows) => count_rows, | |
| Err(err) => { | |
| tracing::error!( | |
| elapsed_ms = local_count_started.elapsed_ms(), | |
| local_chain_ids_count, | |
| error = %err, | |
| "getTrades local DB count failed" | |
| ); | |
| return Err(err.into()); | |
| } | |
| }; | |
| local_count_ms = Some(local_count_started.elapsed_ms()); | |
| local_total_count = extract_trades_count(&count_rows); | |
| total_count += local_total_count; | |
| all_trades.extend( | |
| local_trades | |
| .into_iter() | |
| .map(RaindexTrade::try_from_local_db_trade) | |
| .collect::<Result<Vec<_>, _>>()?, | |
| ); | |
| tracing::debug!( | |
| local_chain_ids_count, | |
| rows = local_rows, | |
| total_count = local_total_count, | |
| fetch_ms = local_fetch_ms, | |
| count_ms = local_count_ms, | |
| "getTrades local DB completed" | |
| ); | |
| } | |
| if !sg_ids.is_empty() { | |
| let multi_subgraph_args = self.get_multi_subgraph_args(Some(sg_ids))?; | |
| let sg_filters: SgTradesListQueryFilters = filters.clone().into(); | |
| let sg_token_filter = filters | |
| .tokens | |
| .clone() | |
| .and_then(Option::<SgTradesTokensFilterArgs>::from) | |
| .map(normalize_trade_tokens); | |
| let name_to_chain_id: std::collections::HashMap<&str, u32> = multi_subgraph_args | |
| .iter() | |
| .flat_map(|(chain_id, args)| args.iter().map(|arg| (arg.name.as_str(), *chain_id))) | |
| .collect(); | |
| let client = MultiOrderbookSubgraphClient::new( | |
| multi_subgraph_args.values().flatten().cloned().collect(), | |
| ); | |
| let subgraph_fetch_started = Timing::now(); | |
| let sg_trades = match client.trades_list_all(sg_filters).await { | |
| Ok(trades) => trades, | |
| Err(err) => { | |
| tracing::error!( | |
| elapsed_ms = subgraph_fetch_started.elapsed_ms(), | |
| subgraph_chain_ids_count, | |
| error = %err, | |
| "getTrades subgraph fetch failed" | |
| ); | |
| return Err(err.into()); | |
| } | |
| }; | |
| subgraph_fetch_ms = Some(subgraph_fetch_started.elapsed_ms()); | |
| subgraph_rows_before_token_filter = sg_trades.len(); | |
| let sg_trades: Vec<_> = if let Some(tokens) = sg_token_filter { | |
| let filtered_trades: Vec<_> = sg_trades | |
| .into_iter() | |
| .filter(|trade| sg_trade_matches_token_filter(&trade.trade, &tokens)) | |
| .collect(); | |
| total_count += filtered_trades.len() as u64; | |
| subgraph_rows_after_token_filter = filtered_trades.len(); | |
| filtered_trades | |
| } else { | |
| total_count += sg_trades.len() as u64; | |
| subgraph_rows_after_token_filter = sg_trades.len(); | |
| sg_trades | |
| }; | |
| for trade_with_name in sg_trades { | |
| let chain_id = name_to_chain_id | |
| .get(trade_with_name.subgraph_name.as_str()) | |
| .copied() | |
| .ok_or(RaindexError::SubgraphNotFound( | |
| trade_with_name.subgraph_name.clone(), | |
| trade_with_name.trade.id.0.clone(), | |
| ))?; | |
| all_trades.push(RaindexTrade::try_from_sg_trade( | |
| chain_id, | |
| trade_with_name.trade, | |
| )?); | |
| } | |
| tracing::debug!( | |
| subgraph_chain_ids_count, | |
| rows_before_token_filter = subgraph_rows_before_token_filter, | |
| rows_after_token_filter = subgraph_rows_after_token_filter, | |
| fetch_ms = subgraph_fetch_ms, | |
| "getTrades subgraph completed" | |
| ); | |
| } | |
| let merge_started = Timing::now(); | |
| all_trades.sort_by(|a, b| b.timestamp.cmp(&a.timestamp).then_with(|| a.id.cmp(&b.id))); | |
| let offset = if local_query_paginated && subgraph_chain_ids_count == 0 { | |
| 0 | |
| } else { | |
| ((page_number - 1) as usize) * (page_size as usize) | |
| }; | |
| let limit = page_size as usize; | |
| let merged_rows_before_pagination = all_trades.len(); | |
| let trades: Vec<_> = all_trades.into_iter().skip(offset).take(limit).collect(); | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
JuaniRios
approved these changes
May 12, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/common/src/raindex_client/trades/get_all.rs`:
- Around line 104-119: local_trades_pagination currently disables DB pagination
whenever any subgraph source exists, causing trades_list_all to pull full local
history; change it so that when has_subgraph_sources is true it requests only up
to offset+limit rows from each source (i.e., set the PaginationParams page/
page_size to cover page_number * page_size rows per source) instead of
PaginationParams::default(), and retain the unbounded/default behavior only when
post-filtering by token forces a full scan; apply the same change pattern to the
other affected blocks referenced (around the 216-239 and 301-343 ranges) so
mixed-source fetches are bounded unless token post-filtering requires a full
merge scan.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86bcae5d-2552-48a1-8d88-fcd9a222ce8e
📒 Files selected for processing (1)
crates/common/src/raindex_client/trades/get_all.rs
Comment on lines
+104
to
+119
| fn local_trades_pagination( | ||
| has_subgraph_sources: bool, | ||
| page_number: u16, | ||
| page_size: u16, | ||
| ) -> (PaginationParams, bool) { | ||
| if has_subgraph_sources { | ||
| (PaginationParams::default(), false) | ||
| } else { | ||
| ( | ||
| PaginationParams { | ||
| page: Some(page_number), | ||
| page_size: Some(page_size), | ||
| }, | ||
| true, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Bound mixed-source fetches to offset + limit unless post-filtering forces a full scan.
local_trades_pagination() disables DB pagination as soon as any subgraph source exists, and the subgraph path always calls trades_list_all(). That makes even page 1 load the full local and remote trade history before slicing, which is likely to become a latency/memory problem on large books. For the unfiltered path, fetch only the first page_number * page_size rows from each source, merge those, and keep the full subgraph scan only for the token post-filter case.
Also applies to: 216-239, 301-343
🤖 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 `@crates/common/src/raindex_client/trades/get_all.rs` around lines 104 - 119,
local_trades_pagination currently disables DB pagination whenever any subgraph
source exists, causing trades_list_all to pull full local history; change it so
that when has_subgraph_sources is true it requests only up to offset+limit rows
from each source (i.e., set the PaginationParams page/ page_size to cover
page_number * page_size rows per source) instead of PaginationParams::default(),
and retain the unbounded/default behavior only when post-filtering by token
forces a full scan; apply the same change pattern to the other affected blocks
referenced (around the 216-239 and 301-343 ranges) so mixed-source fetches are
bounded unless token post-filtering requires a full merge scan.
hardyjosh
approved these changes
May 13, 2026
Merge activity
|
## Related Issue - [RAI-522: Add get v1/trades token tokenAddress](https://linear.app/makeitrain/issue/RAI-522/add-get-v1tradestokentokenaddress) ## Motivation Consumers need the SDK-level `getTrades` API to support token filtering, matching the filter shape already available on `getOrders`. The implementation should work across local DB and subgraph sources without changing the existing owner-specific or transaction-specific trade APIs. ## Solution - Add `RaindexClient.getTrades` in `trades/get_all.rs`, keeping `trades/mod.rs` limited to module wiring and exports. - Add general local DB `fetch_trades` and `fetch_trades_count` query wrappers with owner, order hash, orderbook, time, pagination, and input/output token filters. - Add a separate subgraph `trades_list` query and filter types instead of repurposing owner or transaction trade queries. - Add multi-subgraph `trades_list` and `trades_count` helpers, with all-subgraph-failure behavior aligned with existing list APIs. - Preserve the existing `getTradesForOwner` and `getTradesByTx` paths. - Add focused tests for local token SQL generation, count query generation, and subgraph filter mapping. ## 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) Additional validation run: - `cargo fmt --all` - `git diff --check` - `nix develop -c cargo check -p rain_orderbook_common` - `nix develop -c cargo test -p rain_orderbook_subgraph_client test_trades_by_transaction_no_subgraphs` - `nix develop -c cargo test -p rain_orderbook_subgraph_client test_trades_by_transaction_with_orderbook_filter` Note: compiling `rain_orderbook_common` tests is currently blocked by missing Foundry artifact JSON files under `lib/rain.interpreter/out/...`; this appears unrelated to this PR. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Comprehensive trades query API with owners, order-hash, time-range filters * Directional and combined token filtering with deterministic matching semantics * Pagination and trade count queries; aggregated results from local DB and subgraph sources * WASM-exported client method to fetch merged, paginated trades * Cross-platform timing utility for elapsed-time measurement * **Tests** * Unit and integration tests covering ordering, token filters, pagination, counts, and multi-subgraph merging <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/rainlanguage/raindex/pull/2570) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/rainlanguage/raindex/pull/2570){kind=link}
graphite-app
Bot
force-pushed
the
arda/get-trades-token-filter
branch
from
0c4d8ec to
1b82157
Compare
graphite-app Bot
pushed a commit
that referenced
this pull request
May 13, 2026
## Related Issue - [RAI-529: Add taker address filters to raindex getTrades SDK](https://linear.app/makeitrain/issue/RAI-529/add-taker-address-filters-to-raindex-gettrades-sdk) ## Dependent PRs - Depends on #2570 ## Motivation Consumers need the SDK-level `getTrades` API to support filtering by taker address. The REST API work identifies the taker address as the trade event sender, which is available in the raindex local DB and subgraph trade models. For the local DB path, the taker predicate also needs to be applied before the trade reconstruction CTEs expand and hydrate rows. Filtering only after the normalized trade result forces SQLite to build a much larger intermediate result set before returning the requested page. ## Solution - Add `takers` to the existing `GetTradesFilters` SDK filter shape. - Map takers into local `FetchTradesArgs` and push the SQL predicate into the source CTEs: `take_orders.sender IN (...)` and `clear_v3_events.sender IN (...)`. - Add sender/time indexes for `take_orders` and `clear_v3_events` to support the new taker lookup path. - Add local DB `getTrades` fetch/count tracing with filter counts, parameter count, row/count results, and duration. - Add a subgraph `TradeEvent_filter` binding and map takers to `tradeEvent_: { sender_in: [...] }`. - Preserve the existing `getTrades` pagination, time, owner, order hash, orderbook, and token filter behavior. - Add focused tests for local SQL generation and subgraph filter mapping. ## Checks By submitting this for review, I confirm I have 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) Additional validation run: - `cargo fmt --all` - `nix develop -c cargo test -p rain_orderbook_common fetch_trades` - `nix develop -c cargo test -p rain_orderbook_common -p rain_orderbook_subgraph_client` - `nix develop -c cargo test --workspace`
Contributor
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=L |
graphite-app Bot
pushed a commit
to ST0x-Technology/st0x.rest.api
that referenced
this pull request
May 13, 2026
## Dependent PRs - Depends on [rainlanguage/raindex#2570](rainlanguage/raindex#2570) for SDK `getTrades` token filtering support. This PR should not merge until that raindex PR lands and the submodule pointer is mergeable. ## Summary - Adds `GET /v1/trades/token/{address}` for paginated trades involving a token address. - Wires the new route through the existing `TradesDataSource` abstraction and the raindex SDK `get_trades` token filters. - Reuses the shared trades list response shape used by owner-based trades, including pagination metadata and time filtering. - Refactors the owner trades route to share pagination and trade mapping helpers with the new token route. - Updates trade test mocks for the expanded `TradesDataSource` trait. - Updates order type detection for the raindex parsed meta variant rename from `DotrainGuiStateV1` to `OrderBuilderStateV1`. - Bumps the `rain.orderbook` submodule to the SDK commit with token-filtered `getTrades` support. Linear: RAI-526 ## Behavior The new endpoint accepts the same pagination/time query parameters as `GET /v1/trades/{address}`: ```text GET /v1/trades/token/{tokenAddress}?page=1&pageSize=20&startTime=...&endTime=... ``` It returns trades where the token appears on either side of the trade by passing the token as both an input-token and output-token filter to the SDK. ## Notes - The current raindex local DB query path is still the baseline implementation. In local benchmarking, high-volume token queries are around four seconds for `pageSize=50`; materialized trade read tables should be handled as a separate SDK/local DB optimization. - The `unimplemented!()` additions are test-only mock methods required by the expanded `TradesDataSource` trait; they are not production route code. ## Verification - `nix develop -c cargo check` - `nix develop -c cargo test` - `nix develop -c cargo fmt` - `nix develop -c rainix-rs-static` `rainix-rs-static` passes with pre-existing dead-code warnings in `src/cache.rs`.
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.
Related Issue
Motivation
Consumers need the SDK-level
getTradesAPI to support token filtering, matching the filter shape already available ongetOrders. The implementation should work across local DB and subgraph sources without changing the existing owner-specific or transaction-specific trade APIs.Solution
RaindexClient.getTradesintrades/get_all.rs, keepingtrades/mod.rslimited to module wiring and exports.fetch_tradesandfetch_trades_countquery wrappers with owner, order hash, orderbook, time, pagination, and input/output token filters.trades_listquery and filter types instead of repurposing owner or transaction trade queries.trades_listandtrades_counthelpers, with all-subgraph-failure behavior aligned with existing list APIs.getTradesForOwnerandgetTradesByTxpaths.Checks
By submitting this for review, I'm confirming I've done the following:
Additional validation run:
cargo fmt --allgit diff --checknix develop -c cargo check -p rain_orderbook_commonnix develop -c cargo test -p rain_orderbook_subgraph_client test_trades_by_transaction_no_subgraphsnix develop -c cargo test -p rain_orderbook_subgraph_client test_trades_by_transaction_with_orderbook_filterNote: compiling
rain_orderbook_commontests is currently blocked by missing Foundry artifact JSON files underlib/rain.interpreter/out/...; this appears unrelated to this PR.Summary by CodeRabbit
New Features
Tests