Merge remote-tracking branch 'origin' into feat/ledger-use-hardware-wallet-error

Author: montelaidev

README.md

@@ -61,7 +61,9 @@ linkStyle default opacity:0.5
   eth_ledger_bridge_keyring --> keyring_api;
   eth_ledger_bridge_keyring --> keyring_utils;
   eth_ledger_bridge_keyring --> account_api;
+  eth_qr_keyring --> keyring_api;
   eth_qr_keyring --> keyring_utils;
+  eth_qr_keyring --> account_api;
   eth_simple_keyring --> keyring_api;
   eth_simple_keyring --> keyring_utils;
   eth_trezor_keyring --> keyring_api;

packages/keyring-api/CHANGELOG.md

@@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Change `KeyringWrapper.capabilities` from a readonly property to a getter ([#447](https://github.com/MetaMask/accounts/pull/447))
+  - Allows subclasses to override and return capabilities dynamically based on runtime state.
 - Refine `EthAddressStruct` in order to make it compatible with the `Hex` type from `@metamask/utils` ([#405](https://github.com/MetaMask/accounts/pull/405))
 
 ## [21.3.0]

packages/keyring-api/src/api/v2/wrapper/keyring-wrapper.ts

@@ -51,7 +51,7 @@ export abstract class KeyringWrapper<
 {
   readonly type: `${KeyringType}`;
 
-  readonly capabilities: KeyringCapabilities;
+  readonly #capabilities: KeyringCapabilities;
 
   protected readonly inner: InnerKeyring;
 
@@ -74,7 +74,19 @@ export abstract class KeyringWrapper<
   constructor(options: KeyringWrapperOptions<InnerKeyring>) {
     this.inner = options.inner;
     this.type = `${options.type}`;
-    this.capabilities = options.capabilities;
+    this.#capabilities = options.capabilities;
+  }
+
+  /**
+   * Get the capabilities of this keyring.
+   *
+   * Subclasses can override this getter to return capabilities dynamically
+   * based on runtime state.
+   *
+   * @returns The keyring's capabilities.
+   */
+  get capabilities(): KeyringCapabilities {
+    return this.#capabilities;
   }
 
   /**

packages/keyring-eth-ledger-bridge/CHANGELOG.md

@@ -10,6 +10,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 
 - Add `LedgerKeyringV2` class implementing `KeyringV2` interface ([#416](https://github.com/MetaMask/accounts/pull/416))
+
+### Fixed
+
+- Normalize signature `v` value from Ledger devices for proper recovery ([#449](https://github.com/MetaMask/accounts/pull/449))
   - Wraps legacy `LedgerKeyring` to expose accounts via the unified `KeyringV2` API and the `KeyringAccount` type.
   - Extends `EthKeyringWrapper` for common Ethereum logic.
 - Integrate `@metamask/hw-wallet-sdk` for standardized hardware wallet error handling ([#446](https://github.com/MetaMask/accounts/pull/446))

packages/keyring-eth-ledger-bridge/jest.config.js

@@ -26,9 +26,9 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 93.75,
-      functions: 98.26,
-      lines: 97.84,
+      branches: 93.78,
+      functions: 98.27,
+      lines: 97.83,
       statements: 97.85,
     },
   },

packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts

@@ -947,6 +947,82 @@ describe('LedgerKeyring', function () {
           keyring.signPersonalMessage(fakeAccounts[0], 'some message'),
         ).rejects.toThrow('Some other transport error');
       });
+
+      it('normalizes v=0 to v=27 for proper signature recovery', async function () {
+        await basicSetupToUnlockOneAccount();
+        jest
+          .spyOn(keyring.bridge, 'deviceSignMessage')
+          .mockResolvedValue({ v: 0, r: 'aabbccdd', s: '11223344' });
+
+        jest
+          .spyOn(sigUtil, 'recoverPersonalSignature')
+          .mockReturnValue(fakeAccounts[0]);
+
+        const result = await keyring.signPersonalMessage(
+          fakeAccounts[0],
+          'some message',
+        );
+
+        // v=0 should be normalized to v=27 (0x1b)
+        expect(result).toBe('0xaabbccdd112233441b');
+      });
+
+      it('normalizes v=1 to v=28 for proper signature recovery', async function () {
+        await basicSetupToUnlockOneAccount();
+        jest
+          .spyOn(keyring.bridge, 'deviceSignMessage')
+          .mockResolvedValue({ v: 1, r: 'aabbccdd', s: '11223344' });
+
+        jest
+          .spyOn(sigUtil, 'recoverPersonalSignature')
+          .mockReturnValue(fakeAccounts[0]);
+
+        const result = await keyring.signPersonalMessage(
+          fakeAccounts[0],
+          'some message',
+        );
+
+        // v=1 should be normalized to v=28 (0x1c)
+        expect(result).toBe('0xaabbccdd112233441c');
+      });
+
+      it('preserves v=27 unchanged', async function () {
+        await basicSetupToUnlockOneAccount();
+        jest
+          .spyOn(keyring.bridge, 'deviceSignMessage')
+          .mockResolvedValue({ v: 27, r: 'aabbccdd', s: '11223344' });
+
+        jest
+          .spyOn(sigUtil, 'recoverPersonalSignature')
+          .mockReturnValue(fakeAccounts[0]);
+
+        const result = await keyring.signPersonalMessage(
+          fakeAccounts[0],
+          'some message',
+        );
+
+        // v=27 should remain as 0x1b
+        expect(result).toBe('0xaabbccdd112233441b');
+      });
+
+      it('preserves v=28 unchanged', async function () {
+        await basicSetupToUnlockOneAccount();
+        jest
+          .spyOn(keyring.bridge, 'deviceSignMessage')
+          .mockResolvedValue({ v: 28, r: 'aabbccdd', s: '11223344' });
+
+        jest
+          .spyOn(sigUtil, 'recoverPersonalSignature')
+          .mockReturnValue(fakeAccounts[0]);
+
+        const result = await keyring.signPersonalMessage(
+          fakeAccounts[0],
+          'some message',
+        );
+
+        // v=28 should remain as 0x1c
+        expect(result).toBe('0xaabbccdd112233441c');
+      });
     });
 
     describe('signMessage', function () {
@@ -1279,7 +1355,9 @@ describe('LedgerKeyring', function () {
         ).rejects.toThrow('Ledger: Unknown error while signing message');
       });
 
-      it('returns signature when recoveryId length < 2', async function () {
+      it('normalizes v=0 to v=27 for proper signature recovery', async function () {
+        // Ledger may return v as 0 or 1 (modern format), but signature
+        // recovery expects 27 or 28 (legacy format)
         jest.spyOn(keyring.bridge, 'deviceSignTypedData').mockResolvedValue({
           v: 0,
           r: '72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b9',
@@ -1292,8 +1370,45 @@ describe('LedgerKeyring', function () {
             version: sigUtil.SignTypedDataVersion.V4,
           },
         );
+        // v=0 should be normalized to v=27 (0x1b)
+        expect(result).toBe(
+          '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e321b',
+        );
+      });
+
+      it('normalizes v=1 to v=28 for proper signature recovery', async function () {
+        jest.spyOn(keyring.bridge, 'deviceSignTypedData').mockResolvedValue({
+          v: 1,
+          r: '72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b9',
+          s: '46759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e32',
+        });
+
+        // v=1 should be normalized to v=28 (0x1c), but this will fail
+        // address recovery since the correct v for this signature is 27
+        await expect(
+          keyring.signTypedData(fakeAccounts[15], fixtureData, {
+            version: sigUtil.SignTypedDataVersion.V4,
+          }),
+        ).rejects.toThrow(
+          'Ledger: The signature doesnt match the right address',
+        );
+      });
+
+      it('preserves v=27 unchanged', async function () {
+        jest.spyOn(keyring.bridge, 'deviceSignTypedData').mockResolvedValue({
+          v: 27,
+          r: '72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b9',
+          s: '46759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e32',
+        });
+        const result = await keyring.signTypedData(
+          fakeAccounts[15],
+          fixtureData,
+          {
+            version: sigUtil.SignTypedDataVersion.V4,
+          },
+        );
         expect(result).toBe(
-          '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e3200',
+          '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e321b',
         );
       });
 
@@ -1421,6 +1536,22 @@ describe('LedgerKeyring', function () {
         expect(result).toStrictEqual(mockResponse);
         expect(result.arbitraryDataEnabled).toBe(0);
       });
+
+      it('handles TransportStatusError when getting app configuration', async function () {
+        const transportError = {
+          statusCode: 27013,
+          message: 'Ledger device: (denied by the user?) (0x6985)',
+          name: 'TransportStatusError',
+        };
+        Object.setPrototypeOf(transportError, TransportStatusError.prototype);
+        jest
+          .spyOn(keyring.bridge, 'getAppConfiguration')
+          .mockRejectedValue(transportError);
+
+        await expect(keyring.getAppConfiguration()).rejects.toThrow(
+          'User rejected action on device',
+        );
+      });
     });
 
     describe('destroy', function () {

packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts

@@ -346,7 +346,14 @@ export class LedgerKeyring implements Keyring {
   }
 
   async getAppConfiguration(): Promise<AppConfigurationResponse> {
-    return await this.bridge.getAppConfiguration();
+    try {
+      return await this.bridge.getAppConfiguration();
+    } catch (error: unknown) {
+      return handleLedgerTransportError(
+        error,
+        'Ledger: Unknown error while getting app configuration',
+      );
+    }
   }
 
   // tx is an instance of the ethereumjs-transaction class.
@@ -478,10 +485,9 @@ export class LedgerKeyring implements Keyring {
       );
     }
 
-    let modifiedV = parseInt(String(payload.v), 10).toString(16);
-    if (modifiedV.length < 2) {
-      modifiedV = `0${modifiedV}`;
-    }
+    const modifiedV = this.#normalizeRecoveryParam(
+      parseInt(String(payload.v), 10),
+    );
 
     const signature = `0x${payload.r}${payload.s}${modifiedV}`;
     const addressSignedWith = recoverPersonalSignature({
@@ -571,10 +577,9 @@ export class LedgerKeyring implements Keyring {
       );
     }
 
-    let recoveryId = parseInt(String(payload.v), 10).toString(16);
-    if (recoveryId.length < 2) {
-      recoveryId = `0${recoveryId}`;
-    }
+    const recoveryId = this.#normalizeRecoveryParam(
+      parseInt(String(payload.v), 10),
+    );
     const signature = `0x${payload.r}${payload.s}${recoveryId}`;
     const addressSignedWith = recoverTypedSignature({
       data,
@@ -717,4 +722,19 @@ export class LedgerKeyring implements Keyring {
   #getChecksumHexAddress(address: string): Hex {
     return getChecksumAddress(add0x(address));
   }
+
+  /**
+   * Normalizes the signature recovery parameter (v) to legacy format.
+   * Ledger devices may return v as 0 or 1 (modern format), but signature
+   * recovery expects 27 or 28 (legacy format per EIP-191/EIP-712).
+   *
+   * @param recoveryParam - The recovery parameter from Ledger.
+   * @returns The normalized recovery parameter as a hex string.
+   */
+  #normalizeRecoveryParam(recoveryParam: number): string {
+    if (recoveryParam === 0 || recoveryParam === 1) {
+      return (recoveryParam + 27).toString(16);
+    }
+    return recoveryParam.toString(16);
+  }
 }

packages/keyring-eth-qr/CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+- Add `QrKeyringV2` class implementing `KeyringV2` interface ([#411](https://github.com/MetaMask/accounts/pull/411)), ([#447](https://github.com/MetaMask/accounts/pull/447))
+  - Wraps legacy `QrKeyring` to expose accounts via the unified `KeyringV2` API and the `KeyringAccount` type.
+  - Extends `EthKeyringWrapper` for common Ethereum logic.
+
 ## [1.1.0]
 
 ### Added

packages/keyring-eth-qr/package.json

@@ -53,6 +53,7 @@
     "@ethereumjs/util": "^9.1.0",
     "@keystonehq/bc-ur-registry-eth": "^0.19.1",
     "@metamask/eth-sig-util": "^8.2.0",
+    "@metamask/keyring-api": "workspace:^",
     "@metamask/keyring-utils": "workspace:^",
     "@metamask/utils": "^11.1.0",
     "async-mutex": "^0.5.0",
@@ -63,6 +64,7 @@
     "@ethereumjs/common": "^4.4.0",
     "@keystonehq/metamask-airgapped-keyring": "^0.15.2",
     "@lavamoat/allow-scripts": "^3.2.1",
+    "@metamask/account-api": "workspace:^",
     "@metamask/auto-changelog": "^3.4.4",
     "@types/hdkey": "^2.0.1",
     "@types/jest": "^29.5.12",

packages/keyring-eth-qr/src/index.ts

@@ -17,3 +17,8 @@ export {
   type SerializedQrKeyringState,
   type SerializedUR,
 } from './qr-keyring';
+export {
+  QrKeyringV2,
+  type QrKeyringV2Options,
+  type QrAccountModeCreateOptions,
+} from './qr-keyring-v2';