fix: quote via orderbook-native multicall, not Multicall3#2550
Open
fix: quote via orderbook-native multicall, not Multicall3#2550
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR replaces Multicall3 Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant QuoteAPI as Quote API<br/>(quote.rs)
participant RPC as RPC Layer<br/>(rpc.rs)
participant Orderbook as Orderbook<br/>Multicall
participant ErrorReg as ErrorRegistry
Caller->>QuoteAPI: do_quote(counterparty)
QuoteAPI->>RPC: batch_quote(targets, counterparty)
RPC->>RPC: Group targets by orderbook and chunk them
RPC->>Orderbook: eth_call multicall(bytes[]) (from=counterparty)
alt Multicall Succeeds
Orderbook-->>RPC: returns bytes[] (per-target ABI payloads)
RPC->>RPC: Decode each bytes element → quote2Return
RPC->>RPC: Map exists=false → FailedQuote::NonExistent
RPC->>RPC: Map decode failures → FailedQuote::CorruptReturnData
else Chunk Reverts
Orderbook-->>RPC: revert with error data
RPC->>ErrorReg: decode revert selector
ErrorReg-->>RPC: known/unknown error
RPC->>RPC: Bisect chunk and repeat eth_call per slice until singletons
alt Singleton Reverts
RPC->>RPC: attribute revert → per-target FailedQuote
else Singleton Succeeds
RPC->>RPC: decode → per-target quote2Return
end
end
RPC-->>QuoteAPI: scatter results preserving input order
QuoteAPI-->>Caller: Vec<QuoteResult>
sequenceDiagram
participant RPC as RPC Layer
participant MC3 as Multicall3<br/>aggregate3()
RPC->>MC3: aggregate3([calls...])
alt All Succeed
MC3-->>RPC: Vec<Result {success, returnData}>
else Any Fails
MC3-->>RPC: Partial results with failure indicators
end
RPC->>RPC: Decode returnData per element and infer errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 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 |
hardyjosh
pushed a commit
to ST0x-Technology/st0x.rest.api
that referenced
this pull request
Apr 15, 2026
Picks up the quote RPC refactor in rainlanguage/raindex#2550: quotes now use orderbook-native `multicall(bytes[])` with `from=counterparty`, preserving `msg.sender` through the batch. This is what lets API-gated strategies (whose `calculate-io` asserts `signed-context<0 0>() == order-counterparty()`) actually survive the quote stage and enter the take-orders selection instead of being silently filtered. No code changes in this repo — the counterparty was already threaded through to the rain.orderbook quote layer via the injector flow in the parent PR; it just wasn't being used by the RPC layer until the submodule fix. Depends on rainlanguage/raindex#2550.
Closed
4 tasks
hardyjosh
force-pushed
the
2026-04-15-quote-counterparty-msg-sender
branch
from
7f6741f to
85bb758
Compare
hardyjosh
pushed a commit
to ST0x-Technology/st0x.rest.api
that referenced
this pull request
Apr 15, 2026
Picks up the quote RPC refactor in rainlanguage/raindex#2550: quotes now use orderbook-native `multicall(bytes[])` with `from=counterparty`, preserving `msg.sender` through the batch. This is what lets API-gated strategies (whose `calculate-io` asserts `signed-context<0 0>() == order-counterparty()`) actually survive the quote stage and enter the take-orders selection instead of being silently filtered. No code changes in this repo — the counterparty was already threaded through to the rain.orderbook quote layer via the injector flow in the parent PR; it just wasn't being used by the RPC layer until the submodule fix. Depends on rainlanguage/raindex#2550.
hardyjosh
force-pushed
the
2026-04-15-quote-counterparty-msg-sender
branch
from
85bb758 to
4dfef89
Compare
hardyjosh
pushed a commit
to ST0x-Technology/st0x.rest.api
that referenced
this pull request
Apr 15, 2026
Picks up the quote RPC refactor in rainlanguage/raindex#2550: quotes now use orderbook-native `multicall(bytes[])` with `from=counterparty`, preserving `msg.sender` through the batch. This is what lets API-gated strategies (whose `calculate-io` asserts `signed-context<0 0>() == order-counterparty()`) actually survive the quote stage and enter the take-orders selection instead of being silently filtered. No code changes in this repo — the counterparty was already threaded through to the rain.orderbook quote layer via the injector flow in the parent PR; it just wasn't being used by the RPC layer until the submodule fix. Depends on rainlanguage/raindex#2550.
hardyjosh
force-pushed
the
2026-04-15-quote-counterparty-msg-sender
branch
from
4dfef89 to
c10ecf1
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/quote/src/order_quotes.rs (1)
250-256: Avoid a release-only panic if the scatter invariant is ever broken.
debug_assert_eq!disappears in release builds, butunwrap()does not. If a future change inBatchQuoteTarget::do_quote()or the scatter logic returns fewer entries thanquoted_slot_indices, this path will panic instead of surfacing a normalError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/quote/src/order_quotes.rs` around lines 250 - 256, The code currently uses debug_assert_eq!(quote_results.len(), quoted_slot_indices.len()) and later unwrap() which can panic in release if the invariant breaks; replace the debug-only assert with a runtime check: if quote_results.len() != quoted_slot_indices.len() return an Err that surfaces a descriptive error (mentioning BatchQuoteTarget::do_quote(), quote_results and quoted_slot_indices) before scattering into all_responses, then perform the scatter and safely collect the options (or return Err if any slot remains None). Ensure you update the function's error return path to propagate this runtime check instead of allowing unwrap()-based panics.
🤖 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/quote/ARCHITECTURE.md`:
- Around line 100-104: Update the ARCHITECTURE.md revert-attribution paragraph
to reflect that quote_chunk_with_probe_and_split always performs bisection (no
probe stage or whole-chunk shortcut anymore): remove references to the "probe"
stage and the "if no probe succeeds, the whole chunk gets the same decoded
failure" branch, and replace step 5 with a concise description that the function
recursively halves the chunk until batch-of-1 failures are isolated and
attributed to specific QuoteTarget entries; leave the note about orderbook
groups running via futures::future::join_all unchanged.
In `@crates/quote/src/rpc.rs`:
- Around line 129-155: The match currently wraps any RpcError (including
Transport/SerError/DeserError/NullResp/UnsupportedFeature/LocalUsageError) and
ErrorResp-without-revert as
Error::ChunkReverted(FailedQuote::CorruptReturnData), which hides infrastructure
failures; change the logic so only ErrorResp with revert bytes is converted into
a per-target FailedQuote and wrapped as Error::ChunkReverted(Box::new(...))
(using as_revert_data + AbiDecodedErrorType::selector_registry_abi_decode ->
FailedQuote::RevertError / RevertErrorDecodeFailed), while all other RpcError
variants (the Err(e) arm and the ErrorResp branch when as_revert_data() is None)
should be returned as an infrastructure-level error (propagate/convert to an
appropriate non-ChunkReverted Error variant) so RPC/transport/serialization
failures are not treated as chunk bisection failures.
---
Nitpick comments:
In `@crates/quote/src/order_quotes.rs`:
- Around line 250-256: The code currently uses
debug_assert_eq!(quote_results.len(), quoted_slot_indices.len()) and later
unwrap() which can panic in release if the invariant breaks; replace the
debug-only assert with a runtime check: if quote_results.len() !=
quoted_slot_indices.len() return an Err that surfaces a descriptive error
(mentioning BatchQuoteTarget::do_quote(), quote_results and quoted_slot_indices)
before scattering into all_responses, then perform the scatter and safely
collect the options (or return Err if any slot remains None). Ensure you update
the function's error return path to propagate this runtime check instead of
allowing unwrap()-based panics.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b293940f-e9b0-41cc-a24c-277352fc82ab
📒 Files selected for processing (8)
crates/common/src/raindex_client/order_quotes.rscrates/quote/ARCHITECTURE.mdcrates/quote/src/cli/mod.rscrates/quote/src/error.rscrates/quote/src/order_quotes.rscrates/quote/src/quote.rscrates/quote/src/rpc.rspackages/orderbook/test/js_api/raindexClient.test.ts
hardyjosh
force-pushed
the
2026-04-15-quote-counterparty-msg-sender
branch
from
c10ecf1 to
160887d
Compare
hardyjosh
changed the base branch from
2026-04-14-take-orders-injected-signed-context
to
main
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
hardyjosh
force-pushed
the
2026-04-15-quote-counterparty-msg-sender
branch
from
160887d to
6df1e5c
Compare
hardyjosh
pushed a commit
to ST0x-Technology/st0x.rest.api
that referenced
this pull request
Apr 15, 2026
Picks up the quote RPC refactor in rainlanguage/raindex#2550: quotes now use orderbook-native `multicall(bytes[])` with `from=counterparty`, preserving `msg.sender` through the batch. This is what lets API-gated strategies (whose `calculate-io` asserts `signed-context<0 0>() == order-counterparty()`) actually survive the quote stage and enter the take-orders selection instead of being silently filtered. No code changes in this repo — the counterparty was already threaded through to the rain.orderbook quote layer via the injector flow in the parent PR; it just wasn't being used by the RPC layer until the submodule fix. Depends on rainlanguage/raindex#2550.
Open
4 tasks
findolor
requested changes
Apr 16, 2026
| all_results.extend(results); | ||
| Ok(all_results) | ||
| // Scatter quote results back into the iteration-ordered response vector. | ||
| debug_assert_eq!(quote_results.len(), quoted_slot_indices.len()); |
Collaborator
There was a problem hiding this comment.
This should be removed from the production code
Comment on lines
+129
to
155
| Err(RpcError::ErrorResp(err_resp)) => { | ||
| if let Some(revert) = err_resp.as_revert_data() { | ||
| let decoded = match AbiDecodedErrorType::selector_registry_abi_decode( | ||
| revert.as_ref(), | ||
| registry, | ||
| ) | ||
| .await | ||
| { | ||
| Ok(e) => results.push(Err(FailedQuote::RevertError(e))), | ||
| Err(e) => results.push(Err(FailedQuote::RevertErrorDecodeFailed(e))), | ||
| } | ||
| Ok(abi_err) => FailedQuote::RevertError(abi_err), | ||
| Err(err) => FailedQuote::RevertErrorDecodeFailed(err), | ||
| }; | ||
| // Wrap the per-target failure as a chunk-level error so the | ||
| // bisection logic can decide whether to split or to attribute | ||
| // it to a single target. | ||
| Err(Error::ChunkReverted(Box::new(decoded))) | ||
| } else { | ||
| Err(Error::ChunkReverted(Box::new( | ||
| FailedQuote::CorruptReturnData(format!( | ||
| "RPC error without revert data: {err_resp}" | ||
| )), | ||
| ))) | ||
| } | ||
| } | ||
| Err(e) => Err(Error::ChunkReverted(Box::new( | ||
| FailedQuote::CorruptReturnData(format!("eth_call transport error: {e}")), | ||
| ))), | ||
| } |
Collaborator
There was a problem hiding this comment.
Error handling is too broad: Logic in this block wraps all RPC errors (transport failures, deserialization errors, network issues) as Error::ChunkReverted(FailedQuote::CorruptReturnData(...)). This means a network timeout gets treated the same as a legitimate strategy revert and triggers bisection. You should only wrap ErrorResp with revert data as ChunkReverted; other RPC errors should propagate as infrastructure failures.
Contributor
Author
There was a problem hiding this comment.
Good catch — d868deb adds a new Error::TransportError variant for non-ErrorResp RPC failures (transport, deser, etc). Only ErrorResp with revert data now produces ChunkReverted.
hardyjosh
force-pushed
the
2026-04-15-quote-counterparty-msg-sender
branch
from
6df1e5c to
d868deb
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/quote/src/rpc.rs (1)
281-290: Consider usingHashMapfor O(n) grouping.The current linear scan for grouping is O(n × g) where g is the number of distinct orderbooks. While this is likely acceptable for typical workloads, a
HashMap<Address, Vec<...>>would be O(n).♻️ Optional refactor using HashMap
+use std::collections::HashMap; + - let mut groups: Vec<(Address, Vec<(usize, QuoteTarget)>)> = Vec::new(); - for (i, target) in quote_targets.iter().enumerate() { - if let Some(group) = groups.iter_mut().find(|(ob, _)| *ob == target.orderbook) { - group.1.push((i, target.clone())); - } else { - groups.push((target.orderbook, vec![(i, target.clone())])); - } - } + let mut groups: HashMap<Address, Vec<(usize, QuoteTarget)>> = HashMap::new(); + for (i, target) in quote_targets.iter().enumerate() { + groups + .entry(target.orderbook) + .or_default() + .push((i, target.clone())); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/quote/src/rpc.rs` around lines 281 - 290, The current grouping loop (building `groups: Vec<(Address, Vec<(usize, QuoteTarget)>)>`) does an O(n×g) scan; replace it with a HashMap-based O(n) grouping by using a HashMap<Address, Vec<(usize, QuoteTarget)>>, insert or push into the map for each (i, target) from `quote_targets.iter().enumerate()`, then convert the map into the final `Vec<(Address, Vec<(usize, QuoteTarget)>)>` if you still need a Vec; update references to `groups` accordingly and keep the original index and `QuoteTarget` clones when inserting so scattering back into the result works the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/quote/src/rpc.rs`:
- Around line 281-290: The current grouping loop (building `groups:
Vec<(Address, Vec<(usize, QuoteTarget)>)>`) does an O(n×g) scan; replace it with
a HashMap-based O(n) grouping by using a HashMap<Address, Vec<(usize,
QuoteTarget)>>, insert or push into the map for each (i, target) from
`quote_targets.iter().enumerate()`, then convert the map into the final
`Vec<(Address, Vec<(usize, QuoteTarget)>)>` if you still need a Vec; update
references to `groups` accordingly and keep the original index and `QuoteTarget`
clones when inserting so scattering back into the result works the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8830088-6a7f-4183-ab4f-5e028b48b84a
📒 Files selected for processing (8)
crates/common/src/raindex_client/order_quotes.rscrates/quote/ARCHITECTURE.mdcrates/quote/src/cli/mod.rscrates/quote/src/error.rscrates/quote/src/order_quotes.rscrates/quote/src/quote.rscrates/quote/src/rpc.rspackages/orderbook/test/js_api/raindexClient.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/orderbook/test/js_api/raindexClient.test.ts
- crates/quote/ARCHITECTURE.md
- crates/quote/src/cli/mod.rs
findolor
approved these changes
Apr 17, 2026
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.
Motivation
OrderBookV6.quote2passesmsg.senderas the counterparty intocalculateOrderIO. The quote crate batched via Multicall3 (provider.multicall().aggregate3()), which forwards via CALL — somsg.senderinsidequote2was always the Multicall3 contract, never the intended taker.For public strategies this didn't matter. For strategies that inspect
msg.sender— e.g. API-gated strategies assertingsigned-context<0 0>() == order-counterparty()(see the companion st0x.rest.api PR) — every quote reverted, the candidate was silently dropped as a failed quote, and the gated order never entered selection even though its actualtakeOrders4path (which usesmsg.sender = taker) worked fine.Relates to the parent PR #2547, which introduced
SignedContextInjectorand already threads acounterpartythrough the quote pipeline. This change makes the counterparty actually arrive atquote2asmsg.sender.Solution
OrderBookV6 inherits OpenZeppelin's
Multicall, which usesdelegatecallto self — preservingmsg.senderthrough the batch. Callingorderbook.multicall([quote2,...])viaeth_callwithfrom=counterpartygives each innerquote2the intendedmsg.sender. One RPC per orderbook, correct semantics — we keep batching without the unsound Multicall3 layer.quote_chunk_oncetakescounterparty: Address, groups targets by orderbook, ABI-encodes eachquote2and wraps in a singleorderbook.multicall(bytes[])eth_callper group withfrom=counterparty.futures::future::join_all.Error::ChunkRevertedand drive the existing probe-and-split bisection to attribute reverts to singletons when OZ Multicall bubbles the first delegatecall revert.multicall_addressplumbing removed from the quote pipeline — the Multicall3 override address no longer applies here. Unchanged in unrelated ERC20 metadata reads.counterparty: Addressthreaded throughbatch_quote,BatchQuoteTarget::do_quote, andget_order_quotes. The wasm-exportedget_order_quotes_batchstays backward-compat viaAddress::ZERO; the injector-aware variant already has the counterparty in scope.eth_callfromis the counterparty.Checks
By submitting this for review, I'm confirming I've done the following:
Verified locally:
cargo check -p rain_orderbook_quote/-p rain_orderbook_commoncleancargo test -p rain_orderbook_quote --lib— 51/51cargo test -p rain_orderbook_common --lib— 898/898cargo fmtSummary by CodeRabbit