From 8ffcb839b1f3183316bcbbe26375a1c7bd7458d8 Mon Sep 17 00:00:00 2001 From: Kanthesha Devaramane Date: Mon, 29 Jul 2024 11:33:36 +0100 Subject: [PATCH 1/5] remove provider undefined type (#4567) ## Explanation To utilise the functionality of TokensController, it is must to pass the provider. So in this PR, we have removed the provider undefined type. ## References * Related to #4559 ## Changelog ### `@metamask/assets-controllers` - **BREAKING**: Provider undefined type removed ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --- packages/assets-controllers/src/TokensController.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/assets-controllers/src/TokensController.ts b/packages/assets-controllers/src/TokensController.ts index 9345f8b3342..0f74f8b8780 100644 --- a/packages/assets-controllers/src/TokensController.ts +++ b/packages/assets-controllers/src/TokensController.ts @@ -190,7 +190,7 @@ export class TokensController extends BaseController< #selectedAccountId: string; - #provider: Provider | undefined; + #provider: Provider; #abortController: AbortController; @@ -209,7 +209,7 @@ export class TokensController extends BaseController< messenger, }: { chainId: Hex; - provider: Provider | undefined; + provider: Provider; state?: Partial; messenger: TokensControllerMessenger; }) { @@ -750,7 +750,6 @@ export class TokensController extends BaseController< #getProvider(networkClientId?: NetworkClientId): Web3Provider { return new Web3Provider( - // @ts-expect-error TODO: remove this annotation once the `Eip1193Provider` class is released networkClientId ? this.messagingSystem.call( 'NetworkController:getNetworkClientById', From efba094ebc623f80e084124e0d893ec6f9045b06 Mon Sep 17 00:00:00 2001 From: Michele Esposito <34438276+mikesposito@users.noreply.github.com> Date: Mon, 29 Jul 2024 13:42:39 +0200 Subject: [PATCH 2/5] fix: throw error when removing selected network (#4566) ## Explanation Currently, deleting a network configuration which is also the current selected network will corrupt the state, since the selected ID will not point to any valid config. With this PR, `removeNetworkConfiguration` throws an error if the caller tries to remove the selected network. ## References * Fixes #4563 ## Changelog ### `@metamask/network-controller` - **FIXED**: Selected network client configuration cannot be removed ## 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 highlighted breaking changes using the "BREAKING" category above as appropriate --- .../src/NetworkController.ts | 4 ++++ .../tests/NetworkController.test.ts | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/packages/network-controller/src/NetworkController.ts b/packages/network-controller/src/NetworkController.ts index 17c5a9fbe91..041a1a07b86 100644 --- a/packages/network-controller/src/NetworkController.ts +++ b/packages/network-controller/src/NetworkController.ts @@ -1248,6 +1248,10 @@ export class NetworkController extends BaseController< ); } + if (networkConfigurationId === this.state.selectedNetworkClientId) { + throw new Error(`The selected network configuration cannot be removed`); + } + const autoManagedNetworkClientRegistry = this.#ensureAutoManagedNetworkClientRegistryPopulated(); const networkClientId = networkConfigurationId; diff --git a/packages/network-controller/tests/NetworkController.test.ts b/packages/network-controller/tests/NetworkController.test.ts index d0f7229ec72..10e2c8f7433 100644 --- a/packages/network-controller/tests/NetworkController.test.ts +++ b/packages/network-controller/tests/NetworkController.test.ts @@ -3766,6 +3766,29 @@ describe('NetworkController', () => { }, ); }); + + it('throws an error if the given ID corresponds to the selected network', async () => { + await withController( + { + state: { + networkConfigurations: { + 'AAAA-AAAA-AAAA-AAAA': { + rpcUrl: 'https://test.network', + ticker: 'TICKER', + chainId: toHex(111), + id: 'AAAA-AAAA-AAAA-AAAA', + }, + }, + selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA', + }, + }, + async ({ controller }) => { + expect(() => + controller.removeNetworkConfiguration('AAAA-AAAA-AAAA-AAAA'), + ).toThrow('The selected network configuration cannot be removed'); + }, + ); + }); }); describe('given an ID that does not identify a network configuration in state', () => { From fc660e94cefe200a63cbd6bc6ef1da797d035a2b Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Mon, 29 Jul 2024 16:31:26 +0100 Subject: [PATCH 3/5] feat: add unlock checks for notification and profile sync controllers (#4569) ## Explanation This adds wallet unlock checks to ensure we only can call these controllers when the wallet is unlocked. This fixes potential issues in extension where we call these controllers when the wallet is locked (e.g. on browser startup) ## References [Ticket](https://consensyssoftware.atlassian.net/browse/NOTIFY-865) ## Changelog ### `@metamask/profile-sync-controller` - **ADDED**: Actions and Events listening to the Keyring Controller unlock requests - **ADDED**: Unlock checks when the preinstalled snap is called. ### `@metamask/notification-services-controller` - **ADDED**: Actions and Events listening to the Keyring Controller unlock requests - **ADDED**: Unlock checks when trying to setup push notifications ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --- .../NotificationServicesController.test.ts | 20 +++- .../NotificationServicesController.ts | 48 +++++++- packages/profile-sync-controller/package.json | 2 + .../AuthenticationController.test.ts | 110 +++++++++++++++++- .../AuthenticationController.ts | 49 +++++++- .../UserStorageController.test.ts | 10 +- .../user-storage/UserStorageController.ts | 43 ++++++- .../tsconfig.build.json | 5 +- .../profile-sync-controller/tsconfig.json | 5 +- yarn.lock | 2 + 10 files changed, 269 insertions(+), 25 deletions(-) diff --git a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts index 3a637e46629..87154aa8c09 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.test.ts @@ -40,6 +40,10 @@ import * as OnChainNotifications from './services/onchain-notifications'; import type { UserStorage } from './types/user-storage/user-storage'; import * as Utils from './utils/utils'; +// Mock type used for testing purposes +// eslint-disable-next-line @typescript-eslint/no-explicit-any +type MockVar = any; + const featureAnnouncementsEnv = { spaceId: ':space_id', accessToken: ':access_token', @@ -664,6 +668,7 @@ function mockNotificationMessenger() { name: 'NotificationServicesController', allowedActions: [ 'KeyringController:getAccounts', + 'KeyringController:getState', 'AuthenticationController:getBearerToken', 'AuthenticationController:isSignedIn', 'NotificationServicesPushController:disablePushNotifications', @@ -676,6 +681,8 @@ function mockNotificationMessenger() { ], allowedEvents: [ 'KeyringController:stateChange', + 'KeyringController:lock', + 'KeyringController:unlock', 'NotificationServicesPushController:onNewNotifications', ], }); @@ -729,6 +736,10 @@ function mockNotificationMessenger() { return mockListAccounts(); } + if (actionType === 'KeyringController:getState') { + return { isUnlocked: true } as MockVar; + } + if (actionType === 'AuthenticationController:getBearerToken') { return mockGetBearerToken(); } @@ -774,12 +785,9 @@ function mockNotificationMessenger() { return mockPerformSetStorage(params[0], params[1]); } - const exhaustedMessengerMocks = (action: never) => { - return new Error( - `MOCK_FAIL - unsupported messenger call: ${action as string}`, - ); - }; - throw exhaustedMessengerMocks(actionType); + throw new Error( + `MOCK_FAIL - unsupported messenger call: ${actionType as string}`, + ); }); return { diff --git a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts index 9775b5fcafb..3b432e8bd0b 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/NotificationServicesController.ts @@ -9,6 +9,9 @@ import { toChecksumHexAddress } from '@metamask/controller-utils'; import type { KeyringControllerGetAccountsAction, KeyringControllerStateChangeEvent, + KeyringControllerGetStateAction, + KeyringControllerLockEvent, + KeyringControllerUnlockEvent, } from '@metamask/keyring-controller'; import type { AuthenticationController, @@ -192,6 +195,7 @@ export type Actions = export type AllowedActions = // Keyring Controller Requests | KeyringControllerGetAccountsAction + | KeyringControllerGetStateAction // Auth Controller Requests | AuthenticationController.AuthenticationControllerGetBearerToken | AuthenticationController.AuthenticationControllerIsSignedIn @@ -214,7 +218,11 @@ export type NotificationServicesControllerMessengerEvents = // Allowed Events export type AllowedEvents = + // Keyring Events | KeyringControllerStateChangeEvent + | KeyringControllerLockEvent + | KeyringControllerUnlockEvent + // Push Notification Events | NotificationServicesPushControllerOnNewNotification; // Type for the messenger of NotificationServicesController @@ -244,6 +252,34 @@ export default class NotificationServicesController extends BaseController< // Temporary boolean as push notifications are not yet enabled on mobile #isPushIntegrated = true; + // Flag to check is notifications have been setup when the browser/extension is initialized. + // We want to re-initialize push notifications when the browser/extension is refreshed + // To ensure we subscribe to the most up-to-date notifications + #isPushNotificationsSetup = false; + + #isUnlocked = false; + + #keyringController = { + setupLockedStateSubscriptions: (onUnlock: () => Promise) => { + const { isUnlocked } = this.messagingSystem.call( + 'KeyringController:getState', + ); + this.#isUnlocked = isUnlocked; + + this.messagingSystem.subscribe('KeyringController:unlock', () => { + this.#isUnlocked = true; + // messaging system cannot await promises + // we don't need to wait for a result on this. + // eslint-disable-next-line @typescript-eslint/no-floating-promises + onUnlock(); + }); + + this.messagingSystem.subscribe('KeyringController:lock', () => { + this.#isUnlocked = false; + }); + }, + }; + #auth = { getBearerToken: async () => { return await this.messagingSystem.call( @@ -338,6 +374,12 @@ export default class NotificationServicesController extends BaseController< if (!this.state.isNotificationServicesEnabled) { return; } + if (this.#isPushNotificationsSetup) { + return; + } + if (!this.#isUnlocked) { + return; + } const storage = await this.#getUserStorage(); if (!storage) { @@ -346,6 +388,7 @@ export default class NotificationServicesController extends BaseController< const uuids = Utils.getAllUUIDs(storage); await this.#pushNotifications.enablePushNotifications(uuids); + this.#isPushNotificationsSetup = true; }, }; @@ -463,10 +506,13 @@ export default class NotificationServicesController extends BaseController< }); this.#isPushIntegrated = env.isPushIntegrated ?? true; - this.#featureAnnouncementEnv = env.featureAnnouncements; this.#registerMessageHandlers(); this.#clearLoadingStates(); + + this.#keyringController.setupLockedStateSubscriptions( + this.#pushNotifications.initializePushNotifications, + ); // eslint-disable-next-line @typescript-eslint/no-floating-promises this.#accounts.initialize(); // eslint-disable-next-line @typescript-eslint/no-floating-promises diff --git a/packages/profile-sync-controller/package.json b/packages/profile-sync-controller/package.json index ee92038cfb9..e81dc89cedc 100644 --- a/packages/profile-sync-controller/package.json +++ b/packages/profile-sync-controller/package.json @@ -53,6 +53,7 @@ "devDependencies": { "@lavamoat/allow-scripts": "^3.0.4", "@metamask/auto-changelog": "^3.4.4", + "@metamask/keyring-controller": "^17.1.2", "@metamask/snaps-controllers": "^9.3.1", "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", @@ -66,6 +67,7 @@ "typescript": "~5.0.4" }, "peerDependencies": { + "@metamask/keyring-controller": "^17.0.0", "@metamask/snaps-controllers": "^9.3.0" }, "engines": { diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts index 66f650dd44e..880a42f0f9f 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.test.ts @@ -12,6 +12,7 @@ import { import type { Actions, AllowedActions, + AllowedEvents, AuthenticationControllerState, } from './AuthenticationController'; import AuthenticationController from './AuthenticationController'; @@ -32,7 +33,7 @@ describe('authentication/authentication-controller - constructor() tests', () => it('should initialize with default state', () => { const metametrics = createMockAuthMetaMetrics(); const controller = new AuthenticationController({ - messenger: createAuthenticationMessenger(), + messenger: createMockAuthenticationMessenger().messenger, metametrics, }); @@ -43,7 +44,7 @@ describe('authentication/authentication-controller - constructor() tests', () => it('should initialize with override state', () => { const metametrics = createMockAuthMetaMetrics(); const controller = new AuthenticationController({ - messenger: createAuthenticationMessenger(), + messenger: createMockAuthenticationMessenger().messenger, state: mockSignedInState(), metametrics, }); @@ -90,6 +91,20 @@ describe('authentication/authentication-controller - performSignIn() tests', () await testAndAssertFailingEndpoints('token'); }); + // When the wallet is locked, we are unable to call the snap + it('should error when wallet is locked', async () => { + const { messenger, mockKeyringControllerGetState } = + createMockAuthenticationMessenger(); + const metametrics = createMockAuthMetaMetrics(); + + // Mock wallet is locked + mockKeyringControllerGetState.mockReturnValue({ isUnlocked: false }); + + const controller = new AuthenticationController({ messenger, metametrics }); + + await expect(controller.performSignIn()).rejects.toThrow(expect.any(Error)); + }); + /** * Jest Test & Assert Utility - for testing and asserting endpoint failures * @@ -208,6 +223,38 @@ describe('authentication/authentication-controller - getBearerToken() tests', () expect(result).toBeDefined(); expect(result).toBe(MOCK_ACCESS_TOKEN); }); + + // If the state is invalid, we need to re-login. + // But as wallet is locked, we will not be able to call the snap + it('should throw error if wallet is locked', async () => { + const metametrics = createMockAuthMetaMetrics(); + const { messenger, mockKeyringControllerGetState } = + createMockAuthenticationMessenger(); + mockAuthenticationFlowEndpoints(); + + // Invalid/old state + const originalState = mockSignedInState(); + if (originalState.sessionData) { + originalState.sessionData.accessToken = 'ACCESS_TOKEN_1'; + + const d = new Date(); + d.setMinutes(d.getMinutes() - 31); // expires at 30 mins + originalState.sessionData.expiresIn = d.toString(); + } + + // Mock wallet is locked + mockKeyringControllerGetState.mockReturnValue({ isUnlocked: false }); + + const controller = new AuthenticationController({ + messenger, + state: originalState, + metametrics, + }); + + await expect(controller.getBearerToken()).rejects.toThrow( + expect.any(Error), + ); + }); }); describe('authentication/authentication-controller - getSessionProfile() tests', () => { @@ -264,6 +311,38 @@ describe('authentication/authentication-controller - getSessionProfile() tests', expect(result.identifierId).toBe(MOCK_LOGIN_RESPONSE.profile.identifier_id); expect(result.profileId).toBe(MOCK_LOGIN_RESPONSE.profile.profile_id); }); + + // If the state is invalid, we need to re-login. + // But as wallet is locked, we will not be able to call the snap + it('should throw error if wallet is locked', async () => { + const metametrics = createMockAuthMetaMetrics(); + const { messenger, mockKeyringControllerGetState } = + createMockAuthenticationMessenger(); + mockAuthenticationFlowEndpoints(); + + // Invalid/old state + const originalState = mockSignedInState(); + if (originalState.sessionData) { + originalState.sessionData.profile.identifierId = 'ID_1'; + + const d = new Date(); + d.setMinutes(d.getMinutes() - 31); // expires at 30 mins + originalState.sessionData.expiresIn = d.toString(); + } + + // Mock wallet is locked + mockKeyringControllerGetState.mockReturnValue({ isUnlocked: false }); + + const controller = new AuthenticationController({ + messenger, + state: originalState, + metametrics, + }); + + await expect(controller.getSessionProfile()).rejects.toThrow( + expect.any(Error), + ); + }); }); /** @@ -272,11 +351,17 @@ describe('authentication/authentication-controller - getSessionProfile() tests', * @returns Auth Messenger */ function createAuthenticationMessenger() { - const messenger = new ControllerMessenger(); + const messenger = new ControllerMessenger< + Actions | AllowedActions, + AllowedEvents + >(); return messenger.getRestricted({ name: 'AuthenticationController', - allowedActions: [`SnapController:handleRequest`], - allowedEvents: [], + allowedActions: [ + 'KeyringController:getState', + 'SnapController:handleRequest', + ], + allowedEvents: ['KeyringController:lock', 'KeyringController:unlock'], }); } @@ -293,6 +378,10 @@ function createMockAuthenticationMessenger() { .fn() .mockResolvedValue('MOCK_SIGNED_MESSAGE'); + const mockKeyringControllerGetState = jest + .fn() + .mockReturnValue({ isUnlocked: true }); + mockCall.mockImplementation((...args) => { const [actionType, params] = args; if (actionType === 'SnapController:handleRequest') { @@ -311,12 +400,21 @@ function createMockAuthenticationMessenger() { ); } + if (actionType === 'KeyringController:getState') { + return mockKeyringControllerGetState(); + } + throw new Error( `MOCK_FAIL - unsupported messenger call: ${actionType as string}`, ); }); - return { messenger, mockSnapGetPublicKey, mockSnapSignMessage }; + return { + messenger, + mockSnapGetPublicKey, + mockSnapSignMessage, + mockKeyringControllerGetState, + }; } /** diff --git a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts index fa9730e0e4a..351dbda53da 100644 --- a/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts +++ b/packages/profile-sync-controller/src/controllers/authentication/AuthenticationController.ts @@ -3,6 +3,11 @@ import type { StateMetadata, } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; +import type { + KeyringControllerGetStateAction, + KeyringControllerLockEvent, + KeyringControllerUnlockEvent, +} from '@metamask/keyring-controller'; import type { HandleSnapRequest } from '@metamask/snaps-controllers'; import { @@ -87,15 +92,21 @@ export type AuthenticationControllerGetSessionProfile = export type AuthenticationControllerIsSignedIn = ActionsObj['isSignedIn']; // Allowed Actions -export type AllowedActions = HandleSnapRequest; +export type AllowedActions = + | HandleSnapRequest + | KeyringControllerGetStateAction; + +export type AllowedEvents = + | KeyringControllerLockEvent + | KeyringControllerUnlockEvent; // Messenger export type AuthenticationControllerMessenger = RestrictedControllerMessenger< typeof controllerName, Actions | AllowedActions, - never, + AllowedEvents, AllowedActions['type'], - never + AllowedEvents['type'] >; /** @@ -109,6 +120,25 @@ export default class AuthenticationController extends BaseController< > { #metametrics: MetaMetricsAuth; + #isUnlocked = false; + + #keyringController = { + setupLockedStateSubscriptions: () => { + const { isUnlocked } = this.messagingSystem.call( + 'KeyringController:getState', + ); + this.#isUnlocked = isUnlocked; + + this.messagingSystem.subscribe('KeyringController:unlock', () => { + this.#isUnlocked = true; + }); + + this.messagingSystem.subscribe('KeyringController:lock', () => { + this.#isUnlocked = false; + }); + }, + }; + constructor({ messenger, state, @@ -135,6 +165,7 @@ export default class AuthenticationController extends BaseController< this.#metametrics = metametrics; + this.#keyringController.setupLockedStateSubscriptions(); this.#registerMessageHandlers(); } @@ -316,6 +347,12 @@ export default class AuthenticationController extends BaseController< return this.#_snapPublicKeyCache; } + if (!this.#isUnlocked) { + throw new Error( + '#snapGetPublicKey - unable to call snap, wallet is locked', + ); + } + const result = (await this.messagingSystem.call( 'SnapController:handleRequest', createSnapPublicKeyRequest(), @@ -339,6 +376,12 @@ export default class AuthenticationController extends BaseController< return this.#_snapSignMessageCache[message]; } + if (!this.#isUnlocked) { + throw new Error( + '#snapSignMessage - unable to call snap, wallet is locked', + ); + } + const result = (await this.messagingSystem.call( 'SnapController:handleRequest', createSnapSignMessageRequest(message), diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts index 354e154b953..8aa8b1098db 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts @@ -18,6 +18,7 @@ import { } from './__fixtures__/mockStorage'; import type { AllowedActions, + AllowedEvents, NotificationServicesControllerDisableNotificationServices, NotificationServicesControllerSelectIsNotificationServicesEnabled, } from './UserStorageController'; @@ -305,10 +306,11 @@ describe('user-storage/user-storage-controller - enableProfileSyncing() tests', function mockUserStorageMessenger() { const messenger = new ControllerMessenger< AllowedActions, - never + AllowedEvents >().getRestricted({ name: 'UserStorageController', allowedActions: [ + 'KeyringController:getState', 'SnapController:handleRequest', 'AuthenticationController:getBearerToken', 'AuthenticationController:getSessionProfile', @@ -318,7 +320,7 @@ function mockUserStorageMessenger() { 'NotificationServicesController:disableNotificationServices', 'NotificationServicesController:selectIsNotificationServicesEnabled', ], - allowedEvents: [], + allowedEvents: ['KeyringController:lock', 'KeyringController:unlock'], }); const mockSnapGetPublicKey = jest.fn().mockResolvedValue('MOCK_PUBLIC_KEY'); @@ -415,6 +417,10 @@ function mockUserStorageMessenger() { return mockAuthPerformSignOut(); } + if (actionType === 'KeyringController:getState') { + return { isUnlocked: true }; + } + const exhaustedMessengerMocks = (action: never) => { throw new Error( `MOCK_FAIL - unsupported messenger call: ${action as string}`, diff --git a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts index 4ff0b6610f4..f4d542ee034 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -3,6 +3,11 @@ import type { StateMetadata, } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; +import type { + KeyringControllerGetStateAction, + KeyringControllerLockEvent, + KeyringControllerUnlockEvent, +} from '@metamask/keyring-controller'; import type { HandleSnapRequest } from '@metamask/snaps-controllers'; import { createSnapSignMessageRequest } from '../authentication/auth-snap-requests'; @@ -87,6 +92,8 @@ export type UserStorageControllerDisableProfileSyncing = // Allowed Actions export type AllowedActions = + // Keyring Requests + | KeyringControllerGetStateAction // Snap Requests | HandleSnapRequest // Auth Requests @@ -99,13 +106,17 @@ export type AllowedActions = | NotificationServicesControllerDisableNotificationServices | NotificationServicesControllerSelectIsNotificationServicesEnabled; +export type AllowedEvents = + | KeyringControllerLockEvent + | KeyringControllerUnlockEvent; + // Messenger export type UserStorageControllerMessenger = RestrictedControllerMessenger< typeof controllerName, Actions | AllowedActions, - never, + AllowedEvents, AllowedActions['type'], - never + AllowedEvents['type'] >; /** @@ -161,6 +172,25 @@ export default class UserStorageController extends BaseController< }, }; + #isUnlocked = false; + + #keyringController = { + setupLockedStateSubscriptions: () => { + const { isUnlocked } = this.messagingSystem.call( + 'KeyringController:getState', + ); + this.#isUnlocked = isUnlocked; + + this.messagingSystem.subscribe('KeyringController:unlock', () => { + this.#isUnlocked = true; + }); + + this.messagingSystem.subscribe('KeyringController:lock', () => { + this.#isUnlocked = false; + }); + }, + }; + getMetaMetricsState: () => boolean; constructor(params: { @@ -176,6 +206,7 @@ export default class UserStorageController extends BaseController< }); this.getMetaMetricsState = params.getMetaMetricsState; + this.#keyringController.setupLockedStateSubscriptions(); this.#registerMessageHandlers(); } @@ -260,7 +291,7 @@ export default class UserStorageController extends BaseController< const isMetaMetricsParticipation = this.getMetaMetricsState(); if (!isMetaMetricsParticipation) { - this.messagingSystem.call('AuthenticationController:performSignOut'); + await this.#auth.signOut(); } this.#setIsProfileSyncingUpdateLoading(false); @@ -389,6 +420,12 @@ export default class UserStorageController extends BaseController< return this.#_snapSignMessageCache[message]; } + if (!this.#isUnlocked) { + throw new Error( + '#snapSignMessage - unable to call snap, wallet is locked', + ); + } + const result = (await this.messagingSystem.call( 'SnapController:handleRequest', createSnapSignMessageRequest(message), diff --git a/packages/profile-sync-controller/tsconfig.build.json b/packages/profile-sync-controller/tsconfig.build.json index 8d4cf54b4cd..d5dd6781e04 100644 --- a/packages/profile-sync-controller/tsconfig.build.json +++ b/packages/profile-sync-controller/tsconfig.build.json @@ -7,9 +7,8 @@ "skipLibCheck": true }, "references": [ - { - "path": "../base-controller/tsconfig.build.json" - } + { "path": "../base-controller/tsconfig.build.json" }, + { "path": "../keyring-controller/tsconfig.build.json" } ], "include": ["../../types", "./src"] } diff --git a/packages/profile-sync-controller/tsconfig.json b/packages/profile-sync-controller/tsconfig.json index 34354c4b09d..092d51bd4d2 100644 --- a/packages/profile-sync-controller/tsconfig.json +++ b/packages/profile-sync-controller/tsconfig.json @@ -3,6 +3,9 @@ "compilerOptions": { "baseUrl": "./" }, - "references": [{ "path": "../base-controller" }], + "references": [ + { "path": "../base-controller" }, + { "path": "../keyring-controller" } + ], "include": ["../../types", "./src"] } diff --git a/yarn.lock b/yarn.lock index 9966639ae7a..4a0966e2cdf 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3551,6 +3551,7 @@ __metadata: "@lavamoat/allow-scripts": "npm:^3.0.4" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^6.0.2" + "@metamask/keyring-controller": "npm:^17.1.2" "@metamask/snaps-controllers": "npm:^9.3.1" "@metamask/snaps-sdk": "npm:^6.1.1" "@metamask/snaps-utils": "npm:^7.8.1" @@ -3570,6 +3571,7 @@ __metadata: typedoc-plugin-missing-exports: "npm:^2.0.0" typescript: "npm:~5.0.4" peerDependencies: + "@metamask/keyring-controller": ^17.0.0 "@metamask/snaps-controllers": ^9.3.0 languageName: unknown linkType: soft From bed57e24260fd08304cb996bf1b71fbdfa2396f5 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 30 Jul 2024 10:29:13 +0200 Subject: [PATCH 4/5] feat: add `KeyringController:addNewAccount` messenger action (#4565) ## Explanation This PR exposes the `addNewAccount` method / action from the `KeyringController`. This method will be used by the `UserStorageController` for the Account syncing feature (from @MetaMask/notifications ). Related to [this Epic](https://consensyssoftware.atlassian.net/jira/software/projects/NOTIFY/boards/616?assignee=712020%3A5843b7e2-a7fe-4c45-9fbd-e1f2b2eb58c2&selectedIssue=NOTIFY-851) ## References ## Changelog ### `@metamask/keyring-controller` - **ADDED**: Add `KeyringController:addNewAccount` messenger action ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --- .../keyring-controller/src/KeyringController.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index b5fca807a11..3e15c3f5b54 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -174,6 +174,11 @@ export type KeyringControllerSignUserOperationAction = { handler: KeyringController['signUserOperation']; }; +export type KeyringControllerAddNewAccountAction = { + type: `${typeof name}:addNewAccount`; + handler: KeyringController['addNewAccount']; +}; + export type KeyringControllerStateChangeEvent = { type: `${typeof name}:stateChange`; payload: [KeyringControllerState, Patch[]]; @@ -212,7 +217,8 @@ export type KeyringControllerActions = | KeyringControllerPersistAllKeyringsAction | KeyringControllerPrepareUserOperationAction | KeyringControllerPatchUserOperationAction - | KeyringControllerSignUserOperationAction; + | KeyringControllerSignUserOperationAction + | KeyringControllerAddNewAccountAction; export type KeyringControllerEvents = | KeyringControllerStateChangeEvent @@ -1796,6 +1802,11 @@ export class KeyringController extends BaseController< `${name}:signUserOperation`, this.signUserOperation.bind(this), ); + + this.messagingSystem.registerActionHandler( + `${name}:addNewAccount`, + this.addNewAccount.bind(this), + ); } /** From c00a309966adad657a9bbab6b0a17c6518d67278 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 30 Jul 2024 13:24:58 +0200 Subject: [PATCH 5/5] feat: add new method `updateAccountMetadata` (#4568) ## Explanation This PR adds a new `updateAccountMetadata` method/action for the `AccountsController`. This method will be needed for the Account syncing feature (from @MetaMask/notifications ), but can also come handy in the future if / when we decide to expand the contents of the metadata object for new features. On the account syncing side, we're still deciding if we're going to manage conflicting names resolution, but in either scenario, this new `updateAccountMetadata` method is needed. ### For example: Scenario 1: we don't manage conflicting names - We'll add a new `lastUpdated` metadata key to the `InternalAccount` type from `@metamask/keyring-api` - We'll read this key when syncing accounts, and act accordingly (replace account name OR decide it's the SOT and keep the account name) - If we replace the account name, we'll update this key with `updateAccountMetadata` Scenario 2: we manage conflicting names - We'll add a new `conflictingNames` metadata key to the `InternalAccount` type from `@metamask/keyring-api` - We'll read this key when displaying conflicts in the UI - When a user resolves a conflict in names from the UI, we'll update this key with `updateAccountMetadata` Related to [this Epic](https://consensyssoftware.atlassian.net/jira/software/projects/NOTIFY/boards/616?assignee=712020%3A5843b7e2-a7fe-4c45-9fbd-e1f2b2eb58c2&selectedIssue=NOTIFY-851) ## References ## Changelog ### `@metamask/accounts-controller` - **ADDED**: Add `updateAccountMetadata` method and `AccountsController:updateAccountMetadata` controller action ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --- .../src/AccountsController.test.ts | 21 ++++++++++++ .../src/AccountsController.ts | 34 ++++++++++++++++--- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 82a960baea0..7c6474385ed 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -2331,6 +2331,27 @@ describe('AccountsController', () => { }); }); + describe('updateAccountMetadata', () => { + it('updates the metadata of an existing account', () => { + const { accountsController } = setupAccountsController({ + initialState: { + internalAccounts: { + accounts: { [mockAccount.id]: mockAccount }, + selectedAccount: mockAccount.id, + }, + }, + }); + accountsController.updateAccountMetadata(mockAccount.id, { + lastSelected: 1, + }); + + expect( + accountsController.getAccountExpect(mockAccount.id).metadata + .lastSelected, + ).toBe(1); + }); + }); + describe('#getNextAccountNumber', () => { // Account names start at 2 since have 1 HD account + 2 simple keypair accounts (and both // those keyring types are "grouped" together) diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index af7714297f8..8deabec7fd4 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -106,6 +106,11 @@ export type AccountsControllerGetAccountAction = { handler: AccountsController['getAccount']; }; +export type AccountsControllerUpdateAccountMetadata = { + type: `${typeof controllerName}:updateAccountMetadata`; + handler: AccountsController['updateAccountMetadata']; +}; + export type AllowedActions = | KeyringControllerGetKeyringForAccountAction | KeyringControllerGetKeyringsByTypeAction @@ -122,7 +127,8 @@ export type AccountsControllerActions = | AccountsControllerGetSelectedAccountAction | AccountsControllerGetNextAvailableAccountNameAction | AccountsControllerGetAccountAction - | AccountsControllerGetSelectedMultichainAccountAction; + | AccountsControllerGetSelectedMultichainAccountAction + | AccountsControllerUpdateAccountMetadata; export type AccountsControllerChangeEvent = ControllerStateChangeEvent< typeof controllerName, @@ -406,8 +412,6 @@ export class AccountsController extends BaseController< * @throws An error if an account with the same name already exists. */ setAccountName(accountId: string, accountName: string): void { - const account = this.getAccountExpect(accountId); - if ( this.listMultichainAccounts().find( (internalAccount) => @@ -418,12 +422,29 @@ export class AccountsController extends BaseController< throw new Error('Account name already exists'); } + this.updateAccountMetadata(accountId, { name: accountName }); + } + + /** + * Updates the metadata of the account with the given ID. + * Use {@link setAccountName} if you only need to update the name of the account. + * + * @param accountId - The ID of the account for which the metadata will be updated. + * @param metadata - The new metadata for the account. + */ + updateAccountMetadata( + accountId: string, + metadata: Partial, + ): void { + const account = this.getAccountExpect(accountId); + this.update((currentState: Draft) => { const internalAccount = { ...account, - metadata: { ...account.metadata, name: accountName }, + metadata: { ...account.metadata, ...metadata }, }; // Do not remove this comment - This error is flaky: Comment out or restore the `ts-expect-error` directive below as needed. + // See: https://github.com/MetaMask/utils/issues/168 // // @ts-expect-error Known issue - `Json` causes recursive error in immer `Draft`/`WritableDraft` types currentState.internalAccounts.accounts[accountId] = internalAccount; }); @@ -1093,5 +1114,10 @@ export class AccountsController extends BaseController< `AccountsController:getAccount`, this.getAccount.bind(this), ); + + this.messagingSystem.registerActionHandler( + `AccountsController:updateAccountMetadata`, + this.updateAccountMetadata.bind(this), + ); } }