feat: update to keyring v2#645
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning MetaMask internal reviewing guidelines:
Ignoring alerts on:
|
|
@SocketSecurity ignore npm/@metamask/snaps-controllers@21.0.0 |
|
@SocketSecurity ignore npm/ses@2.2.0 |
|
@SocketSecurity ignore npm/@ethersproject/web@5.8.0 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ 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 54d5d1f. Configure here.
|
|
||
| expect(response).toRespondWithError({ | ||
| code: -32000, | ||
| code: -32603, |
There was a problem hiding this comment.
What is this code exactly? Would it be worth extracting into a constant seeing it's reused a lot throughout the file?
I know this wasn't in a constant before, so feel free to ignore!
| accounts[expectedAddress] = response.response.result as KeyringAccount; | ||
| accounts[expectedAddress] = ( | ||
| response.response.result as KeyringAccount[] | ||
| )[0]!; |
There was a problem hiding this comment.
Is the non null assertion here "legal"? Can we maybe find a way to explicitly go around it?
| response.response as { result: KeyringAccount } | ||
| ).result; | ||
| const { result } = response.response as { result: KeyringAccount[] }; | ||
| const account = result[0]!; |
| writeFileSync( | ||
| join(__dirname, '../locales/en.json'), | ||
| JSON.stringify(englishLocale, null, 2), | ||
| `${JSON.stringify(englishLocale, null, 2)}\n`, |
| "module": "ESNext", | ||
| "moduleResolution": "bundler", |
There was a problem hiding this comment.
Is this related to / needed for V2 migration?
There was a problem hiding this comment.
yup! needed to make a similar change on the sol snap
| if (options.type === AccountCreationType.Bip44DerivePath) { | ||
| const parts = (options.derivationPath as string).split('/'); | ||
| const purpose = parts[1]?.replace("'", ''); | ||
| const coinType = parts[2]?.replace("'", ''); | ||
| const accountIndex = parts[3]?.replace("'", ''); |
There was a problem hiding this comment.
I think this could be a bit dangerous and brittle. We're pulling out purpose, coinType, and accountIndex purely by position, but the validation struct only guarantees the shape of each segment, not how many there are.
It seems we had extractAccountIndex / extractAddressType doing this with before, should we potentially bring those back (or a similar single, validated parser) instead of parsing inline?
| // validate default address type is P2WPKH just to be sure | ||
| if (resolvedAddressType !== 'p2wpkh') { | ||
|
|
||
| if (coinType !== '0' && coinType !== '1') { |
There was a problem hiding this comment.
Nit 😄 could pull the supported types into a constant to make the check read a bit nicer:
const SUPPORTED_COIN_TYPES = ['0', '1'];
if (!SUPPORTED_COIN_TYPES.includes(coinType)) {
throw new FormatError(
'Unsupported coin type: only coin type 0 (mainnet) and 1 (regtest) are supported',
);
}| const traceName = 'Create Bitcoin Accounts Batch'; | ||
| const traceStarted = await this.#snapClient.startTrace(traceName); | ||
| // Only mainnet is supported for v2 account creation. | ||
| const network = scopeToNetwork[SUPPORTED_SCOPES[0] ?? BtcScope.Mainnet]; |
There was a problem hiding this comment.
Can SUPPORTED_SCOPES[0] be dangerous if the manifest order changes?
2 participants
Summary
Migrates the Bitcoin snap to the v2 Keyring API and declares the corresponding capabilities in the manifest. Also removes all v1-only keyring methods and replaces the manual routing table with the v2 dispatcher.
KeyringHandlerto implementKeyringSnapRpc(from@metamask/keyring-api/v2): addsgetAccounts,getAccount,createAccounts,exportAccount,getAccountBalances,getAccountAssets,getAccountTransactions,deleteAccount,submitRequest,setSelectedAccounts, andresolveAccountAddresscreateAccount,discoverAccounts,filterAccountChains,updateAccount) and stops emittingAccountCreated/AccountDeletedeventsbip44:discoverintocreateAccounts: callsAccountUseCases.discover(), deletes the account from state if no transactions are found, and returns[]to signal end-of-discoveryexportAccountwith WIF private-key export (Base58Check, version byte 0x80/0xEF); usesis()notassert()to prevent the private key from appearing in aStructErrormessage on encoding failureSUPPORTED_SCOPESdirectly fromsnap.manifest.jsoncapabilities block (same pattern as Solana snap) instead of hardcoding the networkhandleKeyringRequestto the v2 dispatcher (@metamask/keyring-snap-sdk/v2)endowment:keyring.capabilitiestosnap.manifest.json:scopes,privateKey.exportFormats, andbip44derive/discover flagsmodule: "ESNext"andmoduleResolution: "bundler"inpackages/snap/tsconfig.jsonso/v2subpath exports resolve correctly@metamask/keyring-api→ ^23.5.0,keyring-snap-sdk→ ^9.2.0,snaps-sdk→ ^11.2.0,snaps-cli→ ^8.4.1,snaps-jest→ ^10.2.0KeyringHandler.test.ts: removes v1-only describe blocks, addsexportAccountandbip44:discovercoverage, mockswifmodule, addsbeforeEach(() => jest.resetAllMocks())to fix mock-call accumulation between testsReferences
N/A
Checklist
Note
High Risk
Breaking keyring RPC surface and account-creation/discovery semantics for MetaMask clients, plus new private-key export—security-sensitive changes across auth/key material handling.
Overview
Migrates the Bitcoin wallet snap to Keyring API v2, replacing the in-snap RPC router with
handleKeyringRequestfrom@metamask/keyring-snap-sdk/v2and v2 method names (keyring_createAccounts,keyring_getAccounts, etc.).KeyringHandlernow implementsKeyringSnapRpc: account lifecycle goes throughcreateAccounts(BIP-44 derive path/index/range andbip44:discoverfolded in—inactive discovered accounts are deleted and[]is returned). Removed v1-onlycreateAccount,discoverAccounts,filterAccountChains, andupdateAccount. Renamed list-style APIs togetAccounts,getAccountAssets,getAccountTransactions. AddsexportAccountwith WIF (base58) export and guards so private keys do not leak in error messages.Manifest & mapping:
endowment:keyringgainscapabilities(scopes,privateKey.exportFormats, bip44 derive/discover). Accounts are markedexportable: true. Supported discovery scope is read from the manifest instead of being hardcoded.Deps & tooling: Bumps keyring-api, keyring-snap-sdk, snaps-sdk/cli/jest; snap
tsconfiguses ESNext/bundler for/v2imports. Integration/unit tests updated for plural create results, new RPC names, and v2 error shapes.Reviewed by Cursor Bugbot for commit e698816. Bugbot is set up for automated code reviews on this repo. Configure here.