Shared RaindexClient: eliminate per-request thread spawning#58
Merged
Shared RaindexClient: eliminate per-request thread spawning#58
Conversation
RaindexClient is now Send+Sync on native (uses Arc<Mutex<...>> internally) and new_native() starts the sync scheduler internally.
Replace per-request thread spawning with a single client created at startup. Thread spawning is kept only for construction (DotrainRegistry future is not Send due to JsValue). Sync is now handled internally by RaindexClient::new_native(), so sync.rs is gutted.
Change async_trait(?Send) to async_trait and add Send + Sync bounds on OrderDataSource and SwapDataSource traits, since RaindexClient is now Send+Sync and handlers no longer need non-Send futures.
Replace run_with_client pattern with direct raindex.client() access in all route handlers. Remove per-request thread spawning overhead.
Remove tests for per-request client init failures (no longer possible since client is created eagerly at startup). Update mock helpers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRaindexProvider now initializes and owns a RaindexClient via load(registry_url, Option); run_with_client and set_db_path/start_sync helpers were removed. Routes and data-source traits switched to use provider.client() directly and now require Send + Sync; several handlers replaced with todo!() placeholders. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "HTTP Route Handler"
participant Provider as "RaindexProvider"
participant RaindexClient as "RaindexClient / Orderbook"
rect rgba(200,230,255,0.5)
Client->>Provider: read State / get reference
Provider->>Provider: .client() (owned client accessor)
end
rect rgba(200,255,200,0.5)
Client->>RaindexClient: call process_* (e.g., process_get_order)
RaindexClient-->>Client: result / error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 237-242: Before calling raindex::RaindexProvider::load (the block
that assigns raindex_config), validate local_db_path: ensure it is either an
absolute path or includes a directory component (i.e.,
Path::new(&local_db_path).is_absolute() ||
Path::new(&local_db_path).parent().and_then(|p| p.to_str()).filter(|s|
!s.is_empty()).is_some()). If the check fails (bare filename like "local.db"),
return or propagate an error / log and exit prior to invoking
RaindexProvider::load so the code does not forward invalid paths; reference the
raindex_config assignment, the local_db_path variable, and
raindex::RaindexProvider::load when adding the guard.
In `@src/raindex/config.rs`:
- Around line 14-20: The load function should reject bare filenames for db_path
at load-time: in pub(crate) async fn load(...), if let Some(db) =
db_path.clone() { check db.parent() and ensure it is Some and not empty (i.e.,
path has a directory prefix like "./data/local.db" or "db/local.db"); if parent
is None or empty, return an Err(RaindexProviderError::InvalidConfig(...)) (or
add a new RaindexProviderError variant) with a clear message so consumers cannot
pass bare "local.db". Implement this validation early in load before using db.
In `@src/routes/order/deploy_dca.rs`:
- Around line 34-35: The handler currently calls todo!() after acquiring
_raindex (shared_raindex.read().await) which panics and bypasses the ApiError
contract; replace the todo!() with proper non-panicking error handling that
returns a Result with an ApiError variant (e.g.,
Err(ApiError::InternalServerError or a more specific ApiError::BadRequest)
rather than panicking), ensure the function (deploy_dca handler) returns the
expected Result type, and propagate or map any errors from shared_raindex access
or subsequent logic into ApiError so all failures go through the ApiError enum.
In `@src/routes/order/deploy_solver.rs`:
- Around line 34-35: The handler currently calls todo!() (after awaiting
shared_raindex.read().await) which panics on every /v1/order/solver request;
replace the panic with a proper ApiError return so the error flows through typed
API error handling. Specifically, remove todo!() and return an
Err(ApiError::...) from the handler (e.g., ApiError::Internal or a more specific
variant like ServiceUnavailable) with a clear message such as "solver endpoint
not implemented" and include any context you want from shared_raindex; ensure
the function signature (the route handler that awaited
shared_raindex.read().await) still returns a Result so the Err variant is used
instead of panicking.
In `@src/routes/orders.rs`:
- Around line 37-38: The handlers currently call todo!() after acquiring the
read lock on shared_raindex (let _raindex = shared_raindex.read().await) which
panics and breaks the endpoints; replace todo!() in both order handlers with
real logic that uses the acquired read guard (use the variable name e.g.,
raindex) to perform the lookup/operation and return Result<..., ApiError>,
mapping any failure to the appropriate ApiError variant instead of panicking or
returning raw status codes; ensure all error paths convert errors into ApiError
(via ? or map_err) and that the handler signatures return the ApiError-based
Response type so the endpoints return typed API errors rather than panics.
In `@src/routes/trades.rs`:
- Around line 37-38: The handlers for /v1/trades/tx/{tx_hash} and
/v1/trades/{address} currently call todo!() and thus panic; replace the todo!()
in the blocks that acquire shared_raindex.read().await with real logic that
queries the raindex and returns results or an ApiError variant instead of
panicking. Specifically, in the function(s) handling tx_hash and address use the
read guard from shared_raindex to perform the lookup, map a missing result to
the appropriate ApiError (e.g., ApiError::NotFound or the project's canonical
error variant) and map any internal errors to ApiError::Internal or
ApiError::ServiceUnavailable as appropriate, then return Ok(response) on
success; ensure every failure path returns Err(ApiError::...) so no raw status
codes or panics remain.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
lib/rain.orderbooksrc/main.rssrc/raindex/config.rssrc/routes/admin.rssrc/routes/order/cancel.rssrc/routes/order/deploy_dca.rssrc/routes/order/deploy_solver.rssrc/routes/order/get_order.rssrc/routes/order/mod.rssrc/routes/orders.rssrc/routes/swap/calldata.rssrc/routes/swap/mod.rssrc/routes/swap/quote.rssrc/routes/tests.rssrc/routes/trades.rssrc/sync.rssrc/test_helpers.rs
Comment on lines
+14
to
+20
| pub(crate) async fn load( | ||
| registry_url: &str, | ||
| db_path: Option<PathBuf>, | ||
| ) -> Result<Self, RaindexProviderError> { | ||
| let url = registry_url.to_string(); | ||
| let (tx, rx) = tokio::sync::oneshot::channel::<Result<DotrainRegistry, WorkerError>>(); | ||
| let db = db_path.clone(); | ||
|
|
There was a problem hiding this comment.
Validate db_path format at load-time.
Line [16] currently accepts bare filenames (for example local.db), which breaks this repo’s configuration-path convention.
Suggested validation guard
impl RaindexProvider {
pub(crate) async fn load(
registry_url: &str,
db_path: Option<PathBuf>,
) -> Result<Self, RaindexProviderError> {
+ if let Some(path) = db_path.as_ref() {
+ let has_parent_dir = path
+ .parent()
+ .is_some_and(|parent| !parent.as_os_str().is_empty());
+ if !has_parent_dir {
+ return Err(RaindexProviderError::ClientInit(
+ "db_path must include a directory prefix (e.g., data/raindex.db)".into(),
+ ));
+ }
+ }
+
let url = registry_url.to_string();
let db = db_path.clone();Based on learnings: "In ST0x-Technology/st0x.rest.api, Rust source files that deal with configuration should specify database paths with a directory prefix ... never use bare filenames like local.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
| pub(crate) async fn load( | |
| registry_url: &str, | |
| db_path: Option<PathBuf>, | |
| ) -> Result<Self, RaindexProviderError> { | |
| let url = registry_url.to_string(); | |
| let (tx, rx) = tokio::sync::oneshot::channel::<Result<DotrainRegistry, WorkerError>>(); | |
| let db = db_path.clone(); | |
| pub(crate) async fn load( | |
| registry_url: &str, | |
| db_path: Option<PathBuf>, | |
| ) -> Result<Self, RaindexProviderError> { | |
| if let Some(path) = db_path.as_ref() { | |
| let has_parent_dir = path | |
| .parent() | |
| .is_some_and(|parent| !parent.as_os_str().is_empty()); | |
| if !has_parent_dir { | |
| return Err(RaindexProviderError::ClientInit( | |
| "db_path must include a directory prefix (e.g., data/raindex.db)".into(), | |
| )); | |
| } | |
| } | |
| let url = registry_url.to_string(); | |
| let db = db_path.clone(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/raindex/config.rs` around lines 14 - 20, The load function should reject
bare filenames for db_path at load-time: in pub(crate) async fn load(...), if
let Some(db) = db_path.clone() { check db.parent() and ensure it is Some and not
empty (i.e., path has a directory prefix like "./data/local.db" or
"db/local.db"); if parent is None or empty, return an
Err(RaindexProviderError::InvalidConfig(...)) (or add a new RaindexProviderError
variant) with a clear message so consumers cannot pass bare "local.db".
Implement this validation early in load before using db.
Collaborator
Author
There was a problem hiding this comment.
Not addressing this. db_path is Option<PathBuf> — None is a valid case (in-memory), and for Some, path validity is naturally validated downstream by the SQLite layer. Adding an arbitrary "must have a directory prefix" rule would silently break valid absolute paths with no parent component and adds no real safety.
There was a problem hiding this comment.
@findolor, understood — those are all valid points. None for in-memory is a legitimate case, and relying on the SQLite layer for path validation at Some avoids duplicating enforcement with an overly narrow rule. I'll stand down on this.
I'll also remove the learning I applied here, since it was scoped to configuration file conventions (constructing paths in main.rs) and shouldn't have been extended to this API-boundary parameter.
⛔ Learnings removed
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 56
File: src/main.rs:239-247
Timestamp: 2026-03-02T09:02:05.104Z
Learning: In ST0x-Technology/st0x.rest.api, Rust source files that deal with configuration should specify database paths with a directory prefix (e.g., "data/raindex.db" or an absolute path like "/mnt/data/st0x-rest-api/raindex.db"), never use bare filenames like "local.db". This convention helps ensure the path is explicit and portable. Enforce this when reading or validating config paths in Rust code (e.g., parsing configuration and constructing paths) and update any tests or documentation accordingly.
🧠 Learnings used
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 56
File: src/main.rs:239-247
Timestamp: 2026-03-02T09:02:05.104Z
Learning: In ST0x-Technology/st0x.rest.api, Rust source files that deal with configuration should specify database paths with a directory prefix (e.g., "data/raindex.db" or an absolute path like "/mnt/data/st0x-rest-api/raindex.db"), never use bare filenames like "local.db". This convention helps ensure the path is explicit and portable. Enforce this when reading or validating config paths in Rust code (e.g., parsing configuration and constructing paths) and update any tests or documentation accordingly.
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 18
File: src/fairings/rate_limiter.rs:158-203
Timestamp: 2026-02-13T09:37:37.357Z
Learning: When multiple guards or layers write to the same cached rate-limit information, document and enforce the execution order so that the client-visible headers reflect the intended quota (global vs per-key). In this repo, rate-limiter.rs and auth-related guards intentionally write to the same CachedRateLimitInfo entry; ensure the per-key quota is what’s surfaced to clients and that the order is explicit in route signatures. Include tests that verify the final headers reflect the intended quota and that writes are intentional and auditable.
Comment on lines
+34
to
+35
| let _raindex = shared_raindex.read().await; | ||
| todo!() |
There was a problem hiding this comment.
Replace todo!() in this handler before merge.
Line [35] unconditionally panics, so /v1/order/dca is effectively broken and bypasses the ApiError contract.
Immediate non-panicking fallback
- let _raindex = shared_raindex.read().await;
- todo!()
+ let _raindex = shared_raindex.read().await;
+ tracing::error!("DCA deploy handler is not implemented");
+ Err(ApiError::Internal(
+ "dca deployment is not implemented".into(),
+ ))As per coding guidelines: "All API errors must go through the ApiError enum, never return raw status codes."
📝 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 _raindex = shared_raindex.read().await; | |
| todo!() | |
| let _raindex = shared_raindex.read().await; | |
| tracing::error!("DCA deploy handler is not implemented"); | |
| Err(ApiError::Internal( | |
| "dca deployment is not implemented".into(), | |
| )) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/order/deploy_dca.rs` around lines 34 - 35, The handler currently
calls todo!() after acquiring _raindex (shared_raindex.read().await) which
panics and bypasses the ApiError contract; replace the todo!() with proper
non-panicking error handling that returns a Result with an ApiError variant
(e.g., Err(ApiError::InternalServerError or a more specific
ApiError::BadRequest) rather than panicking), ensure the function (deploy_dca
handler) returns the expected Result type, and propagate or map any errors from
shared_raindex access or subsequent logic into ApiError so all failures go
through the ApiError enum.
Collaborator
Author
There was a problem hiding this comment.
Valid. The todo!() will be replaced before merge — this is an intentional stub for this PR. The actual implementation is tracked separately.
Comment on lines
+34
to
+35
| let _raindex = shared_raindex.read().await; | ||
| todo!() |
There was a problem hiding this comment.
todo!() in the live request path is a blocker.
Line [35] panics on every /v1/order/solver request and bypasses typed API error handling.
Immediate non-panicking fallback
- let _raindex = shared_raindex.read().await;
- todo!()
+ let _raindex = shared_raindex.read().await;
+ tracing::error!("Solver deploy handler is not implemented");
+ Err(ApiError::Internal(
+ "solver deployment is not implemented".into(),
+ ))As per coding guidelines: "All API errors must go through the ApiError enum, never return raw status codes."
📝 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 _raindex = shared_raindex.read().await; | |
| todo!() | |
| let _raindex = shared_raindex.read().await; | |
| tracing::error!("Solver deploy handler is not implemented"); | |
| Err(ApiError::Internal( | |
| "solver deployment is not implemented".into(), | |
| )) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/order/deploy_solver.rs` around lines 34 - 35, The handler
currently calls todo!() (after awaiting shared_raindex.read().await) which
panics on every /v1/order/solver request; replace the panic with a proper
ApiError return so the error flows through typed API error handling.
Specifically, remove todo!() and return an Err(ApiError::...) from the handler
(e.g., ApiError::Internal or a more specific variant like ServiceUnavailable)
with a clear message such as "solver endpoint not implemented" and include any
context you want from shared_raindex; ensure the function signature (the route
handler that awaited shared_raindex.read().await) still returns a Result so the
Err variant is used instead of panicking.
Collaborator
Author
There was a problem hiding this comment.
Valid. Same as the DCA handler — intentional stub, will be replaced before merge.
There was a problem hiding this comment.
@findolor, understood — thanks for the context. Since both deploy_solver.rs and deploy_dca.rs are intentional stubs pending implementation, it may be worth tracking them (e.g., via a linked issue or a // TODO(#<issue>): comment) to ensure neither slips through before the final merge. That said, happy to leave the call to you.
🧠 Learnings used
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 18
File: src/fairings/rate_limiter.rs:158-203
Timestamp: 2026-02-13T09:37:37.357Z
Learning: When multiple guards or layers write to the same cached rate-limit information, document and enforce the execution order so that the client-visible headers reflect the intended quota (global vs per-key). In this repo, rate-limiter.rs and auth-related guards intentionally write to the same CachedRateLimitInfo entry; ensure the per-key quota is what’s surfaced to clients and that the order is explicit in route signatures. Include tests that verify the final headers reflect the intended quota and that writes are intentional and auditable.
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 38
File: src/routes/swap/mod.rs:115-129
Timestamp: 2026-02-17T09:35:57.431Z
Learning: In Rust server code for swap endpoints, if the response contains a non-empty approvals array, the behavior uses a two-phase flow: first the client sends an approval transaction, then queries the endpoint again to obtain the actual swap calldata. In the approval response, set to = approval.spender() to signal the eventual target, and set data = Bytes::new() (empty) to indicate 'no transaction yet.' Prefer this approach over using Address::ZERO for clarity. This pattern is specific to the swap flow in src/routes/swap/mod.rs and should be followed in similar swap-related endpoints with analogous approval semantics.
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 52
File: src/routes/orders/get_by_token.rs:116-123
Timestamp: 2026-02-25T12:18:58.001Z
Learning: Adopt a centralized error-logging pattern in Rust REST handlers: route handlers should log a general message (e.g., 'request received') and key decisions, but should not duplicate error logging at the boundary. Lower-level data-access implementations (e.g., OrdersListDataSource) should log errors at their origin with tracing::error! before returning an ApiError. In handlers, propagate errors using map_err(ApiError::from) or equivalent, e.g., ... .map_err(ApiError::from) or ?? ; the key is to avoid re-logging the same error at the boundary. This pattern should apply across src/routes/orders/, src/routes/order/, and src/routes/swap/, including get_by_token.rs and similar files.
Comment on lines
+37
to
+38
| let _raindex = shared_raindex.read().await; | ||
| todo!() |
There was a problem hiding this comment.
todo!() in both orders handlers is a blocker.
Lines [38] and [73] panic on every request, so both endpoints are effectively down and no longer return typed API errors.
As per coding guidelines: "All API errors must go through the ApiError enum, never return raw status codes."
Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/orders.rs` around lines 37 - 38, The handlers currently call
todo!() after acquiring the read lock on shared_raindex (let _raindex =
shared_raindex.read().await) which panics and breaks the endpoints; replace
todo!() in both order handlers with real logic that uses the acquired read guard
(use the variable name e.g., raindex) to perform the lookup/operation and return
Result<..., ApiError>, mapping any failure to the appropriate ApiError variant
instead of panicking or returning raw status codes; ensure all error paths
convert errors into ApiError (via ? or map_err) and that the handler signatures
return the ApiError-based Response type so the endpoints return typed API errors
rather than panics.
Collaborator
Author
There was a problem hiding this comment.
Valid. Both handlers are stubs pending implementation. Will be addressed before merge.
There was a problem hiding this comment.
@findolor, understood — thanks for confirming. I'll keep the comment open until the implementations land. When both handlers are wired up, please make sure each error path flows through an appropriate ApiError variant (via ? or map_err) per the existing pattern in the codebase.
🧠 Learnings used
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 52
File: src/routes/orders/get_by_token.rs:116-123
Timestamp: 2026-02-25T12:19:05.029Z
Learning: In the st0x.rest.api Rust codebase, the error-logging pattern is: route handlers log "request received" and key decisions, while lower-level data-access functions (like OrdersListDataSource implementations) log errors at their origin using tracing::error! before returning ApiError. Handlers should NOT duplicate error logging at the boundary; they propagate errors with map_err(ApiError::from)? or similar. This pattern is used consistently across handlers in src/routes/orders/, src/routes/order/, and src/routes/swap/.
Learnt from: CR
Repo: ST0x-Technology/st0x.rest.api PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-19T09:28:42.183Z
Learning: Applies to **/*.rs : Never use `expect` or `unwrap` in production code; handle errors gracefully or exit with a message
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 18
File: src/fairings/rate_limiter.rs:158-203
Timestamp: 2026-02-13T09:37:37.357Z
Learning: When multiple guards or layers write to the same cached rate-limit information, document and enforce the execution order so that the client-visible headers reflect the intended quota (global vs per-key). In this repo, rate-limiter.rs and auth-related guards intentionally write to the same CachedRateLimitInfo entry; ensure the per-key quota is what’s surfaced to clients and that the order is explicit in route signatures. Include tests that verify the final headers reflect the intended quota and that writes are intentional and auditable.
Learnt from: findolor
Repo: ST0x-Technology/st0x.rest.api PR: 38
File: src/routes/swap/mod.rs:115-129
Timestamp: 2026-02-17T09:35:57.431Z
Learning: In Rust server code for swap endpoints, if the response contains a non-empty approvals array, the behavior uses a two-phase flow: first the client sends an approval transaction, then queries the endpoint again to obtain the actual swap calldata. In the approval response, set to = approval.spender() to signal the eventual target, and set data = Bytes::new() (empty) to indicate 'no transaction yet.' Prefer this approach over using Address::ZERO for clarity. This pattern is specific to the swap flow in src/routes/swap/mod.rs and should be followed in similar swap-related endpoints with analogous approval semantics.
Comment on lines
+37
to
+38
| let _raindex = shared_raindex.read().await; | ||
| todo!() |
There was a problem hiding this comment.
Both trades handlers panic unconditionally (todo!()), which is a release blocker.
Lines [38] and [73] make /v1/trades/tx/{tx_hash} and /v1/trades/{address} non-functional and bypass ApiError handling.
As per coding guidelines: "All API errors must go through the ApiError enum, never return raw status codes."
Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/trades.rs` around lines 37 - 38, The handlers for
/v1/trades/tx/{tx_hash} and /v1/trades/{address} currently call todo!() and thus
panic; replace the todo!() in the blocks that acquire
shared_raindex.read().await with real logic that queries the raindex and returns
results or an ApiError variant instead of panicking. Specifically, in the
function(s) handling tx_hash and address use the read guard from shared_raindex
to perform the lookup, map a missing result to the appropriate ApiError (e.g.,
ApiError::NotFound or the project's canonical error variant) and map any
internal errors to ApiError::Internal or ApiError::ServiceUnavailable as
appropriate, then return Ok(response) on success; ensure every failure path
returns Err(ApiError::...) so no raw status codes or panics remain.
Collaborator
Author
There was a problem hiding this comment.
Valid. Same — intentional stubs, will be replaced before merge.
hardyjosh
approved these changes
Mar 5, 2026
Update method call from get_raindex_client_native to get_raindex_client to match the renamed API in the new submodule version.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/raindex/config.rs (1)
142-155: Consider adding coverage forWorkerPanickedvariant.The error mapping test covers
RegistryLoadandClientInit, but skipsWorkerPanicked. While this variant is harder to trigger in a unit test (requires the spawned thread to panic), adding a direct test for itsApiErrormapping would complete the coverage.Optional test addition
let api_err: ApiError = err.into(); assert!( matches!(api_err, ApiError::Internal(msg) if msg == "failed to initialize orderbook client") ); + + let err = RaindexProviderError::WorkerPanicked; + let api_err: ApiError = err.into(); + assert!( + matches!(api_err, ApiError::Internal(msg) if msg == "failed to initialize client runtime") + ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/raindex/config.rs` around lines 142 - 155, The test test_error_maps_to_api_error is missing coverage for the RaindexProviderError::WorkerPanicked variant; add an assertion that converting RaindexProviderError::WorkerPanicked("...".into()) into ApiError yields the expected ApiError::Internal message (e.g., matches!(api_err, ApiError::Internal(msg) if msg == "worker panicked") or whatever the mapping uses). Update the test_error_maps_to_api_error function to construct RaindexProviderError::WorkerPanicked, call .into() to get an ApiError, and assert the pattern and message just like the existing RegistryLoad and ClientInit checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/raindex/config.rs`:
- Around line 142-155: The test test_error_maps_to_api_error is missing coverage
for the RaindexProviderError::WorkerPanicked variant; add an assertion that
converting RaindexProviderError::WorkerPanicked("...".into()) into ApiError
yields the expected ApiError::Internal message (e.g., matches!(api_err,
ApiError::Internal(msg) if msg == "worker panicked") or whatever the mapping
uses). Update the test_error_maps_to_api_error function to construct
RaindexProviderError::WorkerPanicked, call .into() to get an ApiError, and
assert the pattern and message just like the existing RegistryLoad and
ClientInit checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 669ae99d-752f-47e2-8548-c80423b02ee5
📒 Files selected for processing (2)
lib/rain.orderbooksrc/raindex/config.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/rain.orderbook
findolor
added a commit
that referenced
this pull request
Mar 5, 2026
Remove stale tracing dep from rain_orderbook_subgraph_client that was incorrectly added from a dirty submodule during PR #58.
4 tasks
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
2 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.
Dependent PRs
Motivation
Every API request previously spawned a new OS thread + Tokio runtime to create a fresh
RaindexClientviarun_with_client(). This was necessary because the oldRaindexClientusedRc/RefCellinternally and wasn'tSend+Sync.The upstream rain.orderbook now makes
RaindexClientSend+Syncon native (usesArc<Mutex<...>>), andnew_native()starts the sync scheduler internally. This lets us create a single shared client at startup and pass it to all handlers via Rocket state.Solution
5c78970c25which provides Send+SyncRaindexClientRaindexProviderto hold a sharedRaindexClientcreated at startup. Thread spawning is kept only for construction (DotrainRegistry future is not Send due to JsValue)sync.rs— sync is now handled internally byRaindexClient::new_native()Send+Sync— changeasync_trait(?Send)toasync_traitonOrderDataSourceandSwapDataSourcerun_with_clientpattern with directraindex.client()accessNet result: -401 lines, +115 lines across 18 files. Local benchmarks show ~2s response times (quote/calldata), consistent with local DB performance.
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Refactor
Chores
Behavior