Skip to content

Add Ledger EIP-712 signing to Guardian - #488

Merged
zeljkoX merged 6 commits into
OpenZeppelin:miden-0.17from
inicio-labs:vaibhav/ledger-eip712-production
Sep 24, 2026
Merged

zeljkoX merged 6 commits into
OpenZeppelin:miden-0.17from
inicio-labs:vaibhav/ledger-eip712-production

Conversation

@VAIBHAVJINDAL3012

@VAIBHAVJINDAL3012 VAIBHAVJINDAL3012 commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Add EIP-712 signing for Miden multisig proposals and Guardian request authentication so a Ledger signer can create and approve proposals through the existing raw-signer flow. Guardian attributes each action to the verified signer. Transaction-summary signatures are checked against the known public key, matching the Miden MASM verifier; raw signing remains unchanged.

The Rust and TypeScript clients share the same EIP-712 signature handling, and the authenticated HTTP routes document the optional auth-format header.

Validation

  • Guardian server: 966 passed, 2 ignored
  • Shared Rust: 46 passed
  • Rust multisig signing/advice, export, and proposal tests: 60 passed
  • TypeScript guardian client: 91 passed; multisig typecheck and focused Ledger/EIP-712 tests passed
  • Rust formatting and diff checks passed

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: c8bf50aa-a9fe-4484-b9a9-522a6ba92ea7

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@VAIBHAVJINDAL3012

Copy link
Copy Markdown
Contributor Author

I confirm that I have read and hereby agree to the OpenZeppelin Contributor License Agreement

@VAIBHAVJINDAL3012
VAIBHAVJINDAL3012 marked this pull request as ready for review September 24, 2026 09:42
@VAIBHAVJINDAL3012 VAIBHAVJINDAL3012 changed the title [WIP] Add Ledger EIP-712 signing to Guardian Add Ledger EIP-712 signing to Guardian Sep 24, 2026
@VAIBHAVJINDAL3012
VAIBHAVJINDAL3012 force-pushed the vaibhav/ledger-eip712-production branch from 444d8e6 to 2095cac Compare September 24, 2026 10:41
@zeljkoX

zeljkoX commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Thanks for working on this, especially the signer-attribution fix.

Before merge, I'd like stronger coverage for the TypeScript-generated signature advice. buildEip712SignatureAdviceEntry constructs the advice key and 32-felt witness manually. The current tests check the key's shape and part of the witness, but don't compare the complete output against upstream. Guardian verifies the approval signature against the public key and digest, so an incorrect witness could pass submission and then fail during transaction execution.

Please add:

  • An exact expected advice key and all 32 witness values generated by upstream eip712_signature_advice, ideally using upstream's MetaMask vector.
  • A mock-chain test in crates/contracts with mixed raw and EIP-712 approvals, including rejection of a wrong signer or transaction summary, and of an EIP-712 witness placed under the raw key.
  • The same happy path in mock-chain-execution.test.ts, exercising the TypeScript-generated advice.

Also, spec/api.md says Guardian accepts v as 27/28 for approvals, but the decoder rejects recovery ids above 3, and the same page elsewhere requires 0/1. Please make it say 0/1 consistently.

A few non-blocking suggestions:

  • Expose the helpers upstream. Rust already uses miden-standards' eip712_hash and eip712_signature_advice, but the web SDK doesn't expose them yet. Would it make sense to open an issue in 0xMiden/web-sdk? Delegating to upstream would remove the duplicated encoding logic and keep it aligned when we update the pinned SDK.
  • Consider naming it Eip712Signer. The implementation works with any compatible EIP-1193 wallet, so LedgerSigner sounds narrower than its actual scope.
  • Accept the proposer's own EIP-712 approval on push. Reusing the signing endpoint's validation, including binding the approval's public key to the authenticated proposer, could save one request-authentication prompt. The proposal approval would still require its own signature.
  • Extend the prompt documentation. The README already explains that approvals, submissions, and authenticated reads prompt the device. A call-by-call create/approve/execute example would help integrators understand the full interaction. Also clarify that the proposal id is the transaction-summary commitment, which may differ from the EIP-712 digest displayed by the wallet.
  • Update docs/MULTISIG_SDK.md. Cover the new signer and explain the distinction between x-auth-format for request authentication and message_format for proposal approvals.

@VAIBHAVJINDAL3012

Copy link
Copy Markdown
Contributor Author

Thanks @zeljkoX — I followed up in c8fb52b.

  • Added the MetaMask vector with an exact advice key and all 32 witness values, checked against the upstream Rust key derivation and signature encoding.
  • Added Guardian contracts mock-chain coverage for mixed raw/EIP-712 execution and rejection of a wrong signer, wrong transaction summary, or EIP-712 witness under the raw key.
  • Added a TypeScript mock-chain execution test using the TypeScript-generated EIP-712 advice.
  • Corrected the approval API documentation to require v = 0/1, exposed Eip712Signer while retaining LedgerSigner as an alias, and documented the wallet prompts and the distinction between request auth and proposal approval.

Validation: the TypeScript package build and 17 focused tests passed; the Rust contracts suite passed (29 tests, with 2 existing ignored tests). I also attempted the full TypeScript suite, but it produced no progress for over two minutes, so I stopped it rather than claim a pass.

I kept approval-on-push out of this PR as a separate design change. The upstream web-SDK helper work is tracked in 0xMiden/web-sdk#428.

@zeljkoX zeljkoX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.
LGTM

@zeljkoX
zeljkoX merged commit c3b1450 into OpenZeppelin:miden-0.17 Sep 24, 2026
3 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants