Conversation
WalkthroughAdds Serde helper deserializer for booleans, introduces wasm-facing LocalDb query modules (fetch_orders, fetch_vault, fetch_vaults) with SQL changes and tests, moves some wasm-exported methods from RaindexClient to LocalDb, adds vault_deltas view and vault-deltas-driven queries, updates insert logic and sync flow to use chain_id, and updates webapp to use LocalDb. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as Webapp UI
participant RC as RaindexClient
participant OB as OrderbookYaml
participant LD as LocalDb
participant DB as SQLite (WASM)
rect #F0F8FF
note right of RC: Chain-id based sync
UI->>RC: sync_database(db_cb, status_cb, chain_id)
RC->>OB: get_orderbooks_by_chain_id(chain_id)
alt orderbooks found
RC->>LD: start sync using chosen orderbook.address (status_cb)
LD->>DB: query/insert events (use orderbook.address)
DB-->>LD: rows/results
LD-->>UI: send status updates via status_cb
else none found
RC-->>UI: return error (no orderbook for chain_id)
end
end
sequenceDiagram
autonumber
actor UI as Webapp UI
participant RC as RaindexClient
participant Q as LocalDbQuery
participant DB as SQLite (WASM)
rect #F5FFF8
note right of Q: Fetch orders and vaults
UI->>RC: get_orders_local_db(chain_id, db_cb)
RC->>Q: fetch_orders(db_cb, filter)
Q->>DB: execute orders QUERY -> JSON (LocalDbOrder[])
DB-->>Q: JSON rows
loop per LocalDbOrder
Q->>Q: parse IO strings into indexed pairs
Q->>DB: execute fetch_vault(query) for each IO entry
DB-->>Q: LocalDbVault[]
RC->>RC: try_from_local_db(order, inputs, outputs) -> RaindexOrder
end
RC-->>UI: Vec<RaindexOrder>
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
findolor
force-pushed
the
local-db-refactor-rpc-client
branch
from
8535d5e to
c4f1619
Compare
findolor
force-pushed
the
local-db-app-integration
branch
from
cec5b7b to
c5b9d1a
Compare
|
@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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/common/src/raindex_client/local_db/insert.rs (1)
572-582: Enhance tests to verify new and renamed columns.The existing tests don't verify:
- The
deposit_amountcolumn is present in the deposit SQL (line 580 only checks uint256)- The renamed
taker_input/taker_outputcolumns are used instead ofinput/outputApply this diff to enhance the tests:
#[test] fn deposit_sql_generation() { let event = sample_deposit_event(); let context = event_context(&event).unwrap(); let DecodedEvent::DepositV2(decoded) = &event.decoded_data else { unreachable!() }; let sql = generate_deposit_sql(&context, decoded).unwrap(); assert!(sql.contains("INSERT INTO deposits")); + assert!(sql.contains("deposit_amount")); + assert!(sql.contains("deposit_amount_uint256")); assert!(sql.contains("0x0000000000000000000000000000000000000000000000000000000000000fa0")); } #[test] fn take_order_sql_generation() { let event = sample_take_event(); let context = event_context(&event).unwrap(); let DecodedEvent::TakeOrderV3(decoded) = &event.decoded_data else { unreachable!() }; let sql = generate_take_order_sql(&context, decoded).unwrap(); assert!(sql.contains("INSERT INTO take_orders")); + assert!(sql.contains("taker_input")); + assert!(sql.contains("taker_output")); + // Ensure old column names are not used + assert!(!sql.contains("VALUES (") || !sql.contains(", '0x00000000000000000000000000000000000000000000000000000000000003e8', '0x00000000000000000000000000000000000000000000000000000000000007d0')") || sql.contains("taker_")); assert!(sql.contains("signer:0x0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a0a")); assert!( sql.contains("'0x000000000000000000000000000000000000000000000000000000000000002a'") ); }Also applies to: 584-597
crates/common/src/raindex_client/local_db/sync.rs (1)
101-144: Chain-level sync now drops all but the first orderbook
get_orderbooks_by_chain_idcan return multiple orderbooks for the same chain, but we immediately pickfirst()and sync only that address. Before this PR the caller supplied the exact contract address, so no data was lost; after the API switch tochain_id, every other orderbook on the chain is silently ignored. Any user selecting a different market on that network will now see stale/empty data in the local DB, breaking the new orders flow.We need to iterate every orderbook returned for the chain (or otherwise select the correct one from additional context) and feed each address through fetch/decode/SQL generation so all markets stay consistent.
♻️ Duplicate comments (4)
crates/common/src/raindex_client/local_db/query/fetch_vaults/query.sql (1)
5-5: Known issue: hard-coded orderbook address.This hard-coded address limitation has been acknowledged in previous reviews and will be addressed in future work when multiorderbook support is implemented.
Based on learnings.
crates/common/src/raindex_client/local_db/query/fetch_vault/query.sql (1)
5-5: Known issue: hard-coded orderbook address.This hard-coded address limitation has been acknowledged in previous reviews and will be addressed in future work when multiorderbook support is implemented.
Based on learnings.
crates/common/src/raindex_client/vaults.rs (1)
1252-1285: Hydrate vault token metadata before constructingRaindexVaultWe still build
RaindexVaultTokenwith placeholders (id = "0x01", stub name/symbol,decimals = 18). As soon as a vault token has non-18 decimals (USDC, USDT, etc.), every downstream path that relies onself.token.decimals—get_deposit_calldata,get_withdraw_calldata, allowance/formatting—will emit incorrect calldata and UI values. This is the same blocker we flagged earlier: until the real metadata is fetched (or at least decimals are loaded) the vault client is wrong for most assets.Please extend the local-DB query to return the real token id/name/symbol/decimals and populate them here; if the data isn’t available yet, return an error instead of shipping placeholder values.
crates/common/src/raindex_client/orders.rs (1)
761-795: Blocking: Local DB orders emit duplicate identifiers
try_from_local_dbstill hardcodesidandorder_bytesto"0x01", so every order returned bygetOrdersLocalDbcarries the same bogus identifiers. Downstream caches, comparisons, and conversions rely on these values being unique and accurate; with the current placeholder they will all collide or overwrite each other. Please plumb the real ID/order-bytes from the local DB record (or fail fast until those fields are available) before exposing this path to callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
crates/common/src/raindex_client/local_db/insert.rs(4 hunks)crates/common/src/raindex_client/local_db/mod.rs(2 hunks)crates/common/src/raindex_client/local_db/query/clear_tables/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/query/create_tables/query.sql(2 hunks)crates/common/src/raindex_client/local_db/query/fetch_last_synced_block/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_orders/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql(2 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault/query.sql(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault_balance_changes/query.sql(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault_balance_history/query.sql(0 hunks)crates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_vaults/query.sql(1 hunks)crates/common/src/raindex_client/local_db/query/mod.rs(1 hunks)crates/common/src/raindex_client/local_db/sync.rs(8 hunks)crates/common/src/raindex_client/local_db/views/vault_deltas.sql(1 hunks)crates/common/src/raindex_client/mod.rs(4 hunks)crates/common/src/raindex_client/orders.rs(3 hunks)crates/common/src/raindex_client/vaults.rs(2 hunks)crates/settings/src/yaml/orderbook.rs(2 hunks)packages/webapp/src/routes/db-playground/+page.svelte(7 hunks)
💤 Files with no reviewable changes (1)
- crates/common/src/raindex_client/local_db/query/fetch_vault_balance_history/query.sql
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2025-10-06T11:28:30.659Z
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.659Z
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/query.sqlcrates/common/src/raindex_client/local_db/query/fetch_vaults/query.sql
📚 Learning: 2025-10-06T11:13:29.938Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2123
File: crates/common/src/raindex_client/local_db/mod.rs:23-29
Timestamp: 2025-10-06T11:13:29.938Z
Learning: In `crates/common/src/raindex_client/local_db/mod.rs`, the `Default` implementation for `LocalDb` that creates an RPC client pointing to `http://localhost:4444` is acceptable because the RPC client must be explicitly configured before actual usage in production scenarios.
Applied to files:
crates/common/src/raindex_client/local_db/query/clear_tables/mod.rscrates/common/src/raindex_client/local_db/query/fetch_last_synced_block/mod.rs
📚 Learning: 2025-05-09T05:28:22.089Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1744
File: crates/js_api/src/subgraph/order.rs:109-114
Timestamp: 2025-05-09T05:28:22.089Z
Learning: In the rain.orderbook project, SubgraphError contains or converts from OrderbookSubgraphClientError, so using OrderbookSubgraphClientError in error creation followed by a conversion to SubgraphError (via ? operator) is acceptable.
Applied to files:
crates/common/src/raindex_client/mod.rs
📚 Learning: 2025-06-20T07:26:50.488Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1941
File: crates/settings/src/yaml/orderbook.rs:112-123
Timestamp: 2025-06-20T07:26:50.488Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to keep the get_network_by_chain_id implementation simple with O(n) lookup rather than adding caching complexity for performance optimization, even if the method might be called frequently.
Applied to files:
crates/common/src/raindex_client/mod.rscrates/settings/src/yaml/orderbook.rs
📚 Learning: 2025-06-18T19:24:40.518Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:185-199
Timestamp: 2025-06-18T19:24:40.518Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers not to refactor get_orderbook_by_network_key to handle multiple orderbooks per network key since their current architecture maintains a one-to-one mapping between orderbooks and networks. They would consider the refactoring if the system evolves to support multiple orderbooks per network in the future.
Applied to files:
crates/settings/src/yaml/orderbook.rs
📚 Learning: 2025-06-18T18:24:32.049Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/settings/src/yaml/orderbook.rs:180-199
Timestamp: 2025-06-18T18:24:32.049Z
Learning: In crates/settings/src/yaml/orderbook.rs, the user prefers to avoid refactoring duplicate search logic between get_orderbook_by_address and get_orderbook_by_network_key when there are only 2 functions, indicating they would consider it if more similar functions are added in the future.
Applied to files:
crates/settings/src/yaml/orderbook.rs
📚 Learning: 2025-10-06T10:23:57.444Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2111
File: packages/webapp/src/routes/db-playground/+page.svelte:149-153
Timestamp: 2025-10-06T10:23:57.444Z
Learning: The db-playground page at packages/webapp/src/routes/db-playground/+page.svelte is used for testing purposes, and production-level concerns (such as invalid RPC tokens) are acceptable in this context.
Applied to files:
packages/webapp/src/routes/db-playground/+page.svelte
📚 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/local_db/query/fetch_orders/mod.rs
📚 Learning: 2025-07-09T07:42:48.458Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1975
File: crates/js_api/src/config.rs:44-45
Timestamp: 2025-07-09T07:42:48.458Z
Learning: User findolor prefers to defer code improvements and refactoring suggestions to future PRs when they are not directly related to the current PR's scope, even when the suggestions are valid improvements.
Applied to files:
crates/common/src/raindex_client/orders.rs
📚 Learning: 2025-09-16T07:37:24.455Z
Learnt from: thedavidmeister
PR: rainlanguage/rain.orderbook#2149
File: src/concrete/arb/RouteProcessorOrderBookV5ArbOrderTaker.sol:37-38
Timestamp: 2025-09-16T07:37:24.455Z
Learning: In the rainlanguage/rain.orderbook codebase, caching token decimals is considered dangerous unless handled with TOFU (Trust On First Use) pattern and expectation checks on every use. The additional validation required eliminates any gas savings from caching, so calling IERC20Metadata(...).decimals() on each use is the preferred safe pattern, as explained by thedavidmeister.
Applied to files:
crates/common/src/raindex_client/vaults.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). (16)
- GitHub Check: git-clean
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-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-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Deploy-Docs-Preview
- GitHub Check: Deploy-Preview-Push
🔇 Additional comments (15)
crates/common/src/raindex_client/local_db/insert.rs (3)
7-7: LGTM: Error handling for float conversion.The new
Floatimport andFloatConversionerror variant provide appropriate support for handling deposit amount conversions. The error variant captures conversion failures with descriptive messages.Also applies to: 29-30
249-249: take_orders schema and queries updated correctly
Thetake_orderstable definition increate_tables/query.sqlincludestaker_inputandtaker_output, and no references to the oldinput/outputcolumns remain in the code.
155-155: Schema includes new deposit columns: Verifieddepositstable incrates/common/src/raindex_client/local_db/query/create_tables/query.sqldefinesdeposit_amount TEXT NOT NULLanddeposit_amount_uint256 TEXT NOT NULL.crates/common/src/raindex_client/local_db/query/clear_tables/mod.rs (1)
14-21: LGTM! Refactoring aligns with LocalDb centralization.The move of
clear_tablesfromRaindexClienttoLocalDbis consistent with the broader PR pattern of consolidating wasm-exposed LocalDb functionality underLocalDbimpls. The method signature, wasm attributes, and functionality remain unchanged.crates/common/src/raindex_client/local_db/views/vault_deltas.sql (1)
1-97: LGTM! Well-structured vault_deltas view.The view consolidates vault balance changes from multiple sources (deposits, withdrawals, takes, clears) into a unified stream. The
NOT EXISTSlogic in lines 24-31 and 48-55 correctly identifies the most recent order event before or at each take order, ensuring proper linkage between takes and their source order definitions.crates/common/src/raindex_client/local_db/query/mod.rs (1)
4-7: LGTM! Module additions extend local DB query surface.The new modules (
fetch_orders,fetch_vault,fetch_vaults) follow the existing pattern and align with the PR's goal of incrementally implementing local DB queries for the webapp.crates/common/src/raindex_client/local_db/query/fetch_last_synced_block/mod.rs (1)
26-39: LGTM! Consistent refactoring to LocalDb.The move of
get_sync_statusfromRaindexClienttoLocalDbfollows the same pattern asclear_tablesand aligns with the broader PR refactoring to centralize wasm-exposed LocalDb functionality.crates/common/src/raindex_client/local_db/query/fetch_vault_balance_changes/query.sql (1)
26-57: Owner derivation logic is well-structured.The COALESCE fallback correctly prioritizes the most recent order owner, then falls back to the latest depositor/withdrawer. This handles both active orders and deposit-only vaults appropriately.
crates/common/src/raindex_client/local_db/query/fetch_vault/query.sql (2)
6-40: LGTM! Vault state aggregation is well-structured.The query correctly aggregates:
- Input/output order hashes using
GROUP_CONCATon distinct values- Balance from
vault_deltaswith proper COALESCE fallback- Appropriate filtering by token and vault_id
42-75: Owner derivation follows established pattern.The COALESCE logic prioritizes the most recent order owner, with a sensible fallback to the latest deposit/withdrawal sender, consistent with the pattern used in
fetch_vault_balance_changes/query.sql.crates/common/src/raindex_client/mod.rs (3)
196-199: LGTM! Simple delegation to orderbook_yaml.The new
get_orderbooks_by_chain_idmethod follows the existing pattern of delegating to the underlyingorderbook_yamlconfiguration, maintaining consistency with other similar methods likeget_rpc_urls_for_chain.
291-292: Error variant addition is necessary.The
LocalDbQueryErrorvariant properly extends the error hierarchy to support local DB query operations introduced in this PR.
412-414: Clear error message for local DB queries.The error message provides appropriate context about local DB query failures, following the pattern of other error message handlers in
to_readable_msg.crates/settings/src/yaml/orderbook.rs (1)
284-304: Nice reuse of existing orderbook lookupsLeveraging
get_network_by_chain_idplus the existingget_orderbookspath keeps the implementation tight, and the key-sorted output plus NotFound handling cover the expected UX. The accompanying tests exercise both success and failure paths well.crates/common/src/raindex_client/local_db/mod.rs (1)
333-395: Solid serde helper for SQLite-style booleansThe visitor cleanly normalises
true/false,0/1, and their string forms while failing fast on anything else. This should make the local-DB JSON round-tripping far less brittle.
| taker_input TEXT NOT NULL, | ||
| taker_output TEXT NOT NULL, |
There was a problem hiding this comment.
Add a schema migration for renamed columns.
Renaming input/output to taker_input/taker_output (and adding the new deposit_amount column above) changes the table layout seen by existing local DBs. Without an ALTER TABLE ... RENAME COLUMN .../ADD COLUMN ... migration, any previously initialized database will still have the old columns, so the WASM queries introduced in this PR will crash with “no such column” errors. Please add an explicit migration (or keep backward-compatible column names) before landing this change.
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/local_db/query/create_tables/query.sql
around lines 71-72 the schema was changed to rename input/output to
taker_input/taker_output and add deposit_amount, which will break existing local
DBs; add a migration that runs ALTER TABLE <table_name> RENAME COLUMN input TO
taker_input; ALTER TABLE <table_name> RENAME COLUMN output TO taker_output; and
ALTER TABLE <table_name> ADD COLUMN deposit_amount <appropriate_type> [DEFAULT
... or NULL] (use the concrete table name and correct SQL type used elsewhere),
and ensure this migration is executed on startup or added as a numbered
migration file so previously initialized DBs get the renamed columns before the
new WASM queries run.
There was a problem hiding this comment.
local db is not live yet
There was a problem hiding this comment.
@findolor, thank you for the clarification! Since the local DB feature isn't live yet, there's no risk of breaking existing databases with these schema changes. The direct column renames are perfectly fine in this development phase.
(❁´◡`❁) ♪
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2145
File: crates/common/src/raindex_client/local_db/query/create_tables/query.sql:71-72
Timestamp: 2025-10-06T11:44:07.867Z
Learning: The local DB feature in the rain.orderbook codebase is not live yet (as of PR #2145), so schema migrations for existing databases are not required when modifying table structures in `crates/common/src/raindex_client/local_db/query/create_tables/query.sql`.
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.
| fa.block_number AS block_number, | ||
| GROUP_CONCAT(CASE WHEN ios.io_type = 'input' THEN ios.vault_id || ':' || ios.token END) AS inputs, | ||
| GROUP_CONCAT(CASE WHEN ios.io_type = 'output' THEN ios.vault_id || ':' || ios.token END) AS outputs, | ||
| '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' AS orderbook_address, |
| )] | ||
| chain_id: u32, | ||
| #[wasm_export(param_description = "JavaScript function to execute database queries")] | ||
| db_callback: js_sys::Function, |
There was a problem hiding this comment.
why does this need to be passed every time? doesn't it make sense that it's an immutable field on the client?
There was a problem hiding this comment.
oh, i see it's a standalone fn, but in that case why is it exposed to the sdk?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/webapp/src/routes/db-playground/+page.svelte (1)
185-241: Acknowledged: Testing ground with console logging.This function contains the console logging approach that was previously discussed. As you noted, this is a testing ground and doesn't need changes at this stage.
Note: The error handling has been improved since the previous review (now correctly assigns
result.error.msg).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
crates/common/src/raindex_client/local_db/mod.rs(2 hunks)packages/webapp/src/routes/db-playground/+page.svelte(7 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/mod.rs
crates/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rust code lives in the workspace under crates/* (e.g., cli, common, bindings, js_api, quote, subgraph, settings, math, integration_tests)
Files:
crates/common/src/raindex_client/local_db/mod.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust with nix develop -c cargo fmt --all
Lint Rust with nix develop -c rainix-rs-static (preconfigured flags)
Use snake_case for Rust crates/modules and PascalCase for types
Files:
crates/common/src/raindex_client/local_db/mod.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/routes/db-playground/+page.svelte
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/routes/db-playground/+page.svelte
packages/**
📄 CodeRabbit inference engine (AGENTS.md)
JavaScript/Svelte projects live under packages/* (webapp, ui-components, orderbook)
Files:
packages/webapp/src/routes/db-playground/+page.svelte
⏰ 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). (2)
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
🔇 Additional comments (9)
crates/common/src/raindex_client/local_db/mod.rs (2)
331-395: Well-implemented Serde deserializer helper.The
bool_from_int_or_boolfunction correctly implements the visitor pattern and handles multiple input types (boolean, integers 0/1, strings "true"/"false"/"1"/"0") with appropriate error messages for invalid values. The use ofdeserialize_anyis the right choice for this flexible deserializer.Minor optimization opportunity (optional): Line 383 uses
to_ascii_lowercase()which allocates a new String. For a micro-optimization, considereq_ignore_ascii_case:fn visit_str<E>(self, v: &str) -> Result<Self::Value, E> where E: Error, { if v.eq_ignore_ascii_case("true") || v == "1" { Ok(true) } else if v.eq_ignore_ascii_case("false") || v == "0" { Ok(false) } else { Err(E::invalid_value( Unexpected::Str(v), &"'true'/'false' or '1'/'0'", )) } }However, the current implementation is clear and the performance difference is likely negligible.
397-443: Comprehensive test coverage.The tests thoroughly verify the
bool_from_int_or_booldeserializer across all supported input types (booleans, integers, strings) and correctly validate rejection of invalid values. Well-structured and complete.packages/webapp/src/routes/db-playground/+page.svelte (7)
7-10: LGTM: LocalDb integration looks good.The import and initialization of
LocalDbclient are correct. The type assertion on line 10 assumes success, but that's acceptable for a playground environment.
18-20: LGTM: Sync state variables properly declared.The
syncStatusandisSyncingvariables are correctly typed and initialized to support sync progress tracking in the UI.Also applies to: 27-29
36-38: LGTM: Proper initialization of sync status on mount.Loading the last sync info after database initialization is the right approach for displaying initial state to the user.
130-130: LGTM: Correct usage of LocalDb.getSyncStatus.The method call properly passes the bound query function and the result is handled correctly.
157-183: LGTM: Enhanced sync with status callback and proper state management.The improvements to
performAutoSyncare well-implemented:
- The status callback correctly updates
syncStatusfor real-time UI feedback- The
isSyncingflag is properly set at the start and reset in the finally block to prevent state inconsistencies- The finally block ensures cleanup even when errors occur
285-296: LGTM: Clean sync status UI implementation.The conditional display logic correctly shows real-time sync status when
isSyncingis true and falls back to displaying the last synced block when idle. The reactive variables will ensure the UI updates automatically.
338-338: LGTM: Proper LocalDb integration in UI controls.The
clearTablesmethod call and the new Order Management section correctly use theLocalDbclient methods. The UI structure is consistent with the existing patterns in the playground.Also applies to: 348-358
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| mod bool_deserialize_tests { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Misleading module name.
The module name bool_deserialize_tests suggests it contains tests for boolean deserialization, but the tests inside (lines 235-329) actually verify LocalDb error handling for decode_events and decoded_events_to_sql. Meanwhile, the actual boolean deserialization tests are in a separate tests module starting at line 397.
Rename this module to better reflect its content:
-mod bool_deserialize_tests {
+mod local_db_error_tests {Alternatively, consolidate the test modules or move the boolean deserialization tests into this module.
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/local_db/mod.rs around line 234, the module
named `bool_deserialize_tests` is misleading because it actually contains
LocalDb error-handling tests for `decode_events` and `decoded_events_to_sql`;
rename the module (for example to `localdb_error_tests` or `decode_error_tests`)
to reflect its purpose, and update the module declaration and any internal
references accordingly; alternatively, move the actual boolean deserialization
tests into this module and keep the current name—choose one approach and make
the module name and contents consistent.
|
@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
Now that we have the sync logic, we are ready to start implementing the queries one by one. This PR implements the first local db function for orders query in webapp and refactors a bunch of queries to test out the functionality.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Improvements
Database Migrations