Skip to content

fix: sign through an embedded local account, and survive hosts without dynamic import (release 0.1.5) - #84

Merged
joeblau merged 2 commits into
mainfrom
fix/local-account-routing-and-viem-import
Jul 28, 2026
Merged

fix: sign through an embedded local account, and survive hosts without dynamic import (release 0.1.5)#84
joeblau merged 2 commits into
mainfrom
fix/local-account-routing-and-viem-import

Conversation

@joeblau

@joeblau joeblau commented Jul 28, 2026

Copy link
Copy Markdown

Bumps to 0.1.5. Two fixes reported from downstream use.

1. A viem WalletClient over a local account lost the raw-digest fast path

createWalletClient({ account: privateKeyToAccount(key), … }) — what wagmi and viem hand around when the key lives in process — always satisfies the JSON-RPC structural guard, because it carries signTypedData, getAddresses and getChainId. So it was adapted as a remote wallet and every L1 action went through generic typed-data encoding, even though the key was right there. Measured downstream at 2.17× on every order, with no warning and no way to notice short of reading the adapter.

Why not just re-route it to the local adapter. That adapter hardcodes getChainId to "0x1" ("local accounts have no notion of chain"), and getChainId feeds signatureChainId for user-signed actions — so the EIP-712 domain would have silently stopped reporting the chain the signature was actually produced on.

Instead adaptViemJsonRpc now additionally sources signDigest from the embedded account. Typed data, address, chain ID and every cache still go through the client. It's a pure addition: only L1 digest signing changes, which is precisely where the cost was.

Detection is deliberately narrow — viem's own type: "local" marker plus a usable raw-digest signer. A client over a remote account keeps signing through the client.

Verified against a real WalletClient: L1 signature byte-identical to the bare account, account.sign called while client.signTypedData is called zero times, chain id still 0xa4b1 rather than 0x1, and a type: "json-rpc" client still routed through the client. Four regression tests.

2. createFastLocalWallet hard-failed where dynamic import is unavailable

Its docstring promised the tiny-secp256k1-missing path was "never a hard failure", but the fallback does await import("viem/accounts"), which throws outright in hosts that cannot service a dynamic import — Jest without --experimental-vm-modules, and some React Native bundlers. Not theoretical: it's why a downstream mobile build was reverted off the fast wallet, and it put the README's React Native claim in question.

The specifier has to stay dynamic. viem is not a dependency of this package (only a devDependency), so a static import would break every consumer that doesn't use viem — which is why a blind static import isn't the fix.

Callers in such hosts now pass options.privateKeyToAccount, used on both viem-dependent paths (the fallback and the lazy signTypedData delegate). When viem is genuinely needed and can be neither imported nor supplied, the error names the cause and the remedy and preserves the host's original error as cause, instead of surfacing an opaque message from deep in the signing path. The docstrings no longer promise a fallback that can't be delivered.

Note: with tiny-secp256k1 present and only L1 actions signed, viem is never loaded at all — the exposure is the fallback and signTypedData.

Also

The v0.1.4 release notes gained a Behavioural change section recording that HyperliquidEventTarget stopped being an EventTarget subclass in that patch release: addEventListener/removeEventListener semantics are preserved, but instanceof EventTarget is now false and dispatchEvent is gone.

Verification

bun run check exits 0; 1658 pass, 236 skip, 0 fail (+6 tests). Unlike #83 this PR touches no perf scenarios, so the Performance gate should compare cleanly rather than fail closed on a suite-fingerprint change.

🤖 Generated with Claude Code

…t dynamic import

Release 0.1.5. Two fixes reported from downstream use.

A viem WalletClient built over a local account lost the fast path

`createWalletClient({ account: privateKeyToAccount(key), ... })` — what
wagmi and viem hand around when the key lives in process — always
satisfies the JSON-RPC structural guard, since it carries signTypedData,
getAddresses and getChainId. It was therefore adapted as a remote wallet
and every L1 action went through generic typed-data encoding, even
though the key was right there. Measured downstream at 2.17x on every
order, with nothing to indicate it was happening.

Re-routing such a client to the local adapter would have been wrong:
that adapter hardcodes getChainId to "0x1" ("local accounts have no
notion of chain"), and getChainId feeds signatureChainId for
user-signed actions, so the EIP-712 domain would have silently stopped
reporting the chain the signature was produced on. Instead the JSON-RPC
adapter now additionally sources `signDigest` from the embedded
account. Typed data, address, chain ID and every cache keep going
through the client, so this is a pure addition: only L1 digest signing
changes, which is exactly where the cost was.

Detection is deliberately narrow — viem's own `type: "local"` marker
plus a usable raw-digest signer. A client over a remote account keeps
signing through the client.

createFastLocalWallet hard-failed where dynamic import is unavailable

Its docstring promised the tiny-secp256k1-missing path was "never a
hard failure", but the fallback does `await import("viem/accounts")`,
which throws outright in hosts that cannot service a dynamic import —
Jest without --experimental-vm-modules, and some React Native bundlers.

The specifier has to stay dynamic: viem is not a dependency of this
package, so a static import would break every consumer that does not
use viem. Callers in such hosts now pass `options.privateKeyToAccount`
instead, which is used on both viem-dependent paths (the fallback and
the lazy signTypedData delegate). When viem is genuinely needed and can
be neither imported nor supplied, the failure now says so and names the
remedy, preserving the host's original error as `cause`, rather than
surfacing an opaque message from deep in the signing path. The
docstrings no longer promise a fallback that cannot be delivered.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@joeblau

joeblau commented Jul 28, 2026

Copy link
Copy Markdown
Author

The perf red is a measurement artifact, not a regression

This PR changes only src/signing/_abstractWallet.ts and src/signing/_fastWallet.ts (plus their tests and the version). subscription/subscribe_user_trio measures subscription setup and cannot be reached by either change.

The per-round numbers from the job show why it flagged:

round measured first measured second
1 (head, base) 57.15 µs (head) 39.01 µs (base)
2 (base, head) 57.75 µs (base) 40.58 µs (head)
3 (head, base) 57.91 µs (head) 41.49 µs (base)

Whichever revision runs first in a round costs ~57.5 µs and whichever runs second costs ~40.4 µs — independent of which revision it is. It is a position-in-round effect, not a revision effect. The reported band [-29.7%, +177.2%] spans zero for the same reason: the rounds disagree in sign.

Because the alternation is (head, base), (base, head), (head, base), head occupies the slow first slot in two of three rounds, so the median is systematically biased against head. That means this scenario will flag a false regression on most PRs, not just this one.

subscribe_user_trio was added in #83 and is the scenario most exposed to this: its per-subscription figure is over half fixed transport setup cost, so a cold first process in a round dominates it. warmupSamples: 3 warms within a scenario run but not across the two processes a round spawns.

Not fixing it here — that would edit tests/perf, changing the suite fingerprint and turning the gate red for a different reason, and it does not belong in a signing fix. Worth its own PR (adding warmup that survives process start, or reducing the fixed-cost share of the measurement).

Every other check is green: test, quality, docs. 52 of 53 scenarios unchanged, 0 genuinely faster or slower.

@joeblau

joeblau commented Jul 28, 2026

Copy link
Copy Markdown
Author

Correction to the analysis above

I attributed the swing to a cold first process in each round. That is wrong — every other scenario is stable across the same two positions, so there is no general cold-start effect. The instability is specific to subscribe_user_trio.

Same three rounds, first-measured vs second-measured:

scenario R1 R2 R3
l2book_dispatch_50_coins 2.49 → 2.65 2.51 → 2.48 2.53 → 2.55
subscribe_200_coins 14.15 → 13.74 14.12 → 15.07 14.47 → 14.48
http_request 2.97 → 2.74 3.00 → … 2.86 → 3.18
subscribe_user_trio 57.15 → 39.01 57.75 → 40.58 57.91 → 41.49

Everything else moves a few percent. subscribe_user_trio lands in one of two tight modes — ~57.5 µs or ~40.4 µs — a 40% gap, with per-run rme of only 5–12%. So it is stable within a process and bimodal across processes: some process-level state it depends on settles differently each time. In these three rounds the high mode happened to land on the first-measured process every time, which is what produced the +39.6% median against head.

That does not change the conclusion, and arguably strengthens it:

  • This PR changes only src/signing/_abstractWallet.ts and src/signing/_fastWallet.ts. subscribe_user_trio measures subscription setup and is unreachable from that diff.
  • The reported band [-29.7%, +177.2%] spans zero — the rounds disagree in sign.
  • The scenario is bimodal by ~40% while every neighbour is stable, so it will keep producing coin-flip verdicts on unrelated PRs.

The follow-up is to find what the scenario's cost depends on across processes and pin it, rather than to widen the threshold or retry the job. Still not doing that here: it would edit tests/perf, change the suite fingerprint, and does not belong in a signing fix.

An adversarial review of c76e21e confirmed three low-severity issues,
all in the new viem-resolution path, plus stale comments the routing
change invalidated.

- The error thrown when `viem/accounts` cannot be loaded asserted one
  cause ("this environment cannot service a dynamic import") and gave a
  remedy — install tiny-secp256k1 — that is impossible on one of its two
  call sites: the `signTypedData` delegate only exists when
  tiny-secp256k1 already loaded. It now names which path needed viem and
  offers both real remedies, since viem simply not being installed is
  the likelier cause.

- An injected `options.privateKeyToAccount` was never checked against
  the key the wallet signs with. On the WASM path `address` and the
  raw-digest signer come from tiny-secp256k1 over the private key while
  `signTypedData` delegates to the injected factory, so a factory closed
  over a different key — `() => myAccount` type-checks — would sign L1
  actions and user-signed actions as two different accounts and report
  only one. The delegate's address is now checked before it is memoized.

- `PrivateKeyToAccount` is the declared type of a public option but was
  not reachable from `@bloxwap/hyperliquid/signing`; it is now
  re-exported so consumers can name it.

- Four comments stated "JSON-RPC wallets never have signDigest", which
  the embedded-local-account routing makes false. They now distinguish
  wallets that can only sign remotely.

The review also reproduced a split-identity case: because the embedded
account is captured when the adapter is memoized while address and chain
ID stay live reads, a wallet object whose `account` changes could sign
L1 actions with the old key. Left as-is deliberately — verification
downgraded it to low after establishing that viem's own bound actions
close over the client at creation, so reassigning `client.account` is
already a no-op for viem itself, and no real wallet or library produces
the dynamic-`account` shape the other direction needs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@joeblau
joeblau merged commit cfeeddb into main Jul 28, 2026
4 checks passed
@joeblau
joeblau deleted the fix/local-account-routing-and-viem-import branch July 28, 2026 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant