devop: hw wallet typed message support#713
Merged
kvhnuke merged 1 commit intodevop/release-2-9from Jun 18, 2025
Merged
Conversation
## Walkthrough
This change introduces EIP-712 typed message signing support for Ethereum hardware wallets, particularly Ledger and Trezor. It adds a unified `TypedMessageSigner` abstraction, updates UI components to display hardware wallet messages, and extends hardware wallet provider interfaces and implementations to handle typed message signing, with explicit support or rejection for each wallet and network.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------|
| packages/extension/src/providers/ethereum/ui/eth-sign-typedata.vue | Refactored signing logic to use new `TypedMessageSigner`; added hardware wallet message UI component; cleaned imports. |
| packages/extension/src/providers/ethereum/ui/libs/signer.ts | Added exported `TypedMessageSigner` function for EIP-712 signing; updated exports. |
| packages/extension/src/providers/ethereum/ui/types.ts | Added `SignerTypedMessageOptions` interface for typed message signing options. |
| packages/extension/src/providers/ethereum/ui/eth-verify-transaction.vue | Changed displayed "To" address logic to prioritize token recipient address. |
| packages/hw-wallets/package.json | Added `@metamask/eth-sig-util` and `@trezor/connect-plugin-ethereum` dependencies. |
| packages/hw-wallets/src/index.ts | Added `signTypedMessage` method to `HWwalletManager`. |
| packages/hw-wallets/src/types.ts | Added `SignTypedMessageRequest` interface and abstract method to `HWWalletProvider`. |
| packages/hw-wallets/src/ledger/ethereum/index.ts | Implemented EIP-712 typed message signing in `LedgerEthereum`; added capability. |
| packages/hw-wallets/src/trezor/ethereum/index.ts | Added `signTypedMessage` method using `transformTypedData` and TrezorConnect's `ethereumSignTypedData`; added capability. |
| packages/hw-wallets/src/ledger/bitcoin/index.ts<br>packages/hw-wallets/src/ledger/solana/index.ts<br>packages/hw-wallets/src/ledger/substrate/index.ts<br>packages/hw-wallets/src/trezor/bitcoin/index.ts<br>packages/hw-wallets/src/trezor/solana/index.ts | Added stub `signTypedMessage` methods that reject with "not supported" errors for unsupported wallets/networks. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant UI as eth-sign-typedata.vue
participant Signer as TypedMessageSigner
participant HWManager as HWwalletManager
participant Ledger as LedgerEthereum
UI->>Signer: TypedMessageSigner(options)
alt Hardware wallet
Signer->>HWManager: signTypedMessage(request)
HWManager->>Ledger: signTypedMessage(request)
Ledger->>HWManager: signature / error
HWManager->>Signer: signature / error
Signer->>UI: signature / error
else Software wallet
Signer->>Internal: Compute hash and send for signing
Internal->>Signer: signature / error
Signer->>UI: signature / error
endSuggested reviewers
|
|
💼 Build Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
packages/hw-wallets/src/types.ts (1)
111-112: Consider default implementation in abstract classEvery provider now copy-pastes a rejection stub. Moving the default to the base class keeps subclasses minimal and avoids string-divergence bugs (see Ledger Substrate below).
-abstract class HWWalletProvider { +abstract class HWWalletProvider { + signTypedMessage(_req: SignTypedMessageRequest): Promise<string> { + return Promise.reject(new Error("signTypedMessage not supported")); + }Subclasses that actually support the feature (e.g. LedgerEthereum) simply override.
Less code, fewer typos.packages/hw-wallets/src/ledger/solana/index.ts (1)
85-89: Minor: preferasync+throwfor uniform styleElsewhere (
signPersonalMessage,signTransaction) the error path usesthrowinsideasyncfunctions. For consistency and stack-trace clarity:- signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { - return Promise.reject( - new Error("ledger-solana: signTypedMessage not supported"), - ); - } + async signTypedMessage(): Promise<string> { + throw new Error("ledger-solana: signTypedMessage not supported"); + }packages/hw-wallets/src/ledger/substrate/index.ts (1)
94-98: Style consistency – see LedgerSolana commentSame suggestion: convert to
async+throwfor uniformity across providers.packages/hw-wallets/src/ledger/bitcoin/index.ts (1)
209-213: Use shared rejection logic to avoid duplicationThis stub duplicates identical logic sprinkled across providers. If the base-class default recommended earlier is adopted, the whole method can be removed here.
- signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { - return Promise.reject( - new Error("ledger-bitcoin: signTypedMessage not supported"), - ); - } + // remove – inherited default already rejectspackages/hw-wallets/src/trezor/ethereum/index.ts (1)
133-137: MakesignTypedMessageasyncfor interface symmetryAll other async capabilities in this provider (
getAddress,signPersonalMessage,signTransaction) are declared withasync.
Although returningPromise.reject(...)is technically fine, marking the methodasyncand using a simplethrowkeeps the mental model consistent and avoids accidental refactors that treat it as a synchronous function.- signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { - return Promise.reject( - new Error("trezor-ethereum: signTypedMessage not supported"), - ); - } + async signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { + throw new Error("trezor-ethereum: signTypedMessage not supported"); + }packages/hw-wallets/src/trezor/solana/index.ts (1)
71-75: Align style with other async membersSame reasoning as in the Ethereum provider: mark as
async+throwfor consistent style.- signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { - return Promise.reject( - new Error("trezor-solana: signTypedMessage not supported"), - ); - } + async signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { + throw new Error("trezor-solana: signTypedMessage not supported"); + }packages/hw-wallets/src/trezor/bitcoin/index.ts (1)
119-123: Minor consistency tweakFor parity with the rest of the class & other providers, prefer
async+throw:- signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { - return Promise.reject( - new Error("trezor-bitcoin: signTypedMessage not supported"), - ); - } + async signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { + throw new Error("trezor-bitcoin: signTypedMessage not supported"); + }packages/hw-wallets/src/index.ts (1)
71-76: Optional DRY refactor opportunity
getAddress,signPersonalMessage,signTypedMessage, andsignTransactionshare a “resolve-provider + delegate” pattern.
A tiny private helper would remove repetition:#delegate<T extends keyof HWWalletProvider>( method: T, options: Parameters<HWWalletProvider[T]>[0] ): ReturnType<HWWalletProvider[T]> { ... }Not blocking, but worth considering to keep this dispatcher lean.
packages/extension/src/providers/ethereum/ui/eth-sign-typedata.vue (2)
105-117: Prevent double-sign clicks & lost rejections
approve()firesTypedMessageSigner()but does not disable the “Sign” button or await the promise.
If a user double-clicks, the function is executed twice and two concurrent background requests are produced, which may confuse both Ledger and the dApp.Add a
pendingflag that disables the right button until the promise settles, andawaitthe signer to surface any synchronous throw.
54-55: Component registration mismatch riskWith
<script setup>an imported component is automatically exposed under its import name (HardwareWalletMsg).
In templates Vue resolves bothHardwareWalletMsgandhardware-wallet-msg, but the kebab-case variant relies on the kebabisation heuristic.
Sticking to<HardwareWalletMsg/>avoids template-compiler edge-cases and improves grep-ability.packages/extension/src/providers/ethereum/ui/libs/signer.ts (2)
124-131: Surface V1-not-supported earlierThe hardware-wallet branch rejects V1 only after the call is made.
Fail fast by validatingversionbefore instantiatingHWwallet– saves an unnecessary USB round-trip.
156-169: ShadowedversionconstantInside the software-wallet branch you redeclare
const version = …which shadows the parameter captured above.
The code works, but the extra binding is unnecessary noise.-const version = options.version as SignTypedDataVersion; -const typedData = options.typedData; +const version = options.version as SignTypedDataVersion; +const { typedData } = options;packages/hw-wallets/src/ledger/ethereum/index.ts (1)
184-191: Advertise capability only when firmware is new enough
typedMessagesupport requires Ledger-Ethereum ≥ 1.10.0.
Blindly announcing the capability may mislead the UI on older firmware, causing silent failures.Consider detecting the app version in
init()and conditionally returning the capability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/extension/src/providers/ethereum/ui/eth-sign-typedata.vue(3 hunks)packages/extension/src/providers/ethereum/ui/libs/signer.ts(2 hunks)packages/extension/src/providers/ethereum/ui/types.ts(1 hunks)packages/hw-wallets/package.json(1 hunks)packages/hw-wallets/src/index.ts(2 hunks)packages/hw-wallets/src/ledger/bitcoin/index.ts(2 hunks)packages/hw-wallets/src/ledger/ethereum/index.ts(3 hunks)packages/hw-wallets/src/ledger/solana/index.ts(2 hunks)packages/hw-wallets/src/ledger/substrate/index.ts(2 hunks)packages/hw-wallets/src/trezor/bitcoin/index.ts(2 hunks)packages/hw-wallets/src/trezor/ethereum/index.ts(2 hunks)packages/hw-wallets/src/trezor/solana/index.ts(2 hunks)packages/hw-wallets/src/types.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
packages/hw-wallets/src/ledger/substrate/index.ts (1)
packages/hw-wallets/src/types.ts (1)
SignTypedMessageRequest(56-62)
packages/hw-wallets/src/trezor/ethereum/index.ts (1)
packages/hw-wallets/src/types.ts (1)
SignTypedMessageRequest(56-62)
packages/hw-wallets/src/ledger/bitcoin/index.ts (1)
packages/hw-wallets/src/types.ts (1)
SignTypedMessageRequest(56-62)
packages/hw-wallets/src/trezor/solana/index.ts (1)
packages/hw-wallets/src/types.ts (1)
SignTypedMessageRequest(56-62)
packages/hw-wallets/src/ledger/solana/index.ts (1)
packages/hw-wallets/src/types.ts (1)
SignTypedMessageRequest(56-62)
packages/hw-wallets/src/trezor/bitcoin/index.ts (1)
packages/hw-wallets/src/types.ts (1)
SignTypedMessageRequest(56-62)
packages/hw-wallets/src/index.ts (1)
packages/hw-wallets/src/types.ts (1)
SignTypedMessageRequest(56-62)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (1)
packages/hw-wallets/package.json (1)
60-62:Details
❓ Verification inconclusive
Pin indirect dependency to avoid accidental breaking changes
@metamask/eth-sig-utilfollows sem-ver but occasionally ships breaking changes in minor bumps. Declaring the range as^8.2.0will auto-upgrade to any 8.x release and may break the Ledger EIP-712 logic without notice.
Consider hard-pinning or using~8.2.0until a compatibility matrix is in place.- "@metamask/eth-sig-util": "^8.2.0", + "@metamask/eth-sig-util": "~8.2.0",
Action Required: Constrain @metamask/eth-sig-util to patch updates
Minor version bumps of
@metamask/eth-sig-utilhave historically shipped breaking changes that can disrupt Ledger’s EIP-712 handling. Until we have a compatibility matrix, it’s safer to pin to the 8.2.x patch range.• File:
packages/hw-wallets/package.json
• Lines: 60–62Suggested diff:
- "@metamask/eth-sig-util": "^8.2.0", + "@metamask/eth-sig-util": "~8.2.0",
Comment on lines
56
to
62
| export interface SignTypedMessageRequest extends BaseRequest { | ||
| domain: Record<string, unknown>; | ||
| message: Record<string, unknown>; | ||
| types: Record<string, any>; | ||
| primaryType: string; | ||
| version: "V3" | "V4"; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten types typing to the canonical EIP-712 shape
types: Record<string, any> loses every bit of compile-time safety and allows malformed payloads to slip through.
You already depend on @metamask/eth-sig-util, which exports TypedData helpers:
import type { MessageTypes, TypedMessage } from "@metamask/eth-sig-util";- types: Record<string, any>;
+ types: MessageTypes;This gives editors/full CI feedback and guarantees that every wallet receives a valid structure.
🤖 Prompt for AI Agents
In packages/hw-wallets/src/types.ts around lines 56 to 62, the `types` property
in the SignTypedMessageRequest interface is currently typed as `Record<string,
any>`, which lacks compile-time safety. To fix this, import the appropriate
types such as `MessageTypes` from `@metamask/eth-sig-util` and replace the
`types` property's type with `MessageTypes` to enforce the canonical EIP-712
structure and improve type safety.
Comment on lines
94
to
98
| signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { | ||
| return Promise.reject( | ||
| new Error("ledger-bitcoin: signTypedMessage not supported"), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Incorrect error prefix – copy/paste artifact
The rejection message says ledger-bitcoin instead of ledger-substrate, which will mislead support and telemetry.
- new Error("ledger-bitcoin: signTypedMessage not supported"),
+ new Error("ledger-substrate: signTypedMessage not supported"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { | |
| return Promise.reject( | |
| new Error("ledger-bitcoin: signTypedMessage not supported"), | |
| ); | |
| } | |
| signTypedMessage(_request: SignTypedMessageRequest): Promise<string> { | |
| return Promise.reject( | |
| new Error("ledger-substrate: signTypedMessage not supported"), | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/hw-wallets/src/ledger/substrate/index.ts around lines 94 to 98, the
error message in the rejected promise incorrectly uses the prefix
"ledger-bitcoin" instead of "ledger-substrate". Update the error message string
to use "ledger-substrate" to accurately reflect the source and avoid confusion
in support and telemetry.
Comment on lines
40
to
45
| export interface SignerTypedMessageOptions { | ||
| typedData: any; | ||
| version: 'V3' | 'V4'; | ||
| network: BaseNetwork; | ||
| account: EnkryptAccount; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten typedData typing to improve type-safety
any forfeits the main benefit of introducing a dedicated interface.
@metamask/eth-sig-util exports TypedMessage / TypedData helpers you can leverage:
-import interface SignerTypedMessageOptions {
- typedData: any;
+import { TypedMessage } from '@metamask/eth-sig-util';
+
+export interface SignerTypedMessageOptions {
+ /** EIP-712 typed data payload */
+ typedData: TypedMessage<any>;This provides compile-time guarantees and IntelliSense when constructing the payload.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/extension/src/providers/ethereum/ui/types.ts around lines 40 to 45,
the typedData property is currently typed as any, which reduces type safety.
Replace the any type with the appropriate TypedMessage or TypedData type
imported from @metamask/eth-sig-util to provide compile-time type checking and
better IntelliSense support. Ensure you import the type correctly and update the
interface definition accordingly.
Comment on lines
141
to
165
| signTypedMessage(request: SignTypedMessageRequest): Promise<string> { | ||
| const messageHash = TypedDataUtils.hashStruct( | ||
| request.primaryType, | ||
| request.message, | ||
| request.types, | ||
| request.version as SignTypedDataVersion.V3 | SignTypedDataVersion.V4, | ||
| ); | ||
| const domainHash = TypedDataUtils.hashStruct( | ||
| "EIP712Domain", | ||
| request.domain, | ||
| request.types, | ||
| request.version as SignTypedDataVersion.V3 | SignTypedDataVersion.V4, | ||
| ); | ||
| const connection = new EthApp(this.transport); | ||
| return connection | ||
| .signEIP712HashedMessage( | ||
| request.pathType.path.replace(`{index}`, request.pathIndex), | ||
| bufferToHex(domainHash, true), | ||
| bufferToHex(messageHash, true), | ||
| ) | ||
| .then((result) => { | ||
| const v = BigInt(result.v - 27); | ||
| return toRpcSig(v, hexToBuffer(result.r), hexToBuffer(result.s)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Guard unexpected Ledger v values
signEIP712HashedMessage normally returns v ∈ {27,28}.
If the app ever changes and returns a raw 0/1, v - 27 underflows to a huge unsigned bigint, producing an invalid RPC-sig.
-const v = BigInt(result.v - 27);
+const rawV = BigInt(result.v);
+if (rawV !== 27n && rawV !== 28n) {
+ throw new Error(`ledger-ethereum: unexpected v=${result.v}`);
+}
+const v = rawV - 27n;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| signTypedMessage(request: SignTypedMessageRequest): Promise<string> { | |
| const messageHash = TypedDataUtils.hashStruct( | |
| request.primaryType, | |
| request.message, | |
| request.types, | |
| request.version as SignTypedDataVersion.V3 | SignTypedDataVersion.V4, | |
| ); | |
| const domainHash = TypedDataUtils.hashStruct( | |
| "EIP712Domain", | |
| request.domain, | |
| request.types, | |
| request.version as SignTypedDataVersion.V3 | SignTypedDataVersion.V4, | |
| ); | |
| const connection = new EthApp(this.transport); | |
| return connection | |
| .signEIP712HashedMessage( | |
| request.pathType.path.replace(`{index}`, request.pathIndex), | |
| bufferToHex(domainHash, true), | |
| bufferToHex(messageHash, true), | |
| ) | |
| .then((result) => { | |
| const v = BigInt(result.v - 27); | |
| return toRpcSig(v, hexToBuffer(result.r), hexToBuffer(result.s)); | |
| }); | |
| } | |
| .then((result) => { | |
| - const v = BigInt(result.v - 27); | |
| + const rawV = BigInt(result.v); | |
| + if (rawV !== 27n && rawV !== 28n) { | |
| + throw new Error(`ledger-ethereum: unexpected v=${result.v}`); | |
| + } | |
| + const v = rawV - 27n; | |
| return toRpcSig(v, hexToBuffer(result.r), hexToBuffer(result.s)); | |
| }); |
🤖 Prompt for AI Agents
In packages/hw-wallets/src/ledger/ethereum/index.ts around lines 141 to 165, the
code assumes the Ledger's returned 'v' value is always 27 or 28, but if it
returns 0 or 1, subtracting 27 causes an underflow and invalid signature. To fix
this, add a guard to check if 'v' is 0 or 1 and if so, add 27 before converting
to BigInt; otherwise, proceed as usual. This ensures the 'v' value is correctly
normalized before creating the RPC signature.
SemajaM
approved these changes
Jun 18, 2025
Merged
Merged
Merged
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
2 participants
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.
Summary by CodeRabbit