From 43c238bd4b488fcb2029fdb3332762ecb064fb9f Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 12 May 2026 12:49:25 +0200 Subject: [PATCH 1/3] feat(snap-account-service): add migration logic + keyring v2 support --- packages/snap-account-service/package.json | 1 + .../src/SnapAccountService.test.ts | 165 +++++++++++++++++- .../src/SnapAccountService.ts | 141 ++++++++++++++- yarn.lock | 1 + 4 files changed, 303 insertions(+), 5 deletions(-) diff --git a/packages/snap-account-service/package.json b/packages/snap-account-service/package.json index a566d7e575..388cc610ac 100644 --- a/packages/snap-account-service/package.json +++ b/packages/snap-account-service/package.json @@ -56,6 +56,7 @@ "@metamask/account-api": "^1.0.4", "@metamask/account-tree-controller": "^7.3.0", "@metamask/eth-snap-keyring": "^22.0.1", + "@metamask/keyring-api": "^23.1.0", "@metamask/keyring-controller": "^25.5.0", "@metamask/messenger": "^1.2.0", "@metamask/snaps-controllers": "^19.0.0", diff --git a/packages/snap-account-service/src/SnapAccountService.test.ts b/packages/snap-account-service/src/SnapAccountService.test.ts index e1569d63a0..9a64a0da24 100644 --- a/packages/snap-account-service/src/SnapAccountService.test.ts +++ b/packages/snap-account-service/src/SnapAccountService.test.ts @@ -1,5 +1,7 @@ import type { AccountGroupId } from '@metamask/account-api'; +import { SNAP_KEYRING_TYPE } from '@metamask/eth-snap-keyring'; import type { SnapKeyring, SnapMessage } from '@metamask/eth-snap-keyring'; +import { KeyringType } from '@metamask/keyring-api/v2'; import { KeyringControllerState, KeyringTypes, @@ -436,6 +438,7 @@ function setup({ } const MOCK_SNAP_ID = 'npm:@metamask/mock-snap' as SnapId; +const MOCK_OTHER_SNAP_ID = 'npm:@metamask/other-snap' as SnapId; describe('SnapAccountService', () => { describe('init', () => { @@ -458,6 +461,93 @@ describe('SnapAccountService', () => { }); }); + describe('migrate', () => { + it('runs the migration only once when called concurrently', async () => { + const { service, mocks } = setup(); + mocks.KeyringController.withController.mockResolvedValue(undefined); + + await Promise.all([service.migrate(), service.migrate()]); + + expect(mocks.KeyringController.withController).toHaveBeenCalledTimes(1); + }); + + it('is a no-op when no legacy Snap keyring is present', async () => { + const addNewKeyring = jest.fn().mockResolvedValue(undefined); + const removeKeyring = jest.fn().mockResolvedValue(undefined); + const { service, mocks } = setup(); + mocks.KeyringController.withController.mockImplementation( + async (operation) => + operation({ keyrings: [], addNewKeyring, removeKeyring }), + ); + + await service.migrate(); + + expect(addNewKeyring).not.toHaveBeenCalled(); + expect(removeKeyring).not.toHaveBeenCalled(); + }); + + it('migrates accounts from the legacy Snap keyring to per-Snap v2 keyrings and removes the legacy entry', async () => { + const addNewKeyring = jest.fn().mockResolvedValue(undefined); + const removeKeyring = jest.fn().mockResolvedValue(undefined); + const legacyKeyringId = 'legacy-keyring-id'; + const account1 = { + id: 'account-1', + address: '0x1', + metadata: { snap: { id: MOCK_SNAP_ID } }, + }; + const account2 = { + id: 'account-2', + address: '0x2', + metadata: { snap: { id: MOCK_SNAP_ID } }, + }; + const account3 = { + id: 'account-3', + address: '0x3', + metadata: { snap: { id: MOCK_OTHER_SNAP_ID } }, + }; + const orphanAccount = { + id: 'orphan', + address: '0x4', + metadata: {}, + }; + const legacyKeyring = { + type: SNAP_KEYRING_TYPE, + listAccounts: jest + .fn() + .mockReturnValue([account1, account2, account3, orphanAccount]), + }; + const { service, mocks } = setup(); + mocks.KeyringController.withController.mockImplementation( + async (operation) => + operation({ + keyrings: [ + { keyring: legacyKeyring, metadata: { id: legacyKeyringId } }, + ], + addNewKeyring, + removeKeyring, + }), + ); + + await service.migrate(); + + expect(addNewKeyring).toHaveBeenCalledTimes(2); + expect(addNewKeyring).toHaveBeenCalledWith(KeyringType.Snap, { + snapId: MOCK_SNAP_ID, + accounts: { + [account1.id]: { id: account1.id, address: account1.address }, + [account2.id]: { id: account2.id, address: account2.address }, + }, + }); + expect(addNewKeyring).toHaveBeenCalledWith(KeyringType.Snap, { + snapId: MOCK_OTHER_SNAP_ID, + accounts: { + [account3.id]: { id: account3.id, address: account3.address }, + }, + }); + expect(removeKeyring).toHaveBeenCalledWith(legacyKeyringId); + }); + }); + describe('ensureReady', () => { it('throws when the Snap is not tracked', async () => { const { service } = setup(); @@ -489,6 +579,73 @@ describe('SnapAccountService', () => { expect(await service.ensureReady(MOCK_SNAP_ID)).toBeUndefined(); }); + it('runs the migration before checking platform readiness', async () => { + const { service, mocks } = setup({ + runnableSnaps: [buildSnap(MOCK_SNAP_ID, true)], + }); + mocks.KeyringController.withController.mockResolvedValue(undefined); + + await service.init(); + // `migrate` is invoked once + `#createKeyringForSnap` is invoked once + // (the cached migrate call is a no-op on subsequent calls). + await service.ensureReady(MOCK_SNAP_ID); + + expect(mocks.KeyringController.withController).toHaveBeenCalledTimes(2); + }); + + it('creates a v2 keyring for the Snap when one does not exist yet', async () => { + const addNewKeyring = jest.fn().mockResolvedValue(undefined); + const removeKeyring = jest.fn().mockResolvedValue(undefined); + const { service, mocks } = setup({ + runnableSnaps: [buildSnap(MOCK_SNAP_ID, true)], + }); + mocks.KeyringController.withController.mockImplementation( + async (operation) => + operation({ keyrings: [], addNewKeyring, removeKeyring }), + ); + + await service.init(); + await service.ensureReady(MOCK_SNAP_ID); + + expect(addNewKeyring).toHaveBeenCalledWith(KeyringType.Snap, { + snapId: MOCK_SNAP_ID, + accounts: {}, + }); + }); + + it('does not create a v2 keyring when one already exists for the Snap', async () => { + const addNewKeyring = jest.fn().mockResolvedValue(undefined); + const removeKeyring = jest.fn().mockResolvedValue(undefined); + const { service, mocks } = setup({ + runnableSnaps: [buildSnap(MOCK_SNAP_ID, true)], + }); + // First call: migration (no legacy snap keyring → early return). + // Second call: ensureReady keyring check (snap keyring already exists). + mocks.KeyringController.withController + .mockImplementationOnce(async (operation) => + operation({ keyrings: [], addNewKeyring, removeKeyring }), + ) + .mockImplementationOnce(async (operation) => + operation({ + keyrings: [ + { + keyringV2: { + type: KeyringType.Snap, + snapId: MOCK_SNAP_ID, + }, + }, + ], + addNewKeyring, + removeKeyring, + }), + ); + + await service.init(); + await service.ensureReady(MOCK_SNAP_ID); + + expect(addNewKeyring).not.toHaveBeenCalled(); + }); + it('waits for the Snap platform to become ready', async () => { const { service, rootMessenger } = setup({ snapIsReady: false, @@ -525,9 +682,9 @@ describe('SnapAccountService', () => { return undefined; }); - // Flush microtasks so #waitForSnapKeyring subscribes. - await Promise.resolve(); - await Promise.resolve(); + // Flush microtasks so migration completes and #waitForSnapKeyring + // subscribes. + await flushMicrotasks(); expect(resolved).toBe(false); @@ -584,7 +741,7 @@ describe('SnapAccountService', () => { return undefined; }); - await Promise.resolve(); + await flushMicrotasks(); expect(ensureOnboardingComplete).toHaveBeenCalledTimes(1); expect(resolved).toBe(false); diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index f8ad28f0b0..c2c7b4af02 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -3,6 +3,8 @@ import type { SnapKeyring as LegacySnapKeyring, SnapMessage, } from '@metamask/eth-snap-keyring'; +import { SnapKeyring, SnapKeyringState } from '@metamask/eth-snap-keyring/v2'; +import { Keyring, KeyringType } from '@metamask/keyring-api/v2'; import type { KeyringControllerGetStateAction, KeyringControllerStateChangeEvent, @@ -59,6 +61,7 @@ export const serviceName = 'SnapAccountService'; * the messenger. */ const MESSENGER_EXPOSED_METHODS = [ + 'migrate', 'ensureReady', 'getSnaps', 'getLegacySnapKeyring', @@ -147,6 +150,17 @@ function isLegacySnapKeyring(keyring: { return keyring.type === KeyringTypes.snap; } +/** + * Checks if a given keyring is a Snap keyring (v2). + * + * @param keyring - The keyring to check. + * @returns `true` if the keyring is a Snap keyring (v2), `false` otherwise. + */ +function isSnapKeyring(keyring: Keyring): keyring is SnapKeyring { + // Using `KeyringType.Snap` (used for v2). + return keyring.type === KeyringType.Snap; +} + /** * Service responsible for managing account management snaps. */ @@ -162,6 +176,8 @@ export class SnapAccountService { readonly #tracker: SnapTracker; + #migratePromise: Promise | null = null; + /** * Constructs a new {@link SnapAccountService}. * @@ -285,7 +301,11 @@ export class SnapAccountService { /** * Ensures everything is ready to use Snap accounts for the given Snap. * 1. Validates that `snapId` is a tracked account-management Snap. - * 2. Waits for the Snap platform to be fully started. + * 2. Runs the legacy -> v2 Snap keyring migration (cached — no-op if + * already done). + * 3. Atomically creates the v2 keyring for this Snap if it doesn't exist + * yet. + * 4. Waits for the Snap platform to be fully started. * * Safe to call concurrently — each step is idempotent or mutex-protected. * @@ -296,11 +316,130 @@ export class SnapAccountService { if (!this.#tracker.canUse(snapId)) { throw new Error(`Unknown snap: "${snapId}"`); } + + // Migrate from the global v1 Snap keyring to the per-Snap v2 keyring + // before doing anything else. + await this.migrate(); + + // We still try to create the keyring for the Snap here, since we might + // want to use a new Snap that never had accounts before. + await this.#createKeyringForSnap(snapId); + // Before doing anything with our Snap, we need to make sure the platform // is ready to process requests. await this.#watcher.ensureCanUseSnapPlatform(); } + /** + * Migrate the legacy Snap keyring to the new (per-snap) Snap keyring v2. + * Safe to call concurrently — the migration runs only once; all callers + * await the same promise. + * + * @returns A promise that resolves when the migration is complete. + */ + async migrate(): Promise { + if (!this.#migratePromise) { + this.#migratePromise = this.#migrate(); + } + return await this.#migratePromise; + } + + /** + * Performs the actual migration logic. Should only be called once, and is not + * safe to call concurrently. + */ + async #migrate(): Promise { + log('Migration started...'); + + await this.#messenger.call( + 'KeyringController:withController', + async (controller) => { + const { keyrings } = controller; + + const legacySnapKeyringEntry = keyrings.find(({ keyring }) => + isLegacySnapKeyring(keyring), + ); + if (!legacySnapKeyringEntry) { + log('No legacy Snap keyring found. Migration not required.'); + return; + } + + // The legacy Snap keyring has never been a true `EthKeyring` so we + // need to cast it to `unknown` first. + const legacySnapKeyring = + legacySnapKeyringEntry.keyring as unknown as LegacySnapKeyring; + + // Compute the account list for each Snap, grouped by snap ID. + const states = new Map(); + for (const internalAccount of legacySnapKeyring.listAccounts()) { + // Convert `InternalAccount` to `KeyringAccount` since the Snap + // keyring (v2) expects accounts in that format and will verify it + // with `superstruct` when adding the keyring. + const { metadata, ...account } = internalAccount; + + const snap = metadata?.snap; + if (snap) { + const snapId = snap.id as SnapId; + + let state = states.get(snapId); + if (!state) { + state = { snapId, accounts: {} }; + states.set(snapId, state); + } + state.accounts[account.id] = account; + } + } + + // Create the new Snap keyring (v2) for each Snap and migrate the + // accounts over. + for (const state of states.values()) { + log(`Migrating accounts for Snap "${state.snapId}"...`); + await controller.addNewKeyring( + // IMPORTANT: The Snap keyring (v2) can also be used as a v1 + // keyring. So the builder associated with the v2 keyring type is + // able to build both v1 and v2 keyrings. + KeyringType.Snap, + state, + ); + } + + // Remove the legacy Snap keyring after migration. + log('Removing legacy Snap keyring...'); + await controller.removeKeyring(legacySnapKeyringEntry.metadata.id); + }, + ); + + log('Migration completed!'); + } + + /** + * Creates a Snap keyring for the given Snap if it doesn't exist yet using + * a safe "get or create" pattern. Safe to call concurrently. + * + * @param snapId - The Snap ID to create the keyring for. + */ + async #createKeyringForSnap(snapId: SnapId): Promise { + await this.#messenger.call( + 'KeyringController:withController', + async (controller) => { + const hasKeyring = controller.keyrings.some( + ({ keyringV2 }) => + keyringV2 && + isSnapKeyring(keyringV2) && + keyringV2.snapId === snapId, + ); + + if (!hasKeyring) { + log(`Creating v2 keyring for Snap "${snapId}"...`); + await controller.addNewKeyring(KeyringType.Snap, { + snapId, + accounts: {}, + }); + } + }, + ); + } + /** * Atomically gets-or-creates the legacy (v1) Snap keyring — the keyring * associated with {@link KeyringTypes.snap}. diff --git a/yarn.lock b/yarn.lock index 4c2c3023aa..5ed0e6dd24 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5457,6 +5457,7 @@ __metadata: "@metamask/account-tree-controller": "npm:^7.3.0" "@metamask/auto-changelog": "npm:^6.1.0" "@metamask/eth-snap-keyring": "npm:^22.0.1" + "@metamask/keyring-api": "npm:^23.1.0" "@metamask/keyring-controller": "npm:^25.5.0" "@metamask/keyring-utils": "npm:^3.2.1" "@metamask/messenger": "npm:^1.2.0" From aa7939093bbfe2be493c3277a5cf7a626c46a1d5 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 12 May 2026 12:52:40 +0200 Subject: [PATCH 2/3] chore: cosmetic --- .../snap-account-service/src/SnapAccountService.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index c2c7b4af02..610e76c820 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -323,7 +323,7 @@ export class SnapAccountService { // We still try to create the keyring for the Snap here, since we might // want to use a new Snap that never had accounts before. - await this.#createKeyringForSnap(snapId); + await this.#ensureKeyringIsReady(snapId); // Before doing anything with our Snap, we need to make sure the platform // is ready to process requests. @@ -413,12 +413,12 @@ export class SnapAccountService { } /** - * Creates a Snap keyring for the given Snap if it doesn't exist yet using - * a safe "get or create" pattern. Safe to call concurrently. + * Ensures a Snap keyring is ready for the given Snap. If it doesn't exist yet, it will be created. + * Safe to call concurrently. * - * @param snapId - The Snap ID to create the keyring for. + * @param snapId - The Snap ID to ensure the keyring is ready for. */ - async #createKeyringForSnap(snapId: SnapId): Promise { + async #ensureKeyringIsReady(snapId: SnapId): Promise { await this.#messenger.call( 'KeyringController:withController', async (controller) => { From 8452267f341aefa76975727a1412f5300358bfed Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 12 May 2026 12:56:57 +0200 Subject: [PATCH 3/3] chore: changelog --- packages/snap-account-service/CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/snap-account-service/CHANGELOG.md b/packages/snap-account-service/CHANGELOG.md index 2d89203d8f..784f37b72a 100644 --- a/packages/snap-account-service/CHANGELOG.md +++ b/packages/snap-account-service/CHANGELOG.md @@ -10,11 +10,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add `SnapAccountService` ([#8414](https://github.com/MetaMask/core/pull/8414)) -- Add `SnapPlatformWatcher` and `SnapAccountService.ensureReady` ([#8715](https://github.com/MetaMask/core/pull/8715)), ([#8725](https://github.com/MetaMask/core/pull/8725)) +- Add `SnapPlatformWatcher` and `SnapAccountService.ensureReady` ([#8715](https://github.com/MetaMask/core/pull/8715)), ([#8725](https://github.com/MetaMask/core/pull/8725)), ([#8732](https://github.com/MetaMask/core/pull/8732)) - Waits for the Snap platform to be ready and for a Snap keyring to appear in `KeyringController` state before allowing Snap account operations. - Callers must ensure `init()` has run and the Snap is currently installed, enabled, non-blocked, and declares `endowment:keyring`. - `SnapAccountService.ensureReady` now awaits the watcher, so it only resolves once both conditions hold (or rejects if the Snap keyring does not appear within the configured timeout). - `SnapAccountService.ensureReady` now throws `Unknown snap: ""` when called with a Snap ID that isn't tracked as an account-management Snap. + - `SnapAccountService.ensureReady` now automatically creates the Snap keyring (v2) for a given Snap ID if it was not available. - Add `config` option to `SnapAccountService` constructor with a `snapPlatformWatcher` field exposing `ensureOnboardingComplete` and `snapKeyringWaitTimeoutMs` ([#8715](https://github.com/MetaMask/core/pull/8715)) - Export `SnapAccountServiceConfig` and `SnapPlatformWatcherConfig` types. - Add `@metamask/keyring-controller` dependency ([#8715](https://github.com/MetaMask/core/pull/8715)) @@ -32,6 +33,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - This logic used to live on the clients. - The service messenger now requires the `KeyringController:unlock`, `AccountTreeController:selectedAccountGroupChange`, `AccountTreeController:accountGroup{Created,Updated,Removed}` events. - The service messenger now requires the `AccountTreeController:getSelectedAccountGroup` and `AccountTreeController:getAccountGroupObject` actions. +- Add `migrate` ([#8732](https://github.com/MetaMask/core/pull/8732)) + - The migration is guaranteed to be run when using `ensureReady`. + - It is conccurent-free and can safely be called by multiple execution flows. + - Once the migration has ran, the legacy Snap keyring will be emptied, thus, consumers are expected to use the new per-Snap keyring (v2) instances instead. ### Changed