Skip to content

Conversation

@ByteZhang1024
Copy link
Contributor

@ByteZhang1024 ByteZhang1024 commented May 9, 2025

Description

This pull request adds support for connecting OneKey hardware wallets to the extension.
eth-onekey-bridge-keyring

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Plug in a OneKey hardware wallet.
  2. Open the MetaMask extension and navigate to the "Connect Hardware Wallet" section.
  3. Select "OneKey" from the list.
  4. Approve the USB permission request if prompted.
  5. Ensure the device is detected and connection is established successfully.

Screenshots/Recordings

Before

image

After

image

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Adds OneKey hardware wallet support with offscreen bridge, keyring integration, UI flow and error pages, localization, and dependencies.

  • Hardware/Controller:
    • Integrates @metamask/eth-onekey-keyring with new OneKeyOffscreenBridge and hardware-onekey-keyring-builder-factory; wires into KeyringControllerInit (MV2/MV3) and adds connectHardwareBeforeCheck.
    • Adds offscreen OneKey handler app/offscreen/onekey.ts; initializes in offscreen.ts; defines OneKey actions/events/targets in shared/constants/offscreen-communication.
    • Marks OneKey as not supporting eth_getEncryptionPublicKey.
  • UI/Flows:
    • Adds OneKey option to Connect Hardware (button, tutorial, WebUSB request, OneKey HD path); new LogoOnekey.
    • Introduces OneKey error route/pages (bridge install, firmware update/force, bootloader exit, WebUSB auth, passphrase/device errors) and registers route.
  • Constants/Localization:
    • Adds OneKey transport types and updates affiliate links; adds EN/EN_GB strings for OneKey flows/errors.
  • Build/Deps/Policy:
    • Adds OneKey SDK deps (@onekeyfe/hd-web-sdk et al.) and @metamask/eth-onekey-keyring; updates LavaMoat policy and yarn.lock.

Written by Cursor Bugbot for commit a53d769. This will update automatically on new commits. Configure here.

@ByteZhang1024 ByteZhang1024 requested a review from a team as a code owner May 9, 2025 11:06
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2025

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@socket-security
Copy link

socket-security bot commented May 9, 2025

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedstring.prototype.matchall@​4.0.7991009381100
Addedpostcss@​8.5.11001008284100
Added@​onekeyfe/​hd-web-sdk@​1.1.17-patch.1851008698100

View full report

@socket-security
Copy link

socket-security bot commented May 9, 2025

Caution

MetaMask internal reviewing guidelines:

  • Do not ignore-all
  • Each alert has instructions on how to review if you don't know what it means. If lost, ask your Security Liaison or the supply-chain group
  • Copy-paste ignore lines for specific packages or a group of one kind with a note on what research you did to deem it safe.
    @SocketSecurity ignore npm/PACKAGE@VERSION
Action Severity Alert  (click "▶" to expand/collapse)
Block High
Potentially malicious package (AI signal): npm @swc/core is 60.0% likely malicious

Notes: This package appears to be a legitimate native/binary package (swc) that installs and builds native bindings. However there are security concerns: the install will execute a local postinstall.js (arbitrary JS during install) and the package lists itself in devDependencies with a conflicting pinned version (=1.2.220), which matches a critical dependency rule and is a supply-chain red flag. Recommend reviewing the contents of postinstall.js and verifying why @swc/core appears in devDependencies at a different pinned version before trusting installation. Verify the integrity/source of optional platform binaries as well.

Confidence: 0.60

Severity: 0.90

From: package.jsonnpm/@storybook/[email protected]npm/@swc/[email protected]

ℹ Read more on: This package | This alert | What is AI-detected potential malware?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: Given the AI system's identification of this package as malware, extreme caution is advised. It is recommended to avoid downloading or installing this package until the threat is confirmed or flagged as a false positive.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/@swc/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

Block Low
Publisher changed: npm secure-json-parse is now published by eomm instead of fdawgs

New Author: eomm

Previous Author: fdawgs

From: ?npm/@onekeyfe/[email protected]npm/[email protected]

ℹ Read more on: This package | This alert | What is new author?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@ByteZhang1024 ByteZhang1024 force-pushed the feat/onekey branch 4 times, most recently from 176f528 to 7c0bfbd Compare May 12, 2025 02:07
@ByteZhang1024 ByteZhang1024 requested review from a team as code owners May 13, 2025 03:51
@Akaryatrh
Copy link
Contributor

@ByteZhang1024 The CI fails because of a failing yarn build dist command. Please run it locally, you'll see this error:

MetaMask build: Encountered an error while running task "dist".
Error: MetaMask build: runInChildProcess for task "scripts:core:dist:standardEntryPoints" encountered an error "255".
  at ChildProcess.eval (/Users/sebastienvaneyck/Documents/consensys/forks/metamask-extension/development/build/task.js:106:13)
  at Object.onceWrapper (node:events:633:26)
  at ChildProcess.emit (node:events:518:28)
  at ChildProcess._handle.onexit (node:internal/child_process:293:12)

This command is running fine on main branch.

@ByteZhang1024
Copy link
Contributor Author

@Akaryatrh I see it, but this error message is pretty vague, and I don’t have a direction for troubleshooting. Do you have any hints you can give me?

@dawnseeker8
Copy link
Contributor

hello, @ByteZhang1024 the reason yarn dist have error is source map of package you use have circle dependancies, i still can't identify which package cause this, however, if i change development/build/scripts.js to disable sourceMap bundle during lamamot build and minify process, then yarn dist can successfully build.

@ByteZhang1024 ByteZhang1024 force-pushed the feat/onekey branch 3 times, most recently from 81a08a1 to 2b110e0 Compare June 26, 2025 02:59
@ByteZhang1024 ByteZhang1024 requested a review from a team as a code owner July 30, 2025 14:23
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@Akaryatrh Akaryatrh changed the title Feat: support OneKey Hardware feat: support OneKey Hardware Jul 31, 2025
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Jul 31, 2025
cursor[bot]

This comment was marked as outdated.

Comment on lines 102 to 134
if (
this.state.selectedDevice === HardwareDeviceNames.oneKey &&
isUSBSupported
) {
this.setState({ trezorRequestDevicePending: true });
try {
const connectedDevice = await window.navigator.usb.requestDevice({
filters: ONEKEY_WEBUSB_FILTER,
});
if (!connectedDevice) {
throw new Error('No device selected');
}
} finally {
this.setState({ trezorRequestDevicePending: false });
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ByteZhang1024 Why do we need to keep Trezor compatibility here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

? history.push(previousPage)
: history.push(DEFAULT_ROUTE);
}
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ByteZhang1024 I think this comment is relevant to modifications applied on ui/pages/create-account/connect-hardware/select-hardware.js

@ByteZhang1024 ByteZhang1024 force-pushed the feat/onekey branch 2 times, most recently from 1e70ede to b3e6e15 Compare August 22, 2025 11:40
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

Copy link
Contributor

@dawnseeker8 dawnseeker8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, Please resolve the conflict to process further.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@ByteZhang1024
Copy link
Contributor Author

@dawnseeker8 How to fix the ‘Check template and add labels / check-template-and-add-labels (pull_request_target)’ CI workflow?

case KeyringType.oneKey: {
return new Promise((_, reject) => {
reject(
new Error('OneKey does not support eth_getEncryptionPublicKey.'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error should be localized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote this by imitating the code above. Do we need to create a separate localized version for OneKey?

LedgerKeyring as unknown as KeyringClass,
keyringOverrides?.ledgerBridge || LedgerIframeBridge,
),
hardwareKeyringBuilderFactory(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Duplicate OneKeyKeyring registration with wrong bridge

In the non-MV3 path (isManifestV3 === false), OneKeyKeyring is registered twice in the additionalKeyrings array. The first registration at lines 82-85 incorrectly uses TrezorConnectBridge as the fallback bridge, while the second registration at lines 90-93 correctly uses OneKeyWebBridge. The first registration with TrezorConnectBridge appears to be leftover code that should have been removed when adding the proper OneKey bridge implementation. This duplication could cause unpredictable behavior when connecting OneKey hardware wallets in non-MV3 builds.

Additional Locations (1)

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contributor INVALID-PR-TEMPLATE PR's body doesn't match template

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants