Skip to content

feat(snap-account-service): add SnapAccountService#8414

Open
ccharly wants to merge 28 commits intomainfrom
cc/feat/snap-account-service
Open

feat(snap-account-service): add SnapAccountService#8414
ccharly wants to merge 28 commits intomainfrom
cc/feat/snap-account-service

Conversation

@ccharly
Copy link
Copy Markdown
Contributor

@ccharly ccharly commented Apr 9, 2026

Explanation

A new service where we will move the common logic of the current monolith SnapKeyring.

The idea is to be able to split this keyring into multiple ones and move the KeyringEvents logic in this service.

High-level architecture is the following:

Screenshot 2026-04-09 at 14 49 23

For now, it's just an empty service, but we will move forward with more follow-up change as we are rewriting the existing SnapKeyring to use the new KeyringV2.

References

N/A

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed
  • I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Adds a new atomic multi-keyring transaction API in KeyringController that can create/remove keyrings and persists or rolls back vault updates, which is a sensitive area for account storage consistency. The new @metamask/snap-account-service package is mostly scaffolding and low risk on its own.

Overview
Introduces a new workspace package, @metamask/snap-account-service, with initial service scaffolding (SnapAccountService) and basic test/config/docs, and wires it into the monorepo (tsconfigs, README, yarn workspace, CODEOWNERS/teams).

Extends @metamask/keyring-controller with a new withController messenger action/method to run atomic, mutex-protected operations across all keyrings via a RestrictedController that exposes a live read-only view plus staged addNewKeyring/removeKeyring; on success it commits + persists, and on error it rolls back and cleans up created/removed keyrings. Adds extensive tests for locking behavior, commit/rollback, immediate visibility, and unsafe direct keyring-return protection.

Reviewed by Cursor Bugbot for commit 2963d98. Bugbot is set up for automated code reviews on this repo. Configure here.

@ccharly ccharly changed the title feat(snap-account-service): add SnapAccountService feat(snap-account-service): add SnapAccountService Apr 9, 2026
@ccharly ccharly marked this pull request as ready for review April 9, 2026 15:35
@ccharly ccharly requested a review from a team as a code owner April 9, 2026 15:35
@ccharly ccharly enabled auto-merge April 13, 2026 11:59
@@ -0,0 +1,15 @@
# `@metamask/snap-account-service`

Service for Account Management Snaps
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have little bit more explanation here? For example, what is the main purpose of this service, what we should keep or implement here. So we build up some integrity of this package.

* Initializes the snap account service.
*/
async init(): Promise<void> {
// TODO: Add initialization logic here.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is this supposed to be implemented? Looks redundant at this moment.

@ccharly ccharly force-pushed the cc/feat/snap-account-service branch from f570e75 to 2963d98 Compare April 14, 2026 11:30
@ccharly ccharly requested a review from a team as a code owner April 14, 2026 11:30
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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 2963d98. Configure here.

keyring: SelectedKeyring;
metadata: KeyringMetadata;
}) => Promise<CallbackResult>,
operation: ({ keyring, metadata }: KeyringEntry) => Promise<CallbackResult>,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overloads lose generic narrowing and expose phantom property

Medium Severity

The two public withKeyring overloads were refactored from { keyring: SelectedKeyring; metadata: KeyringMetadata } to KeyringEntry. This introduces two problems: (1) the callback's keyring is now typed as EthKeyring instead of SelectedKeyring, silently dropping the generic type narrowing that the filter type guard in KeyringSelector was designed to propagate; and (2) the callback type now includes keyringV2?: KeyringV2, but the implementation only passes { keyring, metadata }, so keyringV2 is always undefined at runtime — a misleading type that can lead to dead code branches.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2963d98. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants