feat(wallet): erc191 variant#242
Conversation
📝 WalkthroughWalkthroughAdds optional ERC-191 signing support to the wallet: new feature flag and dependency, feature-gated ERC-191 signing implementation and module, contract wiring for the new signing standard, and reproducible-build metadata variant for ERC-191. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Contract as Wallet Contract
participant Lib as defuse-erc191 / secp256k1
Client->>Contract: submit signed payload (SignedErc191 JSON)
Contract->>Contract: parse SignedErc191, deserialize payload -> msg
Contract->>Contract: compare msg == expected message
Contract->>Lib: recover public key from signature
Lib-->>Contract: recovered public key
Contract->>Contract: compare recovered pk == provided Secp256k1PublicKey
alt match
Contract-->>Client: verification success
else mismatch
Contract-->>Client: verification failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
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 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 |
![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 `@contracts/wallet/src/signature/erc191.rs`:
- Around line 23-29: The code currently deserializes signature.payload.0 into M
and compares msg to &payload but doesn't detect extra unknown fields; to enforce
strict equivalence, first parse signature.payload.0 into a serde_json::Value
(orig_value), then deserialize that same JSON into M (payload) as you already
do, then convert the deserialized payload back to a serde_json::Value
(reconstructed = serde_json::to_value(&payload)), and finally compare orig_value
== reconstructed; if they differ return false (in addition to the existing msg
!= &payload check). This ensures the signed JSON contains exactly the expected
fields and no extras while keeping the existing equality check between msg and
payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06e81ea7-e0a8-4b2c-bf6b-9f1a51f72fbb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
contracts/wallet/Cargo.tomlcontracts/wallet/src/contract/impl_.rscontracts/wallet/src/signature/erc191.rscontracts/wallet/src/signature/mod.rscrates/signatures/erc191/Cargo.toml
| let Ok(payload) = serde_json::from_str::<M>(&signature.payload.0) else { | ||
| return false; | ||
| }; | ||
|
|
||
| // check that signed the same message | ||
| if msg != &payload { | ||
| return false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect current ERC-191 verification path
nl -ba contracts/wallet/src/signature/erc191.rs | sed -n '17,33p'
# Check whether RequestMessage opts into strict unknown-field rejection
rg -n 'deny_unknown_fields|struct RequestMessage|near\(serializers = \[borsh, json\]\)' contracts/wallet/src/signature/mod.rs -C2Repository: near/intents
Length of output: 98
🏁 Script executed:
# Inspect current ERC-191 verification path - use cat with line numbers via sed
cat -n contracts/wallet/src/signature/erc191.rs | sed -n '17,40p'Repository: near/intents
Length of output: 961
🏁 Script executed:
# Search for RequestMessage struct definition and deny_unknown_fields
rg -n 'deny_unknown_fields|struct RequestMessage' contracts/wallet/src/signature/ -A 5 -B 2Repository: near/intents
Length of output: 620
🏁 Script executed:
# Get broader context of the erc191.rs file structure
cat -n contracts/wallet/src/signature/erc191.rs | head -50Repository: near/intents
Length of output: 1448
🏁 Script executed:
# Check for any serde attributes in the RequestMessage definition
rg -n 'RequestMessage' contracts/wallet/src/signature/ -B 10 -A 10 | head -100Repository: near/intents
Length of output: 2859
🏁 Script executed:
# Find usages of Erc191::verify or erc191 module
rg -n 'Erc191|erc191::verify' contracts/wallet/src/ -B 2 -A 2Repository: near/intents
Length of output: 1838
🏁 Script executed:
# Check if there are tests for the verify function
rg -n '#\[test\]|#\[cfg\(test\)\]' contracts/wallet/src/signature/ -A 10 | grep -E '(test|verify|erc191)'Repository: near/intents
Length of output: 38
🏁 Script executed:
# Look at the SignedErc191Payload to understand what payload.0 is
rg -n 'SignedErc191Payload|payload.*0' contracts/wallet/src/signature/erc191.rs -B 3 -A 3Repository: near/intents
Length of output: 734
🏁 Script executed:
# Check the defuse_erc191 dependency to understand the payload structure
rg -n 'struct SignedErc191Payload|pub payload' contracts/wallet/src/signature/erc191.rs -B 5 -A 10Repository: near/intents
Length of output: 38
🏁 Script executed:
# Search for where Erc191::verify is actually called
rg -n 'SigningStandard.*verify|\.verify\(' contracts/wallet/src/ -B 2 -A 2 | head -50Repository: near/intents
Length of output: 1122
🏁 Script executed:
# Check Cargo.toml to understand defuse_erc191 dependency
cat Cargo.toml | grep -A 5 -B 5 defuseRepository: near/intents
Length of output: 2284
🏁 Script executed:
# Look for actual test files
fd -e test.rs -e _test.rs contracts/wallet/Repository: near/intents
Length of output: 429
🏁 Script executed:
# Check if there are integration tests
fd -e rs tests/ 2>/dev/null | head -20Repository: near/intents
Length of output: 38
🏁 Script executed:
# Search for how RequestMessage is used with verify
rg -n 'RequestMessage.*verify|verify.*RequestMessage' contracts/wallet/src/ -B 5 -A 5Repository: near/intents
Length of output: 38
🏁 Script executed:
# Search for the defuse_erc191 crate to understand SignedErc191Payload
find . -path "*/crates/signatures/erc191*" -name "*.rs" | head -10Repository: near/intents
Length of output: 93
🏁 Script executed:
# Check the erc191 crate structure
ls -la crates/signatures/erc191/ 2>/dev/null || echo "Not found at that path"Repository: near/intents
Length of output: 275
🏁 Script executed:
# Search more broadly
fd -type f -name "lib.rs" | xargs grep -l "SignedErc191Payload" 2>/dev/nullRepository: near/intents
Length of output: 228
🏁 Script executed:
# Look at what payload.0 contains (likely a String)
rg -n "pub struct.*Payload|pub payload" crates/ -A 5 | grep -A 5 "erc191\|payload"Repository: near/intents
Length of output: 4259
🏁 Script executed:
# Check if RequestMessage actually uses deny_unknown_fields anywhere
rg -n "RequestMessage" contracts/wallet/src/signature/mod.rs -A 100 | grep -E "(deny_unknown|RequestMessage|pub struct)" | head -30Repository: near/intents
Length of output: 112
🏁 Script executed:
# Read the full erc191 crate
cat -n crates/signatures/erc191/src/lib.rsRepository: near/intents
Length of output: 5249
🏁 Script executed:
# Check the full RequestMessage definition
cat -n contracts/wallet/src/signature/mod.rs | sed -n '32,120p'Repository: near/intents
Length of output: 4290
🏁 Script executed:
# Verify if RequestMessage has deny_unknown_fields
rg -n "deny_unknown_fields" contracts/wallet/src/signature/mod.rsRepository: near/intents
Length of output: 38
🏁 Script executed:
# Check what the actual M type is when verify is called
rg -n "SigningStandard::verify\|\.verify\(" contracts/wallet/src/contract/mod.rs -B 10 -A 5Repository: near/intents
Length of output: 38
🏁 Script executed:
# Find where SigningStandard::verify is called to understand what M is
cat -n contracts/wallet/src/contract/mod.rs | sed -n '70,95p'Repository: near/intents
Length of output: 1109
🏁 Script executed:
# Look at the full context around the verify call
cat -n contracts/wallet/src/contract/mod.rs | sed -n '1,120p'Repository: near/intents
Length of output: 4174
🏁 Script executed:
# Check if there are any tests for the erc191 verify method
find . -name "*.rs" -type f -exec grep -l "Erc191::verify\|test.*erc191\|test.*signature" {} \; | head -10Repository: near/intents
Length of output: 131
🏁 Script executed:
# Check the actual implementation of the signature verification flow
cat -n contracts/wallet/src/signature/erc191.rsRepository: near/intents
Length of output: 1448
🏁 Script executed:
# Verify the serde defaults - check what serde_json does with unknown fields
rg -n "deny_unknown_fields" contracts/wallet/src/ crates/Repository: near/intents
Length of output: 38
🏁 Script executed:
# Look at the patch suggestion more carefully - verify if it compiles conceptually
# Check what serde_json::to_value returns for RequestMessage
cat -n contracts/wallet/src/signature/mod.rs | sed -n '32,120p' | grep -E "derive|near|serde"Repository: near/intents
Length of output: 237
🏁 Script executed:
# Let's test the actual behavior of serde_json with unknown fields
python3 << 'EOF'
import json
# Simulate the behavior
test_json = '{"chain_id": "mainnet", "signer_id": "alice", "nonce": 42, "extra_field": "should_be_ignored"}'
print("Testing serde_json behavior with unknown fields:")
print(f"Input JSON: {test_json}")
print("\nKey insight: serde_json::from_str ignores unknown fields by default")
print("This means deserialization would succeed and 'extra_field' would be dropped")
print("But the signature verification uses signature.payload.0 (the raw JSON string)")
print("\nThe concern is about the COMPARISON check (line 28):")
print("- Original msg: has known fields only")
print("- Deserialized payload: has known fields only (extra_field dropped)")
print("- PartialEq comparison: succeeds")
print("\nBUT the signature itself was on the raw JSON WITH extra_field")
print("The signature.verify() call checks if the cryptographic signature is valid")
print("for the prehash of the raw JSON string, so that would catch tampering")
EOFRepository: near/intents
Length of output: 835
🏁 Script executed:
# Check if there are any tests that validate the signature behavior
rg -n "test" contracts/wallet/src/signature/ -A 10 | head -80Repository: near/intents
Length of output: 38
🏁 Script executed:
# Look for where RequestMessage gets serialized/deserialized in tests or examples
rg -n "RequestMessage" contracts/wallet/src/ -B 2 -A 5 | grep -E "(json|serialize|test)" | head -20Repository: near/intents
Length of output: 458
Enforce strict payload equivalence to reject signed payloads with unexpected fields.
The code currently accepts signed JSON with extra unknown fields because serde_json ignores them during deserialization. While this is not a cryptographic vulnerability (the signature is properly verified against the raw JSON), for strict signature semantics, extra fields in a signed payload should be detected and rejected at the application level. Add a check to ensure the signed JSON structure exactly matches the expected message structure, not just the known fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/wallet/src/signature/erc191.rs` around lines 23 - 29, The code
currently deserializes signature.payload.0 into M and compares msg to &payload
but doesn't detect extra unknown fields; to enforce strict equivalence, first
parse signature.payload.0 into a serde_json::Value (orig_value), then
deserialize that same JSON into M (payload) as you already do, then convert the
deserialized payload back to a serde_json::Value (reconstructed =
serde_json::to_value(&payload)), and finally compare orig_value ==
reconstructed; if they differ return false (in addition to the existing msg !=
&payload check). This ensures the signed JSON contains exactly the expected
fields and no extras while keeping the existing equality check between msg and
payload.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contracts/wallet/src/signature/erc191.rs (1)
20-26:⚠️ Potential issue | 🟡 MinorEnforce strict JSON payload equivalence before accepting
msg == payload.At Lines 20-26, deserializing into
Mmay ignore unknown fields, so extended signed JSON can still pass equality after field dropping. Add a structural JSON equivalence check (raw JSON vs re-serializedM) or enforce strict unknown-field rejection on the message type.Proposed fix
-use near_sdk::{serde::de::DeserializeOwned, serde_json}; +use near_sdk::{ + serde::{de::DeserializeOwned, Serialize}, + serde_json::{self, Value}, +}; impl<M> SigningStandard<&M> for Erc191 where - M: DeserializeOwned + PartialEq, + M: DeserializeOwned + Serialize + PartialEq, { @@ - // deserialize the payload as message - let Ok(payload) = serde_json::from_str::<M>(&signature.payload.0) else { + // parse raw signed JSON first + let Ok(orig_value) = serde_json::from_str::<Value>(&signature.payload.0) else { + return false; + }; + + // deserialize signed JSON into expected message type + let Ok(payload) = serde_json::from_value::<M>(orig_value.clone()) else { + return false; + }; + + // ensure no unknown/extra structure survives deserialization + let Ok(reconstructed) = serde_json::to_value(&payload) else { return false; }; + if orig_value != reconstructed { + return false; + } // check that signed the same message if msg != &payload { return false; }#!/bin/bash set -euo pipefail # Verify current ERC-191 verification path lacks structural-equivalence guard. nl -ba contracts/wallet/src/signature/erc191.rs | sed -n '14,36p' # Check whether strict unknown-field rejection is configured on signature message types. rg -n 'deny_unknown_fields|struct RequestMessage' contracts/wallet/src/signature/ -A4 -B2 # Confirm no JSON value round-trip equivalence guard exists in ERC-191 verifier. rg -n 'serde_json::Value|from_value|to_value' contracts/wallet/src/signature/erc191.rs -A2 -B2Expected result: no
deny_unknown_fieldsguard and noValueround-trip check in this verifier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/wallet/src/signature/erc191.rs` around lines 20 - 26, The deserialization into M (let Ok(payload) = serde_json::from_str::<M>(&signature.payload.0) ...) can drop unknown fields and let extended JSON pass equality; update the ERC-191 verifier to enforce structural JSON equivalence by either (A) parsing the raw signature JSON (signature.payload.0) into serde_json::Value and comparing it to serde_json::to_value(&payload) for exact equality before accepting msg == payload, or (B) ensure the message type M uses #[serde(deny_unknown_fields)] so serde_json::from_str::<M> fails on extra fields; apply the chosen check in the same function where msg, payload and signature.payload.0 are used so equality only succeeds when the JSON structure matches exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@contracts/wallet/src/signature/erc191.rs`:
- Around line 20-26: The deserialization into M (let Ok(payload) =
serde_json::from_str::<M>(&signature.payload.0) ...) can drop unknown fields and
let extended JSON pass equality; update the ERC-191 verifier to enforce
structural JSON equivalence by either (A) parsing the raw signature JSON
(signature.payload.0) into serde_json::Value and comparing it to
serde_json::to_value(&payload) for exact equality before accepting msg ==
payload, or (B) ensure the message type M uses #[serde(deny_unknown_fields)] so
serde_json::from_str::<M> fails on extra fields; apply the chosen check in the
same function where msg, payload and signature.payload.0 are used so equality
only succeeds when the JSON structure matches exactly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe3438e3-86ff-472e-a603-38c2dc6ec759
📒 Files selected for processing (1)
contracts/wallet/src/signature/erc191.rs
frol
left a comment
frol
left a comment
There was a problem hiding this comment.
This looks exactly as I would expect it to be, even though the specifics would take me much longer to implement myself. @mitinarseny Thank you for the quick turn on this front!
2 participants
Summary by CodeRabbit