Open
Conversation
Add SgTransactionTradesQuery to fetch trades by transaction hash from the subgraph, with tests for found, empty, and error cases.
Add SQL query and statement builder to fetch trades by transaction hash from the local SQLite database, unioning take_trades, clear_alice, and clear_bob with vault balances and token metadata.
Wire up the local DB query through the raindex client layer with LocalDbTrades::get_by_tx_hash, converting LocalDbOrderTrade results to RaindexTrade. Includes WASM tests.
Add RaindexClient::get_trades_for_transaction with WASM binding, routing to local DB when supported and falling back to the subgraph. Includes tests with httpmock for found, empty, and error cases.
Contributor
WalkthroughAdds end-to-end support to fetch trades by transaction hash: SQL query + Rust builder, local DB executor and wrapper, RaindexClient local-db-first polling with subgraph fallback, GraphQL query/types, wasm and unit tests, and small JS API visibility changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RaindexClient
participant LocalDb
participant SqlExecutor
participant OrderbookSubgraph
Client->>RaindexClient: get_trades_for_transaction(chain_id, orderbook_addr, tx_hash)
activate RaindexClient
alt Local DB available & chain supported
RaindexClient->>LocalDb: LocalDbTrades::get_by_tx_hash(ob_id, tx_hash)
activate LocalDb
LocalDb->>SqlExecutor: build_fetch_trades_by_tx_stmt(ob_id, tx_hash) + execute
activate SqlExecutor
SqlExecutor-->>LocalDb: Vec<LocalDbOrderTrade>
deactivate SqlExecutor
LocalDb-->>RaindexClient: Vec<RaindexTrade> (converted)
deactivate LocalDb
else Fallback to Subgraph
RaindexClient->>OrderbookSubgraph: transaction_trades(tx_id) (polling until results or timeout)
activate OrderbookSubgraph
OrderbookSubgraph-->>RaindexClient: Vec<SgTrade> or Empty/Error
deactivate OrderbookSubgraph
RaindexClient->>RaindexClient: map SgTrade -> RaindexTrade
end
RaindexClient-->>Client: Result<Vec<RaindexTrade>, RaindexError>
deactivate RaindexClient
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
🤖 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_trades_by_tx/query.sql`:
- Around line 236-255: The trade_rows CTE is a direct 1:1 projection of
unioned_trades (no transforms); remove the redundant trade_rows CTE and update
any downstream references (e.g., within CTEs or final SELECTs that currently
reference trade_rows) to reference unioned_trades directly (search for the
symbol trade_rows and replace usages with unioned_trades, ensuring column names
still match).
In `@crates/common/src/raindex_client/local_db/query/fetch_trades_by_tx.rs`:
- Around line 33-37: The tx_hash.clone() call is unnecessary because B256 is
Copy; update the call to build_fetch_trades_by_tx_stmt to pass tx_hash by value
instead of cloning it (locate the call to build_fetch_trades_by_tx_stmt that
constructs expected_stmt and remove the .clone() on tx_hash), leaving
OrderbookIdentifier::new(chain_id, orderbook) and the surrounding logic
unchanged.
In `@crates/common/src/raindex_client/trades/get_by_tx.rs`:
- Around line 1-2: Remove the redundant explicit import by deleting the line
that says "use super::RaindexTrade;" since "use super::*" already brings
RaindexTrade into scope; keep only "use super::*" to avoid duplicate imports and
potential warnings in the module containing RaindexTrade and the surrounding
code in get_by_tx.rs.
In `@crates/subgraph/src/orderbook_client/transaction.rs`:
- Around line 37-54: Add a missing doc comment for the public async method
transaction_trades to match the style used by its siblings
transaction_add_orders and transaction_remove_orders; locate the
transaction_trades function and add a /// brief description above it explaining
what the method returns (e.g., fetches trades for a transaction id and returns
Result<Vec<SgTrade>, OrderbookSubgraphClientError>), and include any important
behavior notes like that it returns OrderbookSubgraphClientError::Empty when no
trades are found.
Comment on lines
+236
to
+255
| trade_rows AS ( | ||
| SELECT | ||
| ut.trade_kind, | ||
| ut.chain_id, | ||
| ut.orderbook_address, | ||
| ut.order_hash, | ||
| ut.order_owner, | ||
| ut.order_nonce, | ||
| ut.transaction_hash, | ||
| ut.log_index, | ||
| ut.block_number, | ||
| ut.block_timestamp, | ||
| ut.transaction_sender, | ||
| ut.input_vault_id, | ||
| ut.input_token, | ||
| ut.input_delta, | ||
| ut.output_vault_id, | ||
| ut.output_token, | ||
| ut.output_delta | ||
| FROM unioned_trades ut |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
trade_rows CTE is a passthrough with no transformation.
This CTE is a 1:1 projection of unioned_trades with all the same columns listed explicitly. It adds no filtering or computation. Consider referencing unioned_trades directly in the subsequent CTE to reduce verbosity.
🤖 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_trades_by_tx/query.sql` around lines
236 - 255, The trade_rows CTE is a direct 1:1 projection of unioned_trades (no
transforms); remove the redundant trade_rows CTE and update any downstream
references (e.g., within CTEs or final SELECTs that currently reference
trade_rows) to reference unioned_trades directly (search for the symbol
trade_rows and replace usages with unioned_trades, ensuring column names still
match).
Collaborator
Author
There was a problem hiding this comment.
Intentional — explicitly pins the column set rather than relying on SELECT * ordering from the union.
Contributor
There was a problem hiding this comment.
@findolor, thank you for clarifying! That makes perfect sense—explicitly projecting the columns ensures a stable, deterministic schema and avoids any fragility from implicit SELECT * ordering. I appreciate the explanation.
(\/)
(^.^)
(")_(") *learning noted*
✏️ Learnings added
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 within the rain.orderbook codebase (e.g., crates/common/src/local_db/query/fetch_trades_by_tx/query.sql), passthrough CTEs that explicitly list all columns from their source (like `trade_rows` projecting `unioned_trades`) are intentional. They pin the column set explicitly rather than relying on `SELECT *` ordering, ensuring deterministic schema and maintainability.
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 used
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: 2145
File: crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql:6-7
Timestamp: 2025-10-06T11:28:30.692Z
Learning: In `crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql`, the orderbook_address is currently hardcoded to '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' because the system only supports a single orderbook at the moment. Multiorderbook logic is not yet implemented and will be added in the future.
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: 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.
Comment on lines
+33
to
+37
| let expected_stmt = build_fetch_trades_by_tx_stmt( | ||
| &OrderbookIdentifier::new(chain_id, orderbook), | ||
| tx_hash.clone(), | ||
| ) | ||
| .unwrap(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: B256 is Copy; .clone() is unnecessary.
B256 implements Copy, so the .clone() on line 35 is superfluous.
Suggested fix
let expected_stmt = build_fetch_trades_by_tx_stmt(
&OrderbookIdentifier::new(chain_id, orderbook),
- tx_hash.clone(),
+ tx_hash,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let expected_stmt = build_fetch_trades_by_tx_stmt( | |
| &OrderbookIdentifier::new(chain_id, orderbook), | |
| tx_hash.clone(), | |
| ) | |
| .unwrap(); | |
| let expected_stmt = build_fetch_trades_by_tx_stmt( | |
| &OrderbookIdentifier::new(chain_id, orderbook), | |
| tx_hash, | |
| ) | |
| .unwrap(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/common/src/raindex_client/local_db/query/fetch_trades_by_tx.rs` around
lines 33 - 37, The tx_hash.clone() call is unnecessary because B256 is Copy;
update the call to build_fetch_trades_by_tx_stmt to pass tx_hash by value
instead of cloning it (locate the call to build_fetch_trades_by_tx_stmt that
constructs expected_stmt and remove the .clone() on tx_hash), leaving
OrderbookIdentifier::new(chain_id, orderbook) and the surrounding logic
unchanged.
Collaborator
Author
There was a problem hiding this comment.
Acknowledged, not addressing.
Contributor
There was a problem hiding this comment.
@findolor Understood, no problem! 👍
🧠 Learnings used
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: 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: 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: 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: 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: findolor
Repo: rainlanguage/rain.orderbook PR: 2145
File: crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql:6-7
Timestamp: 2025-10-06T11:28:30.692Z
Learning: In `crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql`, the orderbook_address is currently hardcoded to '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' because the system only supports a single orderbook at the moment. Multiorderbook logic is not yet implemented and will be added in the future.
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: 2163
File: crates/common/src/raindex_client/orders.rs:738-741
Timestamp: 2025-10-02T19:17:20.332Z
Learning: In crates/common/src/raindex_client/orders.rs, fetch_dotrain_source() is intentionally called in try_from_sg_order for every order conversion because the dotrain source information is needed immediately. A future optimization with local DB logic is planned to eliminate the network round-trip concern.
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2044
File: crates/common/src/raindex_client/vaults_list.rs:363-423
Timestamp: 2025-07-31T19:34:11.716Z
Learning: In the rainlanguage/rain.orderbook project, for WASM-exposed functionality like VaultsList, the team prefers to keep comprehensive tests in the non-WASM environment due to the complexity of recreating objects like RaindexVaults in WASM. WASM tests focus on basic functionality and error cases since the WASM code reuses the already-tested non-WASM implementation.
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1859
File: crates/quote/src/quote_debug.rs:472-492
Timestamp: 2025-05-20T10:20:08.206Z
Learning: In the Rain Orderbook codebase, the `#[tokio::test(flavor = "multi_thread")]` annotation is specifically needed for tests that use `LocalEvm`, not just for consistency across all async tests.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2358
File: crates/common/src/local_db/query/fetch_orders/mod.rs:17-21
Timestamp: 2026-01-16T06:20:47.764Z
Learning: In crates/common/src/local_db/query/fetch_orders/mod.rs, for internal structs like FetchOrdersTokensFilter that are never compared directly (tests compare individual fields instead), PartialEq and Eq derives are not needed. Documentation can be skipped when field names are self-explanatory, especially for internal types.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1706
File: crates/quote/src/js_api/mod.rs:57-61
Timestamp: 2025-04-29T06:19:15.446Z
Learning: In the rain.orderbook codebase, the `get_id` function is designed to use `U256::from_str()` rather than `U256::from_hex()` for parsing order hashes, despite the potential for hex string inputs.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2344
File: crates/common/src/local_db/pipeline/runner/mod.rs:18-31
Timestamp: 2025-12-03T10:40:25.429Z
Learning: In `crates/common/src/local_db/pipeline/runner/mod.rs`, the `TargetSuccess` struct does not need separate `ob_id` or `orderbook_key` fields because the contained `SyncOutcome` already includes orderbook identification information such as chain_id and orderbook_address. This avoids redundant data duplication.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2000
File: crates/common/src/raindex_client/vaults.rs:183-183
Timestamp: 2025-07-16T10:40:05.717Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor considers breaking changes from Option<U256> to U256 for required fields like decimals in RaindexVaultToken to be acceptable and safe, even when they affect multiple usage sites across the codebase.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1903
File: crates/common/src/deposit.rs:63-64
Timestamp: 2025-06-17T16:54:43.176Z
Learning: In the rain.orderbook codebase, the maintainer findolor prefers to keep the current implementation that clones the rpcs vector when calling ReadableClient::new_from_urls rather than optimizing it to avoid the clone or adding early validation for empty rpcs. Performance micro-optimizations like avoiding vector clones in this context are not a priority.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2163
File: crates/cli/src/commands/order/calldata.rs:47-48
Timestamp: 2025-09-24T10:54:42.060Z
Learning: In crates/cli/src/commands/order/calldata.rs, the config_deployment variable is used both as a parameter to AddOrderArgs::new_from_deployment() and later to extract RPC information, requiring the clone() to maintain access after ownership transfer.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2202
File: crates/common/src/raindex_client/local_db/sync.rs:33-34
Timestamp: 2025-10-14T07:51:55.148Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the hard-coded `DEFAULT_SYNC_CHAIN_ID` constant (set to `SUPPORTED_LOCAL_DB_CHAINS[0]`) will be replaced with proper chain ID handling in downstream PRs as part of the multi-network/orderbook implementation.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1858
File: crates/subgraph/src/cynic_client.rs:27-29
Timestamp: 2025-05-19T12:25:02.658Z
Learning: In the Rain Orderbook codebase, when passing a URL from `self.get_base_url()` to `reqwest::Client::post()`, the `.clone()` is necessary because `get_base_url()` returns a reference (`&Url`), but an owned `Url` is needed for the reqwest client in the async context to avoid lifetime issues.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2145
File: crates/common/src/raindex_client/local_db/query/create_tables/query.sql:71-72
Timestamp: 2025-10-06T11:44:07.888Z
Learning: The local DB feature in the rain.orderbook codebase is not live yet (as of PR `#2145`), so schema migrations for existing databases are not required when modifying table structures in `crates/common/src/raindex_client/local_db/query/create_tables/query.sql`.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2418
File: crates/cli/src/commands/local_db/executor.rs:168-170
Timestamp: 2026-01-28T14:11:21.065Z
Learning: In the rain.orderbook codebase, the `wipe_and_recreate` method on `LocalDbQueryExecutor` is primarily needed for the browser/WASM environment (JsCallbackExecutor with sqlite-web) to handle database corruption automatically. The CLI executor (RusqliteExecutor in `crates/cli/src/commands/local_db/executor.rs`) can return `NotImplemented` for `wipe_and_recreate` because CLI users have direct file system access and can manually delete corrupted database files if needed.
Comment on lines
+1
to
+2
| use super::RaindexTrade; | ||
| use super::*; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant explicit import.
use super::* on Line 2 already includes RaindexTrade, making the explicit import on Line 1 unnecessary.
🧹 Proposed fix
-use super::RaindexTrade;
use super::*;📝 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
| use super::RaindexTrade; | |
| use super::*; | |
| use super::*; |
🤖 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_tx.rs` around lines 1 - 2,
Remove the redundant explicit import by deleting the line that says "use
super::RaindexTrade;" since "use super::*" already brings RaindexTrade into
scope; keep only "use super::*" to avoid duplicate imports and potential
warnings in the module containing RaindexTrade and the surrounding code in
get_by_tx.rs.
Collaborator
Author
There was a problem hiding this comment.
Acknowledged, not addressing.
Contributor
There was a problem hiding this comment.
@findolor, understood! I'll leave this as is.
(\(\
(-.-)
o_(")(")
🧠 Learnings used
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: 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: 2163
File: crates/common/src/raindex_client/orders.rs:738-741
Timestamp: 2025-10-02T19:17:20.332Z
Learning: In crates/common/src/raindex_client/orders.rs, fetch_dotrain_source() is intentionally called in try_from_sg_order for every order conversion because the dotrain source information is needed immediately. A future optimization with local DB logic is planned to eliminate the network round-trip concern.
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: 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: 1950
File: crates/common/src/raindex_client/orders.rs:301-301
Timestamp: 2025-06-24T13:36:28.797Z
Learning: In the RaindexClient codebase, when Arc::new(RwLock::new(self.clone())) is used (e.g., in get_orders and get_order_by_hash methods), this creates new Arc/RwLock wrappers around a cloned RaindexClient, but the underlying client data is functionally the same. This pattern is used to provide the correct Arc<RwLock<RaindexClient>> type expected by RaindexOrder::try_from_sg_order() method.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2391
File: crates/rest_api/src/routes/take_orders.rs:75-93
Timestamp: 2026-01-16T11:17:16.153Z
Learning: In crates/rest_api, the take_orders handler uses spawn_blocking with a dedicated single-threaded Tokio runtime per request because RaindexClient contains Rc<RefCell<...>> internally which is not Send, while Rocket requires handler futures to be Send. This pattern is intentional and required to satisfy Rust's thread-safety requirements.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2008
File: crates/common/src/raindex_client/add_orders.rs:85-86
Timestamp: 2025-07-18T10:31:05.498Z
Learning: In the rainlanguage/rain.orderbook codebase, cfg-guarded imports like `#[cfg(not(target_family = "wasm"))] use super::*;` in test modules are sometimes needed to fix compiler warnings, even when similar imports exist in nested sub-modules. These should not be removed as they serve a specific purpose in the compilation process.
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1911
File: crates/subgraph/src/utils/mod.rs:1-1
Timestamp: 2025-07-21T16:37:20.599Z
Learning: In the rainlanguage/rain.orderbook codebase, user 0xgleb prefers explicit module imports over re-exporting symbols at higher levels. Specifically for the float constants in crates/subgraph/src/utils/float.rs, they prefer using `utils::float::*` rather than re-exporting with `pub use float::*` in the utils module, as the explicit import makes it clearer what is being imported and why it's needed.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2358
File: crates/common/src/local_db/query/fetch_orders/mod.rs:17-21
Timestamp: 2026-01-16T06:20:47.764Z
Learning: In crates/common/src/local_db/query/fetch_orders/mod.rs, for internal structs like FetchOrdersTokensFilter that are never compared directly (tests compare individual fields instead), PartialEq and Eq derives are not needed. Documentation can be skipped when field names are self-explanatory, especially for internal types.
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: brusherru
Repo: rainlanguage/rain.orderbook PR: 2044
File: crates/common/src/raindex_client/vaults_list.rs:363-423
Timestamp: 2025-07-31T19:34:11.716Z
Learning: In the rainlanguage/rain.orderbook project, for WASM-exposed functionality like VaultsList, the team prefers to keep comprehensive tests in the non-WASM environment due to the complexity of recreating objects like RaindexVaults in WASM. WASM tests focus on basic functionality and error cases since the WASM code reuses the already-tested non-WASM implementation.
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: 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: 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: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1860
File: packages/ui-components/src/lib/stores/transactionStore.ts:130-136
Timestamp: 2025-05-19T18:24:17.608Z
Learning: In the `rain.orderbook` project, the `getTransaction` function (and related entity fetch functions) use `wasmEncodedResult` to validate the returned data, ensuring that only valid data is returned. This makes a simple truthiness check (`!!data`) sufficient for the `isSuccess` predicate in `awaitSubgraphIndexing`.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2202
File: crates/common/src/raindex_client/local_db/sync.rs:33-34
Timestamp: 2025-10-14T07:51:55.148Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the hard-coded `DEFAULT_SYNC_CHAIN_ID` constant (set to `SUPPORTED_LOCAL_DB_CHAINS[0]`) will be replaced with proper chain ID handling in downstream PRs as part of the multi-network/orderbook implementation.
Comment on lines
37
to
54
| pub async fn transaction_trades( | ||
| &self, | ||
| id: Id, | ||
| ) -> Result<Vec<SgTrade>, OrderbookSubgraphClientError> { | ||
| let data = self | ||
| .query::<SgTransactionTradesQuery, TransactionTradesVariables>( | ||
| TransactionTradesVariables { | ||
| id: SgBytes(id.inner().to_string()), | ||
| }, | ||
| ) | ||
| .await?; | ||
|
|
||
| if data.trades.is_empty() { | ||
| return Err(OrderbookSubgraphClientError::Empty); | ||
| } | ||
|
|
||
| Ok(data.trades) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing doc comment for transaction_trades.
The sibling methods transaction_add_orders (Line 17) and transaction_remove_orders (Line 56) have /// doc comments, but transaction_trades does not. Add one for consistency.
📝 Proposed fix
+ /// Fetch all trades for a given transaction
pub async fn transaction_trades(📝 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
| pub async fn transaction_trades( | |
| &self, | |
| id: Id, | |
| ) -> Result<Vec<SgTrade>, OrderbookSubgraphClientError> { | |
| let data = self | |
| .query::<SgTransactionTradesQuery, TransactionTradesVariables>( | |
| TransactionTradesVariables { | |
| id: SgBytes(id.inner().to_string()), | |
| }, | |
| ) | |
| .await?; | |
| if data.trades.is_empty() { | |
| return Err(OrderbookSubgraphClientError::Empty); | |
| } | |
| Ok(data.trades) | |
| } | |
| /// Fetch all trades for a given transaction | |
| pub async fn transaction_trades( | |
| &self, | |
| id: Id, | |
| ) -> Result<Vec<SgTrade>, OrderbookSubgraphClientError> { | |
| let data = self | |
| .query::<SgTransactionTradesQuery, TransactionTradesVariables>( | |
| TransactionTradesVariables { | |
| id: SgBytes(id.inner().to_string()), | |
| }, | |
| ) | |
| .await?; | |
| if data.trades.is_empty() { | |
| return Err(OrderbookSubgraphClientError::Empty); | |
| } | |
| Ok(data.trades) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/subgraph/src/orderbook_client/transaction.rs` around lines 37 - 54,
Add a missing doc comment for the public async method transaction_trades to
match the style used by its siblings transaction_add_orders and
transaction_remove_orders; locate the transaction_trades function and add a ///
brief description above it explaining what the method returns (e.g., fetches
trades for a transaction id and returns Result<Vec<SgTrade>,
OrderbookSubgraphClientError>), and include any important behavior notes like
that it returns OrderbookSubgraphClientError::Empty when no trades are found.
Collaborator
Author
There was a problem hiding this comment.
We don't have a convention of doc comments on public methods — the existing ones are incidental.
Contributor
There was a problem hiding this comment.
@findolor, thank you for clarifying! I understand—I won't flag missing doc comments on public methods in the future.
✏️ Learnings added
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2465
File: crates/subgraph/src/orderbook_client/transaction.rs:37-54
Timestamp: 2026-02-18T08:41:10.689Z
Learning: In the rainlanguage/rain.orderbook codebase, there is no convention requiring doc comments on public methods. Existing doc comments on public methods are incidental and not part of a deliberate documentation standard.
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 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: 1744
File: crates/js_api/src/subgraph/order.rs:109-114
Timestamp: 2025-05-09T05:28:22.089Z
Learning: In the rain.orderbook project, SubgraphError contains or converts from OrderbookSubgraphClientError, so using OrderbookSubgraphClientError in error creation followed by a conversion to SubgraphError (via ? operator) is acceptable.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2358
File: crates/common/src/local_db/query/fetch_orders/mod.rs:17-21
Timestamp: 2026-01-16T06:20:47.764Z
Learning: In crates/common/src/local_db/query/fetch_orders/mod.rs, for internal structs like FetchOrdersTokensFilter that are never compared directly (tests compare individual fields instead), PartialEq and Eq derives are not needed. Documentation can be skipped when field names are self-explanatory, especially for internal types.
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1724
File: packages/ui-components/src/__tests__/ButtonDarkMode.test.ts:1-54
Timestamp: 2025-05-03T10:29:04.009Z
Learning: JSDoc comments are not considered necessary for test files in the rainlanguage/rain.orderbook repository. Test descriptions and assertions are sufficient documentation.
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.
Mirror the two-phase polling pattern from get_transaction: poll local DB for the full window first, then fall back to subgraph polling. Returns TradesIndexingTimeout when all attempts are exhausted.
…into 2026-02-17-trades-by-tx-hash
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/raindex_client/trades/get_by_tx.rs`:
- Line 130: The TradesIndexingTimeout error currently receives the variable
attempts which under-reports the actual number of poll attempts when both the
local DB and subgraph phases run (total polls can be the sum of both phases);
update get_by_tx.rs so that before constructing
Err(RaindexError::TradesIndexingTimeout { tx_hash, attempts }) you compute and
pass an accurate attempts count (e.g., total_attempts = local_attempts +
subgraph_attempts) or else pass both configured_attempts and actual_attempts in
the error variant; adjust the code paths around the polling loops that use
attempts (in the functions/blocks responsible for local DB polling and subgraph
polling in get_by_tx.rs) so the value passed into TradesIndexingTimeout reflects
your chosen semantics.
- Around line 93-106: The local-DB branch currently falls through to the
subgraph polling after exhausting attempts; change it so that when
is_chain_supported_local_db(chain_id) is true and self.local_db() returns Some,
the function returns a TradesIndexingTimeout (or appropriate error) immediately
after the LocalDbTrades::new(...).get_by_tx_hash(...) loop finishes without
results instead of continuing to the subgraph block; specifically modify the
block containing LocalDbTrades::new, get_by_tx_hash, attempts and interval_ms to
return Err(TradesIndexingTimeout { attempts }) (or construct the same timeout
error used by the subgraph path) once attempts are exhausted so the local DB
remains the exclusive source. Ensure you reference and update the same error
type used later so callers see consistent timeout reporting.
In `@crates/js_api/src/gui/order_operations.rs`:
- Around line 99-108: The module export makes CalldataFunction and
VaultAndDeposit transitively public via pub mod order_operations; restrict their
visibility to avoid unintentionally expanding the public API by changing their
declarations from pub to either pub(super) or pub(crate) (whichever fits the
intended encapsulation) so they remain usable by prepare_calldata_generation and
get_vaults_and_deposits but are not exposed as part of the module's public
surface.
---
Duplicate comments:
In `@crates/common/src/raindex_client/trades/get_by_tx.rs`:
- Around line 1-2: Remove the redundant import: `use super::RaindexTrade` is
already covered by `use super::*`, so delete the explicit `use
super::RaindexTrade` line (or alternatively remove `use super::*` if you prefer
explicit imports) so only one import remains for RaindexTrade and avoid
duplicate imports.
Comment on lines
+93
to
+106
| if is_chain_supported_local_db(chain_id) { | ||
| if let Some(local_db) = self.local_db() { | ||
| let local_source = LocalDbTrades::new(&local_db); | ||
| for attempt in 1..=attempts { | ||
| let trades = local_source.get_by_tx_hash(&ob_id, tx_hash).await?; | ||
| if !trades.is_empty() { | ||
| return Ok(trades); | ||
| } | ||
| if attempt < attempts { | ||
| sleep_ms(interval_ms).await; | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Local DB path falls through to subgraph — violates the exclusive local DB pattern.
When is_chain_supported_local_db(chain_id) is true and local_db is Some, the inner loop exhausts all attempts and then silently falls through to the subgraph polling block at Line 108. This directly contradicts the established codebase design: when local DB is supported and available, it must be the sole data source and should return TradesIndexingTimeout itself rather than delegating to the subgraph.
The same attempts budget is then applied again to the subgraph phase, so a caller who configured max_attempts = 5 actually waits for up to 10 poll cycles without knowing it, and the resulting TradesIndexingTimeout reports 5 instead of the real poll count.
Additionally, all five tests use chain_id = 1, which is not a local DB supported chain, so this entire code path has zero test coverage.
🛡️ Proposed fix — return timeout from local DB phase instead of falling through
if is_chain_supported_local_db(chain_id) {
if let Some(local_db) = self.local_db() {
let local_source = LocalDbTrades::new(&local_db);
for attempt in 1..=attempts {
let trades = local_source.get_by_tx_hash(&ob_id, tx_hash).await?;
if !trades.is_empty() {
return Ok(trades);
}
if attempt < attempts {
sleep_ms(interval_ms).await;
}
}
+ return Err(RaindexError::TradesIndexingTimeout { tx_hash, attempts });
}
}Based on learnings: "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() and get_vaults_volume() 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."
🤖 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_tx.rs` around lines 93 - 106,
The local-DB branch currently falls through to the subgraph polling after
exhausting attempts; change it so that when
is_chain_supported_local_db(chain_id) is true and self.local_db() returns Some,
the function returns a TradesIndexingTimeout (or appropriate error) immediately
after the LocalDbTrades::new(...).get_by_tx_hash(...) loop finishes without
results instead of continuing to the subgraph block; specifically modify the
block containing LocalDbTrades::new, get_by_tx_hash, attempts and interval_ms to
return Err(TradesIndexingTimeout { attempts }) (or construct the same timeout
error used by the subgraph path) once attempts are exhausted so the local DB
remains the exclusive source. Ensure you reference and update the same error
type used later so callers see consistent timeout reporting.
| } | ||
| } | ||
|
|
||
| Err(RaindexError::TradesIndexingTimeout { tx_hash, attempts }) |
Contributor
There was a problem hiding this comment.
attempts in TradesIndexingTimeout under-reports actual poll count.
If the local DB phase runs to completion and then falls through to the subgraph phase (current behaviour), the total polls performed is 2 * attempts, but the error message says "not indexed after {attempts} attempts". Even if the local DB fallthrough is fixed, consider whether the attempts field in the error should carry the original user-configured value (fine for UX) or the actual attempts exhausted (more accurate for debugging).
This is low-impact if the major fallthrough issue above is resolved, but worth auditing the error field semantics now to avoid confusion in callers.
🤖 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_tx.rs` at line 130, The
TradesIndexingTimeout error currently receives the variable attempts which
under-reports the actual number of poll attempts when both the local DB and
subgraph phases run (total polls can be the sum of both phases); update
get_by_tx.rs so that before constructing Err(RaindexError::TradesIndexingTimeout
{ tx_hash, attempts }) you compute and pass an accurate attempts count (e.g.,
total_attempts = local_attempts + subgraph_attempts) or else pass both
configured_attempts and actual_attempts in the error variant; adjust the code
paths around the polling loops that use attempts (in the functions/blocks
responsible for local DB polling and subgraph polling in get_by_tx.rs) so the
value passed into TradesIndexingTimeout reflects your chosen semantics.
Comment on lines
99
to
108
| pub struct DeploymentTransactionArgs { | ||
| approvals: Vec<ExtendedApprovalCalldata>, | ||
| pub approvals: Vec<ExtendedApprovalCalldata>, | ||
| #[tsify(type = "string")] | ||
| deployment_calldata: Bytes, | ||
| pub deployment_calldata: Bytes, | ||
| #[tsify(type = "string")] | ||
| orderbook_address: Address, | ||
| chain_id: u32, | ||
| pub orderbook_address: Address, | ||
| pub chain_id: u32, | ||
| #[tsify(type = "ExternalCall | undefined")] | ||
| emit_meta_call: Option<ExternalCall>, | ||
| pub emit_meta_call: Option<ExternalCall>, | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM — field visibility change is clean and additive.
Making DeploymentTransactionArgs fields public is a non-breaking, additive change. Tsify/serde serialization is unaffected, so the WASM/TS API surface is unchanged.
One optional consideration: with pub mod order_operations in mod.rs, CalldataFunction (line 25) and VaultAndDeposit (lines 122–127) are now transitively reachable as gui::order_operations::CalldataFunction and gui::order_operations::VaultAndDeposit. Both are used only by private methods (prepare_calldata_generation, get_vaults_and_deposits) so they're not truly usable externally, but they appear in the module's public surface without docs or intent. If that's undesirable, scope those two types to pub(super) or pub(crate).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/js_api/src/gui/order_operations.rs` around lines 99 - 108, The module
export makes CalldataFunction and VaultAndDeposit transitively public via pub
mod order_operations; restrict their visibility to avoid unintentionally
expanding the public API by changing their declarations from pub to either
pub(super) or pub(crate) (whichever fits the intended encapsulation) so they
remain usable by prepare_calldata_generation and get_vaults_and_deposits but are
not exposed as part of the module's public surface.
Use direct transaction: String field on TradeEvent_filter instead of nesting into Transaction_filter, which the subgraph does not support.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/subgraph/src/orderbook_client/transaction.rs`:
- Around line 454-477: Add a request-body assertion to the
test_transaction_trades_found test to ensure the transaction ID is actually
sent: update the sg_server.mock setup inside test_transaction_trades_found (the
same block that calls when.method(POST).path("/") and then.status(200)...) to
include a .body_contains(format!("\"id\":\"{}\"", tx_id_str)) check so the mock
verifies the serialized id used by client.transaction_trades is correct.
In `@crates/subgraph/src/types/order_trade.rs`:
- Around line 42-49: SgTransactionTradesQuery is inconsistent with its siblings:
remove the unnecessary #[serde(rename_all = "camelCase")] and add Clone to its
derive list so it matches SgOrderTradesListQuery and SgOrderTradeDetailQuery;
update the struct derive to include Clone (e.g., derive(..., Clone, ...)) and
delete the spurious serde attribute above the SgTransactionTradesQuery
definition.
Comment on lines
+454
to
+477
| #[tokio::test] | ||
| async fn test_transaction_trades_found() { | ||
| let sg_server = MockServer::start_async().await; | ||
| let client = setup_client(&sg_server); | ||
| let tx_id_str = "0xtx_trades_1"; | ||
| let tx_id = Id::new(tx_id_str); | ||
| let expected_trades = vec![ | ||
| default_sg_trade(tx_id_str, "0xtrade1"), | ||
| default_sg_trade(tx_id_str, "0xtrade2"), | ||
| ]; | ||
|
|
||
| sg_server.mock(|when, then| { | ||
| when.method(POST).path("/"); | ||
| then.status(200) | ||
| .json_body(json!({"data": {"trades": expected_trades}})); | ||
| }); | ||
|
|
||
| let result = client.transaction_trades(tx_id).await; | ||
| assert!(result.is_ok(), "Result was: {:?}", result); | ||
| let trades = result.unwrap(); | ||
| assert_eq!(trades.len(), 2); | ||
| assert_eq!(trades[0].id.0, "0xtrade1"); | ||
| assert_eq!(trades[1].id.0, "0xtrade2"); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
test_transaction_trades_found doesn't assert the sent request body contains the transaction ID.
The sibling test_transaction_add_orders_found (line 281) uses .body_contains(format!("\"id\":\"{}\"", tx_id_str)) to verify the variable is correctly serialised and sent. This test omits that check, so a regression where the wrong or empty ID is passed to the query would go undetected.
♻️ Proposed fix
sg_server.mock(|when, then| {
- when.method(POST).path("/");
+ when.method(POST)
+ .path("/")
+ .body_contains(format!("\"id\":\"{}\"", tx_id_str));
then.status(200)
.json_body(json!({"data": {"trades": expected_trades}}));
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/subgraph/src/orderbook_client/transaction.rs` around lines 454 - 477,
Add a request-body assertion to the test_transaction_trades_found test to ensure
the transaction ID is actually sent: update the sg_server.mock setup inside
test_transaction_trades_found (the same block that calls
when.method(POST).path("/") and then.status(200)...) to include a
.body_contains(format!("\"id\":\"{}\"", tx_id_str)) check so the mock verifies
the serialized id used by client.transaction_trades is correct.
Comment on lines
+42
to
+49
| #[derive(cynic::QueryFragment, Debug, Serialize)] | ||
| #[cynic(graphql_type = "Query", variables = "TransactionTradesVariables")] | ||
| #[cfg_attr(target_family = "wasm", derive(Tsify))] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct SgTransactionTradesQuery { | ||
| #[arguments(where: { tradeEvent_: { transaction: $id } })] | ||
| pub trades: Vec<SgTrade>, | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clone missing and spurious #[serde(rename_all = "camelCase")] — inconsistent with sibling types.
Both SgOrderTradesListQuery (line 7) and SgOrderTradeDetailQuery (line 28) derive Clone, Serialize without a rename_all attribute. SgTransactionTradesQuery drops Clone and adds #[serde(rename_all = "camelCase")], which is a no-op here (the only field is trades, unchanged under camelCase).
♻️ Proposed fix
-#[derive(cynic::QueryFragment, Debug, Serialize)]
+#[derive(cynic::QueryFragment, Debug, Clone, Serialize)]
#[cynic(graphql_type = "Query", variables = "TransactionTradesVariables")]
#[cfg_attr(target_family = "wasm", derive(Tsify))]
-#[serde(rename_all = "camelCase")]
pub struct SgTransactionTradesQuery {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/subgraph/src/types/order_trade.rs` around lines 42 - 49,
SgTransactionTradesQuery is inconsistent with its siblings: remove the
unnecessary #[serde(rename_all = "camelCase")] and add Clone to its derive list
so it matches SgOrderTradesListQuery and SgOrderTradeDetailQuery; update the
struct derive to include Clone (e.g., derive(..., Clone, ...)) and delete the
spurious serde attribute above the SgTransactionTradesQuery definition.
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
1 participant
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 PR
Motivation
See issues:
Adds the ability to fetch all trades associated with a given transaction hash. This is needed to support transaction-level trade lookups in the REST API.
Solution
Adds a full-stack implementation for fetching trades by transaction hash, spanning subgraph, local DB, and public API layers:
crates/subgraph): Newtransaction_tradesquery onOrderbookSubgraphClientthat fetches trades filtered by transaction ID.crates/common/src/local_db/query/fetch_trades_by_tx): SQL query and builder for fetching trades by tx hash, chain ID, and orderbook address from the local SQLite DB.crates/common/src/raindex_client/local_db/trades.rs):LocalDbTradesstruct withget_by_tx_hashthat executes the query and converts results toRaindexTrade.crates/common/src/raindex_client/trades/get_by_tx.rs):get_trades_for_transactiononRaindexClientwith wasm binding (getTradesForTransaction), routing to local DB when supported or falling back to subgraph.Returns
Vec<RaindexTrade>, consistent with existing trade list APIs.Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Error Handling
Tests