fix: properly surface Treezor user rejection errors#8490
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6fae189. Configure here.
| this.#hasUserRejectedMessage( | ||
| error as Error & { cause?: unknown; originalError?: unknown }, | ||
| visited, | ||
| ) |
There was a problem hiding this comment.
Error cause/originalError never checked for Error instances
High Severity
In #hasUserRejectedMessage, the instanceof Error branch recurses with error as Error & { cause?: unknown; originalError?: unknown } — but that type assertion is compile-time only, so it passes the same object reference back. Since error was already added to visited, the recursive call immediately returns false, meaning cause and originalError are never inspected for Error instances. The typeof error === 'object' branch below correctly checks those properties, but it's unreachable for Errors. Compare with #isSigningUserRejectedError in KeyringController.ts which correctly accesses each property individually.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6fae189. Configure here.
|
@copilot resolve the merge conflicts in this pull request |
…ve-error-trezor # Conflicts: # packages/keyring-controller/package.json # packages/transaction-controller/src/TransactionController.ts Co-authored-by: dawnseeker8 <7315988+dawnseeker8@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MetaMask/core/sessions/64a5b953-7f1d-4e47-be62-0f18a2f761f9 Co-authored-by: dawnseeker8 <7315988+dawnseeker8@users.noreply.github.com>
Resolved the merge conflicts in commit
The yarn.lock was updated accordingly, and all 1,400+ tests across both affected packages continue to pass. |
|
@Olivier-BB please update this branch and fix CI so we can get this reviewed and merged. |
|
@copilot resolve the merge conflicts in this pull request |
…essage methods - Introduced tests to ensure that errors with code 4001 and string rejections are normalized to user rejection errors. - Added checks to verify that non-rejection errors are re-thrown unchanged, including those with circular references. - Included a test for normalizing cancellation-like errors in signPersonalMessage to a 4001 code.
- Consolidated multiple lines of jest.spyOn calls into single lines for improved readability. - Maintained existing test functionality while enhancing code clarity.
Explanation
Treezor user rejection errors were not being surfaced clearly, which could result in uninformative error handling around rejected requests.
This change updates the relevant controller flows so Treezor user rejection errors are surfaced properly, and adds test coverage in both
keyring-controllerandtransaction-controllerto verify the behavior.KeyringControllernow catchessignMessage/signPersonalMessage/signTransactionerrors (and conditionallysignTypedMessage) and normalizes cancellation-like errors by recursively inspectingcode,message,stack,cause, andoriginalError.TransactionControllersimilarly detects cancellation-like errors (including a Trezor/"unknown error" string case), persists them asuserRejectedRequest, and updates tests to assert the persisted errorcode.The
TransactionControllertests for "cancelled"/"canceled" signing error normalization have been updated to use theKeyringController:signTransactionmessenger action handler (rather than the legacysignoption) to properly exercise the error path, reflecting the current signing architecture.References
Checklist