Pc02 allocator data validation (fix merge the pc04)#369
Open
nahimterrazas wants to merge 4 commits into
Open
Conversation
nahimterrazas
requested review from
pepebndc,
shahnami and
tirumerla
as code owners
|
Warning Review limit reached
More reviews will be available in 22 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements end-to-end validation for EIP-7683 Compact/ResourceLock orders. It adds canonical signature decoding, EIP-712 claim-hash computation, on-chain allocator authorization checks, refactors the compact signature validator to use shared utilities, extends the test harness and server mocks, and updates both the intake and off-chain discovery pipelines to enforce these validations. ChangesCompact/EIP-7683 ResourceLock Order Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
nahimterrazas
changed the title
![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)
crates/solver-discovery/src/implementations/offchain/_7683.rs (2)
1174-1216: 💤 Low valueTest helper duplication with
server.rs.The
abi_word,padded_bytes,shifted_compact_signature, andcompact_signaturefunctions are duplicated fromcrates/solver-service/src/server.rs. Consider extracting these to a shared test utilities module to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/solver-discovery/src/implementations/offchain/_7683.rs` around lines 1174 - 1216, The helper functions abi_word, padded_bytes, shifted_compact_signature, and compact_signature are duplicated here and in server.rs; extract them into a single shared test utilities module (e.g., a new crate or a common tests/util module) and replace the duplicates with imports. Create a new module exposing pub (or pub(crate) as appropriate) functions named abi_word, padded_bytes, shifted_compact_signature, and compact_signature, move the implementations there, update both crates to depend on that module (or add a path dependency/test-utils crate), and change the uses in this file and in server.rs to call the shared functions instead of the local copies. Ensure visibility and Cargo.toml are adjusted so tests can access the shared utilities.
641-761: ⚖️ Poor tradeoffConsider extracting shared allocator validation logic to reduce duplication.
This function largely duplicates the logic in
crates/solver-service/src/validators/compact_allocator.rs::validate_allocator_authorization. The main difference is this version usesDynProviderdirectly while the service version usesDeliveryService. Consider extracting the common validation logic into a shared utility insolver-typesthat both can reuse, parameterized over the RPC call mechanism.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/solver-discovery/src/implementations/offchain/_7683.rs` around lines 641 - 761, The validate_resource_lock_allocator_authorization function duplicates allocator validation logic present in validate_allocator_authorization; extract the shared steps (resolving network/provider, parsing TheCompact and allocator addresses, iterating inputs to resolve a single allocator, optional configured allocator check, computing claim_hash via compute_batch_compact_claim_hash, and calling isClaimAuthorized on IAllocator) into a new shared utility (e.g., in solver-types) that accepts: origin_chain_id/network config, a pluggable RPC client trait or async closure (to support DynProvider or DeliveryService), the order and allocator_data, and returns Result<(), DiscoveryError>; then update validate_resource_lock_allocator_authorization and the service validator to call this new utility (keeping provider-specific adapters that implement the RPC trait or wrap provider vs DeliveryService). Ensure you reference functions/types: validate_resource_lock_allocator_authorization, validate_allocator_authorization, compute_batch_compact_claim_hash, ITheCompact, IAllocator, AlloyAddress, and DynProvider when extracting and parameterizing the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/solver-service/src/validators/compact_allocator.rs`:
- Around line 116-123: The code calls
AlloyAddress::from_slice(&configured_allocator.0) which will panic if
configured_allocator.0 is not exactly 20 bytes; add an explicit length check
before conversion: verify configured_allocator.0.len() == 20 (or pattern-match
the inner bytes) and return Err(validation_error(...)) with a clear message
(e.g. "configured allocator_address must be 20 bytes") if the length is wrong,
then only call AlloyAddress::from_slice(&configured_allocator.0) and compare to
allocator; adjust the existing branch around configured_allocator to perform
this validation using the configured_allocator identifier and keep the same
validation_error flow used in ResourceLock checks.
---
Nitpick comments:
In `@crates/solver-discovery/src/implementations/offchain/_7683.rs`:
- Around line 1174-1216: The helper functions abi_word, padded_bytes,
shifted_compact_signature, and compact_signature are duplicated here and in
server.rs; extract them into a single shared test utilities module (e.g., a new
crate or a common tests/util module) and replace the duplicates with imports.
Create a new module exposing pub (or pub(crate) as appropriate) functions named
abi_word, padded_bytes, shifted_compact_signature, and compact_signature, move
the implementations there, update both crates to depend on that module (or add a
path dependency/test-utils crate), and change the uses in this file and in
server.rs to call the shared functions instead of the local copies. Ensure
visibility and Cargo.toml are adjusted so tests can access the shared utilities.
- Around line 641-761: The validate_resource_lock_allocator_authorization
function duplicates allocator validation logic present in
validate_allocator_authorization; extract the shared steps (resolving
network/provider, parsing TheCompact and allocator addresses, iterating inputs
to resolve a single allocator, optional configured allocator check, computing
claim_hash via compute_batch_compact_claim_hash, and calling isClaimAuthorized
on IAllocator) into a new shared utility (e.g., in solver-types) that accepts:
origin_chain_id/network config, a pluggable RPC client trait or async closure
(to support DynProvider or DeliveryService), the order and allocator_data, and
returns Result<(), DiscoveryError>; then update
validate_resource_lock_allocator_authorization and the service validator to call
this new utility (keeping provider-specific adapters that implement the RPC
trait or wrap provider vs DeliveryService). Ensure you reference
functions/types: validate_resource_lock_allocator_authorization,
validate_allocator_authorization, compute_batch_compact_claim_hash, ITheCompact,
IAllocator, AlloyAddress, and DynProvider when extracting and parameterizing the
logic.
🪄 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: ace8317d-c24c-4ae7-b973-d32ca88b3b04
📒 Files selected for processing (12)
crates/solver-discovery/src/implementations/offchain/_7683.rscrates/solver-e2e-tests/src/lib.rscrates/solver-e2e-tests/tests/compact_allocator_e2e.rscrates/solver-service/src/eip712/compact/mod.rscrates/solver-service/src/eip712/mod.rscrates/solver-service/src/server.rscrates/solver-service/src/validators/compact_allocator.rscrates/solver-service/src/validators/mod.rscrates/solver-service/src/validators/signature.rscrates/solver-types/src/standards/eip7683.rscrates/solver-types/src/standards/eip7683/compact_claims.rscrates/solver-types/src/standards/eip7683/compact_signatures.rs
Comment on lines
+116
to
+123
| if let Some(expected) = configured_allocator { | ||
| let expected = AlloyAddress::from_slice(&expected.0); | ||
| if expected != allocator { | ||
| return Err(validation_error( | ||
| "ResourceLock inputs use an allocator that differs from the configured allocator_address", | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing length validation for configured_allocator.
The Address wrapper could theoretically contain a non-20-byte slice. Converting directly via AlloyAddress::from_slice(&expected.0) would panic if the length is incorrect. The downstream usage in solver-discovery/_7683.rs (context snippet 1, lines 719-726) explicitly checks for 20-byte length before conversion.
🛡️ Proposed fix to add length validation
// If the solver pins an allocator, the resolved one must match it.
if let Some(expected) = configured_allocator {
+ if expected.0.len() != 20 {
+ return Err(validation_error(
+ "Invalid configured allocator address length",
+ ));
+ }
let expected = AlloyAddress::from_slice(&expected.0);
if expected != allocator {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/solver-service/src/validators/compact_allocator.rs` around lines 116 -
123, The code calls AlloyAddress::from_slice(&configured_allocator.0) which will
panic if configured_allocator.0 is not exactly 20 bytes; add an explicit length
check before conversion: verify configured_allocator.0.len() == 20 (or
pattern-match the inner bytes) and return Err(validation_error(...)) with a
clear message (e.g. "configured allocator_address must be 20 bytes") if the
length is wrong, then only call
AlloyAddress::from_slice(&configured_allocator.0) and compare to allocator;
adjust the existing branch around configured_allocator to perform this
validation using the configured_allocator identifier and keep the same
validation_error flow used in ResourceLock checks.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
New Features
Tests
Chores