implementing exclusivity orders#318
Open
nahimterrazas wants to merge 1 commit into
Open
Conversation
nahimterrazas
requested review from
NicoMolinaOZ,
pepebndc and
shahnami
as code owners
📝 WalkthroughWalkthroughThis PR introduces optional exclusive-fill configuration to quote generation. It defines Changes
Sequence DiagramsequenceDiagram
participant Client as Client Request
participant QG as QuoteGenerator
participant ER as Exclusivity Resolver
participant EC as Context Encoder
participant PS as Protocol Signing
participant Output as Quote Response
Client->>QG: Quote request with metadata
QG->>ER: resolve_exclusivity(config, request, now)
ER->>ER: Validate duration from metadata
ER->>ER: Check against config mode
ER-->>QG: ExclusivityParams or None
alt Exclusivity Enabled
QG->>EC: encode_exclusive_context(solver_address, start_time)
EC-->>QG: context_bytes (37-byte tagged format)
QG->>PS: Sign with context_bytes
PS->>PS: Hash context into witness digest
PS-->>Output: Include context_hex in output
else Exclusivity Disabled
QG->>PS: Sign without context
PS->>PS: Use empty context hash
PS-->>Output: context_hex = "0x"
end
Output-->>Client: Quote with protocol-specific context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/solver-service/src/apis/quote/generation.rs (1)
822-824:⚠️ Potential issue | 🟡 MinorStale rustdoc on
compute_eip3009_order_identifier.The doc comment still says it returns
(nonce, order_identifier)but the signature now returns a 3-tuple includingcontext_bytes. Update the doc so callers don't have to read the body to know about the exclusive-context payload.📝 Proposed doc fix
- /// Compute the orderIdentifier for an EIP-3009 order by building a StandardOrder - /// and calling the contract's orderIdentifier function using the delivery service - /// Returns (nonce, order_identifier) + /// Compute the orderIdentifier for an EIP-3009 order by building a StandardOrder + /// and calling the contract's orderIdentifier function using the delivery service. + /// + /// Returns `(nonce, order_identifier, context_bytes)` where `context_bytes` is the + /// resolved exclusive-fill context (empty when exclusivity is disabled) that was + /// embedded into `outputs[0].context` for the orderIdentifier computation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-service/src/apis/quote/generation.rs` around lines 822 - 824, Update the stale doc comment for compute_eip3009_order_identifier to reflect the new return type: mention that it returns a 3-tuple (nonce, order_identifier, context_bytes) where context_bytes is the exclusive-context payload used in the EIP-3009 order; keep the existing brief description about building a StandardOrder and calling the contract's orderIdentifier function via the delivery service but add the new context_bytes element to the documented return value so callers see all three components without reading the implementation.
🧹 Nitpick comments (3)
crates/solver-core/src/engine/mod.rs (1)
671-674: Tighten doc wording for the accessor return typeThe method returns
&solver_types::Address(wrapper type), not raw bytes directly. Clarifying this avoids caller confusion.✏️ Suggested doc tweak
-/// Returns the solver address as raw bytes. +/// Returns the solver address.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-core/src/engine/mod.rs` around lines 671 - 674, The doc comment for the accessor solver_address should be corrected to state that it returns a reference to the wrapper type solver_types::Address (not raw bytes); update the doc line above pub fn solver_address(&self) -> &solver_types::Address to clarify it returns &solver_types::Address (a wrapper/address type) and not raw bytes, so callers understand they receive the Address wrapper rather than raw byte data.crates/solver-service/src/apis/quote/signing/exclusivity.rs (1)
107-110: Make address-length validation non-panicking in production
copy_from_slicewill panic ifsolver_address.0.len() != 20. Consider makingencode_exclusive_contextfallible and propagating an explicitQuoteErrorinstead of relying on a debug-only assert.🛡️ Suggested direction
-pub fn encode_exclusive_context(solver_address: &Address, start_time: u32) -> Vec<u8> { +pub fn encode_exclusive_context( + solver_address: &Address, + start_time: u32, +) -> Result<Vec<u8>, QuoteError> { let mut out = Vec::with_capacity(1 + 32 + 4); out.push(EXCLUSIVE_CONTEXT_TAG); let mut bytes32 = [0u8; 32]; let addr_bytes = &solver_address.0; - debug_assert_eq!(addr_bytes.len(), 20, "solver address must be 20 bytes"); + if addr_bytes.len() != 20 { + return Err(QuoteError::InvalidRequest( + "solver address must be 20 bytes".to_string(), + )); + } bytes32[12..].copy_from_slice(addr_bytes); out.extend_from_slice(&bytes32); out.extend_from_slice(&start_time.to_be_bytes()); - out + Ok(out) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-service/src/apis/quote/signing/exclusivity.rs` around lines 107 - 110, The code uses debug_assert_eq and copy_from_slice on solver_address.0 which will panic in production if the length is not 20; change encode_exclusive_context to be fallible (return Result<..., QuoteError>), replace the debug_assert_eq with an explicit length check on solver_address.0 (expecting 20) that returns a descriptive QuoteError on mismatch, and only then perform bytes32[12..].copy_from_slice(addr_bytes) and out.extend_from_slice(&bytes32); update callers of encode_exclusive_context to propagate the Result accordingly.crates/solver-service/src/apis/quote/generation.rs (1)
843-858: Extract the exclusivity-resolution boilerplate into a helper.The same five-line
now_secs+config.api → quote → exclusivity+resolve_exclusivity(...)block is duplicated verbatim in three places:
- here (lines 843-858) inside
compute_eip3009_order_identifier,- lines 1235-1256 inside
build_compact_message,- lines 1678-1687 inside
build_permit2_message_object.Beyond the DRY violation, hoisting this into a
QuoteGeneratormethod (or, even better, intogenerate_eip3009_orderso it runs beforetokio::try_join!at lines 725-736) lets the EIP-3009 path fail-fast on malformedmetadata.exclusivitywithout first dispatching the siblingbuild_eip3009_domain_object/get_eip3009_domain_separatorcontract calls. The resolvedExclusivityParams(or already-encoded bytes) can then be threaded down intocompute_eip3009_order_identifierthe same way it is intobuild_permit2_batch_witness_digest.♻️ Sketch of the helper
fn resolve_request_exclusivity( &self, request: &GetQuoteRequest, config: &Config, ) -> Result<Option<crate::apis::quote::signing::exclusivity::ExclusivityParams>, QuoteError> { use crate::apis::quote::signing::exclusivity::resolve_exclusivity; let now_secs = chrono::Utc::now().timestamp() as u64; let excl_cfg = config .api .as_ref() .and_then(|a| a.quote.as_ref()) .and_then(|q| q.exclusivity.as_ref()); resolve_exclusivity(excl_cfg, request.intent.metadata.as_ref(), now_secs) }Then
compute_eip3009_order_identifieracceptsexclusivity: Option<ExclusivityParams>(mirroring the permit2 helper),build_compact_messagecallsself.resolve_request_exclusivity(request, config)?once, andgenerate_eip3009_orderresolves once beforetry_join!.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-service/src/apis/quote/generation.rs` around lines 843 - 858, Duplicate exclusivity-resolution logic appears in compute_eip3009_order_identifier, build_compact_message, and build_permit2_message_object; extract it into a QuoteGenerator method (e.g., resolve_request_exclusivity) that encapsulates the now_secs + config.api → quote → exclusivity lookup and resolve_exclusivity(...) call and returns Result<Option<ExclusivityParams>, QuoteError>. Call this helper once from generate_eip3009_order before the tokio::try_join! so the EIP-3009 path fails fast, and change compute_eip3009_order_identifier, build_compact_message, and build_permit2_message_object to accept the resolved Option<ExclusivityParams> (or pre-encoded context bytes) instead of re-running the boilerplate.
🤖 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/solver-types/src/standards/eip7683.rs`:
- Around line 754-763: The code currently treats a present-but-non-string
outputs[].context as "0x" which silently changes reconstructed output context;
update the extraction to explicitly reject non-string types: instead of using
output_obj.get("context").and_then(|c| c.as_str()).unwrap_or("0x"), match on
output_obj.get("context") and if Some(value) then require value.as_str() to
return Some(str) or return an Err with a clear message (e.g., "outputs[].context
must be a string") so malformed types are rejected; keep the existing hex
decoding logic for the resulting string (refer to the
output_obj/context_str/context_bytes variables in this block).
---
Outside diff comments:
In `@crates/solver-service/src/apis/quote/generation.rs`:
- Around line 822-824: Update the stale doc comment for
compute_eip3009_order_identifier to reflect the new return type: mention that it
returns a 3-tuple (nonce, order_identifier, context_bytes) where context_bytes
is the exclusive-context payload used in the EIP-3009 order; keep the existing
brief description about building a StandardOrder and calling the contract's
orderIdentifier function via the delivery service but add the new context_bytes
element to the documented return value so callers see all three components
without reading the implementation.
---
Nitpick comments:
In `@crates/solver-core/src/engine/mod.rs`:
- Around line 671-674: The doc comment for the accessor solver_address should be
corrected to state that it returns a reference to the wrapper type
solver_types::Address (not raw bytes); update the doc line above pub fn
solver_address(&self) -> &solver_types::Address to clarify it returns
&solver_types::Address (a wrapper/address type) and not raw bytes, so callers
understand they receive the Address wrapper rather than raw byte data.
In `@crates/solver-service/src/apis/quote/generation.rs`:
- Around line 843-858: Duplicate exclusivity-resolution logic appears in
compute_eip3009_order_identifier, build_compact_message, and
build_permit2_message_object; extract it into a QuoteGenerator method (e.g.,
resolve_request_exclusivity) that encapsulates the now_secs + config.api → quote
→ exclusivity lookup and resolve_exclusivity(...) call and returns
Result<Option<ExclusivityParams>, QuoteError>. Call this helper once from
generate_eip3009_order before the tokio::try_join! so the EIP-3009 path fails
fast, and change compute_eip3009_order_identifier, build_compact_message, and
build_permit2_message_object to accept the resolved Option<ExclusivityParams>
(or pre-encoded context bytes) instead of re-running the boilerplate.
In `@crates/solver-service/src/apis/quote/signing/exclusivity.rs`:
- Around line 107-110: The code uses debug_assert_eq and copy_from_slice on
solver_address.0 which will panic in production if the length is not 20; change
encode_exclusive_context to be fallible (return Result<..., QuoteError>),
replace the debug_assert_eq with an explicit length check on solver_address.0
(expecting 20) that returns a descriptive QuoteError on mismatch, and only then
perform bytes32[12..].copy_from_slice(addr_bytes) and
out.extend_from_slice(&bytes32); update callers of encode_exclusive_context to
propagate the Result accordingly.
🪄 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: 865a6279-c196-451f-89dd-893a86f8090d
📒 Files selected for processing (8)
crates/solver-config/src/lib.rscrates/solver-core/src/engine/mod.rscrates/solver-service/src/apis/quote/generation.rscrates/solver-service/src/apis/quote/mod.rscrates/solver-service/src/apis/quote/signing/exclusivity.rscrates/solver-service/src/apis/quote/signing/mod.rscrates/solver-service/src/apis/quote/signing/payloads/permit2.rscrates/solver-types/src/standards/eip7683.rs
Comment on lines
+754
to
+763
| let context_str = output_obj | ||
| .get("context") | ||
| .and_then(|c| c.as_str()) | ||
| .unwrap_or("0x"); | ||
| let context_bytes = if context_str == "0x" || context_str.is_empty() { | ||
| Vec::new() | ||
| } else { | ||
| hex::decode(context_str.trim_start_matches("0x")) | ||
| .map_err(|e| format!("Invalid context hex '{context_str}': {e}"))? | ||
| }; |
There was a problem hiding this comment.
Reject malformed outputs[].context types instead of silently zeroing
If context is present but not a string, current logic treats it as "0x". That silently changes reconstructed output context and can produce inconsistent order reconstruction behavior.
🔧 Suggested fix
- let context_str = output_obj
- .get("context")
- .and_then(|c| c.as_str())
- .unwrap_or("0x");
+ let context_str = match output_obj.get("context") {
+ None => "0x",
+ Some(v) => v
+ .as_str()
+ .ok_or("Invalid context in output metadata: expected hex string")?,
+ };📝 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 context_str = output_obj | |
| .get("context") | |
| .and_then(|c| c.as_str()) | |
| .unwrap_or("0x"); | |
| let context_bytes = if context_str == "0x" || context_str.is_empty() { | |
| Vec::new() | |
| } else { | |
| hex::decode(context_str.trim_start_matches("0x")) | |
| .map_err(|e| format!("Invalid context hex '{context_str}': {e}"))? | |
| }; | |
| let context_str = match output_obj.get("context") { | |
| None => "0x", | |
| Some(v) => v | |
| .as_str() | |
| .ok_or("Invalid context in output metadata: expected hex string")?, | |
| }; | |
| let context_bytes = if context_str == "0x" || context_str.is_empty() { | |
| Vec::new() | |
| } else { | |
| hex::decode(context_str.trim_start_matches("0x")) | |
| .map_err(|e| format!("Invalid context hex '{context_str}': {e}"))? | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/solver-types/src/standards/eip7683.rs` around lines 754 - 763, The
code currently treats a present-but-non-string outputs[].context as "0x" which
silently changes reconstructed output context; update the extraction to
explicitly reject non-string types: instead of using
output_obj.get("context").and_then(|c| c.as_str()).unwrap_or("0x"), match on
output_obj.get("context") and if Some(value) then require value.as_str() to
return Some(str) or return an Err with a clear message (e.g., "outputs[].context
must be a string") so malformed types are rejected; keep the existing hex
decoding logic for the resulting string (refer to the
output_obj/context_str/context_bytes variables in this block).
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.
Summary
Testing Process
Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests