Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces EIP-712 signing support to the wallet contract by adding a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Wallet/Client
participant Contract as Wallet Contract
participant EIP712 as EIP712 Module
participant Secp256k1 as Secp256k1
Client->>Contract: verify(msg, public_key, signature)
Contract->>EIP712: Parse signature as SignedEip712Payload
EIP712->>EIP712: Deserialize payload into message type M
EIP712->>EIP712: Verify message equality
EIP712->>Secp256k1: Recover public key from signature
Secp256k1-->>EIP712: Recovered public key
EIP712->>EIP712: Compare recovered key with expected key
EIP712-->>Contract: Verification result (bool)
Contract-->>Client: Signature valid/invalid
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🤖 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/signatures/eip712/src/lib.rs`:
- Around line 109-110: The verify method currently forwards the signature's
recovery byte unmodified to Secp256k1::verify (which calls env::ecrecover) but
Ethereum-style v values (27/28) must be normalized to secp256k1 format (0/1);
update the verify logic in the impl where AsCurve<Secp256k1> deserialized
signature is passed to Secp256k1::verify so that you read the signature's
recovery byte v, map 27->0 and 28->1 (leave 0/1 unchanged) before calling
Secp256k1::verify, and add a unit test using the verify method that asserts
signatures with v=0/1 and v=27/28 both validate correctly.
🪄 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: 5bbe8f3b-8c4b-43d6-9f64-4304ccb04c06
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcontracts/wallet/Cargo.tomlcontracts/wallet/src/contract/impl_.rscontracts/wallet/src/signature/eip712.rscontracts/wallet/src/signature/mod.rscrates/signatures/eip712/Cargo.tomlcrates/signatures/eip712/src/lib.rs
| Secp256k1::verify(&self.signature, &self.payload.hash(), &()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm secp256k1 verifier expectation in current code.
rg -n -C3 'v ∈ \{0, 1\}|rollback|fn verify\(' crates/crypto/src/curve/secp256k1.rs
# 2) Check whether any serde/parser path already normalizes recovery byte values.
rg -n -C3 'AsCurve|secp256k1|recovery|normalize|27|28' crates/crypto crates/signatures/eip712 contracts/wallet
# 3) Check if there are tests covering v=27/28 compatibility.
rg -n -C3 'eip712|secp256k1|recover|27|28|1b|1c' crates/signatures/eip712 contracts/walletRepository: near/intents
Length of output: 36699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check AsCurve deserializer implementation
echo "=== AsCurve Deserializer Implementation ==="
rg -A 15 'impl.*DeserializeAs.*AsCurve' crates/crypto/src/serde.rs
# Check if there are any integration tests with Ethereum signatures
echo -e "\n=== EIP712 Tests ==="
rg -B 2 -A 10 'fn test.*eip712|fn test.*signature|#\[test\]' crates/signatures/eip712/src/lib.rs | head -60
# Check the actual signature type and how it's used
echo -e "\n=== Secp256k1 Signature Type ==="
rg -B 2 -A 5 'type Signature.*=' crates/crypto/src/curve/secp256k1.rsRepository: near/intents
Length of output: 2799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there are signature verification tests in the wallet
echo "=== Wallet EIP712 signature verification tests ==="
rg -B 3 -A 10 'verify|test.*eip712' contracts/wallet/src/signature/eip712.rs
# Check if there are any test fixtures with real signatures
echo -e "\n=== Test fixtures or examples with recovery byte ==="
rg -n '27|28|0x[a-f0-9]{130}' crates/signatures/eip712/src/lib.rs contracts/wallet/src/
# Verify the full verify method chain
echo -e "\n=== Full verify method in secp256k1 ==="
sed -n '33,50p' crates/crypto/src/curve/secp256k1.rs
</parameter>Repository: near/intents
Length of output: 1758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if env::ecrecover handles normalization
echo "=== env::ecrecover signature ==="
rg -B 2 -A 5 'fn ecrecover' --type rust
# Check if there's any normalization in the wallet or elsewhere
echo -e "\n=== Search for any v normalization logic ==="
rg -n 'v.*-.*27|27.*-.*v|normalize.*recovery|recovery.*normalize' crates/ contracts/
# Look for actual signature parsing or creation
echo -e "\n=== SignedEip712Payload structure and deserialization ==="
rg -B 5 -A 10 'struct SignedEip712Payload' crates/signatures/eip712/src/lib.rsRepository: near/intents
Length of output: 89
🏁 Script executed:
#!/bin/bash
set -euo sapipefail
# Look for actual signature parsing or creation
echo "=== SignedEip712Payload structure ==="
sed -n '85,110p' crates/signatures/eip712/src/lib.rs
# Check if there's normalization anywhere in the codebase
echo -e "\n=== Search for recovery byte normalization ==="
rg 'recovery|normalize' crates/ contracts/ --type rust
# Look at how the signature deserialization happens
echo -e "\n=== AsCurve parse_base58 implementation ==="
rg -B 2 -A 10 'fn parse_base58' crates/crypto/src/Repository: near/intents
Length of output: 112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at SignedEip712Payload structure
echo "=== SignedEip712Payload structure ==="
sed -n '85,110p' crates/signatures/eip712/src/lib.rs
# Check if there's normalization anywhere in the codebase
echo -e "\n=== Search for recovery byte normalization ==="
rg 'recovery|normalize' crates/ contracts/ --type rust
# Look at how the signature deserialization happens
echo -e "\n=== AsCurve parse_base58 implementation ==="
rg -B 2 -A 10 'fn parse_base58' crates/crypto/src/Repository: near/intents
Length of output: 2469
Normalize recovery byte v from Ethereum format (27/28) to secp256k1 format (0/1) in verify method.
The verify() method passes the recovery byte directly to Secp256k1::verify, which forwards it to env::ecrecover. Per the documentation in crates/crypto/src/curve/secp256k1.rs (line 20), the recovery byte must be in {0, 1}. However, Ethereum signers commonly output v ∈ {27, 28}, causing valid signatures to be rejected.
The signature is deserialized via AsCurve<Secp256k1> which only base58-decodes without normalization. Add normalization at the verify boundary:
Proposed fix
impl SignedPayload for SignedEip712Payload {
type PublicKey = <Secp256k1 as Curve>::PublicKey;
#[inline]
fn verify(&self) -> Option<Self::PublicKey> {
- Secp256k1::verify(&self.signature, &self.payload.hash(), &())
+ let mut signature = self.signature;
+ signature[64] = match signature[64] {
+ 0 | 1 => signature[64],
+ 27 | 28 => signature[64] - 27,
+ _ => return None,
+ };
+
+ Secp256k1::verify(&signature, &self.payload.hash(), &())
}
}Also add a test vector verifying both v=0/1 and v=27/28 inputs work correctly.
📝 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.
| Secp256k1::verify(&self.signature, &self.payload.hash(), &()) | |
| } | |
| let mut signature = self.signature; | |
| signature[64] = match signature[64] { | |
| 0 | 1 => signature[64], | |
| 27 | 28 => signature[64] - 27, | |
| _ => return None, | |
| }; | |
| Secp256k1::verify(&signature, &self.payload.hash(), &()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/signatures/eip712/src/lib.rs` around lines 109 - 110, The verify
method currently forwards the signature's recovery byte unmodified to
Secp256k1::verify (which calls env::ecrecover) but Ethereum-style v values
(27/28) must be normalized to secp256k1 format (0/1); update the verify logic in
the impl where AsCurve<Secp256k1> deserialized signature is passed to
Secp256k1::verify so that you read the signature's recovery byte v, map 27->0
and 28->1 (leave 0/1 unchanged) before calling Secp256k1::verify, and add a unit
test using the verify method that asserts signatures with v=0/1 and v=27/28 both
validate correctly.
frol-ai
force-pushed
the
feat/wallet-eip712
branch
from
b44fcd8 to
a17361b
Compare
Add EIP-712 (eth_signTypedData_v4) signature verification to the
wallet-contract, following the same pattern as the erc191 variant.
EIP-712 typed data structure:
EIP712Domain(string name, string version)
name = "NEAR Wallet Contract", version = "1"
WalletMessage(string payload)
payload = JSON-serialized RequestMessage
The signing flow:
1. Client JSON-serializes the RequestMessage
2. Client signs via eth_signTypedData_v4 with the above typed data
3. Contract reconstructs the EIP-712 hash and uses ecrecover to
verify the secp256k1 public key matches
New feature flag: `eip712` (enables `defuse-crypto/secp256k1` + `defuse-eip712`)
frol-ai
force-pushed
the
feat/wallet-eip712
branch
from
a17361b to
3f4555a
Compare
2 participants
Add EIP-712 (
eth_signTypedData_v4) signature verification to the wallet-contract, following the same pattern as the erc191 variant (#242).EIP-712 typed data structure
Signing flow
RequestMessageeth_signTypedData_v4(e.g. MetaMask, WalletConnect) with the above typed datakeccak256("\x19\x01" || domainSeparator || hashStruct(WalletMessage))ecrecoverto verify the secp256k1 public key matchesChanges
crates/signatures/eip712/— EIP-712 typed data hashing and signature verification (4 tests)eip712feature flag,signature/eip712.rssigning standard, contract variant wiring[package.metadata.near.reproducible_build.variant.eip712]Motivation
EIP-712 provides structured data signing that wallets like MetaMask display in a human-readable format, unlike ERC-191 which shows raw bytes. This enables a better UX for Ethereum wallet users interacting with NEAR wallet-contracts via WalletConnect or injected providers.
Summary by CodeRabbit