Implement local db trades list to order details #2155
Conversation
WalkthroughAdds LocalDb-backed order trade retrieval and counting with new SQL queries and Rust models/APIs, integrates a LocalDb fast-path into trade listing/counting and conversion to existing Raindex types, and switches vault balance zero defaults to use FLOAT_ZERO_HEX(). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Order as RaindexOrder
participant LocalDb as LocalDbQuery
participant SG as SubgraphClient
Caller->>Order: get_trades_list(start?, end?)
alt LocalDb supported and callback provided
Order->>LocalDb: fetch_order_trades(chain_id, order_hash, start?, end?)
LocalDb-->>Order: Vec<LocalDbOrderTrade>
Order->>Order: map to Vec<RaindexTrade> (try_from_local_db_trade)
Order-->>Caller: trades (LocalDb)
else
Order->>SG: get_trades_list(order_id, start?, end?)
SG-->>Order: trades
Order-->>Caller: trades (SG)
end
sequenceDiagram
autonumber
actor Caller
participant Order as RaindexOrder
participant LocalDb as LocalDbQuery
participant SG as SubgraphClient
Caller->>Order: get_trade_count(start?, end?)
alt LocalDb supported and callback provided
Order->>LocalDb: fetch_order_trades_count(order_hash, start?, end?)
LocalDb-->>Order: u64 count
Order-->>Caller: count (LocalDb)
else
Order->>SG: get_trade_count(order_id, start?, end?)
SG-->>Order: count
Order-->>Caller: count (SG)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
crates/common/src/raindex_client/local_db/query/fetch_order_trades/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_order_trades/query.sql(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_order_trades_count/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_order_trades_count/query.sql(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault/query.sql(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault_balance_changes/query.sql(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_vaults/query.sql(1 hunks)crates/common/src/raindex_client/local_db/query/mod.rs(1 hunks)crates/common/src/raindex_client/orders.rs(2 hunks)crates/common/src/raindex_client/trades.rs(4 hunks)crates/common/src/raindex_client/vaults.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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
PR: rainlanguage/rain.orderbook#2145
File: crates/common/src/raindex_client/local_db/query/create_tables/query.sql:71-72
Timestamp: 2025-10-06T11:44:07.867Z
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
PR: rainlanguage/rain.orderbook#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.
📚 Learning: 2025-07-04T10:24:56.163Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
Applied to files:
crates/common/src/raindex_client/local_db/query/fetch_order_trades_count/mod.rscrates/common/src/raindex_client/trades.rscrates/common/src/raindex_client/local_db/query/fetch_order_trades_count/query.sql
📚 Learning: 2025-10-06T11:13:29.938Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2123
File: crates/common/src/raindex_client/local_db/mod.rs:23-29
Timestamp: 2025-10-06T11:13:29.938Z
Learning: In `crates/common/src/raindex_client/local_db/mod.rs`, the `Default` implementation for `LocalDb` that creates an RPC client pointing to `http://localhost:4444` is acceptable because the RPC client must be explicitly configured before actual usage in production scenarios.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 Learning: 2025-10-06T11:44:07.867Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2145
File: crates/common/src/raindex_client/local_db/query/create_tables/query.sql:71-72
Timestamp: 2025-10-06T11:44:07.867Z
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`.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 Learning: 2025-04-29T06:19:15.446Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
Applied to files:
crates/common/src/raindex_client/orders.rs
🔇 Additional comments (12)
crates/common/src/raindex_client/local_db/query/fetch_vaults/query.sql (1)
69-69: LGTM! Consistent with related changes.The change to use
FLOAT_ZERO_HEX()aligns with the same update in the Rust constantBALANCE_EXPR(mod.rs line 84) and other vault balance queries in this PR.crates/common/src/raindex_client/local_db/query/fetch_vault/query.sql (1)
69-69: LGTM! Consistent with related changes.The use of
FLOAT_ZERO_HEX()is consistent with the similar changes across vault balance queries in this PR.crates/common/src/raindex_client/local_db/query/fetch_vault_balance_changes/query.sql (1)
13-14: LGTM! Consistent with related changes.The change to
FLOAT_ZERO_HEX()for the running balance default aligns with the pattern applied across all vault balance queries in this PR.crates/common/src/raindex_client/local_db/query/fetch_order_trades/mod.rs (3)
6-58: LGTM! Comprehensive trade data model.The
LocalDbOrderTradestruct is well-structured with appropriate field types and serde aliases for JSON deserialization. The mix of required and optional fields correctly reflects that token metadata may not always be available.
68-70: Basic SQL injection prevention is adequate for local DB context.The
sanitize_literalclosure only escapes single quotes, which is minimal but acceptable since:
- This is a local database in a WASM environment
- The
order_hashis the only user-controlled string input- Other parameters (
chain_id, timestamps) are strongly typed primitivesHowever, consider documenting this security assumption if the local DB handling changes in the future.
101-141: LGTM! Tests verify core functionality.The tests appropriately cover:
- JSON deserialization into
LocalDbOrderTrade- SQL placeholder replacement including case normalization
- Timestamp filter injection
crates/common/src/raindex_client/local_db/query/fetch_order_trades/query.sql (6)
26-26: LGTM! Trade ID computation looks correct.The trade_id is computed as
'0x' || lower(replace(transaction_hash, '0x', '')) || printf('%016x', log_index), which creates a unique identifier by concatenating the normalized transaction hash with the zero-padded log index.
30-30: Verify hardcoded orderbook address is intentional.The orderbook address
0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eBis hardcoded in all three UNION branches. This prevents the query from working with other orderbook deployments.Is this hardcoded address intentional for the local DB snapshot, or should it be parameterized like
order_hashandchain_id?If this local DB is intended to support only a specific orderbook instance, consider adding a comment documenting this assumption. Otherwise, consider adding an
orderbook_addressparameter to thefetch_order_tradesfunction.Also applies to: 144-144, 258-258
50-66: Running balance calculation logic appears correct.The running balance subquery correctly:
- Sums all
vault_deltasup to and including the current event- Filters by owner, token, and vault_id
- Uses block_number and log_index for ordering
- Returns
FLOAT_ZERO_HEX()as default when no deltas exist- Returns NULL when vault_deltas record is not available (ELSE NULL on line 65)
The same pattern is consistently applied to both input and output balances across all three UNION branches.
367-371: LGTM! Final filtering and ordering are appropriate.The query correctly:
- Uses placeholder replacement for optional timestamp filters
- Orders by timestamp, block_number, and log_index in descending order (most recent first)
The
WHERE 1=1pattern (line 368) is a common technique to simplify optional filter injection.
163-163: Fields exist:after_clear_v2_eventsincludesalice_inputandalice_output.
Schema in crates/common/src/raindex_client/local_db/query/create_tables/query.sql (lines 125–128) confirms both columns.
49-66: Verify custom SQL functions are registered. I couldn’t locate any binding forFLOAT_SUM,FLOAT_ZERO_HEX, orFLOAT_NEGATEin thelocal_dbsetup—ensure they’re registered in the WASM SQLite environment.
| let sql = QUERY | ||
| .replace("'?order_hash'", &format!("'{}'", order_hash)) | ||
| .replace("'?chain_id'", &chain_id.to_string()) | ||
| .replace("?filter_start_timestamp", &filter_start_timestamp) | ||
| .replace("?filter_end_timestamp", &filter_end_timestamp); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Verify SQL placeholder replacement pattern consistency.
The placeholder replacement uses different quoting patterns:
- Line 80:
'?order_hash'→'{}'(quotes in both pattern and replacement) - Line 81:
'?chain_id'→{}(quotes only in pattern) - Lines 82-83:
?filter_start_timestamp→ filter string (no quotes in pattern)
This inconsistency could lead to SQL syntax errors if not carefully managed.
Run this script to check if other query modules follow the same pattern:
🏁 Script executed:
#!/bin/bash
# Check placeholder patterns in other local_db query modules
fd -e sql -e rs . crates/common/src/raindex_client/local_db/query/ --exec rg -n "replace\(" {} \; | head -50Length of output: 2071
Extract a helper for SQL placeholder replacement Consolidate the varying .replace() patterns ('?param' vs ?param) and apply quoting consistently via a type-aware utility to remove ad-hoc formatting.
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/local_db/query/fetch_order_trades/mod.rs
around lines 79 to 83, the SQL string is built with multiple ad-hoc .replace()
calls that mix quoted ('?param') and unquoted (?param) patterns and perform
manual quoting/formatting; extract a small helper to centralize placeholder
replacement and quoting (e.g., replace_placeholder(sql: &str, name: &str, value:
impl ToSqlLiteral)) that accepts a parameter name and a type-aware value,
escapes and quotes strings, leaves numeric/boolean types unquoted, and replaces
both "'?name'" and "?name" occurrences consistently; then call that helper for
order_hash, chain_id, filter_start_timestamp, filter_end_timestamp to remove
inline formatting and ensure correct, safe SQL literal insertion.
There was a problem hiding this comment.
this is fine for now
There was a problem hiding this comment.
@findolor, understood! I've noted your decision. Feel free to revisit this optimization if needed in the future.
| }; | ||
|
|
||
| const BALANCE_EXPR: &str = "COALESCE((\n SELECT FLOAT_SUM(vd.delta)\n FROM vault_deltas vd\n WHERE vd.owner = o.owner\n AND vd.token = o.token\n AND vd.vault_id = o.vault_id\n ), '0x0000000000000000000000000000000000000000000000000000000000000000')"; | ||
| const BALANCE_EXPR: &str = "COALESCE((\n SELECT FLOAT_SUM(vd.delta)\n FROM vault_deltas vd\n WHERE vd.owner = o.owner\n AND vd.token = o.token\n AND vd.vault_id = o.vault_id\n ), FLOAT_ZERO_HEX())"; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that FLOAT_ZERO_HEX() is defined and returns the expected zero value.
The change replaces a literal zero hex string with a call to FLOAT_ZERO_HEX(). Ensure that this function is properly defined in your SQL environment and returns a value semantically equivalent to the previous literal zero hex string (e.g., '0x0000000000000000000000000000000000000000000000000000000000000000').
Run the following script to search for the definition or registration of FLOAT_ZERO_HEX():
🏁 Script executed:
#!/bin/bash
# Description: Search for FLOAT_ZERO_HEX function definition or registration.
# Search for function definition/registration in Rust code
rg -nP --type=rust -C3 'FLOAT_ZERO_HEX|float_zero_hex'
# Search for SQL function definitions
rg -nP --type=sql -C3 'CREATE\s+FUNCTION.*FLOAT_ZERO_HEX|FLOAT_ZERO_HEX'Length of output: 8022
Missing SQL function FLOAT_ZERO_HEX(): No definition found in the codebase; add or register this function (e.g., in your SQL migrations) to return the expected zero-hex literal so COALESCE(..., FLOAT_ZERO_HEX()) works as intended.
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs around
line 84, the SQL uses FLOAT_ZERO_HEX() but there is no function definition or
registration for it; add a SQL migration (or DB initialization step) that
creates/defines the FLOAT_ZERO_HEX() function to return the expected zero-hex
floating literal (matching the DB engine's function signature and return type)
and ensure that migration is applied/registered on startup so COALESCE(...,
FLOAT_ZERO_HEX()) resolves correctly.
There was a problem hiding this comment.
it's fine. this function is available in the db lib sdk
There was a problem hiding this comment.
@findolor, thank you for the clarification! I understand that FLOAT_ZERO_HEX() is provided by the database library SDK as an external dependency.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2155
File: crates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs:84-84
Timestamp: 2025-10-06T14:12:34.855Z
Learning: The SQL function FLOAT_ZERO_HEX() is available in the db lib SDK and does not need to be defined in the codebase. It is used in SQL queries as an external function provided by the database library.
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.
| let chain_id = self.chain_id(); | ||
| if LocalDb::check_support(chain_id) { | ||
| let raindex_client = self.get_raindex_client(); | ||
| if let Some(db_cb) = raindex_client.local_db_callback() { | ||
| let order_hash = self.order_hash().to_string(); | ||
| let local_trades = LocalDbQuery::fetch_order_trades( | ||
| &db_cb, | ||
| chain_id, | ||
| &order_hash, | ||
| start_timestamp, | ||
| end_timestamp, | ||
| ) | ||
| .await?; | ||
|
|
||
| let trades = local_trades | ||
| .into_iter() | ||
| .map(|trade| RaindexTrade::try_from_local_db_trade(chain_id, trade)) | ||
| .collect::<Result<Vec<RaindexTrade>, RaindexError>>()?; | ||
|
|
||
| return Ok(trades); |
There was a problem hiding this comment.
Restore pagination semantics for the LocalDb fast-path.
get_trades_list still accepts page, but the LocalDb branch now calls fetch_order_trades without any pagination inputs, so it returns the entire trade set. The subgraph path continues to respect page + DEFAULT_PAGE_SIZE. This discrepancy breaks the existing contract and can dump unbounded data for orders with many trades. Please wire pagination (e.g., pass page/limit into the LocalDb query or fall back to the remote path unless page/limit are honored).
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/trades.rs around lines 133 to 152, the
LocalDb fast-path ignores pagination and returns all trades; reintroduce
pagination by wiring the page and limit into the LocalDb query: compute offset =
(page.saturating_sub(1)) * page_size (use DEFAULT_PAGE_SIZE if limit not
provided) and pass offset/limit (or page/limit) into
LocalDbQuery::fetch_order_trades so it returns only that window; if LocalDbQuery
cannot support pagination, explicitly fall back to the remote/subgraph path when
pagination is requested or return an error indicating pagination is unsupported,
ensuring the function honors the same page + DEFAULT_PAGE_SIZE contract as the
subgraph branch.
There was a problem hiding this comment.
there is no need to do this. page is there is keep backwards compatibility with subgraph logic
There was a problem hiding this comment.
@findolor, understood! I've noted that the page parameter is maintained for backwards compatibility with the subgraph logic, and the LocalDb path intentionally returns all trades without pagination.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2155
File: crates/common/src/raindex_client/trades.rs:133-152
Timestamp: 2025-10-06T14:13:18.506Z
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.
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
PR: rainlanguage/rain.orderbook#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.
| const QUERY: &str = include_str!("query.sql"); | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct LocalDbOrderTrade { |
There was a problem hiding this comment.
looks like
- we don't need the alias stuff everywhere since it's already snake case
- consider using
- an enum for trade_kind
- Address for all address fields (orderbook_address, order_owner, transaction_sender, input_token, output_token)
- B256 for 32-byte values (order_hash, transaction_hash, input_vault_id, output_vault_id, trade_id)
- Float for amounts (input_delta, input_running_balance, output_delta, output_running_balance)
There was a problem hiding this comment.
will be getting back to this with this issue #2182
| }; | ||
|
|
||
| const BALANCE_EXPR: &str = "COALESCE((\n SELECT FLOAT_SUM(vd.delta)\n FROM vault_deltas vd\n WHERE vd.owner = o.owner\n AND vd.token = o.token\n AND vd.vault_id = o.vault_id\n ), '0x0000000000000000000000000000000000000000000000000000000000000000')"; | ||
| const BALANCE_EXPR: &str = "COALESCE((\n SELECT FLOAT_SUM(vd.delta)\n FROM vault_deltas vd\n WHERE vd.owner = o.owner\n AND vd.token = o.token\n AND vd.vault_id = o.vault_id\n ), FLOAT_ZERO_HEX())"; |
There was a problem hiding this comment.
nitpick: would be more readable if broken down into multiple lines
| chain_id, | ||
| // TODO: Needs updating | ||
| id: Bytes::from_str("0x01")?, | ||
| id: Bytes::from(keccak256(&id).as_slice().to_vec()), |
There was a problem hiding this comment.
same comment about the id hashing as from the other pr
There was a problem hiding this comment.
this is fixed in upstream
| let mut id = Vec::with_capacity(order.orderbook_address.len() + order.order_hash.len()); | ||
| id.extend_from_slice(order.orderbook_address.as_bytes()); | ||
| id.extend_from_slice(order.order_hash.as_bytes()); |
There was a problem hiding this comment.
| let mut id = Vec::with_capacity(order.orderbook_address.len() + order.order_hash.len()); | |
| id.extend_from_slice(order.orderbook_address.as_bytes()); | |
| id.extend_from_slice(order.order_hash.as_bytes()); | |
| let id = [order.orderbook_address.as_bytes(), order.order_hash.as_bytes()] | |
| .concat(); |
There was a problem hiding this comment.
might be good to add some test coverage for the new logic in this module
| }) | ||
| } | ||
|
|
||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
imo it'd be better to refactor the function than to disable the warning. token stuff can be put in a struct and then you have four fewer args already
There was a problem hiding this comment.
might be good to add test coverage for this function as well
There was a problem hiding this comment.
will update it accordingly
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/common/src/raindex_client/orders.rs (1)
918-927: Fix local order ID hashing to use decoded bytes.Hashing
order.orderbook_address.as_bytes()+order.order_hash.as_bytes()feeds the ASCII text (including"0x"and casing) into keccak, so the ID diverges from the subgraph’s ID, breaking parity across data sources. Decode the address/hash to raw bytes before hashing so both pathways yield the same 32-byte ID.Apply this diff:
- let id = [ - order.orderbook_address.as_bytes(), - order.order_hash.as_bytes(), - ] - .concat(); - Ok(Self { raindex_client: Rc::clone(&raindex_client), chain_id, - id: Bytes::from(keccak256(&id).as_slice().to_vec()), - order_bytes: Bytes::from_str(&order.order_bytes)?, - order_hash: Bytes::from_str(&order.order_hash)?, + id: { + let orderbook_address = Address::from_str(&order.orderbook_address)?; + let order_hash_bytes = Bytes::from_str(&order.order_hash)?; + let mut preimage = Vec::with_capacity(20 + order_hash_bytes.len()); + preimage.extend_from_slice(orderbook_address.as_slice()); + preimage.extend_from_slice(order_hash_bytes.as_slice()); + Bytes::from(keccak256(preimage).as_slice().to_vec()) + }, + order_bytes: Bytes::from_str(&order.order_bytes)?, + order_hash: Bytes::from_str(&order.order_hash)?, owner: Address::from_str(&order.owner)?, @@ - orderbook: Address::from_str(&order.orderbook_address)?, + orderbook: Address::from_str(&order.orderbook_address)?,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
crates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs(1 hunks)crates/common/src/raindex_client/orders.rs(2 hunks)crates/common/src/raindex_client/trades.rs(4 hunks)crates/common/src/raindex_client/vaults.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
crates/*/{src,tests,benches,examples}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Rust code must pass clippy with all warnings denied (cargo clippy --workspace --all-targets --all-features -D warnings)
Files:
crates/common/src/raindex_client/vaults.rscrates/common/src/raindex_client/trades.rscrates/common/src/raindex_client/orders.rscrates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs
crates/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rust code lives in the workspace under crates/* (e.g., cli, common, bindings, js_api, quote, subgraph, settings, math, integration_tests)
Files:
crates/common/src/raindex_client/vaults.rscrates/common/src/raindex_client/trades.rscrates/common/src/raindex_client/orders.rscrates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust with nix develop -c cargo fmt --all
Lint Rust with nix develop -c rainix-rs-static (preconfigured flags)
Use snake_case for Rust crates/modules and PascalCase for types
Files:
crates/common/src/raindex_client/vaults.rscrates/common/src/raindex_client/trades.rscrates/common/src/raindex_client/orders.rscrates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs
🧠 Learnings (8)
📓 Common learnings
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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
PR: rainlanguage/rain.orderbook#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
PR: rainlanguage/rain.orderbook#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`.
📚 Learning: 2025-10-06T14:13:18.531Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 Learning: 2025-07-04T10:24:56.163Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 Learning: 2025-10-02T19:17:20.332Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 Learning: 2025-07-04T10:27:22.544Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 Learning: 2025-07-15T08:01:38.534Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1994
File: crates/common/src/raindex_client/vaults.rs:282-292
Timestamp: 2025-07-15T08:01:38.534Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor prefers to avoid concurrency optimizations like using `futures::future::try_join_all` for parallel processing of balance changes, considering such optimizations "not that critical at the moment" when the performance impact is minimal.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 Learning: 2025-04-29T06:19:15.446Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
Applied to files:
crates/common/src/raindex_client/orders.rs
📚 Learning: 2025-10-06T14:12:34.864Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2155
File: crates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs:84-84
Timestamp: 2025-10-06T14:12:34.864Z
Learning: The SQL function FLOAT_ZERO_HEX() is available in the db lib SDK and does not need to be defined in the codebase. It is used in SQL queries as an external function provided by the database library.
Applied to files:
crates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs
🧬 Code graph analysis (3)
crates/common/src/raindex_client/vaults.rs (1)
crates/common/src/raindex_client/transactions.rs (7)
from(27-29)from(46-48)from_local_parts(58-70)timestamp(36-39)timestamp(52-54)id(23-25)id(43-45)
crates/common/src/raindex_client/trades.rs (6)
crates/common/src/raindex_client/local_db/query/fetch_order_trades/mod.rs (2)
fetch_order_trades(61-86)LocalDbQuery(85-85)crates/common/src/raindex_client/local_db/query/fetch_order_trades_count/mod.rs (2)
LocalDbQuery(35-35)fetch_order_trades_count(13-39)crates/common/src/raindex_client/vaults.rs (43)
vaults(1158-1179)chain_id(87-89)chain_id(134-136)chain_id(228-230)chain_id(255-257)orderbook(119-121)orderbook(158-160)orderbook(667-669)orderbook(706-708)transaction(663-665)transaction(703-705)try_from_local_trade_side(832-882)new(177-182)vault(1454-1458)vault(1459-1463)vault_id(103-105)vault_id(146-148)vault_id(625-628)vault_id(676-678)id(95-97)id(140-142)id(232-234)id(258-260)id(897-900)id(912-914)timestamp(658-661)timestamp(700-702)amount(634-636)amount(682-684)new_balance(642-644)new_balance(688-690)old_balance(650-652)old_balance(694-696)token(115-117)token(155-157)token(630-632)token(679-681)token(902-904)token(915-917)symbol(244-246)symbol(267-269)address(236-238)address(261-263)crates/common/src/raindex_client/local_db/mod.rs (1)
check_support(198-200)crates/common/src/raindex_client/transactions.rs (9)
from_local_parts(58-70)from(27-29)from(46-48)id(23-25)id(43-45)block_number(31-34)block_number(49-51)timestamp(36-39)timestamp(52-54)crates/common/src/raindex_client/mod.rs (8)
from(238-242)from(320-322)from(326-328)from(447-449)from(453-458)get_local_db_test_yaml(544-588)new_test_client_with_db_callback(534-541)new(111-132)
crates/common/src/raindex_client/orders.rs (4)
crates/js_api/src/bindings/mod.rs (1)
keccak256(94-99)crates/common/src/raindex_client/trades.rs (2)
id(38-40)id(69-71)crates/common/src/raindex_client/vaults.rs (6)
id(95-97)id(140-142)id(232-234)id(258-260)id(897-900)id(912-914)crates/common/src/raindex_client/transactions.rs (2)
id(23-25)id(43-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: git-clean
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: test
- GitHub Check: Deploy-Docs-Preview
- GitHub Check: Deploy-Preview-Push
🔇 Additional comments (4)
crates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs (1)
84-91: LGTM! Consistent with broader vault query updates.The switch to
FLOAT_ZERO_HEX()as the COALESCE fallback is correct and aligns with parallel updates in other vault-related SQL files. The SQL logic properly sums vault deltas with a zero default. Based on learnings,FLOAT_ZERO_HEX()is provided by the database library SDK.Optional nitpick: The raw string delimiter change from
r"..."tor#"..."#appears unnecessary since the content doesn't contain quote characters, but it's harmless and doesn't affect functionality.crates/common/src/raindex_client/vaults.rs (3)
711-717: LGTM! Clean struct definition.The struct appropriately models token information with optional fields for metadata that may not always be available.
719-723: LGTM! Appropriate structure.The optional
running_balancecorrectly handles cases where cumulative balance tracking may not be available.
832-882: Verify timestamp handling.The implementation looks correct overall. However, the separate
block_timestampparameter could lead to inconsistency withtransaction.timestamp(). In the test (lines 1886-1892), the transaction is created with timestamp 456, but the function is called withblock_timestamp789, and the result uses 789 (line 1935).Consider whether the function should use
transaction.timestamp()instead of a separate parameter to ensure consistency and reduce the parameter count from 7 to 6.Please clarify:
- Why is
block_timestampseparate fromtransaction.timestamp()?- Should they always match, or are there valid cases where they differ?
If they should always match, refactor to eliminate the parameter:
pub(crate) fn try_from_local_trade_side( chain_id: u32, orderbook: Address, transaction: &RaindexTransaction, vault_id: &str, token: LocalTradeTokenInfo, balance: LocalTradeBalanceInfo, - block_timestamp: u64, ) -> Result<Self, RaindexError> { // ... existing code ... Ok(Self { r#type: RaindexVaultBalanceChangeType::TradeVaultBalanceChange, vault_id: U256::from_str(vault_id)?, token, amount, formatted_amount, new_balance, formatted_new_balance, old_balance, formatted_old_balance, - timestamp: U256::from(block_timestamp), + timestamp: transaction.timestamp(), transaction: transaction.clone(), orderbook, }) }
| )\n", | ||
| expr = BALANCE_EXPR, | ||
| ) | ||
| format!("\nAND NOT FLOAT_IS_ZERO({expr})\n", expr = BALANCE_EXPR) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good refactor for idiomatic Rust.
The single-line format!() macro with the named expr argument is cleaner and more idiomatic than the previous multi-line string assembly. The logic correctly filters vaults with non-zero balances.
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs around
line 94, the single-line format! macro using the named expr argument is already
the preferred, idiomatic form and correctly implements the non-zero balance
filter; no code changes are required—keep the current format!("\nAND NOT
FLOAT_IS_ZERO({expr})\n", expr = BALANCE_EXPR) as-is.
| #[test] | ||
| fn test_try_from_local_trade_side_with_running_balance() { | ||
| let chain_id = 42161; | ||
| let orderbook = | ||
| Address::from_str("0x0000000000000000000000000000000000000001").unwrap(); | ||
| let transaction = RaindexTransaction::from_local_parts( | ||
| "0xdeadbeef", | ||
| "0x0000000000000000000000000000000000000002", | ||
| 123, | ||
| 456, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let amount = Float::parse("1".to_string()).unwrap(); | ||
| let amount_hex = amount.as_hex(); | ||
| let new_balance = Float::parse("5".to_string()).unwrap(); | ||
| let new_balance_hex = new_balance.as_hex(); | ||
| let expected_old_balance = Float::parse("4".to_string()).unwrap(); | ||
|
|
||
| let change = RaindexVaultBalanceChange::try_from_local_trade_side( | ||
| chain_id, | ||
| orderbook, | ||
| &transaction, | ||
| "0x10", | ||
| LocalTradeTokenInfo { | ||
| address: "0x0000000000000000000000000000000000000003".to_string(), | ||
| name: Some("Token In".to_string()), | ||
| symbol: Some("TIN".to_string()), | ||
| decimals: Some(6), | ||
| }, | ||
| LocalTradeBalanceInfo { | ||
| delta: amount_hex.clone(), | ||
| running_balance: Some(new_balance_hex.clone()), | ||
| }, | ||
| 789, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!( | ||
| change.r#type(), | ||
| RaindexVaultBalanceChangeType::TradeVaultBalanceChange | ||
| ); | ||
| assert_eq!(change.vault_id(), U256::from_str("0x10").unwrap()); | ||
| assert!(change.amount().eq(amount).unwrap()); | ||
| assert!(change.new_balance().eq(new_balance).unwrap()); | ||
| assert_eq!(change.formatted_amount(), amount.format().unwrap()); | ||
| assert_eq!( | ||
| change.formatted_new_balance(), | ||
| new_balance.format().unwrap() | ||
| ); | ||
| assert_eq!( | ||
| change.formatted_old_balance(), | ||
| expected_old_balance.format().unwrap() | ||
| ); | ||
| assert_eq!(change.timestamp(), U256::from(789)); | ||
| assert_eq!(change.orderbook(), orderbook); | ||
| assert_eq!(change.transaction().id(), transaction.id()); | ||
|
|
||
| let token = change.token(); | ||
| assert_eq!(token.chain_id(), chain_id); | ||
| assert_eq!( | ||
| token.address(), | ||
| Address::from_str("0x0000000000000000000000000000000000000003").unwrap() | ||
| ); | ||
| assert_eq!(token.decimals(), 6); | ||
| assert_eq!(token.name(), Some("Token In".to_string())); | ||
| assert_eq!(token.symbol(), Some("TIN".to_string())); | ||
| assert_eq!( | ||
| token.id(), | ||
| "0x0000000000000000000000000000000000000003".to_string() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_try_from_local_trade_side_defaults() { | ||
| let chain_id = 1; | ||
| let orderbook = | ||
| Address::from_str("0x0000000000000000000000000000000000000004").unwrap(); | ||
| let transaction = RaindexTransaction::from_local_parts( | ||
| "0xfeedface", | ||
| "0x0000000000000000000000000000000000000005", | ||
| 111, | ||
| 222, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let amount = Float::parse("2".to_string()).unwrap(); | ||
| let amount_hex = amount.as_hex(); | ||
| let zero = Float::parse("0".to_string()).unwrap(); | ||
|
|
||
| let change = RaindexVaultBalanceChange::try_from_local_trade_side( | ||
| chain_id, | ||
| orderbook, | ||
| &transaction, | ||
| "0x20", | ||
| LocalTradeTokenInfo { | ||
| address: "0x0000000000000000000000000000000000000006".to_string(), | ||
| name: None, | ||
| symbol: None, | ||
| decimals: None, | ||
| }, | ||
| LocalTradeBalanceInfo { | ||
| delta: amount_hex.clone(), | ||
| running_balance: None, | ||
| }, | ||
| 333, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| assert!(change.amount().eq(amount).unwrap()); | ||
| assert!(change.new_balance().eq(amount).unwrap()); | ||
| assert!(change.old_balance().eq(zero).unwrap()); | ||
| assert_eq!(change.formatted_amount(), amount.format().unwrap()); | ||
| assert_eq!(change.formatted_new_balance(), amount.format().unwrap()); | ||
| assert_eq!(change.formatted_old_balance(), zero.format().unwrap()); | ||
|
|
||
| let token = change.token(); | ||
| assert_eq!(token.decimals(), 18); | ||
| assert!(token.name().is_none()); | ||
| assert!(token.symbol().is_none()); | ||
| assert_eq!( | ||
| token.address(), | ||
| Address::from_str("0x0000000000000000000000000000000000000006").unwrap() | ||
| ); | ||
| assert_eq!( | ||
| token.id(), | ||
| "0x0000000000000000000000000000000000000006".to_string() | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Comprehensive test coverage.
The tests thoroughly verify the function's behavior with both provided and defaulted values. Good coverage of field mapping and balance calculations.
Consider adding a test for negative amounts (e.g., output vaults in trades) to verify the balance calculation works correctly when tokens are withdrawn:
#[test]
fn test_try_from_local_trade_side_with_negative_amount() {
let chain_id = 42161;
let orderbook = Address::from_str("0x0000000000000000000000000000000000000001").unwrap();
let transaction = RaindexTransaction::from_local_parts(
"0xdeadbeef",
"0x0000000000000000000000000000000000000002",
123,
456,
).unwrap();
let amount = Float::parse("-2".to_string()).unwrap();
let new_balance = Float::parse("3".to_string()).unwrap();
let expected_old_balance = Float::parse("5".to_string()).unwrap(); // 3 - (-2) = 5
let change = RaindexVaultBalanceChange::try_from_local_trade_side(
chain_id,
orderbook,
&transaction,
"0x10",
LocalTradeTokenInfo {
address: "0x0000000000000000000000000000000000000003".to_string(),
name: Some("Token Out".to_string()),
symbol: Some("TOUT".to_string()),
decimals: Some(18),
},
LocalTradeBalanceInfo {
delta: amount.as_hex(),
running_balance: Some(new_balance.as_hex()),
},
789,
).unwrap();
assert!(change.amount().eq(amount).unwrap());
assert!(change.new_balance().eq(new_balance).unwrap());
assert!(change.old_balance().eq(expected_old_balance).unwrap());
}🤖 Prompt for AI Agents
In crates/common/src/raindex_client/vaults.rs around lines 1881 to 2009, add a
new unit test to cover negative delta values: create a test similar to existing
ones that builds a RaindexTransaction, parses a negative Float amount (e.g.,
"-2"), constructs a LocalTradeBalanceInfo with delta set to amount.as_hex() and
running_balance set to a positive new balance, call
RaindexVaultBalanceChange::try_from_local_trade_side, and assert
change.amount(), change.new_balance(), and change.old_balance() match amount,
new_balance, and new_balance - amount respectively (use Float.eq and format
checks where appropriate), and also verify token metadata like
decimals/name/symbol/address as in other tests.
|
@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:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=L 🧠 Learnings used |
3 participants
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit