feat: add OneKey keyring #353
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
2dc83be to
a18c779
Compare
Akaryatrh
added
the
team-hardware-wallets
|
@metamaskbot publish-preview |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
2 similar comments
|
@metamaskbot publish-preview |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
dawnseeker8
left a comment
dawnseeker8
left a comment
There was a problem hiding this comment.
Please change your new keyring to implement the new keyring type to support new BIP44 structure tree.
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
2 times, most recently
from
e912151 to
2ee6f90
Compare
|
Warning MetaMask internal reviewing guidelines:
|
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
2ee6f90 to
f0a1825
Compare
|
@SocketSecurity ignore npm/[email protected] |
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
f0a1825 to
7d43e97
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
| page = 0; | ||
|
|
||
| perPage = 5; | ||
|
|
||
| unlockedAccount = 0; | ||
|
|
||
| hdk = new HDKey(); | ||
|
|
||
| accounts: readonly Hex[] = []; | ||
|
|
||
| accountDetails: Record<string, AccountDetails> = {}; | ||
|
|
||
| passphraseState: string | undefined; | ||
|
|
||
| hdPath = defaultHdPath; | ||
|
|
||
| network: NetworkApiUrls = NetworkApiUrls.Mainnet; | ||
|
|
||
| implementFullBIP44 = false; |
There was a problem hiding this comment.
Can we make these properties sharp, and readonly where possible?
| lock(): void { | ||
| this.hdk = new HDKey(); | ||
| } | ||
|
|
||
| isUnlocked(): boolean { | ||
| return Boolean(this.hdk?.publicKey); | ||
| } |
There was a problem hiding this comment.
Why do we need this locking mechanism internal to the keyring? What does this lock represent?
There was a problem hiding this comment.
lock, isUnlocked
These two are designed to optimize the user experience of our hardware passphrase functionality.
Without them, users would need to re-enter the passphrase every time. We lock when entering the connect wallet page, then have the device re-enter the passphrase. This ensures that scenarios like address pagination won't trigger passphrase re-entry.
There was a problem hiding this comment.
hmm the only thing I see this doing is removing the cached HD public key coming from the device. Though .lock is not a common method that other hardware keyrings have, and we'd have to add specific code on clients to be call this function on the OneKey keyring. I was wondering if we really need this mechanism instead of just relying on usual patterns we have on other keyrings, where the "lock" is coming from KeyringController in the form of destroying keyring instances altogether
| return Promise.resolve(); | ||
| } | ||
|
|
||
| getModel(): string | undefined { |
There was a problem hiding this comment.
Is/will this method used somewhere? If not, it can be removed.
There was a problem hiding this comment.
used to distinguish equipment models.
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
7d43e97 to
be5d34b
Compare
| } | ||
| const signature = addHexPrefix(response.payload.signature); | ||
| // eslint-disable-next-line promise/no-multiple-resolved | ||
| resolve(signature); |
There was a problem hiding this comment.
Bug: Missing return after reject causes double promise resolution
In signPersonalMessage, when the address verification fails (lines 410-414), reject() is called but execution continues without a return statement. This causes resolve(signature) to also be called on line 418. The // eslint-disable-next-line promise/no-multiple-resolved comment explicitly acknowledges this issue. While Promise semantics cause the first call (reject) to take effect, the control flow is broken—the code after reject() still runs unnecessarily. The similar signTypedData method correctly handles this case by using throw instead, which properly exits the function.
There was a problem hiding this comment.
Cursor is right here, you should remove the // eslint-disable-next-line and put the resolve in a else {} block.
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
be5d34b to
a9761e5
Compare
Akaryatrh
force-pushed
the
feat/onekey-keyring
branch
from
a9761e5 to
8508712
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
| reject(new Error('signature doesnt match the right address')); | ||
| } | ||
| // eslint-disable-next-line promise/no-multiple-resolved | ||
| resolve(signature); |
There was a problem hiding this comment.
Bug: Missing return after reject causes dual promise settlement
In signPersonalMessage, when the recovered address doesn't match the expected account, reject() is called but execution continues without returning. This causes resolve(signature) on line 427 to also be called. The eslint-disable-next-line promise/no-multiple-resolved comment explicitly suppresses the linter warning about this issue rather than fixing it. While Promise semantics make only the first settlement effective, calling both reject() and resolve() is a control flow error indicating the code doesn't properly stop execution after detecting the address mismatch.
| async dispose(): Promise<void> { | ||
| this.sdk?.dispose(); | ||
| return Promise.resolve(); | ||
| } |
There was a problem hiding this comment.
Bug: Dispose leaves bridge in inconsistent reinitialization state
The dispose() method calls this.sdk?.dispose() but doesn't reset isSDKInitialized or sdk. When keyring.destroy() calls bridge.dispose(), the bridge is left in an inconsistent state where isSDKInitialized remains true and sdk still references the disposed SDK. If init() is called afterward, it returns early because isSDKInitialized is true, preventing reinitialization. Subsequent operations will then use the disposed SDK. The destroy() method properly resets state but is never called by the keyring.
Additional Locations (1)
There was a problem hiding this comment.
Cursor's comment here make sense. We should add this.isSDKInitialized = false;.
| return await this.sdk | ||
| .evmGetPublicKey('', '', { ...params, skipPassphraseCheck: true }) | ||
| .then((result) => { |
There was a problem hiding this comment.
Can we avoid mixing .then().catch() with async/awaits? Can we use async/await only to improve readability?
| }, | ||
| }; | ||
| } | ||
| return await this.sdk.getPassphraseState('').then((result) => { |
There was a problem hiding this comment.
similar to my other comment, can we use a more consistent pattern with promises?
| ...params, | ||
| skipPassphraseCheck: true, | ||
| }) | ||
| .then((result) => { |
There was a problem hiding this comment.
similar to other comments, can we use async/await only?
| ...params, | ||
| skipPassphraseCheck: true, | ||
| }) | ||
| .then((result) => { |
| ...params, | ||
| skipPassphraseCheck: true, | ||
| }) | ||
| .then((result) => { |
Adds OneKey keyring based on OneKey libraries.
Note
Introduce
@metamask/eth-onekey-keyringwith OneKey Web SDK integration for account derivation and EVM signing.@metamask/eth-onekey-keyringOneKeyKeyringwith HD path management, account pagination, and device passphrase handling.OneKeyWebBridgeadapter over@onekeyfe/hd-web-sdkfor public key export, transaction/message/typed-data signing, transport switching, and UI event handling.OneKeyBridge) and entrypoints viasrc/index.ts.README.mdlist and dependency graph.tsconfig.jsonandtsconfig.build.jsonreferences to include the new package.Written by Cursor Bugbot for commit 8508712. This will update automatically on new commits. Configure here.