Implement Keyring API v2#151
Conversation
529fdab to
00fcbf1
Compare
00fcbf1 to
03fdea9
Compare
|
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-all |
There was a problem hiding this comment.
Pull request overview
This PR migrates the Stellar snap’s keyring handler to the remaining MetaMask Keyring API v2 surface, adding support for getAccounts and secure private-key export via exportAccount, along with required permissions/manifest updates.
Changes:
- Implemented Keyring API v2 semantics in the keyring handler (
KeyringRpc), includinggetAccounts,exportAccount, and updatedgetAccountmissing-id behavior. - Added wallet-level raw seed export helpers and tests for hex/base58 export round-trips.
- Updated snap permissions/manifest and dependency resolution to support v2 types and keyring capabilities.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates lockfile to reflect new/updated dependency graph needed for v2 support. |
| packages/snap/tsconfig.json | Enables moduleResolution: bundler (and module: preserve) to resolve /v2 subpath types. |
| packages/snap/src/services/wallet/Wallet.ts | Adds exportKey for raw seed export in hex/base58. |
| packages/snap/src/services/wallet/Wallet.test.ts | Adds tests validating exportKey output and round-trip behavior. |
| packages/snap/src/permissions.ts | Adds v2 RPC method permissions (GetAccounts, ExportAccount) and updates selected method constants. |
| packages/snap/src/handlers/keyring/keyring.ts | Migrates dispatcher to @metamask/keyring-snap-sdk/v2, implements KeyringRpc, adds getAccounts + exportAccount, and redacts export results from debug logs. |
| packages/snap/src/handlers/keyring/keyring.test.ts | Updates tests for v2 dispatcher usage, new semantics, export behavior, and log redaction. |
| packages/snap/src/handlers/keyring/api.ts | Adds Base58Struct and handler-level request struct for exportAccount. |
| packages/snap/src/handlers/keyring/api.test.ts | Adds tests for Base58Struct. |
| packages/snap/src/context.ts | Wires AccountResolver into KeyringHandler construction. |
| packages/snap/snap.manifest.json | Declares keyring capabilities for scopes, private-key export formats, and bip44 features. |
| packages/snap/package.json | Adds @scure/base dependency for base58 encoding. |
| packages/snap/CHANGELOG.md | Documents added v2 functionality and the getAccount breaking change. |
| package.json | Pins @metamask/snaps-utils via resolutions to support capabilities validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
03fdea9 to
c267be1
Compare
| * @returns The encoded raw secret seed. | ||
| */ | ||
| exportKey(encoding: PrivateKeyEncoding): string { | ||
| const rawSeed = bufferToUint8Array(this.#signer.rawSecretKey()); |
There was a problem hiding this comment.
add try catch with new custom error to mask the root cause
|
|
||
| ### Added | ||
|
|
||
| - Private-key export (`keyring_exportAccount`) for Stellar accounts (hex/base58). |
There was a problem hiding this comment.
no need to add any change log
Migrates the keyring handler onto the remaining Keyring API v2 surface and adds private-key export. - Add exportAccount: exports the raw ed25519 secret seed as hexadecimal (default) or base58, resolved through the existing AccountResolver. The exported value is validated with a boolean guard rather than an asserting one so a validation failure can never leak the key into an error message, and the same value is redacted before it reaches the handler's debug log. - Add getAccounts (delegates to listAccounts) and make getAccount throw for an unknown id instead of returning undefined, matching the v2 contract. This is a breaking change for any caller relying on the old undefined-on-missing behavior. - Switch the keyring dispatcher to the v2 handleKeyringRequest and implement KeyringRpc instead of Keyring. - Grant the v2 RPC permissions (GetAccounts, ExportAccount for the MetaMask origin only) and declare endowment:keyring.capabilities in the manifest (scopes, export formats, bip44). - Enable moduleResolution: bundler so the /v2 subpath types resolve. - Pin @metamask/snaps-utils to an exact 12.2.0 in resolutions: endowment:keyring.capabilities validation was only added in that version, one minor above what the existing snaps-cli range resolved to. Pinned exact rather than caret because ^12.2.0 resolves to 12.2.1, which bumps @metamask/permission-controller to a 13.x major and drags in an unrelated ethersproject/ controller-utils subtree; 12.2.0 exact avoids that while still satisfying snaps-cli's own ^12.1.0 range. Exported keys are raw 32-byte seed bytes, not the Stellar S... secret-seed format - that StrKey encoding isn't expressible through the v2 exportAccount contract, so this export is not directly importable into wallets like Freighter or Lobstr. A StrKey-compatible export would need a separate, non-keyring-v2 mechanism.
c267be1 to
99030f1
Compare
| @@ -1,4 +1,6 @@ | |||
| import { PrivateKeyEncoding } from '@metamask/keyring-api/v2'; | |||
| import { hexToBytes } from '@metamask/utils'; | |||
| import { base58 } from '@scure/base'; | |||
There was a problem hiding this comment.
may consider using the same package as solona if we need base58
| result: keyringRequestResult, | ||
| // SECURITY: never debug-log the exportAccount result — it carries the | ||
| // raw private key. Every other method's result is safe to log as-is. | ||
| result: |
There was a problem hiding this comment.
just remove this debug, as we should no longer needed
| * exported private key — NEVER as an asserting validator (a StructError would | ||
| * embed the key value in its message). | ||
| */ | ||
| export const Base58Struct = define<string>( |
There was a problem hiding this comment.
please consider the same validation as solana if we need base58
| validateRequest(accountId, GetAccountRequestStruct); | ||
| const account = await this.#accountService.findById(accountId); | ||
| return account ? this.#toKeyringAccount(account) : undefined; | ||
| const { account } = await this.#accountService.resolveAccount({ |
There was a problem hiding this comment.
resolveAccount is slow , becoz it will compare the wallet address
| }, | ||
| "resolutions": { | ||
| "@metamask/snaps-sdk": "^11.1.0", | ||
| "@metamask/snaps-utils": "12.2.0", |
There was a problem hiding this comment.
why we need this bump?
Explanation
Migrates the Stellar snap's keyring handler onto the remaining MetaMask Keyring API v2 surface and adds private-key export:
exportAccount— exports the raw ed25519 secret seed as hexadecimal (default) or base58, resolved through the existingAccountResolver. The exported value is validated with a boolean guard rather than an asserting one so a validation failure can never leak the key into an error message; the same value is redacted before it reaches the handler's debug log.getAccounts(delegates tolistAccounts) and makesgetAccountthrow for an unknown id instead of returningundefined— this is a breaking change for any caller relying on the oldundefined-on-missing behavior.@metamask/keyring-snap-sdk/v2and implementsKeyringRpcinstead ofKeyring.GetAccounts,ExportAccountfor the MetaMask origin only) and manifestendowment:keyring.capabilities(scopes, export formats, bip44).moduleResolution: bundlerso the/v2subpath types resolve. NoplatformVersionchange was needed. One dependency-resolution bump was required:endowment:keyring.capabilitiesvalidation was only added in@metamask/snaps-utils@12.2.0, one minor above whatsnaps-cli's existing range resolved to, so an exactresolutionspin was added (a caret range resolves to a patch release that pulls in an unrelated major bump elsewhere in the graph, which the exact pin avoids).Exported keys are raw 32-byte seed bytes (hex/base58), not the Stellar
S…secret-seed format — that StrKey encoding isn't expressible through the v2exportAccountcontract (which only supportshexadecimal/base58over raw bytes), so this export is not directly importable into wallets like Freighter or Lobstr. Flagging this as a known limitation rather than a gap in this PR; a StrKey-compatible export would need a separate, non-keyring-v2 mechanism.References
Checklist