Introduce SqlStament and refactor queries#2256
Conversation
WalkthroughReplaces raw SQL strings with a SqlStatement abstraction (SqlStatement/SqlValue/SqlBuildError), rewrites many query builders to produce parameterized statements, and updates LocalDbQueryExecutor and native/WASM executors to accept &SqlStatement and bind/serialize parameters before execution. Tests and mocks updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,240,255,0.3)
participant Caller
participant Builder
participant SqlStatement
end
rect rgba(240,255,220,0.3)
participant Executor
participant DB
end
Caller->>Builder: build_*_stmt(args)
Builder->>SqlStatement: SqlStatement::new(template) + push/bind_*
Builder-->>Caller: Ok(SqlStatement)
Caller->>Executor: query_json(&stmt) / query_text(&stmt)
alt stmt.params is empty
Executor->>DB: execute_batch(stmt.sql)
else params present
Executor->>DB: prepare(stmt.sql)
Executor->>DB: bind_parameters(stmt.params)
Executor->>DB: step/execute
end
DB-->>Executor: rows/result
Executor-->>Caller: Result<T, Error>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested labels
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: 39
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
crates/common/src/raindex_client/orders.rs (1)
997-1012: Avoid substring matching on params; parse JsValue → SqlValue[].String-searching the serialized params (
params_json.contains(needle)) is brittle. ParseparamsintoVec<SqlValue>and compare text values exactly; also store the raw vault_id (no extra quotes).Apply this refactor:
- // Match on the vault_id value passed as a parameter to the query. - // We will check this against the serialized params JSON. - let lookup = format!("\"{}\"", vault.vault_id); + // Match on the exact vault_id param value bound by the query. + let lookup = vault.vault_id.clone(); @@ - if sql.contains("FLOAT_SUM(vd.delta)") { - // Serialize params and try to match against captured vault ids - let params_json = js_sys::JSON::stringify(¶ms) - .unwrap() - .as_string() - .unwrap_or_default(); - for (needle, payload) in &vault_payloads { - if params_json.contains(needle) { - return js_sys::JSON::parse(payload).unwrap(); - } - } - } + if sql.contains("FLOAT_SUM(vd.delta)") { + // Parse params and match against captured vault ids + let params_vec: Vec<crate::local_db::query::SqlValue> = + serde_wasm_bindgen::from_value(params.clone()).unwrap_or_default(); + for (needle, payload) in &vault_payloads { + let matched = params_vec.iter().any(|p| match p { + crate::local_db::query::SqlValue::Text(s) => s == needle, + _ => false, + }); + if matched { + return js_sys::JSON::parse(payload).unwrap(); + } + } + }Also applies to: 1014-1036
crates/common/src/local_db/query/fetch_order_trades/query.sql (1)
247-250: Critical: Replace quoted string literals with positional parameter.Lines 247 and 250 still use
'?chain_id'as quoted string literals instead of the positional parameter?2. This is inconsistent with the rest of the file (lines 134, 137, 361, 364) where?2is correctly used for chain_id binding.This will cause the query to fail parameter binding for the alice clear trades branch, leading to incorrect results or runtime errors.
Apply this diff to fix the parameter binding:
LEFT JOIN erc20_tokens et_in - ON et_in.chain_id = '?chain_id' + ON et_in.chain_id = ?2 AND lower(et_in.address) = lower(io_in.token) LEFT JOIN erc20_tokens et_out - ON et_out.chain_id = '?chain_id' + ON et_out.chain_id = ?2 AND lower(et_out.address) = lower(io_out.token)crates/common/src/raindex_client/local_db/query/fetch_orders.rs (1)
157-175: Invalid JSON test: assert params tooMirror the params assertion to tighten guarantees.
Apply this diff:
- assert_eq!(store.borrow().clone().0, expected_stmt.sql); + let (captured_sql, captured_params) = store.borrow().clone(); + assert_eq!(captured_sql, expected_stmt.sql); + let expected_params = serde_wasm_bindgen::to_value(&expected_stmt.params).unwrap(); + assert_eq!(captured_params, expected_params);crates/cli/src/commands/local_db/executor.rs (1)
56-66: Avoid duplicate JSON keys for duplicate/blank column names.Duplicate/blank column names overwrite earlier entries in the row map. Ensure uniqueness by suffixing duplicates.
- let column_names: Vec<String> = (0..s.column_count()) - .map(|i| { - let raw = s.column_name(i).unwrap_or(""); - let trimmed = raw.trim(); - if trimmed.is_empty() { - format!("column_{}", i) - } else { - trimmed.to_string() - } - }) - .collect(); + let mut seen = std::collections::HashSet::new(); + let column_names: Vec<String> = (0..s.column_count()) + .map(|i| { + let raw = s.column_name(i).unwrap_or(""); + let mut name = raw.trim().to_string(); + if name.is_empty() { + name = format!("column_{}", i); + } + if !seen.insert(name.clone()) { + name = format!("{}_{}", name, i); + // best-effort; second insert can't fail due to index suffix + let _ = seen.insert(name.clone()); + } + name + }) + .collect();crates/common/src/local_db/sync.rs (2)
366-391: MockDb ignores params; can mask binding bugs.Map JSON/text fixtures by
(sql, params)to ensure tests fail when parameters change.Minimal change:
- json_map: std::collections::HashMap<String, String>, - text_map: std::collections::HashMap<String, String>, + json_map: std::collections::HashMap<(String, Vec<SqlValue>), String>, + text_map: std::collections::HashMap<(String, Vec<SqlValue>), String>,Add helper:
impl MockDb { fn key(stmt: &SqlStatement) -> (String, Vec<SqlValue>) { (stmt.sql.clone(), stmt.params.clone()) } fn with_json_stmt(mut self, stmt: &SqlStatement, value: &str) -> Self { self.json_map.insert(Self::key(stmt), value.to_string()); self } }And in impl:
- let sql = &stmt.sql; - if self.err_on.contains(sql) { ... } - let Some(body) = self.json_map.get(sql) else { ... }; + let key = (stmt.sql.clone(), stmt.params.clone()); + if self.err_on.contains(&stmt.sql) { ... } + let Some(body) = self.json_map.get(&key) else { ... };Then update tests to use
with_json_stmt(&fetch_tables_stmt(), ...), etc. This will catch param numbering/regression issues early.
402-407: Tests: prefer statement-aware fixtures.Replace
with_json(&fetch_*_stmt().sql, ...)withwith_json_stmt(&fetch_*_stmt(), ...)after MockDb change above to assert both SQL and params.I can push a scoped patch once MockDb gains
with_json_stmt.Also applies to: 418-423, 426-430, 432-440, 457-460, 469-475, 477-482, 484-492
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (47)
crates/cli/src/commands/local_db/executor.rs(9 hunks)crates/cli/src/commands/local_db/sync/runner/apply.rs(4 hunks)crates/cli/src/commands/local_db/sync/runner/mod.rs(5 hunks)crates/cli/src/commands/local_db/sync/runner/window.rs(5 hunks)crates/cli/src/commands/local_db/sync/storage.rs(10 hunks)crates/cli/src/commands/local_db/sync/token.rs(2 hunks)crates/common/src/local_db/query/clear_tables/mod.rs(1 hunks)crates/common/src/local_db/query/create_tables/mod.rs(2 hunks)crates/common/src/local_db/query/executor.rs(2 hunks)crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs(3 hunks)crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/query.sql(1 hunks)crates/common/src/local_db/query/fetch_last_synced_block/mod.rs(1 hunks)crates/common/src/local_db/query/fetch_order_trades/mod.rs(2 hunks)crates/common/src/local_db/query/fetch_order_trades/query.sql(5 hunks)crates/common/src/local_db/query/fetch_order_trades_count/mod.rs(3 hunks)crates/common/src/local_db/query/fetch_order_trades_count/query.sql(4 hunks)crates/common/src/local_db/query/fetch_orders/mod.rs(3 hunks)crates/common/src/local_db/query/fetch_orders/query.sql(1 hunks)crates/common/src/local_db/query/fetch_store_addresses/mod.rs(1 hunks)crates/common/src/local_db/query/fetch_tables/mod.rs(1 hunks)crates/common/src/local_db/query/fetch_vault/mod.rs(3 hunks)crates/common/src/local_db/query/fetch_vault/query.sql(2 hunks)crates/common/src/local_db/query/fetch_vault_balance_changes/mod.rs(2 hunks)crates/common/src/local_db/query/fetch_vault_balance_changes/query.sql(3 hunks)crates/common/src/local_db/query/fetch_vaults/mod.rs(4 hunks)crates/common/src/local_db/query/fetch_vaults/query.sql(1 hunks)crates/common/src/local_db/query/mod.rs(3 hunks)crates/common/src/local_db/query/sql_statement.rs(1 hunks)crates/common/src/local_db/query/update_last_synced_block/mod.rs(2 hunks)crates/common/src/local_db/query/update_last_synced_block/query.sql(1 hunks)crates/common/src/local_db/sync.rs(18 hunks)crates/common/src/raindex_client/local_db/executor.rs(8 hunks)crates/common/src/raindex_client/local_db/query/clear_tables.rs(2 hunks)crates/common/src/raindex_client/local_db/query/create_tables.rs(2 hunks)crates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rs(2 hunks)crates/common/src/raindex_client/local_db/query/fetch_last_synced_block.rs(3 hunks)crates/common/src/raindex_client/local_db/query/fetch_order_trades.rs(4 hunks)crates/common/src/raindex_client/local_db/query/fetch_order_trades_count.rs(4 hunks)crates/common/src/raindex_client/local_db/query/fetch_orders.rs(6 hunks)crates/common/src/raindex_client/local_db/query/fetch_store_addresses.rs(2 hunks)crates/common/src/raindex_client/local_db/query/fetch_tables.rs(2 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault.rs(6 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault_balance_changes.rs(4 hunks)crates/common/src/raindex_client/local_db/query/fetch_vaults.rs(4 hunks)crates/common/src/raindex_client/local_db/query/update_last_synced_block.rs(2 hunks)crates/common/src/raindex_client/orders.rs(2 hunks)crates/common/src/raindex_client/vaults.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-18T10:38:41.273Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#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.
Applied to files:
crates/cli/src/commands/local_db/sync/storage.rs
📚 Learning: 2025-07-31T19:34:11.716Z
Learnt from: brusherru
PR: rainlanguage/rain.orderbook#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.
Applied to files:
crates/common/src/raindex_client/orders.rs
⏰ 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: 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-legal)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: Deploy-Preview-Push
- GitHub Check: Deploy-Docs-Preview
🔇 Additional comments (76)
crates/common/src/local_db/query/fetch_vaults/query.sql (1)
76-76: Param binding and marker migration looks good; verify builder usage.
- Switching to
?1foret.chain_idand marker clauses/*OWNERS_CLAUSE*/,/*TOKENS_CLAUSE*/,/*HIDE_ZERO_BALANCE*/is consistent with the new SqlStatement flow.- Please confirm the builder always binds chain_id first and fully removes markers when filters are empty (no trailing AND/blank lines).
Also applies to: 79-81
crates/common/src/local_db/query/create_tables/mod.rs (1)
1-1: Add SqlStatement variant for CREATE TABLES; minor polish.
create_tables_stmt()is fine. Consider assertingparams.is_empty()in a unit test to document the no-param contract.Also applies to: 26-28
crates/common/src/local_db/query/fetch_order_trades_count/query.sql (1)
6-7: Param and time filter markers LGTM.
?1order-hash binding across all UNION branches is correct./*START_TS_CLAUSE*///*END_TS_CLAUSE*/under the outerWHERE 1=1will filter the unified rows byblock_timestampas intended.Also applies to: 16-17, 52-52, 77-77, 80-81
crates/common/src/local_db/query/update_last_synced_block/mod.rs (1)
1-1: Fix u64→i64 overflow risk in block_number and timestamp conversions; chain_id casts are safe.The concern is valid but partially overstated. The overflow risk applies to
u64values (block_number, timestamps), but notu32values (chain_id, which max at 4.3B—far below i64::MAX of 9.2E18).Affected locations (u64 casts only):
crates/common/src/local_db/query/update_last_synced_block/mod.rs:8– block_number (u64)crates/common/src/local_db/query/fetch_order_trades_count/mod.rs:29, 34– start_timestamp, end_timestamp (u64)crates/common/src/local_db/query/fetch_order_trades/mod.rs:82, 87– timestamps (u64)crates/common/src/local_db/query/sql_statement.rs:29– genericFrom<u64>impl (systemic root)Not an issue:
- fetch_vaults/mod.rs:28, fetch_vault/mod.rs:30 – chain_id (u32) cannot overflow i64
Recommend replacing the
From<u64>trait impl in sql_statement.rs withTryFrom<u64>to enforce checked conversion at the source, rather than sprinkling guards at each call site.⛔ Skipped due to learnings
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.crates/common/src/raindex_client/vaults.rs (1)
1818-1818: Simplify JsValue import to direct wasm-bindgen source for consistency.In this WASM-only test context (inside
#[cfg(target_family = "wasm")]module), the direct import fromwasm-bindgenis the canonical choice. Line 1612 of the same file already usesuse wasm_bindgen::{prelude::Closure, JsCast, JsValue};in an identical WASM-only test. Using the utils wrapper here introduces unnecessary inconsistency without benefit in WASM-only compilation.- use wasm_bindgen_utils::prelude::JsValue; + use wasm_bindgen::JsValue;crates/common/src/local_db/query/update_last_synced_block/query.sql (1)
1-1: LGTM!The parameterization correctly migrates from the named parameter
?block_numberto the positional parameter?1, aligning with the SqlStatement-based approach.crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/query.sql (1)
3-4: LGTM!The parameterization correctly uses positional parameter
?1for chain_id, and the/*ADDRESSES_CLAUSE*/placeholder enables dynamic address filter construction in the statement builder.crates/common/src/local_db/query/fetch_order_trades/query.sql (5)
87-97: LGTM!The order_hash parameter is correctly updated to use positional parameter
?1in both the JOIN and subquery WHERE clauses for the take_orders branch.
134-137: LGTM!The chain_id parameter is correctly updated to use positional parameter
?2in the LEFT JOIN clauses for token metadata lookups in the take_orders branch.
252-252: LGTM!The order_hash parameter is correctly updated to use positional parameter
?1in the WHERE clause for the alice clear trades branch.
361-366: LGTM!The chain_id and order_hash parameters are correctly updated to use positional parameters
?2and?1respectively in the bob clear trades branch.
369-370: LGTM!The timestamp filter placeholders
/*START_TS_CLAUSE*/and/*END_TS_CLAUSE*/enable dynamic construction of time-range filtering in the statement builder, consistent with the SqlStatement-based approach.crates/common/src/raindex_client/local_db/query/fetch_last_synced_block.rs (2)
1-12: LGTM!The function correctly migrates from using raw SQL strings to the SqlStatement-based API by calling
fetch_last_synced_block_stmt()and passing a reference to the executor.
42-54: LGTM!The test is properly updated to validate the SqlStatement object, with the store capturing both the SQL string and JsValue, and the assertion checking the statement's
sqlfield.crates/cli/src/commands/local_db/sync/token.rs (2)
97-97: LGTM!The import correctly adds
SqlStatementto support the new statement-based API.
115-121: LGTM!The SQL statements are properly wrapped in
SqlStatement::new()calls, consistent with the new executor API that accepts&SqlStatementreferences.crates/common/src/raindex_client/local_db/query/create_tables.rs (2)
1-9: LGTM!The function correctly migrates to the SqlStatement-based API by creating a statement via
create_tables_stmt()and passing a reference to the executor.
19-33: LGTM!The test is properly updated to validate the SqlStatement object, with the store capturing both the SQL string and JsValue, and the assertion checking the statement's
sqlfield.crates/common/src/local_db/query/fetch_orders/query.sql (2)
67-70: LGTM!The filter_active parameter is correctly updated to use positional parameter
?1in all three comparison expressions, maintaining the same active/all/inactive filter logic.
71-73: LGTM!The placeholder comments (
/*OWNERS_CLAUSE*/,/*ORDER_HASH_CLAUSE*/,/*TOKENS_CLAUSE*/) enable dynamic filter construction in the statement builder, consistent with the SqlStatement-based approach.crates/common/src/local_db/query/fetch_vault/query.sql (1)
73-74: LGTM!All parameters are correctly updated to use positional parameters:
?1for chain_id,?2for vault_id, and?3for token. The case-insensitive comparisons usinglower()are properly applied throughout, maintaining query correctness while adopting the SqlStatement-based approach.Also applies to: 82-83, 92-93, 97-98, 106-107
crates/common/src/local_db/query/clear_tables/mod.rs (2)
1-9: LGTM! Clean migration to SqlStatement.The refactoring correctly wraps the static SQL in a SqlStatement and updates the public API. The function signature change from returning
&'static strtoSqlStatementis a breaking change but aligns with the PR's objective to introduce prepared statement support.
11-25: LGTM! Comprehensive test coverage.The test validates all essential aspects: the SQL content matches the constant, parameters are empty (as expected for static SQL), and key SQL fragments are present.
crates/common/src/raindex_client/local_db/query/clear_tables.rs (2)
1-11: LGTM! Correct adoption of SqlStatement API.The implementation correctly uses the new
clear_tables_stmt()function and passes the statement reference to the executor.
36-48: LGTM! Test harness updated correctly.The test infrastructure now captures both SQL and parameters as a tuple
(String, JsValue), which aligns with the SqlStatement abstraction that supports parameter binding. The assertion correctly extracts the SQL portion via.0and compares it against the statement's sql field.crates/common/src/raindex_client/local_db/query/update_last_synced_block.rs (2)
1-10: LGTM! Correct migration to SqlStatement.The implementation correctly uses
build_update_last_synced_block_stmt()and passes the statement reference to the executor.
48-54: This review comment is based on incorrect assumptions about the code's scope.The closure signature change occurs in test-only code (
#[cfg(all(test, target_family = "wasm"))]) and is not part of the public JavaScript/TypeScript API. Thecreate_sql_capturing_callbackhelper function already uses the two-parameter signature|sql: String, params: JsValue|, and all test modules across the codebase consistently expect this signature. No public#[wasm_bindgen]exports are affected, and there are no external JS/TS consumers of this internal callback interface to update.Likely an incorrect or invalid review comment.
crates/common/src/raindex_client/local_db/query/fetch_vaults.rs (2)
39-40: LGTM! Proper error handling for dynamic query building.The function correctly propagates
SqlBuildErrorusing the?operator. This is appropriate sincebuild_fetch_vaults_stmtperforms dynamic query construction with parameter binding that can fail. The error is properly converted toLocalDbQueryErrorat the function boundary.
90-104: LGTM! Test correctly handles fallible statement construction.The test appropriately uses
.unwrap()onbuild_fetch_vaults_stmt()since test failures are acceptable in this context. The assertion correctly compares the captured SQL with the statement's sql field.crates/cli/src/commands/local_db/sync/runner/window.rs (2)
143-143: LGTM! Necessary import for SqlStatement.The import is correctly added to support the new statement-based executor interface.
216-218: LGTM! Consistent migration to SqlStatement.All
query_textcalls correctly wrap SQL strings withSqlStatement::new(). The pattern is consistently applied across all test cases, maintaining the existing test semantics while adopting the new API.Also applies to: 241-243, 273-275, 299-301
crates/cli/src/commands/local_db/sync/runner/mod.rs (3)
2-5: LGTM! Import updated for SqlStatement.The import restructuring correctly includes
SqlStatementfrom the query module, necessary for the new statement-based execution.
155-157: LGTM! Dynamically generated SQL wrapped correctly.The dynamically generated SQL string from
prepare_sql()is correctly wrapped inSqlStatement::new()before execution. This maintains the existing error handling flow while adopting the new API.
424-426: LGTM! Test SQL wrapped consistently.All test database initialization code correctly wraps SQL strings in
SqlStatement::new(), maintaining consistency with the new executor API.Also applies to: 532-558
crates/common/src/raindex_client/local_db/query/fetch_store_addresses.rs (2)
1-8: LGTM! Consistent migration to SqlStatement API.The implementation correctly uses
fetch_store_addresses_stmt()and passes the statement reference to the executor.
20-32: LGTM! Test harness aligned with new pattern.The test correctly captures both SQL and parameters as a tuple and validates the SQL portion against the expected statement.
crates/common/src/local_db/query/fetch_last_synced_block/mod.rs (2)
19-23: LGTM! Clean migration to SqlStatement.The function correctly wraps the static SQL constant in a
SqlStatement, following the same pattern as other static query wrappers in this PR.
25-36: LGTM! Thorough test coverage.The test validates all essential properties: SQL content matches the constant, no parameters are bound, and key SQL fragments are present. This provides confidence in the refactoring.
crates/common/src/local_db/query/fetch_tables/mod.rs (1)
12-14: LGTM: simple, correct wrapper around static SQLcrates/common/src/local_db/query/fetch_vault_balance_changes/mod.rs (1)
27-33: Parameter order and binding look correctPlease confirm
query.sqluses?1forvault_idand?2fortokento match the push order.crates/common/src/raindex_client/local_db/query/fetch_order_trades.rs (1)
13-15: LGTM: build stmt, then awaitCorrect pattern (avoid borrowing a temporary across await).
crates/common/src/raindex_client/local_db/query/fetch_order_trades_count.rs (2)
12-14: Prepared-statement usage is correct.Switch to build_fetch_trade_count_stmt + exec.query_json(&stmt) is clean and consistent with the new executor API.
25-25: Remove the uncommon import and import directly fromwasm_bindgen.Line 25 attempts to import
wasm_bindgenfromwasm_bindgen_utils::prelude, but based on the crate's documentation, the prelude exports helper macros, not thewasm_bindgenmodule. The code requireswasm_bindgenin scope only forwasm_bindgen::JsValue::UNDEFINEDusage on lines 39 and 56. Instead of the problematic import path, either remove line 25 if it's unused, or replace it with a direct import from thewasm_bindgencrate (e.g.,use wasm_bindgen;oruse wasm_bindgen::JsValue;).⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/rain.orderbook#1858 File: crates/subgraph/src/orderbook_client/mod.rs:54-58 Timestamp: 2025-05-19T13:40:56.080Z Learning: The `wasm_bindgen_utils` crate in the Rain Orderbook project handles conditional compilation for `JsValue` and `JsError` internally, allowing `impl From<Error> for JsValue` to work on non-WASM targets without explicit cfg guards.Learnt from: findolor PR: rainlanguage/rain.orderbook#1950 File: crates/common/src/raindex_client/transactions.rs:34-38 Timestamp: 2025-06-24T13:30:02.968Z Learning: When using #[wasm_bindgen] on an impl block, all methods within that block must have wasm_bindgen attributes, even if they are conditionally compiled for non-WASM targets using #[cfg(not(target_family = "wasm"))]. Removing these attributes causes compiler errors because the wasm_bindgen macro expansion processes the entire impl block and expects consistent attribute usage across all methods.crates/common/src/local_db/query/fetch_vault/mod.rs (1)
27-34: Builder looks good and types are appropriate.Param order and SqlValue types are sensible (I64 for chain_id, Text for ids).
crates/common/src/raindex_client/local_db/query/fetch_vault_balance_changes.rs (1)
11-13: Statement flow is correct.Construction and exec.query_json(&stmt) align with the new trait.
crates/common/src/raindex_client/local_db/query/fetch_vault.rs (2)
12-14: Good migration to SqlStatement.Wrapper build + exec.query_json(&stmt) is consistent.
125-136: No action required; WasmEncodedResult is already in scope.The file imports
use wasm_bindgen_utils::prelude::*;at line 48, which bringsWasmEncodedResultinto scope. The type is defined in thewasm_bindgen_utilscrate (not the executor module), and the existing wildcard import is sufficient. The code at lines 125–136 does not need any changes.crates/common/src/local_db/query/executor.rs (1)
12-16: Trait signature updates look correct.Accepting &SqlStatement across query_json/query_text matches the new binding model and keeps wasm compatibility via ?Send.
crates/common/src/local_db/query/mod.rs (3)
14-19: Publicly exposing sql_statement types looks goodRe-export makes the new statement API available to dependents. LGTM.
37-39: Confirm Clone compatibility for new error variantLocalDbQueryError derives Clone; this requires SqlBuildError: Clone. Please confirm SqlBuildError derives Clone (and ideally Debug + Error) to avoid a compile break.
59-63: From mapping is correctClean conversion path for builder failures. LGTM.
crates/cli/src/commands/local_db/sync/storage.rs (9)
16-16: Executor/SqlStatement importsImport changes align with the new API. LGTM.
34-36: Prepared-statement usage for table introspectionSwitch to SqlStatement here is correct. LGTM.
49-52: Schema creation via SqlStatementExecuting DDL through query_text(&SqlStatement) matches the new executor signature. LGTM.
58-61: Last-synced query migrated correctlyPrepared-statement path with SqlStatement is correct. LGTM.
67-70: Store addresses query migrated correctlyPrepared-statement path with SqlStatement is correct. LGTM.
114-121: Behavior for empty address listShort-circuiting with Ok(None) avoids invalid IN () and extra DB work. LGTM. One check: ensure callers always pass normalized lowercase addresses if the underlying SQL compares text without LOWER(), or make the template handle LOWER(address). Please confirm expectation.
123-126: Executing built ERC20 fetch statementPassing the built statement directly is correct. LGTM.
188-193: Tests: schema init via SqlStatementTest updates reflect the new API and look correct.
205-230: Tests: insert using SqlStatementInline INSERT executed via SqlStatement is fine. LGTM.
crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs (5)
1-1: Imports for builder typesCorrectly pulls in SqlStatement, SqlValue, and SqlBuildError. LGTM.
15-16: Clause markers are clear and specificDistinct marker/body strings make template drift detectable. LGTM.
19-21: Clear behavior contract in docsReturning Ok(None) for empty input is a good API. LGTM.
46-49: Test: empty input fast-pathCovers Ok(None) branch. LGTM.
66-81: Test: template drift detectionGood negative test for MissingMarker. LGTM.
crates/common/src/raindex_client/local_db/query/fetch_orders.rs (4)
2-3: Updated import to stmt builderAligns with the new API. LGTM.
43-45: Wrapper builds and executes prepared statementCorrect propagation of builder errors and execution via query_json. LGTM.
123-141: WASM error path setup is soundClosure captures (sql, params) and returns an error wrapper. LGTM.
181-191: Invalid response test uses new callback signature correctlyExercise InvalidResponse path; looks good.
crates/common/src/local_db/query/fetch_order_trades/mod.rs (2)
1-1: Imports for statement buildingMatches new API usage. LGTM.
60-66: Time filter clause markersClear markers and bodies for optional filters. LGTM.
crates/cli/src/commands/local_db/executor.rs (1)
26-31: Clarify semantics of execute_batch on empty-params path.execute_batch ignores result sets; if a SELECT is mistakenly passed here it silently returns empty String. Document that query_text is for non-SELECT/batch statements, or guard with a check.
crates/common/src/local_db/query/fetch_orders/mod.rs (1)
53-56: Confirm template aliases used in clause bodies exist.ORDER_HASH_CLAUSE_BODY references aliases la and l; ensure QUERY_TEMPLATE defines these in scope where the clause is inserted.
crates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rs (1)
9-13: Good early-return on empty addresses.Avoids unnecessary round-trips and matches builder’s None path.
crates/common/src/raindex_client/local_db/executor.rs (1)
21-28: UNDEFINED for empty params matches expected SDK semantics.The special-case keeps backward compatibility for call sites that distinguish undefined vs []. LGTM.
crates/common/src/local_db/query/sql_statement.rs (1)
154-162: Update test to use checked conversion for u64.Adjust the u64 case to use
try_from.Apply:
- assert_eq!(SqlValue::from(7u64), SqlValue::I64(7)); + assert_eq!(SqlValue::try_from(7u64).unwrap(), SqlValue::I64(7));
| pub fn build_fetch_stmt( | ||
| chain_id: u32, | ||
| addresses: &[String], | ||
| ) -> Result<Option<SqlStatement>, SqlBuildError> { | ||
| if addresses.is_empty() { | ||
| return None; | ||
| return Ok(None); | ||
| } | ||
|
|
||
| let in_clause = addresses | ||
| .iter() | ||
| .map(|a| format!("'{}'", a.replace('\'', "''"))) | ||
| .collect::<Vec<_>>() | ||
| .join(", "); | ||
|
|
||
| let sql = FETCH_ERC20_TOKENS_BY_ADDRESSES_SQL | ||
| .replace("?chain_id", &chain_id.to_string()) | ||
| .replace("?addresses_in", &in_clause); | ||
|
|
||
| Some(sql) | ||
| let mut stmt = SqlStatement::new(FETCH_ERC20_TOKENS_BY_ADDRESSES_SQL); | ||
| // ?1: chain id | ||
| stmt.push(chain_id as i64); | ||
| // IN list for addresses | ||
| stmt.bind_list_clause( | ||
| ADDRESSES_CLAUSE, | ||
| ADDRESSES_CLAUSE_BODY, | ||
| addresses.iter().cloned().map(SqlValue::Text), | ||
| )?; | ||
| Ok(Some(stmt)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Robust statement builder with list binding
Chain ID as ?1 and list binding for addresses via a clause marker is solid and injection-safe. Consider normalizing addresses to lowercase here (or enforce at call sites) to ensure case-insensitive matches if the DB stores lowercase; otherwise queries may miss checksum-cased inputs.
🤖 Prompt for AI Agents
crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs around
lines 21 to 39: the builder currently binds the provided addresses directly
which can miss matches if the DB stores addresses in lowercase; normalize
addresses to a canonical form before binding by mapping the input slice to
lowercase Strings (e.g., addresses.iter().map(|a| a.to_lowercase())) and then
converting those lowercase Strings into SqlValue::Text for the bind_list_clause
so the query is case-insensitive with respect to address casing.
There was a problem hiding this comment.
@findolor, understood! I've noted your decision.
| let store = Rc::new(RefCell::new(( | ||
| String::new(), | ||
| wasm_bindgen::JsValue::UNDEFINED, | ||
| ))); | ||
| let callback = create_sql_capturing_callback(row_json, store.clone()); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add param assertions to the second wasm test as well.
Mirror the above to prevent silent param-order regressions.
@@
- let res = super::fetch_vault_balance_changes(&exec, vault_id, token).await;
+ let res = super::fetch_vault_balance_changes(&exec, vault_id, token).await;
assert!(res.is_ok());
let rows = res.unwrap();
assert_eq!(rows.len(), 1);
- assert_eq!(store.borrow().clone().0, expected_stmt.sql);
+ let captured = store.borrow().clone();
+ assert_eq!(captured.0, expected_stmt.sql);
+ use wasm_bindgen_utils::prelude::serde_wasm_bindgen::from_value;
+ use serde_json::json;
+ let params: serde_json::Value = from_value(captured.1).unwrap();
+ assert_eq!(params, json!(["v01", "0xtoken"]));Also applies to: 73-74
There was a problem hiding this comment.
@findolor, understood. I'll skip this suggestion.
| let store = Rc::new(RefCell::new(( | ||
| String::new(), | ||
| wasm_bindgen::JsValue::UNDEFINED, | ||
| ))); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Also assert params in wasm test.
Since store keeps (sql, params), verify [chain_id, vault_id, token] binding order to catch regressions.
@@
- let captured = store.borrow().clone();
- assert_eq!(captured.0, expected_stmt.sql);
+ let captured = store.borrow().clone();
+ assert_eq!(captured.0, expected_stmt.sql);
+ use wasm_bindgen_utils::prelude::serde_wasm_bindgen::from_value;
+ use serde_json::json;
+ let params: serde_json::Value = from_value(captured.1).unwrap();
+ assert_eq!(params, json!([100, "0x01", "0xabc"]));Also applies to: 64-65
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/local_db/query/fetch_vault.rs around lines
52-55 (and also apply the same change at lines 64-65), the test’s in-memory
store captures (sql, params) but does not assert the order or contents of
params; add assertions in the wasm test that the captured store contains the
expected SQL and that the params array is present and has length 3 with elements
bound in the exact order [chain_id, vault_id, token] (compare values/types
appropriately for JsValue), so any regression in binding order is detected;
implement the same param-order assertions at both locations mentioned.
There was a problem hiding this comment.
@findolor, understood! I'll skip the additional parameter assertions in the wasm tests.
(‿ˠ‿)
| let store = Rc::new(RefCell::new(( | ||
| String::new(), | ||
| wasm_bindgen::JsValue::UNDEFINED, | ||
| ))); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add param assertion to the second wasm test.
Mirror the above param check.
@@
- assert!(row.is_some());
- assert_eq!(store.borrow().clone().0, expected_stmt.sql);
+ assert!(row.is_some());
+ let captured = store.borrow().clone();
+ assert_eq!(captured.0, expected_stmt.sql);
+ use wasm_bindgen_utils::prelude::serde_wasm_bindgen::from_value;
+ use serde_json::json;
+ let params: serde_json::Value = from_value(captured.1).unwrap();
+ assert_eq!(params, json!([100, "0x01", "0xabc"]));Also applies to: 88-89
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/local_db/query/fetch_vault.rs around lines
77 to 80 (and similarly at 88–89), the wasm test is missing a parameter
assertion that mirrors the earlier param check; add the same assertion used
above to validate the second test's parameter (i.e., assert the expected
string/JsValue for that param) so both wasm tests perform identical param
validation, placing the assertion immediately after the created Rc<RefCell<...>>
store setup in each location.
There was a problem hiding this comment.
@findolor, understood! I'll skip the additional parameter assertion. Let me know if there's anything else I can help with.
| let expected1 = build_fetch_vault_stmt(chain_id, "v1", "t1"); | ||
| let expected2 = build_fetch_vault_stmt(chain_id, "v2", "t2"); | ||
| assert_eq!(captured, vec![expected1.sql, expected2.sql]); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optionally also capture params for ordering checks.
You currently only push sql into calls; consider capturing params too (Vec<(String, JsValue)>) and assert both order and param content.
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/local_db/query/fetch_vault.rs around lines
148 to 151, the test only records SQL strings and doesn't capture the query
parameters, so ordering assertions miss param verification; update the test
harness to record both SQL and params as Vec<(String, Vec<JsValue>)> (or
Vec<(String, JsValue)> if single param), change the places that push calls to
push the tuple (sql, params), update build_fetch_vault_stmt (or its returned
struct) to expose expected params alongside expected.sql, and then assert_eq! on
the captured vector against vec![(expected1.sql, expected1.params),
(expected2.sql, expected2.params)] to verify both order and parameter content.
There was a problem hiding this comment.
@findolor, understood. I'll skip this suggestion. The current test adequately verifies the call order.
| ); | ||
| let captured_sql = Rc::new(RefCell::new(String::new())); | ||
| let captured_sql = Rc::new(RefCell::new((String::new(), JsValue::UNDEFINED))); | ||
| let json = serde_json::to_string(&vec![keep_vault]).unwrap(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen param assertions in the filter test.
Great checks for first param and presence of owner/token. Also assert the total param count equals chain_id + owners.len() + tokens.len() (here 3) to catch regressions.
let sql = captured_sql.borrow();
// SQL should contain parameterized IN-clauses and hide-zero filter body
assert!(sql.0.contains("lower(o.owner) IN ("));
assert!(sql.0.contains("lower(o.token) IN ("));
assert!(sql.0.contains("AND NOT FLOAT_IS_ZERO("));
// Params should include chain id, owner and token values bound in order
let params: Vec<crate::local_db::query::SqlValue> =
wasm_bindgen_utils::prelude::serde_wasm_bindgen::from_value(sql.1.clone())
.expect("params parse");
+ assert_eq!(params.len(), 3, "expected chain_id + 1 owner + 1 token");Also applies to: 1859-1886
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/vaults.rs around line 1831 (and similarly
for the block at 1859-1886), the filter test currently checks the first param
and presence of owner/token but does not assert the total number of params;
update the test to compute the expected_param_count as 1 (for chain_id) +
owners.len() + tokens.len() and add an assertion that params.len() ==
expected_param_count (or the explicit value 3 in this case) to catch
regressions, and apply the same exact check to the other test block at
1859-1886.
There was a problem hiding this comment.
@findolor, understood! I'll skip this assertion. The existing checks for the chain_id, owner, and token presence are sufficient for your needs.
| let mut owners: Vec<String> = Vec::new(); | ||
| let mut seen: HashSet<String> = HashSet::new(); | ||
| for owner in args.owners.iter() { | ||
| let t = owner.trim(); | ||
| if t.is_empty() { | ||
| continue; | ||
| } | ||
| let lowered = t.to_ascii_lowercase(); | ||
| if seen.insert(lowered.clone()) { | ||
| owners.push(lowered); | ||
| } | ||
| } |
There was a problem hiding this comment.
nitpick: consider (requires itertools)
| let mut owners: Vec<String> = Vec::new(); | |
| let mut seen: HashSet<String> = HashSet::new(); | |
| for owner in args.owners.iter() { | |
| let t = owner.trim(); | |
| if t.is_empty() { | |
| continue; | |
| } | |
| let lowered = t.to_ascii_lowercase(); | |
| if seen.insert(lowered.clone()) { | |
| owners.push(lowered); | |
| } | |
| } | |
| let owners: Vec<String> = args.owners | |
| .iter() | |
| .map(|owner| owner.trim().to_ascii_lowercase()) | |
| .filter(|s| !s.is_empty()) | |
| .unique() | |
| .collect(); |
| let mut tokens: Vec<String> = Vec::new(); | ||
| let mut seen_tokens: HashSet<String> = HashSet::new(); | ||
| for token in args.tokens.iter() { | ||
| let t = token.trim(); | ||
| if t.is_empty() { | ||
| continue; | ||
| } | ||
| let lowered = t.to_ascii_lowercase(); | ||
| if seen_tokens.insert(lowered.clone()) { | ||
| tokens.push(lowered); | ||
| } | ||
| } |
There was a problem hiding this comment.
| let mut tokens: Vec<String> = Vec::new(); | |
| let mut seen_tokens: HashSet<String> = HashSet::new(); | |
| for token in args.tokens.iter() { | |
| let t = token.trim(); | |
| if t.is_empty() { | |
| continue; | |
| } | |
| let lowered = t.to_ascii_lowercase(); | |
| if seen_tokens.insert(lowered.clone()) { | |
| tokens.push(lowered); | |
| } | |
| } | |
| let tokens: Vec<String> = args.tokens | |
| .iter() | |
| .map(|token| token.trim().to_ascii_lowercase()) | |
| .filter(|s| !s.is_empty()) | |
| .unique() | |
| .collect(); |
| let mut list = String::new(); | ||
| let mut first = true; | ||
| let mut count = 0usize; | ||
| for v in it { | ||
| count += 1; | ||
| let ph = self.push(v); | ||
| if !first { | ||
| list.push_str(", "); | ||
| } else { | ||
| first = false; | ||
| } | ||
| list.push_str(&ph); | ||
| } | ||
| if count == 0 { | ||
| self.sql = self.sql.replace(clause_marker, ""); | ||
| return Ok(self); | ||
| } |
There was a problem hiding this comment.
| let mut list = String::new(); | |
| let mut first = true; | |
| let mut count = 0usize; | |
| for v in it { | |
| count += 1; | |
| let ph = self.push(v); | |
| if !first { | |
| list.push_str(", "); | |
| } else { | |
| first = false; | |
| } | |
| list.push_str(&ph); | |
| } | |
| if count == 0 { | |
| self.sql = self.sql.replace(clause_marker, ""); | |
| return Ok(self); | |
| } | |
| let placeholders: Vec<_> = it.map(|v| self.push(v)).collect(); | |
| let count = placeholders.len(); | |
| if count == 0 { | |
| self.sql = self.sql.replace(clause_marker, ""); | |
| return Ok(self); | |
| } | |
| let list = placeholders.join(", "); |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/local_db/query/fetch_vaults/mod.rs (1)
87-113: Strengthen assertions: verify param order and values.Guard against regressions: check chain_id first, then owners, then tokens, all lowercased.
- // Params: chain id + owners + tokens - assert!(!stmt.params.is_empty()); + use crate::local_db::query::SqlValue; + // chain_id (?1), then owners (in given order after normalize), then tokens + assert_eq!(stmt.params().len(), 1 + 2 + 1); + assert_eq!(stmt.params()[0], SqlValue::I64(137)); + assert_eq!(stmt.params()[1], SqlValue::Text("0xa".into)); + assert_eq!(stmt.params()[2], SqlValue::Text("o'owner".into)); + assert_eq!(stmt.params()[3], SqlValue::Text("tok'a".into));Optionally also assert the generated SQL has two IN‑lists present.
♻️ Duplicate comments (3)
crates/common/src/raindex_client/local_db/executor.rs (1)
192-227: Great: test ensures non‑empty params go through as JS Array.Prevents regressions in param encoding path.
crates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rs (1)
40-80: Nice: wrapper asserts SQL and params exactly match builder output.This closes the coverage gap noted earlier.
crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs (1)
52-67: Good: test now asserts param ordering and values.Covers chain_id first, then addresses in order.
Also applies to: 58-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
crates/cli/src/commands/local_db/executor.rs(9 hunks)crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs(3 hunks)crates/common/src/local_db/query/fetch_order_trades/mod.rs(2 hunks)crates/common/src/local_db/query/fetch_order_trades_count/mod.rs(3 hunks)crates/common/src/local_db/query/fetch_orders/mod.rs(3 hunks)crates/common/src/local_db/query/fetch_vaults/mod.rs(4 hunks)crates/common/src/local_db/query/sql_statement.rs(1 hunks)crates/common/src/raindex_client/local_db/executor.rs(8 hunks)crates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rs(2 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/local_db/query/fetch_order_trades_count/mod.rscrates/common/src/local_db/query/fetch_orders/mod.rscrates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rscrates/common/src/local_db/query/fetch_order_trades/mod.rscrates/cli/src/commands/local_db/executor.rscrates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rscrates/common/src/local_db/query/fetch_vaults/mod.rscrates/common/src/local_db/query/sql_statement.rscrates/common/src/raindex_client/local_db/executor.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/local_db/query/fetch_order_trades_count/mod.rscrates/common/src/local_db/query/fetch_orders/mod.rscrates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rscrates/common/src/local_db/query/fetch_order_trades/mod.rscrates/cli/src/commands/local_db/executor.rscrates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rscrates/common/src/local_db/query/fetch_vaults/mod.rscrates/common/src/local_db/query/sql_statement.rscrates/common/src/raindex_client/local_db/executor.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/local_db/query/fetch_order_trades_count/mod.rscrates/common/src/local_db/query/fetch_orders/mod.rscrates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rscrates/common/src/local_db/query/fetch_order_trades/mod.rscrates/cli/src/commands/local_db/executor.rscrates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rscrates/common/src/local_db/query/fetch_vaults/mod.rscrates/common/src/local_db/query/sql_statement.rscrates/common/src/raindex_client/local_db/executor.rs
🧬 Code graph analysis (8)
crates/common/src/local_db/query/fetch_order_trades_count/mod.rs (2)
crates/common/src/local_db/query/sql_statement.rs (5)
new(44-49)new(172-176)try_from(30-34)sql(51-53)params(55-57)crates/common/src/local_db/query/fetch_order_trades/mod.rs (1)
builds_without_time_filters_when_none(127-137)
crates/common/src/local_db/query/fetch_orders/mod.rs (1)
crates/common/src/local_db/query/sql_statement.rs (6)
new(44-49)new(172-176)s(355-355)s(393-393)sql(51-53)params(55-57)
crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs (1)
crates/common/src/local_db/query/sql_statement.rs (4)
new(44-49)new(172-176)sql(51-53)params(55-57)
crates/common/src/local_db/query/fetch_order_trades/mod.rs (2)
crates/common/src/local_db/query/sql_statement.rs (5)
new(44-49)new(172-176)try_from(30-34)sql(51-53)params(55-57)crates/common/src/local_db/query/fetch_order_trades_count/mod.rs (1)
builds_without_time_filters_when_none(79-86)
crates/cli/src/commands/local_db/executor.rs (3)
crates/common/src/raindex_client/local_db/executor.rs (3)
query_text(17-59)new(10-12)query_json(61-68)crates/common/src/local_db/query/executor.rs (2)
query_text(16-16)query_json(12-14)crates/common/src/local_db/query/sql_statement.rs (5)
new(44-49)new(172-176)s(355-355)s(393-393)params(55-57)
crates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rs (3)
crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs (1)
build_fetch_stmt(21-39)crates/common/src/local_db/query/sql_statement.rs (3)
new(44-49)new(172-176)sql(51-53)crates/common/src/raindex_client/local_db/executor.rs (2)
new(10-12)create_sql_capturing_callback(111-134)
crates/common/src/local_db/query/fetch_vaults/mod.rs (2)
crates/common/src/local_db/query/fetch_orders/mod.rs (2)
args(73-84)args(104-115)crates/common/src/local_db/query/sql_statement.rs (4)
new(44-49)new(172-176)sql(51-53)params(55-57)
crates/common/src/raindex_client/local_db/executor.rs (3)
crates/cli/src/commands/local_db/executor.rs (3)
query_text(32-51)query_json(53-109)new(23-27)crates/common/src/local_db/query/executor.rs (2)
query_text(16-16)query_json(12-14)crates/common/src/local_db/query/sql_statement.rs (4)
new(44-49)new(172-176)sql(51-53)params(55-57)
🔇 Additional comments (10)
crates/common/src/local_db/query/fetch_order_trades_count/mod.rs (1)
27-31: Good: validated time range and safe u64→i64 conversionstart<=end check plus checked conversions avoid silent wrap and bad ranges. Binding via bind_param_clause is correct. Nice.
Also applies to: 33-54
crates/common/src/local_db/query/fetch_order_trades/mod.rs (1)
73-77: Parameter order and tests look correctOrder hash then chain id as fixed params, with timestamp clauses bound via markers. Tests assert both SQL and param ordering—good coverage.
Also applies to: 110-124
crates/cli/src/commands/local_db/executor.rs (2)
14-20: Nice: centralized param mapping + busy_timeoutHelper sqlvalue_to_rusqlite removes duplication, and busy_timeout will reduce SQLITE_BUSY flakiness. LGTM.
Also applies to: 35-36, 59-60
32-50: Avoid blocking the async runtime; move DB work to spawn_blockingrusqlite is blocking. Running it directly inside async fns can stall the runtime. Offload to a blocking thread pool.
Example for query_text (apply similarly to query_json):
- async fn query_text(&self, stmt: &SqlStatement) -> Result<String, LocalDbQueryError> { - let conn = Connection::open(&self.db_path) - .map_err(|e| LocalDbQueryError::database(format!("Failed to open database: {e}")))?; - conn.busy_timeout(Duration::from_millis(500)) - .map_err(|e| LocalDbQueryError::database(format!("Failed to set busy_timeout: {e}")))?; - if stmt.params().is_empty() { - conn.execute_batch(stmt.sql()) - .map_err(|e| LocalDbQueryError::database(format!("SQL execution failed: {e}")))?; - Ok(String::new()) - } else { - let mut s = conn.prepare(stmt.sql()).map_err(|e| { - LocalDbQueryError::database(format!("Failed to prepare query: {e}")) - })?; - let bound = stmt.params().iter().cloned().map(sqlvalue_to_rusqlite); - let params = rusqlite::params_from_iter(bound); - s.execute(params) - .map_err(|e| LocalDbQueryError::database(format!("SQL execution failed: {e}")))?; - Ok(String::new()) - } - } + async fn query_text(&self, stmt: &SqlStatement) -> Result<String, LocalDbQueryError> { + let db_path = self.db_path.clone(); + let sql = stmt.sql().to_owned(); + let params: Vec<_> = stmt.params().iter().cloned().map(sqlvalue_to_rusqlite).collect(); + tokio::task::spawn_blocking(move || { + let conn = Connection::open(&db_path) + .map_err(|e| LocalDbQueryError::database(format!("Failed to open database: {e}")))?; + conn.busy_timeout(Duration::from_millis(500)) + .map_err(|e| LocalDbQueryError::database(format!("Failed to set busy_timeout: {e}")))?; + if params.is_empty() { + conn.execute_batch(&sql) + .map_err(|e| LocalDbQueryError::database(format!("SQL execution failed: {e}")))?; + Ok(String::new()) + } else { + let mut s = conn + .prepare(&sql) + .map_err(|e| LocalDbQueryError::database(format!("Failed to prepare query: {e}")))?; + let params = rusqlite::params_from_iter(params.into_iter()); + s.execute(params) + .map_err(|e| LocalDbQueryError::database(format!("SQL execution failed: {e}")))?; + Ok(String::new()) + } + }) + .await + .map_err(|e| LocalDbQueryError::database(format!("Join error: {e}")))? + }This keeps the async surface without blocking.
Also applies to: 57-109
⛔ Skipped due to learnings
Learnt from: findolor PR: rainlanguage/rain.orderbook#2256 File: crates/common/src/raindex_client/local_db/query/fetch_tables.rs:7-7 Timestamp: 2025-10-21T05:15:50.518Z Learning: In Rust 2021+ editions, passing a reference to a temporary into an async function within the same expression (e.g., `exec.query_json(&fetch_tables_stmt()).await`) is safe and idiomatic. The temporary lifetime is extended across the await point, so this pattern should not be flagged as an issue.crates/common/src/local_db/query/sql_statement.rs (3)
28-35: Checked u64→i64 via TryFromPrevents lossy casts and surfaces errors—exactly what we want for safety.
66-86: replace(): clear docs and safety caveatsDoc now states “all occurrences” and warns about injecting static SQL—good clarity and safer usage.
87-111: Clause binders handle presence/empties and numbering correctlyMarker/token validation plus placeholder continuity is robust; tests cover edge cases. LGTM.
Also applies to: 113-149
crates/common/src/local_db/query/fetch_orders/mod.rs (1)
61-93: No spacing issues found; code is correctThe markers in query.sql are positioned correctly with proper spacing:
- Line 70 ends:
)- Lines 71–73:
/*OWNERS_CLAUSE*/,/*ORDER_HASH_CLAUSE*/,/*TOKENS_CLAUSE*/(each on separate lines)- Line 74 begins:
GROUP BYAll clause bodies start with
"AND":
OWNERS_CLAUSE_BODY:"AND lower(l.order_owner) IN ({list})"ORDER_HASH_CLAUSE_BODY:"AND lower(COALESCE(la.order_hash, l.order_hash)) = lower({param})"TOKENS_CLAUSE_BODY:"AND EXISTS (...)"When markers are replaced, the result is:
) AND ... GROUP BYwith proper line breaks and SQL syntax is valid. No changes required.crates/common/src/local_db/query/fetch_vaults/mod.rs (1)
44-48: Confirm bind_list_clause behavior for empty lists (defensive test).Add tests that when owners/tokens end up empty after normalization, the clause markers are removed and no params are added.
I can add such tests if desired.
Also applies to: 63-67
crates/common/src/raindex_client/local_db/executor.rs (1)
61-68: The proposed change is incorrect and would not compile.The
FromDbJsontrait (defined atcrates/common/src/local_db/query/mod.rs:67) is a marker trait with no custom methods:pub trait FromDbJson: DeserializeOwned {} impl<T> FromDbJson for T where T: DeserializeOwned {}There is no
from_db_jsonmethod to call. The current implementation usingserde_json::from_str(&value)is the correct approach—it already respects theT: FromDbJsonbound and uses the trait's inheritedDeserializeOwnedcapability. The suggestion to "centralize decoding rules" cannot be applied here since the trait defines no such rules to leverage.Likely an incorrect or invalid review comment.
| let mut stmt = SqlStatement::new(FETCH_ERC20_TOKENS_BY_ADDRESSES_SQL); | ||
| // ?1: chain id | ||
| stmt.push(chain_id as i64); | ||
| // IN list for addresses | ||
| stmt.bind_list_clause( | ||
| ADDRESSES_CLAUSE, | ||
| ADDRESSES_CLAUSE_BODY, | ||
| addresses.iter().cloned().map(SqlValue::Text), | ||
| )?; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: deduplicate addresses to reduce placeholders and stabilize plans.
Order‑preserving dedup avoids redundant IN entries and params.
- // IN list for addresses
- stmt.bind_list_clause(
+ // IN list for addresses (order-preserving dedup)
+ let mut seen = std::collections::HashSet::new();
+ let addrs: Vec<_> = addresses
+ .iter()
+ .filter_map(|a| {
+ if seen.insert(a) { Some(SqlValue::Text(a.clone())) } else { None }
+ })
+ .collect();
+ stmt.bind_list_clause(
ADDRESSES_CLAUSE,
ADDRESSES_CLAUSE_BODY,
- addresses.iter().cloned().map(SqlValue::Text),
+ addrs.into_iter(),
)?;📝 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.
| let mut stmt = SqlStatement::new(FETCH_ERC20_TOKENS_BY_ADDRESSES_SQL); | |
| // ?1: chain id | |
| stmt.push(chain_id as i64); | |
| // IN list for addresses | |
| stmt.bind_list_clause( | |
| ADDRESSES_CLAUSE, | |
| ADDRESSES_CLAUSE_BODY, | |
| addresses.iter().cloned().map(SqlValue::Text), | |
| )?; | |
| let mut stmt = SqlStatement::new(FETCH_ERC20_TOKENS_BY_ADDRESSES_SQL); | |
| // ?1: chain id | |
| stmt.push(chain_id as i64); | |
| // IN list for addresses (order-preserving dedup) | |
| let mut seen = std::collections::HashSet::new(); | |
| let addrs: Vec<_> = addresses | |
| .iter() | |
| .filter_map(|a| { | |
| if seen.insert(a) { Some(SqlValue::Text(a.clone())) } else { None } | |
| }) | |
| .collect(); | |
| stmt.bind_list_clause( | |
| ADDRESSES_CLAUSE, | |
| ADDRESSES_CLAUSE_BODY, | |
| addrs.into_iter(), | |
| )?; |
🤖 Prompt for AI Agents
In crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs
around lines 29 to 37, the addresses passed into bind_list_clause can contain
duplicates which creates redundant placeholders and unstable query plans;
deduplicate the addresses in an order-preserving way before calling
bind_list_clause (e.g., iterate the original addresses and push each into a new
Vec only if not seen, using a small HashSet/IndexSet to track seen entries),
then bind that deduplicated Vec.map(SqlValue::Text) instead of the original
iterator.
| // SQL has markers resolved and ?1 present for chain id | ||
| assert!(stmt.sql.contains("WHERE chain_id = ?1")); | ||
| assert!(!stmt.sql.contains(ADDRESSES_CLAUSE)); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use accessors in tests to reduce coupling.
Switch to stmt.sql() / stmt.params() for robustness.
- assert!(stmt.sql.contains("WHERE chain_id = ?1"));
- assert!(!stmt.sql.contains(ADDRESSES_CLAUSE));
+ assert!(stmt.sql().contains("WHERE chain_id = ?1"));
+ assert!(!stmt.sql().contains(ADDRESSES_CLAUSE));
...
- assert_eq!(stmt.params.len(), 3);
- assert_eq!(stmt.params[0], SqlValue::I64(137));
- assert_eq!(stmt.params[1], SqlValue::Text(addrs[0].clone()));
- assert_eq!(stmt.params[2], SqlValue::Text(addrs[1].clone()));
+ assert_eq!(stmt.params().len(), 3);
+ assert_eq!(stmt.params()[0], SqlValue::I64(137));
+ assert_eq!(stmt.params()[1], SqlValue::Text(addrs[0].clone()));
+ assert_eq!(stmt.params()[2], SqlValue::Text(addrs[1].clone()));Also applies to: 63-67
🤖 Prompt for AI Agents
In crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs
around lines 58 to 61 (and similarly for lines 63 to 67), the test currently
accesses stmt.sql and stmt.params directly which couples the test to the struct
internals; replace those direct field reads with the public accessors stmt.sql()
and stmt.params() to make the test robust to implementation changes, updating
the assertions to call stmt.sql() for the SQL string and stmt.params() for
parameter checks.
| let start_param = if let Some(v) = start_timestamp { | ||
| let i = i64::try_from(v).map_err(|e| { | ||
| SqlBuildError::new(format!( | ||
| "start_timestamp out of range for i64: {} ({})", | ||
| v, e | ||
| )) | ||
| })?; | ||
| Some(SqlValue::I64(i)) | ||
| } else { | ||
| None | ||
| }; | ||
| stmt.bind_param_clause(START_TS_CLAUSE, START_TS_BODY, start_param)?; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DRY the timestamp-to-SqlValue conversion
Same conversion appears twice. A tiny helper/closure reduces duplication and keeps error messages uniform.
- let start_param = if let Some(v) = start_timestamp {
- let i = i64::try_from(v).map_err(|e| {
- SqlBuildError::new(format!(
- "start_timestamp out of range for i64: {} ({})",
- v, e
- ))
- })?;
- Some(SqlValue::I64(i))
- } else {
- None
- };
+ let to_i64 = |label: &str, v: u64| -> Result<i64, SqlBuildError> {
+ i64::try_from(v)
+ .map_err(|e| SqlBuildError::new(format!("{label} out of range for i64: {v} ({e})")))
+ };
+ let start_param = start_timestamp
+ .map(|v| to_i64("start_timestamp", v))
+ .transpose()?
+ .map(SqlValue::I64);
@@
- let end_param = if let Some(v) = end_timestamp {
- let i = i64::try_from(v).map_err(|e| {
- SqlBuildError::new(format!("end_timestamp out of range for i64: {} ({})", v, e))
- })?;
- Some(SqlValue::I64(i))
- } else {
- None
- };
+ let end_param = end_timestamp
+ .map(|v| to_i64("end_timestamp", v))
+ .transpose()?
+ .map(SqlValue::I64);Also applies to: 46-54
🤖 Prompt for AI Agents
In crates/common/src/local_db/query/fetch_order_trades_count/mod.rs around lines
33 to 44 (and also apply same change at 46 to 54), extract the repeated
timestamp-to-SqlValue conversion into a small helper function or closure that
takes Option<u64> and returns Result<Option<SqlValue>, SqlBuildError>
(performing i64::try_from and mapping the same error message), then call that
helper for both start_timestamp and end_timestamp and pass the resulting
Option<SqlValue> to stmt.bind_param_clause; this removes duplication and ensures
uniform error messages.
| #[test] | ||
| fn builds_with_time_filters_and_sanitization() { | ||
| let q = build_fetch_trade_count_query(" 0xABC'DEF ", Some(1000), Some(2000)); | ||
|
|
||
| // Placeholders should be gone | ||
| assert!(!q.contains("?filter_start_timestamp")); | ||
| assert!(!q.contains("?filter_end_timestamp")); | ||
| assert!(!q.contains("?order_hash")); | ||
|
|
||
| // Time filters present | ||
| assert!(q.contains("block_timestamp >= 1000")); | ||
| assert!(q.contains("block_timestamp <= 2000")); | ||
|
|
||
| // Order hash trimmed, lowercased, and quotes sanitized | ||
| // We expect lower('0xabc''def') to appear at least once. | ||
| assert!(q.contains("lower('0xabc''def')")); | ||
| fn builds_with_time_filters() { | ||
| let stmt = build_fetch_trade_count_stmt("0xABC'DEF", Some(1000), Some(2000)).unwrap(); | ||
| // Time filter clauses present | ||
| assert!(!stmt.sql.contains(START_TS_CLAUSE)); | ||
| assert!(!stmt.sql.contains(END_TS_CLAUSE)); | ||
| assert!(stmt.sql.contains("block_timestamp >=")); | ||
| assert!(stmt.sql.contains("block_timestamp <=")); | ||
| // Params include order hash and two timestamps | ||
| assert_eq!(stmt.params.len(), 3); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests: assert param values and ordering
Lock the exact values so template/ordering regressions are caught.
fn builds_with_time_filters() {
- let stmt = build_fetch_trade_count_stmt("0xABC'DEF", Some(1000), Some(2000)).unwrap();
+ let stmt = build_fetch_trade_count_stmt("0xABC'DEF", Some(1000), Some(2000)).unwrap();
// Time filter clauses present
assert!(!stmt.sql.contains(START_TS_CLAUSE));
assert!(!stmt.sql.contains(END_TS_CLAUSE));
assert!(stmt.sql.contains("block_timestamp >="));
assert!(stmt.sql.contains("block_timestamp <="));
// Params include order hash and two timestamps
assert_eq!(stmt.params.len(), 3);
+ assert_eq!(stmt.params[0], SqlValue::Text("0xABC'DEF".to_string()));
+ assert_eq!(stmt.params[1], SqlValue::I64(1000));
+ assert_eq!(stmt.params[2], SqlValue::I64(2000));
}
@@
fn builds_without_time_filters_when_none() {
let stmt = build_fetch_trade_count_stmt("hash", None, None).unwrap();
assert!(!stmt.sql.contains("block_timestamp >="));
assert!(!stmt.sql.contains("block_timestamp <="));
assert!(!stmt.sql.contains(START_TS_CLAUSE));
assert!(!stmt.sql.contains(END_TS_CLAUSE));
assert_eq!(stmt.params.len(), 1);
+ assert_eq!(stmt.params[0], SqlValue::Text("hash".to_string()));
}Also applies to: 80-86
🤖 Prompt for AI Agents
In crates/common/src/local_db/query/fetch_order_trades_count/mod.rs around lines
66 to 76 (and likewise update the test at lines 80-86), the tests only assert
the params length; change them to assert the exact parameter values and ordering
so regressions are caught: replace the len() check with an assertion that
stmt.params equals the expected sequence [order_hash, start_ts, end_ts] (i.e.
"0xABC'DEF", 1000, 2000) in the correct order, and apply the same exact-value
assertions to the other test block at lines 80-86.
| // Optional time filters | ||
| let start_param = if let Some(v) = start_timestamp { | ||
| let i = i64::try_from(v).map_err(|e| { | ||
| SqlBuildError::new(format!( | ||
| "start_timestamp out of range for i64: {} ({})", | ||
| v, e | ||
| )) | ||
| })?; | ||
| Some(SqlValue::I64(i)) | ||
| } else { | ||
| None | ||
| }; | ||
| stmt.bind_param_clause(START_TS_CLAUSE, START_TS_BODY, start_param)?; |
There was a problem hiding this comment.
Add start<=end validation (align with trade_count builder)
fetch_order_trades_count validates inverted ranges; this builder should too for consistency and early erroring.
// Optional time filters
+ if let (Some(start), Some(end)) = (start_timestamp, end_timestamp) {
+ if start > end {
+ return Err(SqlBuildError::new("start_timestamp > end_timestamp"));
+ }
+ }Keeps behavior uniform across builders.
🤖 Prompt for AI Agents
crates/common/src/local_db/query/fetch_order_trades/mod.rs around lines 78 to
90: add the same start<=end validation used in the trade_count builder before
binding params — if both start_timestamp and end_timestamp are Some, convert
both to i64 (using try_from with the same map_err -> SqlBuildError pattern) and
return an SqlBuildError (with a clear message like "start_timestamp must be <=
end_timestamp") if start > end; perform this check before building/binding the
START_TS_CLAUSE so inverted ranges error early and behavior matches the count
builder.
| fn owners_tokens_and_order_hash_filters_with_params() { | ||
| let mut args = mk_args(); | ||
| args.filter = FetchOrdersActiveFilter::Active; | ||
| args.owners = vec![" 0xA ".into(), "".into(), "O'Owner".into()]; | ||
| args.tokens = vec!["TOK'A".into(), " ".into()]; | ||
| args.order_hash = Some(" 0xHash ' ".into()); | ||
|
|
||
| let q = build_fetch_orders_query(&args); | ||
| args.owners = vec![" 0xA ".into(), "".into(), "Owner".into()]; | ||
| args.tokens = vec!["TOKA".into(), " ".into()]; | ||
| args.order_hash = Some(" 0xHash ".into()); | ||
|
|
||
| // Active filter string inserted | ||
| assert!(q.contains("'active'")); | ||
| let stmt = build_fetch_orders_stmt(&args).unwrap(); | ||
|
|
||
| // Owners filter inserted as IN clause with lowercase + sanitized values | ||
| assert!(q.contains("AND lower(l.order_owner) IN ('0xa', 'o''owner')")); | ||
| // Active filter parameterized | ||
| assert!(stmt.sql.contains("?1 = 'active'")); | ||
|
|
||
| // Tokens filter inserted as IN clause with lowercase + sanitized values | ||
| assert!(q.contains("lower(io2.token) IN ('tok''a')")); | ||
| // Owners clause present, tokens clause present, order hash clause present | ||
| assert!(!stmt.sql.contains(OWNERS_CLAUSE)); | ||
| assert!(!stmt.sql.contains(TOKENS_CLAUSE)); | ||
| assert!(!stmt.sql.contains(ORDER_HASH_CLAUSE)); | ||
|
|
||
| // Order hash filter present, sanitized, applied to COALESCE hash | ||
| // Note: builder relies on SQL lower(...) rather than pre-lowercasing the literal. | ||
| assert!(q.contains("lower(COALESCE(la.order_hash, l.order_hash)) = lower('0xHash ''')")); | ||
|
|
||
| // No placeholders remain | ||
| assert!(!q.contains("?filter_owners")); | ||
| assert!(!q.contains("?filter_tokens")); | ||
| assert!(!q.contains("?filter_order_hash")); | ||
| // Params include at least one for the active filter | ||
| assert!(!stmt.params.is_empty()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests: also assert param values and ordering
Strengthen the test to pin exact params and order (active, owners…, order_hash, tokens…).
fn owners_tokens_and_order_hash_filters_with_params() {
@@
- // Params include at least one for the active filter
- assert!(!stmt.params.is_empty());
+ // Expect params: active filter (?1), owners (deduped, lowercased), order_hash, tokens
+ assert!(stmt.params.len() >= 3);
+ assert_eq!(stmt.params[0], SqlValue::Text("active".into()));
+ // Owners: " 0xA " -> "0xa", "Owner" -> "owner"
+ assert!(stmt.params.contains(&SqlValue::Text("0xa".into())));
+ assert!(stmt.params.contains(&SqlValue::Text("owner".into())));
+ // Order hash trimmed
+ assert!(stmt.params.contains(&SqlValue::Text("0xHash".into())));
+ // Tokens: "TOKA" -> "toka"
+ assert!(stmt.params.contains(&SqlValue::Text("toka".into())));🤖 Prompt for AI Agents
In crates/common/src/local_db/query/fetch_orders/mod.rs around lines 148 to 167,
update the test to assert exact parameter values and ordering rather than just
non-emptiness: after building stmt, trim and filter out empty/whitespace inputs
the same way production code does, then assert stmt.params equals the ordered
list ["active", "0xA", "Owner", "0xHash", "TOKA"] (or the equivalent parameter
representations used by build_fetch_orders_stmt) to enforce the parameter
sequence (active, owners…, order_hash, tokens…); also assert stmt.params.len()
matches this expected count.
| // Owners list (trim, non-empty, lowercase) with order-preserving dedup | ||
| let mut owners: Vec<String> = Vec::new(); | ||
| let mut seen: HashSet<String> = HashSet::new(); | ||
| for owner in args.owners.iter() { | ||
| let t = owner.trim(); | ||
| if t.is_empty() { | ||
| continue; | ||
| } | ||
| let lowered = t.to_ascii_lowercase(); | ||
| if seen.insert(lowered.clone()) { | ||
| owners.push(lowered); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract a shared normalize+dedup helper to remove duplication.
Same pattern for owners/tokens; factor into one function for clarity and fewer bugs.
Add helper (outside this hunk):
fn normalize_dedup_ordered<'a>(inp: impl Iterator<Item = &'a String>) -> Vec<String> {
use std::collections::HashSet;
let mut out = Vec::new();
let mut seen = HashSet::new();
for s in inp {
let t = s.trim();
if t.is_empty() {
continue;
}
let low = t.to_ascii_lowercase();
if seen.insert(low.clone()) {
out.push(low);
}
}
out
}Then simplify both blocks:
- let mut owners: Vec<String> = Vec::new();
- let mut seen: HashSet<String> = HashSet::new();
- for owner in args.owners.iter() {
- let t = owner.trim();
- if t.is_empty() {
- continue;
- }
- let lowered = t.to_ascii_lowercase();
- if seen.insert(lowered.clone()) {
- owners.push(lowered);
- }
- }
+ let owners = normalize_dedup_ordered(args.owners.iter());
- let mut tokens: Vec<String> = Vec::new();
- let mut seen_tokens: HashSet<String> = HashSet::new();
- for token in args.tokens.iter() {
- let t = token.trim();
- if t.is_empty() {
- continue;
- }
- let lowered = t.to_ascii_lowercase();
- if seen_tokens.insert(lowered.clone()) {
- tokens.push(lowered);
- }
- }
+ let tokens = normalize_dedup_ordered(args.tokens.iter());Also applies to: 50-62
🤖 Prompt for AI Agents
In crates/common/src/local_db/query/fetch_vaults/mod.rs around lines 31-43 (and
similarly lines 50-62), extract the repeated normalize+dedup logic into a single
helper function (placed outside this hunk, e.g., in the module scope) that
accepts an iterator of &String, trims, skips empty, lowercases, and preserves
insertion order while deduplicating via a HashSet; then replace the two existing
owner/token loops with calls to that helper to return Vec<String>, ensuring you
import std::collections::HashSet in the helper and update the call sites to use
the helper instead of inline duplication.
| let stmt = build_fetch_vaults_stmt(1, &args).unwrap(); | ||
| assert!(stmt.sql.contains("et.chain_id = ?1")); | ||
| assert!(!stmt.sql.contains(OWNERS_CLAUSE)); | ||
| assert!(!stmt.sql.contains(TOKENS_CLAUSE)); | ||
| assert!(!stmt.sql.contains(HIDE_ZERO_BALANCE_CLAUSE)); | ||
| assert_eq!(stmt.params.len(), 1); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use accessors instead of direct field access to decouple tests from struct layout.
Reduces coupling and future breakage if fields become private.
- assert!(stmt.sql.contains("et.chain_id = ?1"));
- assert!(!stmt.sql.contains(OWNERS_CLAUSE));
- assert!(!stmt.sql.contains(TOKENS_CLAUSE));
- assert!(!stmt.sql.contains(HIDE_ZERO_BALANCE_CLAUSE));
- assert_eq!(stmt.params.len(), 1);
+ assert!(stmt.sql().contains("et.chain_id = ?1"));
+ assert!(!stmt.sql().contains(OWNERS_CLAUSE));
+ assert!(!stmt.sql().contains(TOKENS_CLAUSE));
+ assert!(!stmt.sql().contains(HIDE_ZERO_BALANCE_CLAUSE));
+ assert_eq!(stmt.params().len(), 1);- assert!(!stmt.sql.contains(OWNERS_CLAUSE));
- assert!(!stmt.sql.contains(TOKENS_CLAUSE));
- assert!(!stmt.sql.contains(HIDE_ZERO_BALANCE_CLAUSE));
- assert!(stmt.sql.contains("AND NOT FLOAT_IS_ZERO("));
+ assert!(!stmt.sql().contains(OWNERS_CLAUSE));
+ assert!(!stmt.sql().contains(TOKENS_CLAUSE));
+ assert!(!stmt.sql().contains(HIDE_ZERO_BALANCE_CLAUSE));
+ assert!(stmt.sql().contains("AND NOT FLOAT_IS_ZERO("));- let bad_template = QUERY_TEMPLATE.replace(HIDE_ZERO_BALANCE_CLAUSE, "");
+ let bad_template = QUERY_TEMPLATE.replace(HIDE_ZERO_BALANCE_CLAUSE, "");Also applies to: 104-113, 116-125
🤖 Prompt for AI Agents
In crates/common/src/local_db/query/fetch_vaults/mod.rs around lines 90-96 (and
similarly at 104-113 and 116-125), the test directly accesses stmt.sql and
stmt.params.len(), coupling the test to the struct layout; replace direct field
access with the public accessor methods (e.g., use stmt.sql() and stmt.params()
or the equivalent getters provided by the statement type), or add small public
getter methods if none exist, and update the assertions to call those accessors
(e.g., assert that stmt.sql().contains(...), assert!(!stmt.sql().contains(...)),
and assert_eq!(stmt.params().len(), 1)) so tests no longer depend on internal
fields.
| // If there are no parameters, pass `undefined` to the JS callback | ||
| // instead of an empty array to match the SDK's expected semantics. | ||
| let js_params_val = if stmt.params.is_empty() { | ||
| JsValue::UNDEFINED | ||
| } else { | ||
| serde_wasm_bindgen::to_value(&stmt.as_js_params()) | ||
| .map_err(|e| LocalDbQueryError::database(format!("encode params: {:?}", e)))? | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use SqlStatement accessors for forward compatibility.
Prefer getters to avoid tight coupling with struct fields.
- let js_params_val = if stmt.params.is_empty() {
+ let js_params_val = if stmt.params().is_empty() {
JsValue::UNDEFINED
} else {
serde_wasm_bindgen::to_value(&stmt.as_js_params())
.map_err(|e| LocalDbQueryError::database(format!("encode params: {:?}", e)))?
};
...
- &JsValue::from_str(&stmt.sql),
+ &JsValue::from_str(stmt.sql()),
&js_params_val,Also applies to: 32-36
🤖 Prompt for AI Agents
crates/common/src/raindex_client/local_db/executor.rs around lines 21-28 (also
apply same change to lines 32-36): replace direct field access on SqlStatement
(e.g., stmt.params) with the public accessor methods provided by the
SqlStatement type for forward compatibility; specifically, call the params
getter (e.g., stmt.params()) when checking emptiness and pass the accessor
result into serde_wasm_bindgen::to_value (or use the provided as_js_params
accessor) so no struct fields are accessed directly.
| #[wasm_bindgen_test] | ||
| async fn empty_addresses_short_circuits_and_executes_no_sql() { | ||
| let store = Rc::new(RefCell::new(String::new())); | ||
| let store = Rc::new(RefCell::new(( | ||
| String::new(), | ||
| wasm_bindgen::JsValue::UNDEFINED, | ||
| ))); | ||
| let callback = create_sql_capturing_callback("[]", store.clone()); | ||
| let exec = JsCallbackExecutor::new(&callback); | ||
| let res = super::fetch_erc20_tokens_by_addresses(&exec, 1, &[]).await; | ||
| assert!(res.is_ok()); | ||
| assert!(res.unwrap().is_empty()); | ||
| assert!(store.borrow().is_empty()); | ||
| assert!(store.borrow().0.is_empty()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Also assert params remain undefined when short‑circuited.
Make the empty‑addresses test check that captured params were not set either.
- assert!(res.unwrap().is_empty());
- assert!(store.borrow().0.is_empty());
+ assert!(res.unwrap().is_empty());
+ let (sql, params) = store.borrow().clone();
+ assert!(sql.is_empty());
+ assert!(params.is_undefined());📝 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.
| #[wasm_bindgen_test] | |
| async fn empty_addresses_short_circuits_and_executes_no_sql() { | |
| let store = Rc::new(RefCell::new(String::new())); | |
| let store = Rc::new(RefCell::new(( | |
| String::new(), | |
| wasm_bindgen::JsValue::UNDEFINED, | |
| ))); | |
| let callback = create_sql_capturing_callback("[]", store.clone()); | |
| let exec = JsCallbackExecutor::new(&callback); | |
| let res = super::fetch_erc20_tokens_by_addresses(&exec, 1, &[]).await; | |
| assert!(res.is_ok()); | |
| assert!(res.unwrap().is_empty()); | |
| assert!(store.borrow().is_empty()); | |
| assert!(store.borrow().0.is_empty()); | |
| } | |
| #[wasm_bindgen_test] | |
| async fn empty_addresses_short_circuits_and_executes_no_sql() { | |
| let store = Rc::new(RefCell::new(( | |
| String::new(), | |
| wasm_bindgen::JsValue::UNDEFINED, | |
| ))); | |
| let callback = create_sql_capturing_callback("[]", store.clone()); | |
| let exec = JsCallbackExecutor::new(&callback); | |
| let res = super::fetch_erc20_tokens_by_addresses(&exec, 1, &[]).await; | |
| assert!(res.is_ok()); | |
| assert!(res.unwrap().is_empty()); | |
| let (sql, params) = store.borrow().clone(); | |
| assert!(sql.is_empty()); | |
| assert!(params.is_undefined()); | |
| } |
🤖 Prompt for AI Agents
In
crates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rs
around lines 26 to 38, the empty-addresses test needs to also assert that the
captured query parameters were not set when short-circuiting; update the test to
check the stored params remain wasm_bindgen::JsValue::UNDEFINED (e.g.
assert_eq!(store.borrow().1, wasm_bindgen::JsValue::UNDEFINED) or equivalent) in
addition to the existing assertions so the callback captured neither SQL nor
params.
|
@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
See issue: #2218
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor