Feat/suite handle fw update check #16477
Conversation
|
🚀 Expo preview is ready!
|
| @@ -67,6 +67,10 @@ const eventsMiddleware = | |||
| } | |||
| } | |||
|
|
|||
| if (action.type === DEVICE.FIRMWARE_VERSION_CHANGED) { | |||
| // TODO: show warning? move this to different middleware? | |||
There was a problem hiding this comment.
TODO log to Sentry (I can do it).
There was a problem hiding this comment.
Example log in Sentry
Mocked by changing this line.
| @@ -165,6 +165,8 @@ export const firmwareUpdate = createThunk< | |||
| errorMessage: firmwareUpdateResponse.payload.checkError, | |||
| }), | |||
| ); | |||
| } else if (!binary && !versionCheck) { | |||
| // TODO: log to sentry | |||
There was a problem hiding this comment.
Example log in Sentry 2
Mocked by changing installedVersion here.
| @@ -29,6 +29,16 @@ const deviceDisconnect = createAction(DEVICE.DISCONNECT, (payload: TrezorDevice) | |||
| payload, | |||
| })); | |||
|
|
|||
| // this action is not used but is required because of the typings | |||
There was a problem hiding this comment.
This is awkward... how should it be resolved, @peter-sanderson? I see you removed DeviceEvent here
There was a problem hiding this comment.
I don't remember but I would guess it has to do something with this:
c02412b#diff-2ec00308835c01545c383eaef7034f26bef251f7e04e990e21aac46d0afb2c8bR71
As I read it, I think that idea was to define actions independently from Connect Event and map them instead of using them directly.
There was a problem hiding this comment.
I think this is the place where mapping was intended to happen: c02412b#diff-2e9d057f0bfe2cc92fe50d4ce28838622d9e79fcca010ab8847a0fa288da13fdR38
There was a problem hiding this comment.
Ultimately the mapping is here: c02412b#diff-2580f7331a6878d62ed30bb9de2c33adbf4566b5d67d8cf33f5705f48af51930R630
There was a problem hiding this comment.
mapping is done only for 2 actions however the rest of DEVICE_EVENTs are dispatched directly from here not from the deviceActions, yet its required to be defined in deviceActions to satisfy the types.
this problem applies to any other DEVICE_EVENTs action.type like DEVICE.DISCONNECT or DEVICE.PASSPHRASE or DEVICE.PIN
There was a problem hiding this comment.
I am looking into it. There is definitely a problem there.
Here it is typed TrezorConnect.on(DEVICE_EVENT, .... as DeviceEventMessage which contains only those types. I assume this is correct.
I have tried to play with it in this commit 29aaaf4 and this is a draft of how I think it shall be done.
But I am not sure how to make types work properly and also I need to run now.
Lets talk tomorrow.
There was a problem hiding this comment.
what is the benefit of doing things like this?
what i see is 1000% more lines of code, more complexity and added another todo's which will be forgotten until next the occurrence + one potential problem if someone add another DEVICE_EVENT to connect (throwing error)
There was a problem hiding this comment.
The idea is to be explicit and to unify syntax across the codebase. However, the current implementation is not finished and having to create actions that are not dispatched is indeed counter-intuitive. @peter-sanderson will try to finish the work he started in his branch - should he not succeed, I'll broaden the type again to remove the counter-intuitive behaviour.
There was a problem hiding this comment.
so in the future you want to duplicate all the definitions of the events emitted from TrezorConnect?
like for example 40+ ui-events? and create this looong if or switch to dispatch a proper action?
because this is how i undestand this comment:
TrezorConnectEvents // Todo: This should not be here, actions shall be defined independently from Connect Events (and they shall be mapped onto them)
instead of 4-6 lines of code (current state) there will be like 200-300 in multiple files?
There was a problem hiding this comment.
I agree this is too many events to map.
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13030999975 |
|
Rebasing failed, please rebase manually. |
szymonlesisz
force-pushed
the
feat/suite-handle-fw-update-check
branch
from
c6bf766 to
160d0c1
Compare
|
rebased, forcepushed, only one commit is relevant (implementation in suite) |
| @@ -67,6 +67,10 @@ const eventsMiddleware = | |||
| } | |||
| } | |||
|
|
|||
| if (action.type === DEVICE.FIRMWARE_VERSION_CHANGED) { | |||
There was a problem hiding this comment.
Despite this is working I think it shall be:
if (deviceActions.deviceFirmwareVersionChanged.match(action)) {(More context coming in other comments).
komret
force-pushed
the
feat/suite-handle-fw-update-check
branch
from
160d0c1 to
b99495e
Compare
WalkthroughThe changes span multiple parts of the codebase, focusing on updating firmware and device-related actions, payload structures, and error reporting. In the firmware actions file, the response payload was altered by removing the properties Suggested labels
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
suite-common/wallet-core/src/device/deviceActions.ts (1)
32-40: Consider removing the unused action creator.The comment indicates this action creator is not used directly but is required for typings. This creates confusion and technical debt. Consider:
- Using the action directly instead of dispatching from connectInitThunk
- Or updating the types to avoid requiring unused action creators
packages/suite/src/middlewares/suite/eventsMiddleware.ts (1)
71-82: Improve action type checking using action matcher.For consistency with other handlers in this file, use the action matcher pattern:
-if (action.type === DEVICE.FIRMWARE_VERSION_CHANGED) { +if (deviceActions.deviceFirmwareVersionChanged.match(action)) {Address the TODO comment about UI.
The TODO comment about adding UI should be tracked properly.
Would you like me to create an issue to track the UI implementation task?
suite-common/redux-utils/src/extraDependenciesType.ts (1)
141-145: Consider using an enum for checkType.The implementation looks good. However, to improve type safety and maintainability, consider using an enum for the
checkTypeparameter instead of string literals.+export enum CheckType { + Entropy = 'Entropy', + FirmwareHash = 'Firmware hash', + FirmwareRevision = 'Firmware revision', + FirmwareVersion = 'Firmware version', +} -reportCheckFail: ( - checkType: 'Entropy' | 'Firmware hash' | 'Firmware revision' | 'Firmware version', - contextData: Record<string, any>, - errorPayload?: unknown, -) => void; +reportCheckFail: ( + checkType: CheckType, + contextData: Record<string, any>, + errorPayload?: unknown, -) => void;suite-common/firmware/src/firmwareThunks.ts (1)
181-192: Consider improving error handling for version check failures.The version check implementation looks good, but consider:
- Adding a user-friendly error message in the UI.
- Providing guidance on how to resolve version mismatches.
- Adding retry logic for transient failures.
Consider implementing a dedicated error handling service that can:
- Display user-friendly error messages
- Provide troubleshooting steps
- Track and report error metrics
- Handle retries with backoff
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/suite/src/actions/firmware/__fixtures__/firmwareActions.ts(1 hunks)packages/suite/src/actions/settings/deviceSettingsActions.ts(1 hunks)packages/suite/src/components/suite/SecurityCheck/useReportDeviceCompromised.ts(1 hunks)packages/suite/src/middlewares/suite/eventsMiddleware.ts(2 hunks)packages/suite/src/support/extraDependencies.ts(2 hunks)suite-common/firmware/src/firmwareThunks.ts(3 hunks)suite-common/redux-utils/src/extraDependenciesType.ts(1 hunks)suite-common/test-utils/src/extraDependenciesMock.ts(1 hunks)suite-common/wallet-core/src/device/deviceActions.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/suite/src/actions/settings/deviceSettingsActions.ts
🔇 Additional comments (5)
packages/suite/src/components/suite/SecurityCheck/useReportDeviceCompromised.ts (1)
13-14: LGTM! The addition of 'Firmware version' check type is well-integrated.The change maintains consistency with existing check types and aligns with the PR's objective.
suite-common/test-utils/src/extraDependenciesMock.ts (1)
149-150: LGTM! The mock implementation is consistent with the codebase patterns.The warning message clearly indicates the mock nature of the implementation.
suite-common/firmware/src/firmwareThunks.ts (1)
180-180: TODO comment needs clarification.The TODO comment "Add to the if-else block above and add handle in UI" is vague. Please clarify:
- What specific UI handling is needed?
- How should this be integrated into the existing if-else block?
packages/suite/src/support/extraDependencies.ts (1)
35-35: LGTM!The integration of
reportCheckFailinto the extra dependencies is clean and follows the existing pattern.Also applies to: 219-219
packages/suite/src/actions/firmware/__fixtures__/firmwareActions.ts (1)
29-31: Verify test coverage for version check scenarios.The fixture updates look good but ensure test coverage for:
- Failed version checks (
versionCheck: false)- Different
checkvalues ('mismatch', 'other-error')- Error reporting through
reportCheckFailRun this script to analyze test coverage:
✅ Verification successful
Firmware Version Check Test Coverage Confirmed
- Failed Version Checks: The test case in
packages/connect/src/core/__tests__/onCallFirmwareUpdate.test.tsverifies scenarios whereversionCheckisfalse.- Different
checkValues: Test cases cover both'mismatch'and'other-error'scenarios, as evidenced by tests and implementation inpackages/connect/src/core/__tests__/onCallFirmwareUpdate.test.tsandpackages/connect/src/core/onCallFirmwareUpdate.ts.- Error Reporting (
reportCheckFail): Usage ofreportCheckFailwith the'Firmware version'context is verified in bothpackages/suite/src/middlewares/suite/eventsMiddleware.tsandsuite-common/firmware/src/firmwareThunks.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for firmware version checks # Search for test cases covering version check scenarios rg -A 5 "versionCheck.*false" --type ts rg -A 5 "check.*mismatch" --type ts rg -A 5 "check.*other-error" --type ts rg -A 5 "reportCheckFail.*Firmware version" --type tsLength of output: 7369
komret
force-pushed
the
feat/suite-handle-fw-update-check
branch
from
b99495e to
8e155a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite/src/middlewares/suite/eventsMiddleware.ts (1)
71-71: 🛠️ Refactor suggestionUse action matcher pattern for consistency.
For consistency with other handlers in this file (e.g., lines 84, 100, 124), use the action matcher pattern.
-if (action.type === DEVICE.FIRMWARE_VERSION_CHANGED) { +if (deviceActions.deviceFirmwareVersionChanged.match(action)) {
🧹 Nitpick comments (2)
packages/suite/src/middlewares/suite/eventsMiddleware.ts (2)
72-72: Enhance the TODO comment.The TODO comment should be more descriptive about what UI needs to be added and why.
- // TODO: Add UI. + // TODO: Add UI notification to inform the user about unexpected firmware version change.
73-82: Improve device access safety and error message clarity.The code could be more defensive when accessing device properties and provide a more informative error message.
- const { device, oldVersion, newVersion } = action.payload; - reportCheckFail('Firmware version', { - model: device?.features?.internal_model, - revision: device?.features?.revision, - oldVersion, - newVersion, - vendor: device?.features?.fw_vendor, - error: 'Firmware version changed unexpectedly.', - }); + const { device, oldVersion, newVersion } = action.payload; + if (!device?.features) { + return; + } + reportCheckFail('Firmware version', { + model: device.features.internal_model, + revision: device.features.revision, + oldVersion, + newVersion, + vendor: device.features.fw_vendor, + error: `Unexpected firmware version change from ${oldVersion} to ${newVersion}.`, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/suite/src/actions/firmware/__fixtures__/firmwareActions.ts(1 hunks)packages/suite/src/actions/settings/deviceSettingsActions.ts(1 hunks)packages/suite/src/components/suite/SecurityCheck/useReportDeviceCompromised.ts(1 hunks)packages/suite/src/middlewares/suite/eventsMiddleware.ts(2 hunks)packages/suite/src/support/extraDependencies.ts(2 hunks)suite-common/firmware/src/firmwareThunks.ts(3 hunks)suite-common/redux-utils/src/extraDependenciesType.ts(1 hunks)suite-common/test-utils/src/extraDependenciesMock.ts(1 hunks)suite-common/wallet-core/src/device/deviceActions.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/suite/src/actions/firmware/fixtures/firmwareActions.ts
- suite-common/redux-utils/src/extraDependenciesType.ts
- packages/suite/src/actions/settings/deviceSettingsActions.ts
- suite-common/test-utils/src/extraDependenciesMock.ts
- suite-common/firmware/src/firmwareThunks.ts
- packages/suite/src/components/suite/SecurityCheck/useReportDeviceCompromised.ts
- suite-common/wallet-core/src/device/deviceActions.ts
- packages/suite/src/support/extraDependencies.ts
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: build-web
🔇 Additional comments (1)
packages/suite/src/middlewares/suite/eventsMiddleware.ts (1)
19-19: LGTM!The import statement is correctly placed and follows the project's conventions.
| @@ -216,5 +217,6 @@ export const extraDependencies: ExtraDependencies = { | |||
| utils: { | |||
| saveAs: (data, fileName) => saveAs(data, fileName), | |||
| connectInitSettings, | |||
| reportCheckFail, | |||
There was a problem hiding this comment.
wouldn't be better to add generic report(Sentry)Error dependency rather then specific reporting?
There was a problem hiding this comment.
I can imagine to move whole reportCheckFail to common, and use it both in common and suite then. But inject the generic error handler (for sentry).
This way we can utilise error handling in any share code.
There was a problem hiding this comment.
(can be done in future PR, but please lets do it).
There was a problem hiding this comment.
I agree, I did it this way because it was minimal effort.
Description
WIP Based on #16421
implementation of
firmwareUpdateversionCheck and TrezorConnectUI.FIRMWARE_VERSION_CHANGEDevent