feat: improve Keystone USB error handling for homepage requirement#750
feat: improve Keystone USB error handling for homepage requirement#750Qkin-Keystone wants to merge 3 commits intoava-labs:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves error handling for Keystone USB hardware wallets when the device is not on its homepage. The changes prevent ineffective auto-retry behavior and provide clear instructions to users.
Changes:
- Added detection and propagation of "not on homepage" error state through the approval flow
- Enhanced UI to display actionable error messages when Keystone device requires manual navigation
- Disabled automatic retry mechanism when device needs to be manually returned to homepage
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/service-worker/src/services/keystone/constants/error.ts | Defines the constant for the Keystone homepage error message |
| packages/service-worker/src/vmModules/ApprovalController.ts | Adds logic to detect and propagate homepage error instead of treating it as a user rejection |
| packages/service-worker/src/services/actions/handlers/updateAction.ts | Wraps async response in try-catch to properly return errors |
| packages/ui/src/hooks/useApproveAction.ts | Captures homepage error and sets error state instead of closing window |
| apps/next/src/pages/Approve/components/hardware/keystone-usb/types.ts | Adds error property to state component props |
| apps/next/src/pages/Approve/components/hardware/keystone-usb/KeystoneUSBApprovalOverlay.tsx | Changes state to 'disconnected' when homepage error occurs and passes error to components |
| apps/next/src/pages/Approve/components/hardware/keystone-usb/components/Disconnected.tsx | Displays homepage-specific error message and disables reconnect button and auto-retry |
| apps/next/src/pages/Approve/components/hardware/HardwareApprovalOverlay.tsx | Passes error prop through component hierarchy |
| apps/next/src/pages/Approve/TransactionApprovalScreen.tsx | Adds note for Keystone users and passes error to overlay component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resolve({ | ||
| error: providerErrors.userRejectedRequest(), | ||
| }); | ||
| this.#requests.delete(action.actionId); |
There was a problem hiding this comment.
The code deletes the request in both the user rejection case (line 233) and the default error case (line 243). This duplication could be simplified by moving the delete operation outside the conditional blocks to a single location that handles all non-homepage error cases.
| }, | ||
| }), | ||
| }); | ||
| this.#requests.delete(action.actionId); |
There was a problem hiding this comment.
The code deletes the request in both the user rejection case (line 233) and the default error case (line 243). This duplication could be simplified by moving the delete operation outside the conditional blocks to a single location that handles all non-homepage error cases.
| > = ({ action, reject, approve }) => { | ||
| const state = useKeystoneUsbApprovalState(); | ||
| > = ({ action, reject, approve, error }) => { | ||
| let state = useKeystoneUsbApprovalState(); |
There was a problem hiding this comment.
Using let to reassign a hook's return value is not a typical React pattern. Consider using a derived state variable or a useMemo hook instead. For example: const derivedState = useMemo(() => state === 'pending' && error === KEYSTONE_NOT_IN_HOMEPAGE_ERROR ? 'disconnected' : state, [state, error]);
| }} | ||
| > | ||
| <strong>Note: </strong> | ||
| {'Ensure your Keystone 3 Pro is on the homepage'} |
There was a problem hiding this comment.
The message text is hardcoded in a JSX expression using curly braces. For consistency with other translatable strings in the component and to support internationalization, this should use the t() function: t('Ensure your Keystone 3 Pro is on the homepage')
| {'Ensure your Keystone 3 Pro is on the homepage'} | |
| {t('Ensure your Keystone 3 Pro is on the homepage')} |
| if (errorMessage.includes(KEYSTONE_NOT_IN_HOMEPAGE_ERROR)) { | ||
| setError(KEYSTONE_NOT_IN_HOMEPAGE_ERROR); | ||
| return false; | ||
| } else { | ||
| if (shouldCloseAfterUpdate) { | ||
| globalThis.close(); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Both branches of the if-else statement return false, making the conditional close behavior the only difference. This can be simplified by handling the window close logic first, then returning false unconditionally: check the error type, set error state if needed, close window if not homepage error and shouldCloseAfterUpdate is true, then return false.
| if (errorMessage.includes(KEYSTONE_NOT_IN_HOMEPAGE_ERROR)) { | |
| setError(KEYSTONE_NOT_IN_HOMEPAGE_ERROR); | |
| return false; | |
| } else { | |
| if (shouldCloseAfterUpdate) { | |
| globalThis.close(); | |
| } | |
| return false; | |
| } | |
| const isKeystoneNotInHomepageError = errorMessage.includes( | |
| KEYSTONE_NOT_IN_HOMEPAGE_ERROR, | |
| ); | |
| if (!isKeystoneNotInHomepageError && shouldCloseAfterUpdate) { | |
| globalThis.close(); | |
| } | |
| if (isKeystoneNotInHomepageError) { | |
| setError(KEYSTONE_NOT_IN_HOMEPAGE_ERROR); | |
| } | |
| return false; |
Qkin-Keystone
force-pushed
the
feat/keystone-usb-error-handling
branch
from
c0051b7 to
38660dd
Compare
There was a problem hiding this comment.
This PR kind of goes against the grain if you look at other wallets' implementations:
- With other devices, the
Approve on <<device>>message is only shown in the UI when the device is in the correct state. This here allows the user to sign, only to then show an error message. Don't get me wrong -- it's better than just erroring out without any context, but the end result is very similar and still annoying -- the user has to manually click reject and retry. - The PR also relies on
ApprovalController.requestApproval()throwing an error, which it should never do. We rely on this method to always resolve (albeit with{ error }property) in other places. - It also creates a very Keystone-specific if-clauses in places where I would not expect them. If we want to add this, we should create a util that is more generic (i.e.
isErrorRetryable, or something along those lines, and allow users to retry signing without forcing them out of the approval flow). - This PR creates a constant
KEYSTONE_NOT_IN_HOMEPAGE_ERRORin our repository that is meant to represent the error message returned by an external SDK/device. I'd say this is not the way to go -- your SDK should either export a specific error class or error code that we can test the error against. Otherwise this is very fragile (if you change the error message on the device, everything fails here).
I'd say let's leave it as it is for now, or add a way to detect the device being on the wrong screen before sending the signing request to the device.
Qkin-Keystone
had a problem deploying
to
alpha
GitHub Actions
Failure
Description
Improves Keystone USB error handling when device is not on homepage. Adds clear user guidance and stops ineffective auto-retry behavior.
Changes
Testing
Screenshots:
N/A - Error handling improvements, no visual design changes
Checklist for the author
Tick each of them when done or if not applicable.