Add chain id and orderbook address to local db logic#2278
Conversation
WalkthroughThis PR introduces Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Pipeline as Sync Pipeline
participant Query as Query Builder
participant DB as Database
participant Insert as Insert Logic
App->>Pipeline: initiate_sync(chain_id, orderbook_addr)
Pipeline->>Pipeline: create OrderbookIdentifier
Pipeline->>Query: fetch_last_synced_block_stmt(ob_id)
Query->>DB: SELECT WHERE chain_id=?1 AND orderbook_addr=?2
DB-->>Query: last_block
Query-->>Pipeline: last_block
Pipeline->>Pipeline: fetch decoded_events
Pipeline->>Insert: decoded_events_to_statements(ob_id, events, decimals)
Insert->>Insert: build EventContext with ob_id
Insert->>Insert: generate SQL with chain_id + orderbook_addr prefix
Insert-->>Pipeline: SqlStatementBatch
Pipeline->>Query: build_update_last_synced_block_stmt(ob_id, new_block)
Query->>DB: UPSERT sync_status WHERE (chain_id, orderbook_addr)
Pipeline->>DB: execute batch
DB-->>Pipeline: ✓ success
Pipeline-->>App: sync_complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related issues
Possibly related PRs
Suggested labels
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: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
crates/common/src/local_db/query/fetch_vaults/query.sql (1)
10-46: Remove ORDER BY non-selected column for deterministic DISTINCT resultsSQLite allows ORDER BY expressions that are not in the SELECT list for DISTINCT queries. However, ordering by
oe.order_hashwhile deduplicating on theitemcolumn creates non-deterministic results. Order by the selected aliasiteminstead to ensure deterministic output aligned with the deduplication key:- ORDER BY oe.order_hash + ORDER BY itemApply to both input and output subqueries (lines 10-46 and 48-84).
crates/common/src/local_db/pipeline/adapters/window.rs (1)
77-82: Minor: avoid variable shadowing for clarity (and clippy pedantic)
targetshadows the function parameter name. Rename to end_block to avoid confusion.- let requested_target = cfg.window_overrides.end_block.unwrap_or(safe_head); - let target = requested_target.min(safe_head); - Ok((start, target)) + let requested_end = cfg.window_overrides.end_block.unwrap_or(safe_head); + let end_block = requested_end.min(safe_head); + Ok((start, end_block))crates/common/src/local_db/query/fetch_order_trades/mod.rs (1)
1-6: Optional: normalize address string if DB uses lowercase.If the DB schema stores addresses normalized to lowercase, bind
orderbook_address.to_string().to_lowercase()for consistency. Otherwise, keep as-is.Also applies to: 68-80
crates/common/src/local_db/pipeline/adapters/tokens.rs (2)
24-36: Avoid lossy cast from u64 to u32 for chain_idCasting u64 to u32 can truncate and will trigger clippy (with warnings denied). Either:
- Make load_existing take chain_id: u32, or
- Use u32::try_from(chain_id)? and propagate an error.
Example:
- async fn load_existing<DB>( + async fn load_existing<DB>( &self, db: &DB, - chain_id: u64, + chain_id: u32, orderbook_address: Address, token_addrs_lower: &[Address], ) -> Result<Vec<Erc20TokenRow>, LocalDbError> ... - let Some(stmt) = build_fetch_stmt(chain_id as u32, orderbook_address, token_addrs_lower)? + let Some(stmt) = build_fetch_stmt(chain_id, orderbook_address, token_addrs_lower)? else { return Ok(vec![]); };If the trait requires u64 elsewhere, prefer:
- let Some(stmt) = build_fetch_stmt(chain_id as u32, orderbook_address, token_addrs_lower)? + let cid = u32::try_from(chain_id).map_err(|_| LocalDbError::CustomError("chain_id > u32".into()))?; + let Some(stmt) = build_fetch_stmt(cid, orderbook_address, token_addrs_lower)?
24-30: Arg naming: drop_lowersuffixtoken_addrs_lower is Address-typed, not lowercased strings. Rename to token_addrs for clarity.
crates/cli/src/commands/local_db/sync/runner/mod.rs (2)
356-370: Fix type of transaction_hash in test helpers (now Bytes, not String)DecodedEventData.transaction_hash uses alloy::primitives::Bytes. Current literals ("0xabc"/"0xstore") will not compile. Update:
- use alloy::primitives::{Address, FixedBytes, U256}; + use alloy::primitives::{Address, Bytes, FixedBytes, U256}; ... - transaction_hash: "0xabc".into(), + transaction_hash: Bytes::from_str("0xabc").unwrap(), ... - transaction_hash: "0xstore".into(), + transaction_hash: Bytes::from_str("0xdeadbeef").unwrap(),
140-153: Parse orderbook address once to avoid duplicate parsingYou parse Address::from_str(params.orderbook_address)? twice. Compute once and reuse:
- let existing_stores = fetch_existing_store_addresses( - self.db_path, - params.chain_id, - Address::from_str(params.orderbook_address)?, - ) + let ob_addr = Address::from_str(params.orderbook_address)?; + let existing_stores = fetch_existing_store_addresses( + self.db_path, + params.chain_id, + ob_addr, + ) .await?; ... - orderbook_address: Address::from_str(params.orderbook_address)?, + orderbook_address: ob_addr,crates/common/src/local_db/sync.rs (3)
70-77: Guard against empty range (start_block > latest_block).BlockRange::inclusive will error if latest_block < start_block (e.g., when deployment_block is ahead of latest). Short‑circuit early and skip syncing in that case.
Apply this minimal guard:
- let range = BlockRange::inclusive(start_block, latest_block)?; + if latest_block < start_block { + status.send(format!( + "No new blocks to sync: start={} > latest={}", + start_block, latest_block + ))?; + return Ok(()); + } + let range = BlockRange::inclusive(start_block, latest_block)?;
94-97: Map store address parse errors to LocalDbError with context.Address::from_str errors bubble via ? without context; prefer mapping to LocalDbError and include the offending string/index.
Use index-aware mapping:
- let store_addresses: Vec<Address> = store_addresses_vec - .iter() - .map(|s| Address::from_str(s)) - .collect::<Result<_, _>>()?; + let store_addresses: Vec<Address> = store_addresses_vec + .iter() + .enumerate() + .map(|(i, s)| Address::from_str(s).map_err(|e| { + LocalDbError::CustomError(format!("invalid store address at index {i}: {s} ({e})")) + })) + .collect::<Result<_, _>>()?;
342-346: Prefer explicit Bytes parsing in tests.Use Bytes::from_str(...) instead of ".into()" for clarity and to avoid relying on implicit conversions that may change.
Example:
-transaction_hash: "0x10".into(), +transaction_hash: Bytes::from_str("0x10").unwrap(),Also applies to: 641-647, 661-667, 681-687
crates/cli/src/commands/local_db/sync/data_source.rs (1)
95-103: Parse orderbook_address with context.Add context to Address::from_str failure to aid debugging.
-Address::from_str(orderbook_address)?, +Address::from_str(orderbook_address) + .with_context(|| format!("invalid orderbook address: {orderbook_address}"))?,crates/common/src/local_db/insert.rs (3)
509-551: Bug: RemoveOrderV3 event_type is in the wrong VALUES position.'RemoveOrderV3' currently occupies the log_index slot; values are shifted/misaligned.
Fix VALUES ordering to match columns (put constant after log_index):
- ) VALUES ( - ?1, - ?2, - ?3, - ?4, - ?5, - 'RemoveOrderV3', - ?6, - ?7, - ?8, - ?9, - ?10, - ?11 + ) VALUES ( + ?1, + ?2, + ?3, + ?4, + ?5, + ?6, + 'RemoveOrderV3', + ?7, + ?8, + ?9, + ?10, + ?11
771-841: Bug: clear_v3_events has 21 columns but only 20 placeholders.VALUES list stops at ?20; last column (bob_output_vault_id) never bound. Runtime bind error risk.
Add the missing placeholder:
) VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15, ?16, ?17, ?18, - ?19, - ?20 + ?19, + ?20, + ?21 );Update the unit test to assert the new highest placeholder:
-assert!(statement.sql().contains("?20")); +assert!(statement.sql().contains("?21"));
154-160: Multiple instances of.to_string()on Address/Bytes types found; should usehex::encode_prefixed()for consistency.The review correctly identifies real instances where hex encoding should be explicit:
- Line 240, 286:
orderbook_address.to_string()in SQL params- Lines 369, 426, 483, 542, 606, 656, 701, 822, 887, 935, 984, 1025, 1041:
transaction_hash.to_string()in SQL params- Line 154-160: eprintln using
event.transaction_hashdirectly instead of hex-encodedThe codebase imports
alloy::hexand already useshex::encode_prefixed()elsewhere (lines 314, 981, 986+), establishing inconsistent encoding patterns.Unable to verify clippy output (nix unavailable in sandbox), but please manually run:
nix develop -c cargo clippy --workspace --all-targets --all-features -D warningsto confirm no new warnings are introduced by applying the suggested refactors across all listed line ranges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (52)
crates/cli/src/commands/local_db/sync/data_source.rs(2 hunks)crates/cli/src/commands/local_db/sync/runner/apply.rs(15 hunks)crates/cli/src/commands/local_db/sync/runner/mod.rs(12 hunks)crates/cli/src/commands/local_db/sync/runner/window.rs(5 hunks)crates/cli/src/commands/local_db/sync/storage.rs(9 hunks)crates/cli/src/commands/local_db/sync/token.rs(5 hunks)crates/common/src/local_db/decode.rs(4 hunks)crates/common/src/local_db/insert.rs(62 hunks)crates/common/src/local_db/mod.rs(3 hunks)crates/common/src/local_db/pipeline/adapters/apply.rs(24 hunks)crates/common/src/local_db/pipeline/adapters/tokens.rs(5 hunks)crates/common/src/local_db/pipeline/adapters/window.rs(5 hunks)crates/common/src/local_db/pipeline/traits.rs(1 hunks)crates/common/src/local_db/query/create_tables/query.sql(9 hunks)crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs(2 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(3 hunks)crates/common/src/local_db/query/fetch_last_synced_block/query.sql(1 hunks)crates/common/src/local_db/query/fetch_order_trades/mod.rs(3 hunks)crates/common/src/local_db/query/fetch_order_trades/query.sql(14 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(5 hunks)crates/common/src/local_db/query/fetch_orders/mod.rs(5 hunks)crates/common/src/local_db/query/fetch_orders/query.sql(1 hunks)crates/common/src/local_db/query/fetch_store_addresses/mod.rs(2 hunks)crates/common/src/local_db/query/fetch_store_addresses/query.sql(1 hunks)crates/common/src/local_db/query/fetch_target_watermark/mod.rs(1 hunks)crates/common/src/local_db/query/fetch_target_watermark/query.sql(1 hunks)crates/common/src/local_db/query/fetch_vault/mod.rs(3 hunks)crates/common/src/local_db/query/fetch_vault/query.sql(3 hunks)crates/common/src/local_db/query/fetch_vault_balance_changes/mod.rs(3 hunks)crates/common/src/local_db/query/fetch_vault_balance_changes/query.sql(2 hunks)crates/common/src/local_db/query/fetch_vaults/mod.rs(4 hunks)crates/common/src/local_db/query/fetch_vaults/query.sql(3 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(25 hunks)crates/common/src/local_db/views/vault_deltas.sql(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rs(4 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(3 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(9 hunks)crates/common/src/raindex_client/local_db/query/fetch_store_addresses.rs(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault.rs(7 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault_balance_changes.rs(5 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(3 hunks)crates/common/src/raindex_client/orders.rs(3 hunks)crates/common/src/raindex_client/trades.rs(4 hunks)crates/common/src/raindex_client/vaults.rs(3 hunks)packages/webapp/src/lib/services/startLocalDbSync.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/local_db/query/fetch_vault_balance_changes.rscrates/common/src/local_db/query/update_last_synced_block/mod.rscrates/common/src/raindex_client/local_db/query/fetch_orders.rscrates/common/src/local_db/decode.rscrates/common/src/local_db/query/fetch_vault_balance_changes/mod.rscrates/common/src/raindex_client/local_db/query/fetch_order_trades.rscrates/common/src/local_db/query/fetch_order_trades_count/mod.rscrates/common/src/local_db/query/fetch_target_watermark/mod.rscrates/common/src/local_db/query/fetch_store_addresses/mod.rscrates/common/src/local_db/pipeline/traits.rscrates/common/src/local_db/query/fetch_vault/mod.rscrates/common/src/local_db/query/fetch_vaults/mod.rscrates/common/src/raindex_client/local_db/query/fetch_store_addresses.rscrates/common/src/raindex_client/local_db/query/fetch_vault.rscrates/cli/src/commands/local_db/sync/token.rscrates/common/src/raindex_client/local_db/query/update_last_synced_block.rscrates/cli/src/commands/local_db/sync/runner/apply.rscrates/common/src/local_db/mod.rscrates/common/src/local_db/query/fetch_orders/mod.rscrates/cli/src/commands/local_db/sync/data_source.rscrates/common/src/raindex_client/local_db/query/fetch_vaults.rscrates/common/src/local_db/pipeline/adapters/window.rscrates/common/src/raindex_client/local_db/query/fetch_order_trades_count.rscrates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rscrates/common/src/local_db/query/fetch_order_trades/mod.rscrates/common/src/raindex_client/local_db/query/fetch_last_synced_block.rscrates/common/src/raindex_client/trades.rscrates/common/src/local_db/query/fetch_last_synced_block/mod.rscrates/cli/src/commands/local_db/sync/runner/window.rscrates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rscrates/common/src/raindex_client/vaults.rscrates/cli/src/commands/local_db/sync/storage.rscrates/common/src/local_db/pipeline/adapters/tokens.rscrates/common/src/raindex_client/orders.rscrates/cli/src/commands/local_db/sync/runner/mod.rscrates/common/src/local_db/insert.rscrates/common/src/local_db/pipeline/adapters/apply.rscrates/common/src/local_db/sync.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/local_db/query/fetch_vault_balance_changes.rscrates/common/src/local_db/query/update_last_synced_block/mod.rscrates/common/src/raindex_client/local_db/query/fetch_orders.rscrates/common/src/local_db/decode.rscrates/common/src/local_db/query/fetch_vault_balance_changes/mod.rscrates/common/src/raindex_client/local_db/query/fetch_order_trades.rscrates/common/src/local_db/query/fetch_order_trades_count/mod.rscrates/common/src/local_db/query/fetch_target_watermark/mod.rscrates/common/src/local_db/query/fetch_store_addresses/mod.rscrates/common/src/local_db/pipeline/traits.rscrates/common/src/local_db/query/fetch_vault/mod.rscrates/common/src/local_db/query/fetch_vaults/mod.rscrates/common/src/raindex_client/local_db/query/fetch_store_addresses.rscrates/common/src/raindex_client/local_db/query/fetch_vault.rscrates/cli/src/commands/local_db/sync/token.rscrates/common/src/raindex_client/local_db/query/update_last_synced_block.rscrates/cli/src/commands/local_db/sync/runner/apply.rscrates/common/src/local_db/mod.rscrates/common/src/local_db/query/fetch_orders/mod.rscrates/cli/src/commands/local_db/sync/data_source.rscrates/common/src/raindex_client/local_db/query/fetch_vaults.rscrates/common/src/local_db/pipeline/adapters/window.rscrates/common/src/raindex_client/local_db/query/fetch_order_trades_count.rscrates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rscrates/common/src/local_db/query/fetch_order_trades/mod.rscrates/common/src/raindex_client/local_db/query/fetch_last_synced_block.rscrates/common/src/raindex_client/trades.rscrates/common/src/local_db/query/fetch_last_synced_block/mod.rscrates/cli/src/commands/local_db/sync/runner/window.rscrates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rscrates/common/src/raindex_client/vaults.rscrates/cli/src/commands/local_db/sync/storage.rscrates/common/src/local_db/pipeline/adapters/tokens.rscrates/common/src/raindex_client/orders.rscrates/cli/src/commands/local_db/sync/runner/mod.rscrates/common/src/local_db/insert.rscrates/common/src/local_db/pipeline/adapters/apply.rscrates/common/src/local_db/sync.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/local_db/query/fetch_vault_balance_changes.rscrates/common/src/local_db/query/update_last_synced_block/mod.rscrates/common/src/raindex_client/local_db/query/fetch_orders.rscrates/common/src/local_db/decode.rscrates/common/src/local_db/query/fetch_vault_balance_changes/mod.rscrates/common/src/raindex_client/local_db/query/fetch_order_trades.rscrates/common/src/local_db/query/fetch_order_trades_count/mod.rscrates/common/src/local_db/query/fetch_target_watermark/mod.rscrates/common/src/local_db/query/fetch_store_addresses/mod.rscrates/common/src/local_db/pipeline/traits.rscrates/common/src/local_db/query/fetch_vault/mod.rscrates/common/src/local_db/query/fetch_vaults/mod.rscrates/common/src/raindex_client/local_db/query/fetch_store_addresses.rscrates/common/src/raindex_client/local_db/query/fetch_vault.rscrates/cli/src/commands/local_db/sync/token.rscrates/common/src/raindex_client/local_db/query/update_last_synced_block.rscrates/cli/src/commands/local_db/sync/runner/apply.rscrates/common/src/local_db/mod.rscrates/common/src/local_db/query/fetch_orders/mod.rscrates/cli/src/commands/local_db/sync/data_source.rscrates/common/src/raindex_client/local_db/query/fetch_vaults.rscrates/common/src/local_db/pipeline/adapters/window.rscrates/common/src/raindex_client/local_db/query/fetch_order_trades_count.rscrates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rscrates/common/src/local_db/query/fetch_order_trades/mod.rscrates/common/src/raindex_client/local_db/query/fetch_last_synced_block.rscrates/common/src/raindex_client/trades.rscrates/common/src/local_db/query/fetch_last_synced_block/mod.rscrates/cli/src/commands/local_db/sync/runner/window.rscrates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rscrates/common/src/raindex_client/vaults.rscrates/cli/src/commands/local_db/sync/storage.rscrates/common/src/local_db/pipeline/adapters/tokens.rscrates/common/src/raindex_client/orders.rscrates/cli/src/commands/local_db/sync/runner/mod.rscrates/common/src/local_db/insert.rscrates/common/src/local_db/pipeline/adapters/apply.rscrates/common/src/local_db/sync.rs
packages/webapp/**/*.{svelte,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Webapp must pass Svelte lint/format checks (npm run svelte-lint-format-check -w @rainlanguage/webapp)
Files:
packages/webapp/src/lib/services/startLocalDbSync.ts
packages/{webapp,ui-components}/**/*.{svelte,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
If you modify frontend code, you must include a screenshot of the built webapp reflecting the change
Files:
packages/webapp/src/lib/services/startLocalDbSync.ts
packages/**
📄 CodeRabbit inference engine (AGENTS.md)
JavaScript/Svelte projects live under packages/* (webapp, ui-components, orderbook)
Files:
packages/webapp/src/lib/services/startLocalDbSync.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2202
File: crates/common/src/raindex_client/local_db/sync.rs:33-34
Timestamp: 2025-10-14T07:51:55.148Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the hard-coded `DEFAULT_SYNC_CHAIN_ID` constant (set to `SUPPORTED_LOCAL_DB_CHAINS[0]`) will be replaced with proper chain ID handling in downstream PRs as part of the multi-network/orderbook implementation.
Learnt from: findolor
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.
📚 Learning: 2025-10-06T11:28:30.692Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2145
File: crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql:6-7
Timestamp: 2025-10-06T11:28:30.692Z
Learning: In `crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql`, the orderbook_address is currently hardcoded to '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' because the system only supports a single orderbook at the moment. Multiorderbook logic is not yet implemented and will be added in the future.
Applied to files:
crates/common/src/raindex_client/local_db/query/fetch_orders.rscrates/common/src/raindex_client/local_db/query/fetch_order_trades.rscrates/common/src/raindex_client/local_db/query/fetch_vault.rscrates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rscrates/cli/src/commands/local_db/sync/storage.rscrates/common/src/raindex_client/orders.rscrates/common/src/local_db/sync.rs
📚 Learning: 2025-05-14T04:49:14.621Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1710
File: crates/quote/Cargo.toml:30-30
Timestamp: 2025-05-14T04:49:14.621Z
Learning: In the rain.orderbook repository, wasm-bindgen-utils is intentionally used as an unconditional dependency because non-wasm targets (like tests) sometimes need to use it.
Applied to files:
crates/common/src/raindex_client/local_db/query/fetch_orders.rs
📚 Learning: 2025-04-30T09:28:36.960Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1715
File: crates/js_api/src/common/mod.rs:111-118
Timestamp: 2025-04-30T09:28:36.960Z
Learning: In the rain.orderbook repository, the WASM tests are already properly configured with conditional compilation using `#[cfg(target_family = "wasm")]` and `#[cfg(not(target_family = "wasm"))]`, and don't require additional `wasm_bindgen_test_configure!(run_in_browser)` directives.
Applied to files:
crates/common/src/raindex_client/local_db/query/fetch_orders.rs
📚 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/common/src/raindex_client/local_db/query/fetch_order_trades.rscrates/common/src/raindex_client/local_db/query/fetch_vault.rscrates/common/src/raindex_client/local_db/query/fetch_last_synced_block.rscrates/common/src/raindex_client/trades.rscrates/common/src/raindex_client/vaults.rscrates/cli/src/commands/local_db/sync/storage.rscrates/common/src/raindex_client/orders.rscrates/common/src/local_db/sync.rs
📚 Learning: 2025-10-28T14:11:56.648Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2277
File: crates/common/src/local_db/query/create_tables/query.sql:11-19
Timestamp: 2025-10-28T14:11:56.648Z
Learning: In the target_watermarks table (crates/common/src/local_db/query/create_tables/query.sql), additional indexes beyond the composite primary key (chain_id, orderbook_address) are not needed because the table will have a small number of rows.
Applied to files:
crates/common/src/local_db/query/fetch_target_watermark/query.sql
📚 Learning: 2025-05-20T15:34:06.751Z
Learnt from: 0xgleb
PR: rainlanguage/rain.orderbook#1846
File: crates/quote/src/quote.rs:309-317
Timestamp: 2025-05-20T15:34:06.751Z
Learning: In Rust, when importing from the alloy crate, `use alloy::hex;` imports the hex macro, not a module, so it can't be consolidated with imports like `use alloy::hex::ToHex;` using the grouped import syntax.
Applied to files:
crates/cli/src/commands/local_db/sync/token.rs
📚 Learning: 2025-10-14T07:51:55.148Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2202
File: crates/common/src/raindex_client/local_db/sync.rs:33-34
Timestamp: 2025-10-14T07:51:55.148Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the hard-coded `DEFAULT_SYNC_CHAIN_ID` constant (set to `SUPPORTED_LOCAL_DB_CHAINS[0]`) will be replaced with proper chain ID handling in downstream PRs as part of the multi-network/orderbook implementation.
Applied to files:
crates/common/src/raindex_client/local_db/query/update_last_synced_block.rscrates/cli/src/commands/local_db/sync/runner/apply.rscrates/cli/src/commands/local_db/sync/data_source.rscrates/common/src/raindex_client/local_db/query/fetch_last_synced_block.rscrates/common/src/local_db/query/fetch_last_synced_block/mod.rscrates/cli/src/commands/local_db/sync/runner/window.rscrates/cli/src/commands/local_db/sync/storage.rscrates/common/src/local_db/query/create_tables/query.sqlcrates/common/src/local_db/sync.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/local_db/query/fetch_order_trades_count.rscrates/common/src/raindex_client/trades.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/trades.rs
📚 Learning: 2025-05-14T05:13:59.713Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/subgraph/src/orderbook_client.rs:44-46
Timestamp: 2025-05-14T05:13:59.713Z
Learning: In the rain.orderbook project, WASM dependencies are intentionally made available in non-WASM targets to facilitate testing of WASM-related functionality, so conditional compilation guards like `#[cfg(target_family = "wasm")]` should not be added to imports or implementations that may be needed for tests.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 Learning: 2025-08-01T09:07:20.383Z
Learnt from: brusherru
PR: rainlanguage/rain.orderbook#2044
File: packages/orderbook/README.md:186-189
Timestamp: 2025-08-01T09:07:20.383Z
Learning: In the rainlanguage/rain.orderbook project, Rust methods on structs like RaindexVaultsList are exported as JavaScript getters in WASM bindings using #[wasm_bindgen(getter)]. This means while the Rust code uses method calls like items(), the JavaScript/WASM API exposes them as property access like .items. The README.md correctly documents the JavaScript API surface, not the Rust implementation details.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 Learning: 2025-08-02T02:34:32.237Z
Learnt from: brusherru
PR: rainlanguage/rain.orderbook#2036
File: crates/js_api/src/filters/raindex_filter_store.rs:446-533
Timestamp: 2025-08-02T02:34:32.237Z
Learning: In the rainlanguage/rain.orderbook project's RaindexFilterStore WASM tests (crates/js_api/src/filters/raindex_filter_store.rs), brusherru decided to focus on testing only methods without side effects (that don't use web_sys) due to difficulties with mocking localStorage and window APIs in the WASM test environment. This pragmatic approach tests pure logic separately from browser integration.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 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-18T10:31:05.498Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2008
File: crates/common/src/raindex_client/add_orders.rs:85-86
Timestamp: 2025-07-18T10:31:05.498Z
Learning: In the rainlanguage/rain.orderbook codebase, cfg-guarded imports like `#[cfg(not(target_family = "wasm"))] use super::*;` in test modules are sometimes needed to fix compiler warnings, even when similar imports exist in nested sub-modules. These should not be removed as they serve a specific purpose in the compilation process.
Applied to files:
crates/common/src/raindex_client/trades.rs
📚 Learning: 2025-10-06T14:39:36.425Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2159
File: crates/cli/src/commands/local_db/sync/cli.rs:37-78
Timestamp: 2025-10-06T14:39:36.425Z
Learning: In the SyncLocalDb CLI command (crates/cli/src/commands/local_db/sync/cli.rs), block range validation (e.g., start_block <= end_block) is handled internally by the sync runner, not at the CLI entry point.
Applied to files:
crates/cli/src/commands/local_db/sync/runner/window.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: build-tauri (ubuntu-22.04, true)
- GitHub Check: test
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, test-js-bindings)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-static)
- GitHub Check: test
- GitHub Check: git-clean
- GitHub Check: test
- GitHub Check: Deploy-Preview-Push
- GitHub Check: Deploy-Docs-Preview
🔇 Additional comments (59)
crates/common/src/local_db/query/fetch_vaults/mod.rs (1)
93-101: Strengthen tests: assert bound params for chain/orderbook.Add assertions that:
params[0]is chain_idparams[1]isorderbook_address.to_string()- For hide_zero: ensure first two params are reused before list params.
@@ - assert_eq!(stmt.params.len(), 2); + assert_eq!(stmt.params.len(), 2); + assert!(matches!(stmt.params[0], SqlValue::I64(1))); + if let SqlValue::Text(ref ob) = stmt.params[1] { + assert_eq!(ob, &Address::ZERO.to_string()); + } else { + panic!("expected Text for orderbook address"); + } @@ - assert!(!stmt.params.is_empty()); + assert!(stmt.params.len() >= 2); + assert!(matches!(stmt.params[0], SqlValue::I64(137))); + if let SqlValue::Text(ref ob) = stmt.params[1] { + assert_eq!(ob, &Address::ZERO.to_string()); + }Also applies to: 109-118
crates/common/src/local_db/query/fetch_vault/mod.rs (1)
70-77: Assert address parameter usage in the template.Current test only checks
?3/?4. Also assert that the SQL contains the?2predicate for orderbook address to guard template drift.@@ - assert!(stmt.sql.contains("et.chain_id = ?1")); + assert!(stmt.sql.contains("et.chain_id = ?1")); + assert!(stmt.sql.contains("orderbook_address")); // ensure scoped by address + assert!(stmt.sql.contains("?2")); // address placeholder presentcrates/common/src/raindex_client/local_db/query/fetch_vault.rs (1)
7-16: Verification passed—all call sites correctly thread orderbook_address.The
fetch_vault()andbuild_fetch_vault_stmt()signatures are consistently updated across call sites (including test fixtures at lines 28, 63, 89). Hardcoded addresses found in test data and CLI documentation are expected and do not indicate incomplete refactoring.crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/query.sql (1)
3-3: Removelower()from column, but recognize this requires systemic changes across multiple queries.The concern is valid:
lower(orderbook_address)prevents SQLite from using theidx_erc20_tokens_tokenindex on(chain_id, orderbook_address, token_address). However, this pattern is systemic across the codebase (fetch_vaults, fetch_orders, fetch_target_watermark, etc.) and there is an explicit test assertion validating its presence, suggesting it's intentional.Recommended approach:
- Verify addresses are consistently normalized at write time across all insert/upsert operations
- Remove
lower()calls from both column and parameter uniformly across all affected queries- Update or remove corresponding assertions (e.g., in fetch_target_watermark/mod.rs:47)
- Test to confirm index utilization improves
For this specific query:
-WHERE chain_id = ?1 AND lower(orderbook_address) = lower(?2) +WHERE chain_id = ?1 AND orderbook_address = ?2Ensure the Rust binding (build_fetch_stmt) is already pushing normalized addresses via
orderbook_address.to_string(), which it appears to do through alloy's Address type.crates/common/src/local_db/query/fetch_store_addresses/query.sql (1)
3-6: Remove the suggestion to droplower()on the column—the Rust code doesn't normalize the parameter.The review suggests changing
lower(orderbook_address)to rely on normalized parameter binding, but the Rust function atcrates/common/src/local_db/query/fetch_store_addresses/mod.rs:16binds the address asSqlValue::from(orderbook_address.to_string())without calling.to_lowercase(). Ethereum addresses support checksummed mixed-case (e.g.,0x1234AbCd), so removing the SQL-sidelower()without normalizing the parameter would break case-insensitive matching.The TRIM suggestion for
store_addressis reasonable and independent of this issue, but the index-optimization strategy is incomplete as written.Likely an incorrect or invalid review comment.
crates/common/src/local_db/decode.rs (1)
4-4: Review comment is incorrect; the code already matches the suggested fix.The struct already has
pub transaction_hash: Bytes(notString), andBytes::from_str("")correctly returnsOk(Bytes::new())per alloy documentation. The test at line 934 validates this behavior by asserting empty transaction_hash parses successfully. No explicit empty handling is needed—the current parsing logic is correct and tested.The optional refactor to
B256remains a valid consideration for stricter typing, but it's not a fix for an existing issue.Likely an incorrect or invalid review comment.
crates/common/src/local_db/query/fetch_vault_balance_changes/mod.rs (1)
28-55: LGTM! Clean parameter threading.The function signature and implementation correctly thread chain_id and orderbook_address through to the SQL statement builder. Test coverage verifies parameter count and ordering, and the parameter binding logic (chain_id as I64, address as Text, trimmed strings) is appropriate.
crates/common/src/local_db/query/fetch_order_trades/query.sql (1)
1-415: LGTM! Consistent parameter threading throughout complex query.The query correctly threads chain_id (?1), orderbook_address (?2), and order_hash (?3) through all branches, joins, and subqueries. Case-insensitive address comparisons are applied uniformly with
lower()on both sides, and the composite key filtering is consistent across all three UNION ALL branches (take orders, alice clear, bob clear).crates/common/src/local_db/query/fetch_orders/query.sql (1)
1-136: LGTM! Correct parameter threading and active filter implementation.The query correctly threads chain_id (?1) and orderbook_address (?2) through all subqueries and joins with case-insensitive address comparisons. The new ?3 parameter implements a three-way active filter (all/active/inactive) that aligns with the computed
activefield. The GROUP BY clause correctly includes all necessary grouping dimensions including the newly exposed orderbook_address and event_type.crates/common/src/local_db/query/fetch_target_watermark/mod.rs (1)
16-48: LGTM! Comprehensive test coverage for address handling.The function correctly binds chain_id and orderbook_address parameters, and the test suite thoroughly verifies parameter ordering, address formatting (0x-prefixed lowercase hex), and case-insensitive SQL comparison. The defensive checks at line 47 ensure the WHERE clause uses the correct comparison semantics.
crates/common/src/local_db/query/update_last_synced_block/query.sql (1)
1-13: LGTM! Correct UPSERT implementation for composite key.The query correctly implements an INSERT...ON CONFLICT pattern for the composite key (chain_id, orderbook_address). The UPDATE clause properly references
excluded.last_synced_blockto use the new value and updatesupdated_atto track modification time.crates/common/src/local_db/query/fetch_target_watermark/query.sql (1)
1-3: LGTM! Case-insensitive address comparison applied correctly.The query correctly filters by chain_id and performs case-insensitive comparison on orderbook_address using
lower()on both sides, consistent with the broader pattern across the PR.Based on learnings
crates/common/src/raindex_client/local_db/query/fetch_vault_balance_changes.rs (1)
7-79: LGTM! Wrapper correctly threads new parameters.The function signature properly extends to include chain_id and orderbook_address, and delegates correctly to the builder. Test coverage verifies that the wrapper uses the builder's SQL exactly and handles parameter passing correctly.
crates/common/src/local_db/query/fetch_last_synced_block/query.sql (1)
1-8: LGTM! Consistent with composite key pattern.The query correctly returns both composite key fields (chain_id, orderbook_address) and applies case-insensitive address filtering. This aligns with the corresponding UPSERT in update_last_synced_block and maintains consistency across the sync status tracking.
crates/common/src/raindex_client/trades.rs (2)
134-138: WASM/non‑WASM orderbook Address handling looks correct.Parsing string to Address under WASM and using the Address directly otherwise is sound, and the value is threaded into fetch_order_trades. No action needed.
Also applies to: 147-151
252-256: Trade count path correctly threads chain_id and orderbook.Passing chain_id and orderbook into fetch_order_trades_count aligns with the new DB scoping and matches the builder signature. Looks good.
Also applies to: 262-270
crates/common/src/local_db/query/update_last_synced_block/mod.rs (2)
6-17: Good: statement now scoped by (chain_id, orderbook). Verify address encoding consistency.Binding parameters in order (?1 chain_id I64, ?2 orderbook Text, ?3 block I64) is correct. Here you use hex::encode_prefixed(orderbook_address); other modules use Address::to_string(). Please standardize one representation (prefer one of: hex::encode_prefixed or Address::to_string()) across all builders to avoid subtle mismatches.
Would you like me to scan the workspace for mixed encodings and propose a unifying change?
27-35: Test covers ON CONFLICT and param values; LGTM.crates/common/src/raindex_client/local_db/query/fetch_order_trades.rs (2)
10-21: API update and forwarding are consistent.Adding orderbook_address and passing it through to build_fetch_order_trades_stmt aligns with the PR-wide scoping changes.
38-45: WASM wrapper test precisely validates SQL; nice coverage.Also applies to: 53-55
packages/webapp/src/lib/services/startLocalDbSync.ts (1)
55-55: Confirm whether syncLocalDatabase also needs orderbookAddress.You pass (chainId, orderbookAddress) to getSyncStatus, but syncLocalDatabase still gets only chainId. If/when multi‑orderbook per chain lands, sync may also need the address to scope writes. For now this may be fine (single orderbook per chain) but please verify the intended API and add a TODO to propagate address when ready.
crates/common/src/raindex_client/local_db/query/fetch_order_trades_count.rs (2)
9-11: Scoping trade count by (chain_id, orderbook) is correct.Forwarding to build_fetch_trade_count_stmt with the new parameters is consistent with the rest of the query layer.
Also applies to: 15-21
42-45: WASM tests cover both non-empty and empty rows; LGTM.Also applies to: 55-57, 73-74
crates/common/src/raindex_client/local_db/query/fetch_vaults.rs (2)
38-42: Vault fetch now scoped by orderbook; good consistency.Passing orderbook_address into build_fetch_vaults_stmt matches the new schema constraints and address scoping.
92-94: WASM wrapper test asserts exact SQL; LGTM.Also applies to: 102-107
crates/common/src/raindex_client/local_db/query/fetch_store_addresses.rs (2)
7-12: Store addresses now filtered by (chain_id, orderbook); good.Wrapper delegates to fetch_store_addresses_stmt with the new parameters and the test confirms SQL parity.
27-29: WASM test validates SQL capture end-to-end; LGTM.Also applies to: 35-38
crates/common/src/local_db/query/fetch_store_addresses/mod.rs (1)
27-35: Test asserts param count and SQL body; LGTM.crates/common/src/raindex_client/vaults.rs (1)
315-323: Correct threading of chain_id and orderbook to local DB balance change fetchPassing chain_id and orderbook is the right fix for multi-tenant scoping. LGTM.
crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs (1)
24-27: Signature and param binding updates look correct
- Adds orderbook_address param and switches addresses to Address typed.
- Binds params as: ?1 chain_id, ?2 orderbook_address, then IN-list.
Hex-encoding via alloy::hex::encode_prefixed(Address) is appropriate for case-insensitive matching in SQL. LGTM.
Also applies to: 32-37, 41-45
crates/common/src/local_db/query/fetch_vault/query.sql (2)
118-126: Good: latest owner resolution now includes log_index and proper DESC orderingIncluding log_index in the projection and ordering by (block_number, log_index) removes ambiguity on same-block events. LGTM.
Also applies to: 131-132
10-16: The review comment is incorrect. SQLite supports ORDER BY expressions not in the SELECT list.SQLite's SELECT documentation states that for a simple SELECT an ORDER BY may contain any arbitrary expressions, which means
ORDER BY oe.order_hashin aSELECT DISTINCT itemquery is valid SQLite syntax. The restriction requiring ORDER BY columns to appear in the SELECT list exists in other databases like PostgreSQL or SQL Server, but not SQLite. The code at lines 10-16 and 44-45 is correct and does not need modification.Likely an incorrect or invalid review comment.
crates/common/src/local_db/pipeline/traits.rs (2)
225-226: Invariant update aligns with composite keyingKeying tokens by (chain_id, orderbook_address, lower(token_address)) matches the new schema. LGTM.
233-235: Manual verification needed for chain_id type consistency across layersBased on the grep results from your prior script, the
u64vsu32split forchain_idappears to follow a consistent pattern:
- Settings/config layer (crates/settings/): uses
chain_id: u32- Local DB layer (crates/common/src/local_db/): uses
chain_id: u64This split could be intentional (architectural separation between config and storage tiers), or it could represent technical debt. Verify:
- Whether this layer-specific split is documented architectural intent
- How chain IDs are converted when crossing from config (u32) to local DB (u64) layers
- If there's a risk of loss of precision (unlikely for chain IDs, but worth confirming)
Regarding the parameter name
token_addrs_lower: &[Address], this is a semantic hint to callers that addresses should be lowercase—this is a valid naming pattern and not a type mismatch.crates/common/src/local_db/query/fetch_orders/mod.rs (2)
60-61: Token EXISTS subquery scoping is correctFiltering io2 by (?1 chain_id, ?2 orderbook_address) before token IN reduces cross‑target bleed. Good.
146-179: Tests validate placeholder positions and param values—niceThe assertions around “?3 = ...” and first two params ensure the template and prelude stay in sync. This will catch regressions in marker/placeholder drift.
Also applies to: 181-187, 189-206
crates/common/src/local_db/pipeline/adapters/window.rs (2)
48-56: Watermark read path LGTMFetching TargetWatermarkRow via fetch_target_watermark_stmt and defaulting to 0 is correct for first‑sync scenarios.
108-128: Uniqueness constraint confirmed—code is safeVerified that
target_watermarkstable enforcesPRIMARY KEY (chain_id, orderbook_address)at line 18 incrates/common/src/local_db/query/create_tables/query.sql. This guarantees at most one row per target, makingrows.first()safe.crates/common/src/raindex_client/local_db/query/update_last_synced_block.rs (1)
5-13: All call sites successfully migrated to new signatureBoth test calls (lines 38 and 63) use the updated 4-argument form with correct threading:
(exec, chain_id, orderbook_address, block_number). No stale 2-argument calls remain.crates/common/src/raindex_client/local_db/query/fetch_orders.rs (1)
40-47: All call sites migrated correctly to 4-arg formVerification confirms all invocations of
fetch_ordersfromcrates/common/src/raindex_client/local_db/query/fetch_orders.rsacross the workspace use the new signature with all four parameters (&executor/exec,chain_id,orderbook_address/orderbook_cfg.address, andfetch_args). No old 2-arg calls remain. Changes are complete and correct.crates/common/src/raindex_client/local_db/query/fetch_last_synced_block.rs (1)
12-19: LGTM: threaded scope is correct.The new (chain_id, orderbook_address) wiring and test assertion against the exact SQL look consistent.
Also applies to: 54-64
crates/cli/src/commands/local_db/sync/token.rs (1)
42-52: LGTM: address/key handling and tests match the new schema.Using row.token_address for keys, scoping by (chain_id, orderbook_address), and test insert covering both columns look correct.
If any callers still expect the old fetch_existing_tokens signature, I can help scan/update them.
Also applies to: 118-121, 140-151
crates/common/src/local_db/mod.rs (2)
264-273: API threading looks gooddecoded_events_to_statements now accepts (chain_id, orderbook_address) and forwards cleanly. LGTM.
Please ensure all callsites migrated to the new signature across the workspace.
275-283: Consistent propagation for raw eventsraw_events_to_statements mirrors the decoded path with the new params. LGTM.
crates/common/src/raindex_client/local_db/query/fetch_erc20_tokens_by_addresses.rs (1)
5-16: Threading of orderbook Address looks correctbuild_fetch_stmt(chain_id, orderbook_address, addresses) and early-short-circuit are sound. LGTM.
Please confirm the SQL param order is [chain_id, orderbook, ...addresses] to match other callsites/tests.
crates/cli/src/commands/local_db/sync/runner/window.rs (1)
41-43: Correctly scopes last_synced by (chain_id, orderbook_address)Switch to fetch_last_synced(db_path, params.chain_id, params.orderbook_address) is correct. LGTM.
crates/common/src/local_db/query/fetch_last_synced_block/mod.rs (1)
25-33: Statement builder with (chain_id, orderbook_address) looks goodUsing U64 for chain_id and hex-prefixed address string is consistent. LGTM.
Project-wide, some callsites use i64 for chain_id params. Consider standardizing on u64 to avoid subtle mismatches.
crates/cli/src/commands/local_db/sync/storage.rs (3)
132-150: Good: token fetch correctly threads orderbook paramDelegation to build_fetch_stmt(chain_id, orderbook_address, addresses) with early None handling looks correct. LGTM.
207-221: Test OK: default last_synced = 0 with scoped queryScope by (chain_id, orderbook) and empty DB returns 0. LGTM.
262-267: Normalization behavior verifiedLowercasing of fetched store addresses is asserted; matches query usage. LGTM.
crates/cli/src/commands/local_db/sync/runner/apply.rs (1)
106-112: Good propagation of chain_id and orderbook_address into SQL builders.Threading of scope parameters looks correct and consistent.
crates/common/src/local_db/pipeline/adapters/apply.rs (2)
44-52: Decimals map precedence is correct (upserts override existing).Building HashMap by chaining existing then upserts ensures “last wins”. Good.
78-86: Watermark statement emitted once at the end.Ordering (raw → tokens → decoded → watermark) and single emission verified by tests. LGTM.
crates/common/src/local_db/query/create_tables/query.sql (6)
94-115: Composite foreign key declaration is verbose but correct.The foreign key constraints spanning multiple lines (lines 104-114) are syntactically correct and properly reference the expanded composite key structure. While verbose, this is the appropriate way to define multi-column foreign keys.
134-159: Composite key width for take_order_contexts is appropriate.The 5-column primary key
(chain_id, orderbook_address, transaction_hash, log_index, context_index)correctly scopes context values to a specific transaction context within a chain/orderbook combination. The matching foreign key structure is sound.
161-190: Composite key width for context_values is appropriate.The 6-column primary key
(chain_id, orderbook_address, transaction_hash, log_index, context_index, value_index)is necessary to uniquely identify individual values within a context. The foreign key totake_order_contextscorrectly references the parent entity's composite key.
244-270: Index consistency improved with chain_id and orderbook_address as leading columns.All indices now include
(chain_id, orderbook_address, ...)as leading columns, which aligns with the new primary key structure and enables efficient scoped queries. Index naming follows a consistent pattern with table name prefixes.
272-282: No issues found — erc20_tokens column rename is properly reflected in all Rust code.All code that inserts into or queries the
erc20_tokenstable has been updated to use the new column names (orderbook_addressandtoken_address). INSERT statements, ON CONFLICT clauses, and test expectations are all aligned with the new schema. No orphaned references to the oldaddresscolumn remain in the database code.
4-27: Migration strategy exists—review comment based on incomplete analysis.The codebase implements explicit schema version validation that detects and handles incompatibilities. When a schema version mismatch is detected, the database is reset with
reset_db()which clears all tables and recreates them. This occurs in the bootstrap pipeline logic, which means existing databases with incompatible versions are not silently left in an inconsistent state—they're explicitly detected and reset.The
CREATE TABLE IF NOT EXISTSstatements serve as the schema definition for fresh databases. When an existing database is opened, the application validates the stored schema version against the expected version, and if they mismatch, triggers a full reset rather than attempting incremental migrations.This is not a bug; it's a deliberate reset-based migration strategy that guarantees schema consistency at the cost of losing local data when versions diverge.
| println!("Fetching metadata for {} new token(s)", missing_addrs.len()); | ||
| let fetched = token_fetcher.fetch(rpc_urls, missing_addrs).await?; | ||
|
|
||
| let tokens_prefix_sql = generate_erc20_token_statements(chain_id, &fetched); | ||
| let tokens_prefix_sql = generate_erc20_token_statements(chain_id, orderbook_address, &fetched); | ||
| for (addr, info) in fetched.into_iter() { | ||
| decimals_by_addr.insert(addr, info.decimals); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use structured logging instead of println!.
Switch to tracing/log to avoid stdout noise and potential clippy flags in strict CI.
- println!("Fetching metadata for {} new token(s)", missing_addrs.len());
+ tracing::info!("Fetching metadata for {} new token(s)", missing_addrs.len());As per coding guidelines (linting with rainix-rs-static).
📝 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.
| println!("Fetching metadata for {} new token(s)", missing_addrs.len()); | |
| let fetched = token_fetcher.fetch(rpc_urls, missing_addrs).await?; | |
| let tokens_prefix_sql = generate_erc20_token_statements(chain_id, &fetched); | |
| let tokens_prefix_sql = generate_erc20_token_statements(chain_id, orderbook_address, &fetched); | |
| for (addr, info) in fetched.into_iter() { | |
| decimals_by_addr.insert(addr, info.decimals); | |
| } | |
| tracing::info!("Fetching metadata for {} new token(s)", missing_addrs.len()); | |
| let fetched = token_fetcher.fetch(rpc_urls, missing_addrs).await?; | |
| let tokens_prefix_sql = generate_erc20_token_statements(chain_id, orderbook_address, &fetched); | |
| for (addr, info) in fetched.into_iter() { | |
| decimals_by_addr.insert(addr, info.decimals); | |
| } |
🤖 Prompt for AI Agents
In crates/cli/src/commands/local_db/sync/token.rs around lines 70 to 76, replace
the println! call with structured tracing logging (e.g., tracing::info! or
tracing::debug!) including the number of tokens as a field (count =
missing_addrs.len()) to avoid stdout noise; ensure the tracing crate is
imported/used (add use tracing::info or call tracing::info! directly) and keep
the rest of the logic unchanged so subsequent operations still execute as
before.
There was a problem hiding this comment.
this is fine. we are going to be replacing this in downstream PRs
There was a problem hiding this comment.
@findolor, understood! I've noted that this will be addressed in downstream PRs.
For context, I do have a learning that simple println! statements are preferred for the local DB sync CLI command (since it's CI-only usage), which aligns with keeping this as-is for now.
🧠 Learnings used
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.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2159
File: crates/cli/src/commands/local_db/sync/runner/mod.rs:52-113
Timestamp: 2025-10-06T14:41:41.909Z
Learning: The local DB sync CLI command (crates/cli/src/commands/local_db/sync/) is designed for CI-only usage, and simple println! statements are preferred over structured logging for status messages.
| addresses | ||
| .iter() | ||
| .cloned() | ||
| .map(|a| SqlValue::Text(hex::encode_prefixed(a))), | ||
| )?; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: dedup addresses to shrink IN lists
If callers can pass duplicates, consider de-duplicating before bind_list_clause to reduce SQL size and param count.
Example:
use std::collections::BTreeSet;
let uniq: BTreeSet<_> = addresses.iter().cloned().collect();🤖 Prompt for AI Agents
In crates/common/src/local_db/query/fetch_erc20_tokens_by_addresses/mod.rs
around lines 41 to 45, the code maps addresses directly into the SQL bind list
which can include duplicates and inflate the IN clause; deduplicate the
addresses before calling bind_list_clause (for example collect into a BTreeSet
or HashSet to remove duplicates, then iterate that set and map to
SqlValue::Text(hex::encode_prefixed(a))) so the SQL parameter list is shorter
and param count is reduced.
There was a problem hiding this comment.
callers do not pass duplicate values
There was a problem hiding this comment.
@findolor, understood! Since callers already ensure unique addresses, the deduplication step is unnecessary. Thanks for the clarification.
| let local_db_orders = | ||
| fetch_orders(executor, chain_id, orderbook_address, fetch_args).await?; | ||
| let raindex_client = Rc::new(self.clone()); | ||
|
|
||
| for local_db_order in local_db_orders { | ||
| let local_orderbook_address = Address::from_str(&local_db_order.orderbook_address)?; | ||
| if local_orderbook_address != orderbook_address { | ||
| continue; | ||
| } | ||
|
|
||
| let input_vaults = | ||
| fetch_vaults_for_io_string(executor, chain_id, &local_db_order.inputs).await?; | ||
| let output_vaults = | ||
| fetch_vaults_for_io_string(executor, chain_id, &local_db_order.outputs).await?; | ||
| if let Some(local_db_order) = local_db_orders.into_iter().next() { | ||
| let input_vaults = fetch_vaults_for_io_string( | ||
| executor, | ||
| chain_id, | ||
| orderbook_address, | ||
| &local_db_order.inputs, | ||
| ) | ||
| .await?; | ||
| let output_vaults = fetch_vaults_for_io_string( | ||
| executor, | ||
| chain_id, | ||
| orderbook_address, | ||
| &local_db_order.outputs, | ||
| ) | ||
| .await?; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Apply the same vault short‑circuit in the single‑order path.
Saves two queries when inputs/outputs are absent.
- let local_db_orders =
- fetch_orders(executor, chain_id, orderbook_address, fetch_args).await?;
+ let local_db_orders =
+ fetch_orders(executor, chain_id, orderbook_address, fetch_args).await?;
@@
- if let Some(local_db_order) = local_db_orders.into_iter().next() {
- let input_vaults = fetch_vaults_for_io_string(
- executor,
- chain_id,
- orderbook_address,
- &local_db_order.inputs,
- )
- .await?;
- let output_vaults = fetch_vaults_for_io_string(
- executor,
- chain_id,
- orderbook_address,
- &local_db_order.outputs,
- )
- .await?;
+ if let Some(local_db_order) = local_db_orders.into_iter().next() {
+ let input_vaults = if local_db_order.inputs.is_some() {
+ fetch_vaults_for_io_string(executor, chain_id, orderbook_address, &local_db_order.inputs).await?
+ } else { Vec::new() };
+ let output_vaults = if local_db_order.outputs.is_some() {
+ fetch_vaults_for_io_string(executor, chain_id, orderbook_address, &local_db_order.outputs).await?
+ } else { Vec::new() };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/orders.rs around lines 750 to 769, the
single-order path always calls fetch_vaults_for_io_string for inputs and outputs
even when they are empty; mirror the batch-path short-circuit by checking
local_db_order.inputs and local_db_order.outputs and only await
fetch_vaults_for_io_string when the corresponding IO string is non-empty,
otherwise set input_vaults/output_vaults to the empty collection value expected
by the surrounding code, preserving types and error handling.
| let callback = Closure::wrap(Box::new(move |sql: String| -> JsValue { | ||
| if sql.contains("GROUP_CONCAT(CASE WHEN ios.io_type = 'input'") { | ||
| if sql.contains("FROM order_events") | ||
| && sql.contains("GROUP_CONCAT(") | ||
| && sql.contains("ios.io_type = 'input'") | ||
| { | ||
| return js_sys::JSON::parse(&orders_payload).unwrap(); | ||
| } | ||
|
|
||
| if sql.contains("COUNT(*) AS trade_count") { | ||
| if sql.contains("SELECT COUNT(*) AS trade_count") { | ||
| return js_sys::JSON::parse(&trade_count_payload).unwrap(); | ||
| } | ||
|
|
||
| if sql.contains("FROM take_orders") || sql.contains("FROM clear_v3_events") { | ||
| if sql.contains("AS trade_kind") { | ||
| return js_sys::JSON::parse(&trades_payload).unwrap(); | ||
| } | ||
|
|
||
| if sql.contains("FLOAT_SUM(vd.delta)") { | ||
| if sql.contains("FLOAT_SUM(vd") { | ||
| for (needle, payload) in &vault_payloads { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
SQL dispatch heuristics in test callback are brittle; centralize matchers.
Multiple sql.contains(...) checks for identifying statements can drift as builders evolve. Consider extracting these predicates into a small helper with well-named functions (is_orders_query, is_trade_count_query, etc.) to keep tests stable and readable.
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/trades.rs around lines 594 to 611, the
test's Closure uses several ad-hoc sql.contains(...) checks which are brittle;
extract these predicates into small, well-named helper functions (e.g.,
is_orders_query, is_trade_count_query, is_trades_query, is_vault_sum_query)
placed near the test or in a test-only module, and replace the inline
contains(...) checks with calls to those helpers; ensure each helper
encapsulates the exact matching logic (and any future tweaks) and add short
comments/tests for each helper to document intent.
There was a problem hiding this comment.
@findolor, understood! I'll skip this refactoring for now. Feel free to reach out if you'd like to revisit it in the future.
🧠 Learnings used
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.
| let fetch_args_template = filters | ||
| .clone() | ||
| .map(FetchVaultsArgs::from_filters) | ||
| .unwrap_or_default(); | ||
|
|
||
| let local_vaults = fetch_vaults(&executor, chain_id, fetch_args).await?; | ||
| let mut vaults = Vec::new(); | ||
| let orderbooks = self.get_orderbooks_by_chain_id(chain_id)?; | ||
| let raindex_client = Rc::new(self.clone()); | ||
|
|
||
| for local_vault in local_vaults { | ||
| let vault = RaindexVault::try_from_local_db( | ||
| Rc::clone(&raindex_client), | ||
| for orderbook_cfg in orderbooks { | ||
| let local_vaults = fetch_vaults( | ||
| &executor, | ||
| chain_id, | ||
| local_vault, | ||
| None, | ||
| )?; | ||
| orderbook_cfg.address, | ||
| fetch_args_template.clone(), | ||
| ) | ||
| .await?; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Per‑orderbook local‑db fetch: good direction; consider batching for latency
Looping over orderbooks per chain is correct. If the executor allows concurrent use, you can batch these calls with try_join_all to reduce wall time; otherwise, keep sequential to respect single-connection JS callbacks.
No behavior change required now.
Also applies to: 1205-1206, 1208-1226
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/vaults.rs around lines 1199-1216 (also
affecting 1205-1206, 1208-1226), the per-orderbook fetch loop currently awaits
each fetch_vaults call sequentially; if the executor supports concurrent usage,
change the implementation to collect the fetch_vaults futures for all orderbooks
(cloning any needed variables such as fetch_args_template, raindex_client/refs)
and use futures::future::try_join_all to run them in parallel, then
flatten/concat the results into vaults; if the executor does not allow
concurrent calls, leave it as-is—ensure any mutable/shared state is properly
cloned or wrapped (Rc/Arc) before creating the futures to avoid borrow issues.
There was a problem hiding this comment.
@findolor, understood! I'll note that the sequential approach is preferred here. No changes needed to this implementation.
🧠 Learnings used
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.
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.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1947
File: crates/common/src/raindex_client/orders.rs:98-125
Timestamp: 2025-06-24T08:46:03.368Z
Learning: In the vault merging logic in crates/common/src/raindex_client/orders.rs, optimization isn't necessary because the maximum list items are usually around 5 items. For such small datasets, the simple three-loop approach is preferred over HashMap-based optimization due to clarity and minimal performance impact.
| }; | ||
|
|
||
| let local_vaults = fetch_vaults(executor, chain_id, fetch_args).await?; | ||
| let local_vaults = fetch_vaults(executor, chain_id, orderbook_address, fetch_args).await?; |
There was a problem hiding this comment.
Avoid full‑scan fetch_vaults for get_vault_local_db; add targeted query
Fetching all vaults for an orderbook then scanning by id does not scale.
Prefer a dedicated SQL that filters by (chain_id, orderbook_address, vault_id) (and derives owner/token as needed) to return at most one row. You already have fetch_vault/query.sql; if it requires token too, add a variant that resolves token from order_ios/vault_deltas by vault_id.
Happy to draft the builder and SQL if you want.
Also applies to: 1393-1407
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/vaults.rs around lines 1388 and also
covering 1393-1407, the code currently calls fetch_vaults to load all vaults
then scans for a specific vault_id which does a full table scan and won't scale;
replace that full-scan with a dedicated SQL query (or query builder) that
selects at most one row by filtering on (chain_id, orderbook_address, vault_id)
and returning the needed columns (derive owner/token from order_ios.vault_deltas
if fetch_vault/query.sql requires token); modify get_vault_local_db to call this
new targeted query, handle the Option/Result correctly (returning NotFound or
None when no row), and remove the post-fetch in-memory scan, ensuring
parameterized inputs and existing error handling are preserved.
There was a problem hiding this comment.
it's fine for now as this system is local and really fast. also this is not part of this PR
There was a problem hiding this comment.
@findolor, understood! You're right that this optimization is out of scope for the current PR. I've noted your feedback and won't pursue this further.
🧠 Learnings used
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.
| const { | ||
| raindexClient, | ||
| localDb, | ||
| chainId = 42161, | ||
| orderbookAddress = '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB', | ||
| intervalMs = 10_000 | ||
| } = options; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid hard‑coding the default orderbook address in the webapp.
Using a network-specific constant as default risks incorrect scoping. Prefer requiring orderbookAddress from caller or deriving it from RaindexClient config/environment.
🤖 Prompt for AI Agents
In packages/webapp/src/lib/services/startLocalDbSync.ts around lines 23 to 29,
the default orderbookAddress is hard-coded to a specific network value; remove
the network-specific hard-coded default by making orderbookAddress required from
the caller or deriving it from raindexClient/config/env before use. Update the
function signature/options so orderbookAddress is not defaulted here (validate
and throw or fallback to raindexClient.getConfig()/process.env lookup), and
ensure callers supply the address or the code retrieves it from raindexClient
configuration.
There was a problem hiding this comment.
there is a note about this. we are going to remove this logic in the future so it serves as a placeholder for now
There was a problem hiding this comment.
@findolor, understood! I see the NOTE comment in the code already documents this as a temporary solution. Thank you for clarifying that this will be addressed when the new sync logic is introduced.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2278
File: packages/webapp/src/lib/services/startLocalDbSync.ts:23-29
Timestamp: 2025-10-29T13:29:29.547Z
Learning: In packages/webapp/src/lib/services/startLocalDbSync.ts, the hard-coded default orderbook address '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' is a temporary placeholder that will be removed when new sync logic is introduced in the Raindex client, as documented in the NOTE comment.
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#2202
File: crates/common/src/raindex_client/local_db/sync.rs:33-34
Timestamp: 2025-10-14T07:51:55.148Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the hard-coded `DEFAULT_SYNC_CHAIN_ID` constant (set to `SUPPORTED_LOCAL_DB_CHAINS[0]`) will be replaced with proper chain ID handling in downstream PRs as part of the multi-network/orderbook implementation.
Learnt from: findolor
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.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2145
File: crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql:6-7
Timestamp: 2025-10-06T11:28:30.692Z
Learning: In `crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql`, the orderbook_address is currently hardcoded to '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' because the system only supports a single orderbook at the moment. Multiorderbook logic is not yet implemented and will be added in the future.
| exec | ||
| .query_text(&SqlStatement::new("INSERT INTO erc20_tokens (chain_id, address, name, symbol, decimals) VALUES (1, '0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', 'A', 'A', 18);")) | ||
| exec.query_text(&SqlStatement::new( | ||
| "INSERT INTO erc20_tokens (chain_id, orderbook_address, token_address, name, symbol, decimals) VALUES (1, '0x0101010101010101010101010101010101010101', '0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', 'A', 'A', 18);", |
There was a problem hiding this comment.
nitpick: consider breaking down into multiple lines
| #[cfg_attr(target_family = "wasm", derive(Tsify))] | ||
| pub struct SyncStatusResponse { | ||
| pub id: u64, | ||
| #[serde(alias = "chainId")] |
There was a problem hiding this comment.
nitpick: why not
#[serde(rename_all = "camelCase")]| const TOKENS_CLAUSE: &str = "/*TOKENS_CLAUSE*/"; | ||
| const TOKENS_CLAUSE_BODY: &str = | ||
| "AND EXISTS ( \n SELECT 1 FROM order_ios io2 \n WHERE io2.transaction_hash = la.transaction_hash \n AND io2.log_index = la.log_index \n AND lower(io2.token) IN ({list}) )"; | ||
| "AND EXISTS ( \n SELECT 1 FROM order_ios io2 \n WHERE io2.chain_id = ?1 \n AND lower(io2.orderbook_address) = lower(?2) \n AND io2.transaction_hash = la.transaction_hash \n AND io2.log_index = la.log_index \n AND lower(io2.token) IN ({list}) )"; |
There was a problem hiding this comment.
could definitely be a cleaner diff if formatted differently
There was a problem hiding this comment.
Actionable comments posted: 4
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/insert.rs (1)
709-836: Critical: Missing SQL placeholder for last parameter.The INSERT statement declares 21 columns (lines 767-788) but the VALUES clause only contains placeholders ?1 through ?20 (lines 789-809). The parameter vector correctly provides 21 values (lines 812-834), but the last value
bob_output_vault_id_hexat line 833 will never be bound because there's no ?21 placeholder.Apply this diff to add the missing placeholder:
?18, - ?19, - ?20 + ?19, + ?20, + ?21 );
♻️ Duplicate comments (1)
crates/common/src/local_db/query/fetch_vaults/mod.rs (1)
37-45: Normalize address encoding consistently across all builders in a follow-up refactoring effort.The review's observation is correct: SQL templates use
lower(orderbook_address) = lower(?2)throughout all query builders (verified across fetch_vaults, fetch_orders, fetch_vault_balance_changes, fetch_order_trades, and others). Normalizing addresses to lowercase in Rust before binding would eliminate repeated function calls and allow SQLite to utilize indexes on address columns (since indexes are not used when LOWER() is applied).However, this optimization requires coordinated changes across all 12 builder files and is outside the current scope. Consider addressing this as a separate refactoring follow-up to maintain scope discipline, per your preference for targeted changes.
| fn clear_v3_sql_generation() { | ||
| let event = sample_clear_event(); | ||
| let context = event_context(&event).unwrap(); | ||
| let context = event_context(&OrderbookIdentifier::new(1, Address::ZERO), &event).unwrap(); | ||
| let DecodedEvent::ClearV3(decoded) = &event.decoded_data else { | ||
| unreachable!() | ||
| }; | ||
| let statement = generate_clear_v3_statement(&context, decoded).unwrap(); | ||
| assert!(statement.sql().contains("INSERT INTO clear_v3_events")); | ||
| assert!(statement.sql().contains("?19")); | ||
| assert!(statement.sql().contains("?20")); | ||
| let params = statement.params(); | ||
| assert_eq!(params.len(), 19); | ||
| assert!(matches!(params[0], SqlValue::U64(v) if v == context.block_number)); | ||
| assert!(matches!(params[3], SqlValue::U64(v) if v == context.log_index)); | ||
| assert_eq!(params.len(), 21); | ||
| assert!(matches!( | ||
| params[0], | ||
| SqlValue::I64(v) if v == context.ob_id.chain_id as i64 | ||
| )); | ||
| assert!(matches!( | ||
| params[1], | ||
| SqlValue::Text(ref v) if v == &context.ob_id.orderbook_address.to_string() | ||
| )); | ||
| assert!(matches!(params[2], SqlValue::U64(v) if v == context.block_number)); | ||
| assert!(matches!(params[3], SqlValue::U64(v) if v == context.block_timestamp)); | ||
| assert!(matches!(params[5], SqlValue::U64(v) if v == context.log_index)); | ||
| let expected_alice_input_vault = hex::encode_prefixed(decoded.alice.validInputs[0].vaultId); | ||
| assert!(matches!(params[10], SqlValue::Text(ref v) if v == &expected_alice_input_vault)); | ||
| assert!(matches!(params[12], SqlValue::Text(ref v) if v == &expected_alice_input_vault)); | ||
| let expected_bob_input_vault = hex::encode_prefixed(decoded.bob.validInputs[0].vaultId); | ||
| assert!(matches!(params[17], SqlValue::Text(ref v) if v == &expected_bob_input_vault)); | ||
| assert!(matches!(params[19], SqlValue::Text(ref v) if v == &expected_bob_input_vault)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test should verify all 21 placeholders are present.
The test checks for "?20" at line 1518, but since the function should generate 21 parameters, it should verify that "?21" exists in the SQL to catch placeholder/parameter count mismatches.
Apply this diff:
- assert!(statement.sql().contains("?20"));
+ assert!(statement.sql().contains("?21"));📝 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.
| fn clear_v3_sql_generation() { | |
| let event = sample_clear_event(); | |
| let context = event_context(&event).unwrap(); | |
| let context = event_context(&OrderbookIdentifier::new(1, Address::ZERO), &event).unwrap(); | |
| let DecodedEvent::ClearV3(decoded) = &event.decoded_data else { | |
| unreachable!() | |
| }; | |
| let statement = generate_clear_v3_statement(&context, decoded).unwrap(); | |
| assert!(statement.sql().contains("INSERT INTO clear_v3_events")); | |
| assert!(statement.sql().contains("?19")); | |
| assert!(statement.sql().contains("?20")); | |
| let params = statement.params(); | |
| assert_eq!(params.len(), 19); | |
| assert!(matches!(params[0], SqlValue::U64(v) if v == context.block_number)); | |
| assert!(matches!(params[3], SqlValue::U64(v) if v == context.log_index)); | |
| assert_eq!(params.len(), 21); | |
| assert!(matches!( | |
| params[0], | |
| SqlValue::I64(v) if v == context.ob_id.chain_id as i64 | |
| )); | |
| assert!(matches!( | |
| params[1], | |
| SqlValue::Text(ref v) if v == &context.ob_id.orderbook_address.to_string() | |
| )); | |
| assert!(matches!(params[2], SqlValue::U64(v) if v == context.block_number)); | |
| assert!(matches!(params[3], SqlValue::U64(v) if v == context.block_timestamp)); | |
| assert!(matches!(params[5], SqlValue::U64(v) if v == context.log_index)); | |
| let expected_alice_input_vault = hex::encode_prefixed(decoded.alice.validInputs[0].vaultId); | |
| assert!(matches!(params[10], SqlValue::Text(ref v) if v == &expected_alice_input_vault)); | |
| assert!(matches!(params[12], SqlValue::Text(ref v) if v == &expected_alice_input_vault)); | |
| let expected_bob_input_vault = hex::encode_prefixed(decoded.bob.validInputs[0].vaultId); | |
| assert!(matches!(params[17], SqlValue::Text(ref v) if v == &expected_bob_input_vault)); | |
| assert!(matches!(params[19], SqlValue::Text(ref v) if v == &expected_bob_input_vault)); | |
| } | |
| fn clear_v3_sql_generation() { | |
| let event = sample_clear_event(); | |
| let context = event_context(&OrderbookIdentifier::new(1, Address::ZERO), &event).unwrap(); | |
| let DecodedEvent::ClearV3(decoded) = &event.decoded_data else { | |
| unreachable!() | |
| }; | |
| let statement = generate_clear_v3_statement(&context, decoded).unwrap(); | |
| assert!(statement.sql().contains("INSERT INTO clear_v3_events")); | |
| assert!(statement.sql().contains("?21")); | |
| let params = statement.params(); | |
| assert_eq!(params.len(), 21); | |
| assert!(matches!( | |
| params[0], | |
| SqlValue::I64(v) if v == context.ob_id.chain_id as i64 | |
| )); | |
| assert!(matches!( | |
| params[1], | |
| SqlValue::Text(ref v) if v == &context.ob_id.orderbook_address.to_string() | |
| )); | |
| assert!(matches!(params[2], SqlValue::U64(v) if v == context.block_number)); | |
| assert!(matches!(params[3], SqlValue::U64(v) if v == context.block_timestamp)); | |
| assert!(matches!(params[5], SqlValue::U64(v) if v == context.log_index)); | |
| let expected_alice_input_vault = hex::encode_prefixed(decoded.alice.validInputs[0].vaultId); | |
| assert!(matches!(params[12], SqlValue::Text(ref v) if v == &expected_alice_input_vault)); | |
| let expected_bob_input_vault = hex::encode_prefixed(decoded.bob.validInputs[0].vaultId); | |
| assert!(matches!(params[19], SqlValue::Text(ref v) if v == &expected_bob_input_vault)); | |
| } |
🤖 Prompt for AI Agents
In crates/common/src/local_db/insert.rs around lines 1510 to 1536, the test
currently asserts the generated SQL contains "?20" but the statement should
produce 21 placeholders; update the test to assert the SQL contains "?21" (i.e.,
replace the "?20" check with a check for "?21") so the placeholder count
mismatch will be caught.
| #[derive(Debug, Clone)] | ||
| pub struct OrderbookIdentifier { | ||
| pub chain_id: u32, | ||
| pub orderbook_address: Address, | ||
| } | ||
| impl OrderbookIdentifier { | ||
| pub fn new(chain_id: u32, orderbook_address: Address) -> Self { | ||
| Self { | ||
| chain_id, | ||
| orderbook_address, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding PartialEq and Eq derives.
The OrderbookIdentifier struct is used as a key/identifier throughout the codebase. Adding PartialEq and Eq derives would enable direct equality comparisons and make it usable in collections like HashSet or as HashMap keys if needed in the future.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq)]
pub struct OrderbookIdentifier {🤖 Prompt for AI Agents
In crates/common/src/local_db/mod.rs around lines 229 to 241, the
OrderbookIdentifier struct only derives Debug and Clone which prevents direct
equality comparisons and use in equality-based collections; update the derive
attribute to include PartialEq and Eq (e.g. #[derive(Debug, Clone, PartialEq,
Eq)]) so the struct supports equality semantics and can be used in sets or as
map keys; ensure the contained Address type already implements PartialEq/Eq or
adjust accordingly.
| let ob_id = &OrderbookIdentifier::new(chain_id, orderbook_cfg.address); | ||
| let local_db_orders = fetch_orders(&executor, ob_id, fetch_args.clone()).await?; | ||
|
|
||
| let order = RaindexOrder::try_from_local_db( | ||
| raindex_client.clone(), | ||
| chain_id, | ||
| local_db_order.clone(), | ||
| input_vaults, | ||
| output_vaults, | ||
| )?; | ||
| orders.push(order); | ||
| for local_db_order in &local_db_orders { | ||
| let input_vaults = | ||
| fetch_vaults_for_io_string(&executor, ob_id, &local_db_order.inputs).await?; | ||
| let output_vaults = | ||
| fetch_vaults_for_io_string(&executor, ob_id, &local_db_order.outputs).await?; | ||
|
|
||
| let order = RaindexOrder::try_from_local_db( | ||
| raindex_client.clone(), | ||
| chain_id, | ||
| local_db_order.clone(), | ||
| input_vaults, | ||
| output_vaults, | ||
| )?; | ||
| orders.push(order); | ||
| } |
There was a problem hiding this comment.
Store the OrderbookIdentifier instead of borrowing a temporary
let ob_id = &OrderbookIdentifier::new(...) takes a reference to a temporary that is dropped immediately, so the borrow does not live long enough. The compiler emits E0597: temporary value dropped while borrowed, preventing this module (and the PR) from compiling. Hold the identifier in a local and pass &ob_id to the async helpers.
- let ob_id = &OrderbookIdentifier::new(chain_id, orderbook_cfg.address);
- let local_db_orders = fetch_orders(&executor, ob_id, fetch_args.clone()).await?;
+ let ob_id = OrderbookIdentifier::new(chain_id, orderbook_cfg.address);
+ let local_db_orders =
+ fetch_orders(&executor, &ob_id, fetch_args.clone()).await?;
for local_db_order in &local_db_orders {
- let input_vaults =
- fetch_vaults_for_io_string(&executor, ob_id, &local_db_order.inputs).await?;
- let output_vaults =
- fetch_vaults_for_io_string(&executor, ob_id, &local_db_order.outputs).await?;
+ let input_vaults =
+ fetch_vaults_for_io_string(&executor, &ob_id, &local_db_order.inputs).await?;
+ let output_vaults =
+ fetch_vaults_for_io_string(&executor, &ob_id, &local_db_order.outputs).await?;🤖 Prompt for AI Agents
crates/common/src/raindex_client/orders.rs around lines 613 to 630: the code
currently borrows a temporary with let ob_id = &OrderbookIdentifier::new(...),
causing E0597 because the temporary is dropped across await points; change this
to create a local owned identifier (let ob_id =
OrderbookIdentifier::new(chain_id, orderbook_cfg.address);) and pass references
(&ob_id) into fetch_orders, fetch_vaults_for_io_string, and any other callers so
the referenced value lives long enough across the async/await boundaries.
| const statusResult = await getSyncStatus( | ||
| queryFn, | ||
| 42161, | ||
| '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! API signature update is correct.
The addition of chain ID and orderbook address parameters correctly aligns with the updated getSyncStatus API signature. The hard-coded values (chain ID 42161 and orderbook address) are temporary placeholders consistent with the broader PR changes.
Optional: Consider extracting the hard-coded chain ID and orderbook address to constants at the top of the file, since they're used in multiple places (lines 136-137, 175, 200). This would improve maintainability and reduce the risk of inconsistent updates.
🤖 Prompt for AI Agents
In packages/webapp/src/routes/db-playground/+page.svelte around lines 134 to
138, there are hard-coded values for chain ID (42161) and the orderbook address
that are repeated elsewhere (lines ~175 and ~200); extract these into named
constants at the top of the file (e.g., CHAIN_ID and ORDERBOOK_ADDRESS), replace
each hard-coded occurrence with those constants, and ensure any imports or
linting rules are updated if needed so the constants are used consistently
across the file.
|
@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 issues:
(chain_id, orderbook_address)and Composite Keys #2250(chain_id, orderbook_address)Through Decode → SQL #2251(chain_id, orderbook_address)#2252Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes
Database Schema Updates