Implement Keyring API v2 - #151
Open
jeremytsng wants to merge 1 commit into
Open
Conversation
jeremytsng
force-pushed
the
feat/stellar-keyring-v2
branch
from
July 2, 2026 11:32
529fdab to
00fcbf1
Compare
jeremytsng
changed the title
jeremytsng
marked this pull request as ready for review
July 2, 2026 11:46
jeremytsng
force-pushed
the
feat/stellar-keyring-v2
branch
from
July 2, 2026 11:50
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:
|
Contributor
Author
|
@SocketSecurity ignore-all |
Contributor
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.
jeremytsng
force-pushed
the
feat/stellar-keyring-v2
branch
from
July 2, 2026 17:30
03fdea9 to
c267be1
Compare
jeremytsng
force-pushed
the
feat/stellar-keyring-v2
branch
from
July 3, 2026 06:14
c267be1 to
99030f1
Compare
jeremytsng
force-pushed
the
feat/stellar-keyring-v2
branch
3 times, most recently
from
July 31, 2026 06:06
74127c4 to
cd42af5
Compare
jeremytsng
force-pushed
the
feat/stellar-keyring-v2
branch
from
July 31, 2026 06:24
cd42af5 to
d2f45f1
Compare
| }, | ||
| "resolutions": { | ||
| "@metamask/snaps-sdk": "11.1.1", | ||
| "@metamask/snaps-utils": "12.2.0", |
Collaborator
There was a problem hiding this comment.
this is not a must for keyring API
| return this.listAccounts(); | ||
| } | ||
|
|
||
| async createAccount(options?: CreateAccountOptions): Promise<KeyringAccount> { |
Collaborator
There was a problem hiding this comment.
no longer needed
can also remove account service create()
|
|
||
| if (options.type === AccountCreationType.Bip44DeriveIndex) { | ||
| if (options.type === AccountCreationType.Bip44Discover) { | ||
| const discovered = await this.discoverAccounts( |
Collaborator
There was a problem hiding this comment.
let do it in that way
let range;
// For discovery, only create the account if it has on-chain activity. No
// activity means we've reached the end of the discoverable accounts, so we
// return nothing and the client stops discovering.
if (
options.type === AccountCreationType.Bip44Discover &&
!await this.#hasOnChainActivity(
[AppConfig.selectedNetwork],
options.entropySource,
options.groupIndex,
);
) {
return []
}
if (options.type === AccountCreationType.Bip44DeriveIndexRange) {
range = options.range;
} else {
// Bip44DeriveIndex | Bip44Discover — a single group index. Ranges are
// inclusive, so `from` and `to` are the same.
range = { from: options.groupIndex, to: options.groupIndex };
}
const createdAccounts = await this.#accountService.batchCreate({
entropySource: options.entropySource,
fromIndex: options.range.from,
toIndex: options.range.to,
});
accounts = createdAccounts.map((account) =>
this.#toKeyringAccount(account),
);
change discoverAccounts to #hasOnChainActivity
|
|
||
| async getAccountBalances( | ||
| accountId: string, | ||
| assets: KnownCaip19AssetIdOrSlip44Id[], |
Collaborator
There was a problem hiding this comment.
seem this change is no need?
| async resolveAccountAddress( | ||
| scope: KnownCaip2ChainId, | ||
| request: ResolveAccountAddressJsonRpcRequest, | ||
| scope: CaipChainId, |
Collaborator
There was a problem hiding this comment.
lets keep the type
| } | ||
|
|
||
| /** | ||
| * Emits the account-created event to the wallet. |
Collaborator
There was a problem hiding this comment.
change #handleSubmitRequest to
async submitRequest(request: KeyringRequest): Promise<KeyringResponse>
and remove the old submitRequest
Comment on lines
-128
to
-132
| this.#logger.debug('Keyring request handled', { | ||
| origin, | ||
| method: request.method, | ||
| result: keyringRequestResult, | ||
| }); |
Collaborator
There was a problem hiding this comment.
i need this debug
| } | ||
|
|
||
| async getAccounts(): Promise<KeyringAccount[]> { | ||
| return this.listAccounts(); |
Collaborator
There was a problem hiding this comment.
dont use the org listAccounts, just move the listAccounts logic here
| @@ -39,6 +43,9 @@ const metamaskPermissions = new Set([ | |||
| KeyringRpcMethod.ListAccountAssets, | |||
| KeyringRpcMethod.ResolveAccountAddress, | |||
| KeyringRpcMethod.SetSelectedAccounts, | |||
Collaborator
There was a problem hiding this comment.
| "allowedOrigins": ["https://portfolio.metamask.io"] | ||
| "allowedOrigins": ["https://portfolio.metamask.io"], | ||
| "capabilities": { | ||
| "scopes": ["stellar:pubnet", "stellar:testnet"], |
Collaborator
There was a problem hiding this comment.
no testnet enable
| /** | ||
| * Emits the account-created event to the wallet. | ||
| * This triggers the wallet to prompt the user to add the account. | ||
| * If the user accepts, the account is added; if the user rejects, an error is thrown. |
Collaborator
There was a problem hiding this comment.
account delete no longer have emitEvent
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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