Add typed-data signing using the shared protocol library - #112
Merged
Merged
Conversation
The previous branch was a root commit with no ancestor in common with main. Its tree was a partial, stale snapshot, so a diff against main reported 219 files and -32807 lines and silently reverted merged work: the txExecution helpers from #76, the internal-helper removal from #87, and validateNodeUrl's normalization to url.origin. Four of five test suites could not even import. Rebuild the branch on origin/main (e3e94c2) by re-applying the typed-data files rather than cherry-picking the snapshot: - src/shared/typedDataHash.js and its tests (hash v1 twin) - src/shared/blsDigest.js and its tests (BLS12-381 short signatures, V2 DST) - src/shared/fixtures/typed-data-v1/ golden vectors - scripts/check-typed-data-fixture-parity.mjs - @noble/curves and @noble/hashes dependencies Deliberately not carried over: the version downgrade to 0.2.0, and the dusk_signBlsDigest RPC case. The latter was never on main, and introducing a raw 32-byte signing oracle reachable by any connected dApp only to remove it in the next commit serves no purpose. RPC, engine, and approval-UI wiring follow in later phases. Spec now lives in dusk-network/connect as the single source of truth; docs/typed-data-v1.md is a pointer, and the cross-repo rollout is in docs/typed-data-v1-implementation-plan.md. (cherry picked from commit be0c45d)
handleRpc validated the caller's origin but never the method name, dispatching straight into `switch (method)`. Neither src/inpage.js nor src/contentScript.js filters method names either, so any case in that switch was callable by any connected dApp regardless of what dusk_getCapabilities advertised. Removing a method from the advertised surface affected discoverability, not access. That is fail-open: an internal or test-only case is public the moment it is added. Reject anything outside DAPP_RPC_METHODS before any permission lookup, settings read, or approval prompt, so an unknown method also costs nothing. Introduce DAPP_TOMBSTONED_METHODS for methods that are deliberately refused. dusk_getAddresses exists so dApps learn that enumerating shielded addresses is declined rather than getting a misleading "Unknown method" — the method is known, it is refused. A bare allowlist flattened that distinction, so the two categories are now declared separately and asserted to stay disjoint. The switch keeps its `default:` case as a backstop. Testing note: behavioural tests cannot distinguish the allowlist from `default:`, since both currently answer METHOD_NOT_FOUND — removing the guard leaves every behavioural test green. The allowlist earns its keep on cases added later, which `default:` would not catch because they would match a case. Two conformance tests cover it structurally instead: one asserts the guard precedes the switch, the other asserts every dusk_ case is canonical or tombstoned. Both were verified to fail when the code under test is reverted. (cherry picked from commit 39132cf)
Adds the sign_typed_data approval screen. Structured values are flattened to one row per leaf with a dotted path, the declared type, and a bounded display value, instead of being dumped as JSON. JSON was the obvious approach and is the wrong one. Pretty-printed output overflows a ~360px popup and pushes the Sign button off-screen, and scroll fatigue produces approvals without reading — the failure this screen exists to prevent. Flat JSON is unreadable once anything nests. Showing the declared type matters independently: `amount: "42"` is identical on screen whether the field is a uint64 or a string, and those sign different bytes. The flattening lives in src/shared/typedDataDisplay.js rather than in the popup because vitest runs with environment: "node" and no DOM implementation is installed, so UI logic embedded in app.js can only be tested by asserting on its source text. The pure module gets real behavioural tests; the render block stays a thin mapping. This mirrors signMessagePreview.js, which already exists for the same reason on the sign_message path. Field values are attacker-controlled and rendered on a signing screen, so strings are capped at 2048 characters and C0/C1 control characters, bidi overrides, and lone surrogates are replaced with a placeholder and flagged. A right-to-left override can make "send 1 DUSK" display as something else entirely. h() sets textContent, so markup injection was never the risk; visual spoofing is. The sign_message path already guarded this and the typed path must not be weaker — the shared control-character definition is now exported from signMessagePreview.js rather than duplicated. Structural limits are separated from content warnings. Depth is capped at 8 and rows at 200, but a subtree cut for depth emits a marker row at the cut point rather than incrementing a counter: a payload nested past the cap at the root would otherwise render an approval with zero visible fields and a "1 more field" notice, showing the user nothing while understating what was hidden. Only deceptive content raises the spoofing warning; a truncated value or an elided subtree reads as a display limit, because a warning that fires on benign input trains users to dismiss it. The digest is shown collapsed so it can be cross-checked against what the dApp claims it asked for. (cherry picked from commit 06fd2c6)
Adds the typed-data signing method to the dApp surface, per the spec in dusk-network/connect. The wallet validates the request, injects its own view of the requesting origin, hashes, obtains user approval showing the rendered fields, then signs. Validation is ordered so nothing expensive or user-visible happens on a bad request: connection, params shape, version, structural validation, chain match, and policy limits all run before any approval popup appears. Two guarantees that did not exist before: - The origin bound into the digest is always the handler's own, never params.origin. A caller able to set it could obtain a signature attributable to a site it does not control, which would defeat the entire point of origin binding. The hash input is assembled field by field rather than by spreading params, so a request field added later cannot leak in. - The active chain is re-read after approval returns and the request is rejected if it changed. A user can switch networks while the popup is open, and a signature must not outlive the context it was approved in. Validation runs on the assembled hash input rather than on the caller's raw params. Origin is a field of that input and the validator checks it, so validating raw params made a caller-supplied origin mandatory — meaning every dApp following the documented request shape would have received INVALID_PARAMS. The suite did not catch this because the test fixture always supplied a hostile origin to prove it was ignored, so no test ever exercised the documented shape. The fixture now omits origin by default and a regression test pins the documented request. Capabilities advertise signTypedDataVersions as an array rather than a scalar version. A scalar means "the one version I do", which forces a flag day: a v1-only dApp seeing 2 cannot tell whether v1 is still accepted, and a wallet supporting both has no way to say so. Result shape matches the spec and the sibling sign_* methods: `signature` rather than `signatureHex`, `publicKeyHex` rather than `fundsPkHex`, with origin, chainId, and primaryType echoed so a verifier need not hold the original request. (cherry picked from commit 000edef)
The typed-data digest is 32 bytes, which makes it indistinguishable from
every other 32-byte value the same Moonlight BLS key can be asked to sign,
including pay-auth digests used elsewhere under the identical DST. Anything
that signs a caller-supplied 32-byte value with this key could therefore
forge a typed-data signature, and a typed-data signature could be replayed
wherever a bare digest is accepted.
The signature now covers a tagged wrapper (spec 12.1):
SIG_TAG = utf8("DUSK_TYPED_DATA_SIG_V1\0") // 23 bytes
signedMessage = SIG_TAG || digest // 55 bytes
which makes the two message spaces structurally disjoint. The tag is applied
outside the digest because a value inside the SHA-256 preimage constrains the
preimage, not the 32-byte output, and the output is what gets signed.
The DST is deliberately unchanged. A typed-data-specific DST would give the
same separation, but the stock dusk-core verification path pins
BlsVersion::V2 and accepts no caller-supplied DST, so a custom one would make
these signatures unverifiable on-chain — a core goal. hashToCurve takes
arbitrary-length input, so the longer message needs no special handling
anywhere.
Adds the engine wiring the RPC layer already expects: signTypedData in
walletEngine and its dispatcher case in the engine runtime. Only the tagged
path is exported. The public surface has no raw-digest method and must not
grow one; signProfileBlsDigest and verifyBlsDigestSignature remain for
internal use and now carry comments saying they must never be reachable from
a dApp-facing RPC.
Tests pin the exact tag bytes and the 55-byte length, and assert both
directions of the separation: a bare-digest signature is rejected by the
tagged verifier, and a tagged signature is rejected by the bare-digest
verifier. Verified cross-repo with real keys — the wallet signs, Connect
verifies, and both cross-rejections hold.
(cherry picked from commit 86014f4)
Adds a dusk_signTypedData example alongside the existing provider example, with the two things an integrator gets wrong otherwise: origin must not be sent, because the wallet injects and returns its own, and domain.chainId must match the active chain or the request is rejected before any approval UI. Also notes that the signature covers a tagged wrapper around digestHex rather than the bare digest, since verifying the bare digest would accept signatures from any raw 32-byte signing path, and links the Connect specification. (cherry picked from commit 3596aed)
…rview Adds the Unreleased changelog entries and the dusk_signTypedData row in the architecture RPC table. Enforcing that table turned up a second omission: dusk_watchAsset was on the canonical surface and in both provider-api.md and SECURITY.md, but had never been added here, because conformance checked those two documents and not this one. Added the missing row and the missing assertion, so the table cannot drift again. Also removes the implementation plan from the repository. It was scaffolding for carrying out this work and describes a branch state that no longer exists; docs/investigations/typed-data-signing.md covers the reasoning that outlives it. (cherry picked from commit 8517dc3)
The docs previously proposed a disposition for #90, recommending it be closed as superseded. That is a decision for #90's own discussion, and stating it here invites a review of this change to become a debate about a different issue. The remaining references describe the technical relationship only: this work removes the coupling between the two, so a raw-digest capability becomes an explicit surface decision rather than a side effect of adding a handler, and whatever #90 concludes cannot produce or accept a typed-data signature. Nothing here argues for or against implementing it. (cherry picked from commit 70c9be9)
signProfileBlsDigest had no production callers. It is the capability issue #90 asks for, and shipping it here would have settled that issue's design — key derivation, DST, result shape — inside a change that is about typed data. #90 is getting its own change, so this module offers no ready-made signer for it. The low-level primitive signBlsMessageBytes stays: the typed-data path uses it, and the tests that assert a bare-digest signature is rejected by the typed-data verifier need to construct one. verifyBlsDigestSignature stays too. Verification is not a capability — it puts no key at risk — and application specific 32-byte digest schemes already in use need a verifier regardless of what #90 concludes. No coverage is lost. The removed test asserted a raw sign/verify round trip already covered by the primitive-level test, plus a profile-to-derived-key agreement already covered by the typed-data profile test. (cherry picked from commit 57e5dbd)
Signing with the profile key is a capability, so growing this module's surface should be a deliberate act rather than a side effect of another change. The export list is now pinned: adding one fails the test until someone updates the list, which makes it visible in review. A second assertion states the specific invariant plainly — exactly one profile-level signing path exists, and it is the typed one. Reintroducing a bare-digest profile signer must delete that assertion, so issue #90 gets an explicit decision in its own review rather than an implicit one in a diff. (cherry picked from commit 23ee28e)
dusk_signTypedData was written before the approval-lifecycle work landed and carried its own post-approval check: re-read settings, compare the chain, throw on mismatch. That solved a narrower version of the problem the shared mechanism now solves, and left two idioms for the same concern in one file. Adopt the sibling idiom. captureApprovalContext before the prompt, withStorageLock plus assertApprovalContext after it, and _approvalContext threaded to the engine. This widens the guarantee from chain only to node URL, profile index, permission identity, wallet identity and unlock state, and serializes the signing path against concurrent lifecycle changes. The digest is unaffected. It binds the dApp's domain.chainId, never the wallet's; the wallet's chain is used only to reject a mismatch and to display and echo. Sourcing it from the approval context changes where that comparison value comes from, not what is hashed. A mid-approval change now reports UNAUTHORIZED rather than INVALID_PARAMS. Nothing is wrong with the request — the wallet moved out from under an approval it had already granted, which is what the sibling methods report. Also adds the engine-side guard, which signTypedData was missing while the other twelve engine entry points had it. That check compares the engine's own view rather than the RPC layer's, so it catches drift the outer check cannot see. It was untested because engineCall is mocked in the RPC tests, and assertApprovalContext returns early when no context is passed, so a missing guard fails open silently. Two engine tests now cover it, and removing the guard fails the first. (cherry picked from commit 239559a)
The numbered step comments had drifted out of sequence — two 5s, two 8s, no 7 — and all eleven in the file were in this one case; no sibling uses them. Numbering rots whenever a step is inserted or removed, which is what happened. The prose is kept, the numbers are not, and one comment that referred to "step 4" now names the thing instead. (cherry picked from commit b8475d6)
Require identifier field names, reject unpaired surrogates before UTF-8 encoding, and retain own debug hashes for valid __proto__ type names. Test malformed inputs at the hash and pre-approval RPC boundaries while preserving well-formed Unicode. Vendor the two rejection vectors byte-for-byte from Connect 72ac9cd2cedab7e66427520234bd9f43fb4f5c50 and advance SOURCE provenance. Document the validation boundary for #22. (cherry picked from commit 51a3f4c)
Use Reflect.ownKeys so symbol-keyed properties cannot escape the extra-field rule. Retain non-enumerable string coverage, exercise all three hashing entrypoints and preserve declared non-enumerable fields. JSON transport behavior and frozen digests are unchanged. Refs #22. (cherry picked from commit e328848)
Translate hash-time value validation through the existing INVALID_PARAMS boundary, and visibly mark string previews that omit signed text. Add RPC and browser-rendering regressions without changing encoding or signing. Refs #22. (cherry picked from commit 42f94d439f3eae28d2340509b94540cc53225ce3)
Retain ichbindas’s consumer-side RPC, signing, approval and lifecycle work on current main, with its original authorship and source trailers. Use @dusk/typed-data for validation, hashing, resource limits and signed-message construction instead of importing the JS encoder and vendored corpus. Keep the existing RPC error mapping and clipped-preview fixes in a separate preceding commit. Add a packaged-fixture/profile-signer interoperability check using the required verification policy and structured result. The unpublished 0.1.0-next.0 dependency and its registry-backed lock entry remain a draft merge blocker. No local tarball paths are committed. Refs #22. Alternative to #101; the original PR is left open. Require an exact active-chain match before approval, without trimming the signed chain ID, to agree with the shared verifier policy.
There was a problem hiding this comment.
🟡 Changes recommended
Two critical findings and one moderate lifecycle finding remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds dusk_signTypedData using @dusk/typed-data, with Wallet-owned signing, approval UI, permissions, and lifecycle checks.
Changes:
- Adds typed-data RPC, validation, hashing, tagged BLS signing, and engine integration.
- Adds bounded approval previews with safety and regression coverage.
- Updates documentation and dependency metadata.
File summaries
| File | Summary |
|---|---|
tests/e2e/typed-data-preview.spec.js |
Preview clipping regression coverage. |
src/ui/notification/app.js |
Typed-data approval rendering. Critical (1 vote): sanitize domain.name and domain.version and include their flags in disclosures. |
src/ui/notification.app.test.js |
Approval UI integration assertions. |
src/shared/walletEngine.test.js |
Signing lifecycle tests. |
src/shared/walletEngine.js |
Wallet-owned typed-data signing. |
src/shared/typedDataIntegration.test.js |
Shared-library integration verification. |
src/shared/typedDataDisplay.test.js |
Preview safety tests. |
src/shared/typedDataDisplay.js |
Safe message flattening and bounded previews. |
src/shared/signMessagePreview.js |
Shared control-character detection. |
src/shared/providerSurface.js |
RPC surface declarations. Moderate (2 votes): register the method in DAPP_ACTIVITY_METHODS to update auto-lock activity. |
src/shared/providerSurface.conformance.test.js |
Provider-surface consistency tests. |
src/shared/blsDigest.test.js |
Tagged BLS signing tests. |
src/shared/blsDigest.js |
Key derivation and tagged signing helpers. |
src/engine/runtime.js |
Engine dispatch integration. |
src/background/rpc.test.js |
RPC validation and lifecycle tests. |
src/background/rpc.js |
Typed-data validation, approval, and signing flow. |
README.md |
Usage guidance. |
package.json |
Adds the shared dependency. Critical (1 vote): the lockfile lacks its root and resolved entries, so clean npm ci fails until regenerated with a published package. |
package-lock.json |
Dependency resolutions; the shared package’s registry-backed lock entry remains pending. |
docs/typed-data-v1.md |
Integration and provenance documentation. |
docs/SECURITY.md |
Security table updates. |
docs/provider-api.md |
Provider API documentation. |
docs/ARCHITECTURE.md |
RPC inventory updates. |
CHANGELOG.md |
Release notes. |
Review details
- Files reviewed: 23/24 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Decode typed-data bytes strictly as hex instead of guessing base64. Reuse the message sanitizer and rendering disclosures for domain strings without changing signing input. Cover all accepted hex spellings and each domain field with hash controls and actual rendered-preview regressions. Refs #22.
Register typed-data signing alongside the existing active dApp methods. Exercise background completion accounting with success, rejection and post-request lock controls. Refs #22.
Contributor
|
Two small things on the dependency and test side. Pin the Noble versions exactly
Worth a line in
So on a major Noble upgrade the assertion fails for a reason unrelated to the key, and the obvious fix — regenerating the constant — would silently replace a Rust-derived value with whatever the new library emits. Serializing by hand keeps the assertion meaning what it claims: function skScalarToBytes(skScalar) {
const out = new Uint8Array(32);
let rest = skScalar;
for (let i = 0; i < 32; i++) {
out[i] = Number(rest & 0xffn);
rest >>= 8n;
}
return out;
}Same fix as in the shared package, where the BLS vector generator avoids |
Use exact direct Noble versions and the already-locked resolutions. Serialize the frozen Rust scalar explicitly as checked 32-byte little-endian data instead of depending on Fr.toBytes conventions. Addresses ichbindas feedback in #112 (comment). Refs #22.
Keep the same pre-approval resource check and RPC error boundary while using the shared package’s explicit signer-policy entrypoint. Refs dusk-network/typed-data#2.
Member
Author
|
Addressed the Noble/scalar feedback in 43639e5, with the policy-import adaptation separately in 4f3829d.
Final validation: 655 Wallet tests, three frontend builds, four rendered-preview tests, fresh 25/25 production Chrome checks and seven real signatures verified in native Rust. The accompanying package cleanup is typed-data #3; registry publication/lockfiles remain blockers. |
Render accepted empty structs as explicit path/type rows, including array elements and row-budget omissions. Include U+061C in the existing bidi-control sanitizer so domain and message previews replace it and warn without changing signed input. Add shared-policy/hash acceptance, complete Unicode Bidi_Control, row-budget and actual-renderer regressions. Refs #22.
Member
Author
|
Fixed the two additional approval-display findings in b67aca6.
Before the fix: five intended unit and five rendered-preview failures, using empty-struct inputs accepted by the shared policy/hash implementation. After: 660 Wallet tests, 7/7 rendered-preview tests, three frontend builds, and fresh 30/30 production Chrome checks. All 12 real signatures verify in npm, JSR and native Rust; removing an empty marker or substituting the sanitized ALM display fails verification. The shared library is merged, but the fresh isolated clean-install probe still fails with the dependency's E404. This stays draft pending actual publication, registry-backed lockfiles and normal review/CI. No merge, release or Connect change. |
This was referenced Sep 13, 2026
Preserve both histories and combine the independent changelog entries. Retain conflict-aware provider selection while bringing the shared typed-data integration onto current main.
Reuse the existing JSR npm registry configuration and lock the exact shared RC. Load frozen fixtures from the installed bridge package and assert that encoder-work refusal remains E_COMPLEXITY, separate from E_POLICY_LIMIT, before approval or signing. Use the existing SDK request wrapper in the discovery-aware README example and describe the bounded preview honestly. Refs #22.
HDauven
marked this pull request as ready for review
September 15, 2026 12:01
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review wording follow-up
Current head:
516785c675fde49a0a2cf2490f8dacc0366c8799.Corrected the single changelog line that claimed every message field is rendered: it now describes bounded, potentially truncated message previews, matching the README. Every other tracked file is unchanged from reviewed
50f5c1f4155bdd8b49de9d670d36769d600d1e91; signing, trusted context, lifecycle, dependencies and locks are untouched.Fresh local clean install/coverage passes: 662 tests /59 files. Current-head CI passed: clean install, coverage/tests, coverage upload and Chrome/Firefox builds, with every job step successful. The browser/native-signature evidence below is retained, not rerun for a wording-only change.
The supplied review approved the integration/signing flow with the disclosure caveat; this is not a formal GitHub approving review or a claim that full disclosure is complete. Normal approving-review gates still apply. #113/#114 remain the explicit disclosure follow-up, and #114's own head/body/base branch name remain unchanged.
Summary
Add
dusk_signTypedDatausing@dusk/typed-data, without the duplicate encoder, vendored vectors or parity scripts from #101.Authorship and retained fixes
The integration was built from main, not #101's head. It ports 12 ichbindas commits and two earlier Hein follow-ups individually, preserving original authors, author dates and messages with source-SHA trailers. Protocol-only commits and obsolete twin-design documents are excluded. #101 remains open and untouched; see integration/provenance.
All subsequent reviewed fixes remain: numeric RPC mapping and visible clipping disclosures; safe hex/domain previews (
f8f09a847bac); activity accounting (1bb5fabfeb73); exact Noble pins and explicit little-endian Rust golden-test scalar serialization (43639e526c4f, addressing ichbindas's feedback); separate policy import (4f3829d6bef6); empty-struct markers and Arabic Letter Mark safeguards (b67aca66801e). Earlier red/green controls remain historical evidence, not new independent reviews.Published JSR dependency — native npm no longer required
The library is published as
@dusk/typed-data@0.1.0-rc.0, from release commitb46ac9ea020376d8f805d229cdf70b5310c304d5."@dusk/typed-data": "npm:@jsr/dusk__typed-data@0.1.0-rc.0", reusing the existing JSR registry configuration for w3sper.npm cisucceeds. No local tarball dependency or fabricated integrity is committed. Existing Noble versions/integrities remain unchanged./blsand/policy; fixtures are physically included but not exported. Tests read them from the pinned installed package with Node'sfindPackageJSON.E_COMPLEXITYrefusal, distinct fromE_POLICY_LIMIT, before approval/signing. The previous pre-budget library fails this new case for the expected code difference; the RC passes.npm ci, and typed-data signing uses the existing discovery-aware SDKwallet.requestwrapper.The previous publication/lockfile blockers are resolved. Protocol v1 remains draft, not frozen. No extension release or protocol freeze is part of this update.
Prior JSR integration validation
Head:
50f5c1f4155bdd8b49de9d670d36769d600d1e91. Current mainf36c064027301af1ec6a09b5cecfd01c44fd83c6was integrated through a merge commit, preserving every original integration commit and the reviewed discovery changes from #115. The JSR follow-up changes metadata, tests and docs, not Wallet runtime implementation. No workflow or protection changes.npm ci; complete coverage: 662 tests / 59 files.Prior integration-head GitHub CI passed: clean install, 662 tests, coverage upload and Chrome/Firefox builds; all job steps succeeded. All existing review threads are resolved. A formal approving review is still required before normal merge.
No independent encoder/security certification, Firefox signing E2E or native-desktop E2E is claimed.
Merge order and known follow-up
#113 records pre-existing bounded-preview limitations, explicitly nonblocking for #112 as integration regressions. The README no longer claims every message field is visible. #114 remains a separate stacked draft: integrate #112 normally, then reconcile/retarget/revalidate #114 against main; do not merge it into this feature branch as a shortcut. Its head and base branch name are unchanged by this update. Do not treat #112 alone as completion of full inspectable disclosure.
Refs #22. #101 stays open; the raw-digest design in #90 remains separate.