Conversation
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| coin: 'ETH', | ||
| }); | ||
| if (!response.success) { | ||
| throw new Error(response.payload?.error || 'Unknown error'); |
There was a problem hiding this comment.
I know this used to be ||, but given that we're changing that, let's fix it too:
| throw new Error(response.payload?.error || 'Unknown error'); | |
| throw new Error(response.payload?.error ?? 'Unknown error'); |
| await expect( | ||
| keyring.signTypedData( | ||
| fakeAccounts[0], | ||
| { | ||
| types: { EIP712Domain: [], EmptyMessage: [] }, | ||
| primaryType: 'EmptyMessage', | ||
| domain: {}, | ||
| message: {}, | ||
| }, | ||
| { version: SignTypedDataVersion.V4 }, | ||
| ), | ||
| ).rejects.toThrow(HardwareWalletError); | ||
|
|
||
| await expect( | ||
| keyring.signTypedData( | ||
| fakeAccounts[0], | ||
| { | ||
| types: { EIP712Domain: [], EmptyMessage: [] }, | ||
| primaryType: 'EmptyMessage', | ||
| domain: {}, | ||
| message: {}, | ||
| }, | ||
| { version: SignTypedDataVersion.V4 }, | ||
| ), | ||
| ).rejects.toMatchObject({ | ||
| code: ErrorCode.Unknown, | ||
| }); |
There was a problem hiding this comment.
Let's be DRY here:
| await expect( | |
| keyring.signTypedData( | |
| fakeAccounts[0], | |
| { | |
| types: { EIP712Domain: [], EmptyMessage: [] }, | |
| primaryType: 'EmptyMessage', | |
| domain: {}, | |
| message: {}, | |
| }, | |
| { version: SignTypedDataVersion.V4 }, | |
| ), | |
| ).rejects.toThrow(HardwareWalletError); | |
| await expect( | |
| keyring.signTypedData( | |
| fakeAccounts[0], | |
| { | |
| types: { EIP712Domain: [], EmptyMessage: [] }, | |
| primaryType: 'EmptyMessage', | |
| domain: {}, | |
| message: {}, | |
| }, | |
| { version: SignTypedDataVersion.V4 }, | |
| ), | |
| ).rejects.toMatchObject({ | |
| code: ErrorCode.Unknown, | |
| }); | |
| const fakeTypedData = { | |
| types: { EIP712Domain: [], EmptyMessage: [] }, | |
| primaryType: 'EmptyMessage', | |
| domain: {}, | |
| message: {}, | |
| }; | |
| await expect( | |
| keyring.signTypedData( | |
| fakeAccounts[0], | |
| fakeTypedData, | |
| { version: SignTypedDataVersion.V4 }, | |
| ), | |
| ).rejects.toThrow(HardwareWalletError); | |
| await expect( | |
| keyring.signTypedData( | |
| fakeAccounts[0], | |
| fakeTypedData, | |
| { version: SignTypedDataVersion.V4 }, | |
| ), | |
| ).rejects.toMatchObject({ | |
| code: ErrorCode.Unknown, | |
| }); |
There was a problem hiding this comment.
I'm slightly confused here, we used to those mapping in our own SDK hw-wallet-sdk, should we put this back there? Like this one: https://github.com/MetaMask/accounts/blob/main/packages/hw-wallet-sdk/src/hardware-error-mappings.ts
There was a problem hiding this comment.
Or maybe we should do it the other way around and have the Ledger mapping in our own "ledger keyring" package. This way we can re-use constants coming from official Ledger packages.
The SDK would just be the common definitions/interface that we can use in of our HW wallet packages.
That might be a better move actually!
| const normalizedIdentifier = normalizeValue(identifier); | ||
| const mappedIdentifier = NORMALIZED_IDENTIFIER_MAP.get(normalizedIdentifier); | ||
| if (!mappedIdentifier) { | ||
| return undefined; | ||
| } | ||
| return TREZOR_ERROR_MAPPINGS[mappedIdentifier]; |
There was a problem hiding this comment.
We can just use this? (we can maybe even omit the ?. part, since it would return undefined if it's not known).
Though, we can keep the if if you prefer to be explicit (I'm definitely ok with this)
| const normalizedIdentifier = normalizeValue(identifier); | |
| const mappedIdentifier = NORMALIZED_IDENTIFIER_MAP.get(normalizedIdentifier); | |
| if (!mappedIdentifier) { | |
| return undefined; | |
| } | |
| return TREZOR_ERROR_MAPPINGS[mappedIdentifier]; | |
| return TREZOR_ERROR_MAPPINGS?.[getTrezorErrorIdentifier(identifier)]; |
| const errorMapping = getTrezorErrorMapping(identifier); | ||
|
|
||
| if (errorMapping) { | ||
| const normalizedContext = context?.trim().toLowerCase(); |
There was a problem hiding this comment.
Let's re-use the same "normalize" function we already have, in case we decide to normalize differently:
| const normalizedContext = context?.trim().toLowerCase(); | |
| const normalizedContext = context ? normalizeValue(context) : undefined; |
|
|
||
| if (errorMapping) { | ||
| const normalizedContext = context?.trim().toLowerCase(); | ||
| const normalizedMessage = errorMapping.message.toLowerCase(); |
There was a problem hiding this comment.
Same here (though, is that ok to trim this one?):
| const normalizedMessage = errorMapping.message.toLowerCase(); | |
| const normalizedMessage = normalizeValue(errorMapping.message); |
| }); | ||
|
|
||
| if (!response.success) { | ||
| throw new Error(response.payload?.error || 'Unknown error'); |
There was a problem hiding this comment.
| throw new Error(response.payload?.error || 'Unknown error'); | |
| throw new Error(response.payload?.error ?? 'Unknown error'); |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
2 participants
This PR updates the trezor keyring to use the hardware error management
Examples
Note
Medium Risk
Changes Trezor keyring error propagation to throw typed
HardwareWalletErrors and adds new mapping data, which can affect how callers detect/handle failures across unlock/sign flows.Overview
Standardizes Trezor error handling across the repo.
@metamask/hw-wallet-sdkgainsTREZOR_ERROR_MAPPINGS(with new tests) and the rootREADMEdependency graph is updated accordingly.Migrates
@metamask/eth-trezor-keyringto use the shared SDK. The package now depends on@metamask/hw-wallet-sdk, exports new Trezor error utilities, and addstrezor-errors/trezor-error-handlerhelpers that convert bridge errors into typedHardwareWalletErrors.Updates keyring flows to use the new typed errors.
unlock,signTransaction,signPersonalMessage, andsignTypedDataare refactored to async/await and route failures throughhandleTrezorTransportError(while preserving address-mismatch validation as plainErrors), with expanded unit tests and higher Jest coverage thresholds.Written by Cursor Bugbot for commit e7616b9. This will update automatically on new commits. Configure here.