Update batch quote logic to use error registry#2268
Conversation
WalkthroughAdded optional Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant batch_quote_wrapper as "batch_quote (wrapper)"
participant batch_quote_impl as "batch_quote_with_registry"
participant OpenChainRegistry as "OpenChainRegistry (default)"
participant ErrorDecoder as "AbiDecodedErrorType/ErrorDecoder"
Caller->>batch_quote_wrapper: batch_quote(..., multicall, registry_opt)
alt registry_opt is None
batch_quote_wrapper->>OpenChainRegistry: create default registry
OpenChainRegistry-->>batch_quote_wrapper: ®istry
else registry_opt provided
batch_quote_wrapper-->>batch_quote_impl: pass provided ®istry
end
batch_quote_wrapper->>batch_quote_impl: batch_quote_with_registry(..., ®istry)
batch_quote_impl->>ErrorDecoder: decode(revert_bytes, ®istry)
ErrorDecoder-->>batch_quote_impl: decoded error/result
batch_quote_impl-->>batch_quote_wrapper: Vec<QuoteResult>
batch_quote_wrapper-->>Caller: Result<Vec<QuoteResult>>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (7)
🧰 Additional context used📓 Path-based instructions (4)crates/*/{src,tests,benches,examples}/**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
crates/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
tauri-app/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-05-20T10:20:08.206ZApplied to files:
⏰ 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). (17)
🔇 Additional comments (13)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/quote/src/quote.rs(4 hunks)crates/quote/src/rpc.rs(9 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/quote/src/quote.rscrates/quote/src/rpc.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/quote/src/quote.rscrates/quote/src/rpc.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/quote/src/quote.rscrates/quote/src/rpc.rs
🧠 Learnings (1)
📚 Learning: 2025-05-20T10:20:08.206Z
Learnt from: 0xgleb
PR: rainlanguage/rain.orderbook#1859
File: crates/quote/src/quote_debug.rs:472-492
Timestamp: 2025-05-20T10:20:08.206Z
Learning: In the Rain Orderbook codebase, the `#[tokio::test(flavor = "multi_thread")]` annotation is specifically needed for tests that use `LocalEvm`, not just for consistency across all async tests.
Applied to files:
crates/quote/src/rpc.rs
🧬 Code graph analysis (1)
crates/quote/src/quote.rs (1)
crates/quote/src/rpc.rs (2)
batch_quote(16-96)rpcs(24-27)
🔇 Additional comments (8)
crates/quote/src/quote.rs (4)
80-91: LGTM: back-compat call updatedPassing None for the new registry parameter keeps existing behavior and compiles against the new signature. No issues spotted.
124-124: LGTM: batch invocation updatedThe added None aligns with the new rpc::batch_quote signature.
190-198: LGTM: subgraph path updatedRegistry parameter wired as None; behavior preserved.
278-287: LGTM: batch spec path updatedCall-site correctly forwards None for registry; flow using VecDeque remains unchanged.
crates/quote/src/rpc.rs (4)
10-10: New decoding imports are appropriateBringing in AbiDecodedErrorType, ErrorRegistry, OpenChainRegistry matches the new registry-aware decoding path.
53-56: Registry-aware decoding correctly appliedBoth multicall error paths now use decode_with_registry(registry). This is consistent and maintains per-call vs. aggregate failure handling.
Also applies to: 85-87
271-286: Tests exercise custom registry pathFakeRegistry via async-trait is minimal and effective; assertions cover Known vs Unknown decoding. Nice.
Also applies to: 294-296, 102-102, 109-109
22-23: All internal call sites correctly updated, but public API break requires external user verificationThe function
batch_quoteis publicly exported (pub mod rpcin lib.rs) and its signature was changed by adding anOption<&dyn ErrorRegistry>parameter. All internal call sites within the workspace have been updated to passNonefor this parameter. However, if this crate is used as a library by external projects, those users' code will break when upgrading. Consider providing a deprecated wrapper function retaining the old signature, or verify through your package's dependency tracking/usage telemetry that no external users depend on the old signature.
| let oc_default = OpenChainRegistry::default(); | ||
| let registry: &dyn ErrorRegistry = registry.unwrap_or(&oc_default); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Slight improvement: avoid constructing default when not needed
Current unwrap_or always builds OpenChainRegistry even when a registry is provided. Minor cost, but you can avoid it with an if-let.
- let oc_default = OpenChainRegistry::default();
- let registry: &dyn ErrorRegistry = registry.unwrap_or(&oc_default);
+ let oc_default;
+ let registry: &dyn ErrorRegistry = if let Some(reg) = registry {
+ reg
+ } else {
+ oc_default = OpenChainRegistry::default();
+ &oc_default
+ };📝 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.
| let oc_default = OpenChainRegistry::default(); | |
| let registry: &dyn ErrorRegistry = registry.unwrap_or(&oc_default); | |
| let oc_default; | |
| let registry: &dyn ErrorRegistry = if let Some(reg) = registry { | |
| reg | |
| } else { | |
| oc_default = OpenChainRegistry::default(); | |
| &oc_default | |
| }; |
🤖 Prompt for AI Agents
In crates/quote/src/rpc.rs around lines 46 to 48, the current code always
constructs OpenChainRegistry::default() even when a registry is provided; change
the logic to only create the default when registry is None by replacing
unwrap_or with an if-let (or match) that checks registry: if Some(r) use r, else
construct OpenChainRegistry::default() and use a reference to it, ensuring the
default is only built when needed.
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=M 🧠 Learnings used |
3 participants
Motivation
See issues:
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Improvements
Chores