From 2d5399cc68c6cbad94c9372b7596a6183ca78205 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Mon, 9 Jun 2025 08:48:40 +0100 Subject: [PATCH 1/5] feat: enhance error handling for Ledger transactions Added static method to identify Ledger-specific errors and improved error handling in the LedgerKeyring class. Now, specific error messages are thrown for user-rejected transactions and when blind signing is disabled, providing clearer feedback to users during transaction signing. --- .../src/ledger-keyring.ts | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index 89fb52f02..3b13c5255 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -94,6 +94,17 @@ export class LedgerKeyring implements Keyring { deviceId = ''; + static #isLedgerError( + error: unknown, + ): error is { errorCode: string; message: string } { + return ( + typeof error === 'object' && + error !== null && + 'errorCode' in error && + 'message' in error + ); + } + readonly type: string = keyringType; page = 0; @@ -414,6 +425,26 @@ export class LedgerKeyring implements Keyring { hdPath, }); } catch (error) { + /** + * for user rejected the transaction error + */ + if ( + LedgerKeyring.#isLedgerError(error) && + error.errorCode === '6985' && + error.message === 'Condition not satisfied' + ) { + throw new Error('Ledger: User rejected the transaction'); + } + /** + * for blind signing disabled error + */ + if ( + LedgerKeyring.#isLedgerError(error) && + error.errorCode === '6a80' && + error.message === 'Invalid data' + ) { + throw new Error('Ledger: Blind signing must be enabled'); + } throw error instanceof Error ? error : new Error('Ledger: Unknown error while signing transaction'); @@ -449,6 +480,17 @@ export class LedgerKeyring implements Keyring { message: remove0x(message), }); } catch (error) { + /** + * for user rejected the transaction error + */ + if ( + LedgerKeyring.#isLedgerError(error) && + error.errorCode === '6985' && + error.message === 'Condition not satisfied' + ) { + throw new Error('Ledger: User rejected the transaction'); + } + throw error instanceof Error ? error : new Error('Ledger: Unknown error while signing message'); @@ -541,6 +583,26 @@ export class LedgerKeyring implements Keyring { }, }); } catch (error) { + /** + * for user rejected the transaction error + */ + if ( + LedgerKeyring.#isLedgerError(error) && + error.errorCode === '6985' && + error.message === 'Condition not satisfied' + ) { + throw new Error('Ledger: User rejected the transaction'); + } + /** + * for blind signing disabled error + */ + if ( + LedgerKeyring.#isLedgerError(error) && + error.errorCode === '6a80' && + error.message === 'Invalid data' + ) { + throw new Error('Ledger: Blind signing must be enabled'); + } throw error instanceof Error ? error : new Error('Ledger: Unknown error while signing message'); From 8d08f2e0bb8a3fe4b730bff1b7febc1516486766 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Mon, 9 Jun 2025 12:46:49 +0100 Subject: [PATCH 2/5] Improve error handling to handle TransportStatusError. --- .../src/ledger-keyring.ts | 97 +++++++++++-------- 1 file changed, 56 insertions(+), 41 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index 3b13c5255..08689986f 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -5,6 +5,7 @@ import { type TypedTransaction, } from '@ethereumjs/tx'; import { publicToAddress } from '@ethereumjs/util'; +import { TransportStatusError } from '@ledgerhq/hw-transport'; import type { MessageTypes, TypedMessage } from '@metamask/eth-sig-util'; import { recoverPersonalSignature, @@ -424,27 +425,29 @@ export class LedgerKeyring implements Keyring { tx: remove0x(rawTxHex), hdPath, }); - } catch (error) { - /** - * for user rejected the transaction error - */ - if ( - LedgerKeyring.#isLedgerError(error) && - error.errorCode === '6985' && - error.message === 'Condition not satisfied' - ) { - throw new Error('Ledger: User rejected the transaction'); - } - /** - * for blind signing disabled error - */ - if ( - LedgerKeyring.#isLedgerError(error) && - error.errorCode === '6a80' && - error.message === 'Invalid data' - ) { - throw new Error('Ledger: Blind signing must be enabled'); + } catch (error: unknown) { + // check whether error is TransportError + if (error instanceof TransportStatusError) { + // cast error to TransportError + const transportError: TransportStatusError = error; + if ( + transportError.statusCode === 27013 && + transportError.message.includes('(denied by the user?) (0x6985)') + ) { + throw new Error('Ledger: User rejected the transaction'); + } + + /** + * for user rejected the transaction error + */ + if ( + transportError.statusCode === 27013 && + transportError.message.includes('(denied by the user?) (0x6985)') + ) { + throw new Error('Ledger: User rejected the transaction'); + } } + throw error instanceof Error ? error : new Error('Ledger: Unknown error while signing transaction'); @@ -479,7 +482,19 @@ export class LedgerKeyring implements Keyring { hdPath, message: remove0x(message), }); - } catch (error) { + } catch (error: unknown) { + if (error instanceof TransportStatusError) { + const transportError: TransportStatusError = error; + /** + * for user rejected the transaction error + */ + if ( + transportError.statusCode === 27013 && + transportError.message.includes('(denied by the user?) (0x6985)') + ) { + throw new Error('Ledger: User rejected the transaction'); + } + } /** * for user rejected the transaction error */ @@ -582,26 +597,26 @@ export class LedgerKeyring implements Keyring { message, }, }); - } catch (error) { - /** - * for user rejected the transaction error - */ - if ( - LedgerKeyring.#isLedgerError(error) && - error.errorCode === '6985' && - error.message === 'Condition not satisfied' - ) { - throw new Error('Ledger: User rejected the transaction'); - } - /** - * for blind signing disabled error - */ - if ( - LedgerKeyring.#isLedgerError(error) && - error.errorCode === '6a80' && - error.message === 'Invalid data' - ) { - throw new Error('Ledger: Blind signing must be enabled'); + } catch (error: unknown) { + if (error instanceof TransportStatusError) { + // cast error to TransportError + const transportError: TransportStatusError = error; + if ( + transportError.statusCode === 27013 && + transportError.message.includes('(denied by the user?) (0x6985)') + ) { + throw new Error('Ledger: User rejected the transaction'); + } + + /** + * for user rejected the transaction error + */ + if ( + transportError.statusCode === 27013 && + transportError.message.includes('(denied by the user?) (0x6985)') + ) { + throw new Error('Ledger: User rejected the transaction'); + } } throw error instanceof Error ? error From 87ad1867cb1f4157dfae5475c4026197b2640f7e Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Mon, 9 Jun 2025 13:56:28 +0100 Subject: [PATCH 3/5] Including blind signing disable error. --- .../src/ledger-keyring.ts | 28 ++++--------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index 08689986f..348c1b1e0 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -437,14 +437,11 @@ export class LedgerKeyring implements Keyring { throw new Error('Ledger: User rejected the transaction'); } - /** - * for user rejected the transaction error - */ if ( - transportError.statusCode === 27013 && - transportError.message.includes('(denied by the user?) (0x6985)') + transportError.statusCode === 27264 && + transportError.message.includes('Invalid data received (0x6a80)') ) { - throw new Error('Ledger: User rejected the transaction'); + throw new Error('Ledger: Blind signing must be enabled'); } } @@ -495,16 +492,6 @@ export class LedgerKeyring implements Keyring { throw new Error('Ledger: User rejected the transaction'); } } - /** - * for user rejected the transaction error - */ - if ( - LedgerKeyring.#isLedgerError(error) && - error.errorCode === '6985' && - error.message === 'Condition not satisfied' - ) { - throw new Error('Ledger: User rejected the transaction'); - } throw error instanceof Error ? error @@ -608,14 +595,11 @@ export class LedgerKeyring implements Keyring { throw new Error('Ledger: User rejected the transaction'); } - /** - * for user rejected the transaction error - */ if ( - transportError.statusCode === 27013 && - transportError.message.includes('(denied by the user?) (0x6985)') + transportError.statusCode === 27264 && + transportError.message.includes('Invalid data received (0x6a80)') ) { - throw new Error('Ledger: User rejected the transaction'); + throw new Error('Ledger: Blind signing must be enabled'); } } throw error instanceof Error From e34ca0948a50d599374b63826108ccb0b6103f71 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Tue, 10 Jun 2025 04:28:58 +0100 Subject: [PATCH 4/5] Enhance error handling in LedgerKeyring tests Added tests to handle specific TransportStatusError codes for user rejection and blind signing scenarios in transaction signing, personal message signing, and typed data signing. This improves the robustness of error handling in the LedgerKeyring class. --- .../src/ledger-keyring.test.ts | 143 ++++++++++++++++++ 1 file changed, 143 insertions(+) diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts index 694ffb536..29f660dfd 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.test.ts @@ -2,6 +2,7 @@ import { Common, Chain, Hardfork } from '@ethereumjs/common'; import { RLP } from '@ethereumjs/rlp'; import { TransactionFactory } from '@ethereumjs/tx'; import * as ethUtil from '@ethereumjs/util'; +import { TransportStatusError } from '@ledgerhq/hw-transport'; import * as sigUtil from '@metamask/eth-sig-util'; import { bytesToHex, Hex, remove0x } from '@metamask/utils'; import EthereumTx from 'ethereumjs-tx'; @@ -784,6 +785,60 @@ describe('LedgerKeyring', function () { keyring.signTransaction(fakeAccounts[0], fakeTx), ).rejects.toThrow('Ledger: The transaction signature is not valid'); }); + + it('throws user rejection error when TransportStatusError with code 27013 is thrown', async function () { + await basicSetupToUnlockOneAccount(); + const transportError = { + statusCode: 27013, + message: 'Ledger device: (denied by the user?) (0x6985)', + name: 'TransportStatusError', + }; + + Object.setPrototypeOf(transportError, TransportStatusError.prototype); + jest + .spyOn(keyring.bridge, 'deviceSignTransaction') + .mockRejectedValue(transportError); + + await expect( + keyring.signTransaction(fakeAccounts[0], fakeTx), + ).rejects.toThrow('Ledger: User rejected the transaction'); + }); + + it('throws blind signing error when TransportStatusError with code 27264 is thrown', async function () { + await basicSetupToUnlockOneAccount(); + const transportError = { + statusCode: 27264, + message: 'Ledger device: Invalid data received (0x6a80)', + name: 'TransportStatusError', + }; + + Object.setPrototypeOf(transportError, TransportStatusError.prototype); + jest + .spyOn(keyring.bridge, 'deviceSignTransaction') + .mockRejectedValue(transportError); + + await expect( + keyring.signTransaction(fakeAccounts[0], fakeTx), + ).rejects.toThrow('Ledger: Blind signing must be enabled'); + }); + + it('re-throws TransportStatusError with unknown status code', async function () { + await basicSetupToUnlockOneAccount(); + const transportError = { + statusCode: 12345, + message: 'Some other transport error', + name: 'TransportStatusError', + }; + + Object.setPrototypeOf(transportError, TransportStatusError.prototype); + jest + .spyOn(keyring.bridge, 'deviceSignTransaction') + .mockRejectedValue(transportError); + + await expect( + keyring.signTransaction(fakeAccounts[0], fakeTx), + ).rejects.toThrow(transportError); + }); }); describe('signPersonalMessage', function () { @@ -858,6 +913,40 @@ describe('LedgerKeyring', function () { 'Ledger: The signature doesnt match the right address', ); }); + + it('throws user rejection error when TransportStatusError with code 27013 is thrown in signPersonalMessage', async function () { + await basicSetupToUnlockOneAccount(); + const transportError = { + statusCode: 27013, + message: 'Ledger device: (denied by the user?) (0x6985)', + name: 'TransportStatusError', + }; + Object.setPrototypeOf(transportError, TransportStatusError.prototype); + jest + .spyOn(keyring.bridge, 'deviceSignMessage') + .mockRejectedValue(transportError); + + await expect( + keyring.signPersonalMessage(fakeAccounts[0], 'some message'), + ).rejects.toThrow('Ledger: User rejected the transaction'); + }); + + it('re-throws TransportStatusError with unknown status code in signPersonalMessage', async function () { + await basicSetupToUnlockOneAccount(); + const transportError = { + statusCode: 12345, + message: 'Some other transport error', + name: 'TransportStatusError', + }; + Object.setPrototypeOf(transportError, TransportStatusError.prototype); + jest + .spyOn(keyring.bridge, 'deviceSignMessage') + .mockRejectedValue(transportError); + + await expect( + keyring.signPersonalMessage(fakeAccounts[0], 'some message'), + ).rejects.toThrow(transportError); + }); }); describe('signMessage', function () { @@ -1207,6 +1296,60 @@ describe('LedgerKeyring', function () { '0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e3200', ); }); + + it('throws user rejection error when TransportStatusError with code 27013 is thrown in signTypedData', 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, 'deviceSignTypedData') + .mockRejectedValue(transportError); + + await expect( + keyring.signTypedData(fakeAccounts[15], fixtureData, { + version: sigUtil.SignTypedDataVersion.V4, + }), + ).rejects.toThrow('Ledger: User rejected the transaction'); + }); + + it('throws blind signing error when TransportStatusError with code 27264 is thrown in signTypedData', async function () { + const transportError = { + statusCode: 27264, + message: 'Ledger device: Invalid data received (0x6a80)', + name: 'TransportStatusError', + }; + Object.setPrototypeOf(transportError, TransportStatusError.prototype); + jest + .spyOn(keyring.bridge, 'deviceSignTypedData') + .mockRejectedValue(transportError); + + await expect( + keyring.signTypedData(fakeAccounts[15], fixtureData, { + version: sigUtil.SignTypedDataVersion.V4, + }), + ).rejects.toThrow('Ledger: Blind signing must be enabled'); + }); + + it('re-throws TransportStatusError with unknown status code in signTypedData', async function () { + const transportError = { + statusCode: 12345, + message: 'Some other transport error', + name: 'TransportStatusError', + }; + Object.setPrototypeOf(transportError, TransportStatusError.prototype); + jest + .spyOn(keyring.bridge, 'deviceSignTypedData') + .mockRejectedValue(transportError); + + await expect( + keyring.signTypedData(fakeAccounts[15], fixtureData, { + version: sigUtil.SignTypedDataVersion.V4, + }), + ).rejects.toThrow(transportError); + }); }); describe('destroy', function () { From fc1045289432862632f6e47df095b1d4aa5cf935 Mon Sep 17 00:00:00 2001 From: Xiaoming Wang Date: Tue, 10 Jun 2025 04:42:29 +0100 Subject: [PATCH 5/5] Remove redundant functions in ledger-keyring.ts --- packages/keyring-eth-ledger-bridge/jest.config.js | 6 +++--- .../keyring-eth-ledger-bridge/src/ledger-keyring.ts | 11 ----------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/packages/keyring-eth-ledger-bridge/jest.config.js b/packages/keyring-eth-ledger-bridge/jest.config.js index de4929243..a6c5d8576 100644 --- a/packages/keyring-eth-ledger-bridge/jest.config.js +++ b/packages/keyring-eth-ledger-bridge/jest.config.js @@ -23,10 +23,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 91.89, + branches: 92.77, functions: 97.87, - lines: 97.16, - statements: 97.19, + lines: 97.28, + statements: 97.31, }, }, }); diff --git a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts index 348c1b1e0..bd51a0e9b 100644 --- a/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts +++ b/packages/keyring-eth-ledger-bridge/src/ledger-keyring.ts @@ -95,17 +95,6 @@ export class LedgerKeyring implements Keyring { deviceId = ''; - static #isLedgerError( - error: unknown, - ): error is { errorCode: string; message: string } { - return ( - typeof error === 'object' && - error !== null && - 'errorCode' in error && - 'message' in error - ); - } - readonly type: string = keyringType; page = 0;