feat: add get_trades_for_owner API with local DB and subgraph support#2499
Merged
feat: add get_trades_for_owner API with local DB and subgraph support#2499
Conversation
Contributor
📝 WalkthroughWalkthroughAdds owner-scoped trade querying: new SQL fetch/count templates, shared binder, Rust builders and wrappers, RaindexClient owner/tx orchestration with pagination/time filters, subgraph owner-trades queries and aggregation, module exports, and wasm import tweaks with comprehensive tests. ChangesOwner Trades (single cohort)
Sequence Diagram(s)sequenceDiagram
participant Client
participant RaindexClient
participant SQLBuilder
participant LocalDB
participant MultiSG
Client->>RaindexClient: get_trades_for_owner(owner, ChainIds, TimeFilter, Pagination)
RaindexClient->>RaindexClient: classify chains
RaindexClient->>SQLBuilder: build fetch/count (if paginating)
SQLBuilder-->>RaindexClient: SqlStatement(s)
RaindexClient->>LocalDB: query_json(stmt)
LocalDB-->>RaindexClient: local trades / count
RaindexClient->>MultiSG: trades_by_owner(owner, start/end, orderbook_in)
MultiSG-->>RaindexClient: sg trades
RaindexClient-->>Client: merged trades, total_count, summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
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
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/common/src/local_db/query/fetch_owner_trades/mod.rs`:
- Around line 74-95: The query builder emits SQL even when args.start_timestamp
> args.end_timestamp, causing list/count mismatch; add the same validation used
in build_fetch_owner_trades_count_stmt by checking args.start_timestamp and
args.end_timestamp early in the function (before creating start_param/end_param
and before stmt.bind_param_clause calls) and return a SqlBuildError when start >
end; reference the existing symbols args.start_timestamp, args.end_timestamp,
SqlBuildError, START_TS_CLAUSE, and END_TS_CLAUSE to locate where to insert this
guard.
In `@crates/common/src/local_db/query/fetch_owner_trades/query.sql`:
- Around line 273-281: The CTEs clear_trades, unioned_trades (and
trade_with_snapshots) use passthrough "SELECT *" which makes downstream schemas
depend on upstream column ordering; replace each "SELECT * FROM clear_alice" /
"SELECT * FROM clear_bob" / "SELECT * FROM take_trades" and any other SELECT *
uses (including the later block around trade_with_snapshots) with explicit,
ordered column lists (e.g., list every column name produced by clear_alice,
clear_bob, and take_trades) so that clear_trades, unioned_trades, and
trade_with_snapshots have deterministic schemas and union columns align by name
rather than position.
In `@crates/common/src/raindex_client/trades/get_by_owner.rs`:
- Around line 127-170: The subgraph branch currently fetches all pages via
MultiOrderbookSubgraphClient::trades_by_owner (which delegates to
trades_by_owner_all), so when a page is requested the subgraph side still
returns the full result and final_total_count only reflects the local DB; fix by
calling the paginated subgraph path and including the subgraph-side count in the
final total: when page is set, invoke the paginated method on
MultiOrderbookSubgraphClient (or pass page_number/page_size into
trades_by_owner/trades_by_owner_all so it uses pagination), collect both the
returned trades and the subgraph total count, push the trades into all_trades as
before, and compute final_total_count as total_count.unwrap_or(0) +
subgraph_count (or if total_count is None use subgraph_count or all_trades.len()
as fallback) so final_total_count accounts for both local DB and subgraph
results; update references around get_multi_subgraph_args,
MultiOrderbookSubgraphClient::trades_by_owner / trades_by_owner_all, all_trades,
total_count, and final_total_count accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65c9dcea-6cb7-48cd-a9f0-10456f351325
📒 Files selected for processing (16)
crates/common/src/local_db/query/fetch_owner_trades/mod.rscrates/common/src/local_db/query/fetch_owner_trades/query.sqlcrates/common/src/local_db/query/fetch_owner_trades_count/mod.rscrates/common/src/local_db/query/fetch_owner_trades_count/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/query/fetch_owner_trades.rscrates/common/src/raindex_client/local_db/query/fetch_owner_trades_count.rscrates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/raindex_client/trades/get_by_owner.rscrates/common/src/raindex_client/trades/get_by_tx.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
Comment on lines
+273
to
+281
| clear_trades AS ( | ||
| SELECT * FROM clear_alice | ||
| UNION ALL | ||
| SELECT * FROM clear_bob | ||
| ), | ||
| unioned_trades AS ( | ||
| SELECT * FROM take_trades | ||
| UNION ALL | ||
| SELECT * FROM clear_trades |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace the passthrough SELECT * projections with explicit column lists.
clear_trades, unioned_trades, and trade_with_snapshots all inherit their shape positionally from upstream CTEs. A later column addition upstream can silently reshape the downstream row set.
Based on learnings, "avoid relying on implicit column ordering via SELECT * in CTEs ... explicitly list all source columns in the CTE and in subsequent SELECT projections".
Also applies to: 305-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/common/src/local_db/query/fetch_owner_trades/query.sql` around lines
273 - 281, The CTEs clear_trades, unioned_trades (and trade_with_snapshots) use
passthrough "SELECT *" which makes downstream schemas depend on upstream column
ordering; replace each "SELECT * FROM clear_alice" / "SELECT * FROM clear_bob" /
"SELECT * FROM take_trades" and any other SELECT * uses (including the later
block around trade_with_snapshots) with explicit, ordered column lists (e.g.,
list every column name produced by clear_alice, clear_bob, and take_trades) so
that clear_trades, unioned_trades, and trade_with_snapshots have deterministic
schemas and union columns align by name rather than position.
Collaborator
Author
There was a problem hiding this comment.
All SQL queries in this codebase use the same SELECT * CTE passthrough pattern. Changing just this one would be inconsistent.
Contributor
There was a problem hiding this comment.
@findolor, understood — maintaining consistency with the established pattern across the codebase is a reasonable call. I'll drop this suggestion.
I'd also like to update the existing knowledge base entry (from PR #2465) that advised against SELECT * in CTEs, since it appears that was specific to a narrower case rather than a general rule.
✏️ Learnings added
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2499
File: crates/common/src/local_db/query/fetch_owner_trades/query.sql:273-281
Timestamp: 2026-03-10T11:19:18.334Z
Learning: In `crates/common/src/local_db/query/` SQL files (rain.orderbook), passthrough `SELECT *` projections in intermediate CTEs (e.g., `clear_trades`, `unioned_trades`) are the accepted codebase-wide pattern. Do not flag these as issues; changing only one query would be inconsistent with the rest of the codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
⛔ Learnings removed
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2465
File: crates/common/src/local_db/query/fetch_trades_by_tx/query.sql:236-255
Timestamp: 2026-02-18T08:41:03.365Z
Learning: In SQL queries (across the repository), avoid relying on implicit column ordering via SELECT * in CTEs. When a CTE (e.g., trade_rows) feeds into another query, explicitly list all source columns in the CTE and in subsequent SELECT projections to pin the exact column set, ensuring deterministic schema, easier maintenance, and safer refactors.
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2465
File: crates/common/src/local_db/query/fetch_trades_by_tx/query.sql:236-255
Timestamp: 2026-02-18T08:41:03.365Z
Learning: In SQL queries (across the repository), avoid relying on implicit column ordering via SELECT * in CTEs. When a CTE (e.g., trade_rows) feeds into another query, explicitly list all source columns in the CTE and in subsequent SELECT projections to pin the exact column set, ensuring deterministic schema, easier maintenance, and safer refactors.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2376
File: crates/common/src/raindex_client/orders.rs:2-2
Timestamp: 2026-01-07T14:21:44.132Z
Learning: When local DB is enabled in the rain.orderbook codebase, the intended design is to use it exclusively without falling back to the subgraph, even when results are empty. Methods like get_trades_list() in trades.rs and get_vaults_volume() in orders.rs follow this pattern—they return local DB results directly when is_chain_supported_local_db() and local_db is available. Mixing data sources could cause inconsistencies. The get_balance_changes() method in vaults.rs that checks !local_changes.is_empty() before returning is an outlier that needs to be fixed to align with the exclusive local DB pattern.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2497
File: crates/common/src/raindex_client/local_db/query/fetch_trades_by_tx.rs:27-49
Timestamp: 2026-03-09T11:54:48.469Z
Learning: In `crates/common/src/raindex_client/local_db/query/fetch_trades_by_tx.rs`, the WASM unit test `wrapper_uses_builder_sql_exactly` intentionally returns an empty `[]` from the callback to focus exclusively on validating that the wrapper forwards the SQL built by `build_fetch_trades_by_tx_stmt`. The deserialization path (including new fields like `chain_id` on `LocalDbOrderTrade`) is intentionally covered by separate WASM and integration tests, not this unit test. This is by design and should not be flagged as a coverage gap.
Learnt from: hardyjosh
Repo: rainlanguage/rain.orderbook PR: 2167
File: crates/virtual-raindex/src/engine/take.rs:131-138
Timestamp: 2025-09-30T21:18:01.636Z
Learning: In the virtual-raindex take order flow (crates/virtual-raindex/src/engine/take.rs), balance diffs are written from the order's perspective where the taker is the counterparty: the order's input column receives taker_output (what the taker provides to the order) and the order's output column receives taker_input (what the taker requests from the order).
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/raindex_client/trades.rs:215-223
Timestamp: 2025-07-04T10:24:56.163Z
Learning: In the rain.orderbook codebase, the get_trade_count method implementation that fetches all trades to count them is intentionally consistent with previous implementations and not considered a performance issue, as indicated by findolor for the trades counting functionality in crates/common/src/raindex_client/trades.rs.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2442
File: crates/common/src/local_db/query/fetch_vaults/mod.rs:228-244
Timestamp: 2026-02-09T12:31:02.217Z
Learning: In crates/common/src/local_db/query/fetch_vaults/mod.rs tests, whitespace-sensitive SQL string assertions (e.g., checking for "GROUP BY chain_id, orderbook_address, owner, token, vault_id\n") are acceptable for verifying query structure until a mock database testing approach is implemented. These assertions help ensure the SQL template is correctly formed even though they may be fragile to formatting changes.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2155
File: crates/common/src/raindex_client/trades.rs:133-152
Timestamp: 2025-10-06T14:13:18.531Z
Learning: In the rain.orderbook codebase, the `page` parameter in `RaindexOrder::get_trades_list` method (in crates/common/src/raindex_client/trades.rs) is kept for backwards compatibility with subgraph logic, but the LocalDb fast-path intentionally returns all trades without implementing pagination.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:26:31.566Z
Learning: For PR `#1884` and future PRs in rainlanguage/rain.orderbook repository, provide PR size reports in a separate comment with ONLY the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number", without any additional text or explanation. Reports should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:25:47.873Z
Learning: After each new commit review for rainlanguage/rain.orderbook repository, generate a PR size report in a separate comment with the format: "TOTAL=number ADDITIONS=number DELETIONS=number". This report should exclude irrelevant files such as lock files (e.g., package-lock.json, cargo.lock).
Comment on lines
+127
to
+170
| if !sg_ids.is_empty() { | ||
| let multi_subgraph_args = self.get_multi_subgraph_args(Some(sg_ids))?; | ||
| let orderbook_in = orderbook_addresses | ||
| .as_deref() | ||
| .filter(|addresses| !addresses.is_empty()) | ||
| .map(|addresses| { | ||
| addresses | ||
| .iter() | ||
| .map(|address| address.to_string().to_lowercase()) | ||
| .collect::<Vec<_>>() | ||
| }); | ||
| if !multi_subgraph_args.is_empty() { | ||
| 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 sg_trades = client | ||
| .trades_by_owner( | ||
| owner.to_string().to_lowercase(), | ||
| start_timestamp, | ||
| end_timestamp, | ||
| orderbook_in, | ||
| ) | ||
| .await; | ||
| 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(), | ||
| ))?; | ||
| let trade = RaindexTrade::try_from_sg_trade(chain_id, trade_with_name.trade)?; | ||
| all_trades.push(trade); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let final_total_count = total_count.unwrap_or(all_trades.len() as u64); |
Contributor
There was a problem hiding this comment.
Apply page to the subgraph branch as well.
Line 149 calls MultiOrderbookSubgraphClient::trades_by_owner, and that helper delegates to trades_by_owner_all in crates/subgraph/src/multi_orderbook_client.rs:162-195, so paginated requests still return every subgraph trade. When both backends are involved, Line 170 can then under report total_count because it only carries the local DB count while all_trades already contains the full subgraph result set. Please use a paginated subgraph path when page is set and include the subgraph side count in the final total. Based on learnings: In crates/common/src/raindex_client/orders.rs, get_orders applies page_number independently to LocalDbOrders::list and SubgraphOrders::list and concatenates the results. This is intentional and correct because classify_chains partitions chain IDs into mutually exclusive sets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/common/src/raindex_client/trades/get_by_owner.rs` around lines 127 -
170, The subgraph branch currently fetches all pages via
MultiOrderbookSubgraphClient::trades_by_owner (which delegates to
trades_by_owner_all), so when a page is requested the subgraph side still
returns the full result and final_total_count only reflects the local DB; fix by
calling the paginated subgraph path and including the subgraph-side count in the
final total: when page is set, invoke the paginated method on
MultiOrderbookSubgraphClient (or pass page_number/page_size into
trades_by_owner/trades_by_owner_all so it uses pagination), collect both the
returned trades and the subgraph total count, push the trades into all_trades as
before, and compute final_total_count as total_count.unwrap_or(0) +
subgraph_count (or if total_count is None use subgraph_count or all_trades.len()
as fallback) so final_total_count accounts for both local DB and subgraph
results; update references around get_multi_subgraph_args,
MultiOrderbookSubgraphClient::trades_by_owner / trades_by_owner_all, all_trades,
total_count, and final_total_count accordingly.
Collaborator
Author
There was a problem hiding this comment.
classify_chains partitions chains into mutually exclusive sets, so in practice only one source is queried. This matches get_trades_for_transaction which also fetches all from subgraph.
Contributor
There was a problem hiding this comment.
@findolor, you're right — since classify_chains produces mutually exclusive sets, only one backend is ever active for a given set of chain IDs. The total_count logic is therefore correct: it either reflects the local DB count (when paginating) or falls back to all_trades.len() for the subgraph path. My concern was mistaken. Apologies for the noise!
（ ／^ω^）／
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2155
File: crates/common/src/raindex_client/trades.rs:133-152
Timestamp: 2025-10-06T14:13:18.531Z
Learning: In the rain.orderbook codebase, the `page` parameter in `RaindexOrder::get_trades_list` method (in crates/common/src/raindex_client/trades.rs) is kept for backwards compatibility with subgraph logic, but the LocalDb fast-path intentionally returns all trades without implementing pagination.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/raindex_client/trades.rs:215-223
Timestamp: 2025-07-04T10:24:56.163Z
Learning: In the rain.orderbook codebase, the get_trade_count method implementation that fetches all trades to count them is intentionally consistent with previous implementations and not considered a performance issue, as indicated by findolor for the trades counting functionality in crates/common/src/raindex_client/trades.rs.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2488
File: crates/common/src/raindex_client/orders.rs:844-865
Timestamp: 2026-03-05T09:51:23.105Z
Learning: In `crates/common/src/raindex_client/orders.rs`, `get_orders` applies `page_number` independently to `LocalDbOrders::list` and `SubgraphOrders::list` and concatenates the results. This is intentional and correct because `classify_chains` partitions chain IDs into mutually exclusive sets (local DB chains vs subgraph chains), so the two source branches never query overlapping data. Global pagination across both backends is not implemented because it would require a complex cursor/merge strategy that is not warranted. Do not flag per-source pagination as a bug when results are from disjoint chain sets.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2376
File: crates/common/src/raindex_client/orders.rs:2-2
Timestamp: 2026-01-07T14:21:44.132Z
Learning: When local DB is enabled in the rain.orderbook codebase, the intended design is to use it exclusively without falling back to the subgraph, even when results are empty. Methods like get_trades_list() in trades.rs and get_vaults_volume() in orders.rs follow this pattern—they return local DB results directly when is_chain_supported_local_db() and local_db is available. Mixing data sources could cause inconsistencies. The get_balance_changes() method in vaults.rs that checks !local_changes.is_empty() before returning is an outlier that needs to be fixed to align with the exclusive local DB pattern.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2497
File: crates/common/src/raindex_client/local_db/query/fetch_trades_by_tx.rs:27-49
Timestamp: 2026-03-09T11:54:48.469Z
Learning: In `crates/common/src/raindex_client/local_db/query/fetch_trades_by_tx.rs`, the WASM unit test `wrapper_uses_builder_sql_exactly` intentionally returns an empty `[]` from the callback to focus exclusively on validating that the wrapper forwards the SQL built by `build_fetch_trades_by_tx_stmt`. The deserialization path (including new fields like `chain_id` on `LocalDbOrderTrade`) is intentionally covered by separate WASM and integration tests, not this unit test. This is by design and should not be flagged as a coverage gap.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/raindex_client/orders.rs:609-609
Timestamp: 2025-07-04T10:27:22.544Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor prefers not to implement overflow protection for trades count casting (usize to u16) at this time, considering it unnecessary for the current scope since the practical risk of orders having 65,535+ trades is extremely low.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2460
File: crates/subgraph/src/orderbook_client/order.rs:143-168
Timestamp: 2026-02-25T12:20:58.472Z
Learning: In the rainlanguage/rain.orderbook codebase at crates/subgraph/src/orderbook_client/order.rs, user findolor prefers not to implement overflow protection for the u16 `page` counter in `orders_count`, considering it unnecessary since reaching 65,535 pages (requiring ~13M orders at page_size=200) is far beyond any realistic dataset and not worth the added complexity.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2237
File: crates/common/src/raindex_client/local_db/sync.rs:79-89
Timestamp: 2025-10-18T10:38:41.273Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the sync_database method currently only supports indexing a single orderbook per chain ID, which is why `.first()` is used to select the orderbook configuration. Multi-orderbook support per chain ID is planned for future PRs.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2497
File: crates/subgraph/src/multi_orderbook_client.rs:128-160
Timestamp: 2026-03-10T06:24:18.339Z
Learning: In `crates/subgraph/src/multi_orderbook_client.rs`, the `filter_map(Result::ok)` pattern for silently dropping failing subgraphs and returning partial results is the intentional, established convention across all multi-subgraph methods (`orders_list`, `vaults_list`, `trades_by_transaction`). Do not flag this as a bug for individual methods — if the error propagation behavior needs to change, it should be done consistently across all methods in a dedicated PR.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/raindex_client/mod.rs:116-135
Timestamp: 2025-07-04T10:26:24.289Z
Learning: In crates/common/src/raindex_client/mod.rs, the get_multi_subgraph_args method intentionally treats Some(empty vector) the same as None for chain_ids parameter. Both cases should return all networks to support UI behavior where no selection or empty selection means "show all networks". Only when specific chain IDs are provided should the results be filtered.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1938
File: crates/js_api/src/raindex/mod.rs:92-99
Timestamp: 2025-06-18T18:18:44.330Z
Learning: In crates/js_api/src/raindex/mod.rs, the get_subgraph_url_for_chain method and get_multi_subgraph_args method intentionally duplicate lookup logic because they serve different purposes: get_subgraph_url_for_chain returns only the URL string, while get_multi_subgraph_args returns MultiSubgraphArgs structures containing both URL and network information (name/label). This duplication is acceptable and by design.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1858
File: crates/subgraph/src/orderbook_client/vault.rs:81-92
Timestamp: 2025-05-19T12:09:10.694Z
Learning: The `vault_balance_changes_list` function in OrderbookSubgraphClient uses a different pagination approach compared to other list methods. It uses hard-coded GraphQL query parameters (first=200, skip=0) while still accepting pagination arguments, and the pagination is handled by special logic inside the `query_paginated` method that properly processes these values.
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2044
File: packages/orderbook/README.md:186-189
Timestamp: 2025-08-01T09:07:20.383Z
Learning: In the rainlanguage/rain.orderbook project, Rust methods on structs like RaindexVaultsList are exported as JavaScript getters in WASM bindings using #[wasm_bindgen(getter)]. This means while the Rust code uses method calls like items(), the JavaScript/WASM API exposes them as property access like .items. The README.md correctly documents the JavaScript API surface, not the Rust implementation details.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2497
File: crates/subgraph/src/types/common.rs:154-159
Timestamp: 2026-03-09T11:54:46.878Z
Learning: In `crates/subgraph/src/types/common.rs`, `SgTradeWithSubgraphName` does not need `impl_wasm_traits!` because it is only used internally in Rust to collect subgraph results before they are converted to `RaindexTrade`. It is never serialized across the WASM boundary directly, unlike `SgOrderWithSubgraphName` which does need the macro.
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1911
File: crates/subgraph/src/types/impls.rs:7-15
Timestamp: 2025-07-21T16:34:31.193Z
Learning: In the rainlanguage/rain.orderbook codebase, user 0xgleb considers breaking changes that remove unsafe default behaviors to be intentional and acceptable. Specifically, the get_decimals() method in crates/subgraph/src/types/impls.rs was intentionally changed to return MissingDecimals error instead of defaulting to 18 decimals, as defaulting to 18 is considered unsafe and should never have been done.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1913
File: Cargo.toml:55-55
Timestamp: 2025-06-06T16:38:04.182Z
Learning: In rain.orderbook repository, during PR chains involving dependency updates, wasm-bindgen-utils may temporarily point to git commits that don't contain the full required functionality, with the understanding that subsequent PRs in the chain will update it to the correct commit.
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2036
File: crates/js_api/src/filters/raindex_filter_store.rs:446-533
Timestamp: 2025-08-02T02:34:32.237Z
Learning: In the rainlanguage/rain.orderbook project's RaindexFilterStore WASM tests (crates/js_api/src/filters/raindex_filter_store.rs), brusherru decided to focus on testing only methods without side effects (that don't use web_sys) due to difficulties with mocking localStorage and window APIs in the WASM test environment. This pragmatic approach tests pure logic separately from browser integration.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:24:53.708Z
Learning: For PR `#1884` in rainlanguage/rain.orderbook, provide a separate comment after each review with PR size statistics in the format: `TOTAL=number ADDITIONS=number DELETIONS=number`, excluding lock files like package-lock.json and cargo.lock.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:26:31.566Z
Learning: For PR `#1884` and future PRs in rainlanguage/rain.orderbook repository, provide PR size reports in a separate comment with ONLY the exact format "TOTAL=number\nADDITIONS=number\nDELETIONS=number", without any additional text or explanation. Reports should exclude irrelevant files such as lock files (package-lock.json, yarn.lock, pnpm-lock.yaml, cargo.lock).
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 0
File: :0-0
Timestamp: 2025-05-21T21:25:47.873Z
Learning: After each new commit review for rainlanguage/rain.orderbook repository, generate a PR size report in a separate comment with the format: "TOTAL=number ADDITIONS=number DELETIONS=number". This report should exclude irrelevant files such as lock files (e.g., package-lock.json, cargo.lock).
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2370
File: crates/common/src/raindex_client/vaults.rs:896-973
Timestamp: 2026-01-07T15:38:32.716Z
Learning: In the rainlanguage/rain.orderbook repo, prefer keeping self-contained match arms over extracting a helper function to avoid duplicating many fields when handling related subgraph types (e.g., SgDeposit, SgWithdrawal, SgClearBounty). The cost of passing 10+ fields to a helper may reduce readability and maintainability; duplicating the match arms is acceptable if it keeps the code clearer in this module. Apply this guidance broadly to Rust files in crates/common/src/raindex_client unless refactoring clearly improves structure without excessive boilerplate.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2399
File: crates/rest_api/src/routes/take_orders.rs:74-95
Timestamp: 2026-01-16T12:07:15.521Z
Learning: In Rust projects using utoipa, when you use #[schema(...)] attributes (for example #[schema(example = json!(...))]), the json! macro is handled internally by utoipa's proc-macro at compile time. You do not need to import serde_json::json! in the module for these attributes. Do not add a serde_json::json! import solely for these schema attributes; ensure serde_json is present only if used elsewhere."
findolor
force-pushed
the
2026-03-09-trade-struct-improvements
branch
from
4175969 to
384ece0
Compare
findolor
force-pushed
the
2026-03-10-trades-by-owner
branch
from
489d309 to
8184013
Compare
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4 tasks
findolor
force-pushed
the
2026-03-09-trade-struct-improvements
branch
2 times, most recently
from
66a68fb to
53020e7
Compare
findolor
force-pushed
the
2026-03-09-trade-struct-improvements
branch
from
53020e7 to
cfcd824
Compare
findolor
force-pushed
the
2026-03-10-trades-by-owner
branch
from
8184013 to
65e8a3c
Compare
JuaniRios
approved these changes
May 7, 2026
0xgleb
approved these changes
May 7, 2026
findolor
changed the base branch from
2026-03-09-trade-struct-improvements
to
graphite-base/2499
findolor
force-pushed
the
2026-03-10-trades-by-owner
branch
from
65e8a3c to
7e965a1
Compare
findolor
force-pushed
the
2026-03-10-trades-by-owner
branch
from
7e965a1 to
8ea4431
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/raindex_client/order_quotes.rs (1)
331-333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the repartition slice to avoid panic on mismatched quote counts.
flat_raindex[offset..offset + count]can panic ifget_order_quotesreturns fewer items thanpair_countspredicts. This should fail with aRaindexErrorinstead of crashing.Suggested fix
let mut result = Vec::with_capacity(orders.len()); let mut offset = 0; for count in pair_counts { - result.push(flat_raindex[offset..offset + count].to_vec()); + let end = offset + count; + if end > flat_raindex.len() { + return Err(RaindexError::PreflightError(format!( + "Quote result count mismatch: expected at least {}, got {}", + end, + flat_raindex.len() + ))); + } + result.push(flat_raindex[offset..end].to_vec()); offset += count; }🤖 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/order_quotes.rs` around lines 331 - 333, The loop slicing flat_raindex with flat_raindex[offset..offset + count] can panic when offset+count exceeds flat_raindex.len(); update the loop in the function that repartitions get_order_quotes results (the code iterating over pair_counts, using variables flat_raindex, offset, count, and pushing into result) to check bounds before slicing and return a RaindexError on mismatch instead of panicking. Specifically, verify offset + count <= flat_raindex.len() for each iteration and if not, return Err(RaindexError::MismatchedQuoteCounts { expected: pair_counts.clone(), actual_len: flat_raindex.len() }) or an appropriate RaindexError variant so callers get a structured error rather than a panic. Ensure offset is still advanced and errors propagate via the function's Result type.
🧹 Nitpick comments (1)
crates/subgraph/src/orderbook_client/order_trade.rs (1)
88-149: ⚡ Quick winAdd rustdoc to the new public owner-trade APIs.
These methods expand the public
OrderbookSubgraphClientsurface, but they currently ship without the rustdoc comments used elsewhere in this directory.📝 Suggested rustdoc shape
+ /// Fetches a single page of trades for an owner, optionally filtered by + /// timestamp range and orderbook addresses. pub async fn trades_by_owner( &self, owner: String, start_timestamp: Option<u64>, end_timestamp: Option<u64>, orderbook_in: Option<Vec<String>>, pagination_args: SgPaginationArgs, ) -> Result<Vec<SgTrade>, OrderbookSubgraphClientError> {+ /// Fetches all trades for an owner by paging through `trades_by_owner` + /// using `ALL_PAGES_QUERY_PAGE_SIZE`. pub async fn trades_by_owner_all( &self, owner: String, start_timestamp: Option<u64>, end_timestamp: Option<u64>, orderbook_in: Option<Vec<String>>, ) -> Result<Vec<SgTrade>, OrderbookSubgraphClientError> {Based on learnings: Enforce a Rust public API documentation convention in this directory. Require rustdoc comments on all public items (pub fn, pub struct, pub enum, etc.) describing behavior and usage.
🤖 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/orderbook_client/order_trade.rs` around lines 88 - 149, Add rustdoc comments for the newly public functions trades_by_owner and trades_by_owner_all: document what each function does, list and describe parameters (owner, start_timestamp, end_timestamp, orderbook_in, and pagination_args for trades_by_owner), explain the return type (Result<Vec<SgTrade>, OrderbookSubgraphClientError>), and note error conditions/behavior (e.g., pagination and query failures). Follow the existing rustdoc style in this module (short summary line, further details, parameter annotations, and an example or usage note for the “all” pager), and place the /// comments immediately above the pub async fn declarations for trades_by_owner and trades_by_owner_all.
🤖 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_owner_trades/mod.rs`:
- Around line 58-69: The pagination handling silently accepts zero values:
args.pagination.page = Some(0) becomes page 1 via saturating_sub(1) and
page_size = Some(0) yields LIMIT 0; add explicit validation in the block using
args.pagination (check args.pagination.page and args.pagination.page_size) to
reject zeroes (return an error or map to DEFAULT_PAGE_SIZE only when page_size
is None), so update the code around the pagination branch that builds the
LIMIT/OFFSET (references: args.pagination.page, args.pagination.page_size,
DEFAULT_PAGE_SIZE, PAGINATION_CLAUSE, stmt, and SqlValue::U64) to validate
inputs first and bail with a clear error for page == 0 or page_size == 0 instead
of generating SQL with LIMIT 0 or silently adjusting page.
---
Outside diff comments:
In `@crates/common/src/raindex_client/order_quotes.rs`:
- Around line 331-333: The loop slicing flat_raindex with
flat_raindex[offset..offset + count] can panic when offset+count exceeds
flat_raindex.len(); update the loop in the function that repartitions
get_order_quotes results (the code iterating over pair_counts, using variables
flat_raindex, offset, count, and pushing into result) to check bounds before
slicing and return a RaindexError on mismatch instead of panicking.
Specifically, verify offset + count <= flat_raindex.len() for each iteration and
if not, return Err(RaindexError::MismatchedQuoteCounts { expected:
pair_counts.clone(), actual_len: flat_raindex.len() }) or an appropriate
RaindexError variant so callers get a structured error rather than a panic.
Ensure offset is still advanced and errors propagate via the function's Result
type.
---
Nitpick comments:
In `@crates/subgraph/src/orderbook_client/order_trade.rs`:
- Around line 88-149: Add rustdoc comments for the newly public functions
trades_by_owner and trades_by_owner_all: document what each function does, list
and describe parameters (owner, start_timestamp, end_timestamp, orderbook_in,
and pagination_args for trades_by_owner), explain the return type
(Result<Vec<SgTrade>, OrderbookSubgraphClientError>), and note error
conditions/behavior (e.g., pagination and query failures). Follow the existing
rustdoc style in this module (short summary line, further details, parameter
annotations, and an example or usage note for the “all” pager), and place the
/// comments immediately above the pub async fn declarations for trades_by_owner
and trades_by_owner_all.
🪄 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: 6a4417e8-1d67-4c0f-bcf3-a2869a4e496c
📒 Files selected for processing (20)
crates/common/src/local_db/query/fetch_owner_trades/mod.rscrates/common/src/local_db/query/fetch_owner_trades/query.sqlcrates/common/src/local_db/query/fetch_owner_trades_common.rscrates/common/src/local_db/query/fetch_owner_trades_count/mod.rscrates/common/src/local_db/query/fetch_owner_trades_count/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/query/fetch_owner_trades.rscrates/common/src/raindex_client/local_db/query/fetch_owner_trades_count.rscrates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/raindex_client/mod.rscrates/common/src/raindex_client/order_quotes.rscrates/common/src/raindex_client/orders.rscrates/common/src/raindex_client/trades/get_by_owner.rscrates/common/src/raindex_client/trades/get_by_tx.rscrates/common/src/raindex_client/trades/mod.rscrates/common/src/raindex_client/types.rscrates/common/src/raindex_client/vaults.rscrates/subgraph/src/multi_orderbook_client.rscrates/subgraph/src/orderbook_client/mod.rscrates/subgraph/src/orderbook_client/order_trade.rs
✅ Files skipped from review due to trivial changes (3)
- crates/subgraph/src/orderbook_client/mod.rs
- crates/common/src/raindex_client/orders.rs
- crates/common/src/raindex_client/vaults.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/common/src/raindex_client/local_db/query/mod.rs
- crates/common/src/raindex_client/local_db/query/fetch_owner_trades_count.rs
- crates/common/src/local_db/query/mod.rs
- crates/subgraph/src/multi_orderbook_client.rs
- crates/common/src/local_db/query/fetch_owner_trades/query.sql
- crates/common/src/local_db/query/fetch_owner_trades_count/mod.rs
- crates/common/src/raindex_client/trades/get_by_tx.rs
Comment on lines
+58
to
+69
| if let Some(page) = args.pagination.page { | ||
| let page_size = args.pagination.page_size.unwrap_or(DEFAULT_PAGE_SIZE); | ||
| let offset = (page.saturating_sub(1) as u64) * (page_size as u64); | ||
| let limit_placeholder = format!("?{}", stmt.params.len() + 1); | ||
| let offset_placeholder = format!("?{}", stmt.params.len() + 2); | ||
| let pagination = format!("LIMIT {} OFFSET {}", limit_placeholder, offset_placeholder); | ||
| stmt.sql = stmt.sql.replace(PAGINATION_CLAUSE, &pagination); | ||
| stmt.push(SqlValue::U64(page_size as u64)); | ||
| stmt.push(SqlValue::U64(offset)); | ||
| } else { | ||
| stmt.sql = stmt.sql.replace(PAGINATION_CLAUSE, ""); | ||
| } |
Contributor
There was a problem hiding this comment.
Reject zero-valued pagination inputs.
page = Some(0) is silently treated as page 1 because of saturating_sub(1), and page_size = Some(0) emits LIMIT 0. Both cases make bad caller input look valid and can return misleading results.
🛠️ Suggested guard
if let Some(page) = args.pagination.page {
+ if page == 0 {
+ return Err(SqlBuildError::new("page must be >= 1"));
+ }
let page_size = args.pagination.page_size.unwrap_or(DEFAULT_PAGE_SIZE);
+ if page_size == 0 {
+ return Err(SqlBuildError::new("page_size must be >= 1"));
+ }
let offset = (page.saturating_sub(1) as u64) * (page_size as u64);
let limit_placeholder = format!("?{}", stmt.params.len() + 1);
let offset_placeholder = format!("?{}", stmt.params.len() + 2);🤖 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_owner_trades/mod.rs` around lines 58 -
69, The pagination handling silently accepts zero values: args.pagination.page =
Some(0) becomes page 1 via saturating_sub(1) and page_size = Some(0) yields
LIMIT 0; add explicit validation in the block using args.pagination (check
args.pagination.page and args.pagination.page_size) to reject zeroes (return an
error or map to DEFAULT_PAGE_SIZE only when page_size is None), so update the
code around the pagination branch that builds the LIMIT/OFFSET (references:
args.pagination.page, args.pagination.page_size, DEFAULT_PAGE_SIZE,
PAGINATION_CLAUSE, stmt, and SqlValue::U64) to validate inputs first and bail
with a clear error for page == 0 or page_size == 0 instead of generating SQL
with LIMIT 0 or silently adjusting page.
Move trades.rs to trades/mod.rs and extract get_trades_for_transaction into trades/get_by_tx.rs for better code organization.
Add fetch_owner_trades and fetch_owner_trades_count SQL queries with list-based chain_id/orderbook filtering, time range filters, and pagination support. Includes executor wrappers and unit tests.
Add SgOwnerTradesQueryVariables, SgOwnerTradesListQuery, trades_by_owner() on OrderbookSubgraphClient and MultiOrderbookSubgraphClient. Includes mock server tests.
Add get_trades_for_owner with WASM binding that queries owner trades across local DB and subgraph data sources using the classify_chains pattern.
… PaginationParams types Extract shared constants, dedup/sort logic, timestamp validation, and clause binding into fetch_owner_trades_common.rs. Introduce TimeFilter and PaginationParams as reusable types in raindex_client, replacing loose start_timestamp/end_timestamp/page parameters throughout the owner trades API.
findolor
force-pushed
the
2026-03-10-trades-by-owner
branch
from
8ea4431 to
337d412
Compare
Collaborator
Author
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/common/src/raindex_client/trades/mod.rs (1)
843-853: ⚡ Quick winAvoid brittle SQL placeholder matching in wasm DB callback.
Matching vault queries via
"?3 AS vault_id"is fragile and can break tests if bind-order changes without semantic query changes. Match on stable query traits (table/alias + vault filter intent) instead.Proposed hardening
- if sql.contains("?3 AS vault_id") { + if sql.contains("FROM vaults") + && sql.to_ascii_lowercase().contains("vault_id") + { if !params.is_undefined() && !params.is_null() { let params_array = Array::from(¶ms); if let Some(vault_id_val) = params_array.get(2).as_string() {🤖 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/mod.rs` around lines 843 - 853, The current brittle check sql.contains("?3 AS vault_id") should be replaced with a more resilient predicate: detect queries that project or alias a vault id and intend to filter by vault (e.g., check for a case-insensitive "AS vault_id" or regex like r"(?i)\bAS\s+vault_id\b" and also require the presence of a vault table/alias token such as " vault ", " v.", or " vaults " and a vault filter token like "WHERE" + "vault_id" or "vault_id =" / "vault_id = ?"); then extract the vault parameter from params_array by scanning for the first string-like param (instead of assuming index 2) and lowercase it before looking up vault_payloads; update the condition around sql, params, params_array, and vault_payloads accordingly to use this stable detection logic.crates/common/src/raindex_client/local_db/query/fetch_owner_trades_count.rs (1)
25-49: ⚡ Quick winWasm test does not verify query parameters, only the SQL string.
captured.1holds the serialised params that the executor received, but the assertion on Line 48 checks onlycaptured.0(SQL text). An accidental regression in parameter binding (e.g. a missing or wrongly-typed param) would not be caught.🛡️ Suggested addition
let captured = store.borrow().clone(); assert_eq!(captured.0, expected_stmt.sql); +// Verify the params passed to the executor match the builder's output. +let expected_params = wasm_bindgen::JsValue::from_str( + &serde_json::to_string(&expected_stmt.params).unwrap(), +); +assert_eq!(format!("{:?}", captured.1), format!("{:?}", expected_params));(Adjust the serialisation approach to match how
JsCallbackExecutorserialisesSqlStatement::params.)🤖 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/local_db/query/fetch_owner_trades_count.rs` around lines 25 - 49, The wasm test wrapper_uses_builder_sql_exactly currently only asserts the SQL string and misses verifying parameters; update the test to also deserialize and assert the captured parameters (store.borrow().1 / captured.1) match the expected SqlStatement params produced by build_fetch_owner_trades_count_stmt(&args). Specifically, after building expected_stmt = build_fetch_owner_trades_count_stmt(&args).unwrap(), deserialize captured.1 using the same serialization format used by JsCallbackExecutor/SqlStatement::params and assert equality with expected_stmt.params (or perform field-wise checks if types differ), keeping all existing assertions for fetch_owner_trades_count(&exec, args) and captured.0.
🤖 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.
Nitpick comments:
In `@crates/common/src/raindex_client/local_db/query/fetch_owner_trades_count.rs`:
- Around line 25-49: The wasm test wrapper_uses_builder_sql_exactly currently
only asserts the SQL string and misses verifying parameters; update the test to
also deserialize and assert the captured parameters (store.borrow().1 /
captured.1) match the expected SqlStatement params produced by
build_fetch_owner_trades_count_stmt(&args). Specifically, after building
expected_stmt = build_fetch_owner_trades_count_stmt(&args).unwrap(), deserialize
captured.1 using the same serialization format used by
JsCallbackExecutor/SqlStatement::params and assert equality with
expected_stmt.params (or perform field-wise checks if types differ), keeping all
existing assertions for fetch_owner_trades_count(&exec, args) and captured.0.
In `@crates/common/src/raindex_client/trades/mod.rs`:
- Around line 843-853: The current brittle check sql.contains("?3 AS vault_id")
should be replaced with a more resilient predicate: detect queries that project
or alias a vault id and intend to filter by vault (e.g., check for a
case-insensitive "AS vault_id" or regex like r"(?i)\bAS\s+vault_id\b" and also
require the presence of a vault table/alias token such as " vault ", " v.", or "
vaults " and a vault filter token like "WHERE" + "vault_id" or "vault_id =" /
"vault_id = ?"); then extract the vault parameter from params_array by scanning
for the first string-like param (instead of assuming index 2) and lowercase it
before looking up vault_payloads; update the condition around sql, params,
params_array, and vault_payloads accordingly to use this stable detection logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81cff30d-36d4-493c-8eb9-fb81ca723805
📒 Files selected for processing (22)
crates/common/src/local_db/query/fetch_owner_trades/mod.rscrates/common/src/local_db/query/fetch_owner_trades/query.sqlcrates/common/src/local_db/query/fetch_owner_trades_common.rscrates/common/src/local_db/query/fetch_owner_trades_count/mod.rscrates/common/src/local_db/query/fetch_owner_trades_count/query.sqlcrates/common/src/local_db/query/mod.rscrates/common/src/raindex_client/local_db/query/fetch_owner_trades.rscrates/common/src/raindex_client/local_db/query/fetch_owner_trades_count.rscrates/common/src/raindex_client/local_db/query/mod.rscrates/common/src/raindex_client/mod.rscrates/common/src/raindex_client/order_quotes.rscrates/common/src/raindex_client/orders.rscrates/common/src/raindex_client/trades/get_by_owner.rscrates/common/src/raindex_client/trades/get_by_tx.rscrates/common/src/raindex_client/trades/mod.rscrates/common/src/raindex_client/types.rscrates/common/src/raindex_client/vaults.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 (6)
- crates/common/src/raindex_client/vaults.rs
- crates/subgraph/src/orderbook_client/mod.rs
- crates/common/src/raindex_client/local_db/query/mod.rs
- crates/common/src/raindex_client/types.rs
- crates/common/src/raindex_client/trades/get_by_owner.rs
- crates/common/src/raindex_client/order_quotes.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/common/src/local_db/query/mod.rs
- crates/common/src/raindex_client/local_db/query/fetch_owner_trades.rs
- crates/subgraph/src/types/order_trade.rs
- crates/subgraph/src/orderbook_client/order_trade.rs
- crates/common/src/local_db/query/fetch_owner_trades_count/query.sql
- crates/common/src/local_db/query/fetch_owner_trades_common.rs
- crates/common/src/raindex_client/trades/get_by_tx.rs
- crates/common/src/raindex_client/mod.rs
- crates/subgraph/src/multi_orderbook_client.rs
- crates/common/src/local_db/query/fetch_owner_trades/mod.rs
- crates/common/src/local_db/query/fetch_owner_trades/query.sql
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 |
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
The parent PR added improved trade structs and trades-by-transaction support. This PR extends the trades API to support fetching trades by owner address, enabling the frontend to display all trades for a given wallet across all orderbooks and chains.
Solution
trades.rsinto atrades/directory withmod.rs,get_by_tx.rs, andget_by_owner.rsfor better code organizationfetch_owner_tradesandfetch_owner_trades_countSQL query modules that filter trades by owner address, with support for chain ID, orderbook, time range, and pagination filtersSgOwnerTradesQueryVariablesandSgOwnerTradesListQuerytypes, plustrades_by_owner/trades_by_owner_allmethods on both single and multi-orderbook subgraph clientsget_trades_for_ownerusing theclassify_chainspattern to query both local DB and subgraph sources, merge results, and returnRaindexTradesListResultwith total count and optional pair summaryChecks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests