feat: update to keyring v2#606
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
|
@SocketSecurity ignore npm/ses@1.15.0 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ccharly
left a comment
ccharly
left a comment
There was a problem hiding this comment.
LGTM for v2!
One thing though, I wonder if we should really expose exportAccount for hexadecimal, it seems base58 is the way to go for Solana.
We could keep it if we wish to export them in "raw format", but I would strip the 0x prefix then (I'm not sure this is commonly used outside of Ethereum, but I could be wrong about that 😄)
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dac2ffa. Configure here.
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
| languageName: node | ||
| linkType: hard | ||
|
|
||
| "@ethersproject/abi@npm:^5.7.0": |
There was a problem hiding this comment.
NOTE: Looks like that's being pulled by @metamask/controller-utils.
|
ccharly
left a comment
ccharly
left a comment
There was a problem hiding this comment.
LGTM + tested on extension e2e, see:
stanleyyconsensys
left a comment
stanleyyconsensys
left a comment
There was a problem hiding this comment.
LGTM
just 1 question and 1 nit review
@ulissesferreira would you mind to give another review, as it has a lot of changes, which im not pretty sure
4 participants
Summary
Migrates the Solana snap to the v2 Keyring API and declares the corresponding capabilities in the manifest. Also removes all v1-only keyring methods and fixes a latent test-isolation bug surfaced while validating the migration.
SolanaKeyringto implementKeyringSnapRpc(from@metamask/keyring-api/v2): addsgetAccounts,getAccount(throws on missing IDs instead of returningundefined), andexportAccount(base58 only); renameslistAccountAssets/listAccountTransactions→getAccountAssets/getAccountTransactionscreateAccount,listAccounts,discoverAccounts,updateAccount,filterAccountChains) and stops emittingAccountCreated/AccountDeletedeventsbip44:discoverintocreateAccountsvia on-chain activity checksSnapErrorhandleKeyringRequestto the v2 dispatcher (@metamask/keyring-snap-sdk/v2)endowment:keyring.capabilitiestosnap.manifest.json@metamask/snaps-sdk→ 11.1.1,snaps-cli→ 8.4.1,snaps-jest→ 10.1.3,keyring-snap-sdk→ 9.1.0, and updates the rootresolutionsto match;snaps-utilsresolves to 12.2.0 which is required to validate the new manifest fieldmoduleResolution: "bundler"inpackages/snap/tsconfig.jsonso v2 subpath types resolve correctlyjest --runInBandfailure: replaces an incompletejest.mock('@noble/ed25519', ...)inindex.test.tswithjest.mock('./polyfills', ...). The previous mock was captured by the ed25519 polyfill's closure when it patchedglobalThis.crypto.subtle, leaking into other test files via shared global state under--runInBandNote
High Risk
Touches account creation/deletion, private key export, and keyring RPC contracts—security-sensitive paths with breaking behavior changes for v1 callers and clients that relied on keyring events or
getAccountreturning undefined.Overview
Migrates the Solana wallet snap from the v1 keyring protocol to Keyring API v2, with dependency bumps and manifest updates so MetaMask can drive account lifecycle through the new RPC surface.
SolanaKeyringnow implementsKeyringSnapRpcinstead of v1Keyring. Entry point useshandleKeyringRequestfrom@metamask/keyring-snap-sdk/v2. Added:getAccounts,exportAccount(base58 64-byte secret key), andbip44:discoverinsidecreateAccountsvia on-chain activity. Renamed:listAccountAssets/listAccountTransactions→getAccountAssets/getAccountTransactions. Removed:createAccount,listAccounts,discoverAccounts,updateAccount,filterAccountChains, andAccountCreated/AccountDeletedevents;deleteAccountonly clears snap state.getAccountnow throws when the id is missing (noundefined). Public keyring errors are wrapped in a singleSnapError.snap.manifest.jsonaddsendowment:keyring.capabilities(scopes,privateKeyexport,bip44derive/discover) and bumpsplatformVersionto 11.1.1.permissions.tsallows v2KeyringSnapRpcMethods plus a subset of v1 method names for compatibility.Tooling:
@metamask/keyring-api^23.5.0,keyring-snap-sdk^9.2.0,snaps-sdk^11.1.1; rootresolutionsand.yarnrc.ymlpreapproved packages for fresh MetaMask releases; snaptsconfigusesmoduleResolution: "bundler".Tests: keyring suite aligned to v2 APIs and
exportAccount;index.test.tsmocks./polyfillsinstead of@noble/ed25519to avoid flakyglobalThis.crypto.subtleleakage underjest --runInBand.Reviewed by Cursor Bugbot for commit 754dc86. Bugbot is set up for automated code reviews on this repo. Configure here.