Skip to content

feat(snap-account-service): add migration logic + keyring v2 support#8732

Open
ccharly wants to merge 8 commits into
cc/feat/snap-account-service-account-group-eventsfrom
cc/feat/snap-account-service-keyring-v2-support
Open

feat(snap-account-service): add migration logic + keyring v2 support#8732
ccharly wants to merge 8 commits into
cc/feat/snap-account-service-account-group-eventsfrom
cc/feat/snap-account-service-keyring-v2-support

Conversation

@ccharly
Copy link
Copy Markdown
Contributor

@ccharly ccharly commented May 7, 2026

Explanation

Add migration logic to migrate from the legacy Snap keyring to the new architecture with 1 Snap keyring v2 per Snaps.

Also enhance :ensureReady so consumers can safely call :withKeyringV2 to get the associated Snap keyring v2 instance before using it.

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

High Risk
High risk because it changes Snap account/keyring lifecycle (migration, auto keyring creation, and message routing) which directly affects account persistence and Snap-driven account operations.

Overview
Implements a legacy Snap keyring → per-Snap v2 keyring migration in SnapAccountService (migrate), designed to be concurrent-safe, retried on failure, and automatically invoked by ensureReady.

Updates ensureReady to run migration first and then atomically create the v2 keyring for a Snap if missing before waiting on platform readiness; handleKeyringSnapMessage is switched to forward messages via KeyringController:withKeyringV2 (with special-casing for GetSelectedAccounts and auto-create on AccountCreated).

In multichain-account-service, Snap-based providers drop KeyringController:withKeyring usage entirely: BaseBip44AccountProvider removes the v1 helper, SnapAccountProvider now targets eth-snap-keyring/v2 APIs (including deleteAccount), and BTC/SOL/TRX/Snap provider tests are updated accordingly; changelogs and dependencies are bumped to reflect these breaking API shifts.

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

Base automatically changed from cc/feat/snap-account-service-track-snaps to main May 7, 2026 15:23
@ccharly ccharly force-pushed the cc/feat/snap-account-service-keyring-v2-support branch from 96269d0 to 43c238b Compare May 12, 2026 10:50
@ccharly ccharly changed the base branch from main to cc/feat/snap-account-service-account-group-events May 12, 2026 10:50
@ccharly ccharly changed the title feat(snap-account-service): add keyring v2 support feat(snap-account-service): add migration logic + keyring v2 support May 12, 2026
@ccharly ccharly marked this pull request as ready for review May 12, 2026 10:57
@ccharly ccharly requested review from a team as code owners May 12, 2026 10:57
Comment thread packages/snap-account-service/src/SnapAccountService.ts
@ccharly ccharly force-pushed the cc/feat/snap-account-service-keyring-v2-support branch from 8452267 to ce71df1 Compare May 12, 2026 13:21
Comment thread packages/multichain-account-service/src/providers/SnapAccountProvider.ts Outdated
Comment thread packages/multichain-account-service/src/providers/SnapAccountProvider.ts Outdated
Comment thread packages/snap-account-service/src/SnapAccountService.ts
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 2adb638. Configure here.


// Remove the legacy Snap keyring after migration.
log('Removing legacy Snap keyring...');
await controller.removeKeyring(legacySnapKeyringEntry.metadata.id);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Migration breaks proactive selected account forwarding to snaps

Medium Severity

The #migrate() method removes the legacy snap keyring via controller.removeKeyring(...), but #forwardSelectedAccounts — which is wired to fire on selectedAccountGroupChange, unlock, and account group lifecycle events — still calls getLegacySnapKeyring(). That method is get-or-create, so after migration it silently spins up a new, empty legacy keyring and calls setSelectedAccounts on it. Since no snaps are connected to this empty keyring, proactive selected-account notifications stop reaching any snap after migration completes.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2adb638. 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.

1 participant