Add support for sr25519 signatures #171
Conversation
📝 WalkthroughWalkthroughAdds Sr25519 support across the workspace: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant CorePayload as core::payload
participant SrCrate as defuse-sr25519
participant Crypto as crypto::curve::Sr25519
participant Schnorrkel
App->>CorePayload: construct SignedSr25519Payload (payload, pubkey, sig)
App->>CorePayload: wrap into MultiPayload::Sr25519
App->>CorePayload: call verify() / extract_defuse_payload()
CorePayload->>SrCrate: delegate verify() / request extraction
SrCrate->>Crypto: call Sr25519::verify(sig, msg, pubkey)
Crypto->>Schnorrkel: parse keys & verify with "substrate" context
Schnorrkel-->>Crypto: verification result
Crypto-->>SrCrate: Option<PublicKey>
SrCrate-->>CorePayload: verification outcome / deserialized DefusePayload<T>
CorePayload-->>App: PublicKey::Sr25519 or None / Deserialized payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. ✨ Finishing touches
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
🧹 Nitpick comments (7)
crypto/src/signature.rs (1)
154-183: Add Sr25519 coverage to Signature parsing testsThe
parse_okandparse_invalid_lengthtests currently exercise only Ed25519, Secp256k1, and P256. Adding Sr25519 examples to both would guard the new variant’s base58 parsing and length validation in the same way as the others.Consider extending the
#[values(...)]lists (or adding a dedicated Sr25519 test) to include:
- At least one well-formed Sr25519 signature string that should parse successfully.
- One or more malformed/short Sr25519 strings that should produce
ParseCurveError::InvalidLength.sr25519/tests/integration_test.rs (1)
1-124: Strong Sr25519 integration tests; only minor optional polishThese tests give solid coverage for Sr25519: correct verification and key recovery, rejection of invalid signatures, cross‑message mismatch, and basic hash consistency for the payload type. The structure matches how other curves are typically exercised.
If you want to tighten things further (optional):
- Consider centralizing the
"substrate"context into a shared constant used both here and incrypto/src/curve/sr25519.rsso a future context change can’t drift between implementation and tests.- You could factor tiny helpers for “sign message and build
SignedSr25519Payload” to reduce duplication across the three signature tests.- For
test_payload_hash_consistency, you might also assert against a fixed expected hash for a canonical message (in addition to equality/inequality) to lock in the hash format more explicitly.core/src/payload/sr25519.rs (1)
2-2: Remove unused imports.
DeserializeandSerializeare imported but not used in this file.-use near_sdk::serde::{Deserialize, Serialize, de::DeserializeOwned}; +use near_sdk::serde::de::DeserializeOwned;crypto/src/curve/sr25519.rs (1)
7-31: Consider documenting theCurveimplementation.The implementation is correct, but adding a doc comment to the
Sr25519struct explaining it implements Schnorr signatures on Ristretto255 (the "sr" in sr25519) would help maintainability.+/// Sr25519 curve implementation using Schnorr signatures on Ristretto255. +/// +/// This is the standard signature scheme for Polkadot/Substrate chains. pub struct Sr25519;crypto/src/public_key.rs (2)
208-226: Consider adding a test case for Sr25519 implicit account ID.The existing tests cover Ed25519, Secp256k1, and P256, but there's no test for the new Sr25519 variant.
#[case( "p256:3aMVMxsoAnHUbweXMtdKaN1uJaNwsfKv7wnc97SDGjXhyK62VyJwhPUPLZefKVthcoUcuWK6cqkSU4M542ipNxS3", "0x7edf07ede58238026db3f90fc8032633b69b8de5" )] + #[case( + "sr25519:3aMVMxsoAnHUbweXMtdKaN1uJaNwsfKv7wnc97SDGjXhyK62VyJwhPUPLZefKVthcoUcuWK6cqkSU4M542ipNxS3", + // Expected value: compute keccak256(b"sr25519" ++ pk)[12..32] and hex encode with 0x prefix + "0x..." // Replace with actual expected value + )] fn to_implicit_account_id(#[case] pk: &str, #[case] expected: &str) {
228-241: Add Sr25519 to invalid length parsing tests.For completeness, include Sr25519 in the invalid length test cases.
#[values( "ed25519:5TagutioHgKLh7KZ1VEFBYfgRkPtqnKm9LoMnJMJ", "ed25519:", "secp256k1:p3UPfBR3kWxE2C8wF1855eguaoRvoW6jV5ZXbu3sTTCs", "secp256k1:", "p256:p3UPfBR3kWxE2C8wF1855eguaoRvoW6jV5ZXbu3sTTCs", - "p256:" + "p256:", + "sr25519:p3UPfBR3kWxE2C8wF1855eguaoRvoW6jV5ZXbu3sTTCs", + "sr25519:" )]sr25519/src/lib.rs (1)
34-38: Consider removing or documenting the unusedget_context()method.This method returns
b"substrate"but is never used in the verification flow—the context is hardcoded incrypto/src/curve/sr25519.rsat line 27. This could cause confusion if someone assumes callingget_context()affects signature operations.Options:
- Remove this method if not needed externally
- Add documentation clarifying it's informational only
/// Get the signing context (always "substrate") + /// + /// Note: This is informational only. The actual signing context is applied + /// internally by the Sr25519 verification implementation. #[inline] pub const fn get_context(&self) -> &'static [u8] { b"substrate" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.toml(3 hunks)core/Cargo.toml(1 hunks)core/src/payload/mod.rs(1 hunks)core/src/payload/multi.rs(5 hunks)core/src/payload/sr25519.rs(1 hunks)crypto/Cargo.toml(1 hunks)crypto/src/curve/mod.rs(2 hunks)crypto/src/curve/sr25519.rs(1 hunks)crypto/src/public_key.rs(6 hunks)crypto/src/signature.rs(5 hunks)sr25519/Cargo.toml(1 hunks)sr25519/src/lib.rs(1 hunks)sr25519/tests/integration_test.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
crypto/src/curve/mod.rs (1)
crypto/src/parse.rs (1)
checked_base58_decode_array(15-26)
sr25519/tests/integration_test.rs (4)
sr25519/src/lib.rs (4)
new(30-32)verify(78-88)hash(43-47)hash(69-71)core/src/payload/multi.rs (2)
verify(85-96)hash(67-78)crypto/src/curve/sr25519.rs (1)
verify(15-31)crypto/src/curve/mod.rs (1)
verify(23-27)
core/src/payload/sr25519.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)
sr25519/src/lib.rs (2)
core/src/payload/multi.rs (2)
hash(67-78)verify(85-96)crypto/src/curve/sr25519.rs (1)
verify(15-31)
🔇 Additional comments (12)
sr25519/Cargo.toml (1)
1-27: sr25519 crate manifest wiring and dependency choices look soundThe manifest follows the workspace conventions, and the explicit
rand = "0.8"pin with a comment clarifying schnorrkel compatibility is a reasonable, well-documented choice for tests here.When bumping
schnorrkelor workspacerand, double‑check therand_corecompatibility still holds so this pin can eventually be unified or adjusted if needed.crypto/Cargo.toml (1)
8-18: Crypto crate dependency wiring for the new curve is consistentExposing the curve library via
schnorrkel.workspace = truematches the existing pattern for other crypto dependencies and keeps the version centralized at the workspace level.Please ensure your CI matrix exercises both host and wasm (if applicable) targets so any feature or
std/no_stdmismatches withschnorrkelare caught early.core/Cargo.toml (1)
8-16: Core’s dependency on defuse-sr25519 is wired correctlyAdding
defuse-sr25519.workspace = truealigns with how other internal crates are pulled into core and is what you need for the new sr25519 payload module.core/src/payload/mod.rs (1)
1-9: Exposing the sr25519 payload module is appropriateAdding
pub mod sr25519;cleanly integrates the new payload type into the existing payload namespace without affecting existing modules.crypto/src/signature.rs (1)
8-11: Sr25519 integration into Signature is consistent with existing curvesThe new
Signature::Sr25519variant is correctly wired throughcurve_type,data(), andFromStr, reusing the same base58 parsing path as the other curves. This keeps the public API and debugging/Display behavior uniform across all supported curves.Also applies to: 19-45, 81-86
crypto/src/curve/mod.rs (1)
1-9: Sr25519 curve is cleanly integrated into the curve moduleThe new
sr25519module, its re-export, and theCurveType::Sr25519variant follow the existing pattern for other curves, so consumers ofCurveTypeand the curve prelude will naturally pick up Sr25519 support.Also applies to: 30-37
Cargo.toml (1)
3-35: Workspace wiring for sr25519 and schnorrkel is coherentRegistering
sr25519as a workspace member, mappingdefuse-sr25519.path = "sr25519", and centralizing theschnorrkel = "0.11"dependency under[workspace.dependencies]all align with the existing layout of internal crates and crypto libraries.Given the mixture of workspace‑wide
rand = "0.9"and the sr25519 crate’s localrand = "0.8"for schnorrkel compatibility, keep an eye on these when upgrading so you can eventually consolidate onto a singlerandversion if/when schnorrkel updates itsrand_coredependency.Also applies to: 44-78, 79-111
core/src/payload/multi.rs (2)
55-58: LGTM! Sr25519 variant follows the established pattern.The documentation comment appropriately references Substrate/Polkadot conventions and compatibility with Polkadot.js wallets, consistent with the implementation.
76-76: LGTM! All trait implementations correctly extended.The
hash(),verify(), andextract_defuse_payload()arms properly delegate to the payload's methods, matching the pattern used for other variants.Also applies to: 94-94, 115-115
core/src/payload/sr25519.rs (1)
6-14: LGTM! Implementation follows the established pattern.The
ExtractDefusePayloadimplementation correctly deserializesself.messageintoDefusePayload<T>, consistent with how other payload types (e.g., Erc191, Nep413) implement this trait.crypto/src/public_key.rs (1)
79-88: LGTM! Implicit account ID derivation follows P256 pattern.The "sr25519" prefix correctly avoids collisions with other curve types. The derivation scheme is consistent with the P256 approach documented in lines 61-77.
sr25519/src/lib.rs (1)
91-112: LGTM! Unit tests cover basic functionality.The tests verify payload creation, context retrieval, and hash consistency. Note that integration tests for signature verification are covered in a separate test file (
sr25519/tests/integration_test.rsper the AI summary).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
sr25519/src/lib.rs (1)
50-66: CRITICAL: Missing#[serde_as]attribute on struct.The struct uses field-level
#[serde_as(as = "...")]attributes (lines 60, 64) but is missing the required#[serde_as]attribute at the struct level. This will cause serde_with macros to fail.Apply this diff to fix:
/// Signed Sr25519 message with signature #[near(serializers = [json])] +#[serde_as] #[autoimpl(Deref using self.payload)] #[derive(Debug, Clone)] pub struct SignedSr25519Payload {
🧹 Nitpick comments (1)
sr25519/src/lib.rs (1)
37-48: Consider using.as_bytes()explicitly for clarity.While
env::sha256_array(&self.message)works due toStringimplementingAsRef<[u8]>, usingenv::sha256_array(self.message.as_bytes())would be more explicit and consistent with line 86 where.as_bytes()is called.- env::sha256_array(&self.message) + env::sha256_array(self.message.as_bytes())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.toml(3 hunks)core/Cargo.toml(1 hunks)core/src/payload/sr25519.rs(1 hunks)crypto/Cargo.toml(1 hunks)crypto/src/curve/mod.rs(2 hunks)crypto/src/curve/sr25519.rs(1 hunks)crypto/src/public_key.rs(6 hunks)crypto/src/signature.rs(5 hunks)sr25519/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- core/Cargo.toml
- crypto/Cargo.toml
- crypto/src/curve/sr25519.rs
🧰 Additional context used
🧬 Code graph analysis (5)
crypto/src/public_key.rs (2)
crypto/src/parse.rs (1)
checked_base58_decode_array(15-26)crypto/src/signature.rs (1)
data(39-47)
core/src/payload/sr25519.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)
crypto/src/signature.rs (2)
crypto/src/parse.rs (1)
checked_base58_decode_array(15-26)crypto/src/public_key.rs (1)
data(40-48)
crypto/src/curve/mod.rs (1)
crypto/src/parse.rs (1)
checked_base58_decode_array(15-26)
sr25519/src/lib.rs (2)
core/src/payload/multi.rs (2)
hash(67-78)verify(85-96)crypto/src/curve/sr25519.rs (1)
verify(22-35)
⏰ 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). (9)
- GitHub Check: Build
- GitHub Check: Build Reproducible
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- 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-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Security Audit - report
🔇 Additional comments (6)
crypto/src/curve/mod.rs (1)
4-4: LGTM! Sr25519 integration follows established patterns.The Sr25519 module is properly integrated into the curve type system, following the same pattern as Ed25519, P256, and Secp256k1. The discriminant value (3) maintains the sequence, and all exports are correctly added.
Also applies to: 8-8, 37-37
crypto/src/signature.rs (1)
9-10: LGTM! Signature enum properly extended for Sr25519.All required methods (
curve_type,data,FromStr) are correctly updated to handle the Sr25519 variant, maintaining consistency with existing curve implementations.Also applies to: 24-24, 34-34, 45-45, 86-86
core/src/payload/sr25519.rs (1)
1-15: LGTM! Payload extraction implementation is correct.The
ExtractDefusePayloadimplementation properly deserializes the Sr25519 payload message. All imports are used and the error type is appropriate for JSON deserialization.crypto/src/public_key.rs (1)
9-10: LGTM! PublicKey properly extended with Sr25519 support.All methods are correctly updated. The implicit account ID derivation (lines 80-89) properly follows the Ethereum-style schema with a "sr25519" prefix to prevent address collisions, consistent with the P256 implementation.
Also applies to: 25-25, 35-35, 46-46, 80-89, 140-140
sr25519/src/lib.rs (2)
75-90: LGTM! Verification implementation is correct.The
SignedPayloadimplementation properly delegates toSr25519::verifywith the message bytes, signature, and public key. The verification logic aligns with the schnorrkel signing context.
92-113: LGTM! Tests provide good coverage.The test module appropriately validates payload creation, hashing behavior, and hash determinism.
|
Hey @green-jay , thank you for submitting the PR! We've updated and fixed it a bit, but the core functionality was kept. As a last step, can you please add a test for signature verification of some message that was signed via real wallet? I'd appreciate if you also specify the exact wallet you used and some link to test dApp (like this one), so we can test it ourselves. Thanks! |
|
Hey @mitinarseny, thanks for looking into it! Please check my last commit. |
|
@green-jay thanks, looks good! |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
sr25519/src/lib.rs (1)
49-65: Missing#[serde_as]attribute on struct.The field-level
#[serde_as(as = "...")]attributes on lines 59 and 63 require the struct to have a#[serde_as]attribute for the serde_with macro to process them correctly.🔎 Proposed fix
/// Signed Sr25519 message with signature #[near(serializers = [json])] +#[serde_as] #[autoimpl(Deref using self.payload)] #[derive(Debug, Clone)] pub struct SignedSr25519Payload {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sr25519/src/lib.rs
⏰ 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). (7)
- 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-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / 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 / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (4)
sr25519/src/lib.rs (4)
1-10: LGTM! Clear documentation and appropriate imports.The module documentation clearly explains the purpose and compatibility, and all imports are utilized appropriately throughout the implementation.
67-89: LGTM! Correct implementation of signature verification.The
PayloadandSignedPayloadtrait implementations correctly delegate hashing and implement verification using the Sr25519 curve with proper message byte conversion.
91-231: LGTM! Comprehensive test coverage.The test suite provides excellent coverage including payload creation, hashing consistency, signature verification, invalid signature rejection, and cross-verification between different messages.
233-269: Excellent! This test addresses the PR objective.This test directly fulfills the request from the PR discussion by verifying a real signature from Polkadot.js Extension. It includes all requested details:
- Exact wallet: Polkadot.js Extension
- Test dApp URL: https://polkadot.js.org/apps/#/signing
- Date and address for reproducibility
- Documentation of the message wrapping behavior (
<Bytes></Bytes>tags)The test validates that the Sr25519 implementation is compatible with actual Substrate wallet signatures.
Based on PR objectives, this addresses mitinarseny's request for a real wallet signature verification test.
|
@mitinarseny should be done, please check |
|
gm @mitinarseny @vzctl, do you think we can merge this? |
|
@green-jay Sorry for taking this long. I've reviewed, the PR is all good from our side. |
4 participants
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.