Refactor: Use new protobuf to compatible with different versions of protocols #458
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update removes the old firmware submodule and adds a new onekey-protocol submodule. It refactors device info extraction, unifies feature handling, and updates protobuf message schemas and types. Several classes now use a new base for firmware updates, and device communication uses new message types and commands. Many type definitions are expanded or renamed for clarity and consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Device
participant FirmwareUpdateBaseMethod
participant TransportManager
App->>Device: StartSession({ ok_dev_info_req })
Device->>Device: fixFeaturesBetweenProtocol()
Device->>TransportManager: reconfigure(features)
Device-->>App: features
App->>FirmwareUpdateBaseMethod: enterBootloaderMode()
FirmwareUpdateBaseMethod->>Device: reboot(BootLoader)
Device-->>FirmwareUpdateBaseMethod: Success/Error (logs error, continues)
FirmwareUpdateBaseMethod->>Device: check bootloader status
FirmwareUpdateBaseMethod->>TransportManager: clear device cache
FirmwareUpdateBaseMethod-->>App: Bootloader mode entered
App->>FirmwareUpdateBaseMethod: reboot(OneKeyRebootType)
FirmwareUpdateBaseMethod->>Device: typedCall (command depends on message version)
Device-->>FirmwareUpdateBaseMethod: Success/Error (logs error, continues)
sequenceDiagram
participant App
participant Device
participant onekeyInfoUtils
App->>Device: getFeatures()
Device->>Device: StartSession({ ok_dev_info_req: { factory, normal } })
Device->>Device: fixFeaturesBetweenProtocol()
Device->>onekeyInfoUtils: getHardwareInfoFromFeatures()
Device->>onekeyInfoUtils: getFirmwareInfoFromFeatures()
Device->>onekeyInfoUtils: getSeInfoFromFeatures()
Device-->>App: features (normalized)
sequenceDiagram
participant App
participant DataManager
App->>DataManager: get messages for version
DataManager-->>App: messages (latest, v1, or v2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
wabicai
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
🔭 Outside diff range comments (5)
packages/core/src/types/api/cardano.ts (3)
141-162: 🛠️ Refactor suggestionNaming inconsistency needs attention
While you updated the
formatproperty type, other related names remain inconsistent. The interface is still calledCardanoCVoteRegistrationParametersand usesCardanoCVoteRegistrationDelegation. These should be renamed to match the newGovernanceprefix pattern.-export interface CardanoCVoteRegistrationDelegation { +export interface CardanoGovernanceRegistrationDelegation { votePublicKey: string; weight: number; } -export interface CardanoCVoteRegistrationParameters { +export interface CardanoGovernanceRegistrationParameters { votePublicKey?: string; stakingPath: string | number[]; paymentAddressParameters: CardanoAddressParameters; nonce: string; format?: PROTO.CardanoGovernanceRegistrationFormat; - delegations?: CardanoCVoteRegistrationDelegation[]; + delegations?: CardanoGovernanceRegistrationDelegation[]; votingPurpose?: number; paymentAddress?: string; }
164-167: 🛠️ Refactor suggestionAdditional naming update needed
Update this interface to use the new governance naming pattern.
export interface CardanoAuxiliaryData { hash?: string; - cVoteRegistrationParameters?: CardanoCVoteRegistrationParameters; + governanceRegistrationParameters?: CardanoGovernanceRegistrationParameters; }
202-206: 🛠️ Refactor suggestionRename field to match naming pattern
Update
cVoteRegistrationSignaturetogovernanceRegistrationSignaturefor consistency.export interface CardanoAuxiliaryDataSupplement { type: PROTO.CardanoTxAuxiliaryDataSupplementType; auxiliaryDataHash: string; - cVoteRegistrationSignature?: string; + governanceRegistrationSignature?: string; }packages/core/src/device/Device.ts (1)
360-384: 🧹 Nitpick (assertive)Swap
console.errorfor the project loggerRaw
console.errorcalls pollute stdout and can hurt performance on mobile. UseLog.debug/Log.error(already imported) or remove the statement.-console.error('======> StartSession', options); +Log.debug('StartSession', options);packages/core/src/api/FirmwareUpdateV2.ts (1)
126-166:⚠️ Potential issueUndeclared methods – compile will fail
_promptDeviceInBootloaderForWebDeviceand_checkDeviceInBootloaderModeno longer exist after the refactor toFirmwareUpdateBaseMethod.
Calls here will break the build.- const confirmed = await this._promptDeviceInBootloaderForWebDevice(); + const confirmed = await this.promptDeviceInBootloaderForWebDevice(); ... - await this._checkDeviceInBootloaderMode(connectId, intervalTimer, timeoutTimer); + await this.checkDeviceInBootloaderMode(connectId, intervalTimer, timeoutTimer);Do the same replacement for the call in the
elsebranch below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (43)
.gitmodules(1 hunks)packages/connect-examples/expo-example/src/components/BaseTestRunner/Context/TestRunnerProvider.tsx(3 hunks)packages/connect-examples/expo-example/src/components/BaseTestRunner/useRunnerTest.ts(0 hunks)packages/connect-examples/expo-example/src/utils/deviceUtils.ts(4 hunks)packages/connect-examples/expo-example/src/views/FirmwareScreen/DeviceField.tsx(3 hunks)packages/connect-examples/expo-example/src/views/FirmwareScreen/ExportDeviceInfo.tsx(6 hunks)packages/connect-examples/expo-example/src/views/FirmwareScreen/index.tsx(2 hunks)packages/core/src/api/FirmwareUpdateV2.ts(7 hunks)packages/core/src/api/FirmwareUpdateV3.ts(3 hunks)packages/core/src/api/GetOnekeyFeatures.ts(2 hunks)packages/core/src/api/device/DeviceRebootToBoardloader.ts(2 hunks)packages/core/src/api/device/DeviceRebootToBootloader.ts(2 hunks)packages/core/src/api/device/DeviceUpdateBootloader.ts(1 hunks)packages/core/src/api/device/DeviceUpdateReboot.ts(1 hunks)packages/core/src/api/firmware/FirmwareUpdateBaseMethod.ts(5 hunks)packages/core/src/api/firmware/bootloaderHelper.ts(2 hunks)packages/core/src/api/firmware/uploadFirmware.ts(2 hunks)packages/core/src/api/test/TestInitializeDeviceDuration.ts(2 hunks)packages/core/src/data-manager/DataManager.ts(3 hunks)packages/core/src/data-manager/MessagesConfig.ts(1 hunks)packages/core/src/data-manager/TransportManager.ts(2 hunks)packages/core/src/data/messages/messages_legacy_v1.json(2 hunks)packages/core/src/device/Device.ts(6 hunks)packages/core/src/device/utils.ts(1 hunks)packages/core/src/types/api/cardano.ts(1 hunks)packages/core/src/types/api/deviceRebootToBoardloader.ts(1 hunks)packages/core/src/types/api/deviceRecovery.ts(2 hunks)packages/core/src/types/device.ts(1 hunks)packages/core/src/utils/deviceFeaturesUtils.ts(3 hunks)packages/core/src/utils/deviceInfoUtils.ts(3 hunks)packages/core/src/utils/deviceVersionUtils.ts(2 hunks)packages/core/src/utils/findDefectiveBatchDevice.ts(1 hunks)packages/core/src/utils/index.ts(1 hunks)packages/core/src/utils/onekeyInfoUtils.ts(1 hunks)packages/hd-transport-http/src/index.ts(2 hunks)packages/hd-transport/__tests__/encode-decode.test.js(1 hunks)packages/hd-transport/__tests__/messages.test.js(3 hunks)packages/hd-transport/scripts/protobuf-build.sh(3 hunks)packages/hd-transport/scripts/protobuf-types.js(1 hunks)packages/hd-transport/src/serialization/protobuf/decode.ts(2 hunks)packages/hd-transport/src/types/messages.ts(75 hunks)submodules/firmware(0 hunks)submodules/onekey-protocol(1 hunks)
💤 Files with no reviewable changes (2)
- packages/connect-examples/expo-example/src/components/BaseTestRunner/useRunnerTest.ts
- submodules/firmware
🧰 Additional context used
🧬 Code Graph Analysis (13)
packages/hd-transport-http/src/index.ts (3)
packages/hd-transport/src/serialization/send.ts (1)
buildOne(13-22)packages/shared/src/HardwareError.ts (1)
HardwareErrorCode(46-402)packages/hd-transport/src/serialization/receive.ts (1)
receiveOne(8-18)
packages/core/src/api/firmware/bootloaderHelper.ts (2)
packages/core/src/utils/deviceVersionUtils.ts (1)
getDeviceBoardloaderVersion(52-58)packages/core/src/utils/index.ts (1)
getDeviceBoardloaderVersion(15-15)
packages/core/src/types/device.ts (1)
packages/hd-transport/src/types/messages.ts (1)
Features(2406-2463)
packages/core/src/api/GetOnekeyFeatures.ts (4)
packages/core/src/utils/deviceFeaturesUtils.ts (1)
getSupportMessageVersion(14-56)packages/core/src/types/device.ts (1)
Features(79-79)packages/hd-transport/src/types/messages.ts (2)
Features(2406-2463)MessageKey(5028-5028)packages/core/src/device/utils.ts (1)
cherryPickFeaturesParams(4-25)
packages/hd-transport/scripts/protobuf-build.sh (2)
packages/hd-transport/__tests__/messages.test.js (1)
json(8-58)packages/hd-transport/scripts/protobuf-types.js (1)
json(9-9)
packages/core/src/utils/findDefectiveBatchDevice.ts (3)
packages/core/src/types/device.ts (1)
Features(79-79)packages/hd-transport/src/types/messages.ts (1)
Features(2406-2463)packages/core/src/utils/onekeyInfoUtils.ts (2)
getHardwareInfoFromFeatures(148-169)getSeInfoFromFeatures(171-280)
packages/core/src/utils/deviceFeaturesUtils.ts (1)
packages/core/src/data-manager/DataManager.ts (1)
DataManager(33-357)
packages/connect-examples/expo-example/src/views/FirmwareScreen/DeviceField.tsx (2)
packages/connect-examples/expo-example/src/views/FirmwareScreen/DeviceFieldContext.ts (1)
useDeviceFieldContext(17-19)packages/connect-examples/expo-example/src/provider/MediaProvider.tsx (1)
useMedia(20-20)
packages/connect-examples/expo-example/src/views/FirmwareScreen/index.tsx (3)
packages/connect-examples/expo-example/src/utils/deviceUtils.ts (1)
getDeviceBasicInfo(50-99)packages/hd-transport/src/types/messages.ts (1)
Features(2406-2463)packages/connect-examples/expo-example/src/views/FirmwareScreen/DeviceField.tsx (1)
DeviceField(74-77)
packages/core/src/device/utils.ts (2)
packages/core/src/device/Device.ts (1)
IFeaturesType(55-58)packages/hd-transport/src/types/messages.ts (2)
OneKeyInfoTargets(3612-3621)OneKeyInfoTypes(3624-3629)
packages/core/src/utils/deviceInfoUtils.ts (3)
packages/core/src/utils/onekeyInfoUtils.ts (2)
getHardwareInfoFromFeatures(148-169)getFirmwareInfoFromFeatures(69-147)packages/core/src/types/device.ts (1)
Features(79-79)packages/hd-transport/src/types/messages.ts (1)
Features(2406-2463)
packages/core/src/api/device/DeviceUpdateReboot.ts (3)
packages/core/src/api/firmware/FirmwareUpdateBaseMethod.ts (1)
FirmwareUpdateBaseMethod(32-451)packages/hd-transport/src/types/messages.ts (1)
DeviceBackToBoot(827-827)packages/core/src/events/ui-request.ts (1)
UI_REQUEST(8-44)
packages/connect-examples/expo-example/src/utils/deviceUtils.ts (3)
packages/core/src/types/device.ts (1)
Features(79-79)packages/hd-transport/src/types/messages.ts (1)
Features(2406-2463)packages/core/src/utils/onekeyInfoUtils.ts (3)
getFirmwareInfoFromFeatures(69-147)getHardwareInfoFromFeatures(148-169)getSeInfoFromFeatures(171-280)
🪛 Biome (1.9.4)
packages/core/src/data-manager/TransportManager.ts
[error] 62-62: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
packages/connect-examples/expo-example/src/views/FirmwareScreen/DeviceField.tsx
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/hd-transport/src/types/messages.ts
[error] 143-143: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 245-245: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 788-788: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 824-824: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 827-827: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 830-830: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 887-887: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 1586-1586: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 1589-1589: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 2466-2466: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 2695-2695: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 2714-2714: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 2717-2717: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 2870-2870: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 3119-3119: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 3179-3179: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 4388-4388: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🪛 Shellcheck (0.10.0)
packages/hd-transport/scripts/protobuf-build.sh
[info] 46-46: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 47-47: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 50-50: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 58-58: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 70-70: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 71-71: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (64)
packages/core/src/types/api/cardano.ts (1)
158-158: Type update looks goodThe change from
PROTO.CardanoCVoteRegistrationFormattoPROTO.CardanoGovernanceRegistrationFormataligns with the broader renaming of Cardano governance types mentioned in the PR summary.submodules/onekey-protocol (1)
1-1: Pin onekey-protocol submodule to correct commit
The subproject file now references commit 8f7a537fc0950866d43da08d4fca76f4635ccdd3, matching the updated.gitmodules.packages/core/src/api/device/DeviceUpdateBootloader.ts (1)
9-9: Imports reduced to only what's needed.Good job simplifying the import statement to include only the necessary
checkBootloaderLengthfunction. This improves code clarity and reduces bundle size.packages/hd-transport/scripts/protobuf-types.js (1)
24-24: Support for protobuf float type added.Adding the
floattonumbermapping completes the protobuf primitive type conversion set. This enables proper handling of floating-point values in protobuf definitions.packages/hd-transport/__tests__/encode-decode.test.js (1)
94-97: Message name updated from "Initialize" to "StartSession".The fixture update aligns with the protocol-level message rename. This change maintains consistency across the codebase.
packages/hd-transport/__tests__/messages.test.js (3)
10-10: Protocol message renamed from "Initialize" to "StartSession"The message schema has been renamed to improve clarity about its purpose - starting a session rather than general initialization.
54-54: MessageType enum value updated to match renamed messageConsistent renaming of the enum value to match the message type name change.
63-63: Test case updated to use new message nameTest case properly updated to use the new message name.
packages/core/src/types/api/deviceRecovery.ts (2)
1-1: Import updated to use the new RecoveryTypeImport correctly updated to use the new type name.
11-11: Type definition updated to match imported typeParameter type properly updated to use RecoveryType.
packages/core/src/utils/index.ts (1)
38-42:✅ Verification successful
New utility functions exported for device info extraction
Good addition of utility functions that centralize device feature extraction.
🏁 Script executed:
#!/bin/bash # Check if the onekeyInfoUtils file exists cat packages/core/src/utils/onekeyInfoUtils.ts | head -20Length of output: 576
🏁 Script executed:
#!/bin/bash set -e file="packages/core/src/utils/onekeyInfoUtils.ts" echo "Searching for utility function definitions in $file" grep -n "getHardwareInfoFromFeatures" $file || echo "getHardwareInfoFromFeatures not found" grep -n "getFirmwareInfoFromFeatures" $file || echo "getFirmwareInfoFromFeatures not found" grep -n "getSeInfoFromFeatures" $file || echo "getSeInfoFromFeatures not found"Length of output: 872
Verification Complete – Exports Match Definitions
All three utilities—getHardwareInfoFromFeatures, getFirmwareInfoFromFeatures, and getSeInfoFromFeatures—are defined in onekeyInfoUtils.ts and correctly re-exported in utils/index.ts. No further action needed.packages/hd-transport-http/src/index.ts (1)
121-121: Protocol update from 'Initialize' to 'StartSession'.The timeout condition correctly reflects the protocol update where 'Initialize' was renamed to 'StartSession'.
packages/core/src/api/test/TestInitializeDeviceDuration.ts (1)
18-18: Protocol update from 'Initialize' to 'StartSession'.The command name update is correct, and the empty object parameter matches the new API signature.
packages/core/src/data-manager/MessagesConfig.ts (1)
13-13: Explicit version labeling improves clarity.Changing from 'latest' to 'v2' makes the version requirements more explicit and less ambiguous.
Also applies to: 17-17
packages/core/src/data/messages/messages_legacy_v1.json (2)
4831-4849: Great rename ofInitializetoStartSession.
You’ve preserved the original numeric ID (0) and deprecated the old field. The structure is concise and clear.
9235-9236:✅ Verification successful
Verify no leftover
Initializereferences.
Ensure all code and tests now useStartSession. Run this script to catch any stray mentions:
🏁 Script executed:
#!/bin/bash # Find any remaining "Initialize" in the repo rg -n '"Initialize\b"'Length of output: 24
No leftover “Initialize” references found
RG returned no matches. All code and tests now useStartSession.packages/core/src/data-manager/DataManager.ts (3)
6-6: Import added for the new v2 legacy messages.This import brings in the new protobuf message definitions needed for the v2 protocol support.
31-31: Type definition updated to include the new v2 message version.The MessageVersion type now correctly includes 'v2' alongside existing options.
68-68: Added v2 message version to the static messages dictionary.Good implementation - follows the existing pattern for message version support.
packages/hd-transport/src/serialization/protobuf/decode.ts (4)
48-49: Improved type safety for enum fields.Added explicit property check before accessing
valuesByIdonfield.resolvedType.
50-51: Enhanced type check for message fields.The code now verifies that
field.resolvedTypeexists and has a 'fields' property before attempting to access it.
59-60: Better type checking for enum resolution.Similar to the repeated fields section, this improves safety when handling enum types.
63-66: Added comprehensive type checks and null handling.This change improves error prevention by:
- Checking for the existence of
resolvedTypeand itsfieldsproperty- Adding explicit handling for null resolved types
Good defensive programming that prevents potential runtime errors.
packages/core/src/api/firmware/uploadFirmware.ts (2)
27-28: Added imports for new feature parameter utilities.These utility functions support the updated device session initialization.
245-247: Replaced race pattern with direct StartSession call.This change:
- Replaces the
Initializecommand with the newStartSessioncommand- Removes the timeout-based race pattern
- Explicitly requests specific features with
cherryPickFeaturesParamsThe code is now cleaner and uses the modern protocol command.
packages/core/src/utils/findDefectiveBatchDevice.ts (3)
2-2: Updated imports to use the new utility functions.Good move to import the centralized feature extraction utilities.
6-7: Refactored to use utility functions instead of direct property access.Now uses
getHardwareInfoFromFeaturesandgetSeInfoFromFeaturesto extract device data. This approach better handles differences between protocol versions.
11-11: Updated condition to use extracted SE version property.Now references
se01Versionfrom the utility function rather than directly accessing feature properties.packages/core/src/api/firmware/bootloaderHelper.ts (1)
4-4: Good move: centralizing version parsingSwitching to
getDeviceBoardloaderVersionandgetDeviceTypekeeps all version/feature parsing logic in one place, which cuts duplication and future‐proofs maintenance.packages/core/src/api/device/DeviceRebootToBootloader.ts (3)
1-2: Import updates align with new class inheritanceThe imports now include
OneKeyRebootTypefor specifying reboot types andFirmwareUpdateBaseMethodas the new parent class.
5-5: Class now extends FirmwareUpdateBaseMethodThe class inheritance change unifies reboot handling with other firmware update methods, improving consistency.
23-23: Standardized reboot method callUsing the base class's
rebootmethod withOneKeyRebootType.BootLoadercentralizes reboot logic and improves maintainability.packages/hd-transport/scripts/protobuf-build.sh (3)
9-10: Updated source path to use onekey-protocolThe source directory correctly points to the new submodule path.
33-42: Added optimization to reuse existing messages.jsonSmart performance improvement that checks if a pre-built messages.json already exists in the submodule.
82-82: Added completion messageHelpful confirmation for users that the build process finished.
packages/core/src/utils/deviceFeaturesUtils.ts (3)
23-28: Added shortcut for new protocol version detectionSmart optimization that returns the latest message version immediately when the device supports the new protocol.
231-232: Enhanced version fallback handlingThe
fixVersionfunction now safely handles undefined inputs by returning a default version.
257-259: Consistent version normalization for onekey_versionSame fix applied to the onekey_version field for complete consistency.
packages/connect-examples/expo-example/src/views/FirmwareScreen/DeviceField.tsx (3)
13-17: Strengthened empty value detectionThe
isNilfunction now checks both null and undefined values, and safely callstrim()only when available.
24-24: Safer type casting for features accessUsing
unknownas an intermediate type offers better type safety when accessing dynamic feature properties.
58-58: Added null fallback for text renderingUsing
fieldValue ?? ''ensures we always render a valid string, preventing rendering issues.packages/connect-examples/expo-example/src/views/FirmwareScreen/ExportDeviceInfo.tsx (8)
2-2: Great addition of explicit type import.Adding the Features type import improves type safety throughout the component.
8-9: Uncommented first array entry properly.Fixed the previously commented-out device_id and label fields in deviceInfoKeys.
28-62: Good expansion of device SE info keys.Adding the type fields (onekey_se0X_type) to each SE device array improves the completeness of exported device information.
82-82: Simplified device context usage.Removing reliance on separate onekeyFeatures object streamlines the code.
84-84: Improved type safety with keyof operator.The getFieldValue function now accepts keys strictly of type keyof Features, preventing potential runtime errors.
96-96: Simplified API call.Updated getDeviceBasicInfo call to only use features parameter, aligning with the unified feature handling approach.
134-135: Added necessary type casting.Proper type casting ensures type safety when accessing dynamic properties from the features object.
147-148: Consistent type casting approach.Maintaining the same type casting pattern for SE info keys as used for device info keys.
packages/core/src/device/utils.ts (2)
1-3: Good imports for feature parameter building.Importing the necessary types for feature parameter cherry-picking.
4-25:✅ Verification successful
Well-structured feature parameter builder.
The cherryPickFeaturesParams function cleanly builds feature request parameters based on the factory flag:
- Always includes basic targets (status, hw, fw, bt) and types (version, specific)
- Conditionally adds advanced targets (SE devices) and types (build_id, hash) when in factory mode
This supports the new protocol's selective feature request approach.
🏁 Script executed:
#!/bin/bash # Check usage of this function to verify proper integration rg -A 3 "cherryPickFeaturesParams" --type tsLength of output: 2614
🏁 Script executed:
#!/bin/bash # Locate the definition of IFeaturesType rg -n "IFeaturesType" --type tsLength of output: 291
🏁 Script executed:
#!/bin/bash # Show full IFeaturesType definition rg -A10 -B2 "export interface IFeaturesType" --type ts packages/core/src/device/Device.tsLength of output: 345
✅ Approve cherryPickFeaturesParams integration
The function correctly builds feature request parameters:
- Always includes basic targets:
status,hw,fw,bt- Always includes basic types:
version,specific- Adds SE targets (
se1–se4) and extra types (build_id,hash) whenfactory = trueWe verified its usage in
Device.ts,GetOnekeyFeatures.ts, anduploadFirmware.ts. No issues found.packages/core/src/utils/deviceInfoUtils.ts (4)
6-6: Good import of centralized utility functions.Importing the helper functions from onekeyInfoUtils centralizes feature extraction logic.
16-17: Improved hardware info extraction.Refactored to use the centralized getHardwareInfoFromFeatures helper instead of direct property access, supporting both legacy and new feature formats.
86-88: Simplified BLE name retrieval.Refactored to use getFirmwareInfoFromFeatures, which handles both old and new property formats consistently.
95-96: Streamlined device UUID extraction.Using the centralized helper function improves maintainability and ensures consistent handling of both old and new serial number formats.
packages/core/src/api/device/DeviceUpdateReboot.ts (4)
1-3: Good imports for reboot functionality.Added OneKeyRebootType enum and switched to FirmwareUpdateBaseMethod for centralized reboot handling.
6-6: Improved class inheritance.Extending FirmwareUpdateBaseMethod gives access to centralized firmware update utilities.
10-10: Better UI control with BOOTLOADER restriction.Adding UI_REQUEST.BOOTLOADER to notAllowDeviceMode prevents using this method when device is already in bootloader mode.
14-14: Simplified reboot implementation.Using the base class's reboot method with OneKeyRebootType.BootLoader parameter:
- Supports different message versions
- Provides better error handling
- Follows the unified device reboot pattern
This is more robust than directly calling device commands.
packages/connect-examples/expo-example/src/views/FirmwareScreen/index.tsx (1)
402-405: Check merge order of feature objectsYou spread
featuresfirst andonekeyFeaturessecond, so any overlapping keys from OneKey will silently override the originalfeatures. This is probably fine, but please double-check that this is the intended precedence – especially for critical flags such asbootloader_mode.packages/core/src/utils/deviceVersionUtils.ts (1)
1-58: Looks goodRefactor to
getFirmwareInfoFromFeaturesremoves duplication and increases consistency.packages/core/src/api/FirmwareUpdateV3.ts (1)
388-395: Ensure every device firmware supportsStartSessionOlder firmware used
Initialize. Swapping toStartSessionunconditionally may break reconnect on legacy devices. Consider a feature-based fallback.await Promise.race([ typedCall(supportsStartSession ? 'StartSession' : 'Initialize', 'Features', {}), // … ]);packages/core/src/device/Device.ts (1)
381-384: Check for undefined before asking for message version
getSupportMessageVersion(this.features)receivesundefinedon the very first call toinitialize, which may break inside that helper. Guard with a null-check or default.Would you confirm that
getSupportMessageVersionhandlesundefinedsafely? If not, wrap the call:const { messageVersion } = getSupportMessageVersion(this.features ?? ({} as any));packages/core/src/utils/onekeyInfoUtils.ts (1)
148-170: Guard empty hardware version
oldHardwareVersionis set to an empty string, then passed intofixVersion.
IffixVersion('')returns"0.0.0"it may overwrite a valid new-protocol version with a bogus default.Consider:
const rawVersion = newHardwareVersion ?? oldHardwareVersion; hardwareVersion: rawVersion ? fixVersion(rawVersion) : undefined,
| @@ -59,7 +59,7 @@ export default class TransportManager { | |||
| } else { | |||
| await this.transport.init(HttpLogger); | |||
| } | |||
| Log.debug('Configuring transports'); | |||
| Log.debug('Configuring transports', this.messageVersion); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhanced logging with message version information.
Logging the message version provides helpful context for debugging transport configuration issues.
Use the class name instead of this in static methods:
- Log.debug('Configuring transports', this.messageVersion);
+ Log.debug('Configuring transports', TransportManager.messageVersion);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Log.debug('Configuring transports', this.messageVersion); | |
| Log.debug('Configuring transports', TransportManager.messageVersion); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🤖 Prompt for AI Agents (early access)
In packages/core/src/data-manager/TransportManager.ts at line 62, the logging statement uses 'this.messageVersion' inside a static method, which is incorrect. Replace 'this.messageVersion' with the class name followed by '.messageVersion' to correctly reference the static property and improve logging context.
| @@ -74,6 +74,7 @@ export default class TransportManager { | |||
| Log.debug(`Begin reconfiguring transports`); | |||
| const { messageVersion, messages } = getSupportMessageVersion(features); | |||
|
|
|||
| console.error('caikaisheng reconfigure', messageVersion); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debugging console.error statement.
This appears to be leftover debugging code that includes a developer identifier.
- console.error('caikaisheng reconfigure', messageVersion);Or if this message is needed for debugging, use the standard logging approach:
- console.error('caikaisheng reconfigure', messageVersion);
+ Log.debug(`Reconfiguring message version: ${messageVersion}`);🤖 Prompt for AI Agents (early access)
In packages/core/src/data-manager/TransportManager.ts at line 77, remove the console.error statement used for debugging that includes a developer identifier. If logging this message is necessary, replace it with the project's standard logging mechanism instead of using console.error directly.
| [submodule "submodules/onekey-protocol"] | ||
| path = submodules/onekey-protocol | ||
| url = git@github.com:OneKeyHQ/onekey-protocol.git |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New onekey-protocol submodule replaces firmware submodule
The change replaces the old firmware submodule with the new onekey-protocol submodule. This aligns with the protocol update mentioned in the PR.
However, SSH URLs may cause issues in some CI environments. Consider using HTTPS URL instead.
🏁 Script executed:
#!/bin/bash
# Check if the submodule exists and is properly configured
git submodule status | grep onekey-protocolLength of output: 114
Update .gitmodules submodule URL for CI compatibility
The old firmware submodule is now replaced by submodules/onekey-protocol. That aligns with the protocol update in this PR.
However, the SSH URL in .gitmodules can fail in some CI environments. Please switch:
- From:
git@github.com:OneKeyHQ/onekey-protocol.git - To:
https://github.com/OneKeyHQ/onekey-protocol.git
.fix in .gitmodules at lines 1–3.
🤖 Prompt for AI Agents (early access)
In the .gitmodules file at lines 1 to 3, the submodule URL uses an SSH format which can cause issues in some CI environments. Change the URL from the SSH format git@github.com:OneKeyHQ/onekey-protocol.git to the HTTPS format https://github.com/OneKeyHQ/onekey-protocol.git to ensure compatibility.
| // TODO: 打补丁 | ||
| export type OnekeyFeatures = PROTO.Features; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
OnekeyFeatures type now points to unified Features type
The change updates OnekeyFeatures to use the unified Features type, which simplifies type management.
However, the TODO comment in Chinese ("打补丁" meaning "apply patch") should be in English for consistency, and it needs to be more specific about what patch is needed.
-// TODO: 打补丁
+// TODO: Apply patch to handle legacy OnekeyFeatures compatibility📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: 打补丁 | |
| export type OnekeyFeatures = PROTO.Features; | |
| // TODO: Apply patch to handle legacy OnekeyFeatures compatibility | |
| export type OnekeyFeatures = PROTO.Features; |
🤖 Prompt for AI Agents (early access)
In packages/core/src/types/device.ts at lines 81 to 82, replace the TODO comment "打补丁" with a clear and specific English comment explaining what patch or fix is intended. This improves code clarity and maintains consistency in language for all developers.
| @@ -111,17 +112,19 @@ export default class HttpTransport { | |||
| this.Log.debug('call-', ' name: ', name, ' data: ', data); | |||
| } | |||
|
|
|||
| console.error('caikaisheng call', name, data); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console debugging statements.
These console.error logs contain personal identifiers and expose internal data. Replace with structured logging if needed.
- console.error('caikaisheng call', name, data);
+ // Consider using this.Log for consistent logging if needed
- console.error('caikaisheng call resData', jsonData);
+ // Consider using this.Log for consistent logging if neededAlso applies to: 127-127
🤖 Prompt for AI Agents (early access)
In packages/hd-transport-http/src/index.ts at lines 115 and 127, remove the console.error debugging statements as they expose personal identifiers and internal data. Replace these with structured logging using the existing logging framework or a secure logger that masks or omits sensitive information, ensuring no personal data is logged directly.
| async reboot(rebootType: OneKeyRebootType) { | ||
| const typedCall = this.device.getCommands().typedCall.bind(this.device.getCommands()); | ||
| const { messageVersion } = getSupportMessageVersion(this.device.features); | ||
| // TODO: fix it | ||
| if (messageVersion === 'latest') { | ||
| try { | ||
| return await typedCall('OneKeyReboot', 'Success', { | ||
| reboot_type: rebootType, | ||
| }); | ||
| } catch (e) { | ||
| Log.error('FirmwareUpdateBaseMethod [reboot] OneKeyReboot failed: ', e); | ||
| return { | ||
| type: 'Success', | ||
| message: 'reboot success', | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| switch (rebootType) { | ||
| case OneKeyRebootType.BootLoader: | ||
| return typedCall('DeviceBackToBoot', 'Success', { | ||
| reboot_type: OneKeyRebootType.BootLoader, | ||
| }); | ||
| // On Touch devices, messsage code 904 is RebootToBoardloader | ||
| // so BininOutMessageSE message code 904 is used here | ||
| case OneKeyRebootType.Boardloader: | ||
| return typedCall('BixinOutMessageSE', 'Success'); | ||
| case OneKeyRebootType.Normal: { | ||
| try { | ||
| return await typedCall('OneKeyReboot', 'Success', { | ||
| reboot_type: OneKeyRebootType.Normal, | ||
| }); | ||
| } catch (e) { | ||
| Log.error('FirmwareUpdateBaseMethod [reboot] OneKeyReboot failed: ', e); | ||
| return { | ||
| type: 'Success', | ||
| message: 'reboot success', | ||
| }; | ||
| } | ||
| } | ||
| default: | ||
| throw new Error('Invalid reboot type'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not swallow reboot errors
reboot() catches every failure and returns a fake “success”, masking real issues and forcing callers to guess. Let errors propagate or return a discriminated union.
- } catch (e) {
- Log.error('FirmwareUpdateBaseMethod [reboot] OneKeyReboot failed: ', e);
- return {
- type: 'Success',
- message: 'reboot success',
- };
- }
+ } catch (e) {
+ Log.error('FirmwareUpdateBaseMethod [reboot] OneKeyReboot failed: ', e);
+ throw e;
+ }🤖 Prompt for AI Agents (early access)
In packages/core/src/api/firmware/FirmwareUpdateBaseMethod.ts between lines 207 and 250, the reboot method catches all errors and returns a fake success response, which hides real failures. To fix this, remove the catch blocks that swallow errors and instead let the errors propagate naturally or refactor the method to return a discriminated union that clearly indicates success or failure, ensuring callers can handle errors properly.
| }; | ||
|
|
||
| export const fixFeaturesBetweenProtocol = (oldFeatures: Features, features: Features) => { | ||
| console.log('======> fixFeaturesBetweenProtocol', oldFeatures); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove hard-coded console.log
console.log in shared utilities clutters production logs and leaks internal objects.
Pipe debug output through the project logger or guard it with an env flag.
- console.log('======> fixFeaturesBetweenProtocol', oldFeatures);
+ // logger.debug('fixFeaturesBetweenProtocol', oldFeatures);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('======> fixFeaturesBetweenProtocol', oldFeatures); | |
| - console.log('======> fixFeaturesBetweenProtocol', oldFeatures); | |
| + // logger.debug('fixFeaturesBetweenProtocol', oldFeatures); |
🤖 Prompt for AI Agents (early access)
In packages/core/src/utils/onekeyInfoUtils.ts at line 304, replace the hard-coded console.log statement with a call to the project’s logger utility or wrap it in a conditional check that only logs when a specific environment debug flag is enabled. This prevents cluttering production logs and leaking internal data.
| const oldLable = (features as any)?.label; | ||
| const newLable = features?.ok_dev_info_resp?.status?.lable; | ||
|
|
||
| const oldInitState = (features as any)?.init_state; | ||
| const newInitState = features?.ok_dev_info_resp?.status?.init_states; | ||
|
|
||
| const oldLanguage = (features as any)?.language; | ||
| const newLanguage = features?.ok_dev_info_resp?.status?.language; | ||
|
|
||
| const oldPassphraseProtection = (features as any)?.passphrase_protection; | ||
| const newPassphraseProtection = features?.ok_dev_info_resp?.status?.passphrase_protection; | ||
|
|
||
| return { | ||
| label: oldLable || newLable, | ||
| init_state: oldInitState || newInitState, | ||
| language: oldLanguage || newLanguage, | ||
| passphrase_protection: oldPassphraseProtection || newPassphraseProtection, |
There was a problem hiding this comment.
Fix typo “Lable” → “Label” and align new-protocol key
The helper misspells both the temporary variables and the key coming from the new protocol.
As written, newLable will always be undefined and the return object will fall back to oldLable, masking data for new-firmware devices.
-const oldLable = (features as any)?.label;
-const newLable = features?.ok_dev_info_resp?.status?.lable;
+const oldLabel = (features as any)?.label;
+// new protocol still uses “label”
+const newLabel = features?.ok_dev_info_resp?.status?.label;
...
- return {
- label: oldLable || newLable,
+ return {
+ label: oldLabel || newLabel,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const oldLable = (features as any)?.label; | |
| const newLable = features?.ok_dev_info_resp?.status?.lable; | |
| const oldInitState = (features as any)?.init_state; | |
| const newInitState = features?.ok_dev_info_resp?.status?.init_states; | |
| const oldLanguage = (features as any)?.language; | |
| const newLanguage = features?.ok_dev_info_resp?.status?.language; | |
| const oldPassphraseProtection = (features as any)?.passphrase_protection; | |
| const newPassphraseProtection = features?.ok_dev_info_resp?.status?.passphrase_protection; | |
| return { | |
| label: oldLable || newLable, | |
| init_state: oldInitState || newInitState, | |
| language: oldLanguage || newLanguage, | |
| passphrase_protection: oldPassphraseProtection || newPassphraseProtection, | |
| const oldLabel = (features as any)?.label; | |
| // new protocol still uses “label” | |
| const newLabel = features?.ok_dev_info_resp?.status?.label; | |
| const oldInitState = (features as any)?.init_state; | |
| const newInitState = features?.ok_dev_info_resp?.status?.init_states; | |
| const oldLanguage = (features as any)?.language; | |
| const newLanguage = features?.ok_dev_info_resp?.status?.language; | |
| const oldPassphraseProtection = (features as any)?.passphrase_protection; | |
| const newPassphraseProtection = features?.ok_dev_info_resp?.status?.passphrase_protection; | |
| return { | |
| label: oldLabel || newLabel, | |
| init_state: oldInitState || newInitState, | |
| language: oldLanguage || newLanguage, | |
| passphrase_protection: oldPassphraseProtection || newPassphraseProtection, | |
| }; |
🤖 Prompt for AI Agents (early access)
In packages/core/src/utils/onekeyInfoUtils.ts lines 283 to 299, fix the typo by renaming all instances of "Lable" to "Label" in both the temporary variables and the new protocol key. Also, correct the new protocol key from "lable" to "label" to properly access the data. This ensures the newLabel variable is correctly assigned and used in the return object, preventing fallback to oldLabel when new firmware data is present.
| isEnteredManuallyBoot(features: Features) { | ||
| const deviceType = getDeviceType(features); | ||
| const isMini = deviceType === EDeviceType.Mini; | ||
| const bootloaderVersion = getDeviceBootloaderVersion(features).join('.'); | ||
| const isBoot183ClassicUpBle = | ||
| this.params.updateType === 'firmware' && | ||
| deviceType === EDeviceType.Classic && | ||
| features.bootloader_version === '1.8.3'; | ||
| bootloaderVersion === '1.8.3'; | ||
| return isMini || isBoot183ClassicUpBle; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: join on already-joined string
getDeviceBootloaderVersion(features).join('.') returns a string, yet you join it again when comparing:
const bootloaderVersion = getDeviceBootloaderVersion(features).join('.');
...
bootloaderVersion === '1.8.3'join('.') is executed twice each polling cycle. Cache it once or compare the array directly.
🤖 Prompt for AI Agents (early access)
In packages/core/src/api/FirmwareUpdateV2.ts around lines 178 to 186, the code calls join('.') on the result of getDeviceBootloaderVersion(features) twice per polling cycle, which is inefficient. To fix this, call getDeviceBootloaderVersion(features) once, store the resulting array in a variable, then either join it once and reuse the joined string for comparison or compare the array directly without joining multiple times.
| }; | ||
|
|
||
| // BenchmarkListNames | ||
| export type BenchmarkListNames = {}; | ||
|
|
||
| // BenchmarkNames | ||
| export type BenchmarkNames = { | ||
| names: string[]; | ||
| }; | ||
|
|
||
| // BenchmarkRun | ||
| export type BenchmarkRun = { | ||
| name?: string; | ||
| }; | ||
|
|
||
| // BenchmarkResult | ||
| export type BenchmarkResult = { | ||
| value?: string; | ||
| unit?: string; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid banned {} type in generated declarations
Biome flags the empty-object type eight times (example below).
Either replace with Record<string, never> or suppress the rule for generated files:
// @ts-expect-error generated code – intentional empty object
export type BenchmarkListNames = Record<string, never>;Alternatively, add /* biome-ignore lint/complexity/noBannedTypes */ above each occurrence and document that this file is auto-generated.
🧰 Tools
🪛 Biome (1.9.4)
[error] 143-143: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🤖 Prompt for AI Agents (early access)
In packages/hd-transport/src/types/messages.ts between lines 140 and 160, the empty object type {} is used, which is banned by Biome lint rules. Replace each occurrence of {} with Record<string, never> to explicitly represent an empty object type, or alternatively, add a comment above each such type declaration to suppress the lint rule using /* biome-ignore lint/complexity/noBannedTypes */ and document that the file is auto-generated.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Documentation