feat: remove Hash-Check from FW upgrade (it is performed on-device-connect anyway #16949
Conversation
|
🚀 Expo preview is ready!
|
peter-sanderson
force-pushed
the
remove-hash-check-after-fw-upgrade
branch
3 times, most recently
from
5cba0bc to
90240f6
Compare
peter-sanderson
requested review from
mroz22,
szymonlesisz,
martykan and
marekrjpolak
as code owners
WalkthroughThis pull request removes the firmware hash validation logic from multiple parts of the codebase. The changes include deleting assertions in tests that previously checked the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
💤 Files with no reviewable changes (8)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (15)
🔇 Additional comments (5)
✨ 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 (1)
suite-common/firmware/src/firmwareThunks.ts (1)
130-131: LGTM! Consider enhancing error reporting.The removal of hash validation logic simplifies the firmware update process. However, the error reporting for unexpected version changes could be more descriptive.
Consider adding more context to the error report:
- error: 'Unexpected firmware version change during firmware update.', + error: `Unexpected firmware version change during firmware update. Expected: ${releaseVersion}, Got: ${installedVersion}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/connect/src/core/__tests__/onCallFirmwareUpdate.test.ts(0 hunks)packages/connect/src/core/onCallFirmwareUpdate.ts(3 hunks)packages/connect/src/types/api/__tests__/management.ts(0 hunks)packages/connect/src/types/api/firmwareUpdate.ts(1 hunks)packages/suite/src/actions/firmware/__fixtures__/firmwareActions.ts(0 hunks)packages/suite/src/components/firmware/FirmwareUpdateHashCheckError.tsx(0 hunks)packages/suite/src/components/firmware/index.tsx(0 hunks)packages/suite/src/middlewares/suite/analyticsMiddleware.ts(1 hunks)packages/suite/src/middlewares/wallet/storageMiddleware.ts(0 hunks)packages/suite/src/views/firmware/FirmwareModal.tsx(1 hunks)packages/suite/src/views/onboarding/steps/FirmwareStep.tsx(1 hunks)suite-common/firmware/src/firmwareActions.ts(0 hunks)suite-common/firmware/src/firmwareReducer.ts(0 hunks)suite-common/firmware/src/firmwareThunks.ts(1 hunks)
💤 Files with no reviewable changes (8)
- packages/suite/src/components/firmware/index.tsx
- suite-common/firmware/src/firmwareActions.ts
- packages/suite/src/middlewares/wallet/storageMiddleware.ts
- suite-common/firmware/src/firmwareReducer.ts
- packages/suite/src/components/firmware/FirmwareUpdateHashCheckError.tsx
- packages/connect/src/core/tests/onCallFirmwareUpdate.test.ts
- packages/connect/src/types/api/tests/management.ts
- packages/suite/src/actions/firmware/fixtures/firmwareActions.ts
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
🔇 Additional comments (8)
packages/connect/src/core/onCallFirmwareUpdate.ts (4)
12-12: No issues with updated import.The addition of
PROTOto the import statement aligns with the usage in the file.
301-305: Good introduction of type safety for function parameters.Defining
OnCallFirmwareUpdateParamsimproves readability and helps ensure correctness by groupingparams: Paramsandcontext: Contextinto a single object.
309-309: Clearer destructuring of parameters.Switching the function signature to
async ({ params, context }: OnCallFirmwareUpdateParams)clarifies the inputs and aligns with the newly introduced type.
503-503: Confirm firmware integrity approach.Returning only version checks (without hash checks) is consistent with the goal of removing redundancy. Ensure the on-device logic still provides any necessary signature/hash validations to maintain firmware integrity.
packages/connect/src/types/api/firmwareUpdate.ts (1)
19-19: Streamlined response type.Replacing the older union type with
FirmwareUpdateResponsecontaining version comparison fields cleanly aligns with the removal of firmware hash checks.packages/suite/src/views/onboarding/steps/FirmwareStep.tsx (1)
22-22: Removed reference to firmware hash state.Eliminating
firmwareHashInvalidfrom the destructuring call aligns with the overall removal of hash validation logic in the firmware update flow.packages/suite/src/views/firmware/FirmwareModal.tsx (1)
14-14: LGTM! Import cleanup aligns with PR objective.The removal of
FirmwareUpdateHashCheckErrorimport is consistent with moving hash validation to device connection.packages/suite/src/middlewares/suite/analyticsMiddleware.ts (1)
4-4: LGTM! Analytics cleanup aligns with PR objective.The removal of hash-check related imports streamlines the analytics middleware while preserving essential firmware update tracking.
| }); | ||
|
|
||
| if (fwAuto.success) { | ||
| fwAuto.payload.check.toLowerCase(); |
There was a problem hiding this comment.
not important, but what was this code actually supposed to do?
String.toLowerCase() does not mutate, so this code does nothing really... 😄
peter-sanderson
force-pushed
the
remove-hash-check-after-fw-upgrade
branch
from
90240f6 to
fd59256
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
suite-common/firmware/src/firmwareThunks.ts (1)
134-145: Consider improving error reporting for version mismatch.The error reporting for unexpected firmware version changes could be enhanced to provide more context to users.
Consider adding more details to the error report:
reportCheckFail('Firmware version', { model: device.features?.internal_model, revision: device.features?.revision, vendor: device.features?.fw_vendor, bootloaderVersion, binaryVersion, installedVersion, releaseVersion, - error: 'Unexpected firmware version change during firmware update.', + error: `Unexpected firmware version change during firmware update. Expected ${binaryVersion.join('.')} but got ${installedVersion.join('.')}`, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/connect/src/core/__tests__/onCallFirmwareUpdate.test.ts(0 hunks)packages/connect/src/core/onCallFirmwareUpdate.ts(3 hunks)packages/connect/src/types/api/__tests__/management.ts(0 hunks)packages/connect/src/types/api/firmwareUpdate.ts(1 hunks)packages/suite/src/actions/firmware/__fixtures__/firmwareActions.ts(0 hunks)packages/suite/src/components/firmware/FirmwareUpdateHashCheckError.tsx(0 hunks)packages/suite/src/components/firmware/index.tsx(0 hunks)packages/suite/src/middlewares/suite/analyticsMiddleware.ts(1 hunks)packages/suite/src/middlewares/wallet/storageMiddleware.ts(0 hunks)packages/suite/src/views/firmware/FirmwareModal.tsx(1 hunks)packages/suite/src/views/onboarding/steps/FirmwareStep.tsx(1 hunks)suite-common/firmware/src/firmwareActions.ts(0 hunks)suite-common/firmware/src/firmwareReducer.ts(0 hunks)suite-common/firmware/src/firmwareThunks.ts(1 hunks)
💤 Files with no reviewable changes (8)
- packages/suite/src/components/firmware/index.tsx
- suite-common/firmware/src/firmwareActions.ts
- packages/suite/src/middlewares/wallet/storageMiddleware.ts
- packages/suite/src/actions/firmware/fixtures/firmwareActions.ts
- packages/suite/src/components/firmware/FirmwareUpdateHashCheckError.tsx
- packages/connect/src/core/tests/onCallFirmwareUpdate.test.ts
- suite-common/firmware/src/firmwareReducer.ts
- packages/connect/src/types/api/tests/management.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/suite/src/views/onboarding/steps/FirmwareStep.tsx
- packages/suite/src/views/firmware/FirmwareModal.tsx
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: build-deploy
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: EAS Update
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (5)
packages/connect/src/types/api/firmwareUpdate.ts (1)
19-25: LGTM! Type definition aligns with firmware hash validation removal.The simplified
FirmwareUpdateResponsetype now correctly focuses on version-related properties, removing unnecessary hash validation fields. This change aligns with the PR objective of removing hash checks from the firmware upgrade process.suite-common/firmware/src/firmwareThunks.ts (1)
130-131: LGTM! Simplified firmware update status handling.The code now directly sets the status to 'done' after firmware update, removing the intermediate hash validation steps. This aligns with the PR objective as hash validation is now performed during device connection.
packages/suite/src/middlewares/suite/analyticsMiddleware.ts (1)
4-4: LGTM! Simplified firmware update analytics.The removal of hash validation related imports and error reporting aligns with the PR objective. The analytics now correctly focuses on tracking firmware update success/failure without hash validation events.
packages/connect/src/core/onCallFirmwareUpdate.ts (2)
301-304: LGTM! Improved function parameter organization.The new
OnCallFirmwareUpdateParamstype provides better organization of function parameters by grouping them into a single object.
503-509: LGTM! Simplified firmware update response.The response now correctly focuses on version validation without hash checks, aligning with the PR objective. The version checks ensure that:
- The installed version matches the binary version
- The installed version matches the requested release version (if applicable)
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13309616056 |
trezor-ci
force-pushed
the
remove-hash-check-after-fw-upgrade
branch
from
fd59256 to
79db32a
Compare
Resolves: https://github.com/trezor/trezor-suite-private/issues/129