Refactor RaindexClient local DB into single-step construction with deterministic query routing#2488
Open
Refactor RaindexClient local DB into single-step construction with deterministic query routing#2488
Conversation
Replace if-let and split_by_query_source patterns with match on query_source() at all call sites. Each match arm is definitive with no cross-source fallback. Remove split_by_query_source and local_db() helper methods that are no longer needed.
Pass localDb query/wipe callbacks and status callback directly into RaindexClient.new() which is now async. Remove separate setDbCallback and startLocalDbScheduler/stopLocalDbScheduler calls from layout. Update all JS test call sites to await RaindexClient.new().
Change native and wasm scheduler start() to accept ParsedRunnerSettings and SyncReadiness instead of raw YAML. Mark chains ready via sync_readiness.mark_ready() after a successful sync cycle.
Convert trades get_trades_list and get_trade_count to match on query_source. Fix import paths in local_db orders/vaults modules. Update test helpers to pass chain IDs for sync readiness.
Remove static SUPPORTED_LOCAL_DB_CHAINS list and is_chain_supported_local_db. Extract parse_runner_settings_from_yaml helper for reuse by callers that already have an OrderbookYaml instance.
Groups all local-DB routing state alongside LocalDb, SyncReadiness, and the scheduler in raindex_client/local_db. Adds LocalDbState::new() constructor and re-exports from raindex_client/mod.rs so consumer imports remain unchanged. Refactors orders/vaults list methods to use classify_chains helper.
Extract routing state (QuerySource, ClassifiedChains, LocalDbState, SyncReadiness) into state.rs and sync status DTOs into status.rs. Add 16 tests for state.rs covering SyncReadiness, query_source routing, classify_chains partitioning, and compute_chain_ids YAML parsing. Re-exports preserve all existing public API paths.
Reflect async constructor, removed methods, new types, and deterministic query routing across ARCHITECTURE.md and README.md files.
Contributor
WalkthroughIntroduces a LocalDbState runtime container and QuerySource routing (LocalDb vs Subgraph), threads SyncReadiness through schedulers/runners, converts RaindexClient construction to an async factory with optional local-db callbacks, reorganizes local DB state/status into new modules, and replaces static chain checks with dynamic routing. Changes
Sequence DiagramssequenceDiagram
participant Client
participant RaindexClient
participant LocalDbState
participant Scheduler
participant SyncReadiness
Client->>RaindexClient: create(yamls, callbacks)
activate RaindexClient
RaindexClient->>LocalDbState: compute_chain_ids(yaml)
LocalDbState-->>RaindexClient: configured_chains
RaindexClient->>LocalDbState: new(db, scheduler, sync_readiness)
activate LocalDbState
LocalDbState->>Scheduler: start(settings, db_path, sync_readiness)
activate Scheduler
Scheduler-->>LocalDbState: handle
deactivate Scheduler
LocalDbState-->>RaindexClient: initialized
deactivate LocalDbState
RaindexClient-->>Client: RaindexClient instance
deactivate RaindexClient
rect rgba(200,150,100,0.5)
Note over Scheduler,SyncReadiness: Async scheduler operations
Scheduler->>Scheduler: sync_network(chain_id)
Scheduler->>SyncReadiness: mark_ready(chain_id)
end
sequenceDiagram
participant Client
participant RaindexClient
participant LocalDbState
participant QuerySource
participant LocalDb
participant Subgraph
Client->>RaindexClient: get_orders(chain_id)
activate RaindexClient
RaindexClient->>LocalDbState: query_source(chain_id)
activate LocalDbState
LocalDbState->>QuerySource: check sync_readiness & config
alt LocalDb available and ready
QuerySource-->>LocalDbState: QuerySource::LocalDb
else Subgraph required
QuerySource-->>LocalDbState: QuerySource::Subgraph
end
LocalDbState-->>RaindexClient: QuerySource variant
deactivate LocalDbState
alt LocalDb
RaindexClient->>LocalDb: fetch_orders()
LocalDb-->>RaindexClient: orders
else Subgraph
RaindexClient->>Subgraph: fetch_orders()
Subgraph-->>RaindexClient: orders
end
RaindexClient-->>Client: orders
deactivate RaindexClient
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 4
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/transactions.rs (1)
172-199:⚠️ Potential issue | 🟠 MajorAvoid initializing the subgraph client before selecting
QuerySource.Line 172 creates a subgraph client unconditionally. That can fail the call before LocalDb polling even starts, despite
QuerySource::LocalDbbeing selected.Based on learnings: Enforce the exclusive local DB data source pattern for all raindex_client files; when local DB is selected, return local DB results directly without subgraph fallback coupling.💡 Proposed fix
- let client = self.get_orderbook_client(orderbook_address)?; - let attempts = max_attempts .unwrap_or(DEFAULT_TRANSACTION_POLL_ATTEMPTS) .max(1); let interval_ms = interval_ms.unwrap_or(DEFAULT_TRANSACTION_POLL_INTERVAL_MS); match self.query_source(chain_id) { QuerySource::LocalDb(local_db) => { let local_source = LocalDbTransactions::new(&local_db); let ob_id = OrderbookIdentifier::new(chain_id, orderbook_address); @@ Err(RaindexError::TransactionIndexingTimeout { tx_hash, attempts }) } QuerySource::Subgraph => { + let client = self.get_orderbook_client(orderbook_address)?; for attempt in 1..=attempts { match client .transaction_detail(Id::new(tx_hash.to_string())) .await {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/common/src/raindex_client/transactions.rs` around lines 172 - 199, The code currently calls self.get_orderbook_client(orderbook_address) before checking query_source, which can fail even when QuerySource::LocalDb is selected; move the get_orderbook_client() call into the QuerySource::Subgraph arm and only construct the subgraph client there, leaving the QuerySource::LocalDb path to use LocalDbTransactions::new(&local_db) with OrderbookIdentifier::new(chain_id, orderbook_address) and return results directly without any subgraph fallback; update the function containing this logic (the block that calls get_orderbook_client, matches on self.query_source, and uses LocalDbTransactions and client.transaction_detail) so that the subgraph client is lazily initialized only in the Subgraph match arm.crates/common/src/raindex_client/local_db/pipeline/runner/scheduler/native.rs (1)
443-653: 🧹 Nitpick | 🔵 TrivialAdd explicit readiness assertions in the updated network-loop tests.
The tests now pass
SyncReadiness, but they do not verify the new behavior at Line 225 (mark_ready(chain_id)). Please add assertions for both success and failure paths to guard routing regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/common/src/raindex_client/local_db/pipeline/runner/scheduler/native.rs` around lines 443 - 653, Tests pass a SyncReadiness into run_network_loop but never assert that mark_ready(chain_id) behavior at the new call site (mark_ready(chain_id)) is exercised; update each affected test (network_loop_runs_until_stopped, network_loop_continues_after_failure, network_loop_handles_not_leader, network_loop_handles_runner_error, network_loop_respects_pre_run_stop_flag, network_loop_respects_post_run_stop_flag) to capture the SyncReadiness instance you pass in and add assertions that verify the readiness state after the run: for success paths assert the chain is marked ready, and for failure/not-leader paths assert it is not marked ready (use the SyncReadiness inspection API such as is_ready/was_marked or equivalent to check the mark_ready(chain_id) outcome).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/common/src/raindex_client/local_db/pipeline/runner/scheduler/wasm.rs`:
- Around line 83-87: The WASM browser-test callsites for start(...) and
spawn_network_loop(...) no longer match the updated function signatures; update
the test module calls to pass the new parameters and types: when calling
start(...) supply ParsedRunnerSettings, LocalDb, an Option<Function> for
status_callback, and a SyncReadiness value (matching the new parameters named
settings, db, status_callback, sync_readiness), and similarly update
spawn_network_loop(...) invocations to the new signature introduced around the
spawn_network_loop definition (adjust argument arity/types to include the new
runner settings/db/status_callback/sync_readiness parameters). Ensure you import
or construct ParsedRunnerSettings, LocalDb, Function (or wrap in Some/None), and
SyncReadiness in the test setup so the calls compile.
In `@crates/common/src/raindex_client/local_db/state.rs`:
- Around line 109-113: The Drop impl for LocalDbState currently unconditionally
takes and stops the scheduler (scheduler.borrow_mut().take() -> handle.stop()),
which will stop the shared scheduler when any clone is dropped; change it to
only stop the scheduler when this is the final live clone by checking the shared
container's strong count (use Rc::strong_count or Arc::strong_count on the
shared scheduler/owner before taking), and only perform
scheduler.borrow_mut().take() and handle.stop() when the strong_count == 1;
update the Drop block in impl Drop for LocalDbState to reference the correct
shared pointer (scheduler) and guard the take()/stop() call with that
strong-count check.
In `@packages/orderbook/ARCHITECTURE.md`:
- Line 84: Update the RaindexClient description to include one-line explanations
for the three callback parameters: describe queryCallback as invoked with
fetched records to apply to the local DB, wipeCallback as invoked when a full
local-db sync/cleanup is required to remove stale data, and statusCallback as
used to report sync progress/errors to the caller; also note that when
local-db-sync is configured the scheduler inside RaindexClient starts
automatically and is cleaned up via Drop on shutdown. Reference RaindexClient,
queryCallback, wipeCallback and statusCallback in the updated sentence so
readers can locate the constructors and behavior easily.
In `@packages/webapp/src/routes/`+layout.ts:
- Around line 74-79: The call to RaindexClient.new is passing
localDb?.query.bind(localDb) and localDb?.wipeAndRecreate.bind(localDb) which
still throws if localDb exists but the methods are undefined; change these to
safely handle missing methods by using optional chaining on the method itself
(e.g., localDb?.query?.bind(localDb) and
localDb?.wipeAndRecreate?.bind(localDb)) or pass explicit undefined when methods
are absent, and update the test mock used for RaindexClient to include no-op
query and wipeAndRecreate functions (or provide bound stubs) so tests exercising
the missing-method path don't hit TypeError; target the RaindexClient.new
invocation and the test mock that constructs localDb to make the fixes.
---
Outside diff comments:
In
`@crates/common/src/raindex_client/local_db/pipeline/runner/scheduler/native.rs`:
- Around line 443-653: Tests pass a SyncReadiness into run_network_loop but
never assert that mark_ready(chain_id) behavior at the new call site
(mark_ready(chain_id)) is exercised; update each affected test
(network_loop_runs_until_stopped, network_loop_continues_after_failure,
network_loop_handles_not_leader, network_loop_handles_runner_error,
network_loop_respects_pre_run_stop_flag,
network_loop_respects_post_run_stop_flag) to capture the SyncReadiness instance
you pass in and add assertions that verify the readiness state after the run:
for success paths assert the chain is marked ready, and for failure/not-leader
paths assert it is not marked ready (use the SyncReadiness inspection API such
as is_ready/was_marked or equivalent to check the mark_ready(chain_id) outcome).
In `@crates/common/src/raindex_client/transactions.rs`:
- Around line 172-199: The code currently calls
self.get_orderbook_client(orderbook_address) before checking query_source, which
can fail even when QuerySource::LocalDb is selected; move the
get_orderbook_client() call into the QuerySource::Subgraph arm and only
construct the subgraph client there, leaving the QuerySource::LocalDb path to
use LocalDbTransactions::new(&local_db) with OrderbookIdentifier::new(chain_id,
orderbook_address) and return results directly without any subgraph fallback;
update the function containing this logic (the block that calls
get_orderbook_client, matches on self.query_source, and uses LocalDbTransactions
and client.transaction_detail) so that the subgraph client is lazily initialized
only in the Subgraph match arm.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
crates/common/ARCHITECTURE.mdcrates/common/src/local_db/mod.rscrates/common/src/local_db/pipeline/runner/utils.rscrates/common/src/raindex_client/add_orders.rscrates/common/src/raindex_client/local_db/mod.rscrates/common/src/raindex_client/local_db/orders.rscrates/common/src/raindex_client/local_db/pipeline/runner/scheduler/native.rscrates/common/src/raindex_client/local_db/pipeline/runner/scheduler/wasm.rscrates/common/src/raindex_client/local_db/state.rscrates/common/src/raindex_client/local_db/status.rscrates/common/src/raindex_client/local_db/vaults.rscrates/common/src/raindex_client/mod.rscrates/common/src/raindex_client/orders.rscrates/common/src/raindex_client/remove_orders.rscrates/common/src/raindex_client/trades.rscrates/common/src/raindex_client/transactions.rscrates/common/src/raindex_client/vaults.rspackages/orderbook/ARCHITECTURE.mdpackages/orderbook/README.mdpackages/orderbook/test/js_api/raindexClient.test.tspackages/webapp/ARCHITECTURE.mdpackages/webapp/src/routes/+layout.sveltepackages/webapp/src/routes/+layout.ts
💤 Files with no reviewable changes (1)
- crates/common/src/local_db/mod.rs
crates/common/src/raindex_client/local_db/pipeline/runner/scheduler/wasm.rs
Show resolved
Hide resolved
- Add SyncReadiness assertions to native/wasm scheduler tests - Fix LocalDbState Drop to only stop scheduler when last Rc owner - Move orderbook client lookup inside Subgraph branch in transactions - Use optional chaining for localDb callbacks in webapp layout - Update ARCHITECTURE.md with expanded RaindexClient docs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/common/src/raindex_client/transactions.rs (1)
177-214: 🧹 Nitpick | 🔵 TrivialAdd explicit LocalDb-path polling tests for
get_transaction.Line 177 introduces critical source-routing behavior, but this module’s visible tests still focus on subgraph polling. Please add LocalDb success and LocalDb-timeout cases to lock this path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/common/src/raindex_client/transactions.rs` around lines 177 - 214, Add unit tests exercising the LocalDb branch of the get_transaction logic: create tests named e.g. get_transaction_localdb_success and get_transaction_localdb_timeout that route QuerySource::LocalDb by constructing a RaindexClient (or the containing type) with a test/local_db and an OrderbookIdentifier/tx_hash; for success, stub or populate LocalDbTransactions::get_by_tx_hash to return Some(transaction) on the first (or Nth) attempt and assert get_transaction returns that transaction, and for timeout, have get_by_tx_hash always return None and assert the call returns RaindexError::TransactionIndexingTimeout with the expected tx_hash and attempts; ensure any sleep_ms polling is either disabled/mocked or set to a short interval so tests run fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/common/src/raindex_client/local_db/state.rs`:
- Around line 71-74: The loop over syncs currently ignores keys missing from
networks (in state.rs), which can hide config typos; update the code that
iterates over syncs.keys() (the syncs/networks/ids logic) to explicitly handle
missing network entries by emitting a warning (e.g., log::warn with the missing
sync_key and context "local-db-sync") or returning an error if you prefer
fail-fast behavior; ensure the message references the unresolved sync key and
the configuration source so callers can diagnose YAML typos and avoid silent
subgraph routing changes.
In `@packages/webapp/src/routes/`+layout.ts:
- Around line 189-191: The test uses mockRaindexClientNew.mockResolvedValue(...)
which differs from the file's pattern; change this to
mockRaindexClientNew.mockResolvedValueOnce(...) so the mock is applied for only
that invocation and matches other mocks (e.g., the one at line 175); update the
single call that sets error: { readableMsg: 'Malformed settings' } to use
mockResolvedValueOnce to maintain consistency with vi.clearAllMocks() usage in
beforeEach and other mocks in this test file.
---
Outside diff comments:
In `@crates/common/src/raindex_client/transactions.rs`:
- Around line 177-214: Add unit tests exercising the LocalDb branch of the
get_transaction logic: create tests named e.g. get_transaction_localdb_success
and get_transaction_localdb_timeout that route QuerySource::LocalDb by
constructing a RaindexClient (or the containing type) with a test/local_db and
an OrderbookIdentifier/tx_hash; for success, stub or populate
LocalDbTransactions::get_by_tx_hash to return Some(transaction) on the first (or
Nth) attempt and assert get_transaction returns that transaction, and for
timeout, have get_by_tx_hash always return None and assert the call returns
RaindexError::TransactionIndexingTimeout with the expected tx_hash and attempts;
ensure any sleep_ms polling is either disabled/mocked or set to a short interval
so tests run fast.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
crates/common/src/raindex_client/local_db/pipeline/runner/scheduler/native.rscrates/common/src/raindex_client/local_db/pipeline/runner/scheduler/wasm.rscrates/common/src/raindex_client/local_db/state.rscrates/common/src/raindex_client/transactions.rsflake.nixpackages/orderbook/ARCHITECTURE.mdpackages/webapp/src/routes/+layout.ts
Comment on lines
+71
to
+74
| for sync_key in syncs.keys() { | ||
| if let Some(network) = networks.get(sync_key) { | ||
| ids.insert(network.chain_id); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle unknown local-db-sync network keys explicitly.
Line 72 silently drops sync keys that don’t exist in networks, which can hide YAML typos and unexpectedly force subgraph routing. Please emit a warning (or fail fast) for unresolved keys.
Suggested change
use std::collections::HashSet;
+use tracing::warn;
...
let mut ids = HashSet::new();
for sync_key in syncs.keys() {
- if let Some(network) = networks.get(sync_key) {
- ids.insert(network.chain_id);
- }
+ match networks.get(sync_key) {
+ Some(network) => {
+ ids.insert(network.chain_id);
+ }
+ None => {
+ warn!(
+ network_key = %sync_key,
+ "local-db-sync entry does not match any network"
+ );
+ }
+ }
}📝 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
| for sync_key in syncs.keys() { | |
| if let Some(network) = networks.get(sync_key) { | |
| ids.insert(network.chain_id); | |
| } | |
| use std::collections::HashSet; | |
| use tracing::warn; | |
| let mut ids = HashSet::new(); | |
| for sync_key in syncs.keys() { | |
| match networks.get(sync_key) { | |
| Some(network) => { | |
| ids.insert(network.chain_id); | |
| } | |
| None => { | |
| warn!( | |
| network_key = %sync_key, | |
| "local-db-sync entry does not match any network" | |
| ); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/common/src/raindex_client/local_db/state.rs` around lines 71 - 74, The
loop over syncs currently ignores keys missing from networks (in state.rs),
which can hide config typos; update the code that iterates over syncs.keys()
(the syncs/networks/ids logic) to explicitly handle missing network entries by
emitting a warning (e.g., log::warn with the missing sync_key and context
"local-db-sync") or returning an error if you prefer fail-fast behavior; ensure
the message references the unresolved sync key and the configuration source so
callers can diagnose YAML typos and avoid silent subgraph routing changes.
Collaborator
Author
There was a problem hiding this comment.
Stray sync keys are harmless — build_runner_targets() already validates the reverse direction (every orderbook network must have a sync config). A sync key with no matching network just goes unused.
Contributor
There was a problem hiding this comment.
@findolor, thank you for the clarification! That makes sense — if build_runner_targets() already validates that every orderbook network has a corresponding sync config, then the reverse (sync keys without matching networks) is indeed harmless and just goes unused. I appreciate the context about the validation direction.
✏️ Learnings added
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2488
File: crates/common/src/raindex_client/local_db/state.rs:71-74
Timestamp: 2026-03-03T13:05:29.126Z
Learning: In `crates/common/src/raindex_client/local_db/state.rs`, the `compute_chain_ids()` method intentionally allows sync keys in the YAML that don't match any network entry; these stray keys are harmless and simply go unused. The validation is performed in the reverse direction by `build_runner_targets()`, which ensures every orderbook network must have a sync config. There's no need to warn about unmatched sync keys.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2202
File: crates/common/src/raindex_client/local_db/sync.rs:33-34
Timestamp: 2025-10-14T07:51:55.148Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the hard-coded `DEFAULT_SYNC_CHAIN_ID` constant (set to `SUPPORTED_LOCAL_DB_CHAINS[0]`) will be replaced with proper chain ID handling in downstream PRs as part of the multi-network/orderbook implementation.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2237
File: crates/common/src/raindex_client/local_db/sync.rs:79-89
Timestamp: 2025-10-18T10:38:41.273Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the sync_database method currently only supports indexing a single orderbook per chain ID, which is why `.first()` is used to select the orderbook configuration. Multi-orderbook support per chain ID is planned for future PRs.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2376
File: crates/common/src/raindex_client/orders.rs:2-2
Timestamp: 2026-01-07T14:21:44.132Z
Learning: When local DB is enabled in the rain.orderbook codebase, the intended design is to use it exclusively without falling back to the subgraph, even when results are empty. Methods like get_trades_list() in trades.rs and get_vaults_volume() in orders.rs follow this pattern—they return local DB results directly when is_chain_supported_local_db() and local_db is available. Mixing data sources could cause inconsistencies. The get_balance_changes() method in vaults.rs that checks !local_changes.is_empty() before returning is an outlier that needs to be fixed to align with the exclusive local DB pattern.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 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.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2344
File: crates/common/src/local_db/pipeline/runner/mod.rs:18-31
Timestamp: 2025-12-03T10:40:25.429Z
Learning: In `crates/common/src/local_db/pipeline/runner/mod.rs`, the `TargetSuccess` struct does not need separate `ob_id` or `orderbook_key` fields because the contained `SyncOutcome` already includes orderbook identification information such as chain_id and orderbook_address. This avoids redundant data duplication.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2145
File: crates/common/src/raindex_client/local_db/query/create_tables/query.sql:71-72
Timestamp: 2025-10-06T11:44:07.888Z
Learning: The local DB feature in the rain.orderbook codebase is not live yet (as of PR `#2145`), so schema migrations for existing databases are not required when modifying table structures in `crates/common/src/raindex_client/local_db/query/create_tables/query.sql`.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2418
File: crates/cli/src/commands/local_db/executor.rs:168-170
Timestamp: 2026-01-28T14:11:21.065Z
Learning: In the rain.orderbook codebase, the `wipe_and_recreate` method on `LocalDbQueryExecutor` is primarily needed for the browser/WASM environment (JsCallbackExecutor with sqlite-web) to handle database corruption automatically. The CLI executor (RusqliteExecutor in `crates/cli/src/commands/local_db/executor.rs`) can return `NotImplemented` for `wipe_and_recreate` because CLI users have direct file system access and can manually delete corrupted database files if needed.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 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.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2159
File: crates/cli/src/commands/local_db/sync/runner/mod.rs:52-113
Timestamp: 2025-10-06T14:41:41.909Z
Learning: The local DB sync CLI command (crates/cli/src/commands/local_db/sync/) is designed for CI-only usage, and simple println! statements are preferred over structured logging for status messages.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2159
File: crates/cli/src/commands/local_db/sync/cli.rs:37-78
Timestamp: 2025-10-06T14:39:36.425Z
Learning: In the SyncLocalDb CLI command (crates/cli/src/commands/local_db/sync/cli.rs), block range validation (e.g., start_block <= end_block) is handled internally by the sync runner, not at the CLI entry point.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1938
File: crates/settings/src/yaml/mod.rs:176-178
Timestamp: 2025-06-18T19:23:33.747Z
Learning: In crates/settings/src/yaml/mod.rs, the YamlError enum has two distinct error variants: `KeyNotFound(String)` for when a specific YAML key is not found in a hash/map, and `NotFound(String)` for when other types of entities (like networks, orderbooks, etc.) are not found in the configuration. These serve different purposes and should not be consolidated.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: crates/common/src/raindex_client/mod.rs:116-135
Timestamp: 2025-07-04T10:26:24.289Z
Learning: In crates/common/src/raindex_client/mod.rs, the get_multi_subgraph_args method intentionally treats Some(empty vector) the same as None for chain_ids parameter. Both cases should return all networks to support UI behavior where no selection or empty selection means "show all networks". Only when specific chain IDs are provided should the results be filtered.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 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.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 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.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2442
File: crates/common/src/local_db/query/fetch_vaults/mod.rs:228-244
Timestamp: 2026-02-09T12:31:02.217Z
Learning: In crates/common/src/local_db/query/fetch_vaults/mod.rs tests, whitespace-sensitive SQL string assertions (e.g., checking for "GROUP BY chain_id, orderbook_address, owner, token, vault_id\n") are acceptable for verifying query structure until a mock database testing approach is implemented. These assertions help ensure the SQL template is correctly formed even though they may be fragile to formatting changes.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2370
File: crates/common/src/raindex_client/vaults.rs:896-973
Timestamp: 2026-01-07T15:38:32.716Z
Learning: In the rainlanguage/rain.orderbook repo, prefer keeping self-contained match arms over extracting a helper function to avoid duplicating many fields when handling related subgraph types (e.g., SgDeposit, SgWithdrawal, SgClearBounty). The cost of passing 10+ fields to a helper may reduce readability and maintainability; duplicating the match arms is acceptable if it keeps the code clearer in this module. Apply this guidance broadly to Rust files in crates/common/src/raindex_client unless refactoring clearly improves structure without excessive boilerplate.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2399
File: crates/rest_api/src/routes/take_orders.rs:74-95
Timestamp: 2026-01-16T12:07:15.521Z
Learning: In Rust projects using utoipa, when you use #[schema(...)] attributes (for example #[schema(example = json!(...))]), the json! macro is handled internally by utoipa's proc-macro at compile time. You do not need to import serde_json::json! in the module for these attributes. Do not add a serde_json::json! import solely for these schema attributes; ensure serde_json is present only if used elsewhere."
Comment on lines
+189
to
191
| mockRaindexClientNew.mockResolvedValue({ | ||
| error: { readableMsg: 'Malformed settings' } | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using mockResolvedValueOnce for consistency.
This test uses mockResolvedValue while other similar mocks use mockResolvedValueOnce (e.g., line 175). While this works correctly due to vi.clearAllMocks() in beforeEach, using mockResolvedValueOnce would be more consistent with the test file's patterns.
♻️ Suggested consistency fix
- mockRaindexClientNew.mockResolvedValue({
+ mockRaindexClientNew.mockResolvedValueOnce({
error: { readableMsg: 'Malformed settings' }
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webapp/src/routes/`+layout.ts around lines 189 - 191, The test uses
mockRaindexClientNew.mockResolvedValue(...) which differs from the file's
pattern; change this to mockRaindexClientNew.mockResolvedValueOnce(...) so the
mock is applied for only that invocation and matches other mocks (e.g., the one
at line 175); update the single call that sets error: { readableMsg: 'Malformed
settings' } to use mockResolvedValueOnce to maintain consistency with
vi.clearAllMocks() usage in beforeEach and other mocks in this test file.
Collaborator
Author
There was a problem hiding this comment.
vi.clearAllMocks() in beforeEach makes this safe, and both mockRaindexClientNew calls (lines 189, 224) consistently use mockResolvedValue. No change needed.
Contributor
There was a problem hiding this comment.
@findolor, you're absolutely right! Both mockRaindexClientNew calls use mockResolvedValue consistently, and with vi.clearAllMocks() resetting state between tests, there's no functional difference. I appreciate you pointing out the internal consistency pattern—that makes sense. 👍
🐰
🧠 Learnings used
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2117
File: packages/ui-components/src/__tests__/VaultIdInformation.test.ts:9-13
Timestamp: 2025-09-02T08:04:44.814Z
Learning: In packages/ui-components/src/__tests__/VaultIdInformation.test.ts and similar test files in the rain.orderbook project, the passthrough vi.mock('rainlanguage/orderbook', async (importOriginal) => { return { ...(await importOriginal()) }; }); block is required for tests to run properly, even when not overriding any exports. This is needed due to the specific Vitest configuration or test environment setup in the project.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1936
File: packages/ui-components/vite.config.ts:21-23
Timestamp: 2025-06-17T14:55:22.914Z
Learning: In the rain.orderbook project, the Vite configuration uses `'import.meta.vitest': 'undefined'` (as a string) combined with conditional `if (import.meta.vitest)` checks for in-source testing. The mock files are excluded from test execution using `exclude: ['src/lib/__mocks__/**/*.ts']`. This configuration successfully allows dev builds to work without `vi` undefined errors, despite the theoretical expectation that the string "undefined" would be truthy and cause issues.
Learnt from: brusherru
Repo: rainlanguage/rain.orderbook PR: 2083
File: packages/ui-components/src/__tests__/VaultsListTable.test.ts:16-19
Timestamp: 2025-08-14T18:29:32.933Z
Learning: In the rain.orderbook project's UI components tests, mocking hooks like useToasts is often required as infrastructure even when not directly asserting on their calls, because components internally depend on these hooks. Removing such mocks would break component rendering in tests and require more complex test setup with providers.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2298
File: packages/webapp/src/lib/constants.ts:3-4
Timestamp: 2025-11-06T12:08:39.643Z
Learning: In the rain.orderbook repository, the REMOTE_SETTINGS_URL constant in packages/webapp/src/lib/constants.ts and tauri-app/src/lib/services/loadRemoteSettings.ts should remain duplicated rather than being consolidated into a single import, as this duplication is intentional.
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1565
File: packages/webapp/src/__tests__/registryManager.test.ts:227-238
Timestamp: 2025-04-09T13:00:24.640Z
Learning: In the rain.orderbook project, error handling was added to the RegistryManager to handle localStorage exceptions, addressing part of the verification agent's suggestions. The team should also consider testing extremely long URLs that might approach browser limits.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1891
File: packages/webapp/src/routes/deploy/[strategyName]/[deploymentKey]/page.test.ts:66-80
Timestamp: 2025-06-08T18:43:51.842Z
Learning: In the rain.orderbook webapp test files, when mocking objects like the transaction manager, it's acceptable to use simple empty objects with ts-expect-error comments rather than providing complete mock implementations with all properties and methods.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1956
File: packages/ui-components/src/__tests__/VaultDetail.test.ts:177-181
Timestamp: 2025-07-04T09:14:35.748Z
Learning: In the rain.orderbook project, maintainer findolor is fine with mutating mock data directly in test files using ts-expect-error comments, preferring this pragmatic approach over immutable patterns like spread operators for test data updates.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1974
File: packages/ui-components/src/__tests__/DeploymentSteps.test.ts:123-126
Timestamp: 2025-07-09T12:35:45.699Z
Learning: In packages/ui-components/src/__tests__/DeploymentSteps.test.ts, findolor prefers to keep mock initializations (like setSelectToken) in individual test cases rather than consolidating them into shared beforeEach blocks, even when it results in duplication.
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1559
File: packages/ui-components/src/__tests__/OrderOrVaultHash.test.ts:94-94
Timestamp: 2025-04-04T11:25:21.518Z
Learning: In the rain.orderbook project, minimal test fixtures are preferred over complete mocks that implement the entire interface. Type casting (e.g., `as unknown as SgVault`) is an acceptable approach to maintain both minimal fixtures and TypeScript type compatibility.
Learnt from: 0xgleb
Repo: rainlanguage/rain.orderbook PR: 1911
File: crates/subgraph/src/vault_balance_changes_query.rs:133-135
Timestamp: 2025-07-23T13:03:40.669Z
Learning: User 0xgleb prefers not to make changes to test code when tests are already passing, even if the code format could be standardized differently. Working test code should be left alone unless there's a functional issue.
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1565
File: packages/webapp/src/routes/deploy/layout.test.ts:12-29
Timestamp: 2025-04-07T08:18:36.473Z
Learning: In test files for this project, hardingjam prefers to use custom mocks (such as for localStorage) rather than relying on environment-provided implementations, as this allows for spying on individual methods and having precise control over implementation details for more robust testing.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1926
File: packages/ui-components/src/lib/__mocks__/stores.ts:13-17
Timestamp: 2025-06-30T14:17:16.626Z
Learning: User findolor reports that vi.mock(import('rainlanguage/orderbook'), async (importOriginal) => { ... }) syntax works in their testing environment, despite official Vitest documentation indicating the first argument should be a string. This suggests there may be specific Vitest versions or configurations that support dynamic import() as the first argument to vi.mock().
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 1996
File: packages/ui-components/src/__tests__/VaultIdInformation.test.ts:6-6
Timestamp: 2025-07-17T10:36:02.846Z
Learning: In packages/ui-components/src/__tests__/VaultIdInformation.test.ts, findolor prefers to keep exported type aliases like `VaultIdInformationComponentProps` in test files, even when static analysis tools flag this as discouraged practice.
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1615
File: packages/webapp/src/routes/deploy/+layout.ts:37-56
Timestamp: 2025-04-09T13:13:48.419Z
Learning: For test fixtures in the Rain codebase, it's acceptable to use type casting (e.g., `as unknown as SomeType[]`) for minimal test fixtures rather than creating fully-populated mock objects, especially when the internal structure of the mock data isn't critical to the tests.
Learnt from: hardingjam
Repo: rainlanguage/rain.orderbook PR: 1597
File: packages/ui-components/src/__tests__/OrderDetail.test.ts:120-120
Timestamp: 2025-04-08T09:18:46.653Z
Learning: In test files for the Rain Orderbook project, it's acceptable to bypass TypeScript's strict typing using constructs like `as unknown as [Type]` to create simplified mock objects that don't need to implement the entire interface.
Learnt from: rouzwelt
Repo: rainlanguage/rain.orderbook PR: 2246
File: packages/webapp/src/hooks.client.ts:2-2
Timestamp: 2025-10-19T20:24:08.629Z
Learning: In the rain.orderbook repository, the import `rainlanguage/orderbook/esm` in packages/webapp/src/hooks.client.ts is intentional and should not be changed to the root package path, as it's specific to the webapp build configuration.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2278
File: packages/webapp/src/lib/services/startLocalDbSync.ts:23-29
Timestamp: 2025-10-29T13:29:29.578Z
Learning: In packages/webapp/src/lib/services/startLocalDbSync.ts, the hard-coded default orderbook address '0x2f209e5b67A33B8fE96E28f24628dF6Da301c8eB' is a temporary placeholder that will be removed when new sync logic is introduced in the Raindex client, as documented in the NOTE comment.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2202
File: crates/common/src/raindex_client/local_db/sync.rs:33-34
Timestamp: 2025-10-14T07:51:55.148Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the hard-coded `DEFAULT_SYNC_CHAIN_ID` constant (set to `SUPPORTED_LOCAL_DB_CHAINS[0]`) will be replaced with proper chain ID handling in downstream PRs as part of the multi-network/orderbook implementation.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2376
File: crates/common/src/raindex_client/orders.rs:2-2
Timestamp: 2026-01-07T14:21:44.132Z
Learning: When local DB is enabled in the rain.orderbook codebase, the intended design is to use it exclusively without falling back to the subgraph, even when results are empty. Methods like get_trades_list() in trades.rs and get_vaults_volume() in orders.rs follow this pattern—they return local DB results directly when is_chain_supported_local_db() and local_db is available. Mixing data sources could cause inconsistencies. The get_balance_changes() method in vaults.rs that checks !local_changes.is_empty() before returning is an outlier that needs to be fixed to align with the exclusive local DB pattern.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2145
File: crates/common/src/raindex_client/local_db/query/create_tables/query.sql:71-72
Timestamp: 2025-10-06T11:44:07.888Z
Learning: The local DB feature in the rain.orderbook codebase is not live yet (as of PR `#2145`), so schema migrations for existing databases are not required when modifying table structures in `crates/common/src/raindex_client/local_db/query/create_tables/query.sql`.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 2237
File: crates/common/src/raindex_client/local_db/sync.rs:79-89
Timestamp: 2025-10-18T10:38:41.273Z
Learning: In `crates/common/src/raindex_client/local_db/sync.rs`, the sync_database method currently only supports indexing a single orderbook per chain ID, which is why `.first()` is used to select the orderbook configuration. Multi-orderbook support per chain ID is planned for future PRs.
Learnt from: findolor
Repo: rainlanguage/rain.orderbook PR: 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.
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
1 participant
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
RaindexClient previously required a multi-step initialization: construct the client, wire in the local DB callback, then start the scheduler. Data source routing relied on a hardcoded list of supported chains and a fallback pattern (try local DB, fall through to subgraph). This made the lifecycle error-prone and routing behavior unpredictable.
Solution
Consolidates local DB setup into a single async constructor (
RaindexClient::create, aliased tonewin JS) that accepts all DB/status callbacks upfront and automatically starts the scheduler whenlocal-db-syncis configured in the YAML.new()+set_local_db_callback()+start_local_db_scheduler()pattern. Scheduler stops viaDrop.QuerySource::LocalDborQuerySource::Subgraph) based on three conditions: DB exists, chain is configured in YAML, and chain has completed at least one sync cycle (SyncReadiness).SUPPORTED_LOCAL_DB_CHAINSconstant. Supported chains are derived from the YAMLlocal-db-syncconfiguration.LocalDbStatestruct: EncapsulatesLocalDb, scheduler handle,SyncReadiness, and configured chain IDs.local_db/mod.rsintostate.rs(runtime state, query routing) andstatus.rs(UI status types), each with tests.crates/common,packages/orderbook, andpackages/webapp.Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Documentation