Add ADR-36 signatures support #195
Conversation
📝 WalkthroughWalkthroughAdds a new workspace crate Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Adr36 as adr36::lib
participant Signer
participant Core
participant Secp as Secp256k1
Client->>Adr36: create Adr36Payload(message, signer, algo)
Adr36->>Adr36: prehash() -> Amino JSON -> SHA-256 / Keccak256
Client->>Signer: request signature(prehash)
Signer->>Client: return signature
Client->>Core: submit SignedAdr36Payload (MultiPayload::Adr36)
Core->>Secp: verify(signature, payload.hash())
Secp-->>Core: public key / None
Core->>Client: verification result and extracted JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In `@adr36/Cargo.toml`:
- Around line 1-19: The Cargo.toml declares an unused dependency
defuse-near-utils; remove the defuse-near-utils workspace entry from the
[dependencies] section (delete the line containing "defuse-near-utils.workspace
= true") so the crate no longer depends on it and then run cargo check to
confirm no missing imports.
In `@adr36/src/lib.rs`:
- Around line 118-119: Rename the test function symbol test_expected_sh256_hash
to the correctly spelled test_expected_sha256_hash; update the #[test] function
declaration for test_expected_sh256_hash to fn test_expected_sha256_hash() and
adjust any references or calls to that test name (if any) so they match the new
identifier to ensure the test runs and naming is consistent.
🧹 Nitpick comments (2)
adr36/src/lib.rs (2)
93-101: Consider extracting sharedfix_v_in_signaturehelper.This function is noted as copy-paste from
erc191/lib.rs. Consider extracting it to a shared location indefuse-cryptoto avoid duplication and ensure consistent behavior across payload types.
141-153: Consider strengthening the negative test assertion.The current test uses
assert_ne!which only verifies the returned public key doesn't matchREFERENCE_PUBKEY. If the goal is to verify that a wrong signature fails verification entirely, consider also checking thatverify()returnsNoneor a different public key.More explicit negative test
#[test] fn test_reference_signature_verification_fails() { let payload = Adr36Payload::new( REFERENCE_MESSAGE.to_string(), REFERENCE_SECP256K1_SIGNER.to_string(), ); let signature = fix_v_in_signature(WRONG_REFERENCE_SIGNATURE); - assert_ne!( - SignedAdr36Payload { payload, signature }.verify(), - Some(REFERENCE_PUBKEY) - ); + let result = SignedAdr36Payload { payload, signature }.verify(); + // Verify either verification fails entirely (None) or recovers a different key + assert!( + result.is_none() || result != Some(REFERENCE_PUBKEY), + "Expected verification to fail or recover different pubkey" + ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomladr36/Cargo.tomladr36/src/lib.rscore/Cargo.tomlcore/src/payload/adr36.rscore/src/payload/mod.rscore/src/payload/multi.rs
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/payload/adr36.rs (2)
core/src/payload/mod.rs (2)
extract_defuse_payload(43-43)extract_defuse_payload(50-52)core/src/payload/multi.rs (1)
extract_defuse_payload(106-117)
adr36/src/lib.rs (2)
core/src/payload/multi.rs (2)
hash(67-78)verify(85-96)crypto/src/signature.rs (1)
sig(159-159)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Check
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (15)
Cargo.toml (1)
5-5: LGTM!The workspace member and path dependency are correctly added following the existing alphabetical ordering and patterns.
Also applies to: 49-49
core/Cargo.toml (1)
10-10: LGTM!The dependency is correctly added using the workspace pattern, consistent with other payload-related dependencies.
core/src/payload/mod.rs (1)
1-1: LGTM!Module declaration correctly placed in alphabetical order before
erc191.core/src/payload/multi.rs (3)
5-5: LGTM!Import correctly added for the new ADR-36 payload type.
76-76: LGTM!The
Adr36variant is correctly integrated into all three trait implementations:
Payload::hash()delegates to the inner payload's hashSignedPayload::verify()maps toPublicKey::Secp256k1(correct for Cosmos SDK which uses secp256k1)ExtractDefusePayload::extract_defuse_payload()delegates extractionThe pattern is consistent with other payload types.
Also applies to: 94-94, 115-115
54-58: LGTM!The
Adr36variant is well-documented with references to the ADR-36 specification and Keplr wallet implementation docs. The ADR-36 prehash construction (base64-encoded message within a standard JSON structure), signature verification using secp256k1, and comprehensive test coverage with reference vectors from the cosmos-sdk all confirm the implementation is correct and spec-compliant.core/src/payload/adr36.rs (2)
5-15: LGTM!The delegation pattern for
SignedAdr36Payloadis consistent with how other signed payload types work in the codebase. The#[inline]hint is appropriate for this thin wrapper.
17-26: LGTM!The JSON parsing from
self.messageis the correct approach for ADR-36 payloads, where the message field contains the serializedDefusePayload. This aligns with theprehash()implementation inadr36/src/lib.rsthat base64-encodes the message.adr36/src/lib.rs (7)
7-20: LGTM!Good documentation with links to the ADR-36 specification and Keplr usage docs. The Bech32 address format explanation for the
signerfield is helpful for maintainability.
22-26: LGTM!The
const fnconstructor is appropriate here. Input validation (e.g., Bech32 format) can reasonably be deferred to signature verification time.
54-59: LGTM!The SHA-256 hash of the prehashed bytes is correct per ADR-36 specification.
61-68: LGTM!The
SignedAdr36Payloadstructure correctly wraps the payload with a Secp256k1 signature, using theAsCurveserde wrapper for proper serialization.
70-75: LGTM!Correctly delegates to the inner payload's hash, which is the standard pattern for signed payloads.
77-84: LGTM!The
SignedPayloadimplementation correctly uses Secp256k1 verification. The integration withMultiPayloadincore/src/payload/multi.rs(line 94) properly maps the result toPublicKey::Secp256k1.
28-51: No action required. The JSON key ordering is correct.The
serde_json::json!macro usesBTreeMapby default, which automatically sorts keys in lexicographic order. This is the correct behavior for ADR-36's canonical JSON serialization requirement. The keys in the prehash JSON are already properly ordered:account_number,chain_id,fee,memo,msgs,sequence. Nopreserve_orderfeature is needed; the default behavior already produces deterministic, sorted output for correct signature verification.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
mitinarseny
left a comment
mitinarseny
left a comment
There was a problem hiding this comment.
@kuksag thanks for the PR! Overall, looks good to me, see just a minor comment below
| { | ||
| "type": "sign/MsgSignData", | ||
| "value": { | ||
| "data": STANDARD.encode(self.message.as_bytes()), |
There was a problem hiding this comment.
According to docs:
It's applications developers decision how Data should be treated, by treated we mean the serialization and deserialization process and the Object Data should represent.
Since we already represent self.message as String, does it make sense to avoid base64 encoding here and embed message as-is?
There was a problem hiding this comment.
Good catch. I'm gonna say I've overlooked that, because I was referencing implementations of the most used wallets on Cosmos SDK, and they wrap a message into base64:
- keplr
https://github.com/chainapsis/keplr-wallet/blob/59b2e18122dc2ec3b12d3005fec709e4bcc885f8/packages/cosmos/src/adr-36/amino.ts#L78 - leap wallet
https://github.com/leapwallet/cosmos-extension/blob/d6747ff73851d8e958dfec9ef1da4809100d694a/packages/wallet-provider/src/provider/core.ts#L295
To comply with the standard and to have something working in practise, we can verify the signature against both b64(message) (as of right now) and plaindata.
There was a problem hiding this comment.
Ah, ok, if that's a convention across ecosystem - I'm ok with leaving it in current state, i.e. always encode as base64
adr36/src/lib.rs
Outdated
| impl Payload for Adr36Payload { | ||
| #[inline] | ||
| fn hash(&self) -> CryptoHash { | ||
| env::sha256_array(self.prehash()) |
There was a problem hiding this comment.
got a question - cosmos adr-36 allows compability with ethereum native wallets using EIP712 standart - https://github.com/chainapsis/keplr-wallet/blob/59b2e18122dc2ec3b12d3005fec709e4bcc885f8/packages/cosmos/src/adr-36/amino.ts#L150
type of algo is not part of signed msg - does it mean that we support only native cosmos signatures? If not may be include algo in payload?
There was a problem hiding this comment.
Nice catch.
We've discussed it internally and decided that if the chain is Ethermint-like, then they can simply sign an intent in evm style from e.g. Metamask.
Or, if the signer is inside Cosmos wallet, they still can sign an evm-style message, like this
Anyways, I've added support for algo selection, just for the sake of compatibility. In theory all known formats can be added, but I think adding support for the rest is too much for now.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@adr36/src/lib.rs`:
- Around line 110-116: The verify() implementation in SignedAdr36Payload
(function SignedAdr36Payload::verify) does not normalize Ethereum-style recovery
byte v (27/28), so production verification can fail for wallet signatures;
update verify() to normalize the signature by calling the existing
fix_v_in_signature(...) on self.signature (or a copy) before passing it to
Secp256k1::verify(&normalized_sig, &self.payload.hash(), &()), ensuring the same
v-normalization used in tests is applied in production.
| impl SignedPayload for SignedAdr36Payload { | ||
| type PublicKey = <Secp256k1 as Curve>::PublicKey; | ||
|
|
||
| #[inline] | ||
| fn verify(&self) -> Option<Self::PublicKey> { | ||
| Secp256k1::verify(&self.signature, &self.payload.hash(), &()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the adr36 directory and examine the file
find . -type f -name "lib.rs" | grep -E "adr36|Adr36"Repository: near/intents
Length of output: 74
🏁 Script executed:
# Read the file to see the context around lines 110-116 and 126-132
cat -n adr36/src/lib.rs | sed -n '100,145p'Repository: near/intents
Length of output: 2104
🏁 Script executed:
# Check the entire file to understand the structure
wc -l adr36/src/lib.rsRepository: near/intents
Length of output: 76
🏁 Script executed:
# Check where fix_v_in_signature is used
grep -n "fix_v_in_signature" adr36/src/lib.rsRepository: near/intents
Length of output: 268
🏁 Script executed:
# See more of the test code to understand how it's used
cat -n adr36/src/lib.rs | sed -n '145,189p'Repository: near/intents
Length of output: 1786
🏁 Script executed:
# Check if there are other files that might handle v normalization
rg "fix_v_in_signature|normalize.*v" --type rustRepository: near/intents
Length of output: 1005
Normalize Ethereum-style recovery v before verification.
Tests normalize v using fix_v_in_signature() (lines 126-133) before calling verify(), but production verify() doesn't perform this normalization. Ethereum wallets emit v = 27/28, and without normalization in the production code, signatures from real wallets will fail verification even though test signatures work (because tests manually normalize before verification).
🛠️ Proposed fix
+fn normalize_v(mut sig: <Secp256k1 as Curve>::Signature) -> <Secp256k1 as Curve>::Signature {
+ // Ethereum-style signatures use v=27/28; normalize to 0/1
+ if sig[64] >= 27 {
+ sig[64] -= 27;
+ }
+ sig
+}
+
impl SignedPayload for SignedAdr36Payload {
type PublicKey = <Secp256k1 as Curve>::PublicKey;
#[inline]
fn verify(&self) -> Option<Self::PublicKey> {
- Secp256k1::verify(&self.signature, &self.payload.hash(), &())
+ let sig = normalize_v(self.signature);
+ Secp256k1::verify(&sig, &self.payload.hash(), &())
}
}📝 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.
| impl SignedPayload for SignedAdr36Payload { | |
| type PublicKey = <Secp256k1 as Curve>::PublicKey; | |
| #[inline] | |
| fn verify(&self) -> Option<Self::PublicKey> { | |
| Secp256k1::verify(&self.signature, &self.payload.hash(), &()) | |
| } | |
| fn normalize_v(mut sig: <Secp256k1 as Curve>::Signature) -> <Secp256k1 as Curve>::Signature { | |
| // Ethereum-style signatures use v=27/28; normalize to 0/1 | |
| if sig[64] >= 27 { | |
| sig[64] -= 27; | |
| } | |
| sig | |
| } | |
| impl SignedPayload for SignedAdr36Payload { | |
| type PublicKey = <Secp256k1 as Curve>::PublicKey; | |
| #[inline] | |
| fn verify(&self) -> Option<Self::PublicKey> { | |
| let sig = normalize_v(self.signature); | |
| Secp256k1::verify(&sig, &self.payload.hash(), &()) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@adr36/src/lib.rs` around lines 110 - 116, The verify() implementation in
SignedAdr36Payload (function SignedAdr36Payload::verify) does not normalize
Ethereum-style recovery byte v (27/28), so production verification can fail for
wallet signatures; update verify() to normalize the signature by calling the
existing fix_v_in_signature(...) on self.signature (or a copy) before passing it
to Secp256k1::verify(&normalized_sig, &self.payload.hash(), &()), ensuring the
same v-normalization used in tests is applied in production.
|
Also I will share with you our plan on how we will integrate it on our side. intents/core/src/engine/mod.rs Line 73 in ad29e01 In case when there's no public keys associated with the intents/crypto/src/public_key.rs Line 53 in 057dc1e Note: signer field in Adr36Payload stays unchanged, as it doesn't play any role in intent execution.
|
4 participants
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.