Feat(suite-native): retry FW revision check if it's stale #17437
Conversation
|
🚀 Expo preview is ready!
|
| this.authenticityChecks = { | ||
| ...this.authenticityChecks, | ||
| firmwareRevision: result, | ||
| }; |
There was a problem hiding this comment.
this shouldn't be necessary 😅
But it is 🙈 see slack convo
TL;DR: there is not yet deepclone isolation between Connect and redux in mobile 📱 , like it is on Web. That causes problems because the objects are passed byRef from Connect, redux does its magic with them, and it causes weird bugs back in Connect. Namely: assignment of authenticityChecks.firmwareRevision does not work, have to assign authenticityChecks 🙉
WalkthroughThe changes update the firmware authenticity verification process and its integration into the application. The device module now creates new objects for firmware hash and revision updates instead of modifying the existing object directly. A new custom React hook, 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-native/device/src/hooks/useRetryFwAuthenticityChecks.ts (1)
19-24: Consider adding error handlingWhile the comment explains that you're not concerned with the promise result, it's a good practice to add error handling to prevent unhandled promise rejections.
- TrezorConnect.runAuthenticityChecks().then(); + TrezorConnect.runAuthenticityChecks().catch(error => { + console.error('Error retrying firmware authenticity checks:', error); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/connect/src/api/index.ts(1 hunks)packages/connect/src/api/runAuthenticityChecks.ts(1 hunks)packages/connect/src/device/Device.ts(4 hunks)packages/connect/src/factory.ts(1 hunks)packages/connect/src/types/api/index.ts(2 hunks)packages/connect/src/types/api/runAuthenticityChecks.ts(1 hunks)suite-native/app/src/hooks/useGlobalHooks.tsx(2 hunks)suite-native/device/src/hooks/useRetryFwAuthenticityChecks.ts(1 hunks)suite-native/device/src/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
🔇 Additional comments (14)
packages/connect/src/api/index.ts (1)
50-50: New API method added correctlyThe
runAuthenticityChecksmethod is properly exported, maintaining the same pattern as other API methods in this file.packages/connect/src/types/api/runAuthenticityChecks.ts (1)
1-4: Type declaration looks goodThe type declaration for
runAuthenticityChecksis correctly defined, taking optional common parameters and returning the appropriate response type.suite-native/device/src/index.ts (1)
7-7: Hook properly exportedThe
useRetryFwAuthenticityCheckshook is correctly exported, making it available for use in the application to implement the retry mechanism for firmware authenticity checks.packages/connect/src/factory.ts (1)
174-175: Method implementation follows established patternThe
runAuthenticityChecksmethod is properly implemented in the factory, following the same pattern as other methods and maintaining consistent spacing.suite-native/app/src/hooks/useGlobalHooks.tsx (1)
5-5: Implementation looks clean!Good integration of the new
useRetryFwAuthenticityCheckshook into the global hooks. The placement after the existing authenticity-related hooks is logical and maintains the code's organization.Also applies to: 25-26
packages/connect/src/types/api/index.ts (1)
69-69: LGTM: Interface properly extendedThe
runAuthenticityChecksmethod is correctly imported and added to the TrezorConnect interface with appropriate typings.Don't forget to add documentation links in the future as noted in the TODO comment.
Also applies to: 299-301
packages/connect/src/api/runAuthenticityChecks.ts (2)
5-7: Verify permission settings with Connect teamBased on the previous comments, there seems to be uncertainty about the correct configuration. The
useEmptyPassphrasesetting is necessary, but you might want to double-check if the current permissions settings are complete.
10-15: Method implementation looks goodThe
runmethod correctly calls the required firmware check methods and returns a success message. The sequence of operations is logical - first checking the firmware hash and then checking the firmware revision.suite-native/device/src/hooks/useRetryFwAuthenticityChecks.ts (2)
14-16: Good error check implementationThe code correctly identifies retriable errors by checking both for non-null error state and membership in the predefined list of retriable errors.
25-28: Clean interval managementThe interval setup and cleanup are correctly implemented. Good use of the dependency array to re-establish the interval only when the
isRetriableErrorstate changes.packages/connect/src/device/Device.ts (4)
786-786: Method visibility changed from private to publicThis change makes
checkFirmwareHashWithRetriesaccessible from outside theDeviceclass, which aligns with the PR objective of implementing a retry mechanism for firmware authenticity checks.
799-803: Immutable state update pattern appliedThe change ensures that the
authenticityChecksobject is updated immutably by creating a new object reference instead of modifying properties directly. This addresses the Redux integration issue mentioned in the past review comments.
883-883: Method visibility changed from private to publicSimilar to the previous change, exposing
checkFirmwareRevisionWithRetriesenables external components to trigger firmware revision checks with retries.
922-925: Immutable state update pattern appliedAs with the earlier change, this ensures the
authenticityChecksobject is updated immutably by creating a new object reference, which prevents issues with Redux in mobile environments as noted in the past review comments.
091cb5e to
08d830c
Compare
08d830c to
739897e
Compare
739897e to
2d58576
Compare
2d58576 to
c69d23f
Compare
Description
Related Issue
Resolve #17133
Screenshots:
Video demonstrating it on the testing branch.
trezordmocked with unrecognized version 9,8,7testing.webm