Refactor orders list query to include token metadata #2147
Merged
Conversation
Contributor
WalkthroughAdds a local SQLite WASM DB to the webapp, wires it to RaindexClient via a JS callback, extends local DB schema/queries (order_bytes, token metadata, new filters), and migrates internal RaindexClient sharing from Arc<RwLock<…>> to Rc<…> across orders, vaults, and related codepaths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant LayoutLoad as +layout.ts
participant SQLiteWasm as sqlite-web
participant LocalDb as SQLiteWasmDatabase
participant RaindexClient
participant UI as Svelte Layout
Browser->>LayoutLoad: load()
LayoutLoad->>SQLiteWasm: init()
SQLiteWasm-->>LayoutLoad: ready / error
alt init ok
LayoutLoad->>LocalDb: new SQLiteWasmDatabase()
LayoutLoad->>RaindexClient: create / hydrate
LayoutLoad->>RaindexClient: set_local_db_callback(LocalDb.query)
LayoutLoad-->>UI: { raindexClient, localDb, stores }
UI->>UI: <LocalDbProvider localDb>…<RaindexClientProvider>…</>
else init error
LayoutLoad-->>UI: { errorMessage, raindexClient: null, localDb: null }
end
sequenceDiagram
autonumber
participant UI
participant Client as RaindexClient
participant LocalDb
participant Subgraph as Orderbook SG
UI->>Client: getOrders(chain_ids, filters, limit)
Client->>Client: partition chains by LocalDb::check_support
alt supported chains
Client->>LocalDb: fetch_orders(FetchOrdersArgs)
LocalDb-->>Client: LocalDbOrder[] (includes order_bytes)
else unsupported chains
Client->>Subgraph: query orders
Subgraph-->>Client: sg orders
end
Client-->>UI: merged orders
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
changed the title
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/common/src/raindex_client/add_orders.rs (1)
61-77: Move theRccreation below the.awaitConstructing the
Rcbefore the.awaitstores it across the suspend point, which makes the async future!Sendand breaks non‑wasm callers (e.g. the existing#[tokio::test]on the multi-thread runtime). Create theRconly after the awaited call so the future remainsSend.Apply this diff:
- let raindex_client = Rc::new(self.clone()); - let client = self.get_orderbook_client(orderbook_address)?; - - let orders = client - .transaction_add_orders(Id::new(tx_hash.to_string())) - .await?; + let client = self.get_orderbook_client(orderbook_address)?; + + let orders = client + .transaction_add_orders(Id::new(tx_hash.to_string())) + .await?; + + let raindex_client = Rc::new(self.clone());packages/webapp/src/routes/+layout.ts (1)
8-12: AddlocalDbto the typed load data
loadreturns alocalDbproperty, butLayoutDatadoesn’t declare it, so TypeScript hits an excess-property error. Declare the field and returnnullin the error paths.Apply this diff:
export interface LayoutData { errorMessage?: string; stores: AppStoresInterface | null; raindexClient: RaindexClient | null; + localDb: SQLiteWasmDatabase | null; } @@ } catch (error: unknown) { errorMessage = 'Failed to get site config settings. ' + (error as Error).message; return { errorMessage, stores: null, - raindexClient: null + raindexClient: null, + localDb: null }; } @@ if (raindexClientRes.error) { return { errorMessage: raindexClientRes.error.readableMsg, stores: null, - raindexClient: null + raindexClient: null, + localDb: null }; } else { raindexClient = raindexClientRes.value; @@ } catch (error: unknown) { return { errorMessage: 'Error initializing RaindexClient: ' + (error as Error).message, stores: null, - raindexClient: null + raindexClient: null, + localDb: null }; } @@ if (localDbRes.error) { return { errorMessage: 'Error initializing local database: ' + localDbRes.error.readableMsg, stores: null, - raindexClient: null + raindexClient: null, + localDb: null }; } else { localDb = localDbRes.value; @@ } catch (error: unknown) { return { errorMessage: 'Error initializing local database: ' + (error as Error).message, stores: null, - raindexClient: null + raindexClient: null, + localDb: null }; }Also applies to: 28-33, 39-44, 49-53, 61-65, 70-74
crates/common/src/raindex_client/remove_orders.rs (1)
62-78: Move theRccreation below the.awaitCreating the
Rcbefore the awaited subgraph call stores it across the suspend point, so this async method becomes!Sendand fails under the default multi-thread tokio runtime. Instantiate theRconly after the awaited call.Apply this diff:
- let raindex_client = Rc::new(self.clone()); - let client = self.get_orderbook_client(orderbook_address)?; - - let orders = client - .transaction_remove_orders(Id::new(tx_hash.to_string())) - .await?; + let client = self.get_orderbook_client(orderbook_address)?; + + let orders = client + .transaction_remove_orders(Id::new(tx_hash.to_string())) + .await?; + + let raindex_client = Rc::new(self.clone());crates/common/src/raindex_client/local_db/query/fetch_vault/query.sql (1)
75-78: Don’t drop vault rows when metadata is missingSwitching to an INNER JOIN against
erc20_tokensmeans any token without metadata (fresh sync, cleared tables, or a failed metadata fetch) causes the entire vault row to disappear. In practiceget_orders_local_dbwill return empty vault lists even though the order events are in the DB. Keep the row by using a LEFT JOIN and defaulting the new fields.- JOIN erc20_tokens et - ON et.chain_id = '?chain_id' - AND lower(et.address) = lower(o.token) + LEFT JOIN erc20_tokens et + ON et.chain_id = '?chain_id' + AND lower(et.address) = lower(o.token)Add
COALESCEaround the select list (e.g.COALESCE(et.name, o.token) AS token_name, similar for symbol/decimals) so the JSON decoding still succeeds when metadata is absent.
📜 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 (24)
crates/common/src/raindex_client/add_orders.rs(2 hunks)crates/common/src/raindex_client/local_db/insert.rs(5 hunks)crates/common/src/raindex_client/local_db/mod.rs(2 hunks)crates/common/src/raindex_client/local_db/query/create_tables/query.sql(1 hunks)crates/common/src/raindex_client/local_db/query/fetch_orders/mod.rs(14 hunks)crates/common/src/raindex_client/local_db/query/fetch_orders/query.sql(3 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault/mod.rs(6 hunks)crates/common/src/raindex_client/local_db/query/fetch_vault/query.sql(2 hunks)crates/common/src/raindex_client/local_db/query/fetch_vaults/mod.rs(2 hunks)crates/common/src/raindex_client/local_db/query/fetch_vaults/query.sql(2 hunks)crates/common/src/raindex_client/local_db/sync.rs(1 hunks)crates/common/src/raindex_client/mod.rs(6 hunks)crates/common/src/raindex_client/orders.rs(19 hunks)crates/common/src/raindex_client/remove_orders.rs(2 hunks)crates/common/src/raindex_client/vaults.rs(13 hunks)packages/ui-components/package.json(1 hunks)packages/ui-components/src/lib/errors/DeploymentStepsError.ts(1 hunks)packages/ui-components/src/lib/hooks/useLocalDb.ts(1 hunks)packages/ui-components/src/lib/index.ts(1 hunks)packages/ui-components/src/lib/providers/LocalDbProvider.svelte(1 hunks)packages/webapp/package.json(0 hunks)packages/webapp/src/routes/+layout.svelte(2 hunks)packages/webapp/src/routes/+layout.ts(3 hunks)packages/webapp/src/routes/db-playground/+page.svelte(2 hunks)
💤 Files with no reviewable changes (1)
- packages/webapp/package.json
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: brusherru
PR: rainlanguage/rain.orderbook#2036
File: crates/js_api/src/filters/raindex_filter_store.rs:319-336
Timestamp: 2025-08-01T07:44:53.910Z
Learning: In the rainlanguage/rain.orderbook project's RaindexFilterStore (crates/js_api/src/filters/raindex_filter_store.rs), the team chose a simplified monolithic approach with hard-coded keys and default auto-save behavior over configurable stores. The update_vaults method intentionally auto-saves to both localStorage and URL after each update as the default behavior, following a design evolution from a previous configurable approach.
📚 Learning: 2025-07-10T12:52:47.468Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1925
File: packages/ui-components/src/lib/errors/DeploymentStepsError.ts:16-16
Timestamp: 2025-07-10T12:52:47.468Z
Learning: In packages/ui-components/src/lib/errors/DeploymentStepsError.ts, the error codes NO_SELECT_TOKENS ('Error loading tokens') and NO_AVAILABLE_TOKENS ('Error loading available tokens') represent different failure scenarios in the token loading workflow and should remain as separate error codes.
Applied to files:
packages/ui-components/src/lib/errors/DeploymentStepsError.ts
📚 Learning: 2025-06-24T13:36:28.797Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1950
File: crates/common/src/raindex_client/orders.rs:301-301
Timestamp: 2025-06-24T13:36:28.797Z
Learning: In the RaindexClient codebase, when Arc::new(RwLock::new(self.clone())) is used (e.g., in get_orders and get_order_by_hash methods), this creates new Arc/RwLock wrappers around a cloned RaindexClient, but the underlying client data is functionally the same. This pattern is used to provide the correct Arc<RwLock<RaindexClient>> type expected by RaindexOrder::try_from_sg_order() method.
Applied to files:
crates/common/src/raindex_client/add_orders.rscrates/common/src/raindex_client/remove_orders.rscrates/common/src/raindex_client/vaults.rscrates/common/src/raindex_client/orders.rs
📚 Learning: 2025-08-26T14:52:37.000Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2099
File: crates/common/src/hyper_rpc.rs:3-7
Timestamp: 2025-08-26T14:52:37.000Z
Learning: In the rain.orderbook codebase, creating new reqwest::Client instances per request in HyperRpcClient is not considered an issue by the maintainers, despite potential performance benefits of client reuse.
Applied to files:
crates/common/src/raindex_client/add_orders.rscrates/common/src/raindex_client/remove_orders.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.rscrates/common/src/raindex_client/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/mod.rscrates/common/src/raindex_client/orders.rs
📚 Learning: 2025-07-16T10:40:05.717Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2000
File: crates/common/src/raindex_client/vaults.rs:183-183
Timestamp: 2025-07-16T10:40:05.717Z
Learning: In the rainlanguage/rain.orderbook codebase, user findolor considers breaking changes from Option<U256> to U256 for required fields like decimals in RaindexVaultToken to be acceptable and safe, even when they affect multiple usage sites across the codebase.
Applied to files:
crates/common/src/raindex_client/vaults.rs
📚 Learning: 2025-06-20T07:51:08.790Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1941
File: crates/js_api/src/raindex/vaults.rs:175-181
Timestamp: 2025-06-20T07:51:08.790Z
Learning: In the RaindexClient vault methods, `&self` parameters are intentionally kept for API consistency to make all vault operations instance methods, even when the methods don't use client state. This is a design preference for maintaining a uniform interface.
Applied to files:
crates/common/src/raindex_client/vaults.rscrates/common/src/raindex_client/orders.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/vaults.rscrates/common/src/raindex_client/orders.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/vaults.rscrates/common/src/raindex_client/orders.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/orders.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/orders.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/orders.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: copilot-setup-steps
- GitHub Check: Deploy-Docs-Preview
- GitHub Check: Deploy-Preview-Push
- GitHub Check: test
- GitHub Check: build-tauri (ubuntu-22.04, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-artifacts, true)
- 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-sol-static)
- GitHub Check: standard-tests (ubuntu-latest, ob-rs-test, true)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-artifacts)
- GitHub Check: standard-tests (ubuntu-latest, rainix-wasm-test)
- GitHub Check: standard-tests (ubuntu-latest, rainix-rs-static)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-legal)
- GitHub Check: standard-tests (ubuntu-latest, rainix-sol-test)
- GitHub Check: test
🔇 Additional comments (6)
crates/common/src/raindex_client/local_db/mod.rs (1)
198-200: LGTM!The
check_supportmethod provides a clean API for verifying chain support before attempting local DB operations. The implementation is straightforward and correct.packages/ui-components/src/lib/errors/DeploymentStepsError.ts (1)
6-6: LGTM!The new error code
NO_LOCAL_DB_PROVIDERfollows the established pattern and provides a clear error message. It's appropriately placed alongside other provider-related error codes.packages/ui-components/src/lib/providers/LocalDbProvider.svelte (1)
1-11: LGTM!The
LocalDbProvidercomponent correctly implements the Svelte context provider pattern:
- Uses
setContextto make the database available to descendants- Shares the context key with
useLocalDbhook for type-safe retrieval- Simple and focused implementation
The component appropriately delegates error handling to the consuming hook rather than checking if
localDbis null here.packages/ui-components/src/lib/index.ts (2)
138-138: LGTM!The public export of
LocalDbProviderappropriately exposes the new local database provider component, following the established pattern for other providers in the library.
143-143: LGTM!The public export of
useLocalDbmakes the local database hook available to consumers, consistent with other hooks exported from this module.Note: There's a critical issue in the
useLocalDbimplementation itself (see separate comment on that file).crates/common/src/raindex_client/local_db/sync.rs (1)
22-22: Confirm DUMP_URL version pinning
- Verified URL is accessible (HTTP 200) and content is valid gzip
- Ensure the commit hash
ca1247390fc95e62fcc7eeab07cef92aa8097b0cis intentionally pinned and documented- Add or update a process (e.g., CI check or documentation) to bump this hash when the dump schema changes
Comment on lines
+20
to
+21
| const SUPPORTED_LOCAL_DB_CHAINS: &[u32] = &[42161]; | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Consider documenting the supported chain selection.
The introduction of SUPPORTED_LOCAL_DB_CHAINS provides a clear gate for local DB functionality. Chain ID 42161 (Arbitrum One) is the only supported chain initially, which is appropriate for a phased rollout.
Consider adding a comment explaining the criteria for supported chains or linking to documentation about the local DB availability.
🤖 Prompt for AI Agents
crates/common/src/raindex_client/local_db/mod.rs around lines 20 to 21: add a
short doc comment above the SUPPORTED_LOCAL_DB_CHAINS constant explaining why
chain ID 42161 is the only supported chain (e.g., phased rollout, operator
support, compatibility requirements), list the selection criteria (network,
compatibility, operator policies), and include a link/reference to external docs
or an issue that tracks broader rollout; keep the comment concise and update any
related README or module-level docs if present.
Contributor
There was a problem hiding this comment.
@findolor, understood! I'll skip this suggestion.
🧠 Learnings used
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.
Comment on lines
+431
to
+435
| result.push(RaindexOrderAsIO { | ||
| id: Bytes::from_str("0x01")?, | ||
| order_hash, | ||
| active, | ||
| }); |
Contributor
There was a problem hiding this comment.
Use the encoded IO id instead of hard-coding 0x01
Every entry returned by try_from_local_db_orders_csv currently reports the same id (0x01) because we drop the first CSV segment. Downstream consumers expect unique IDs (the subgraph delivers them), so lists keyed by id collapse and tooling can’t distinguish entries. Parse the first field instead of hard-wiring it.
- result.push(RaindexOrderAsIO {
- id: Bytes::from_str("0x01")?,
+ result.push(RaindexOrderAsIO {
+ id: Bytes::from_str(_id_str)?,
order_hash,
active,
});If we later start emitting real IDs from SQL, this will just work.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/orders.rs around lines 431–435, replace the
hard-coded id Bytes::from_str("0x01")? with the actual first CSV segment parsed
for the row (e.g. parse the first field used earlier, trim it, ensure it has the
expected "0x" hex prefix if required) and call Bytes::from_str on that value;
this ensures each RaindexOrderAsIO uses the CSV-provided id instead of a
constant so downstream consumers get unique IDs.
Collaborator
Author
There was a problem hiding this comment.
this logic changes in downstream PRs so no need to update here
Contributor
There was a problem hiding this comment.
@findolor, understood! I'll leave this for the downstream PR where the logic will be updated. Thanks for clarifying! 🐰
🧠 Learnings used
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2163
File: crates/common/src/raindex_client/orders.rs:738-741
Timestamp: 2025-10-02T19:17:20.332Z
Learning: In crates/common/src/raindex_client/orders.rs, fetch_dotrain_source() is intentionally called in try_from_sg_order for every order conversion because the dotrain source information is needed immediately. A future optimization with local DB logic is planned to eliminate the network round-trip concern.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1706
File: crates/quote/src/js_api/mod.rs:57-61
Timestamp: 2025-04-29T06:19:15.446Z
Learning: In the rain.orderbook codebase, the `get_id` function is designed to use `U256::from_str()` rather than `U256::from_hex()` for parsing order hashes, despite the potential for hex string inputs.
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1950
File: crates/common/src/raindex_client/orders.rs:301-301
Timestamp: 2025-06-24T13:36:28.797Z
Learning: In the RaindexClient codebase, when Arc::new(RwLock::new(self.clone())) is used (e.g., in get_orders and get_order_by_hash methods), this creates new Arc/RwLock wrappers around a cloned RaindexClient, but the underlying client data is functionally the same. This pattern is used to provide the correct Arc<RwLock<RaindexClient>> type expected by RaindexOrder::try_from_sg_order() method.
| "fuse.js": "7.0.0", | ||
| "lightweight-charts": "4.2.1", | ||
| "lodash": "4.17.21", | ||
| "sqlite-web": "file:../webapp/sqlite-web-0.1.0.tgz", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Verify local tarball dependency and consider alternative approaches.
The sqlite-web dependency references a local tarball in the ../webapp directory. This approach has several considerations:
- Build reproducibility: Local file dependencies can cause issues in CI/CD if the tarball isn't committed or is out of sync
- Cross-package coupling: ui-components now depends on a file in the webapp package
- Version management: Updates to the tarball require manual coordination
Please verify:
- Is the tarball committed to the repository?
- Is there a build process that generates/updates this tarball?
- Would it be better to:
- Publish sqlite-web to npm (even as a private package)
- Use workspace dependencies if this is a monorepo
- Move the tarball to a shared location
Run the following script to check if the tarball exists:
🏁 Script executed:
#!/bin/bash
# Check if the sqlite-web tarball exists
TARBALL_PATH="packages/webapp/sqlite-web-0.1.0.tgz"
if [ -f "$TARBALL_PATH" ]; then
echo "✓ Tarball exists at $TARBALL_PATH"
echo "Size: $(du -h "$TARBALL_PATH" | cut -f1)"
# Check if it's committed to git
if git ls-files --error-unmatch "$TARBALL_PATH" 2>/dev/null; then
echo "✓ Tarball is committed to git"
else
echo "✗ Tarball is NOT committed to git"
fi
else
echo "✗ Tarball not found at $TARBALL_PATH"
fiLength of output: 134
Use a workspace dependency or publish sqlite-web instead of a committed tarball
- The
sqlite-web-0.1.0.tgzfile is present and committed, so CI reproducibility is covered. - However, local tarballs require manual updates and introduce cross-package coupling.
- Switch to a workspace reference (Yarn/PNPM workspaces or Lerna) or publish
sqlite-webas a private npm package to streamline versioning and builds.
🤖 Prompt for AI Agents
In packages/ui-components/package.json around line 77, the dependency currently
points to a committed tarball ("file:../webapp/sqlite-web-0.1.0.tgz"); replace
this with a workspace dependency or a published package version to avoid manual
tarball updates. If using workspaces (Yarn/PNPM/Lerna), change the dependency to
the workspace protocol (e.g., "sqlite-web": "workspace:*" or the appropriate
workspace spec for your monorepo), remove the committed tarball from the repo
and gitignore generated tgz files, then run an install to update the lockfile;
alternatively, publish sqlite-web to your private registry and point the
dependency to the published version (e.g., "sqlite-web": "1.0.0").
Collaborator
Author
There was a problem hiding this comment.
we remove this in downstream PRs so no need to update here
Contributor
There was a problem hiding this comment.
@findolor, understood! Since this will be addressed in downstream PRs, I'll skip this suggestion for now. Thank you for the clarification.
Comment on lines
+7
to
+13
| export function useLocalDb(): SQLiteWasmDatabase { | ||
| const db = getContext<SQLiteWasmDatabase>(LOCAL_DB_CONTEXT_KEY); | ||
| if (!db) { | ||
| DeploymentStepsError.catch(null, DeploymentStepsErrorCode.NO_LOCAL_DB_PROVIDER); | ||
| } | ||
| return db; | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Function returns undefined despite non-nullable return type.
The useLocalDb function has a type safety violation:
- Return type is
SQLiteWasmDatabase(non-nullable) - If
dbis null/undefined,DeploymentStepsError.catchis called (which sets an error store but doesn't throw) - The function then returns the null/undefined
dbon line 12
This creates a runtime error when consumers use the returned value expecting a valid database instance.
Apply one of these fixes:
Option 1: Throw an error (recommended)
export function useLocalDb(): SQLiteWasmDatabase {
const db = getContext<SQLiteWasmDatabase>(LOCAL_DB_CONTEXT_KEY);
if (!db) {
- DeploymentStepsError.catch(null, DeploymentStepsErrorCode.NO_LOCAL_DB_PROVIDER);
+ const error = new DeploymentStepsError(DeploymentStepsErrorCode.NO_LOCAL_DB_PROVIDER);
+ DeploymentStepsError.error.set(error);
+ throw error;
}
return db;
}Option 2: Change return type to be nullable
-export function useLocalDb(): SQLiteWasmDatabase {
+export function useLocalDb(): SQLiteWasmDatabase | undefined {
const db = getContext<SQLiteWasmDatabase>(LOCAL_DB_CONTEXT_KEY);
if (!db) {
DeploymentStepsError.catch(null, DeploymentStepsErrorCode.NO_LOCAL_DB_PROVIDER);
}
return db;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function useLocalDb(): SQLiteWasmDatabase { | |
| const db = getContext<SQLiteWasmDatabase>(LOCAL_DB_CONTEXT_KEY); | |
| if (!db) { | |
| DeploymentStepsError.catch(null, DeploymentStepsErrorCode.NO_LOCAL_DB_PROVIDER); | |
| } | |
| return db; | |
| } | |
| export function useLocalDb(): SQLiteWasmDatabase { | |
| const db = getContext<SQLiteWasmDatabase>(LOCAL_DB_CONTEXT_KEY); | |
| if (!db) { | |
| const error = new DeploymentStepsError(DeploymentStepsErrorCode.NO_LOCAL_DB_PROVIDER); | |
| DeploymentStepsError.error.set(error); | |
| throw error; | |
| } | |
| return db; | |
| } |
🤖 Prompt for AI Agents
In packages/ui-components/src/lib/hooks/useLocalDb.ts around lines 7 to 13, the
function declares a non-nullable return type but can return undefined when
getContext returns null; call to DeploymentStepsError.catch only sets an error
store and does not stop execution. Fix by making the function never return
undefined: either (recommended) throw an Error (or rethrow a
DeploymentStepsError) immediately after the DeploymentStepsError.catch call so
callers never receive undefined, or change the function signature and all
callers to accept a nullable return type (SQLiteWasmDatabase | null) and handle
nulls appropriately.
Collaborator
Author
There was a problem hiding this comment.
this setup is the same setup as our other hooks and does not have an issue
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all hook files in the ui-components package
fd -e ts -e js 'use.*\.ts$' packages/ui-components/src/lib/hooks/
# Show the content of other hooks to compare patterns
fd -e ts 'use.*\.ts$' packages/ui-components/src/lib/hooks/ --exec cat {}
Length of output: 3986
🏁 Script executed:
#!/bin/bash
# Find and examine the DeploymentStepsError class implementation
fd -e ts 'DeploymentStepsError' packages/ui-components/src/lib/errors/ --exec cat {}
Length of output: 1808
🏁 Script executed:
#!/bin/bash
# Search for other usages of DeploymentStepsError.catch pattern
rg -nP --type=ts 'DeploymentStepsError\.catch\(' packages/ui-components/
Length of output: 411
@findolor, you're absolutely right! I can see that useGui.ts and useRaindexClient.ts follow the exact same pattern:
// useGui.ts
export function useGui(): DotrainOrderGui {
const gui = getContext<DotrainOrderGui>(GUI_CONTEXT_KEY);
if (!gui) {
DeploymentStepsError.catch(null, DeploymentStepsErrorCode.NO_GUI_PROVIDER);
}
return gui;
}
// useRaindexClient.ts
export function useRaindexClient(): RaindexClient {
const raindexClient = getContext<RaindexClient>(RAINDEX_CLIENT_CONTEXT_KEY);
if (!raindexClient) {
DeploymentStepsError.catch(null, DeploymentStepsErrorCode.NO_RAINDEX_CLIENT_PROVIDER);
}
return raindexClient;
}This is your established pattern where DeploymentStepsError.catch sets an error in a Svelte store, and the UI handles the error state before any code tries to use the undefined value. I've noted this for future reviews.
✏️ Learnings added
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2147
File: packages/ui-components/src/lib/hooks/useLocalDb.ts:7-13
Timestamp: 2025-10-06T13:22:34.108Z
Learning: In the rain.orderbook repository, custom hooks in packages/ui-components/src/lib/hooks/ (useGui, useRaindexClient, useLocalDb) intentionally use a pattern where they call DeploymentStepsError.catch when getContext returns null/undefined, then return the potentially null value despite having a non-nullable return type. This is the established pattern where error handling is done via Svelte stores and UI error states, not through thrown exceptions or nullable return types.
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.
0xgleb
requested changes
Oct 7, 2025
|
|
||
| sql.push_str(&format!( | ||
| "INSERT INTO order_events (block_number, block_timestamp, transaction_hash, log_index, event_type, sender, order_hash, order_owner, order_nonce) VALUES ({}, {}, '{}', {}, 'AddOrderV3', '{}', '{}', '{}', '{}');\n", | ||
| "INSERT INTO order_events (block_number, block_timestamp, transaction_hash, log_index, event_type, sender, order_hash, order_owner, order_nonce, order_bytes) VALUES ({}, {}, '{}', {}, 'AddOrderV3', '{}', '{}', '{}', '{}', '{}');\n", |
Collaborator
There was a problem hiding this comment.
nitpick: would be worth breaking this down into multiple lines for readability
Comment on lines
+533
to
+543
| #[cfg(target_family = "wasm")] | ||
| pub fn new_test_client_with_db_callback( | ||
| yamls: Vec<String>, | ||
| callback: js_sys::Function, | ||
| ) -> RaindexClient { | ||
| let mut client = RaindexClient::new(yamls, None).expect("test yaml should be valid"); | ||
| client | ||
| .set_local_db_callback(callback) | ||
| .expect("setting db callback should succeed"); | ||
| client | ||
| } |
Collaborator
There was a problem hiding this comment.
turn this into Result instead of using .expect
Collaborator
Author
There was a problem hiding this comment.
this is test logic so i'm just going to use unwrap
Collaborator
There was a problem hiding this comment.
didn't realize it was inside of mod tests. all good as is
Comment on lines
+401
to
+430
| let mut segs = part.split(':'); | ||
| let _id_str = segs.next().ok_or(RaindexError::JsError(format!( | ||
| "Invalid {} entry: missing id", | ||
| field_name | ||
| )))?; | ||
| let hash_str = segs.next().ok_or(RaindexError::JsError(format!( | ||
| "Invalid {} entry: missing order hash", | ||
| field_name | ||
| )))?; | ||
| let active_str = segs.next().ok_or(RaindexError::JsError(format!( | ||
| "Invalid {} entry: missing active flag", | ||
| field_name | ||
| )))?; | ||
| if segs.next().is_some() { | ||
| return Err(RaindexError::JsError(format!( | ||
| "Invalid {} entry: too many fields", | ||
| field_name | ||
| ))); | ||
| } | ||
| let order_hash = Bytes::from_str(hash_str)?; | ||
| let active = match active_str { | ||
| "1" => true, | ||
| "0" => false, | ||
| _ => { | ||
| return Err(RaindexError::JsError(format!( | ||
| "Invalid active flag in {}: {}", | ||
| field_name, active_str | ||
| ))) | ||
| } | ||
| }; |
Collaborator
There was a problem hiding this comment.
can we not use csv crate or something to make this less manual?
Comment on lines
+1239
to
+1248
| let mut id = Vec::with_capacity( | ||
| vault.orderbook_address.len() | ||
| + vault.owner.len() | ||
| + vault.token.len() | ||
| + vault.vault_id.len(), | ||
| ); | ||
| id.extend_from_slice(vault.orderbook_address.as_bytes()); | ||
| id.extend_from_slice(vault.owner.as_bytes()); | ||
| id.extend_from_slice(vault.token.as_bytes()); | ||
| id.extend_from_slice(vault.vault_id.as_bytes()); |
Collaborator
There was a problem hiding this comment.
Suggested change
| let mut id = Vec::with_capacity( | |
| vault.orderbook_address.len() | |
| + vault.owner.len() | |
| + vault.token.len() | |
| + vault.vault_id.len(), | |
| ); | |
| id.extend_from_slice(vault.orderbook_address.as_bytes()); | |
| id.extend_from_slice(vault.owner.as_bytes()); | |
| id.extend_from_slice(vault.token.as_bytes()); | |
| id.extend_from_slice(vault.vault_id.as_bytes()); | |
| let id: Vec<u8> = vault.orderbook_address.as_bytes() | |
| .iter() | |
| .chain(vault.owner.as_bytes()) | |
| .chain(vault.token.as_bytes()) | |
| .chain(vault.vault_id.as_bytes()) | |
| .copied() | |
| .collect(); |
| "{}-{}", | ||
| vault.vault_id, vault.token | ||
| )))?, | ||
| id: Bytes::from(keccak256(&id).as_slice().to_vec()), |
Collaborator
There was a problem hiding this comment.
do we need to hash the id here?
0xgleb
approved these changes
Oct 7, 2025
Comment on lines
+411
to
+451
| let record = record.map_err(|err| { | ||
| RaindexError::JsError(format!( | ||
| "Invalid {} entry: failed to parse record ({err})", | ||
| field_name | ||
| )) | ||
| })?; | ||
| let mut fields = record.iter(); | ||
| let _id_str = fields.next().ok_or(RaindexError::JsError(format!( | ||
| "Invalid {} entry: missing id", | ||
| field_name | ||
| )))?; | ||
| let hash_str = fields.next().ok_or(RaindexError::JsError(format!( | ||
| "Invalid {} entry: missing order hash", | ||
| field_name | ||
| )))?; | ||
| let active_str = fields.next().ok_or(RaindexError::JsError(format!( | ||
| "Invalid {} entry: missing active flag", | ||
| field_name | ||
| )))?; | ||
| if fields.next().is_some() { | ||
| return Err(RaindexError::JsError(format!( | ||
| "Invalid {} entry: too many fields", | ||
| field_name | ||
| ))); | ||
| } | ||
| let order_hash = Bytes::from_str(hash_str)?; | ||
| let active = match active_str { | ||
| "1" => true, | ||
| "0" => false, | ||
| _ => { | ||
| return Err(RaindexError::JsError(format!( | ||
| "Invalid active flag in {}: {}", | ||
| field_name, active_str | ||
| ))) | ||
| } | ||
| }; | ||
| result.push(RaindexOrderAsIO { | ||
| id: Bytes::from_str("0x01")?, | ||
| order_hash, | ||
| active, | ||
| }); |
Collaborator
There was a problem hiding this comment.
nitpick: RaindexOrderAsIO can probably just implement some trait for csv parsing and then this function can become much simpler. not critical though
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/ui-components/package.json (1)
77-77: Blocking: file-based dependency breaks published package installsShipping
@rainlanguage/ui-componentswith"sqlite-web": "file:../webapp/sqlite-web-0.1.0.tgz"means anyone installing the published package (npm/yarn/pnpm) will fail—consumers do not have../webapp/in their node_modules tree, so the dependency cannot be resolved. Please switch to a workspace protocol (e.g.,workspace:*) or publishsqlite-webunder a versioned tag before release. Otherwise the package cannot be installed outside this monorepo.crates/common/src/raindex_client/orders.rs (1)
391-455: CSV parsing is functional; hard-coded id is tracked for downstream fix.The method correctly parses the compact CSV format (
id:hash:active,id:hash:active,...) with proper error handling and validation. As noted in previous review feedback, the parsing could potentially leverage csv crate's serde support or a trait-based approach for better maintainability, though the current implementation is clear and functional.The hard-coded id at line 448 (
Bytes::from_str("0x01")?) was previously flagged and confirmed by findolor to be addressed in downstream PRs.
📜 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 (7)
crates/common/src/raindex_client/local_db/insert.rs(11 hunks)crates/common/src/raindex_client/local_db/mod.rs(2 hunks)crates/common/src/raindex_client/mod.rs(6 hunks)crates/common/src/raindex_client/orders.rs(21 hunks)crates/common/src/raindex_client/vaults.rs(13 hunks)packages/ui-components/package.json(1 hunks)packages/webapp/src/routes/db-playground/+page.svelte(2 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/vaults.rscrates/common/src/raindex_client/local_db/insert.rscrates/common/src/raindex_client/mod.rscrates/common/src/raindex_client/local_db/mod.rscrates/common/src/raindex_client/orders.rs
crates/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rust code lives in the workspace under crates/* (e.g., cli, common, bindings, js_api, quote, subgraph, settings, math, integration_tests)
Files:
crates/common/src/raindex_client/vaults.rscrates/common/src/raindex_client/local_db/insert.rscrates/common/src/raindex_client/mod.rscrates/common/src/raindex_client/local_db/mod.rscrates/common/src/raindex_client/orders.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust with nix develop -c cargo fmt --all
Lint Rust with nix develop -c rainix-rs-static (preconfigured flags)
Use snake_case for Rust crates/modules and PascalCase for types
Files:
crates/common/src/raindex_client/vaults.rscrates/common/src/raindex_client/local_db/insert.rscrates/common/src/raindex_client/mod.rscrates/common/src/raindex_client/local_db/mod.rscrates/common/src/raindex_client/orders.rs
packages/**
📄 CodeRabbit inference engine (AGENTS.md)
JavaScript/Svelte projects live under packages/* (webapp, ui-components, orderbook)
Files:
packages/ui-components/package.jsonpackages/webapp/src/routes/db-playground/+page.svelte
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
🧠 Learnings (19)
📚 Learning: 2025-06-20T07:51:08.790Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1941
File: crates/js_api/src/raindex/vaults.rs:175-181
Timestamp: 2025-06-20T07:51:08.790Z
Learning: In the RaindexClient vault methods, `&self` parameters are intentionally kept for API consistency to make all vault operations instance methods, even when the methods don't use client state. This is a design preference for maintaining a uniform interface.
Applied to files:
crates/common/src/raindex_client/vaults.rscrates/common/src/raindex_client/orders.rs
📚 Learning: 2025-06-24T13:36:28.797Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1950
File: crates/common/src/raindex_client/orders.rs:301-301
Timestamp: 2025-06-24T13:36:28.797Z
Learning: In the RaindexClient codebase, when Arc::new(RwLock::new(self.clone())) is used (e.g., in get_orders and get_order_by_hash methods), this creates new Arc/RwLock wrappers around a cloned RaindexClient, but the underlying client data is functionally the same. This pattern is used to provide the correct Arc<RwLock<RaindexClient>> type expected by RaindexOrder::try_from_sg_order() method.
Applied to files:
crates/common/src/raindex_client/vaults.rscrates/common/src/raindex_client/orders.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/vaults.rscrates/common/src/raindex_client/orders.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/vaults.rscrates/common/src/raindex_client/orders.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:
packages/ui-components/package.json
📚 Learning: 2025-10-06T10:23:57.479Z
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.479Z
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-07-21T16:34:04.947Z
Learnt from: 0xgleb
PR: rainlanguage/rain.orderbook#1911
File: crates/common/src/raindex_client/orders.rs:720-720
Timestamp: 2025-07-21T16:34:04.947Z
Learning: In the rainlanguage/rain.orderbook codebase, user 0xgleb prefers using `.unwrap()` in test code rather than `.expect()` with descriptive messages, considering the direct unwrap approach acceptable for test contexts where failures should be fast and clear.
Applied to files:
crates/common/src/raindex_client/mod.rs
📚 Learning: 2025-10-06T11:13:29.956Z
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.956Z
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/mod.rs
📚 Learning: 2025-04-29T06:19:15.446Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1706
File: crates/quote/src/js_api/mod.rs:57-61
Timestamp: 2025-04-29T06:19:15.446Z
Learning: In the rain.orderbook codebase, the `get_id` function is designed to use `U256::from_str()` rather than `U256::from_hex()` for parsing order hashes, despite the potential for hex string inputs.
Applied to files:
crates/common/src/raindex_client/orders.rs
📚 Learning: 2025-10-02T19:17:20.332Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#2163
File: crates/common/src/raindex_client/orders.rs:738-741
Timestamp: 2025-10-02T19:17:20.332Z
Learning: In crates/common/src/raindex_client/orders.rs, fetch_dotrain_source() is intentionally called in try_from_sg_order for every order conversion because the dotrain source information is needed immediately. A future optimization with local DB logic is planned to eliminate the network round-trip concern.
Applied to files:
crates/common/src/raindex_client/orders.rs
📚 Learning: 2025-07-15T08:01:38.534Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1994
File: crates/common/src/raindex_client/vaults.rs:282-292
Timestamp: 2025-07-15T08:01:38.534Z
Learning: In the rainlanguage/rain.orderbook codebase, findolor prefers to avoid concurrency optimizations like using `futures::future::try_join_all` for parallel processing of balance changes, considering such optimizations "not that critical at the moment" when the performance impact is minimal.
Applied to files:
crates/common/src/raindex_client/orders.rs
📚 Learning: 2025-06-18T12:56:44.290Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1938
File: crates/js_api/src/raindex/mod.rs:102-118
Timestamp: 2025-06-18T12:56:44.290Z
Learning: In the rainlanguage/rain.orderbook codebase, it's acceptable to scaffold unused enum variants in initial implementation PRs when they will be implemented in future PRs, as confirmed by findolor.
Applied to files:
crates/common/src/raindex_client/orders.rs
📚 Learning: 2025-07-04T09:02:57.301Z
Learnt from: findolor
PR: rainlanguage/rain.orderbook#1956
File: crates/common/src/fuzz/mod.rs:64-64
Timestamp: 2025-07-04T09:02:57.301Z
Learning: In rainlanguage/rain.orderbook, user findolor prefers to limit type consistency changes to only the parts directly related to the current work scope. For example, when updating chain_id fields from u64 to u32 in fuzz-related code, unrelated files like tauri-app wallet commands can remain as u64 if they serve different purposes and aren't part of the current changes.
Applied to files:
crates/common/src/raindex_client/orders.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/orders.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/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/orders.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/orders.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/orders.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/orders.rs
🧬 Code graph analysis (5)
crates/common/src/raindex_client/vaults.rs (3)
crates/common/src/raindex_client/mod.rs (6)
new(111-132)from(238-242)from(320-322)from(326-328)from(447-449)from(453-458)crates/common/src/erc20.rs (2)
new(31-33)name(51-59)crates/common/src/raindex_client/orders.rs (6)
chain_id(76-78)chain_id(147-149)id(80-82)id(150-152)try_from_local_db_orders_csv(392-454)try_from_local_db(834-884)
crates/common/src/raindex_client/local_db/insert.rs (1)
crates/common/src/raindex_client/local_db/tokens.rs (2)
sample_order(64-82)build_event(53-62)
crates/common/src/raindex_client/mod.rs (2)
crates/common/src/raindex_client/local_db/mod.rs (1)
new(190-192)crates/settings/src/spec_version.rs (1)
current(11-13)
crates/common/src/raindex_client/local_db/mod.rs (2)
crates/common/src/raindex_client/orders.rs (2)
chain_id(76-78)chain_id(147-149)crates/common/src/raindex_client/vaults.rs (4)
chain_id(79-81)chain_id(126-128)chain_id(220-222)chain_id(247-249)
crates/common/src/raindex_client/orders.rs (5)
crates/common/src/raindex_client/local_db/query/fetch_orders/mod.rs (3)
fetch_orders(91-171)LocalDbQuery(170-170)from(35-63)crates/common/src/raindex_client/local_db/query/fetch_vault/mod.rs (3)
fetch_vault(28-41)LocalDbQuery(39-39)fetch_vaults_for_io_string(63-76)crates/common/src/raindex_client/mod.rs (9)
get_orderbook_client(199-207)new(111-132)from(238-242)from(320-322)from(326-328)from(447-449)from(453-458)get_local_db_test_yaml(544-588)new_test_client_with_db_callback(534-541)crates/common/src/raindex_client/vaults.rs (36)
get_orderbook_client(270-272)new(169-174)id(87-89)id(132-134)id(224-226)id(250-252)id(757-760)id(772-774)chain_id(79-81)chain_id(126-128)chain_id(220-222)chain_id(247-249)try_from_local_db(1230-1276)vaults(976-992)vault(1176-1180)vault(1181-1185)vault_id(95-97)vault_id(138-140)vault_id(591-594)vault_id(642-644)owner(91-93)owner(135-137)orderbook(111-113)orderbook(150-152)orderbook(633-635)orderbook(672-674)transaction(629-631)transaction(669-671)token(107-109)token(147-149)token(596-598)token(645-647)token(762-764)token(775-777)symbol(236-238)symbol(259-261)crates/common/src/raindex_client/local_db/mod.rs (3)
new(190-192)check_support(198-200)from(158-163)
🔇 Additional comments (5)
crates/common/src/raindex_client/orders.rs (5)
1-62: LGTM! Architectural change from Arc to Rc is well-executed.The migration from
Arc<RwLock<RaindexClient>>toRc<RaindexClient>(line 47) simplifies ownership and access patterns throughout the codebase. The new imports for CSV parsing (line 15) and local DB support (lines 1-4) align with the PR objectives to integrate token metadata from local DB.Based on learnings
234-244: LGTM! Access patterns correctly updated for Rc ownership.Direct method calls replace the previous read-lock pattern, simplifying the code and removing unnecessary synchronization overhead. The
Rc::cloneusage (line 235) is idiomatic and efficient.
489-629: LGTM! Well-structured dual-path query logic.The refactored
get_ordersmethod (lines 489-537) cleanly partitions chain IDs into local DB-supported and subgraph-backed paths, fetching from both sources when needed. The use offutures::future::try_join_all(line 520) for parallel local DB queries across multiple chains is appropriate.In
get_orders_local_db(lines 605-616), vault fetching is sequential per order. While this could be optimized with parallel fetching, the current approach is acceptable and maintains clear error handling.
740-884: LGTM! Conversion methods correctly updated for Rc ownership.Both
try_from_sg_orderandtry_from_local_dbconsistently useRc::clonefor sharing the raindex_client across vaults (lines 764, 778, 858, 869). The TODO comment at line 850 for the hard-coded id is noted and tracks the same issue flagged at line 448.The use of
order_bytesfromLocalDbOrder(line 851) aligns with the schema updates mentioned in the PR objectives.
891-1132: LGTM! Excellent test coverage for new local DB functionality.The new WASM test (lines 891-1075) comprehensively validates the local DB query path, including order construction, vault associations, and field mappings. The unit tests for CSV parsing (lines 1097-1132) properly test both successful parsing and error cases (invalid active flag).
The mock callback setup (lines 905-965) is well-structured and handles different SQL query patterns appropriately.
Comment on lines
+1239
to
1275
| let id: Vec<u8> = vault | ||
| .orderbook_address | ||
| .as_bytes() | ||
| .iter() | ||
| .chain(vault.owner.as_bytes()) | ||
| .chain(vault.token.as_bytes()) | ||
| .chain(vault.vault_id.as_bytes()) | ||
| .copied() | ||
| .collect(); | ||
|
|
||
| Ok(Self { | ||
| raindex_client, | ||
| chain_id, | ||
| vault_type, | ||
| // TODO: Needs updating | ||
| id: Bytes::from_str(&encode_prefixed(format!( | ||
| "{}-{}", | ||
| vault.vault_id, vault.token | ||
| )))?, | ||
| id: Bytes::from(id), | ||
| owner: Address::from_str(&vault.owner)?, | ||
| vault_id: U256::from_str(&vault.vault_id)?, | ||
| balance, | ||
| formatted_balance, | ||
| token: RaindexVaultToken { | ||
| chain_id, | ||
| // TODO: Needs updating | ||
| id: "0x01".to_string(), | ||
| id: vault.token.clone(), | ||
| address: Address::from_str(&vault.token)?, | ||
| // TODO: Needs updating | ||
| name: Some("vault.token_name".to_string()), | ||
| // TODO: Needs updating | ||
| symbol: Some("vault.token_symbol".to_string()), | ||
| decimals: 18, | ||
| name: Some(vault.token_name), | ||
| symbol: Some(vault.token_symbol), | ||
| decimals: vault.token_decimals, | ||
| }, | ||
| orderbook: Address::from_str(&vault.orderbook_address)?, | ||
| orders_as_inputs: vec![], | ||
| orders_as_outputs: vec![], | ||
| // orders_as_inputs: vault.input_order_hashes.map(|hashes| { | ||
| // hashes | ||
| // .into_iter() | ||
| // .map(|hash| RaindexOrderAsIO { | ||
| // order_hash: Bytes::from_str(&hash).unwrap_or_default(), | ||
| // }) | ||
| // .collect() | ||
| // }), | ||
| // orders_as_outputs: vault | ||
| // .orders_as_output | ||
| // .iter() | ||
| // .map(|order| RaindexOrderAsIO::try_from(order.clone())) | ||
| // .collect::<Result<Vec<RaindexOrderAsIO>, RaindexError>>()?, | ||
| orders_as_inputs: RaindexOrderAsIO::try_from_local_db_orders_csv( | ||
| "inputOrders", | ||
| &vault.input_orders, | ||
| )?, | ||
| orders_as_outputs: RaindexOrderAsIO::try_from_local_db_orders_csv( | ||
| "outputOrders", | ||
| &vault.output_orders, | ||
| )?, | ||
| }) |
Contributor
There was a problem hiding this comment.
Correct the vault ID derivation for local DB vaults
Line 1239 builds the ID by concatenating the ASCII bytes of the hex strings. When Bytes::from(id) is later rendered, you end up with something like 0x30… (the hex for '0', 'x', …) instead of the canonical vault identifier composed of the raw address/vault bytes. Any downstream call that relies on this ID matching the subgraph (e.g. vault.get_balance_changes) will fail for vaults sourced from the local DB.
Decode the hex into raw bytes before concatenation so the ID matches the subgraph encoding:
- let id: Vec<u8> = vault
- .orderbook_address
- .as_bytes()
- .iter()
- .chain(vault.owner.as_bytes())
- .chain(vault.token.as_bytes())
- .chain(vault.vault_id.as_bytes())
- .copied()
- .collect();
-
- Ok(Self {
- raindex_client,
- chain_id,
- vault_type,
- id: Bytes::from(id),
- owner: Address::from_str(&vault.owner)?,
- vault_id: U256::from_str(&vault.vault_id)?,
+ let orderbook_address = Address::from_str(&vault.orderbook_address)?;
+ let owner = Address::from_str(&vault.owner)?;
+ let token_address = Address::from_str(&vault.token)?;
+ let vault_id = U256::from_str(&vault.vault_id)?;
+
+ let mut id = Vec::with_capacity(Address::len_bytes() * 3 + B256::len_bytes());
+ id.extend_from_slice(orderbook_address.as_slice());
+ id.extend_from_slice(owner.as_slice());
+ id.extend_from_slice(token_address.as_slice());
+ id.extend_from_slice(B256::from(vault_id).as_slice());
+
+ Ok(Self {
+ raindex_client,
+ chain_id,
+ vault_type,
+ id: Bytes::from(id),
+ owner,
+ vault_id,
balance,
formatted_balance,
token: RaindexVaultToken {
chain_id,
id: vault.token.clone(),
- address: Address::from_str(&vault.token)?,
+ address: token_address,
name: Some(vault.token_name),
symbol: Some(vault.token_symbol),
decimals: vault.token_decimals,
},
- orderbook: Address::from_str(&vault.orderbook_address)?,
+ orderbook: orderbook_address,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let id: Vec<u8> = vault | |
| .orderbook_address | |
| .as_bytes() | |
| .iter() | |
| .chain(vault.owner.as_bytes()) | |
| .chain(vault.token.as_bytes()) | |
| .chain(vault.vault_id.as_bytes()) | |
| .copied() | |
| .collect(); | |
| Ok(Self { | |
| raindex_client, | |
| chain_id, | |
| vault_type, | |
| // TODO: Needs updating | |
| id: Bytes::from_str(&encode_prefixed(format!( | |
| "{}-{}", | |
| vault.vault_id, vault.token | |
| )))?, | |
| id: Bytes::from(id), | |
| owner: Address::from_str(&vault.owner)?, | |
| vault_id: U256::from_str(&vault.vault_id)?, | |
| balance, | |
| formatted_balance, | |
| token: RaindexVaultToken { | |
| chain_id, | |
| // TODO: Needs updating | |
| id: "0x01".to_string(), | |
| id: vault.token.clone(), | |
| address: Address::from_str(&vault.token)?, | |
| // TODO: Needs updating | |
| name: Some("vault.token_name".to_string()), | |
| // TODO: Needs updating | |
| symbol: Some("vault.token_symbol".to_string()), | |
| decimals: 18, | |
| name: Some(vault.token_name), | |
| symbol: Some(vault.token_symbol), | |
| decimals: vault.token_decimals, | |
| }, | |
| orderbook: Address::from_str(&vault.orderbook_address)?, | |
| orders_as_inputs: vec![], | |
| orders_as_outputs: vec![], | |
| // orders_as_inputs: vault.input_order_hashes.map(|hashes| { | |
| // hashes | |
| // .into_iter() | |
| // .map(|hash| RaindexOrderAsIO { | |
| // order_hash: Bytes::from_str(&hash).unwrap_or_default(), | |
| // }) | |
| // .collect() | |
| // }), | |
| // orders_as_outputs: vault | |
| // .orders_as_output | |
| // .iter() | |
| // .map(|order| RaindexOrderAsIO::try_from(order.clone())) | |
| // .collect::<Result<Vec<RaindexOrderAsIO>, RaindexError>>()?, | |
| orders_as_inputs: RaindexOrderAsIO::try_from_local_db_orders_csv( | |
| "inputOrders", | |
| &vault.input_orders, | |
| )?, | |
| orders_as_outputs: RaindexOrderAsIO::try_from_local_db_orders_csv( | |
| "outputOrders", | |
| &vault.output_orders, | |
| )?, | |
| }) | |
| let orderbook_address = Address::from_str(&vault.orderbook_address)?; | |
| let owner = Address::from_str(&vault.owner)?; | |
| let token_address = Address::from_str(&vault.token)?; | |
| let vault_id = U256::from_str(&vault.vault_id)?; | |
| let mut id = Vec::with_capacity(Address::len_bytes() * 3 + B256::len_bytes()); | |
| id.extend_from_slice(orderbook_address.as_slice()); | |
| id.extend_from_slice(owner.as_slice()); | |
| id.extend_from_slice(token_address.as_slice()); | |
| id.extend_from_slice(B256::from(vault_id).as_slice()); | |
| Ok(Self { | |
| raindex_client, | |
| chain_id, | |
| vault_type, | |
| id: Bytes::from(id), | |
| owner, | |
| vault_id, | |
| balance, | |
| formatted_balance, | |
| token: RaindexVaultToken { | |
| chain_id, | |
| id: vault.token.clone(), | |
| address: token_address, | |
| name: Some(vault.token_name), | |
| symbol: Some(vault.token_symbol), | |
| decimals: vault.token_decimals, | |
| }, | |
| orderbook: orderbook_address, | |
| orders_as_inputs: RaindexOrderAsIO::try_from_local_db_orders_csv( | |
| "inputOrders", | |
| &vault.input_orders, | |
| )?, | |
| orders_as_outputs: RaindexOrderAsIO::try_from_local_db_orders_csv( | |
| "outputOrders", | |
| &vault.output_orders, | |
| )?, | |
| }) |
🤖 Prompt for AI Agents
In crates/common/src/raindex_client/vaults.rs around lines 1239 to 1275, the ID
is currently built by concatenating ASCII bytes of the hex string
representations (producing bytes for '0','x','a','b',...), which yields an
incorrect ID; instead decode each hex string (strip any "0x" prefix) into its
raw byte representation and concatenate those raw bytes for orderbook_address,
owner, token and vault_id before constructing Bytes::from(id); ensure you use
the existing hex/Address parsing helpers or hex::decode (returning an error on
invalid hex) so errors propagate correctly.
hardyjosh
approved these changes
Oct 8, 2025
Contributor
|
@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:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. SIZE=L 🧠 Learnings used |
4 tasks
4 tasks
This was referenced Nov 17, 2025
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This PR updates the orders list query to work with token metadata to properly return data for raindex order conversion.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes / UX
Chores