feat: Enhance EIP-712 support with token versioning#294
feat: Enhance EIP-712 support with token versioning#294nahimterrazas wants to merge 3 commits intomainfrom
Conversation
nahimterrazas
requested review from
NicoMolinaOZ,
pepebndc and
shahnami
as code owners
📝 WalkthroughWalkthroughAdds EIP-712 version resolution and propagation: retrieves a token's EIP-712 version via EIP-5267, version() call, or a known mapping (default "1"), integrates the version into EIP-3009 domain construction and EIP-712 digest reconstruction, and expands tests for these flows. Changes
Sequence DiagramsequenceDiagram
actor Client
participant TokenContract
participant Registry
participant DomainBuilder
Client->>TokenContract: call EIP-5267 domain()
alt EIP-5267 returns domain+version
TokenContract-->>Client: domain (with version)
else EIP-5267 fails
Client->>TokenContract: call version()
alt version() returns non-empty
TokenContract-->>Client: version string
else version() empty/fails
Client->>Registry: lookup known token version
alt Registry has mapping
Registry-->>Client: known version
else no mapping
Client-->>Client: use default "1"
end
end
end
Client->>DomainBuilder: build EIP-3009 domain (includes version)
DomainBuilder->>DomainBuilder: compute domain hash (version-aware if present)
DomainBuilder-->>Client: return domain object/hash
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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: 2
🤖 Fix all issues with AI agents
In `@crates/solver-service/src/apis/quote/generation.rs`:
- Around line 1300-1352: In get_token_eip712_version_via_eip5267, inspect the
returned eip712DomainCall::abi_decode_returns() result's fields before using
domain.version: parse domain.fields (bytes1) and check the version bit (0x02) is
set (e.g., (domain.fields[0] & 0x02) != 0); if the bit is not set, return an
Err(QuoteError::InvalidRequest(...)) indicating the token EIP-712 domain does
not include the version field, otherwise proceed to validate/trimming
domain.version as currently done.
- Around line 1213-1224: The domain version resolution must be validated against
the on-chain DOMAIN_SEPARATOR to avoid producing domains that mismatch on-chain
verification: after calling get_token_eip712_version(&alloy_token_address,
chain_id) and before returning the JSON domain (the block that builds the domain
object with "name","version","chainId","verifyingContract"), fetch the token's
DOMAIN_SEPARATOR from chain and compute the expected domain separator using the
resolved version; if they differ, fallback to retrying domain construction
without the "version" field (or log and return an explicit error) so we don't
silently return a domain that will produce signatures that fail on-chain; update
the code around the token version resolution and domain construction to perform
this compare and handle the mismatch (reference get_token_eip712_version, the
domain JSON construction, and the on-chain DOMAIN_SEPARATOR check).
🧹 Nitpick comments (1)
crates/solver-service/src/apis/quote/generation.rs (1)
1399-1420: Hardcoded known token registry — maintainability consideration.This offline registry covers known USDC deployments and will need manual updates for new chain deployments. The
to_ascii_lowercase()call on line 1405 is redundant sinceformat!("0x{:040x}", ...)already produces lowercase, but it's harmless.Consider adding a code comment listing the token name (e.g.,
// USDC) next to the chain/address pairs for future maintainers, since the addresses alone aren't self-documenting.
| let token_version = self | ||
| .get_token_eip712_version(&alloy_token_address, chain_id) | ||
| .await; | ||
|
|
||
| // Build domain object similar to TheCompact structure (without pre-computed domainSeparator) | ||
| // The client will compute the domainSeparator using these fields | ||
| Ok(serde_json::json!({ | ||
| "name": token_name, | ||
| "version": token_version, | ||
| "chainId": chain_id.to_string(), | ||
| "verifyingContract": format!("0x{:040x}", alloy_token_address) | ||
| })) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where domain_separator is used in generation.rs and related files
rg -n "domain_separator" crates/solver-service/src/apis/quote/generation.rs -C 3Repository: openintentsframework/oif-solver
Length of output: 2005
🏁 Script executed:
#!/bin/bash
# Check if there's any validation comparing computed domain hash vs fetched domain_separator
rg -n "domain_separator\|domain_hash" crates/solver-service/src/apis/quote/ -A 2 -B 2 --type=rust | head -100Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look for the get_eip3009_domain_separator function definition
rg -n "fn get_eip3009_domain_separator" crates/solver-service/src/apis/quote/generation.rs -A 20Repository: openintentsframework/oif-solver
Length of output: 767
🏁 Script executed:
#!/bin/bash
# Search for where the domain is actually used during signature generation/verification
rg -n "\.domain\|payload\.domain" crates/solver-service/src/apis/quote/generation.rs -B 2 -A 2 | head -80Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or documentation about EIP-3009 domain validation
rg -n "EIP.?3009\|eip.?3009" crates/solver-service/src/apis/quote/generation.rs -B 1 -A 3 | head -100Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check the build_eip3009_eip712_types to see the fixed domain structure
rg -n "fn build_eip3009_eip712_types" crates/solver-service/src/apis/quote/generation.rs -A 40 | head -60Repository: openintentsframework/oif-solver
Length of output: 1681
🏁 Script executed:
#!/bin/bash
# Look at where the domain is actually used in payload construction
rg -n "\.domain\|payload" crates/solver-service/src/apis/quote/generation.rs -B 1 -A 1 | grep -A 2 -B 2 "eip3009\|domain"Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check the known USDC versions and see if there's any validation
rg -n "get_known_eip3009_token_version\|known.*version" crates/solver-service/src/apis/quote/generation.rs -A 25 | head -80Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look at eip712.rs to see how domain_separator is used during reconstruction
rg -n "reconstruct_eip3009\|domain_separator" crates/solver-service/src/eip712.rs -B 2 -A 5 | head -100Repository: openintentsframework/oif-solver
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check if there's any logic that validates domain hash matches
rg -n "compute.*domain\|validate.*domain\|hash.*domain" crates/solver-service/src/ -i --type=rust | head -30Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Find the actual domain construction for EIP-3009
rg -n "build_eip3009_domain_object" crates/solver-service/src/apis/quote/generation.rs -A 20 | head -50Repository: openintentsframework/oif-solver
Length of output: 2073
🏁 Script executed:
#!/bin/bash
# Search for known USDC versions - look around line 1399
sed -n '1399,1430p' crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 1309
🏁 Script executed:
#!/bin/bash
# Look at the complete get_known_eip3009_token_version function
sed -n '1399,1425p' crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 1141
🏁 Script executed:
#!/bin/bash
# Find where the payload with domain and types is returned
rg -n "types.*domain\|\"types\"\|\"domain\"" crates/solver-service/src/apis/quote/generation.rs -B 2 -A 2 | head -80Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check if there's any code that computes the domain separator hash from the domain object
rg -n "domain_separator\|hash" crates/solver-service/src/apis/quote/generation.rs | head -50Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look for the EIP712 domain hash computation logic - check if it exists anywhere
find crates/solver-service/src -name "*.rs" -type f | xargs rg -l "domain.*hash\|EIP.?712.*domain" | head -10Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look at get_token_eip712_version to understand the complete fallback logic
sed -n '1267,1298p' crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 817
🏁 Script executed:
#!/bin/bash
# See if there's any code that uses the computed domain hash and compares it
rg -n "compute\|hash\|verify" crates/solver-service/src/apis/quote/generation.rs | grep -i domainRepository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check the complete flow - look at where domain and domain_separator are both present
sed -n '600,660p' crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 2490
🏁 Script executed:
#!/bin/bash
# Look for any validation or comparison of domain hash vs fetched separator
rg -n "domain_separator.*==" crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Verify: does metadata include both domain and domain_separator?
sed -n '642,680p' crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 1897
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that validate domain_separator vs computed domain
rg -n "test.*domain\|domain.*test" crates/solver-service/src/apis/quote/generation.rs | grep -i separatorRepository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look at the test for build_eip3009_domain_object to see what it expects
sed -n '3049,3080p' crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 947
🏁 Script executed:
#!/bin/bash
# Check test coverage for domain version fallback scenarios
rg -n "test_get_token_eip712_version" crates/solver-service/src/apis/quote/generation.rs -A 30 | head -80Repository: openintentsframework/oif-solver
Length of output: 2905
🏁 Script executed:
#!/bin/bash
# Look for any error handling or validation related to domain mismatch
rg -n "version.*mismatch\|domain.*mismatch\|mismatch" crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Search for client-side domain validation or signature generation logic
find crates -name "*.rs" -type f | xargs rg -l "domain.*hash\|sign.*domain" | head -10Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Let's look at the complete flow to understand the signature generation and metadata usage
sed -n '595,700p' crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 4040
🏁 Script executed:
#!/bin/bash
# Check if there's any code path that verifies domain hash matches domain_separator
rg -n "DOMAIN_SEPARATOR\|domain_separator" crates/solver-service/src/apis/quote/generation.rs -B 3 -A 3Repository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look for any comments about domain validation or known issues
rg -n "TODO\|FIXME\|XXX\|BUG\|domain.*version\|version.*domain" crates/solver-service/src/apis/quote/generation.rs -iRepository: openintentsframework/oif-solver
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check what happens in the payload - does it include both domain and domain_separator?
sed -n '685,695p' crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 321
🏁 Script executed:
#!/bin/bash
# Look for any client-side code that uses the domain object to compute domain hash
find crates -name "*.rs" -type f | xargs rg -l "domain.*hash\|compute.*domain" 2>/dev/nullRepository: openintentsframework/oif-solver
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check if OrderPayload and metadata are both returned to the client
rg -n "OrderPayload\|OifOrder" crates/solver-service/src/apis/quote/generation.rs | head -20Repository: openintentsframework/oif-solver
Length of output: 57
Domain version resolution works well for common tokens but lacks validation for edge cases.
The version resolution uses solid fallbacks (EIP-5267 → version() call → known registry → default "1"), and all known USDC deployments are correctly registered with version "2". However, for unknown tokens without EIP-5267 or version() support whose on-chain DOMAIN_SEPARATOR was computed without a version field, defaulting to "1" and unconditionally including version in the domain could cause signatures that pass server-side verification to fail on-chain.
Consider adding a validation step that compares the domain hash computed from the resolved version against the fetched DOMAIN_SEPARATOR. If they don't match, you could either retry without version or log an explicit error, preventing silent signature failures.
🤖 Prompt for AI Agents
In `@crates/solver-service/src/apis/quote/generation.rs` around lines 1213 - 1224,
The domain version resolution must be validated against the on-chain
DOMAIN_SEPARATOR to avoid producing domains that mismatch on-chain verification:
after calling get_token_eip712_version(&alloy_token_address, chain_id) and
before returning the JSON domain (the block that builds the domain object with
"name","version","chainId","verifyingContract"), fetch the token's
DOMAIN_SEPARATOR from chain and compute the expected domain separator using the
resolved version; if they differ, fallback to retrying domain construction
without the "version" field (or log and return an explicit error) so we don't
silently return a domain that will produce signatures that fail on-chain; update
the code around the token version resolution and domain construction to perform
this compare and handle the mismatch (reference get_token_eip712_version, the
domain JSON construction, and the on-chain DOMAIN_SEPARATOR check).
There was a problem hiding this comment.
yeah, perhaps this makes sense, wdyt @shahnami, still with that we managed most cases
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| /// Resolve token EIP-712 version from EIP-5267 `eip712Domain()`. | ||
| async fn get_token_eip712_version_via_eip5267( | ||
| &self, | ||
| token_address: &alloy_primitives::Address, | ||
| chain_id: u64, | ||
| ) -> Result<String, QuoteError> { | ||
| use alloy_sol_types::{sol, SolCall}; | ||
|
|
||
| sol! { | ||
| function eip712Domain() external view returns ( | ||
| bytes1 fields, | ||
| string name, | ||
| string version, | ||
| uint256 chainId, | ||
| address verifyingContract, | ||
| bytes32 salt, | ||
| uint256[] extensions | ||
| ); | ||
| } | ||
|
|
||
| let tx = solver_types::Transaction { | ||
| to: Some(solver_types::Address(token_address.0.to_vec())), | ||
| data: eip712DomainCall {}.abi_encode(), | ||
| value: alloy_primitives::U256::ZERO, | ||
| chain_id, | ||
| nonce: None, | ||
| gas_limit: None, | ||
| gas_price: None, | ||
| max_fee_per_gas: None, | ||
| max_priority_fee_per_gas: None, | ||
| }; | ||
|
|
||
| let result = self | ||
| .delivery_service | ||
| .contract_call(chain_id, tx) | ||
| .await | ||
| .map_err(|e| { | ||
| QuoteError::InvalidRequest(format!("Failed to get token EIP-712 domain: {e}")) | ||
| })?; | ||
|
|
||
| let domain = eip712DomainCall::abi_decode_returns(&result).map_err(|e| { | ||
| QuoteError::InvalidRequest(format!("Failed to decode token EIP-712 domain: {e}")) | ||
| })?; | ||
| let version = domain.version; | ||
|
|
||
| if version.trim().is_empty() { | ||
| return Err(QuoteError::InvalidRequest( | ||
| "Token EIP-712 version is empty".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| Ok(version) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
EIP-5267 fields bitmask eip712Domain specification
💡 Result:
In EIP-5267, the fields return value is a bitmask indicating which optional EIP-712 EIP712Domain fields are present. Bit i is 1 iff domain field i is present, with bits read least-significant to most-significant and indices matching the EIP-712 field order (the same order used in the eip712Domain() return tuple). [1]
Bit positions (EIP-5267 ⇄ EIP-712 EIP712Domain)
| Bit | Value | Field (EIP-712) |
|---|---|---|
| 0 | 0x01 |
name |
| 1 | 0x02 |
version |
| 2 | 0x04 |
chainId |
| 3 | 0x08 |
verifyingContract |
| 4 | 0x10 |
salt |
EIP-712 defines the EIP712Domain struct as having one or more of these optional fields (name, version, chainId, verifyingContract, salt), and the domain separator is hashStruct(eip712Domain). [2]
Example
If a domain includes name, chainId, verifyingContract (but not version or salt), then:
- bits set: 0, 2, 3 → binary
0b01101→ hex0x0d[1]
Sources: EIP-5267 [1], EIP-712 [2].
🏁 Script executed:
cd crates/solver-service && find . -type f -name "*.rs" | head -20Repository: openintentsframework/oif-solver
Length of output: 589
🏁 Script executed:
# Check the code around line 1300-1352 to understand the fields type
sed -n '1300,1352p' src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 146
🏁 Script executed:
# Look for similar eip712Domain usages in the codebase to see how fields is accessed elsewhere
rg "eip712Domain\|fields.*0x" -A 3 -B 3 --type rsRepository: openintentsframework/oif-solver
Length of output: 101
🏁 Script executed:
# Read the relevant section from the correct path
sed -n '1300,1352p' crates/solver-service/src/apis/quote/generation.rsRepository: openintentsframework/oif-solver
Length of output: 1424
🏁 Script executed:
# Search for eip712Domain usage in the repository
rg "eip712Domain" -A 5 -B 2 crates/solver-service/src/apis/quote/Repository: openintentsframework/oif-solver
Length of output: 9214
EIP-5267 fields bitmask must be checked before using version.
Per EIP-5267, the fields return value is a bitmask where bit 1 (0x02) indicates whether version is part of the domain. The current code ignores fields and always uses the returned version string. If a token's fields value excludes the version bit but still returns a non-empty default for the version string, this could resolve to an incorrect version.
Add a check for the fields bitmask before accepting the version:
Proposed fix
let domain = eip712DomainCall::abi_decode_returns(&result).map_err(|e| {
QuoteError::InvalidRequest(format!("Failed to decode token EIP-712 domain: {e}"))
})?;
+ // Check EIP-5267 fields bitmask: bit 1 (0x02) indicates version is part of the domain
+ if domain.fields[0] & 0x02 == 0 {
+ return Err(QuoteError::InvalidRequest(
+ "Token EIP-712 domain does not include version field (per EIP-5267 fields bitmask)".to_string(),
+ ));
+ }
let version = domain.version;📝 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.
| /// Resolve token EIP-712 version from EIP-5267 `eip712Domain()`. | |
| async fn get_token_eip712_version_via_eip5267( | |
| &self, | |
| token_address: &alloy_primitives::Address, | |
| chain_id: u64, | |
| ) -> Result<String, QuoteError> { | |
| use alloy_sol_types::{sol, SolCall}; | |
| sol! { | |
| function eip712Domain() external view returns ( | |
| bytes1 fields, | |
| string name, | |
| string version, | |
| uint256 chainId, | |
| address verifyingContract, | |
| bytes32 salt, | |
| uint256[] extensions | |
| ); | |
| } | |
| let tx = solver_types::Transaction { | |
| to: Some(solver_types::Address(token_address.0.to_vec())), | |
| data: eip712DomainCall {}.abi_encode(), | |
| value: alloy_primitives::U256::ZERO, | |
| chain_id, | |
| nonce: None, | |
| gas_limit: None, | |
| gas_price: None, | |
| max_fee_per_gas: None, | |
| max_priority_fee_per_gas: None, | |
| }; | |
| let result = self | |
| .delivery_service | |
| .contract_call(chain_id, tx) | |
| .await | |
| .map_err(|e| { | |
| QuoteError::InvalidRequest(format!("Failed to get token EIP-712 domain: {e}")) | |
| })?; | |
| let domain = eip712DomainCall::abi_decode_returns(&result).map_err(|e| { | |
| QuoteError::InvalidRequest(format!("Failed to decode token EIP-712 domain: {e}")) | |
| })?; | |
| let version = domain.version; | |
| if version.trim().is_empty() { | |
| return Err(QuoteError::InvalidRequest( | |
| "Token EIP-712 version is empty".to_string(), | |
| )); | |
| } | |
| Ok(version) | |
| } | |
| /// Resolve token EIP-712 version from EIP-5267 `eip712Domain()`. | |
| async fn get_token_eip712_version_via_eip5267( | |
| &self, | |
| token_address: &alloy_primitives::Address, | |
| chain_id: u64, | |
| ) -> Result<String, QuoteError> { | |
| use alloy_sol_types::{sol, SolCall}; | |
| sol! { | |
| function eip712Domain() external view returns ( | |
| bytes1 fields, | |
| string name, | |
| string version, | |
| uint256 chainId, | |
| address verifyingContract, | |
| bytes32 salt, | |
| uint256[] extensions | |
| ); | |
| } | |
| let tx = solver_types::Transaction { | |
| to: Some(solver_types::Address(token_address.0.to_vec())), | |
| data: eip712DomainCall {}.abi_encode(), | |
| value: alloy_primitives::U256::ZERO, | |
| chain_id, | |
| nonce: None, | |
| gas_limit: None, | |
| gas_price: None, | |
| max_fee_per_gas: None, | |
| max_priority_fee_per_gas: None, | |
| }; | |
| let result = self | |
| .delivery_service | |
| .contract_call(chain_id, tx) | |
| .await | |
| .map_err(|e| { | |
| QuoteError::InvalidRequest(format!("Failed to get token EIP-712 domain: {e}")) | |
| })?; | |
| let domain = eip712DomainCall::abi_decode_returns(&result).map_err(|e| { | |
| QuoteError::InvalidRequest(format!("Failed to decode token EIP-712 domain: {e}")) | |
| })?; | |
| // Check EIP-5267 fields bitmask: bit 1 (0x02) indicates version is part of the domain | |
| if domain.fields[0] & 0x02 == 0 { | |
| return Err(QuoteError::InvalidRequest( | |
| "Token EIP-712 domain does not include version field (per EIP-5267 fields bitmask)".to_string(), | |
| )); | |
| } | |
| let version = domain.version; | |
| if version.trim().is_empty() { | |
| return Err(QuoteError::InvalidRequest( | |
| "Token EIP-712 version is empty".to_string(), | |
| )); | |
| } | |
| Ok(version) | |
| } |
🤖 Prompt for AI Agents
In `@crates/solver-service/src/apis/quote/generation.rs` around lines 1300 - 1352,
In get_token_eip712_version_via_eip5267, inspect the returned
eip712DomainCall::abi_decode_returns() result's fields before using
domain.version: parse domain.fields (bytes1) and check the version bit (0x02) is
set (e.g., (domain.fields[0] & 0x02) != 0); if the bit is not set, return an
Err(QuoteError::InvalidRequest(...)) indicating the token EIP-712 domain does
not include the version field, otherwise proceed to validate/trimming
domain.version as currently done.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-service/src/apis/quote/generation.rs`:
- Around line 3083-3092: The tests test_get_token_eip712_version_uses_eip5267
and test_get_token_eip712_version_via_eip5267_empty_version construct an
eip712DomainReturn with fields set to FixedBytes::<1>::from([0x1]) which only
marks the name bit and omits the version per EIP-5267; update the fields bitmask
in those test instances of eip712DomainReturn to include name, version, chainId,
and verifyingContract (use 0x0f) so the mocked EIP-5267 response correctly
indicates the version is present and the assertions against version "2" will be
valid.
---
Duplicate comments:
In `@crates/solver-service/src/apis/quote/generation.rs`:
- Around line 1340-1351: Before using domain.version (decoded via
eip712DomainCall::abi_decode_returns), check the EIP-5267 fields bitmask:
inspect domain.fields[0] & 0x02 to confirm the version field is present; if the
bit is not set, return an appropriate QuoteError::InvalidRequest (or treat as
absent) instead of reading domain.version, otherwise proceed to validate
version.trim().is_empty() as before. Ensure you reference domain.fields,
domain.version, eip712DomainCall::abi_decode_returns, and
QuoteError::InvalidRequest when locating and updating the code.
- Around line 1213-1224: The code unconditionally inserts the token "version"
from get_token_eip712_version into the reconstructed domain JSON (in
generation.rs where the domain object is built), which can mismatch tokens whose
on-chain DOMAIN_SEPARATOR was computed without a version. Change the logic to
query the token's on-chain DOMAIN_SEPARATOR (call DOMAIN_SEPARATOR()), then
compute the local domain separator twice: once including the obtained version
from get_token_eip712_version and once without a version; compare each computed
separator to the on-chain value and only include the version in the returned
JSON if the version-inclusive computation matches the on-chain
DOMAIN_SEPARATOR—otherwise omit the version (or use the variant that matches
on-chain). Use the existing get_token_eip712_version helper and the domain
construction code around the serde_json::json! block to implement this
validation and selection.
| let eip712_response = | ||
| Bytes::from(eip712DomainCall::abi_encode_returns(&eip712DomainReturn { | ||
| fields: FixedBytes::<1>::from([0x1]), | ||
| name: "USD Coin".to_string(), | ||
| version: "2".to_string(), | ||
| chainId: U256::from(1u64), | ||
| verifyingContract: AlloyAddress::from([0x11; 20]), | ||
| salt: FixedBytes::<32>::from([0u8; 32]), | ||
| extensions: Vec::<U256>::new(), | ||
| })); |
There was a problem hiding this comment.
Incorrect EIP-5267 fields bitmask in two tests — version bit (0x02) is not set.
Both test_get_token_eip712_version_uses_eip5267 (line 3085) and test_get_token_eip712_version_via_eip5267_empty_version (line 3240) use fields: FixedBytes::<1>::from([0x1]). Per EIP-5267:
| Bit | Mask | Field |
|---|---|---|
| 0 | 0x01 |
name |
| 1 | 0x02 |
version |
| 2 | 0x04 |
chainId |
| 3 | 0x08 |
verifyingContract |
0x01 has only the name bit set, meaning the domain does not include a version field per spec. A response representing a standard token domain (name + version + chainId + verifyingContract) should use 0x0f.
As written, test_get_token_eip712_version_uses_eip5267 asserts version "2" is returned from a response where the fields bitmask says version is absent — which is only true because the fields check is not implemented. If the fields check from the previous review comment is added, this test will fail.
🔧 Suggested fix for both tests
- fields: FixedBytes::<1>::from([0x1]),
+ fields: FixedBytes::<1>::from([0x0f]),Also applies to: 3237-3248
🤖 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 3083 - 3092,
The tests test_get_token_eip712_version_uses_eip5267 and
test_get_token_eip712_version_via_eip5267_empty_version construct an
eip712DomainReturn with fields set to FixedBytes::<1>::from([0x1]) which only
marks the name bit and omits the version per EIP-5267; update the fields bitmask
in those test instances of eip712DomainReturn to include name, version, chainId,
and verifyingContract (use 0x0f) so the mocked EIP-5267 response correctly
indicates the version is present and the assertions against version "2" will be
valid.
2 participants
Summary
Testing Process
Checklist
Summary by CodeRabbit
New Features
Tests
solves #286