Hardware wallet support: Ledger accounts across wallet, dApps, and x402 - #149
Conversation
…et support
Replace raw-private-key threading with a Signer interface resolved by
getSigner(walletIndex): {getAddress, signTransaction, signMessage,
signTypedData}. All signing call sites (wallet IPC, tx recorder, x402
client) consume the interface; only the vault backend inside signers.js
ever touches key material.
signAndSendTransaction now signs then broadcasts as separate steps
(signer.signTransaction → provider.broadcastTransaction), which a
hardware signer requires — the provider only ever sees the serialized
signed tx. dApp wire-shape normalization (0x-hex personal messages,
JSON-string typed data) happens once in the factory so future backends
can't drift. x402's createVaultBackedX402Client becomes createX402Client:
it is backend-agnostic by construction now.
No behavior change intended.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VtT8ToNv26pYdwB1BvXVyu
Adds hardware-wallet accounts to the wallet list: derivedWallets[]
entries gain a type ('mnemonic' | 'ledger'); Ledger entries persist the
device-read address and derivation path since nothing can be re-derived
locally. getWalletRecord/getWalletList become the shared normalization
seam, and withVaultPrivateKey now refuses to derive a vault key at a
hardware account's index — the chokepoint guard that keeps any future
caller from silently signing with a phantom mnemonic key.
Main-process ledger module (src/main/wallet/ledger/):
- transport.js: node-hid transport behind a serialization queue (one
APDU exchange at a time), lazy native-module load, account discovery
over Ledger Live and legacy derivation schemes
- errors.js: APDU status words and transport errors mapped to stable
LEDGER_* codes with user-facing instructions
- signer.js: signer-factory backend; getAddress from the stored record,
signing fails closed until the device-confirmation flow lands
UI: "Connect Hardware Wallet" in the wallet selector opens a subscreen
that polls for a device with the Ethereum app open, pages through
device accounts, and adds the chosen one (no vault unlock needed).
Ledger accounts show a badge in the selector; private-key export is
blocked for them.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VtT8ToNv26pYdwB1BvXVyu
The Ledger signer backend now signs for real: transactions (sign/ broadcast split — the device signs the unsigned serialization, the provider broadcasts), EIP-191 personal messages, and EIP-712 typed data (full payload via TypedDataEncoder.getPayload, with a hashed-message fallback for older Ethereum apps). Every signing session first verifies the attached device derives the account's address at the stored path, so a different Ledger/seed fails with LEDGER_WRONG_DEVICE instead of silently signing from a foreign address. Transactions are deliberately signed without @LedgerHQ's hosted clear-signing resolution: it would post full tx contents to Ledger's registry, against the project's no-hosted-services rule. Plain ETH transfers display normally on-device; contract calls show as raw data until an opt-in setting or offline token registry lands. Approval UX: Ledger accounts skip the vault-unlock gate (no vault key involved) in the dApp tx/sign dialogs and the send flow; pending states read "Confirm on your Ledger" while the device waits for a physical confirmation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VtT8ToNv26pYdwB1BvXVyu
x402 signing already flows through the signer factory since the WP1 refactor, so Ledger accounts produce EIP-3009 payment authorizations on-device with no protocol changes. This wires the flow around it: - The payment approval card skips the vault-unlock gate for Ledger accounts (no vault key involved) and shows "Confirm on your Ledger" while the device waits for the physical confirmation. - The auto-pay grant editor states plainly that auto-pay skips the dialog but never the device confirmation — a spend cap on a hardware account cannot pay silently, by design. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VtT8ToNv26pYdwB1BvXVyu
flotob
left a comment
flotob
left a comment
There was a problem hiding this comment.
Adversarial review of head ab23552: I found no additional blocking issue in the Ledger account model, serialized device transport, wrong-device check, signer abstraction, transaction/message/typed-data paths, x402 integration, or native-build coverage. The deliberate blind-signing/privacy tradeoff is clearly surfaced and the cryptographic verification tests are appropriate. CI is green; refresh from main before merge. This remains the required base for #159.
flotob
left a comment
flotob
left a comment
There was a problem hiding this comment.
Fresh re-review of unchanged head ab23552f, including signer abstraction boundaries, account-type isolation, Ledger address verification, transaction/message/x402 paths, packaging, and current base/CI state: no blocking findings.
Current status: 36/36 checks successful. This stacked base is still behind main; refresh it before merge.
flotob
left a comment
flotob
left a comment
There was a problem hiding this comment.
Re-re-review of unchanged head ab23552f: no new code, review follow-up, CI regression, or signer/account-isolation finding. No blocking issues. 36/36 checks successful; stacked branch remains behind main.
meinharrd
added
alan:reviewing
|
[alan-review R1] Blocking — Ledger accounts are offered as Swarm publisher identities but can never sign, leaving a persistently broken origin identity
But Swarm signing needs a raw private key: Failure scenario: user adds a Ledger account (this PR's connect flow) → a Swarm-enabled site requests feed access → the identity chooser (swarm-connect prompt, or Publisher Identities settings via Fix: filter |
|
[alan-review R1] Blocking — sign/broadcast split regresses fee handling: a tx with no fee params is now signed with
Every node rejects that broadcast (underpriced / below base fee), surfaced to the user as the misleading 'Gas estimation error. The transaction may fail.' Reachable path: Fix options: populate fees in |
|
[alan-review R1] Minor findings (combined):
Verification notes: full unit suite passes locally on the merge head (only the known pre-existing |
|
[alan-verify R1] Both findings CONFIRMED — I tried to refute each and could not; both reproduce empirically. R1-F1 — Ledger accounts offered as Swarm publisher identities — CONFIRMEDI tried three refutation angles, all failed:
Downstream is then unconditionally broken for that origin: Note this is a reachability regression, not a diff-line regression — R1-F2 — 0-fee signing after the sign/broadcast split — CONFIRMEDReproduced end-to-end through the real Same input on So the fee data really was coming from
Result: a type-2 tx every node rejects as underpriced — and on a Ledger, only after the user physically confirmed it on-device. No repo state changed; scratch test files removed. |
…ssing tx fees R1-F1: Swarm feed/SOC signing needs a raw private key (resolveSignerKey), which a Ledger account can never hand out. The identity chooser still offered them, and ensureEthereumWalletIdentity persisted+activated one with no type check, leaving the origin with an identity whose every feed operation failed with an opaque INTERNAL_ERROR. feed-store now filters hardware accounts out of the offered identity list, refuses to persist one, and flags any already-stored hardware-backed identity as unavailable; the shared publisher identity selector renders unavailable rows greyed out and inert. R1-F2: the sign/broadcast split dropped ethers' fee population — with no fee params the tx was signed with maxFeePerGas = 0 and rejected by every node as underpriced, on Ledger only after the user confirmed on-device. signAndSendTransaction now resolves missing fees from the network before signing, and refuses to sign at all when no usable price is available.
|
[alan-fix R1] Both confirmed findings fixed in 32f44fc. R1-F1 — Ledger accounts offered as Swarm publisher identities ( Root cause: Swarm feed/SOC signing needs a raw private key (
Acceptance evidence — the greyed, unselectable row as a user sees it (real app, harness run):
R1-F2 — sign/broadcast split regressed fee handling (
Tests / verification
|
meinharrd
added
alan:reviewing
|
[alan-review R1] (post-fix review of head
Failure A — in-app stranded funds. Main(0) + mnemonic Wallet 2 (index 1, funded). Delete Wallet 2, then add a Ledger account → the Ledger takes index 1. The mnemonic account at derivation index 1 is now unreachable in-app: Failure B — silent signer swap without consent. Fix direction: never reuse indexes (persist a monotonic |
|
[alan-review R1] (post-fix review of head
Failure scenario (Ledger account): user clicks Confirm → sidebar shows "Confirm on your Ledger…", device shows the tx. User changes their mind and clicks Reject in the sidebar: the dApp promise settles as rejected (4001) and the screen closes — but the device prompt is still active. If the user then presses approve on the Ledger (plausibly believing that's how to clear the prompt), main signs and broadcasts the transaction: funds move for a request the dApp saw as rejected, and the hash is delivered to no one ( Fix: disable Reject/Back (and the screen-hider path) while the sign IPC is in flight for the hardware path — or track a generation/closed flag and make the continuation drop broadcast + auto-approve when the screen was closed. Note the same pattern exists in |
|
[alan-review R1] (post-fix review of head
Verification notes: touched unit suites pass at head 32f44fc (390/390 across wallet/x402/feed-store/identity/preload). Checked and clean: hw-app-eth sig reassembly (v normalization incl. EIP-155), explicit |
|
[alan-review R1] (review of head
The module header (signer.js:14-17) says transactions are deliberately signed without hosted clear-signing resolution because it "violates this project's rule against routing user data through hosted services" — but that opt-out ( Secondary harm: the axios calls have no timeout and run inside Fix: pass a load config that nulls the hosted endpoints ( |
|
[alan-review R1] (review of head Two distinct holes in (a) Re-entrance via the accept chooser. (b) Stale mainFrame continuation clobbers the next card. For a mainFrame 402, Also worth fixing in the same pass: Fix shape: |
|
[alan-review R1] (review of head The PR deliberately signs with
Failure scenario: fresh Ledger (blind signing off — the factory default), user tries a USDC send (or any dApp tx / x402 payment with calldata). The device shows nothing signable, the app fails, and the UI tells them to reconnect the device — which cannot help. Reconnect-retry loops forever; the actual fix (enable Blind signing in the device's Ethereum app settings) is never surfaced anywhere, even though signer.js's own header comment (lines 18-20) predicts exactly this situation. This is a guaranteed dead end on a core advertised flow ("ERC-20 sends … may need blind signing enabled" — the PR description), not an exotic path. Fix: add a dedicated code (e.g. |
|
[alan-review R1] (review of head
Verification notes for this round: full unit suite at head passes (2230 passed; only the known pre-existing |
|
[alan-verify R1] All four findings CONFIRMED, each reproduced against the code as written (b22f94f). R1-F1 — CONFIRMED (empirical). Drove the real
R1-F2 — CONFIRMED (e2e, real renderer + real app). Multi-accept card, Ledger account, hanging On the mainFrame path that second call falls through the staleness check (
R1-F3 — CONFIRMED (unit test against the real module). R1-F4 — CONFIRMED (empirical). Fed hw-app-eth's Verdict: 4 confirmed, 0 refuted. Repo left clean (probe scripts and the temporary spec deleted). |
Four confirmed review findings on the hardware-wallet PR. R1-F1: hw-app-eth was constructed with no load config, so its defaults applied and signTypedData routed every EIP-712 signature through Ledger's hosted registry (crypto-assets-service.api.ledger.com): chain id, verifying contract and a schema hash of the typed data left the machine before the device was touched, from inside the exclusive device queue. The transport now builds Eth with an offline load config that nulls every service URL, which is what makes the library skip the lookups. Verified empirically against the installed hw-app-eth: default config attempts one request to crypto-assets-service; the offline config attempts none. R1-F2: x402 approve() had no re-entrance guard, and a chooser flip mid-signature re-rendered the card and re-enabled Pay — a second click started a second, concurrent device signature. approve() now guards on pending.signing (as dapp-tx/dapp-sign do), renderCard re-applies the in-flight lock, and a chooser change while signing snaps the radio back to the accept that is actually on the device. R1-F3: the approve() continuation had no flight-identity check, so a stale one (its card torn down by the cancelled-tab path) could release or tear down the card that replaced it. approve() captures its request and settles only that: endSignatureFlight is identity-scoped, and both the success and failure continuations bail out when the card has moved on. R1-F4: blind-signing refusals (APDU 0x6a80 / EthAppPleaseEnableContractData) mapped to LEDGER_UNKNOWN and told the user to reconnect the device — the one failure the null-resolution design guarantees on a factory-default device. They now map to LEDGER_BLIND_SIGNING_REQUIRED with an instruction naming the setting to enable.
|
[alan-fix R1] All four confirmed findings fixed in de8a434. R1-F1 — EIP-712 signing went through Ledger's hosted services ( Verified empirically against the installed New tests: the Eth app is always constructed with that config, plus a guard that resolves it through the library's own R1-F2 — second concurrent device sign from the chooser ( R1-F3 — stale continuation settling the next card ( R1-F4 — blind signing disabled surfaced as "Reconnect the device" ( Verification — full unit suite (2238 passed; only the known Acceptance evidence, real app, Chooser flipped mid-payment — the radio stays on the accept the device holds, details still read 2.5 USDC, Pay stays "Confirm on your Ledger…" and disabled, Reject/Back inert, and main sees exactly one
Blind-signing refusal (message produced by the real main-process mapper) — actionable instruction, card usable again for a retry:
One note for the author: the PR description's clear-signing paragraph now understates the change — the no-hosted-services rule is enforced for typed data as well as transactions, via the transport's load config. Worth a line before merge. |
|
[alan-review R2] Blocking — CI is red at head
Failing checks right now: This was raised as R1 minor #4 ("cheap to add Fix: |
|
[alan-review R2] Blocking — x402 EIP-3009
const now = Math.floor(Date.now() / 1e3);
const authorization = { …, validBefore: (now + paymentRequirements.maxTimeoutSeconds).toString(), … };
const signature = await signEIP3009Authorization(signer, authorization, …); // ← open-ended device wait
Failure scenario: paywall card → Pay → "Confirm on your Ledger…" → user takes >60s on the device (reading the payload, or enabling blind signing mid-flow and retrying navigation) → signature succeeds, header stashed ( Fix direction (writer's call): after the signature resolves, check |
|
[alan-review R2] Minor findings (combined):
Verification notes for this round: the four de8a434 fixes check out — |
|
[alan-verify R2] Both findings survive refutation — confirmed. R2-F1 — Failing: R2-F2 — { "stampedAt": 1786561039, "validBefore": 1786561041,
"signatureReturnedAt": 1786561042, "expiredOnArrival": true, "secondsAlreadyExpired": 1 }Refutation attempts that failed:
No visual evidence attached: F1 is a CI-log fact and F2 needs a physical Ledger (the |
…ization R2-F1: all four Linux `libudev-dev` installs ran against the runner image's pre-seeded package index, which can name a build the mirrors have already rotated out — the fetch 404s and three ubuntu jobs fail before they start. `apt-get update` first, at every site. R2-F2: the x402 SDK stamps `validBefore = now + maxTimeoutSeconds` before it asks the signer to sign, and on a Ledger that call blocks for as long as the user takes to review the EIP-712 payload. A confirmation slower than the server's window (often ~60s) yields a valid signature over an already-expired authorization; dispatching it burns the charge — the facilitator refuses it, the server re-402s, the loop guard declines to re-sign, and a `failed` row lands after the user physically confirmed on the device. Check the runway left on the signed authorization against the facilitator's own `validBefore < now + 6` rule and throw before anything is stashed or navigated. The detection stays in the map and no pending payment is armed, so the approval card shows the error with Pay live again — a second device prompt, but no reload and no bogus failure row. Widening the window instead isn't available: v2 echoes the selected requirements as `payload.accepted`, which the server deep-equals against its own accepts.
|
[alan-fix R2] Both confirmed findings fixed in R2-F1 — R2-F2 — EIP-3009 The obvious fix — padding So the fix stops the doomed dispatch instead: after signing,
Tests: 4 new cases in Visual acceptance evidence — the card after the guard fires, error inline, Pay and Reject live again:
Verification run locally: |
|
[alan-review R3] No blocking findings at head
CI note: head Verification notes for this round: both |
meinharrd
added
alan:clean
…tor spec 1. MIN_AUTHORIZATION_RUNWAY_SECONDS 6 -> 20: the client-side runway gate matched the facilitator's bare 6s threshold exactly, leaving no margin for the re-navigation + server round-trip + verify hop, so a signature landing with ~6-10s of runway passed the gate yet could still be refused server-side (the burned-charge case the gate exists to prevent). A false positive only re-shows the 'try again' card. 2. publisher-identity-selector.spec: wait for the dropdown to settle closed before the reopen-for-screenshot toggle — under CPU contention a reopen click landing mid-close raced the toggle and left the list hidden.
|
Fixed both R3 minors in
Lint clean. Note: the one red check on the prior head ( |
# Conflicts: # package-lock.json # package.json # src/main/preload.test.js # src/renderer/lib/sidebar.js # src/renderer/lib/wallet/swarm-connect.js
The main-merge regenerated package-lock.json from main's lockfile, which re-resolved node-hid to compile its libusb backend from source; the e2e jobs then failed at 'Install dependencies' on 'libusb.h: No such file' (only libudev-dev is installed). Restore #149's original node-hid resolution (verified byte-identical) so the working prebuilt/hidraw install path is used again.
|
The behind- Cause: merging Fix: restored #149's original Recommended follow-up (I couldn't push it — the bot token lacks |
node-hid can compile either its hidraw (libudev) or libusb backend depending on the runner image; the e2e jobs only installed libudev-dev, so a libusb-backend build failed on 'libusb.h: No such file'. Install both dev libraries in the four e2e 'Install Linux native USB build dependency' steps so the compile succeeds regardless of backend.
|
Landed the recommended follow-up in |
2 participants
Summary
Adds Ledger hardware-wallet support end to end: connect a device, add its accounts alongside mnemonic wallets, and sign everything — manual sends, dApp transactions/messages/typed data, and x402 payment authorizations — on the device. Verified end to end against a Ledger Stax (connect flow, send, dApp signing, x402 pay).
Architecture
getSigner(walletIndex)→{getAddress, signTransaction, signMessage, signTypedData}insrc/main/wallet/signers.js. The account'stypein vault-meta picks the backend: vault-borrowed mnemonic key or Ledger device. Transaction signing is split from broadcasting (signer.signTransaction→provider.broadcastTransaction), which a hardware signer requires. The x402 client (createX402Client) is backend-agnostic by construction.derivedWallets[]entries gaintype: 'mnemonic' | 'ledger'; Ledger entries persist the device-read address and derivation path (nothing is derivable locally). Newsrc/main/wallet/ledger/module: node-hid transport behind a serialization queue (one APDU exchange at a time; native module lazy-loaded so boot pays nothing), account discovery over Ledger Live + legacy path schemes, and stableLEDGER_*error codes with user-facing instructions. Safety chokepoint:withVaultPrivateKeyrefuses to derive a vault key at a hardware account's index — no code path can silently sign with a phantom mnemonic key.LEDGER_WRONG_DEVICEinstead of signing from a foreign address.UX
Privacy decision (reviewers: please weigh in)
Ledger's hosted clear-signing resolution (token/plugin metadata) is deliberately not used — it would post full transaction contents to Ledger's registry before signing, against the project's no-hosted-services rule. Plain ETH transfers display normally on-device; ERC-20 sends show raw calldata and may need blind signing enabled in the device's Ethereum app. Follow-up options: opt-in setting, or a bundled offline token registry.
Testing
personal_sign/eth_signTypedData_v4, transaction approval, x402 payment, wrong-device and reject-on-device error paths.bee-to-ant-migration.test.jsalso fails onmain(antd identity adoption) — not touched by this branch.🤖 Generated with Claude Code
https://claude.ai/code/session_01VtT8ToNv26pYdwB1BvXVyu