diff --git a/README.md b/README.md index b163294787..d2810cadd5 100644 --- a/README.md +++ b/README.md @@ -509,6 +509,7 @@ linkStyle default opacity:0.5 signature_controller --> logging_controller; signature_controller --> messenger; signature_controller --> network_controller; + snap_account_service --> account_tree_controller; snap_account_service --> keyring_controller; snap_account_service --> messenger; social_controllers --> base_controller; diff --git a/packages/snap-account-service/CHANGELOG.md b/packages/snap-account-service/CHANGELOG.md index 6dbce3c074..8aae4645b9 100644 --- a/packages/snap-account-service/CHANGELOG.md +++ b/packages/snap-account-service/CHANGELOG.md @@ -28,6 +28,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - The service messenger now requires the `KeyringController:withController` action. - Add `handleKeyringSnapMessage` ([#8758](https://github.com/MetaMask/core/pull/8758)) - This will be the new entry point for consumer that needs to forward keyring events to a account management Snap (instead of using the legacy Snap keyring instance directly). +- Forward selected account group accounts ([#8763](https://github.com/MetaMask/core/pull/8763)) + - This logic used to live on the clients. + - The service messenger now requires the `KeyringController:unlock`, `AccountTreeController:selectedAccountGroupChange` events and `AccountTreeController:getAccountGroupObject` action. ### Changed diff --git a/packages/snap-account-service/package.json b/packages/snap-account-service/package.json index cc1e974237..a566d7e575 100644 --- a/packages/snap-account-service/package.json +++ b/packages/snap-account-service/package.json @@ -53,6 +53,8 @@ "test:watch": "NODE_OPTIONS=--experimental-vm-modules jest --watch" }, "dependencies": { + "@metamask/account-api": "^1.0.4", + "@metamask/account-tree-controller": "^7.3.0", "@metamask/eth-snap-keyring": "^22.0.1", "@metamask/keyring-controller": "^25.5.0", "@metamask/messenger": "^1.2.0", diff --git a/packages/snap-account-service/src/SnapAccountService.test.ts b/packages/snap-account-service/src/SnapAccountService.test.ts index 4b85f2a863..871206f85d 100644 --- a/packages/snap-account-service/src/SnapAccountService.test.ts +++ b/packages/snap-account-service/src/SnapAccountService.test.ts @@ -1,3 +1,4 @@ +import type { AccountGroupId } from '@metamask/account-api'; import type { SnapKeyring, SnapMessage } from '@metamask/eth-snap-keyring'; import { KeyringControllerState, @@ -22,6 +23,7 @@ import type { SnapAccountServiceOptions, } from './SnapAccountService'; import { SnapAccountService } from './SnapAccountService'; +import type { AccountGroupObject } from './types'; type RootMessenger = Messenger< MockAnyNamespace, @@ -49,6 +51,13 @@ type Mocks = { getState: jest.MockedFunction<() => { keyrings: { type: string }[] }>; withController: jest.Mock; }; + // eslint-disable-next-line @typescript-eslint/naming-convention + AccountTreeController: { + getAccountGroupObject: jest.MockedFunction< + (groupId: AccountGroupId) => AccountGroupObject | undefined + >; + getSelectedAccountGroup: jest.MockedFunction<() => AccountGroupId | ''>; + }; }; /** @@ -82,6 +91,8 @@ function getMessenger( 'SnapController:getRunnableSnaps', 'KeyringController:getState', 'KeyringController:withController', + 'AccountTreeController:getAccountGroupObject', + 'AccountTreeController:getSelectedAccountGroup', ], events: [ 'SnapController:stateChange', @@ -92,6 +103,8 @@ function getMessenger( 'SnapController:snapUnblocked', 'SnapController:snapUninstalled', 'KeyringController:stateChange', + 'KeyringController:unlock', + 'AccountTreeController:selectedAccountGroupChange', ], }); return messenger; @@ -131,6 +144,45 @@ function publishKeyrings( ); } +/** + * Flushes pending microtasks so that chained `await`s in fire-and-forget + * handlers resolve before assertions run. + * + * @returns A promise that resolves once the event loop has drained. + */ +async function flushMicrotasks(): Promise { + await new Promise((resolve) => setImmediate(resolve)); +} + +/** + * Publishes an AccountTreeController selectedAccountGroupChange event on the + * root messenger. + * + * @param rootMessenger - The root messenger. + * @param next - The newly selected account group ID (or ''). + * @param previous - The previously selected account group ID (or ''). + */ +function publishSelectedAccountGroupChange( + rootMessenger: RootMessenger, + next: AccountGroupId | '', + previous: AccountGroupId | '' = '', +): void { + rootMessenger.publish( + 'AccountTreeController:selectedAccountGroupChange', + next, + previous, + ); +} + +/** + * Publishes a KeyringController unlock event on the root messenger. + * + * @param rootMessenger - The root messenger. + */ +function publishUnlock(rootMessenger: RootMessenger): void { + rootMessenger.publish('KeyringController:unlock'); +} + /** * Builds a minimal `TruncatedSnap` for tests. * @@ -147,6 +199,16 @@ function buildSnap(id: string, hasKeyring: boolean): TruncatedSnap { } as MockTruncatedSnap as TruncatedSnap; } +/** + * Builds a minimal `AccountGroupObject` for tests. + * + * @param accounts - The list of account IDs in the group. + * @returns A minimal `AccountGroupObject`. + */ +function buildGroup(accounts: string[]): AccountGroupObject { + return { accounts } as unknown as AccountGroupObject; +} + /** * Builds a fake {@link KeyringEntry} with the given type. * @@ -199,20 +261,26 @@ function mockWithController( * @param mocks - The mocks object from {@link setup}. * @param keyring - The mocked Snap keyring methods. * @param keyring.handleKeyringSnapMessage - The mocked implementation. + * @param keyring.setSelectedAccounts - The mocked implementation. */ function mockLegacySnapKeyring( mocks: Mocks, { handleKeyringSnapMessage, + setSelectedAccounts, }: { - handleKeyringSnapMessage: jest.MockedFunction< + handleKeyringSnapMessage?: jest.MockedFunction< SnapKeyring['handleKeyringSnapMessage'] >; + setSelectedAccounts?: jest.MockedFunction< + SnapKeyring['setSelectedAccounts'] + >; }, ): void { const snapKeyring = { type: KeyringTypes.snap, handleKeyringSnapMessage, + setSelectedAccounts, }; mocks.KeyringController.withController.mockImplementation(async (operation) => operation({ @@ -270,6 +338,10 @@ function setup({ getState: jest.fn().mockReturnValue({ keyrings }), withController: jest.fn(), }, + AccountTreeController: { + getAccountGroupObject: jest.fn().mockReturnValue(undefined), + getSelectedAccountGroup: jest.fn().mockReturnValue(''), + }, }; rootMessenger.registerActionHandler( @@ -288,6 +360,14 @@ function setup({ 'KeyringController:withController', mocks.KeyringController.withController, ); + rootMessenger.registerActionHandler( + 'AccountTreeController:getAccountGroupObject', + mocks.AccountTreeController.getAccountGroupObject, + ); + rootMessenger.registerActionHandler( + 'AccountTreeController:getSelectedAccountGroup', + mocks.AccountTreeController.getSelectedAccountGroup, + ); const service = new SnapAccountService({ messenger, config }); @@ -549,4 +629,164 @@ describe('SnapAccountService', () => { expect(result).toBe('pong'); }); }); + + describe('on AccountTreeController:selectedAccountGroupChange', () => { + const MOCK_GROUP_ID = 'keyring:01JABC/group-1' as AccountGroupId; + const MOCK_ACCOUNTS = [ + '00000000-0000-0000-0000-000000000001', + '00000000-0000-0000-0000-000000000002', + ]; + + it('forwards the selected accounts to the Snap keyring', async () => { + const { service, rootMessenger, mocks } = setup(); + const setSelectedAccounts = jest.fn().mockResolvedValue(undefined); + mockLegacySnapKeyring(mocks, { setSelectedAccounts }); + mocks.AccountTreeController.getAccountGroupObject.mockReturnValue( + buildGroup(MOCK_ACCOUNTS), + ); + expect(service).toBeDefined(); + + publishSelectedAccountGroupChange(rootMessenger, MOCK_GROUP_ID); + await flushMicrotasks(); + + expect( + mocks.AccountTreeController.getAccountGroupObject, + ).toHaveBeenCalledWith(MOCK_GROUP_ID); + expect(setSelectedAccounts).toHaveBeenCalledWith(MOCK_ACCOUNTS); + }); + + it('does nothing when the new group ID is empty', async () => { + const { service, rootMessenger, mocks } = setup(); + const setSelectedAccounts = jest.fn().mockResolvedValue(undefined); + mockLegacySnapKeyring(mocks, { setSelectedAccounts }); + expect(service).toBeDefined(); + + publishSelectedAccountGroupChange(rootMessenger, ''); + await flushMicrotasks(); + + expect( + mocks.AccountTreeController.getAccountGroupObject, + ).not.toHaveBeenCalled(); + expect(setSelectedAccounts).not.toHaveBeenCalled(); + }); + + it('does nothing when the account group is not found', async () => { + const { service, rootMessenger, mocks } = setup(); + const setSelectedAccounts = jest.fn().mockResolvedValue(undefined); + mockLegacySnapKeyring(mocks, { setSelectedAccounts }); + mocks.AccountTreeController.getAccountGroupObject.mockReturnValue( + undefined, + ); + expect(service).toBeDefined(); + + publishSelectedAccountGroupChange(rootMessenger, MOCK_GROUP_ID); + await flushMicrotasks(); + + expect( + mocks.AccountTreeController.getAccountGroupObject, + ).toHaveBeenCalledWith(MOCK_GROUP_ID); + expect(setSelectedAccounts).not.toHaveBeenCalled(); + }); + + it('logs an error when forwarding to the Snap keyring fails', async () => { + const { service, rootMessenger, mocks } = setup(); + const error = new Error('forward boom'); + const setSelectedAccounts = jest.fn().mockRejectedValue(error); + mockLegacySnapKeyring(mocks, { setSelectedAccounts }); + mocks.AccountTreeController.getAccountGroupObject.mockReturnValue( + buildGroup(MOCK_ACCOUNTS), + ); + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + expect(service).toBeDefined(); + + publishSelectedAccountGroupChange(rootMessenger, MOCK_GROUP_ID); + await flushMicrotasks(); + + expect(setSelectedAccounts).toHaveBeenCalledWith(MOCK_ACCOUNTS); + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error handling selected account group change:', + error, + ); + + consoleErrorSpy.mockRestore(); + }); + }); + + describe('on KeyringController:unlock', () => { + const MOCK_GROUP_ID = 'keyring:01JABC/group-1' as AccountGroupId; + const MOCK_ACCOUNTS = [ + '00000000-0000-0000-0000-000000000001', + '00000000-0000-0000-0000-000000000002', + ]; + + it('forwards the currently selected account group to the Snap keyring', async () => { + const { service, rootMessenger, mocks } = setup(); + const setSelectedAccounts = jest.fn().mockResolvedValue(undefined); + mockLegacySnapKeyring(mocks, { setSelectedAccounts }); + mocks.AccountTreeController.getSelectedAccountGroup.mockReturnValue( + MOCK_GROUP_ID, + ); + mocks.AccountTreeController.getAccountGroupObject.mockReturnValue( + buildGroup(MOCK_ACCOUNTS), + ); + expect(service).toBeDefined(); + + publishUnlock(rootMessenger); + await flushMicrotasks(); + + expect( + mocks.AccountTreeController.getSelectedAccountGroup, + ).toHaveBeenCalledTimes(1); + expect( + mocks.AccountTreeController.getAccountGroupObject, + ).toHaveBeenCalledWith(MOCK_GROUP_ID); + expect(setSelectedAccounts).toHaveBeenCalledWith(MOCK_ACCOUNTS); + }); + + it('does nothing when no account group is selected', async () => { + const { service, rootMessenger, mocks } = setup(); + const setSelectedAccounts = jest.fn().mockResolvedValue(undefined); + mockLegacySnapKeyring(mocks, { setSelectedAccounts }); + mocks.AccountTreeController.getSelectedAccountGroup.mockReturnValue(''); + expect(service).toBeDefined(); + + publishUnlock(rootMessenger); + await flushMicrotasks(); + + expect( + mocks.AccountTreeController.getAccountGroupObject, + ).not.toHaveBeenCalled(); + expect(setSelectedAccounts).not.toHaveBeenCalled(); + }); + + it('logs an error when forwarding to the Snap keyring fails', async () => { + const { service, rootMessenger, mocks } = setup(); + const error = new Error('forward boom'); + const setSelectedAccounts = jest.fn().mockRejectedValue(error); + mockLegacySnapKeyring(mocks, { setSelectedAccounts }); + mocks.AccountTreeController.getSelectedAccountGroup.mockReturnValue( + MOCK_GROUP_ID, + ); + mocks.AccountTreeController.getAccountGroupObject.mockReturnValue( + buildGroup(MOCK_ACCOUNTS), + ); + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => undefined); + expect(service).toBeDefined(); + + publishUnlock(rootMessenger); + await flushMicrotasks(); + + expect(setSelectedAccounts).toHaveBeenCalledWith(MOCK_ACCOUNTS); + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error forwarding selected account group on unlock:', + error, + ); + + consoleErrorSpy.mockRestore(); + }); + }); }); diff --git a/packages/snap-account-service/src/SnapAccountService.ts b/packages/snap-account-service/src/SnapAccountService.ts index 6f67c358c1..ec2a285bec 100644 --- a/packages/snap-account-service/src/SnapAccountService.ts +++ b/packages/snap-account-service/src/SnapAccountService.ts @@ -1,3 +1,4 @@ +import { AccountGroupId } from '@metamask/account-api'; import type { SnapKeyring as LegacySnapKeyring, SnapMessage, @@ -5,6 +6,7 @@ import type { import type { KeyringControllerGetStateAction, KeyringControllerStateChangeEvent, + KeyringControllerUnlockEvent, KeyringControllerWithControllerAction, KeyringEntry, } from '@metamask/keyring-controller'; @@ -36,6 +38,11 @@ import type { import { SnapPlatformWatcher } from './SnapPlatformWatcher'; import type { SnapPlatformWatcherConfig } from './SnapPlatformWatcher'; import { SnapTracker } from './SnapTracker'; +import type { + AccountTreeControllerGetAccountGroupObjectAction, + AccountTreeControllerGetSelectedAccountGroupAction, + AccountTreeControllerSelectedAccountGroupChangeEvent, +} from './types'; /** * The name of the {@link SnapAccountService}, used to namespace the service's @@ -71,7 +78,9 @@ type AllowedActions = | SnapControllerGetSnapAction | SnapControllerGetRunnableSnapsAction | KeyringControllerGetStateAction - | KeyringControllerWithControllerAction; + | KeyringControllerWithControllerAction + | AccountTreeControllerGetAccountGroupObjectAction + | AccountTreeControllerGetSelectedAccountGroupAction; /** * Events that {@link SnapAccountService} exposes to other consumers. @@ -89,7 +98,9 @@ type AllowedEvents = | SnapControllerSnapBlockedEvent | SnapControllerSnapUnblockedEvent | SnapControllerSnapUninstalledEvent - | KeyringControllerStateChangeEvent; + | KeyringControllerStateChangeEvent + | KeyringControllerUnlockEvent + | AccountTreeControllerSelectedAccountGroupChangeEvent; /** * The messenger which is restricted to actions and events accessed by @@ -164,6 +175,43 @@ export class SnapAccountService { this, MESSENGER_EXPOSED_METHODS, ); + + this.#messenger.subscribe( + 'AccountTreeController:selectedAccountGroupChange', + (groupId) => this.#handleSelectedAccountGroupChange(groupId), + ); + + this.#messenger.subscribe('KeyringController:unlock', () => + this.#handleUnlock(), + ); + } + + /** + * Handles changes to the selected account group by forwarding the new + * group's accounts to the Snap keyring. + * + * @param groupId - The ID of the newly selected account group. + */ + #handleSelectedAccountGroupChange(groupId: AccountGroupId | ''): void { + this.#forwardSelectedAccountGroup(groupId).catch((error) => { + console.error('Error handling selected account group change:', error); + }); + } + + /** + * Handles the keyring controller unlock event by forwarding the currently + * selected account group's accounts to the Snap keyring. + */ + #handleUnlock(): void { + const groupId = this.#messenger.call( + 'AccountTreeController:getSelectedAccountGroup', + ); + this.#forwardSelectedAccountGroup(groupId).catch((error) => { + console.error( + 'Error forwarding selected account group on unlock:', + error, + ); + }); } /** @@ -268,4 +316,30 @@ export class SnapAccountService { const snapKeyring = await this.getLegacySnapKeyring(); return snapKeyring.handleKeyringSnapMessage(snapId, message); } + + /** + * Forwards the accounts of the given account group to the Snap keyring. + * + * @param groupId - The ID of the account group whose accounts should be + * forwarded. If empty, this is a no-op. + */ + async #forwardSelectedAccountGroup( + groupId: AccountGroupId | '', + ): Promise { + if (groupId) { + const group = this.#messenger.call( + 'AccountTreeController:getAccountGroupObject', + groupId, + ); + + if (group) { + log( + `Forwarding selected accounts (from "${groupId}") to Snap keyring: ${group.accounts.join(', ')}`, + ); + + const snapKeyring = await this.getLegacySnapKeyring(); + await snapKeyring.setSelectedAccounts(group.accounts); + } + } + } } diff --git a/packages/snap-account-service/src/types.ts b/packages/snap-account-service/src/types.ts new file mode 100644 index 0000000000..9e175a1b7e --- /dev/null +++ b/packages/snap-account-service/src/types.ts @@ -0,0 +1,53 @@ +import type { AccountGroupId } from '@metamask/account-api'; +import { AccountId } from '@metamask/keyring-utils'; + +/* + * NOTE: The types below intentionally duplicate definitions from + * `@metamask/account-tree-controller`. They are mirrored here to break a + * package-level circular dependency: + * + * account-tree-controller -> multichain-account-service -> snap-account-service + * -> account-tree-controller + * + * `multichain-account-service` legitimately depends on `snap-account-service` + * (for `:ensureReady`), and `account-tree-controller` legitimately depends on + * `multichain-account-service`. Importing `AccountTreeController` types here + * would close the cycle and crash `ts-bridge` with a stack overflow during + * project-reference builds. + * + * Keep these signatures in sync with `@metamask/account-tree-controller`. + */ + +/** + * Minimal shape of an account group object as consumed by + * {@link SnapAccountService}. Only the `accounts` field is read at runtime; + * the rest of the structure is intentionally omitted to keep this mirror + * narrow. See the note above. + */ +export type AccountGroupObject = { + accounts: AccountId[]; +}; + +/** + * Mirror of `AccountTreeControllerGetAccountGroupObjectAction`. + */ +export type AccountTreeControllerGetAccountGroupObjectAction = { + type: `AccountTreeController:getAccountGroupObject`; + handler: (groupId: AccountGroupId) => AccountGroupObject | undefined; +}; + +/** + * Mirror of `AccountTreeControllerGetSelectedAccountGroupAction`. + */ +export type AccountTreeControllerGetSelectedAccountGroupAction = { + type: `AccountTreeController:getSelectedAccountGroup`; + handler: () => AccountGroupId | ''; +}; + +/** + * Mirror of `AccountTreeControllerSelectedAccountGroupChangeEvent`. + */ +export type AccountTreeControllerSelectedAccountGroupChangeEvent = { + type: `AccountTreeController:selectedAccountGroupChange`; + payload: [AccountGroupId | '', AccountGroupId | '']; +}; diff --git a/packages/snap-account-service/tsconfig.build.json b/packages/snap-account-service/tsconfig.build.json index 59c9b7a1df..f5d033dda3 100644 --- a/packages/snap-account-service/tsconfig.build.json +++ b/packages/snap-account-service/tsconfig.build.json @@ -6,6 +6,15 @@ "rootDir": "./src" }, "references": [ + // READ THIS CAREFULLY: + // `account-tree-controller` is intentionally NOT referenced here to break + // a project-reference cycle: + // account-tree-controller -> multichain-account-service + // -> snap-account-service + // -> account-tree-controller + // The types we'd otherwise pull from `@metamask/account-tree-controller` + // are mirrored locally in `./src/types.ts` — keep them in sync. + // { "path": "../account-tree-controller/tsconfig.build.json" }, { "path": "../keyring-controller/tsconfig.build.json" }, { "path": "../messenger/tsconfig.build.json" } ], diff --git a/packages/snap-account-service/tsconfig.json b/packages/snap-account-service/tsconfig.json index a0556a87d1..4fd7b9ded3 100644 --- a/packages/snap-account-service/tsconfig.json +++ b/packages/snap-account-service/tsconfig.json @@ -4,6 +4,15 @@ "baseUrl": "./" }, "references": [ + // READ THIS CAREFULLY: + // `account-tree-controller` is intentionally NOT referenced here to break + // a project-reference cycle: + // account-tree-controller -> multichain-account-service + // -> snap-account-service + // -> account-tree-controller + // The types we'd otherwise pull from `@metamask/account-tree-controller` + // are mirrored locally in `./src/types.ts` — keep them in sync. + // { "path": "../account-tree-controller" }, { "path": "../keyring-controller" }, { "path": "../messenger" } ], diff --git a/yarn.lock b/yarn.lock index 4bcaffa08d..584a18ddba 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5453,6 +5453,8 @@ __metadata: version: 0.0.0-use.local resolution: "@metamask/snap-account-service@workspace:packages/snap-account-service" dependencies: + "@metamask/account-api": "npm:^1.0.4" + "@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-controller": "npm:^25.5.0"