fix: review

packages/keyring-eth-onekey/CHANGELOG.md

@@ -9,7 +9,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
-- Bump axios
-- Init Project
+- Add initial implementation of the OneKey keyring ([#353](https://github.com/MetaMask/accounts/pull/353))
 
 [Unreleased]: https://github.com/MetaMask/accounts/

packages/keyring-eth-onekey/jest.config.js

@@ -23,10 +23,10 @@ module.exports = merge(baseConfig, {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 70.81,
-      functions: 87.95,
-      lines: 92.22,
-      statements: 92.28,
+      branches: 68.48,
+      functions: 94.36,
+      lines: 93.41,
+      statements: 93.46,
     },
   },
 });

packages/keyring-eth-onekey/package.json

@@ -1,7 +1,7 @@
 {
   "name": "@metamask/eth-onekey-keyring",
   "version": "0.1.0",
-  "description": "A MetaMask compatible keyring, for onekey hardware wallets",
+  "description": "A MetaMask compatible keyring, for OneKey hardware wallets",
   "keywords": [
     "ethereum",
     "keyring",
@@ -52,10 +52,10 @@
     "@metamask/eth-sig-util": "^8.2.0",
     "@metamask/utils": "^11.1.0",
     "@noble/hashes": "^1.7.0",
-    "@onekeyfe/hd-core": "1.1.6-patch.4",
-    "@onekeyfe/hd-shared": "1.1.6-patch.4",
-    "@onekeyfe/hd-transport": "1.1.6-patch.4",
-    "@onekeyfe/hd-web-sdk": "1.1.6-patch.4",
+    "@onekeyfe/hd-core": "1.1.17-patch.1",
+    "@onekeyfe/hd-shared": "1.1.17-patch.1",
+    "@onekeyfe/hd-transport": "1.1.17-patch.1",
+    "@onekeyfe/hd-web-sdk": "1.1.17-patch.1",
     "hdkey": "^2.1.0"
   },
   "devDependencies": {

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

@@ -1,4 +1,3 @@
 export * from './onekey-keyring';
 export type * from './onekey-bridge';
 export * from './onekey-web-bridge';
-export * from './constants';

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

@@ -24,13 +24,17 @@ type Success<T> = {
 };
 type Response<T> = Promise<Success<T> | Unsuccessful>;
 
+/**
+ * Hardware UI event payload
+ */
+export type HardwareUIEvent = {
+  error: string;
+  code?: string | number;
+};
+
 export type OneKeyBridge = {
   model?: string;
 
-  on(event: string, callback: (event: any) => void): void;
-
-  off(event: string): void;
-
   init(): Promise<void>;
 
   dispose(): Promise<void>;

packages/keyring-eth-onekey/src/onekey-keyring.test.ts

@@ -7,7 +7,7 @@ import {
 import { Address } from '@ethereumjs/util';
 import { SignTypedDataVersion } from '@metamask/eth-sig-util';
 // eslint-disable-next-line @typescript-eslint/naming-convention
-import EthereumTx from 'ethereumjs-tx';
+import type EthereumTx from 'ethereumjs-tx';
 // eslint-disable-next-line @typescript-eslint/naming-convention
 import HDKey from 'hdkey';
 import * as sinon from 'sinon';
@@ -17,6 +17,26 @@ import type { AccountDetails } from './onekey-keyring';
 import { OneKeyKeyring } from './onekey-keyring';
 import { OneKeyWebBridge } from './onekey-web-bridge';
 
+// Mock @onekeyfe/hd-web-sdk to avoid browser environment dependencies
+jest.mock('@onekeyfe/hd-web-sdk', () => ({
+  PascalCase: true,
+  default: {
+    HardwareWebSdk: {
+      init: jest.fn(),
+      on: jest.fn(),
+      uiResponse: jest.fn(),
+      dispose: jest.fn(),
+      switchTransport: jest.fn(),
+      evmGetPublicKey: jest.fn(),
+      getPassphraseState: jest.fn(),
+      evmSignTransaction: jest.fn(),
+      evmSignMessage: jest.fn(),
+      evmSignTypedData: jest.fn(),
+    },
+    HardwareSDKLowLevel: {},
+  },
+}));
+
 const fakeAccounts = [
   '0x73d0385F4d8E00C5e6504C6030F47BF6212736A8',
   '0xFA01a39f8Abaeb660c3137f14A310d0b414b2A15',
@@ -38,16 +58,6 @@ const fakeAccounts = [
 const fakeXPubKey =
   'xpub6CNFa58kEQJu2hwMVoofpDEKVVSg6gfwqBqE2zHAianaUnQkrJzJJ42iLDp7Dmg2aP88qCKoFZ4jidk3tECdQuF4567NGHDfe7iBRwHxgke';
 const fakeHdKey = HDKey.fromExtendedKey(fakeXPubKey);
-const fakeTx = new EthereumTx({
-  nonce: '0x00',
-  gasPrice: '0x09184e72a000',
-  gasLimit: '0x2710',
-  to: '0x0000000000000000000000000000000000000000',
-  value: '0x00',
-  data: '0x7f7465737432000000000000000000000000000000000000000000000000000000600057',
-  // EIP 155 chainId - mainnet: 1, ropsten: 3
-  chainId: 1,
-});
 
 const common = new Common({ chain: 'mainnet' });
 const commonEIP1559 = new Common({
@@ -411,33 +421,6 @@ describe('OneKeyKeyring', function () {
   });
 
   describe('signTransaction', function () {
-    it('should pass serialized transaction to onekey and return signed tx', async function () {
-      const ethereumSignTransactionStub = sinon.stub().resolves({
-        success: true,
-        payload: { v: '0x1', r: '0x0', s: '0x0' },
-      });
-      bridge.ethereumSignTransaction = ethereumSignTransactionStub;
-
-      sinon.stub(fakeTx, 'verifySignature').callsFake(() => true);
-      sinon
-        .stub(fakeTx, 'getSenderAddress')
-        .callsFake(() =>
-          Buffer.from(Address.fromString(fakeAccounts[0]).bytes),
-        );
-
-      // @ts-expect-error: intentionally using an old library that doesn't comply with TypedTransaction
-      const returnedTx = await keyring.signTransaction(fakeAccounts[0], fakeTx);
-      // assert that the v,r,s values got assigned to tx.
-      expect(returnedTx.v).toBeDefined();
-      expect(returnedTx.r).toBeDefined();
-      expect(returnedTx.s).toBeDefined();
-      // ensure we get a older version transaction back
-      // @ts-expect-error: intentionally using an old library that doesn't comply with TypedTransaction
-      expect((returnedTx as EthereumTx).getChainId()).toBe(1);
-      expect(returnedTx.common).toBeUndefined();
-      expect(ethereumSignTransactionStub.calledOnce).toBe(true);
-    });
-
     it('should pass serialized newer transaction to onekey and return signed tx', async function () {
       sinon.stub(TransactionFactory, 'fromTxData').callsFake(() => {
         // without having a private key/public key pair in this test, we have
@@ -740,24 +723,6 @@ describe('OneKeyKeyring', function () {
   });
 
   describe('error handling and edge cases', function () {
-    it('should handle signing errors', async function () {
-      await keyring.addAccounts(1);
-      const errorResponse = {
-        success: false,
-        payload: { error: 'Signing failed' },
-      };
-      bridge.ethereumSignTransaction = sinon.stub().resolves(errorResponse);
-
-      try {
-        // @ts-expect-error: intentionally using an old library that doesn't comply with TypedTransaction
-        await keyring.signTransaction(fakeAccounts[0], fakeTx);
-        throw new Error('Expected error was not thrown');
-      } catch (error) {
-        // eslint-disable-next-line jest/no-conditional-expect
-        expect((error as Error).message).toContain('Signing failed');
-      }
-    });
-
     it('should handle message signing errors', async function () {
       await keyring.addAccounts(1);
       const errorResponse = {
@@ -804,6 +769,82 @@ describe('OneKeyKeyring', function () {
       expect(accounts).toHaveLength(keyring.perPage);
       expect(keyring.page).toBe(1); // When page <= 0, it gets set to 1
     });
+
+    it('should handle unlock getPublicKey failure', async function () {
+      const getPassphraseStateStub = sinon.stub().resolves({
+        success: true,
+        payload: '',
+      });
+      const getPublicKeyStub = sinon.stub().resolves({
+        success: false,
+        payload: { error: 'Failed to get public key' },
+      });
+      bridge.getPassphraseState = getPassphraseStateStub;
+      bridge.getPublicKey = getPublicKeyStub;
+
+      keyring.hdk = new HDKey();
+
+      try {
+        await keyring.unlock();
+        throw new Error('Expected error was not thrown');
+      } catch (error) {
+        expect((error as Error).message).toContain('getPublicKey failed');
+      }
+    });
+
+    it('should handle unlock getPassphraseState failure', async function () {
+      const getPassphraseStateStub = sinon.stub().resolves({
+        success: false,
+        payload: { error: 'Failed to get passphrase state' },
+      });
+      bridge.getPassphraseState = getPassphraseStateStub;
+
+      keyring.hdk = new HDKey();
+
+      try {
+        await keyring.unlock();
+        throw new Error('Expected error was not thrown');
+      } catch (error) {
+        expect((error as Error).message).toContain(
+          'Failed to get passphrase state',
+        );
+      }
+    });
+
+    it('should handle unlock getPublicKey exception', async function () {
+      const getPassphraseStateStub = sinon.stub().resolves({
+        success: true,
+        payload: '',
+      });
+      const getPublicKeyStub = sinon.stub().rejects(new Error('Network error'));
+      bridge.getPassphraseState = getPassphraseStateStub;
+      bridge.getPublicKey = getPublicKeyStub;
+
+      keyring.hdk = new HDKey();
+
+      try {
+        await keyring.unlock();
+        throw new Error('Expected error was not thrown');
+      } catch (error) {
+        expect((error as Error).message).toContain('Network error');
+      }
+    });
+
+    it('should handle unlock getPassphraseState exception', async function () {
+      const getPassphraseStateStub = sinon
+        .stub()
+        .rejects(new Error('Connection error'));
+      bridge.getPassphraseState = getPassphraseStateStub;
+
+      keyring.hdk = new HDKey();
+
+      try {
+        await keyring.unlock();
+        throw new Error('Expected error was not thrown');
+      } catch (error) {
+        expect((error as Error).message).toContain('Connection error');
+      }
+    });
   });
 
   describe('HD path validation edge cases', function () {
@@ -866,42 +907,6 @@ describe('OneKeyKeyring', function () {
   });
 
   describe('transaction serialization edge cases', function () {
-    it('should handle transaction with empty "to" field (contract deployment)', async function () {
-      await keyring.addAccounts(1);
-
-      const successResponse = {
-        success: true,
-        payload: { v: '0x1', r: '0x0', s: '0x0' },
-      };
-      const ethereumSignTransactionStub = sinon
-        .stub()
-        .resolves(successResponse);
-      bridge.ethereumSignTransaction = ethereumSignTransactionStub;
-
-      sinon.stub(fakeTx, 'verifySignature').callsFake(() => true);
-      sinon
-        .stub(fakeTx, 'getSenderAddress')
-        .callsFake(() =>
-          Buffer.from(Address.fromString(fakeAccounts[0]).bytes),
-        );
-
-      // Simulate a contract deployment transaction by setting to to null
-      const originalTo = fakeTx.to;
-      // @ts-expect-error - for testing purposes
-      fakeTx.to = null;
-
-      // @ts-expect-error: intentionally using an old library that doesn't comply with TypedTransaction
-      const result = await keyring.signTransaction(fakeAccounts[0], fakeTx);
-      expect(result).toBeDefined();
-
-      const call = ethereumSignTransactionStub.getCall(0);
-      expect(call.args[0].transaction.to).toBe('0x');
-
-      // Restore original to value
-      // eslint-disable-next-line require-atomic-updates
-      fakeTx.to = originalTo;
-    });
-
     it('should test hex prefix utilities', async function () {
       // Test the serialize method
       const serialized = await keyring.serialize();
@@ -951,12 +956,6 @@ describe('OneKeyKeyring', function () {
       expect(Object.keys(keyring.accountDetails)).toHaveLength(0);
     });
 
-    it('should handle exportAccount error', function () {
-      expect(() => {
-        keyring.exportAccount();
-      }).toThrow('Not supported on this device');
-    });
-
     it('should handle isUnlocked when not unlocked', function () {
       keyring.hdk = new HDKey();
       expect(keyring.isUnlocked()).toBe(false);
@@ -1016,30 +1015,6 @@ describe('OneKeyKeyring', function () {
       ).toBe(true);
     });
 
-    it('should handle transaction signing with address verification', async function () {
-      await keyring.addAccounts(1);
-      const successResponse = {
-        success: true,
-        payload: { v: '0x1', r: '0x0', s: '0x0' },
-      };
-      bridge.ethereumSignTransaction = sinon.stub().resolves(successResponse);
-
-      // Mock the transaction verification to pass
-      sinon.stub(fakeTx, 'verifySignature').callsFake(() => true);
-      sinon
-        .stub(fakeTx, 'getSenderAddress')
-        .callsFake(() =>
-          Buffer.from(Address.fromString(fakeAccounts[0]).bytes),
-        );
-
-      // @ts-expect-error: intentionally using an old library that doesn't comply with TypedTransaction
-      const result = await keyring.signTransaction(fakeAccounts[0], fakeTx);
-      expect(result).toBeDefined();
-      expect(result.v).toBeDefined();
-      expect(result.r).toBeDefined();
-      expect(result.s).toBeDefined();
-    });
-
     it('should handle basic unlock scenarios', async function () {
       const accounts = await keyring.getAccounts();
       expect(accounts).toStrictEqual([]);