chore: ledger-v backport

.changeset/twenty-humans-beg.md

@@ -0,0 +1,6 @@
+---
+'@celo/wallet-ledger': patch
+'@celo/viem-account-ledger': patch
+---
+
+Safer handling of v from device

.prettierignore

@@ -7,6 +7,7 @@ package.json
 
 packages/**/dist
 packages/**/lib
+packages/**/lib-es
 
 # Needed because we have packages/celotool/src/lib
 !packages/celotool/src/**

packages/sdk/wallets/wallet-ledger/src/ledger-signer.ts

@@ -49,16 +49,33 @@ export class LedgerSigner implements Signer {
     try {
       const validatedDerivationPath = await this.getValidatedDerivationPath()
       await this.checkForKnownToken(encodedTx)
-      const signature = await this.ledger!.signTransaction(
+      let {
+        r,
+        s,
+        v: _v,
+      } = await this.ledger!.signTransaction(
         validatedDerivationPath,
         trimLeading0x(encodedTx.rlpEncode), // the ledger requires the rlpEncode without the leading 0x
         null
       )
 
+      if (typeof _v === 'string' && (_v === '' || _v === '0x')) {
+        console.warn(
+          `ledger-signer@signTransaction: signature \`v\` was malformed \`${_v}\`. Replaced with "0x0"`
+        )
+        _v = '0x0'
+      }
+      const v = typeof _v === 'string' ? parseInt(ensureLeading0x(_v), 16) : _v
+      if (isNaN(v)) {
+        throw new Error(
+          `ledger-signer@signTransaction: signature \`v\` was malformed and was parsed to NaN \`${_v}\``
+        )
+      }
+
       return {
-        v: parseInt(signature.v, 16),
-        r: ethUtil.toBuffer(ensureLeading0x(signature.r)),
-        s: ethUtil.toBuffer(ensureLeading0x(signature.s)),
+        v,
+        r: ethUtil.toBuffer(ensureLeading0x(r)),
+        s: ethUtil.toBuffer(ensureLeading0x(s)),
       }
     } catch (error: unknown) {
       if (error instanceof TransportStatusError) {

packages/viem-account-ledger/.gitignore

@@ -1,4 +1,5 @@
 lib/
+lib-es/
 tmp/
 .tmp/
-.env
+.env

packages/viem-account-ledger/package.json

@@ -3,10 +3,9 @@
   "version": "1.0.0-beta.1",
   "description": "Helper library to make ledger<->viem interactions easier",
   "type": "module",
-  "exports": {
-    ".": "./lib/index.js"
-  },
-  "types": "./lib/index.d.ts",
+  "main": "lib/index.js",
+  "module": "lib-es/index.js",
+  "types": "lib/index.d.ts",
   "author": "cLabs",
   "license": "Apache-2.0",
   "homepage": "https://docs.celo.org/developer/tools",
@@ -18,12 +17,14 @@
     "ledger"
   ],
   "scripts": {
-    "build": "yarn run --top-level tsc -b .",
-    "clean": "yarn run --top-level tsc -b . --clean",
-    "docs": "yarn run --top-level typedoc",
+    "build": "yarn build:cjs && yarn build:esm",
+    "build:cjs": "yarn --top-level run tsc -m commonjs",
+    "build:esm": "yarn --top-level run tsc -m ES6 --outDir lib-es",
+    "clean": "yarn --top-level run tsc -b . --clean && yarn run rimraf lib lib-es",
+    "docs": "yarn --top-level run typedoc",
     "test": "yarn run vitest",
-    "lint": "yarn run --top-level eslint -c .eslintrc.cjs ",
-    "prepublishOnly": "yarn build"
+    "lint": "yarn --top-level run eslint -c .eslintrc.cjs ",
+    "prepublishOnly": "yarn clean && yarn build"
   },
   "peerDependencies": {
     "@ledgerhq/hw-transport-node-hid": "^6.x",
@@ -45,6 +46,7 @@
     "@ledgerhq/hw-transport-node-hid": "^6.29.5",
     "@vitest/coverage-v8": "2.1.2",
     "dotenv": "^8.2.0",
+    "rimraf": "^4.4.1",
     "viem": "^2.21.14",
     "vitest": "^2.1.2"
   },

packages/viem-account-ledger/src/ledger-to-account.test.ts

@@ -1,30 +1,40 @@
+import { recoverTransaction } from '@celo/wallet-base'
 import TransportNodeHid from '@ledgerhq/hw-transport-node-hid'
-import { describe, expect, it, test, vi } from 'vitest'
+import { beforeAll, describe, expect, it, test, vi } from 'vitest'
 import { ledgerToAccount } from './ledger-to-account.js'
-import { mockLedger, TEST_CHAIN_ID } from './test-utils.js'
+import { mockLedger, TEST_CHAIN_ID, test_ledger } from './test-utils.js'
+import { generateLedger } from './utils.js'
+
+const USE_PHYSICAL_LEDGER = process.env.USE_PHYSICAL_LEDGER === 'true'
+const hardwareDescribe = USE_PHYSICAL_LEDGER ? describe : describe.skip
+const syntheticDescribe = USE_PHYSICAL_LEDGER ? describe.skip : describe
+
+const transport = USE_PHYSICAL_LEDGER
+  ? TransportNodeHid.open('')
+  : Promise.resolve(undefined as unknown as TransportNodeHid)
 
 vi.mock('./utils.js', async () => {
   const module = await vi.importActual('./utils.js')
 
   return {
     ...module,
-    generateLedger: vi.fn(() => Promise.resolve(mockLedger())),
+    generateLedger: vi.fn((...args) =>
+      // @ts-expect-error
+      USE_PHYSICAL_LEDGER ? module.generateLedger(...args) : Promise.resolve(mockLedger())
+    ),
   }
 })
 
-const transport =
-  process.env.USE_PHYSICAL_LEDGER === 'true'
-    ? TransportNodeHid.open('')
-    : Promise.resolve(undefined as unknown as TransportNodeHid)
+syntheticDescribe('ledgerToAccount (mocked ledger)', () => {
+  let account: Awaited<ReturnType<typeof ledgerToAccount>>
+  beforeAll(async () => {
+    account = await ledgerToAccount({
+      transport: await transport,
+    })
+  })
 
-describe('ledgerToAccount', () => {
   it('can be setup', async () => {
-    await expect(
-      ledgerToAccount({
-        transport: await transport,
-      })
-    ).resolves.not.toBe(undefined)
-    // expect((generateLedger as ReturnType<(typeof jest)['fn']>).mock.calls.length).toBe(1)
+    expect((generateLedger as ReturnType<(typeof jest)['fn']>).mock.calls.length).toBe(1)
   })
 
   describe('signs txs', () => {
@@ -37,46 +47,189 @@ describe('ledgerToAccount', () => {
       maxPriorityFeePerGas: BigInt(100),
     } as const
 
-    test('eip1559', async () => {
-      const account = await ledgerToAccount({
-        transport: await transport,
+    describe('eip1559', () => {
+      test('v=0', async () => {
+        const txHash = await account.signTransaction(txData)
+        expect(txHash).toMatchInlineSnapshot(
+          `"0x02f86282aef32a6464809412345678901234567890123456789012345678907b80c080a05e130d8edb38e3ee8ab283af7c03a2579598b9a77807d7d796060358787d4707a07219dd22fe3bf3fe57682041d8f80dc9909cd70d903163b077d19625c4cd6e67"`
+        )
+        const [decoded, signer] = recoverTransaction(txHash)
+        expect(signer.toLowerCase()).toBe(account.address.toLowerCase())
+        // @ts-expect-error
+        expect(decoded.yParity).toMatchInlineSnapshot(`0`)
+      })
+      test('v=1', async () => {
+        const txHash = await account.signTransaction({ ...txData, nonce: 100 })
+        expect(txHash).toMatchInlineSnapshot(
+          `"0x02f86282aef3646464809412345678901234567890123456789012345678907b80c001a05d166032c75a416c4e552223b9288a7a280d47909d7f526c2884d21d05a28747a047b32b31eb8a9f035b73218ab2b8b8f3211713fc44ef9b9965e268b6ae064cfc"`
+        )
+        const [decoded, signer] = recoverTransaction(txHash)
+        expect(signer.toLowerCase()).toBe(account.address.toLowerCase())
+        // @ts-expect-error
+        expect(decoded.yParity).toMatchInlineSnapshot(`1`)
       })
-      await expect(account.signTransaction(txData)).resolves.toMatchInlineSnapshot(
-        `"0x02f86282aef32a6464809412345678901234567890123456789012345678907b80c080a05e130d8edb38e3ee8ab283af7c03a2579598b9a77807d7d796060358787d4707a07219dd22fe3bf3fe57682041d8f80dc9909cd70d903163b077d19625c4cd6e67"`
-      )
     })
 
-    test('cip64', async () => {
-      const account = await ledgerToAccount({
-        transport: await transport,
+    describe('cip64', () => {
+      test('v=0', async () => {
+        const account = await ledgerToAccount({
+          transport: await transport,
+        })
+        const cUSDa = '0x874069fa1eb16d44d622f2e0ca25eea172369bc1'
+        const txHash = await account.signTransaction({ ...txData, feeCurrency: cUSDa })
+        expect(txHash).toMatchInlineSnapshot(
+          `"0x7bf87782aef32a6464809412345678901234567890123456789012345678907b80c094874069fa1eb16d44d622f2e0ca25eea172369bc180a017d8df83b40dc645b60142280613467ca92438ff5aa0811a6ceff399fe66d661a02efe4eea14146f41d4f776bec1ededc486ddee37cea8304d297a69dbf27c4089"`
+        )
+        const [decoded, signer] = recoverTransaction(txHash)
+        expect(signer.toLowerCase()).toBe(account.address.toLowerCase())
+        // @ts-expect-error
+        expect(decoded.yParity).toMatchInlineSnapshot(`0`)
       })
-      const cUSDa = '0x874069fa1eb16d44d622f2e0ca25eea172369bc1'
-      await expect(
-        account.signTransaction({
-          ...txData,
-
-          feeCurrency: cUSDa,
+      test('v=1', async () => {
+        const account = await ledgerToAccount({
+          transport: await transport,
         })
-      ).resolves.toMatchInlineSnapshot(
-        `"0x7bf87782aef32a6464809412345678901234567890123456789012345678907b80c094874069fa1eb16d44d622f2e0ca25eea172369bc180a017d8df83b40dc645b60142280613467ca92438ff5aa0811a6ceff399fe66d661a02efe4eea14146f41d4f776bec1ededc486ddee37cea8304d297a69dbf27c4089"`
-      )
+        const cUSDa = '0x874069fa1eb16d44d622f2e0ca25eea172369bc1'
+        const txHash = await account.signTransaction({ ...txData, feeCurrency: cUSDa, nonce: 100 })
+        expect(txHash).toMatchInlineSnapshot(
+          `"0x7bf87782aef3646464809412345678901234567890123456789012345678907b80c094874069fa1eb16d44d622f2e0ca25eea172369bc101a02425b4eed4b98f3e0b206ca0bc6d6eb7d144ab6a676dc46bb02a243f3b810b84a00364b83eebbb23cbc9a76406842166e0d78086a820388adb1e249f9ed9753474"`
+        )
+        const [decoded, signer] = recoverTransaction(txHash)
+        expect(signer.toLowerCase()).toBe(account.address.toLowerCase())
+        // @ts-expect-error
+        expect(decoded.yParity).toMatchInlineSnapshot(`1`)
+      })
+    })
+
+    describe('malformed v values', () => {
+      test('empty string', async () => {
+        const test_vs_0_and_1 = [
+          [0, '', '00', '0x', '0x0', '0x00', '0x1b', 27], // yParity 0
+          [1, '1', '0x1', '0x01', '01', '0x1c', 28], // vParity 1
+        ]
+        for (const expectedyParity in test_vs_0_and_1) {
+          const test_vs = test_vs_0_and_1[+expectedyParity]
+          for (const v of test_vs) {
+            vi.spyOn(test_ledger, 'signTransaction').mockImplementationOnce(() =>
+              // @ts-expect-error
+              Promise.resolve({
+                v,
+                r: '0x1',
+                s: '0x1',
+              })
+            )
+            const txHash = await account.signTransaction(txData)
+            const [recovered] = recoverTransaction(txHash)
+            // @ts-expect-error
+            expect(recovered.yParity).toBe(+expectedyParity)
+          }
+        }
+      })
     })
   })
 
   it('signs messages', async () => {
-    const account = await ledgerToAccount({
-      transport: await transport,
-    })
     await expect(account.signMessage({ message: 'Hello World' })).resolves.toMatchInlineSnapshot(
       `"0x2f9a547e69592e98114263c08c6f7a6e6cd2f991fc29f442947179419233fe9641c8e4c86975a2722b54313e47768d2ffe2608c497ff9fe7f8c61b12e6257e571c"`
     )
   })
 
   it('signs typed data', async () => {
-    const account = await ledgerToAccount({
+    await expect(
+      account.signTypedData({
+        domain: {
+          name: 'foo',
+          version: '0.0.0',
+          chainId: BigInt(42),
+          verifyingContract: '0x123',
+        },
+        primaryType: 'EIP712Domain',
+        types: {
+          EIP712Domain: [
+            { name: 'name', type: 'string' },
+            { name: 'version', type: 'string' },
+            { name: 'chainId', type: 'uint256' },
+            { name: 'verifyingContract', type: 'address' },
+          ],
+        },
+      })
+    ).rejects.toMatchInlineSnapshot(`[Error: Not implemented as of this release.]`)
+  })
+})
+
+hardwareDescribe('ledgerToAccount (device ledger)', () => {
+  let account: Awaited<ReturnType<typeof ledgerToAccount>>
+  beforeAll(async () => {
+    account = await ledgerToAccount({
       transport: await transport,
     })
+  })
+
+  it('can be setup', async () => {
+    expect((generateLedger as ReturnType<(typeof jest)['fn']>).mock.calls.length).toBe(1)
+  })
+
+  describe('signs txs', () => {
+    const txData = {
+      to: '0x1234567890123456789012345678901234567890',
+      value: BigInt(123),
+      chainId: TEST_CHAIN_ID,
+      nonce: 42,
+      maxFeePerGas: BigInt(100),
+      maxPriorityFeePerGas: BigInt(100),
+    } as const
+
+    describe('eip1559', async () => {
+      test('v=0', async () => {
+        const txHash = await account.signTransaction(txData)
+        const [decoded, signer] = recoverTransaction(txHash)
+        expect(signer.toLowerCase()).toBe(account.address.toLowerCase())
+        // @ts-expect-error
+        expect(decoded.yParity).toMatchInlineSnapshot(`1`)
+      }, 20_000)
+      test('v=1', async () => {
+        const txHash = await account.signTransaction({ ...txData, nonce: 100 })
+        const [decoded, signer] = recoverTransaction(txHash)
+        expect(signer.toLowerCase()).toBe(account.address.toLowerCase())
+        // @ts-expect-error
+        expect(decoded.yParity).toMatchInlineSnapshot(`1`)
+      }, 20_000)
+    })
+
+    describe('cip64', async () => {
+      test('v=0', async () => {
+        const account = await ledgerToAccount({
+          transport: await transport,
+        })
+        const cUSDa = '0x874069fa1eb16d44d622f2e0ca25eea172369bc1'
+        const txHash = await account.signTransaction({ ...txData, feeCurrency: cUSDa, nonce: 0 })
+        const [decoded, signer] = recoverTransaction(txHash)
+        expect(signer.toLowerCase()).toBe(account.address.toLowerCase())
+        // @ts-expect-error
+        expect(decoded.yParity).toMatchInlineSnapshot(`0`)
+      }, 20_000)
+      test('v=1', async () => {
+        const account = await ledgerToAccount({
+          transport: await transport,
+        })
+        const cUSDa = '0x874069fa1eb16d44d622f2e0ca25eea172369bc1'
+        const txHash = await account.signTransaction({ ...txData, feeCurrency: cUSDa, nonce: 100 })
+        const [decoded, signer] = recoverTransaction(txHash)
+        expect(signer.toLowerCase()).toBe(account.address.toLowerCase())
+        // @ts-expect-error
+        expect(decoded.yParity).toMatchInlineSnapshot(`1`)
+      }, 20_000)
+    })
+  })
+
+  it('signs messages', async () => {
+    // TODO: refactor to check signer rather than snapshot to be device-agnostic
+    await expect(account.signMessage({ message: 'Hello World' })).resolves.toMatchInlineSnapshot(
+      `"0x15859cf38f7b58d98e330b1d105add503a31c88f02104d43b4f9cc7cb08dbb483e51abc79881d0f8562073ad57dc60a4a567ddedc1176bf94d4dccc69fce09c51c"`
+    )
+  }, 20_000)
 
+  it('signs typed data', async () => {
     await expect(
       account.signTypedData({
         domain: {