Add ok/err type for oracle response#6
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 2 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughBatch request handling now accumulates per-request results instead of failing fast. Responses use a tagged Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as Request Handler
participant Resolver as Pair Resolver
participant Builder as Response Builder
Client->>Handler: Batch request (Vec<orders>)
Note over Handler: For each order in batch
Handler->>Resolver: resolve_pair_for_order(order)
alt resolved
Resolver-->>Handler: Ok(ResolvedPair)
Handler->>Builder: build_response_from_quote(ResolvedPair)
alt built ok
Builder-->>Handler: OracleResponse::Ok(OracleOkResponse)
else built err
Builder-->>Handler: OracleResponse::Err(OracleErrResponse)
end
else resolution failed
Resolver-->>Handler: Err(OracleErrResponse)
Handler-->>Handler: OracleResponse::Err(OracleErrResponse)
end
Note over Handler: Accumulate Vec<OracleResponse> (mix Ok/Err)
Handler->>Client: HTTP 200 with Vec<OracleResponse>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (2)
src/lib.rs (1)
145-148: Use.map()instead of.and_then(|p| Some(...)).The
and_thenclosure that wraps inSomeis equivalent tomap.♻️ Proposed fix
let needed_symbols: Vec<&str> = resolved .iter() - .filter_map(|(_, res)| res.as_ref().ok().and_then(|p| Some(p.symbol.as_str()))) + .filter_map(|(_, res)| res.as_ref().ok().map(|p| p.symbol.as_str())) .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 145 - 148, The closure in the computation of needed_symbols uses and_then(|p| Some(p.symbol.as_str())), which is equivalent to map; update the iterator chain that builds needed_symbols (the resolved.iter().filter_map(...) expression) to replace and_then(|p| Some(p.symbol.as_str())) with map(|p| p.symbol.as_str()) so the intent is clearer and removes the unnecessary Some wrapper.tests/smoke_prod.rs (1)
123-146: Consider extracting unwrapped results to reduce repetition.The same
as_result().unwrap()call is made multiple times on the same response objects. While acceptable in test code, extracting to local variables would improve readability.♻️ Optional refactor
assert_eq!(responses.len(), 2, "batch → length-2 array"); - assert_eq!(responses[0].as_result().unwrap().context.len(), 3); - assert_eq!(responses[1].as_result().unwrap().context.len(), 3); + let r0 = responses[0].as_result().unwrap(); + let r1 = responses[1].as_result().unwrap(); + assert_eq!(r0.context.len(), 3); + assert_eq!(r1.context.len(), 3); // The two responses should carry the SAME publish_time because both // resolve to COIN and read the same cache entry. assert_eq!( - responses[0].as_result().unwrap().context[2], - responses[1].as_result().unwrap().context[2], + r0.context[2], + r1.context[2], "both orders resolve to COIN and should share publish_time" ); // And the second should be the inverse (1/bid) of the first side. - let buy = Float::from(alloy::primitives::B256::from( - responses[0].as_result().unwrap().context[1], - )) - .format() - .unwrap(); - let sell = Float::from(alloy::primitives::B256::from( - responses[1].as_result().unwrap().context[1], - )) - .format() - .unwrap(); + let buy = Float::from(alloy::primitives::B256::from(r0.context[1])) + .format() + .unwrap(); + let sell = Float::from(alloy::primitives::B256::from(r1.context[1])) + .format() + .unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/smoke_prod.rs` around lines 123 - 146, Extract the repeated responses[0].as_result().unwrap() and responses[1].as_result().unwrap() calls into local variables (e.g., let r0 = responses[0].as_result().unwrap(); let r1 = responses[1].as_result().unwrap()) and then use r0 and r1 for all subsequent .context accesses, length assertions, publish_time comparison, and when building buy/sell (with Float::from(alloy::primitives::B256::from(...))). This reduces duplication while keeping the same assertions and messages (including the publish_time equality check and buy/sell non-equality).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration.rs`:
- Around line 195-200: The test currently discards the boolean returned by
matches!(responses[0], OracleResponse::Err(_)), so it never fails; change it to
assert the result by wrapping the matches! call in an assert! (e.g.,
assert!(matches!(responses[0], OracleResponse::Err(_)))); this ensures the test
actually fails when responses[0] is not an OracleResponse::Err variant. Use the
existing responses variable and the OracleResponse enum in the assertion.
---
Nitpick comments:
In `@src/lib.rs`:
- Around line 145-148: The closure in the computation of needed_symbols uses
and_then(|p| Some(p.symbol.as_str())), which is equivalent to map; update the
iterator chain that builds needed_symbols (the resolved.iter().filter_map(...)
expression) to replace and_then(|p| Some(p.symbol.as_str())) with map(|p|
p.symbol.as_str()) so the intent is clearer and removes the unnecessary Some
wrapper.
In `@tests/smoke_prod.rs`:
- Around line 123-146: Extract the repeated responses[0].as_result().unwrap()
and responses[1].as_result().unwrap() calls into local variables (e.g., let r0 =
responses[0].as_result().unwrap(); let r1 = responses[1].as_result().unwrap())
and then use r0 and r1 for all subsequent .context accesses, length assertions,
publish_time comparison, and when building buy/sell (with
Float::from(alloy::primitives::B256::from(...))). This reduces duplication while
keeping the same assertions and messages (including the publish_time equality
check and buy/sell non-equality).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bc65d78-069c-4e96-8541-b27f9949e85f
📒 Files selected for processing (4)
src/lib.rssrc/oracle.rstests/integration.rstests/smoke_prod.rs
hardyjosh
left a comment
hardyjosh
left a comment
There was a problem hiding this comment.
Architectural triage — not a correctness review. A few things worth resolving before merge, mostly around naming, leaky internal types, and the silent broadening of the wire-format change beyond what the PR description advertises. CodeRabbit's nits already stand; the inline comments below are the non-overlapping ones.
| /// Oracle response matching Rain's SignedContextV1 format. | ||
| /// The JSON array shape of this struct is what upstream | ||
| /// `rain.orderbook/crates/quote/src/oracle.rs` expects to deserialize. | ||
| /// Oracle ok response matching Rain's SignedContextV2 format. |
There was a problem hiding this comment.
Doc says SignedContextV2 but SCHEMA_VERSION (line 10, unchanged) is still 1. The wire format is changing (single struct → {status, ...} envelope) and the comment at the const explicitly says to bump this whenever the layout changes. Either bump to 2 so strategies can reject old-shape clients, or leave a note stating the deliberate decision not to. Worth calling out in the PR description either way.
|
|
||
| /// Single oracle result, can be ok or err. | ||
| /// The JSON array shape of this struct is what upstream | ||
| /// `rain.orderbook/crates/quote/src/oracle.rs` expects to deserialize. |
There was a problem hiding this comment.
This comment (and the copy of it at line 63 on the OracleResponse alias) claims rain.orderbook/crates/quote/src/oracle.rs expects to deserialize this shape. That file doesn't exist in rain.orderbook — grepped crates/quote/ and crates/common/. The reference was already stale pre-PR; now it's duplicated. Either point at the real consumer or drop the reference so future readers don't chase a ghost.
| /// List of oracle result. | ||
| /// The JSON shape of this struct is what upstream | ||
| /// `rain.orderbook/crates/quote/src/oracle.rs` expects to deserialize. | ||
| pub type OracleResponse = Vec<OracleResult>; |
There was a problem hiding this comment.
Naming foot-gun: OracleResponse used to be the single-response struct. Keeping the identifier and making it a Vec<OracleResult> alias means OracleResponse::new() now silently means Vec::new() (see lib.rs:133). Consider OracleBatchResponse (or OracleResults) so the old name doesn't silently change meaning.
| None => OracleResult::Err(AppError::Unavailable(format!( | ||
| "No cached quote for {} yet. The poll loop has not succeeded since startup.", | ||
| pair.symbol | ||
| )).into()) |
There was a problem hiding this comment.
This quietly broadens the PR's scope: AppError::Unavailable (transient — cache hasn't populated yet) now collapses into 200 OK + inner Err, same as BadRequest (permanent). That erases the transport-layer distinction between "retry might work" and "your request is malformed" — clients can't use HTTP retry heuristics anymore. The PR description only mentions ok/err framing; this extra behavioural change isn't called out. Intentional? If yes, worth adding to the description and probably encoding transient-ness in the error payload so clients can still tell them apart.
| input_io_index: alloy::primitives::U256, | ||
| output_io_index: alloy::primitives::U256, | ||
| ) -> Result<ResolvedPair, AppError> { | ||
| ) -> Result<ResolvedPair, OracleErrResponse> { |
There was a problem hiding this comment.
Internal helper now returns the wire type OracleErrResponse. That leaks — an internal resolver should fail with AppError and be converted to the wire envelope at the HTTP boundary (in post_signed_context_v1). Keeping AppError here keeps wire-shape construction in one place and lets any future caller reuse the resolver.
| pair: &ResolvedPair, | ||
| quote: &crate::alpaca::QuoteData, | ||
| ) -> Result<oracle::OracleResponse, AppError> { | ||
| ) -> OracleResult { |
There was a problem hiding this comment.
Return type changing from Result<_, AppError> to OracleResult kills ? propagation — the body below is now nested match + map_or_else. Cleaner: keep Result<OracleOkResponse, OracleErrResponse> (or Result<_, AppError>) internally so ? still works, and convert once at the caller when pushing onto the Vec. Reads better and removes the map_or_else around sign_context.
| Err(e) => OracleResult::Err(OracleErrResponse { msg: e.to_string() }), | ||
| Ok(context) => state.signer.sign_context(&context).await.map_or_else( | ||
| |err| { | ||
| OracleResult::Err(OracleErrResponse { |
There was a problem hiding this comment.
Inconsistent construction: a few lines up (242–247 and 254–256) you go through AppError::X(...).into() via the From<AppError> impl. Here and at 271 you build OracleErrResponse { msg: ... } directly. Pick one — either always go through AppError (so formatting stays centralised in the Display impl) or always construct OracleErrResponse directly (and drop the From impl).
| #[tokio::test] | ||
| async fn test_v1_batch_returns_mixed_result() { | ||
| let app = test_app().await; | ||
| // Two orders: one i sknown and chade, other is unknown and not cached. |
There was a problem hiding this comment.
Typo: one i sknown and chade, other is unknown and not cached → one is known and cached, the other is unknown and not cached.
2 participants
Motivation
resolves #5
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Tests