Repository navigation
Add ML-DSA-65 signing, default stays ML-DSA-87 - #4
Conversation
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT 5.6 Sol
Verdict (advisory): Request changes
Blocking finding:
- [P1] Use the effective scheme when choosing the default mnemonic HD index (
js/index.ts:213-219,js/index.ts:242-249). Both mnemonic signing wrappers encodeopts.scheme ?? params.scheme, but choose the default address index from onlyopts.scheme. Consequently, the supported params-only formsignTransferFromMnemonic(mnemonic, { scheme: "ml-dsa-65", ... })signs with ML-DSA-65 at index 0, whileaccountFromMnemonic(mnemonic, { scheme: "ml-dsa-65" })and the documented wallet-compatible default use index 1. A built-package check confirmed the embedded signer equals the explicit index-0 account and differs from the default index-1 account;signCallFromMnemonicbehaves the same way. Resolve the effective scheme once and use it for both encoding anddefaultAddressIndex, then cover the params-only form for both functions. Otherwise callers can sign from an unexpected account rather than the account they derived or funded.
PR #3 is merged, but this PR still targets fix/extrinsic-signing-context; retarget it to main before merging.
Validation:
npm ciandnpm test: pass (9 Rust tests, 14 JS tests, WASM build, TypeScript build).cargo check --locked --no-default-features --target wasm32-unknown-unknown: pass.cargo clippy --locked --all-targets --all-features -- -D warnings: pass.git diff --check: pass; GitHub CItest: pass.cargo fmt --all -- --check: fails with rustfmt diffs insrc/ext.rsandsrc/mnemonic.rs(the output includes both changed and pre-existing lines).
No other blocking code findings found in the reviewed diff.
The runtime accepts two signature schemes since spec 139:
DilithiumSignatureScheme::Dilithium87 (variant 0) and Dilithium65
(variant 1). Every entry point now takes an optional scheme
("ml-dsa-87" or "ml-dsa-65") in its options/params object. Omitting it
keeps the ML-DSA-87 behaviour, so existing callers are unchanged.
Rust: a Scheme enum and a scheme-agnostic Keypair wrapper so account
derivation and extrinsic assembly are written once. AccountKeys and the
JS Account expose the scheme. Mnemonic derivation uses the hdwallet
per-scheme modules; the JS default addressIndex is 0 for ML-DSA-87 and
1 for ML-DSA-65, matching the Quantus wallets.
Tests cover both schemes against qp-dilithium-crypto 0.7.0: account
ids, signature bytes equal to sp_core::Pair::sign, SCALE signature
field, context binding, and Verify on the decoded extrinsic. JS tests
add the wallet-known ML-DSA-65 mnemonic address and a frozen ML-DSA-65
extrinsic.
n13
left a comment
There was a problem hiding this comment.
Reviewer model: GPT 5.6 Sol
Verdict (advisory): Approve
No blocking findings at c5ce3b2338512e544615a1258670874f66a09dc6.
The earlier mnemonic-signing issue is fixed: both wrappers now resolve opts.scheme ?? params.scheme once and use that effective scheme for key derivation, default HD index selection, and signing context encoding. The new params-only regression coverage exercises both transfer and generic-call signing. The ML-DSA-65 key sizes, Poseidon account derivation, wire variant, context-bound signature bytes, and decoded extrinsic verification also match the canonical chain crates, while omission of scheme preserves the existing ML-DSA-87 outputs.
Validation:
npm test: pass (9 Rust tests, release WASM build, TypeScript build, 16 Node tests, and package-content verification).cargo clippy --all-targets --all-features -- -D warnings: pass.cargo check --no-default-features: pass.- Focused built-package checks for mnemonic option/parameter precedence and invalid schemes: pass.
git diff --check: pass; GitHub CItest: pass at the reviewed head.- Cross-checked current chain enum ordering (
Dilithium87 = 0,Dilithium65 = 1) and the CLI/app ML-DSA-65 default HD index (.../1').
Non-blocking: cargo fmt --all -- --check reports rustfmt-only layout differences in src/ext.rs and src/mnemonic.rs; formatting is not enforced by the current CI.
Stacked on #3 (signing context fix). Merge #3 first; this PR then retargets to
main.What
The runtime accepts two signature schemes since spec 139:
Dilithium87(variant 0) andDilithium65(variant 1). This adds ML-DSA-65 to the whole interface while keeping the existing API: every function takes an optionalscheme("ml-dsa-87"or"ml-dsa-65") in its options/params object, and omitting it is exactly the old ML-DSA-87 behaviour.account(seed, { scheme }),QuantusAccount.schemesignTransfer(seed, { scheme, ... }),signCall(seed, call, { scheme, ... })accountFromMnemonic/signTransferFromMnemonic/signCallFromMnemonicwithMnemonicOptions.scheme. DefaultaddressIndexis 0 for ML-DSA-87 and 1 for ML-DSA-65, matchingquantus wallet import --scheme ml-dsa-65and the mobile SDK, so the two schemes never derive from the same entropy.QUANTUS_EXTRINSICcontext and same Poseidon account derivation for both.Rust:
Schemeenum plus a scheme-agnosticKeypairwrapper, so derivation and extrinsic assembly stay written once.Tests
Rust (9 pass), each oracle check runs for both schemes against
qp-dilithium-crypto0.7.0: account ids, signature bytes equalsp_core::Pair::sign, SCALE signature field, context binding (empty/other context rejected), andVerifyon the decoded extrinsic signature field with a wrong-signer negative. Frozen golden digests for both schemes.JS (14 pass): default remains ML-DSA-87; ML-DSA-65 sizes;
accountFromMnemonic(MNEMONIC, { scheme: "ml-dsa-65" })equals the wallet vectorqzoyC4eRTrexYoutXABVsf61QJZxJim3iWvayRQwEjXWgA4mw; frozen ML-DSA-65 extrinsic with variant byte 1;signCall/signTransferagreement; invalid scheme throws.Live on a local
quantus-node --dev(spec 148): funded an ML-DSA-65 account fromcrystal_alice, then signed a transfer from it withscheme: "ml-dsa-65". Accepted (0xf35b6075…), included, balance and nonce moved.