fix: ledger ETH app errors#26322
Conversation
…24547) The BLE transport disconnects when the Ledger switches from BOLOS to the Ethereum app. If the disconnect races ahead of the APDU response, the `openEthereumAppOnLedger()` promise rejects and the workflow restart state (`shouldRestartConnection` / `workflowSteps`) is never set, preventing the disconnect handler from re-entering the signing flow. Move the restart registration before the APDU calls that trigger the disconnect and treat `DisconnectedDevice*` errors as expected (non-fatal) during app-switch so the reconnect handler can resume the workflow. The same pattern is applied to `closeRunningAppOnLedger()`. Co-authored-by: Cursor <cursoragent@cursor.com>
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
This update introduces an `isReconnecting` ref to manage late Bluetooth Low Energy (BLE) disconnects during app-switching. The hook now prevents workflow steps from being cleared when a disconnect occurs while processing the app switch. Additionally, the documentation has been updated to reflect this internal mechanism, noting that it cannot be tested due to transport mock limitations.
…dling This commit modifies the `ledgerLogicToRun` function to utilize a ref, allowing for better management of asynchronous operations within the `LedgerSelectAccount` component. The changes ensure that the latest version of `ledgerLogicToRun` is always referenced during execution, enhancing the reliability of ledger operations. Additionally, the test files are updated to reflect this change in behavior.
This commit modifies the `ledgerLogicToRun` function in the `LedgerSelectAccount` tests to accept a transport parameter, enhancing its async handling capabilities. The change ensures that the function can be called with an undefined transport, improving the flexibility of the mock implementation. Corresponding test updates have been made to reflect this change.
…ive error handling This commit refactors the `isDisconnectError` function to be re-exported from `ledgerErrors`, enhancing its accessibility across the codebase. The tests for `isDisconnectError` have been updated to ensure comprehensive coverage, including various error scenarios. Additionally, the documentation has been improved to clarify the purpose of the `DISCONNECT_ERROR_NAMES` constant and its usage in error classification.
…lity and maintaining existing functionality. Comprehensive tests have been added to ensure proper behavior with the Bluetooth transport.
…ror recovery This commit adds comprehensive tests to the `useLedgerBluetooth` hook, ensuring it properly handles synchronous disconnects during app operations and resets the reconnecting state on errors. The tests cover scenarios for both BOLOS and non-Ethereum app interactions, verifying that the hook maintains expected behavior and error states during these operations.
…ount component - Enhanced error handling in ledger account fetching functions to ensure error messages are displayed correctly. - Updated loading modal visibility management to ensure it is consistently shown and hidden during asynchronous operations. - Refactored pagination logic to streamline account retrieval for next and previous pages, improving code readability and maintainability.
- Introduced functions to classify transport status errors and error messages, improving the handling of various Ledger communication errors. - Refactored the error handling logic in the `processLedgerWorkflow` function to streamline the management of app opening and closing errors. - Added retry logic for connection attempts, ensuring better resilience in the Bluetooth communication process with Ledger devices.
The committed fixture schema is out of date. To update, comment: |
Modified the type handling in the LedgerSelectAccount tests and the loadBleTransport hook to use BluetoothInterface for better type safety. This change enhances code clarity and ensures proper type assertions during Bluetooth transport operations.
Replaced the type assertion for `connectLedgerHardware` with the `jest.mocked` utility in the LedgerBluetoothAdapter test suite. This change improves type safety and consistency across the tests, ensuring better maintainability and clarity in the mocking of hardware interactions.
…th hook Refactored the error classification logic in the useLedgerBluetooth hook for better readability by formatting the conditional statements and return statements. This change enhances code clarity without altering functionality.
|
@metamaskbot update-mobile-fixture |
NicolasMassart
left a comment
There was a problem hiding this comment.
despite my multiple requests, I eventually filled the PR Manual testing steps myself, so we can have future easy understanding of the fixed issues and testing without having to dig in the related issue.
Sometimes there's discrepency between issue and PR for many reasons, so PR should be the final source of truth and include all the required data to approve.
Note that when this will land in a release, it will be required to test again and you will have long forgotten the context. Having clear tests steps for you to follow will be crucial.
Thank you for taking this into account for next time.
The manually tests steps have been mentioned in 3 relative issues. |
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
Why no E2E tags are needed:
The changes improve Ledger connection reliability and error handling but don't impact any testable E2E flows. Performance Test Selection: |
Menaul tests steps have been copied from three issues into this PR for future references. |
|
Description
This PR solve multiple issues related to the ledger ETH app. These are issue #24547, #25631, and #25947
Changelog
CHANGELOG entry: fix Ledger transaction not displayed after opening ETH app.
CHANGELOG entry: fix pagination and unlock accounts uninformative error after opening ETH app
Related issues
Fixes: #24547
Fixes: #25631
Fixes: #25947
Manual testing steps
Steps to reproduce
#24547
Open MM mobile
#25631
#25947
Screenshots/Recordings
Before
Check issues: #24547 #25631 #25947
After
TBC...
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Refactors Ledger BLE workflow/retry and disconnect handling, which can affect hardware-wallet connection stability and signing flows. Risk is mitigated by substantial new unit coverage across app-switch, error-classification, and retry scenarios.
Overview
Fixes Ledger ETH-app recovery issues by routing account pagination/unlock flows through
useLedgerBluetooth.ledgerLogicToRun, ensuring operations resume cleanly after prompting users to open/close the Ethereum app and avoiding stuck loading or misleading disconnect errors.Refactors
useLedgerBluetoothto use a mockableloadBleTransportwrapper (instead of native dynamic import), adds explicit disconnect classification via newledgerErrorshelpers (DISCONNECT_ERROR_NAMES,isDisconnectError), and hardens reconnect/disconnect state handling (including app-switch recursion + restart limits). Tests are significantly expanded for the hook and updated forLedgerBluetoothAdaptertransient BLE retry behavior.Written by Cursor Bugbot for commit f4f232f. This will update automatically on new commits. Configure here.