feat(wallet): sr25519 signing standard#270
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds workspace wiring, schnorrkel-backed SR25519 crypto types and verification, and a new wallet contract supporting signing, proof validation, optional contract registration, formatting, and tests. ChangesSR25519 Wallet Signature Support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WalletSr25519Signer
participant RequestMessage
participant schnorrkel_Keypair
participant WalletSr25519
WalletSr25519Signer->>RequestMessage: hash request
WalletSr25519Signer->>schnorrkel_Keypair: sign wrapped request hash
schnorrkel_Keypair-->>WalletSr25519Signer: return signature proof
WalletSr25519Signer->>WalletSr25519: verify proof
WalletSr25519->>RequestMessage: hash request
WalletSr25519-->>WalletSr25519Signer: return verification result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (3)
crates/crypto/src/public_key.rs (1)
100-112: ⚡ Quick winLock the sr25519 implicit-account mapping down with a test vector.
This branch introduces a new externally visible address-derivation rule, but the test table below still has no
sr25519case. Please add at least one fixedto_implicit_account_idassertion for ansr25519:key so the"sr25519"prefix and hash slice stay stable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/crypto/src/public_key.rs` around lines 100 - 112, Add a test vector asserting the new sr25519 implicit-account mapping so the "sr25519" prefix and keccak slice remain stable: locate the unit tests that assert to_implicit_account_id (the table of test cases used for other curve variants) and add at least one entry that constructs a PublicKey::Sr25519 with a fixed byte sequence and asserts its to_implicit_account_id equals the expected "0x..." hex string computed from keccak256([b"sr25519", pk].concat())[12..32]; reference the PublicKey::Sr25519 enum variant and the to_implicit_account_id behavior in the assertion.crates/crypto/src/signature.rs (1)
113-114: ⚡ Quick winAdd sr25519 parser regression cases.
The new
CurveType::Sr25519branch is not covered by theparse_ok/parse_invalid_lengthrstests below, so this path is easier to break than the existing curves.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/crypto/src/signature.rs` around lines 113 - 114, The new CurveType::Sr25519 branch (in signature.rs) lacks rstest coverage; add regression tests mirroring the existing parse_ok and parse_invalid_length cases for other curves to exercise checked_base58_decode_array and the CurveType::Sr25519 mapping. Create a parse_ok test entry with a valid sr25519 base58 string that decodes to the expected byte array mapped to CurveType::Sr25519, and a parse_invalid_length entry with a base58 string of wrong length to assert the parser rejects it; follow the same test names/structure used for the other curves so checked_base58_decode_array and the CurveType::Sr25519 branch are exercised.crates/signatures/sr25519/src/lib.rs (1)
53-67: ⚡ Quick winAdd a JSON round-trip test for the proof shape.
The wallet consumes this type over JSON, but the tests only construct it in Rust. A serde fixture here would lock down
flattenplusAsCurve<Sr25519>and catch wire-format regressions early.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/signatures/sr25519/src/lib.rs` around lines 53 - 67, Add a serde JSON round-trip test for the SignedSr25519Payload shape to ensure flatten + AsCurve<Sr25519> wire format stays stable: write a unit test that constructs a SignedSr25519Payload (using Sr25519 keypair/signature utilities from this crate), serialize it to JSON, compare against a checked-in JSON fixture (or write and then deserialize the fixture), then deserialize back and assert equality with the original; place the test near other sr25519 tests and reference the SignedSr25519Payload type, the payload field, public_key and signature fields to validate the flattened structure is preserved across serialization and deserialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/crypto/src/public_key.rs`:
- Around line 100-112: Add a test vector asserting the new sr25519
implicit-account mapping so the "sr25519" prefix and keccak slice remain stable:
locate the unit tests that assert to_implicit_account_id (the table of test
cases used for other curve variants) and add at least one entry that constructs
a PublicKey::Sr25519 with a fixed byte sequence and asserts its
to_implicit_account_id equals the expected "0x..." hex string computed from
keccak256([b"sr25519", pk].concat())[12..32]; reference the PublicKey::Sr25519
enum variant and the to_implicit_account_id behavior in the assertion.
In `@crates/crypto/src/signature.rs`:
- Around line 113-114: The new CurveType::Sr25519 branch (in signature.rs) lacks
rstest coverage; add regression tests mirroring the existing parse_ok and
parse_invalid_length cases for other curves to exercise
checked_base58_decode_array and the CurveType::Sr25519 mapping. Create a
parse_ok test entry with a valid sr25519 base58 string that decodes to the
expected byte array mapped to CurveType::Sr25519, and a parse_invalid_length
entry with a base58 string of wrong length to assert the parser rejects it;
follow the same test names/structure used for the other curves so
checked_base58_decode_array and the CurveType::Sr25519 branch are exercised.
In `@crates/signatures/sr25519/src/lib.rs`:
- Around line 53-67: Add a serde JSON round-trip test for the
SignedSr25519Payload shape to ensure flatten + AsCurve<Sr25519> wire format
stays stable: write a unit test that constructs a SignedSr25519Payload (using
Sr25519 keypair/signature utilities from this crate), serialize it to JSON,
compare against a checked-in JSON fixture (or write and then deserialize the
fixture), then deserialize back and assert equality with the original; place the
test near other sr25519 tests and reference the SignedSr25519Payload type, the
payload field, public_key and signature fields to validate the flattened
structure is preserved across serialization and deserialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22a67744-4b64-4108-bee2-8c424481ad86
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.tomlcontracts/wallet/Cargo.tomlcontracts/wallet/src/contract/impl_.rscontracts/wallet/src/signature/mod.rscontracts/wallet/src/signature/sr25519.rscrates/crypto/Cargo.tomlcrates/crypto/src/curve/mod.rscrates/crypto/src/curve/sr25519.rscrates/crypto/src/lib.rscrates/crypto/src/parse.rscrates/crypto/src/public_key.rscrates/crypto/src/signature.rscrates/signatures/sr25519/Cargo.tomlcrates/signatures/sr25519/src/lib.rs
…etto255) support: - `defuse-crypto` gains an `sr25519` feature and a `Sr25519` `Curve` impl backed by `schnorrkel` with the Substrate `"substrate"` signing context. - `defuse-wallet-sr25519` is a new wallet variant that verifies proofs over `<Bytes>...</Bytes>`-wrapped canonical `RequestMessage` hashes — matching what Polkadot.js Extension, Talisman, and other Substrate wallets produce via `signRaw`. - A `WalletSigner<WalletSr25519>` implementation is provided for `schnorrkel::Keypair` behind the `signer` feature.
f8b3799 to
488d16b
Compare
Adapts the sr25519 wallet variant to the new `Signer<C>` trait pattern introduced by near#315.
3 participants
Summary
Adds Polkadot/Substrate-compatible Sr25519 (Schnorr on Ristretto255) support to the wallet contract, aligned with the current post-#314 codebase structure.
Changes
defuse-crypto— newsr25519feature gates aSr25519Curveimpl backed byschnorrkelwith the Substrateb"substrate"signing context.Sr25519PublicKeyandSr25519Signaturefollow the same shape as the existingEd25519PublicKey/Ed25519Signature:borsh/serde/schemars/arbitraryderives,fmt::TypedCurvebase58 encoding under thesr25519:prefix, and safeTryFromconversions to/from rawschnorrkeltypes.defuse-wallet-sr25519(new crate atcontracts/wallet/signatures/sr25519/) — a wallet variant crate mirroringdefuse-wallet-ed25519:WalletSr25519: SignatureSchemaverifies proofs over<Bytes>...</Bytes>-wrapped canonicalRequestMessage::hash()bytes. That's exactly what Polkadot.js Extension, Talisman, Subwallet, and other Substrate wallets produce viasignRaw.WalletSigner<WalletSr25519>is implemented forschnorrkel::Keypairbehind thesignerfeature — clients can sign requests with the SDK just like ed25519.wallet!macro invocation exposes the contract entrypoint underwallet-sr25519v1.0.0 with the standard reproducible-build metadata.Wiring — new workspace member + path alias, added to
Makefile'sCONTRACT_CRATESallowlist somake defuse-wallet-sr25519/make check-contracts/defuse-wallet-sr25519work.Verification
cargo fmt --checkcleancargo clippy --workspace --all-targets -- -D warningscleancargo clippy -p defuse-wallet-sr25519 --target wasm32-unknown-unknown --features contract -- -D warningscleancargo test -p defuse-wallet-sr25519 --features abi,contract,signer— 2 passing (round-trip + wrong-message-rejected)cargo test -p defuse-cryptosr25519 module — 3 passing, including a real Polkadot.js Extension signature kept as regression vector (address167y8dsUr7kaM1FNoCtXWy2unEnjGHiN7ML3vawR6Nwywbci, message"Hello from Intents!")make check-contracts/defuse-wallet-sr25519cleanTest plan
schnorrkel::Keypair+WalletSr25519::verifycontractfeature (ZBA-relevant)defuse-tests— omitted for now; requires reproducible-build wasm artifact (res/defuse-wallet-sr25519.wasm) which will land after this PR ships and a real Polkadot.js flow is exercisedNotes for reviewers
<Bytes>...</Bytes>, not a human-readable JSON payload. Same UX as the ed25519 variant.env::ed25519_verify, sr25519 verification runs in-WASM via schnorrkel. Wasm size / gas cost impact should be verified before mainnet deployment.Summary by CodeRabbit