From 94ac34e18b387d1107fb4f55b86f29ca8768ccad Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 14 Jan 2025 18:22:22 +0100 Subject: [PATCH 1/4] feat: persist `isAccountSyncingReadyToBeDispatched` statue value (#5147) ## Explanation This PR changes the `isAccountSyncingReadyToBeDispatched` state value so it is persisted. ## References ## Changelog ### `@metamask/profile-sync-controller` - **CHANGED**: `isAccountSyncingReadyToBeDispatched` state value is now persisted ## 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 - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- .../src/controllers/user-storage/UserStorageController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 619336ccae..ec620709e3 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts @@ -126,7 +126,7 @@ const metadata: StateMetadata = { anonymous: false, }, isAccountSyncingReadyToBeDispatched: { - persist: false, + persist: true, anonymous: false, }, isAccountSyncingInProgress: { From 74f6d9c7ab0af8746fd16e2ad656b8ba77143855 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 14 Jan 2025 18:36:56 +0100 Subject: [PATCH 2/4] Release 283.0.0 (#5148) ## Explanation This is a RC for v283.0.0. See changelog for more details @metamask/profile-sync-controller@4.1.0 ## References ## Changelog ```ms ### Changed - Bump `@metamask/profile-sync-controller` from `^4.0.1` to `^4.1.0` ([#5148](https://github.com/MetaMask/core/pull/5148)) ``` ## 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 - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- package.json | 2 +- packages/notification-services-controller/package.json | 2 +- packages/profile-sync-controller/CHANGELOG.md | 9 ++++++++- packages/profile-sync-controller/package.json | 2 +- yarn.lock | 4 ++-- 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 123572835c..acd475a8e8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/core-monorepo", - "version": "282.0.0", + "version": "283.0.0", "private": true, "description": "Monorepo for packages shared between MetaMask clients", "repository": { diff --git a/packages/notification-services-controller/package.json b/packages/notification-services-controller/package.json index bd0d95d4a3..7515a0a3e6 100644 --- a/packages/notification-services-controller/package.json +++ b/packages/notification-services-controller/package.json @@ -113,7 +113,7 @@ "@lavamoat/preinstall-always-fail": "^2.1.0", "@metamask/auto-changelog": "^3.4.4", "@metamask/keyring-controller": "^19.0.3", - "@metamask/profile-sync-controller": "^4.0.1", + "@metamask/profile-sync-controller": "^4.1.0", "@types/jest": "^27.4.1", "@types/readable-stream": "^2.3.0", "contentful": "^10.15.0", diff --git a/packages/profile-sync-controller/CHANGELOG.md b/packages/profile-sync-controller/CHANGELOG.md index 3e21a911ae..5cf6aa5b8d 100644 --- a/packages/profile-sync-controller/CHANGELOG.md +++ b/packages/profile-sync-controller/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [4.1.0] + +### Changed + +- Persist `isAccountSyncingReadyToBeDispatched` state value ([#5147](https://github.com/MetaMask/core/pull/5147)) + ## [4.0.1] ### Added @@ -416,7 +422,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Initial release -[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@4.0.1...HEAD +[Unreleased]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@4.1.0...HEAD +[4.1.0]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@4.0.1...@metamask/profile-sync-controller@4.1.0 [4.0.1]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@4.0.0...@metamask/profile-sync-controller@4.0.1 [4.0.0]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@3.3.0...@metamask/profile-sync-controller@4.0.0 [3.3.0]: https://github.com/MetaMask/core/compare/@metamask/profile-sync-controller@3.2.0...@metamask/profile-sync-controller@3.3.0 diff --git a/packages/profile-sync-controller/package.json b/packages/profile-sync-controller/package.json index d109e073f9..232671b4cd 100644 --- a/packages/profile-sync-controller/package.json +++ b/packages/profile-sync-controller/package.json @@ -1,6 +1,6 @@ { "name": "@metamask/profile-sync-controller", - "version": "4.0.1", + "version": "4.1.0", "description": "The profile sync helps developers synchronize data across multiple clients and devices in a privacy-preserving way. All data saved in the user storage database is encrypted client-side to preserve privacy. The user storage provides a modular design, giving developers the flexibility to construct and manage their storage spaces in a way that best suits their needs", "keywords": [ "MetaMask", diff --git a/yarn.lock b/yarn.lock index d28def18d5..3401672a50 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3415,7 +3415,7 @@ __metadata: "@metamask/base-controller": "npm:^7.1.1" "@metamask/controller-utils": "npm:^11.4.5" "@metamask/keyring-controller": "npm:^19.0.3" - "@metamask/profile-sync-controller": "npm:^4.0.1" + "@metamask/profile-sync-controller": "npm:^4.1.0" "@metamask/utils": "npm:^11.0.1" "@types/jest": "npm:^27.4.1" "@types/readable-stream": "npm:^2.3.0" @@ -3596,7 +3596,7 @@ __metadata: languageName: unknown linkType: soft -"@metamask/profile-sync-controller@npm:^4.0.1, @metamask/profile-sync-controller@workspace:packages/profile-sync-controller": +"@metamask/profile-sync-controller@npm:^4.1.0, @metamask/profile-sync-controller@workspace:packages/profile-sync-controller": version: 0.0.0-use.local resolution: "@metamask/profile-sync-controller@workspace:packages/profile-sync-controller" dependencies: From c5d49d7ba5c393406da14827a8ee52840b57bd24 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Tue, 14 Jan 2025 19:15:34 +0100 Subject: [PATCH 3/4] fix: Migrate `AbstractMessageManager` from `BaseControllerV1` to `BaseControllerV2` (#5103) ## Explanation This PR aims to remove `BaseControllerV1` usage from `AbstractMessageManager`. As expected this change affected both `DecryptMessageManager` (`DMM`) and `EncryptionPublicKeyManager`(`EPKM`). Since extension already have wrapper classes for both `DMM` & `EPKM` in the extension code, we want to keep changes minimal and make these both classes in the core work like controller but let wrappers sync the state in their classes as we currently do. You can find the extension PR in the references below. ## References * Fixes : https://github.com/MetaMask/MetaMask-planning/issues/3747 * Related extension PR: https://github.com/MetaMask/metamask-extension/pull/29237 ## Changelog ### `@metamask/message-manager` - **BREAKING:** Base class of `DecryptMessageManager` and `EncryptionPublicKeyManager`(`AbstractMessageManager`) now expects new options to initialise - **BREAKING:** Removed internal event emitter (`hub` property) from `AbstractMessageManager` - **BREAKING:** `unapprovedMessage` and `updateBadge` removed from internal events. These events are now emitted from messaging system - Controllers should now listen to `DerivedManagerName:X` event instead of using internal event emitter. ## 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 - [X] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- packages/message-manager/CHANGELOG.md | 7 + .../src/AbstractMessageManager.test.ts | 129 ++++--- .../src/AbstractMessageManager.ts | 319 ++++++++++-------- .../src/DecryptMessageManager.test.ts | 32 +- .../src/DecryptMessageManager.ts | 133 +++++--- .../src/EncryptionPublicKeyManager.test.ts | 33 +- .../src/EncryptionPublicKeyManager.ts | 126 ++++--- 7 files changed, 492 insertions(+), 287 deletions(-) diff --git a/packages/message-manager/CHANGELOG.md b/packages/message-manager/CHANGELOG.md index ac90e79a53..f9ca2a8819 100644 --- a/packages/message-manager/CHANGELOG.md +++ b/packages/message-manager/CHANGELOG.md @@ -9,8 +9,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** Base class of `DecryptMessageManager` and `EncryptionPublicKeyManager`(`AbstractMessageManager`) now expects new options to initialise ([#5103](https://github.com/MetaMask/core/pull/5103)) - Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079)) +### Removed + +- **BREAKING:** Removed internal event emitter (`hub` property) from `AbstractMessageManager` ([#5103](https://github.com/MetaMask/core/pull/5103)) +- **BREAKING:** `unapprovedMessage` and `updateBadge` removed from internal events. These events are now emitted from messaging system ([#5103](https://github.com/MetaMask/core/pull/5103)) + - Controllers should now listen to `DerivedManagerName:X` event instead of using internal event emitter. + ## [11.0.3] ### Changed diff --git a/packages/message-manager/src/AbstractMessageManager.test.ts b/packages/message-manager/src/AbstractMessageManager.test.ts index 740ed38e2c..fcf520854c 100644 --- a/packages/message-manager/src/AbstractMessageManager.test.ts +++ b/packages/message-manager/src/AbstractMessageManager.test.ts @@ -1,3 +1,4 @@ +import type { RestrictedControllerMessenger } from '@metamask/base-controller'; import { ApprovalType } from '@metamask/controller-utils'; import type { @@ -20,10 +21,15 @@ type ConcreteMessageParamsMetamask = ConcreteMessageParams & { metamaskId?: string; }; +type ConcreteMessageManagerActions = never; +type ConcreteMessageManagerEvents = never; + class AbstractTestManager extends AbstractMessageManager< ConcreteMessage, ConcreteMessageParams, - ConcreteMessageParamsMetamask + ConcreteMessageParamsMetamask, + ConcreteMessageManagerActions, + ConcreteMessageManagerEvents > { addRequestToMessageParams( messageParams: MessageParams, @@ -56,6 +62,26 @@ class AbstractTestManager extends AbstractMessageManager< } } +const MOCK_MESSENGER = { + clearEventSubscriptions: jest.fn(), + publish: jest.fn(), + registerActionHandler: jest.fn(), + registerInitialEventPayload: jest.fn(), +} as unknown as RestrictedControllerMessenger< + 'AbstractMessageManager', + never, + never, + string, + string +>; + +const MOCK_INITIAL_OPTIONS = { + additionalFinishStatuses: undefined, + messenger: MOCK_MESSENGER, + name: 'AbstractMessageManager' as const, + securityProviderRequest: undefined, +}; + const messageId = '1'; const messageId2 = '2'; const from = '0x0123'; @@ -78,20 +104,15 @@ const mockMessageParams = { from, test: testData }; describe('AbstractTestManager', () => { it('should set default state', () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); expect(controller.state).toStrictEqual({ unapprovedMessages: {}, unapprovedMessagesCount: 0, }); }); - it('should set default config', () => { - const controller = new AbstractTestManager(); - expect(controller.config).toStrictEqual({}); - }); - it('should add a valid message', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { @@ -115,7 +136,7 @@ describe('AbstractTestManager', () => { }); it('should get all messages', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const message = { id: messageId, messageParams: { @@ -148,11 +169,10 @@ describe('AbstractTestManager', () => { const securityProviderRequestMock: SecurityProviderRequest = jest .fn() .mockResolvedValue(securityProviderResponseMock); - const controller = new AbstractTestManager( - undefined, - undefined, - securityProviderRequestMock, - ); + const controller = new AbstractTestManager({ + ...MOCK_INITIAL_OPTIONS, + securityProviderRequest: securityProviderRequestMock, + }); await controller.addMessage({ id: messageId, messageParams: { @@ -180,7 +200,7 @@ describe('AbstractTestManager', () => { }); it('should reject a message', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { @@ -200,7 +220,7 @@ describe('AbstractTestManager', () => { }); it('should sign a message', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { @@ -221,12 +241,10 @@ describe('AbstractTestManager', () => { }); it('sets message to one of the allowed statuses', async () => { - const controller = new AbstractTestManager( - undefined, - undefined, - undefined, - ['test-status'], - ); + const controller = new AbstractTestManager({ + ...MOCK_INITIAL_OPTIONS, + additionalFinishStatuses: ['test-status'], + }); await controller.addMessage({ id: messageId, messageParams: { @@ -246,12 +264,10 @@ describe('AbstractTestManager', () => { }); it('should set a status to inProgress', async () => { - const controller = new AbstractTestManager( - undefined, - undefined, - undefined, - ['test-status'], - ); + const controller = new AbstractTestManager({ + ...MOCK_INITIAL_OPTIONS, + additionalFinishStatuses: ['test-status'], + }); await controller.addMessage({ id: messageId, messageParams: { @@ -285,7 +301,7 @@ describe('AbstractTestManager', () => { time: 123, type: 'eth_signTypedData', }; - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage(firstMessage); await controller.addMessage(secondMessage); expect(controller.getUnapprovedMessagesCount()).toBe(2); @@ -296,7 +312,7 @@ describe('AbstractTestManager', () => { }); it('should approve message', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const firstMessage = { from: '0xfoO', test: testData }; await controller.addMessage({ id: messageId, @@ -319,7 +335,7 @@ describe('AbstractTestManager', () => { describe('addRequestToMessageParams', () => { it('adds original request id and origin to messageParams', () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const result = controller.addRequestToMessageParams( mockMessageParams, @@ -336,7 +352,7 @@ describe('AbstractTestManager', () => { describe('createUnapprovedMessage', () => { it('creates a Message object with an unapproved status', () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const result = controller.createUnapprovedMessage( mockMessageParams, @@ -361,7 +377,7 @@ describe('AbstractTestManager', () => { emit: jest.fn(), })); - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { ...mockMessageParams }, @@ -379,7 +395,7 @@ describe('AbstractTestManager', () => { }); it('throws an error if the message is not found', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); expect(() => controller.setMessageStatus(messageId, 'newstatus')).toThrow( 'AbstractMessageManager: Message not found for id: 1.', @@ -393,7 +409,7 @@ describe('AbstractTestManager', () => { emit: jest.fn(), })); - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { ...mockMessageParams }, @@ -407,14 +423,13 @@ describe('AbstractTestManager', () => { controller.setMessageStatusAndResult(messageId, 'newRawSig', 'newstatus'); const messageAfter = controller.getMessage(messageId); - // expect(controller.hub.emit).toHaveBeenNthCalledWith(1, 'updateBadge'); expect(messageAfter?.status).toBe('newstatus'); }); }); describe('setMetadata', () => { it('should set the given message metadata', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); await controller.addMessage({ id: messageId, messageParams: { ...mockMessageParams }, @@ -432,7 +447,7 @@ describe('AbstractTestManager', () => { }); it('should throw an error if message is not found', () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); expect(() => controller.setMetadata(messageId, { foo: 'bar' })).toThrow( 'AbstractMessageManager: Message not found for id: 1.', @@ -442,7 +457,7 @@ describe('AbstractTestManager', () => { describe('waitForFinishStatus', () => { it('signs the message when status is "signed"', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const promise = controller.waitForFinishStatus( { from: fromMock, @@ -452,7 +467,7 @@ describe('AbstractTestManager', () => { ); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'signed', rawSig: rawSigMock, }); @@ -462,7 +477,7 @@ describe('AbstractTestManager', () => { }); it('rejects with an error when status is "rejected"', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const promise = controller.waitForFinishStatus( { from: fromMock, @@ -472,7 +487,7 @@ describe('AbstractTestManager', () => { ); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'rejected', }); }, 100); @@ -483,7 +498,7 @@ describe('AbstractTestManager', () => { }); it('rejects with an error when finishes with unknown status', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const promise = controller.waitForFinishStatus( { from: fromMock, @@ -493,7 +508,7 @@ describe('AbstractTestManager', () => { ); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'unknown', }); }, 100); @@ -508,7 +523,7 @@ describe('AbstractTestManager', () => { }); it('rejects with an error when finishes with errored status', async () => { - const controller = new AbstractTestManager(); + const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS); const promise = controller.waitForFinishStatus( { from: fromMock, @@ -518,7 +533,7 @@ describe('AbstractTestManager', () => { ); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'errored', error: 'error message', }); @@ -529,4 +544,26 @@ describe('AbstractTestManager', () => { ); }); }); + + describe('clearUnapprovedMessages', () => { + it('clears the unapproved messages', () => { + const controller = new AbstractTestManager({ + ...MOCK_INITIAL_OPTIONS, + state: { + unapprovedMessages: { + '1': { + id: '1', + messageParams: { from: '0x1', test: 1 }, + status: 'unapproved', + time: 10, + type: 'type', + }, + }, + unapprovedMessagesCount: 1, + }, + }); + controller.clearUnapprovedMessages(); + expect(controller.getUnapprovedMessagesCount()).toBe(0); + }); + }); }); diff --git a/packages/message-manager/src/AbstractMessageManager.ts b/packages/message-manager/src/AbstractMessageManager.ts index bdd8401f54..10bcc6d452 100644 --- a/packages/message-manager/src/AbstractMessageManager.ts +++ b/packages/message-manager/src/AbstractMessageManager.ts @@ -1,29 +1,41 @@ -import type { BaseConfig, BaseState } from '@metamask/base-controller'; -import { BaseControllerV1 } from '@metamask/base-controller'; +import { BaseController } from '@metamask/base-controller'; +import type { + ActionConstraint, + EventConstraint, + RestrictedControllerMessenger, +} from '@metamask/base-controller'; import type { ApprovalType } from '@metamask/controller-utils'; -import type { Hex, Json } from '@metamask/utils'; +import type { Json } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. // eslint-disable-next-line import/no-nodejs-modules import { EventEmitter } from 'events'; +import type { Draft } from 'immer'; import { v1 as random } from 'uuid'; +const stateMetadata = { + unapprovedMessages: { persist: false, anonymous: false }, + unapprovedMessagesCount: { persist: false, anonymous: false }, +}; + +const getDefaultState = () => ({ + unapprovedMessages: {}, + unapprovedMessagesCount: 0, +}); + /** * @type OriginalRequest * * Represents the original request object for adding a message. * @property origin? - Is it is specified, represents the origin */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface OriginalRequest { +export type OriginalRequest = { id?: number; origin?: string; securityAlertResponse?: Record; -} +}; /** - * @type Message + * @type AbstractMessage * * Represents and contains data about a signing type signature request. * @property id - An id to track and identify the message object @@ -33,10 +45,7 @@ export interface OriginalRequest { * @property securityProviderResponse - Response from a security provider, whether it is malicious or not * @property metadata - Additional data for the message, for example external identifiers */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface AbstractMessage { +export type AbstractMessage = { id: string; time: number; status: string; @@ -46,7 +55,7 @@ export interface AbstractMessage { securityAlertResponse?: Record; metadata?: Json; error?: string; -} +}; /** * @type AbstractMessageParams @@ -57,15 +66,12 @@ export interface AbstractMessage { * @property requestId? - Original request id * @property deferSetAsSigned? - Whether to defer setting the message as signed immediately after the keyring is told to sign it */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface AbstractMessageParams { +export type AbstractMessageParams = { from: string; origin?: string; requestId?: number; deferSetAsSigned?: boolean; -} +}; /** * @type MessageParamsMetamask @@ -76,12 +82,9 @@ export interface AbstractMessageParams { * @property from - Address from which the message is processed * @property origin? - Added for request origin identification */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface AbstractMessageParamsMetamask extends AbstractMessageParams { +export type AbstractMessageParamsMetamask = AbstractMessageParams & { metamaskId?: string; -} +}; /** * @type MessageManagerState @@ -90,15 +93,15 @@ export interface AbstractMessageParamsMetamask extends AbstractMessageParams { * @property unapprovedMessages - A collection of all Messages in the 'unapproved' state * @property unapprovedMessagesCount - The count of all Messages in this.unapprovedMessages */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// TODO: Either fix this lint violation or explain why it's necessary to ignore. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions, @typescript-eslint/naming-convention -export interface MessageManagerState - extends BaseState { - unapprovedMessages: { [key: string]: M }; +export type MessageManagerState = { + unapprovedMessages: Record; unapprovedMessagesCount: number; -} +}; + +export type UpdateBadgeEvent = { + type: `${string}:updateBadge`; + payload: []; +}; /** * A function for verifying a message, whether it is malicious or not @@ -108,32 +111,82 @@ export type SecurityProviderRequest = ( messageType: string, ) => Promise; -// TODO: Either fix this lint violation or explain why it's necessary to ignore. -// eslint-disable-next-line @typescript-eslint/naming-convention -type getCurrentChainId = () => Hex; +/** + * AbstractMessageManager constructor options. + * + * @property additionalFinishStatuses - Optional list of statuses that are accepted to emit a finished event. + * @property messenger - Controller messaging system. + * @property name - The name of the manager. + * @property securityProviderRequest - A function for verifying a message, whether it is malicious or not. + * @property state - Initial state to set on this controller. + */ +export type AbstractMessageManagerOptions< + Message extends AbstractMessage, + Action extends ActionConstraint, + Event extends EventConstraint, +> = { + additionalFinishStatuses?: string[]; + messenger: RestrictedControllerMessenger< + string, + Action, + Event | UpdateBadgeEvent, + string, + string + >; + name: string; + securityProviderRequest?: SecurityProviderRequest; + state?: MessageManagerState; +}; /** * Controller in charge of managing - storing, adding, removing, updating - Messages. */ export abstract class AbstractMessageManager< - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention - M extends AbstractMessage, - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention - P extends AbstractMessageParams, - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/naming-convention - PM extends AbstractMessageParamsMetamask, -> extends BaseControllerV1> { - protected messages: M[]; - - protected getCurrentChainId: getCurrentChainId | undefined; + Message extends AbstractMessage, + Params extends AbstractMessageParams, + ParamsMetamask extends AbstractMessageParamsMetamask, + Action extends ActionConstraint, + Event extends EventConstraint, +> extends BaseController< + string, + MessageManagerState, + RestrictedControllerMessenger< + string, + Action, + Event | UpdateBadgeEvent, + string, + string + > +> { + protected messages: Message[]; private readonly securityProviderRequest: SecurityProviderRequest | undefined; private readonly additionalFinishStatuses: string[]; + internalEvents = new EventEmitter(); + + constructor({ + additionalFinishStatuses, + messenger, + name, + securityProviderRequest, + state = {} as MessageManagerState, + }: AbstractMessageManagerOptions) { + super({ + messenger, + metadata: stateMetadata, + name, + state: { + ...getDefaultState(), + ...state, + }, + }); + this.messages = []; + this.securityProviderRequest = securityProviderRequest; + this.additionalFinishStatuses = additionalFinishStatuses ?? []; + } + /** * Adds request props to the messsage params and returns a new messageParams object. * @param messageParams - The messageParams to add the request props to. @@ -183,11 +236,16 @@ export abstract class AbstractMessageManager< * @param emitUpdateBadge - Whether to emit the updateBadge event. */ protected saveMessageList(emitUpdateBadge = true) { - const unapprovedMessages = this.getUnapprovedMessages(); - const unapprovedMessagesCount = this.getUnapprovedMessagesCount(); - this.update({ unapprovedMessages, unapprovedMessagesCount }); + this.update((state) => { + state.unapprovedMessages = + this.getUnapprovedMessages() as unknown as Record< + string, + Draft + >; + state.unapprovedMessagesCount = this.getUnapprovedMessagesCount(); + }); if (emitUpdateBadge) { - this.hub.emit('updateBadge'); + this.messagingSystem.publish(`${this.name as string}:updateBadge`); } } @@ -200,18 +258,26 @@ export abstract class AbstractMessageManager< protected setMessageStatus(messageId: string, status: string) { const message = this.getMessage(messageId); if (!message) { - throw new Error(`${this.name}: Message not found for id: ${messageId}.`); + throw new Error( + `${this.name as string}: Message not found for id: ${messageId}.`, + ); } - message.status = status; - this.updateMessage(message); - this.hub.emit(`${messageId}:${status}`, message); + const updatedMessage = { + ...message, + status, + }; + this.updateMessage(updatedMessage); + this.internalEvents.emit(`${messageId}:${status}`, updatedMessage); if ( status === 'rejected' || status === 'signed' || status === 'errored' || this.additionalFinishStatuses.includes(status) ) { - this.hub.emit(`${messageId}:finished`, message); + this.internalEvents.emit( + `${messageId as string}:finished`, + updatedMessage, + ); } } @@ -222,7 +288,7 @@ export abstract class AbstractMessageManager< * @param message - A Message that will replace an existing Message (with the id) in this.messages. * @param emitUpdateBadge - Whether to emit the updateBadge event. */ - protected updateMessage(message: M, emitUpdateBadge = true) { + protected updateMessage(message: Message, emitUpdateBadge = true) { const index = this.messages.findIndex((msg) => message.id === msg.id); /* istanbul ignore next */ if (index !== -1) { @@ -237,7 +303,7 @@ export abstract class AbstractMessageManager< * @param message - The message to verify. * @returns A promise that resolves to a secured message with additional security provider response data. */ - private async securityCheck(message: M): Promise { + private async securityCheck(message: Message): Promise { if (this.securityProviderRequest) { const securityProviderResponse = await this.securityProviderRequest( message, @@ -251,42 +317,11 @@ export abstract class AbstractMessageManager< return message; } - /** - * EventEmitter instance used to listen to specific message events - */ - hub: EventEmitter = new EventEmitter(); - - /** - * Name of this controller used during composition - */ - override name = 'AbstractMessageManager'; - - /** - * Creates an AbstractMessageManager instance. - * - * @param config - Initial options used to configure this controller. - * @param state - Initial state to set on this controller. - * @param securityProviderRequest - A function for verifying a message, whether it is malicious or not. - * @param additionalFinishStatuses - Optional list of statuses that are accepted to emit a finished event. - * @param getCurrentChainId - Optional function to get the current chainId. - */ - constructor( - config?: Partial, - state?: Partial>, - securityProviderRequest?: SecurityProviderRequest, - additionalFinishStatuses?: string[], - getCurrentChainId?: getCurrentChainId, - ) { - super(config, state); - this.defaultState = { - unapprovedMessages: {}, - unapprovedMessagesCount: 0, - }; - this.messages = []; - this.securityProviderRequest = securityProviderRequest; - this.additionalFinishStatuses = additionalFinishStatuses ?? []; - this.getCurrentChainId = getCurrentChainId; - this.initialize(); + clearUnapprovedMessages() { + this.update((state) => { + state.unapprovedMessages = {}; + state.unapprovedMessagesCount = 0; + }); } /** @@ -306,10 +341,10 @@ export abstract class AbstractMessageManager< getUnapprovedMessages() { return this.messages .filter((message) => message.status === 'unapproved') - .reduce((result: { [key: string]: M }, message: M) => { + .reduce((result: Record, message) => { result[message.id] = message; return result; - }, {}) as { [key: string]: M }; + }, {}); } /** @@ -318,7 +353,7 @@ export abstract class AbstractMessageManager< * * @param message - The Message to add to this.messages. */ - async addMessage(message: M) { + async addMessage(message: Message) { const securedMessage = await this.securityCheck(message); this.messages.push(securedMessage); this.saveMessageList(); @@ -352,7 +387,7 @@ export abstract class AbstractMessageManager< * plus data added by MetaMask. * @returns Promise resolving to the messageParams with the metamaskId property removed. */ - approveMessage(messageParams: PM): Promise

{ + approveMessage(messageParams: ParamsMetamask): Promise { // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore this.setMessageStatusApproved(messageParams.metamaskId); @@ -414,8 +449,13 @@ export abstract class AbstractMessageManager< if (!message) { return; } - message.rawSig = result; - this.updateMessage(message, false); + this.updateMessage( + { + ...message, + rawSig: result, + }, + false, + ); } /** @@ -424,14 +464,20 @@ export abstract class AbstractMessageManager< * @param messageId - The id of the Message to update * @param metadata - The data with which to replace the metadata property in the message */ - setMetadata(messageId: string, metadata: Json) { const message = this.getMessage(messageId); if (!message) { - throw new Error(`${this.name}: Message not found for id: ${messageId}.`); + throw new Error( + `${this.name as string}: Message not found for id: ${messageId}.`, + ); } - message.metadata = metadata; - this.updateMessage(message, false); + this.updateMessage( + { + ...message, + metadata, + }, + false, + ); } /** @@ -441,7 +487,9 @@ export abstract class AbstractMessageManager< * @param messageParams - The messageParams to modify * @returns Promise resolving to the messageParams with the metamaskId property removed */ - abstract prepMessageForSigning(messageParams: PM): Promise

; + abstract prepMessageForSigning( + messageParams: ParamsMetamask, + ): Promise; /** * Creates a new Message with an 'unapproved' status using the passed messageParams. @@ -454,7 +502,7 @@ export abstract class AbstractMessageManager< * @returns The id of the newly created message. */ abstract addUnapprovedMessage( - messageParams: PM, + messageParams: ParamsMetamask, request: OriginalRequest, version?: string, ): Promise; @@ -481,34 +529,35 @@ export abstract class AbstractMessageManager< ): Promise { const { metamaskId: messageId, ...messageParams } = messageParamsWithId; return new Promise((resolve, reject) => { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - this.hub.once(`${messageId}:finished`, (data: AbstractMessage) => { - switch (data.status) { - case 'signed': - return resolve(data.rawSig as string); - case 'rejected': - return reject( - new Error( - `MetaMask ${messageName} Signature: User denied message signature.`, - ), - ); - case 'errored': - return reject( - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - new Error(`MetaMask ${messageName} Signature: ${data.error}`), - ); - default: - return reject( - new Error( - `MetaMask ${messageName} Signature: Unknown problem: ${JSON.stringify( - messageParams, - )}`, - ), - ); - } - }); + this.internalEvents.once( + `${messageId as string}:finished`, + (data: AbstractMessage) => { + switch (data.status) { + case 'signed': + return resolve(data.rawSig as string); + case 'rejected': + return reject( + new Error( + `MetaMask ${messageName} Signature: User denied message signature.`, + ), + ); + case 'errored': + return reject( + new Error( + `MetaMask ${messageName} Signature: ${data.error as string}`, + ), + ); + default: + return reject( + new Error( + `MetaMask ${messageName} Signature: Unknown problem: ${JSON.stringify( + messageParams, + )}`, + ), + ); + } + }, + ); }); } } diff --git a/packages/message-manager/src/DecryptMessageManager.test.ts b/packages/message-manager/src/DecryptMessageManager.test.ts index d81b336141..1e59533ae5 100644 --- a/packages/message-manager/src/DecryptMessageManager.test.ts +++ b/packages/message-manager/src/DecryptMessageManager.test.ts @@ -1,4 +1,18 @@ import { DecryptMessageManager } from './DecryptMessageManager'; +import type { DecryptMessageManagerMessenger } from './DecryptMessageManager'; + +const mockMessenger = { + registerActionHandler: jest.fn(), + registerInitialEventPayload: jest.fn(), + publish: jest.fn(), + clearEventSubscriptions: jest.fn(), +} as unknown as DecryptMessageManagerMessenger; + +const mockInitialOptions = { + additionalFinishStatuses: undefined, + messenger: mockMessenger, + securityProviderRequest: undefined, +}; describe('DecryptMessageManager', () => { let controller: DecryptMessageManager; @@ -9,7 +23,7 @@ describe('DecryptMessageManager', () => { const dataMock = '0x12345'; beforeEach(() => { - controller = new DecryptMessageManager(); + controller = new DecryptMessageManager(mockInitialOptions); }); it('sets default state', () => { @@ -19,10 +33,6 @@ describe('DecryptMessageManager', () => { }); }); - it('sets default config', () => { - expect(controller.config).toStrictEqual({}); - }); - it('adds a valid message', async () => { const messageData = '0x123'; const messageTime = Date.now(); @@ -52,9 +62,7 @@ describe('DecryptMessageManager', () => { describe('addUnapprovedMessageAsync', () => { beforeEach(() => { - controller = new DecryptMessageManager(undefined, undefined, undefined, [ - 'decrypted', - ]); + controller = new DecryptMessageManager(mockInitialOptions); jest .spyOn(controller, 'addUnapprovedMessage') @@ -72,7 +80,7 @@ describe('DecryptMessageManager', () => { data: dataMock, }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'decrypted', rawSig: rawSigMock, }); @@ -88,7 +96,7 @@ describe('DecryptMessageManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'rejected', }); }, 100); @@ -105,7 +113,7 @@ describe('DecryptMessageManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'errored', }); }, 100); @@ -122,7 +130,7 @@ describe('DecryptMessageManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'unknown', }); }, 100); diff --git a/packages/message-manager/src/DecryptMessageManager.ts b/packages/message-manager/src/DecryptMessageManager.ts index 4036e79d90..571ca9e760 100644 --- a/packages/message-manager/src/DecryptMessageManager.ts +++ b/packages/message-manager/src/DecryptMessageManager.ts @@ -1,14 +1,52 @@ +import type { + ActionConstraint, + EventConstraint, + RestrictedControllerMessenger, +} from '@metamask/base-controller'; import { ApprovalType } from '@metamask/controller-utils'; import type { AbstractMessage, AbstractMessageParams, AbstractMessageParamsMetamask, + MessageManagerState, OriginalRequest, + SecurityProviderRequest, } from './AbstractMessageManager'; import { AbstractMessageManager } from './AbstractMessageManager'; import { normalizeMessageData, validateDecryptedMessageData } from './utils'; +const managerName = 'DecryptMessageManager'; + +export type DecryptMessageManagerState = MessageManagerState; + +export type DecryptMessageManagerUnapprovedMessageAddedEvent = { + type: `${typeof managerName}:unapprovedMessage`; + payload: [AbstractMessageParamsMetamask]; +}; + +export type DecryptMessageManagerUpdateBadgeEvent = { + type: `${typeof managerName}:updateBadge`; + payload: []; +}; + +export type DecryptMessageManagerMessenger = RestrictedControllerMessenger< + string, + ActionConstraint, + | EventConstraint + | DecryptMessageManagerUnapprovedMessageAddedEvent + | DecryptMessageManagerUpdateBadgeEvent, + string, + string +>; + +type DecryptMessageManagerOptions = { + messenger: DecryptMessageManagerMessenger; + securityProviderRequest?: SecurityProviderRequest; + state?: MessageManagerState; + additionalFinishStatuses?: string[]; +}; + /** * @type DecryptMessage * @@ -19,12 +57,9 @@ import { normalizeMessageData, validateDecryptedMessageData } from './utils'; * @property type - The json-prc signing method for which a signature request has been made. * A 'DecryptMessage' which always has a 'eth_decrypt' type */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface DecryptMessage extends AbstractMessage { +export type DecryptMessage = AbstractMessage & { messageParams: DecryptMessageParams; -} +}; /** * @type DecryptMessageParams @@ -32,12 +67,9 @@ export interface DecryptMessage extends AbstractMessage { * Represents the parameters to pass to the eth_decrypt method once the request is approved. * @property data - A hex string conversion of the raw buffer data of the signature request */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface DecryptMessageParams extends AbstractMessageParams { +export type DecryptMessageParams = AbstractMessageParams & { data: string; -} +}; /** * @type DecryptMessageParamsMetamask @@ -63,12 +95,26 @@ export interface DecryptMessageParamsMetamask export class DecryptMessageManager extends AbstractMessageManager< DecryptMessage, DecryptMessageParams, - DecryptMessageParamsMetamask + DecryptMessageParamsMetamask, + ActionConstraint, + | EventConstraint + | DecryptMessageManagerUnapprovedMessageAddedEvent + | DecryptMessageManagerUpdateBadgeEvent > { - /** - * Name of this controller used during composition - */ - override name = 'DecryptMessageManager' as const; + constructor({ + additionalFinishStatuses, + messenger, + securityProviderRequest, + state, + }: DecryptMessageManagerOptions) { + super({ + additionalFinishStatuses, + messenger, + name: managerName, + securityProviderRequest, + state, + }); + } /** * Creates a new Message with an 'unapproved' status using the passed messageParams. @@ -86,32 +132,35 @@ export class DecryptMessageManager extends AbstractMessageManager< const messageId = await this.addUnapprovedMessage(messageParams, req); return new Promise((resolve, reject) => { - this.hub.once(`${messageId}:finished`, (data: DecryptMessage) => { - switch (data.status) { - case 'decrypted': - return resolve(data.rawSig as string); - case 'rejected': - return reject( - new Error( - 'MetaMask DecryptMessage: User denied message decryption.', - ), - ); - case 'errored': - return reject( - new Error( - 'MetaMask DecryptMessage: This message cannot be decrypted.', - ), - ); - default: - return reject( - new Error( - `MetaMask DecryptMessage: Unknown problem: ${JSON.stringify( - messageParams, - )}`, - ), - ); - } - }); + this.internalEvents.once( + `${messageId}:finished`, + (data: DecryptMessage) => { + switch (data.status) { + case 'decrypted': + return resolve(data.rawSig as string); + case 'rejected': + return reject( + new Error( + 'MetaMask DecryptMessage: User denied message decryption.', + ), + ); + case 'errored': + return reject( + new Error( + 'MetaMask DecryptMessage: This message cannot be decrypted.', + ), + ); + default: + return reject( + new Error( + `MetaMask DecryptMessage: Unknown problem: ${JSON.stringify( + messageParams, + )}`, + ), + ); + } + }, + ); }); } @@ -144,7 +193,7 @@ export class DecryptMessageManager extends AbstractMessageManager< const messageId = messageData.id; await this.addMessage(messageData); - this.hub.emit(`unapprovedMessage`, { + this.messagingSystem.publish(`${managerName}:unapprovedMessage`, { ...updatedMessageParams, metamaskId: messageId, }); diff --git a/packages/message-manager/src/EncryptionPublicKeyManager.test.ts b/packages/message-manager/src/EncryptionPublicKeyManager.test.ts index 8617c16534..81735a5fda 100644 --- a/packages/message-manager/src/EncryptionPublicKeyManager.test.ts +++ b/packages/message-manager/src/EncryptionPublicKeyManager.test.ts @@ -1,4 +1,18 @@ import { EncryptionPublicKeyManager } from './EncryptionPublicKeyManager'; +import type { EncryptionPublicKeyManagerMessenger } from './EncryptionPublicKeyManager'; + +const mockMessenger = { + registerActionHandler: jest.fn(), + registerInitialEventPayload: jest.fn(), + publish: jest.fn(), + clearEventSubscriptions: jest.fn(), +} as unknown as EncryptionPublicKeyManagerMessenger; + +const mockInitialOptions = { + additionalFinishStatuses: undefined, + messenger: mockMessenger, + securityProviderRequest: undefined, +}; describe('EncryptionPublicKeyManager', () => { let controller: EncryptionPublicKeyManager; @@ -8,7 +22,7 @@ describe('EncryptionPublicKeyManager', () => { const rawSigMock = '231124fe67213512='; beforeEach(() => { - controller = new EncryptionPublicKeyManager(); + controller = new EncryptionPublicKeyManager(mockInitialOptions); }); it('sets default state', () => { @@ -18,10 +32,6 @@ describe('EncryptionPublicKeyManager', () => { }); }); - it('sets default config', () => { - expect(controller.config).toStrictEqual({}); - }); - it('adds a valid message', async () => { const messageTime = Date.now(); const messageStatus = 'unapproved'; @@ -48,12 +58,7 @@ describe('EncryptionPublicKeyManager', () => { describe('addUnapprovedMessageAsync', () => { beforeEach(() => { - controller = new EncryptionPublicKeyManager( - undefined, - undefined, - undefined, - ['received'], - ); + controller = new EncryptionPublicKeyManager(mockInitialOptions); jest .spyOn(controller, 'addUnapprovedMessage') @@ -70,7 +75,7 @@ describe('EncryptionPublicKeyManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'received', rawSig: rawSigMock, }); @@ -85,7 +90,7 @@ describe('EncryptionPublicKeyManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'rejected', }); }, 100); @@ -101,7 +106,7 @@ describe('EncryptionPublicKeyManager', () => { }); setTimeout(() => { - controller.hub.emit(`${messageIdMock}:finished`, { + controller.internalEvents.emit(`${messageIdMock}:finished`, { status: 'unknown', }); }, 100); diff --git a/packages/message-manager/src/EncryptionPublicKeyManager.ts b/packages/message-manager/src/EncryptionPublicKeyManager.ts index 6fdf1518f7..6654bc0132 100644 --- a/packages/message-manager/src/EncryptionPublicKeyManager.ts +++ b/packages/message-manager/src/EncryptionPublicKeyManager.ts @@ -1,14 +1,53 @@ +import type { + ActionConstraint, + EventConstraint, + RestrictedControllerMessenger, +} from '@metamask/base-controller'; import { ApprovalType } from '@metamask/controller-utils'; import type { AbstractMessage, AbstractMessageParams, AbstractMessageParamsMetamask, + MessageManagerState, OriginalRequest, + SecurityProviderRequest, } from './AbstractMessageManager'; import { AbstractMessageManager } from './AbstractMessageManager'; import { validateEncryptionPublicKeyMessageData } from './utils'; +const managerName = 'EncryptionPublicKeyManager'; + +export type EncryptionPublicKeyManagerState = + MessageManagerState; + +export type EncryptionPublicKeyManagerUnapprovedMessageAddedEvent = { + type: `${typeof managerName}:unapprovedMessage`; + payload: [AbstractMessageParamsMetamask]; +}; + +export type EncryptionPublicKeyManagerUpdateBadgeEvent = { + type: `${typeof managerName}:updateBadge`; + payload: []; +}; + +export type EncryptionPublicKeyManagerMessenger = RestrictedControllerMessenger< + string, + ActionConstraint, + | EventConstraint + | EncryptionPublicKeyManagerUnapprovedMessageAddedEvent + | EncryptionPublicKeyManagerUpdateBadgeEvent, + string, + string +>; + +type EncryptionPublicKeyManagerOptions = { + messenger: EncryptionPublicKeyManagerMessenger; + securityProviderRequest?: SecurityProviderRequest; + state?: MessageManagerState; + additionalFinishStatuses?: string[]; +}; + /** * @type EncryptionPublicKey * @@ -20,12 +59,9 @@ import { validateEncryptionPublicKeyMessageData } from './utils'; * A 'Message' which always has a 'eth_getEncryptionPublicKey' type * @property rawSig - Encryption public key */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface EncryptionPublicKey extends AbstractMessage { +export type EncryptionPublicKey = AbstractMessage & { messageParams: EncryptionPublicKeyParams; -} +}; /** * @type EncryptionPublicKeyParams @@ -46,13 +82,10 @@ export type EncryptionPublicKeyParams = AbstractMessageParams; * @property from - Address from which to extract the encryption public key * @property origin? - Added for request origin identification */ -// This interface was created before this ESLint rule was added. -// Convert to a `type` in a future major version. -// eslint-disable-next-line @typescript-eslint/consistent-type-definitions -export interface EncryptionPublicKeyParamsMetamask - extends AbstractMessageParamsMetamask { - data: string; -} +export type EncryptionPublicKeyParamsMetamask = + AbstractMessageParamsMetamask & { + data: string; + }; /** * Controller in charge of managing - storing, adding, removing, updating - Messages. @@ -60,12 +93,26 @@ export interface EncryptionPublicKeyParamsMetamask export class EncryptionPublicKeyManager extends AbstractMessageManager< EncryptionPublicKey, EncryptionPublicKeyParams, - EncryptionPublicKeyParamsMetamask + EncryptionPublicKeyParamsMetamask, + ActionConstraint, + | EventConstraint + | EncryptionPublicKeyManagerUnapprovedMessageAddedEvent + | EncryptionPublicKeyManagerUpdateBadgeEvent > { - /** - * Name of this controller used during composition - */ - override name = 'EncryptionPublicKeyManager' as const; + constructor({ + additionalFinishStatuses, + messenger, + securityProviderRequest, + state, + }: EncryptionPublicKeyManagerOptions) { + super({ + additionalFinishStatuses, + messenger, + name: managerName, + securityProviderRequest, + state, + }); + } /** * Creates a new Message with an 'unapproved' status using the passed messageParams. @@ -83,26 +130,29 @@ export class EncryptionPublicKeyManager extends AbstractMessageManager< const messageId = await this.addUnapprovedMessage(messageParams, req); return new Promise((resolve, reject) => { - this.hub.once(`${messageId}:finished`, (data: EncryptionPublicKey) => { - switch (data.status) { - case 'received': - return resolve(data.rawSig as string); - case 'rejected': - return reject( - new Error( - 'MetaMask EncryptionPublicKey: User denied message EncryptionPublicKey.', - ), - ); - default: - return reject( - new Error( - `MetaMask EncryptionPublicKey: Unknown problem: ${JSON.stringify( - messageParams, - )}`, - ), - ); - } - }); + this.internalEvents.once( + `${messageId}:finished`, + (data: EncryptionPublicKey) => { + switch (data.status) { + case 'received': + return resolve(data.rawSig as string); + case 'rejected': + return reject( + new Error( + 'MetaMask EncryptionPublicKey: User denied message EncryptionPublicKey.', + ), + ); + default: + return reject( + new Error( + `MetaMask EncryptionPublicKey: Unknown problem: ${JSON.stringify( + messageParams, + )}`, + ), + ); + } + }, + ); }); } @@ -134,7 +184,7 @@ export class EncryptionPublicKeyManager extends AbstractMessageManager< const messageId = messageData.id; await this.addMessage(messageData); - this.hub.emit(`unapprovedMessage`, { + this.messagingSystem.publish(`${this.name as string}:unapprovedMessage`, { ...updatedMessageParams, metamaskId: messageId, }); From 7409b3fa6eddc1fbc3dad4a55b59db676b7c3211 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Tue, 14 Jan 2025 11:56:06 -0700 Subject: [PATCH 4/4] Fix ESLint config (#5132) The upgrade to ESLint 9 which occurred in a previous commit was not performed correctly. It seems that an array must be passed to `createConfig` instead of a series of arguments, and that was not done. As a result, a lot of lint rules were disabled accidentally. This commit fixes the ESLint config and corrects any lint violations. Unsurprisingly, upgrading the ESLint packages created a bunch of new lint violations, so those have been turned into warnings rather than errors in this commit. To prevent new instances of these violations from occurring, however, a script has been added which acts as a quality gate, running `eslint` as before, but then capturing existing warning counts and comparing them to a previously generated threshold file, exiting with non-zero if there is an increase in warnings for any particular ESLint rule. This script is also now used for `yarn lint:eslint` so that CI will fail if the count increases. --- docs/contributing.md | 4 +- eslint-warning-thresholds.json | 29 ++ eslint.config.mjs | 129 +++++--- package.json | 2 +- packages/controller-utils/package.json | 1 + packages/controller-utils/src/util.ts | 2 +- packages/json-rpc-engine/src/JsonRpcEngine.ts | 2 +- .../src/index.test.ts | 2 +- .../src/KeyringController.ts | 1 - .../src/AbstractMessageManager.ts | 2 +- .../src/create-network-client.ts | 2 - .../tests/NetworkController.test.ts | 7 +- .../services/push/push-web.ts | 2 +- .../src/PermissionController.ts | 1 - .../src/PhishingDetector.test.ts | 2 +- .../src/AbstractPollingController.ts | 1 - .../network-syncing/controller-integration.ts | 2 +- .../tests/SelectedNetworkController.test.ts | 2 +- .../src/SignatureController.ts | 4 +- .../src/TransactionController.ts | 3 +- .../src/helpers/GasFeePoller.ts | 2 +- .../src/helpers/IncomingTransactionHelper.ts | 2 +- .../src/helpers/MethodDataHelper.ts | 2 +- .../src/helpers/PendingTransactionTracker.ts | 2 +- .../src/UserOperationController.ts | 2 +- .../helpers/PendingUserOperationTracker.ts | 2 +- scripts/run-eslint.ts | 289 ++++++++++++++++++ yarn.config.cjs | 7 +- yarn.lock | 13 +- 29 files changed, 448 insertions(+), 73 deletions(-) create mode 100644 eslint-warning-thresholds.json create mode 100644 scripts/run-eslint.ts diff --git a/docs/contributing.md b/docs/contributing.md index 43b9230d95..e9007a0ccf 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -46,9 +46,9 @@ If you need to customize the behavior of Jest for a package, see `jest.config.js ## Linting -[ESLint](https://eslint.org/docs/v8.x/) v8 (via [MetaMask's shared ESLint configurations](https://github.com/MetaMask/eslint-config)) is used to check for code quality issues, and [Prettier](https://prettier.io/docs/en/) is used to format files. +[ESLint](https://eslint.org) v9 (via [MetaMask's shared ESLint configurations](https://github.com/MetaMask/eslint-config)) is used to check for code quality issues, and [Prettier](https://prettier.io/docs/en/) is used to format files. -If you need to customize the behavior of ESLint, see `.eslintrc.js` in the root. +If you need to customize the behavior of ESLint, see `eslint.config.mjs` in the root. - Run `yarn lint` to lint all files and show possible violations across the monorepo. - Run `yarn lint:fix` to fix any automatically fixable violations. diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json new file mode 100644 index 0000000000..dcad81bed1 --- /dev/null +++ b/eslint-warning-thresholds.json @@ -0,0 +1,29 @@ +{ + "@typescript-eslint/consistent-type-exports": 19, + "@typescript-eslint/no-base-to-string": 3, + "@typescript-eslint/no-duplicate-enum-values": 2, + "@typescript-eslint/no-misused-promises": 3, + "@typescript-eslint/no-unsafe-enum-comparison": 59, + "@typescript-eslint/no-unused-vars": 36, + "@typescript-eslint/prefer-promise-reject-errors": 13, + "@typescript-eslint/prefer-readonly": 152, + "@typescript-eslint/switch-exhaustiveness-check": 10, + "import-x/namespace": 189, + "import-x/no-named-as-default": 1, + "import-x/no-named-as-default-member": 8, + "import-x/order": 209, + "jest/no-conditional-in-test": 104, + "jsdoc/check-tag-names": 372, + "jsdoc/require-returns": 22, + "jsdoc/tag-lines": 329, + "n/no-unsupported-features/node-builtins": 18, + "n/prefer-global/text-encoder": 4, + "n/prefer-global/text-decoder": 4, + "prettier/prettier": 116, + "promise/always-return": 3, + "promise/catch-or-return": 2, + "promise/param-names": 8, + "no-empty-function": 2, + "no-shadow": 8, + "no-unused-private-class-members": 6 +} diff --git a/eslint.config.mjs b/eslint.config.mjs index ec01ff56f2..91185d8cb8 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -1,19 +1,12 @@ import base, { createConfig } from '@metamask/eslint-config'; -import nodejs from '@metamask/eslint-config-nodejs'; import jest from '@metamask/eslint-config-jest'; +import nodejs from '@metamask/eslint-config-nodejs'; import typescript from '@metamask/eslint-config-typescript'; -const config = createConfig( +const config = createConfig([ + ...base, { ignores: [ - 'yarn.lock', - '**/**.map', - '**/**.tsbuildinfo', - '**/*.json', - '**/*.md', - '**/LICENSE', - '**/*.sh', - '**/.DS_Store', '**/dist/**', '**/docs/**', '**/coverage/**', @@ -22,7 +15,6 @@ const config = createConfig( 'scripts/create-package/package-template/**', ], }, - ...base, { rules: { // Left disabled because various properties throughough this repo are snake_case because the @@ -32,14 +24,12 @@ const config = createConfig( 'id-length': 'off', // TODO: re-enble most of these rules - '@typescript-eslint/naming-convention': 'off', 'function-paren-newline': 'off', 'id-denylist': 'off', 'implicit-arrow-linebreak': 'off', - 'import/no-anonymous-default-export': 'off', - 'import/no-unassigned-import': 'off', + 'import-x/no-anonymous-default-export': 'off', + 'import-x/no-unassigned-import': 'off', 'lines-around-comment': 'off', - 'n/no-sync': 'off', 'no-async-promise-executor': 'off', 'no-case-declarations': 'off', 'no-invalid-this': 'off', @@ -53,6 +43,12 @@ const config = createConfig( 'off', { matchDescription: '^[A-Z`\\d_][\\s\\S]*[.?!`>)}]$' }, ], + + // TODO: These rules created more errors after the upgrade to ESLint 9. + // Re-enable these rules and address any lint violations. + 'import-x/no-named-as-default-member': 'warn', + 'prettier/prettier': 'warn', + 'no-empty-function': 'warn', }, settings: { jsdoc: { @@ -62,26 +58,36 @@ const config = createConfig( }, { files: [ - '**/jest.config.js', - '**/jest.environment.js', - '**/tests/**/*.{ts,js}', - '*.js', - '*.test.{ts,js}', + '**/*.{js,cjs,mjs}', + '**/*.test.{js,ts}', + '**/tests/**/*.{js,ts}', 'scripts/*.ts', - 'scripts/create-package/*.ts', - 'yarn.config.cjs', + 'scripts/create-package/**/*.ts', ], extends: [nodejs], + rules: { + // TODO: Re-enable this + 'n/no-sync': 'off', + // TODO: These rules created more errors after the upgrade to ESLint 9. + // Re-enable these rules and address any lint violations. + 'n/no-unsupported-features/node-builtins': 'warn', + }, }, { - files: ['*.test.{ts,js}', '**/tests/**/*.{ts,js}'], + files: ['**/*.test.{js,ts}', '**/tests/**/*.{js,ts}'], extends: [jest], + rules: { + // TODO: These rules created more errors after the upgrade to ESLint 9. + // Re-enable these rules and address any lint violations. + 'jest/no-conditional-in-test': 'warn', + }, }, { // These files are test helpers, not tests. We still use the Jest ESLint // config here to ensure that ESLint expects a test-like environment, but // various rules meant just to apply to tests have been disabled. - files: ['**/tests/**/*.{ts,js}', '!*.test.{ts,js}'], + files: ['**/tests/**/*.{js,ts}'], + ignores: ['**/*.test.{js,ts}'], rules: { 'jest/no-export': 'off', 'jest/require-top-level-describe': 'off', @@ -89,20 +95,31 @@ const config = createConfig( }, }, { - files: ['*.js', '*.cjs'], - parserOptions: { + files: ['**/*.{js,cjs}'], + languageOptions: { sourceType: 'script', - ecmaVersion: '2020', + ecmaVersion: 2020, }, }, { - files: ['*.ts'], + files: ['**/*.ts'], extends: [typescript], - parserOptions: { - tsconfigRootDir: import.meta.dirname, - project: ['./tsconfig.packages.json'], + languageOptions: { + parserOptions: { + tsconfigRootDir: import.meta.dirname, + project: './tsconfig.packages.json', + // Disable `projectService` because we run into out-of-memory issues. + // See this ticket for inspiration out how to solve this: + // + projectService: false, + }, }, rules: { + // This rule does not detect multiple imports of the same file where types + // are being imported in one case and runtime values are being imported in + // another + 'import-x/no-duplicates': 'off', + // Enable rules that are disabled in `@metamask/eslint-config-typescript` '@typescript-eslint/no-explicit-any': 'error', @@ -110,6 +127,7 @@ const config = createConfig( '@typescript-eslint/promise-function-async': 'off', // TODO: re-enable most of these rules + '@typescript-eslint/naming-convention': 'off', '@typescript-eslint/no-unnecessary-type-assertion': 'off', '@typescript-eslint/unbound-method': 'off', '@typescript-eslint/prefer-enum-initializers': 'off', @@ -118,26 +136,51 @@ const config = createConfig( '@typescript-eslint/prefer-reduce-type-parameter': 'off', 'no-restricted-syntax': 'off', 'no-restricted-globals': 'off', + + // TODO: These rules created more errors after the upgrade to ESLint 9. + // Re-enable these rules and address any lint violations. + '@typescript-eslint/consistent-type-exports': 'warn', + '@typescript-eslint/explicit-function-return-type': 'off', + '@typescript-eslint/no-base-to-string': 'warn', + '@typescript-eslint/no-duplicate-enum-values': 'warn', + '@typescript-eslint/no-misused-promises': 'warn', + '@typescript-eslint/no-unsafe-enum-comparison': 'warn', + '@typescript-eslint/no-unused-vars': 'warn', + '@typescript-eslint/only-throw-error': 'warn', + '@typescript-eslint/prefer-promise-reject-errors': 'warn', + '@typescript-eslint/prefer-readonly': 'warn', + '@typescript-eslint/switch-exhaustiveness-check': 'warn', + 'import-x/namespace': 'warn', + 'import-x/no-named-as-default': 'warn', + 'import-x/order': 'warn', + 'jsdoc/check-tag-names': 'warn', + 'jsdoc/require-returns': 'warn', + 'jsdoc/tag-lines': 'warn', + 'no-unused-private-class-members': 'warn', + 'promise/always-return': 'warn', + 'promise/catch-or-return': 'warn', + 'promise/param-names': 'warn', }, }, { files: ['tests/setupAfterEnv/matchers.ts'], - parserOptions: { + languageOptions: { sourceType: 'script', }, }, + // This should really be in `@metamask/eslint-config-typescript` { - files: ['*.d.ts'], + files: ['**/*.d.ts'], rules: { '@typescript-eslint/naming-convention': 'warn', - 'import/unambiguous': 'off', + 'import-x/unambiguous': 'off', }, }, { files: ['scripts/*.ts'], rules: { - // All scripts will have shebangs. - 'n/shebang': 'off', + // Scripts may be self-executable and thus have hashbangs. + 'n/hashbang': 'off', }, }, { @@ -145,8 +188,20 @@ const config = createConfig( rules: { // These files run under Node, and thus `require(...)` is expected. 'n/global-require': 'off', + + // TODO: These rules created more errors after the upgrade to ESLint 9. + // Re-enable these rules and address any lint violations. + 'n/prefer-global/text-encoder': 'warn', + 'n/prefer-global/text-decoder': 'warn', + 'no-shadow': 'warn', + }, + }, + { + files: ['**/*.mjs'], + languageOptions: { + sourceType: 'module', }, }, -); +]); export default config; diff --git a/package.json b/package.json index acd475a8e8..a52e92956a 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,7 @@ "lint": "yarn lint:eslint && yarn lint:misc --check && yarn constraints && yarn lint:dependencies && yarn lint:teams", "lint:dependencies": "depcheck && yarn dedupe --check", "lint:dependencies:fix": "depcheck && yarn dedupe", - "lint:eslint": "eslint . --cache", + "lint:eslint": "yarn ts-node ./scripts/run-eslint.ts --cache", "lint:fix": "yarn lint:eslint --fix && yarn lint:misc --write && yarn constraints --fix && yarn lint:dependencies:fix", "lint:misc": "prettier --no-error-on-unmatched-pattern '**/*.json' '**/*.md' '**/*.yml' '!.yarnrc.yml' '!merged-packages/**' --ignore-path .gitignore", "lint:teams": "ts-node scripts/lint-teams-json.ts", diff --git a/packages/controller-utils/package.json b/packages/controller-utils/package.json index cb2ec2ee88..4cc3ceb4bf 100644 --- a/packages/controller-utils/package.json +++ b/packages/controller-utils/package.json @@ -65,6 +65,7 @@ "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", "jest": "^27.5.1", + "jest-environment-jsdom": "^27.5.1", "nock": "^13.3.1", "sinon": "^9.2.4", "ts-jest": "^27.1.4", diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index 14b41caedb..7f3358b64c 100644 --- a/packages/controller-utils/src/util.ts +++ b/packages/controller-utils/src/util.ts @@ -618,7 +618,7 @@ function logOrRethrowError(error: unknown, codesToCatch: number[] = []) { throw error; } } else { - // eslint-disable-next-line @typescript-eslint/no-throw-literal + // eslint-disable-next-line @typescript-eslint/only-throw-error throw error; } } diff --git a/packages/json-rpc-engine/src/JsonRpcEngine.ts b/packages/json-rpc-engine/src/JsonRpcEngine.ts index 62fb5204d0..c589e13358 100644 --- a/packages/json-rpc-engine/src/JsonRpcEngine.ts +++ b/packages/json-rpc-engine/src/JsonRpcEngine.ts @@ -489,7 +489,7 @@ export class JsonRpcEngine extends SafeEventEmitter { // Now we re-throw the middleware processing error, if any, to catch it // further up the call chain. if (error) { - // eslint-disable-next-line @typescript-eslint/no-throw-literal + // eslint-disable-next-line @typescript-eslint/only-throw-error throw error; } } diff --git a/packages/json-rpc-middleware-stream/src/index.test.ts b/packages/json-rpc-middleware-stream/src/index.test.ts index d770cad07b..6395c629a7 100644 --- a/packages/json-rpc-middleware-stream/src/index.test.ts +++ b/packages/json-rpc-middleware-stream/src/index.test.ts @@ -8,7 +8,7 @@ import { createStreamMiddleware, createEngineStream } from '.'; const artificialDelay = async (time = 0) => new Promise((resolve) => setTimeout(resolve, time)); // TODO: Replace `any` with type -// eslint-disable-next-line @typescript-eslint/no-empty-function, @typescript-eslint/no-explicit-any +// eslint-disable-next-line @typescript-eslint/no-empty-function,@typescript-eslint/no-explicit-any const noop = function (_a: any) {}; const jsonrpc = '2.0' as const; diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 4cc90868a8..bf5b853aee 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1580,7 +1580,6 @@ export class KeyringController extends BaseController< * @deprecated Use `withKeyring` instead. */ async cancelQRSynchronization(): Promise { - // eslint-disable-next-line n/no-sync (await this.getOrAddQRKeyring()).cancelSync(); } diff --git a/packages/message-manager/src/AbstractMessageManager.ts b/packages/message-manager/src/AbstractMessageManager.ts index 10bcc6d452..7027acd686 100644 --- a/packages/message-manager/src/AbstractMessageManager.ts +++ b/packages/message-manager/src/AbstractMessageManager.ts @@ -7,7 +7,7 @@ import type { import type { ApprovalType } from '@metamask/controller-utils'; import type { Json } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import { EventEmitter } from 'events'; import type { Draft } from 'immer'; import { v1 as random } from 'uuid'; diff --git a/packages/network-controller/src/create-network-client.ts b/packages/network-controller/src/create-network-client.ts index dab4702a94..e662018487 100644 --- a/packages/network-controller/src/create-network-client.ts +++ b/packages/network-controller/src/create-network-client.ts @@ -71,7 +71,6 @@ export function createNetworkClient( const rpcProvider = providerFromMiddleware(rpcApiMiddleware); const blockTrackerOpts = - // eslint-disable-next-line n/no-process-env process.env.IN_TEST && networkConfig.type === 'custom' ? { pollingInterval: SECOND } : {}; @@ -190,7 +189,6 @@ function createCustomNetworkMiddleware({ chainId: Hex; rpcApiMiddleware: JsonRpcMiddleware; }): JsonRpcMiddleware { - // eslint-disable-next-line n/no-process-env const testMiddlewares = process.env.IN_TEST ? [createEstimateGasDelayTestMiddleware()] : []; diff --git a/packages/network-controller/tests/NetworkController.test.ts b/packages/network-controller/tests/NetworkController.test.ts index 3f224b7170..009fdc9513 100644 --- a/packages/network-controller/tests/NetworkController.test.ts +++ b/packages/network-controller/tests/NetworkController.test.ts @@ -13069,6 +13069,7 @@ function refreshNetworkTests({ initialState?: Partial; operation: (controller: NetworkController) => Promise; }) { + // eslint-disable-next-line jest/require-top-level-describe it('emits networkWillChange with state payload', async () => { await withController( { @@ -13097,6 +13098,7 @@ function refreshNetworkTests({ ); }); + // eslint-disable-next-line jest/require-top-level-describe it('emits networkDidChange with state payload', async () => { await withController( { @@ -13126,6 +13128,7 @@ function refreshNetworkTests({ }); if (expectedNetworkClientConfiguration.type === NetworkClientType.Custom) { + // eslint-disable-next-line jest/require-top-level-describe it('sets the provider to a custom RPC provider initialized with the RPC target and chain ID', async () => { await withController( { @@ -13167,8 +13170,7 @@ function refreshNetworkTests({ ); }); } else { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + // eslint-disable-next-line jest/require-top-level-describe it(`sets the provider to an Infura provider pointed to ${expectedNetworkClientConfiguration.network}`, async () => { await withController( { @@ -13209,6 +13211,7 @@ function refreshNetworkTests({ }); } + // eslint-disable-next-line jest/require-top-level-describe it('replaces the provider object underlying the provider proxy without creating a new instance of the proxy itself', async () => { await withController( { diff --git a/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts b/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts index ccfab4e40f..67b46f68d0 100644 --- a/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts +++ b/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts @@ -20,7 +20,7 @@ import type { PushNotificationEnv } from '../../types/firebase'; declare const self: ServiceWorkerGlobalScope; // Exported to help testing -// eslint-disable-next-line import/no-mutable-exports +// eslint-disable-next-line import-x/no-mutable-exports export let supportedCache: boolean | null = null; const getPushAvailability = async () => { diff --git a/packages/permission-controller/src/PermissionController.ts b/packages/permission-controller/src/PermissionController.ts index b5e33393f8..27cf026d01 100644 --- a/packages/permission-controller/src/PermissionController.ts +++ b/packages/permission-controller/src/PermissionController.ts @@ -1982,7 +1982,6 @@ export class PermissionController< target: string, ): void { if (!isPlainObject(caveat)) { - // eslint-disable-next-line @typescript-eslint/no-throw-literal throw new InvalidCaveatError(caveat, origin, target); } diff --git a/packages/phishing-controller/src/PhishingDetector.test.ts b/packages/phishing-controller/src/PhishingDetector.test.ts index f76b809965..38e2f2b8b6 100644 --- a/packages/phishing-controller/src/PhishingDetector.test.ts +++ b/packages/phishing-controller/src/PhishingDetector.test.ts @@ -1110,7 +1110,7 @@ describe('PhishingDetector', () => { ]; // CID should be blocked - for await (const entry of expectedToBeBlocked) { + for (const entry of expectedToBeBlocked) { await withPhishingDetector( [ { diff --git a/packages/polling-controller/src/AbstractPollingController.ts b/packages/polling-controller/src/AbstractPollingController.ts index d52aab938a..f22455e4b1 100644 --- a/packages/polling-controller/src/AbstractPollingController.ts +++ b/packages/polling-controller/src/AbstractPollingController.ts @@ -82,7 +82,6 @@ export function AbstractPollingControllerBaseMixin< const callbacks = this.#callbacks.get(keyToDelete); if (callbacks) { for (const callback of callbacks) { - // eslint-disable-next-line n/callback-return callback(JSON.parse(keyToDelete)); } callbacks.clear(); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts index 63df3b5721..359520d829 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts @@ -29,7 +29,7 @@ type PerformMainNetworkSyncProps = { * Ensures that listeners do not fire during main sync (prevent double requests) */ // Exported to help testing -// eslint-disable-next-line import/no-mutable-exports +// eslint-disable-next-line import-x/no-mutable-exports export let isMainNetworkSyncInProgress = false; /** diff --git a/packages/selected-network-controller/tests/SelectedNetworkController.test.ts b/packages/selected-network-controller/tests/SelectedNetworkController.test.ts index fd975016ca..847f1b5898 100644 --- a/packages/selected-network-controller/tests/SelectedNetworkController.test.ts +++ b/packages/selected-network-controller/tests/SelectedNetworkController.test.ts @@ -44,7 +44,7 @@ function buildMessenger() { * @param options.getSubjectNames - Permissions controller list of domains with permissions * @returns The network controller restricted messenger. */ -export function buildSelectedNetworkControllerMessenger({ +function buildSelectedNetworkControllerMessenger({ messenger = new ControllerMessenger< SelectedNetworkControllerActions | AllowedActions, SelectedNetworkControllerEvents | AllowedEvents diff --git a/packages/signature-controller/src/SignatureController.ts b/packages/signature-controller/src/SignatureController.ts index 54b8f1e63e..239cd76386 100644 --- a/packages/signature-controller/src/SignatureController.ts +++ b/packages/signature-controller/src/SignatureController.ts @@ -30,7 +30,7 @@ import { import type { NetworkControllerGetNetworkClientByIdAction } from '@metamask/network-controller'; import type { Hex, Json } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { v1 as random } from 'uuid'; @@ -782,7 +782,6 @@ export class SignatureController extends BaseController< const originalStatus = metadata.status; - // eslint-disable-next-line n/callback-return callback(metadata); statusChanged = metadata.status !== originalStatus; @@ -802,7 +801,6 @@ export class SignatureController extends BaseController< #updateState(callback: (state: SignatureControllerState) => void) { return this.update((state) => { - // eslint-disable-next-line n/callback-return, n/no-callback-literal callback(state as unknown as SignatureControllerState); const unapprovedRequests = Object.values(state.signatureRequests).filter( diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index ff7c60f7ea..a92967f210 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -47,7 +47,7 @@ import type { Hex } from '@metamask/utils'; import { add0x, hexToNumber } from '@metamask/utils'; import { Mutex } from 'async-mutex'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import { EventEmitter } from 'events'; import { cloneDeep, mapValues, merge, pickBy, sortBy } from 'lodash'; import { v1 as random } from 'uuid'; @@ -3495,7 +3495,6 @@ export class TransactionController extends BaseController< const originalTransactionMeta = cloneDeep(transactionMeta); - // eslint-disable-next-line n/callback-return transactionMeta = callback(transactionMeta) ?? transactionMeta; if (skipValidation !== true) { diff --git a/packages/transaction-controller/src/helpers/GasFeePoller.ts b/packages/transaction-controller/src/helpers/GasFeePoller.ts index b4b87a94dc..82ad6090fe 100644 --- a/packages/transaction-controller/src/helpers/GasFeePoller.ts +++ b/packages/transaction-controller/src/helpers/GasFeePoller.ts @@ -7,7 +7,7 @@ import type { NetworkClientId, Provider } from '@metamask/network-controller'; import type { Hex } from '@metamask/utils'; import { createModuleLogger } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { projectLogger } from '../logger'; diff --git a/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts b/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts index 172f0d8c00..f8d97dd5c4 100644 --- a/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts +++ b/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts @@ -1,7 +1,7 @@ import type { AccountsController } from '@metamask/accounts-controller'; import type { Hex } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { incomingTransactionsLogger as log } from '../logger'; diff --git a/packages/transaction-controller/src/helpers/MethodDataHelper.ts b/packages/transaction-controller/src/helpers/MethodDataHelper.ts index 54dbdb6ab4..2542cb6717 100644 --- a/packages/transaction-controller/src/helpers/MethodDataHelper.ts +++ b/packages/transaction-controller/src/helpers/MethodDataHelper.ts @@ -3,7 +3,7 @@ import { createModuleLogger } from '@metamask/utils'; import { Mutex } from 'async-mutex'; import { MethodRegistry } from 'eth-method-registry'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { projectLogger } from '../logger'; diff --git a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts index 5520fe3a71..557a5d7c30 100644 --- a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts +++ b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts @@ -5,7 +5,7 @@ import type { NetworkClientId, } from '@metamask/network-controller'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { cloneDeep, merge } from 'lodash'; diff --git a/packages/user-operation-controller/src/UserOperationController.ts b/packages/user-operation-controller/src/UserOperationController.ts index 2c70bd80c6..eede19f089 100644 --- a/packages/user-operation-controller/src/UserOperationController.ts +++ b/packages/user-operation-controller/src/UserOperationController.ts @@ -26,7 +26,7 @@ import { } from '@metamask/transaction-controller'; import { add0x } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import type { Patch } from 'immer'; import { cloneDeep } from 'lodash'; diff --git a/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts b/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts index 54fa8ce602..551ac6736a 100644 --- a/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts +++ b/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts @@ -8,7 +8,7 @@ import type { import { BlockTrackerPollingControllerOnly } from '@metamask/polling-controller'; import { createModuleLogger, type Hex } from '@metamask/utils'; // This package purposefully relies on Node's EventEmitter module. -// eslint-disable-next-line import/no-nodejs-modules +// eslint-disable-next-line import-x/no-nodejs-modules import EventEmitter from 'events'; import { projectLogger } from '../logger'; diff --git a/scripts/run-eslint.ts b/scripts/run-eslint.ts new file mode 100644 index 0000000000..cabf068470 --- /dev/null +++ b/scripts/run-eslint.ts @@ -0,0 +1,289 @@ +import { ESLint } from 'eslint'; +import fs from 'fs'; +import path from 'path'; +import yargs from 'yargs'; + +const EXISTING_WARNINGS_FILE = path.resolve( + __dirname, + '../eslint-warning-thresholds.json', +); + +/** + * An object mapping rule IDs to their warning counts. + */ +type WarningCounts = Record; + +/** + * An object indicating the difference in warnings for a specific rule. + */ +type WarningComparison = { + /** The ID of the ESLint rule. */ + ruleId: string; + /** The previous count of warnings for the rule. */ + threshold: number; + /** The current count of warnings for the rule. */ + count: number; + /** The difference between the count and the threshold for the rule. */ + difference: number; +}; + +/** + * The warning severity of level of an ESLint rule. + */ +const WARNING = 1; + +// Run the script. +main().catch((error) => { + console.error(error); + process.exitCode = 1; +}); + +/** + * The entrypoint to this script. + */ +async function main() { + const { cache, fix, quiet } = parseCommandLineArguments(); + + const eslint = new ESLint({ cache, fix }); + const results = await runESLint(eslint, { fix, quiet }); + const hasErrors = results.some((result) => result.errorCount > 0); + + if (!quiet && !hasErrors) { + evaluateWarnings(results); + } +} + +/** + * Uses `yargs` to parse the arguments given to the script. + * + * @returns The parsed arguments. + */ +function parseCommandLineArguments() { + return yargs(process.argv.slice(2)) + .option('cache', { + type: 'boolean', + description: 'Cache results to speed up future runs', + default: false, + }) + .option('fix', { + type: 'boolean', + description: 'Automatically fix problems', + default: false, + }) + .option('quiet', { + type: 'boolean', + description: + 'Only report errors, disabling the warnings quality gate in the process', + default: false, + }) + .help().argv; +} + +/** + * Runs ESLint on the project files. + * + * @param eslint - The ESLint instance. + * @param options - The options for running ESLint. + * @param options.quiet - Whether to only report errors (true) or not (false). + * @param options.fix - Whether to automatically fix problems (true) or not + * (false). + * @returns A promise that resolves to the lint results. + */ +async function runESLint( + eslint: ESLint, + options: { quiet: boolean; fix: boolean }, +): Promise { + let results = await eslint.lintFiles(['.']); + + const formatter = await eslint.loadFormatter('stylish'); + const resultText = formatter.format(results); + console.log(resultText); + + if (options.quiet) { + results = ESLint.getErrorResults(results); + } + + if (options.fix) { + await ESLint.outputFixes(results); + } + + return results; +} + +/** + * This function represents the ESLint warnings quality gate, which will cause + * linting to pass or fail depending on how many new warnings have been + * produced. + * + * - If we have no record of warnings from a previous run, then we simply + * capture the new warnings in a file and continue. + * - If we have a record of warnings from a previous run and there are any + * changes to the number of warnings overall, then we list which ESLint rules + * had increases and decreases. If are were more warnings overall then we fail, + * otherwise we pass. + * + * @param results - The results of running ESLint. + */ +function evaluateWarnings(results: ESLint.LintResult[]) { + const warningThresholds = loadWarningThresholds(); + const warningCounts = getWarningCounts(results); + + if (Object.keys(warningThresholds).length === 0) { + console.log( + 'The following ESLint warnings were produced and will be captured as thresholds for future runs:\n', + ); + for (const [ruleId, count] of Object.entries(warningCounts)) { + console.log(`- ${ruleId}: ${count}`); + } + saveWarningThresholds(warningCounts); + } else { + const comparisons = compareWarnings(warningThresholds, warningCounts); + + const changes = comparisons.filter( + (comparison) => comparison.difference !== 0, + ); + const regressions = comparisons.filter( + (comparison) => comparison.difference > 0, + ); + + if (changes.length > 0) { + if (regressions.length > 0) { + console.log( + '🛑 New ESLint warnings have been introduced and need to be resolved for linting to pass:\n', + ); + process.exitCode = 1; + } else { + console.log( + 'The overall number of ESLint warnings have decreased, good work! ❤️ \n', + ); + } + for (const { ruleId, threshold, count, difference } of changes) { + console.log( + `- ${ruleId}: ${threshold} -> ${count} (${difference > 0 ? '+' : ''}${difference})`, + ); + } + // We are still seeing differences on CI when it comes to linting results. + // Only write the thresholds file if there are regressions in that case + // to prevent the "working directory clean" step in CI from failing. + // Also, it's okay to use `process.env` as this is a script. + // eslint-disable-next-line n/no-process-env + if (regressions.length > 0 || !process.env.CI) { + saveWarningThresholds(warningCounts); + } + } + } +} + +/** + * Loads previous warning counts from a file. + * + * @returns An object mapping rule IDs to their previous warning counts. + */ +function loadWarningThresholds(): WarningCounts { + if (fs.existsSync(EXISTING_WARNINGS_FILE)) { + const data = fs.readFileSync(EXISTING_WARNINGS_FILE, 'utf-8'); + return JSON.parse(data); + } + return {}; +} + +/** + * Saves current warning counts to a file so they can be used for a future run. + * + * @param warningCounts - An object mapping rule IDs to their current warning + * counts. + */ +function saveWarningThresholds(warningCounts: WarningCounts): void { + fs.writeFileSync( + EXISTING_WARNINGS_FILE, + `${JSON.stringify(warningCounts, null, 2)}\n`, + 'utf-8', + ); +} + +/** + * Given a list of results from an the ESLint run, counts the number of warnings + * produced per rule. + * + * @param results - The ESLint results. + * @returns An object mapping rule IDs to their warning counts, sorted by rule + * ID. + */ +function getWarningCounts(results: ESLint.LintResult[]): WarningCounts { + const warningCounts = results.reduce((acc, result) => { + for (const message of result.messages) { + if (message.severity === WARNING && message.ruleId) { + acc[message.ruleId] = (acc[message.ruleId] ?? 0) + 1; + } + } + return acc; + }, {} as WarningCounts); + + return Object.keys(warningCounts) + .sort(sortRules) + .reduce((sortedWarningCounts, key) => { + return { ...sortedWarningCounts, [key]: warningCounts[key] }; + }, {} as WarningCounts); +} + +/** + * Compares previous and current warning counts. + * + * @param warningThresholds - An object mapping rule IDs to the warning + * thresholds established in a previous run. + * @param warningCounts - An object mapping rule IDs to the current warning + * counts. + * @returns An array of objects indicating comparisons in warnings. + */ +function compareWarnings( + warningThresholds: WarningCounts, + warningCounts: WarningCounts, +): WarningComparison[] { + const ruleIds = Array.from( + new Set([...Object.keys(warningThresholds), ...Object.keys(warningCounts)]), + ); + return ruleIds + .map((ruleId) => { + const threshold = warningThresholds[ruleId] ?? 0; + const count = warningCounts[ruleId] ?? 0; + const difference = count - threshold; + return { ruleId, threshold, count, difference }; + }) + .sort((a, b) => sortRules(a.ruleId, b.ruleId)); +} + +/** + * Sorts rule IDs, ensuring that rules with namespaces appear before rules + * without. + * + * @param ruleIdA - The first rule ID. + * @param ruleIdB - The second rule ID. + * @returns A negative number if the first rule ID should come before the + * second, a positive number if the first should come _after_ the second, or 0 + * if they should stay where they are. + * @example + * ``` typescript + * sortRules( + * '@typescript-eslint/naming-convention', + * '@typescript-eslint/explicit-function-return-type' + * ) //=> 1 (sort A after B) + * sortRules( + * 'explicit-function-return-type', + * '@typescript-eslint/naming-convention' + * ) //=> 1 (sort A after B) + */ +function sortRules(ruleIdA: string, ruleIdB: string): number { + const [namespaceA, ruleA] = ruleIdA.includes('/') + ? ruleIdA.split('/') + : ['', ruleIdA]; + const [namespaceB, ruleB] = ruleIdB.includes('/') + ? ruleIdB.split('/') + : ['', ruleIdB]; + if (namespaceA && !namespaceB) { + return -1; + } + if (!namespaceA && namespaceB) { + return 1; + } + return namespaceA.localeCompare(namespaceB) || ruleA.localeCompare(ruleB); +} diff --git a/yarn.config.cjs b/yarn.config.cjs index b802565a6a..aa8f334e17 100644 --- a/yarn.config.cjs +++ b/yarn.config.cjs @@ -1,4 +1,9 @@ -// @ts-check +/* @ts-check */ +// The ESLint JSDoc plugin usually disables this rule for TypeScript files, +// but for JavaScript files we are typechecking, we need to disable it manually. +// See: +/* eslint-disable jsdoc/no-undefined-types */ + // This file is used to define, among other configuration, rules that Yarn will // execute when you run `yarn constraints`. These rules primarily check the // manifests of each package in the monorepo to ensure they follow a standard diff --git a/yarn.lock b/yarn.lock index 3401672a50..f88317fe24 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2555,6 +2555,7 @@ __metadata: eth-ens-namehash: "npm:^2.0.8" fast-deep-equal: "npm:^3.1.3" jest: "npm:^27.5.1" + jest-environment-jsdom: "npm:^27.5.1" nock: "npm:^13.3.1" sinon: "npm:^9.2.4" ts-jest: "npm:^27.1.4" @@ -5784,11 +5785,11 @@ __metadata: linkType: hard "acorn-walk@npm:^8.1.1": - version: 8.3.3 - resolution: "acorn-walk@npm:8.3.3" + version: 8.3.4 + resolution: "acorn-walk@npm:8.3.4" dependencies: acorn: "npm:^8.11.0" - checksum: 10/59701dcb7070679622ba8e9c7f37577b4935565747ca0fd7c1c3ad30b3f1b1b008276282664e323b5495eb49f77fa12d3816fd06dc68e18f90fbebe759f71450 + checksum: 10/871386764e1451c637bb8ab9f76f4995d408057e9909be6fb5ad68537ae3375d85e6a6f170b98989f44ab3ff6c74ad120bc2779a3d577606e7a0cd2b4efcaf77 languageName: node linkType: hard @@ -10933,9 +10934,9 @@ __metadata: linkType: hard "nwsapi@npm:^2.2.0": - version: 2.2.12 - resolution: "nwsapi@npm:2.2.12" - checksum: 10/172119e9ef492467ebfb337f9b5fd12a94d2b519377cde3f6ec2f74a86f6d5c00ef3873539bed7142f908ffca4e35383179be2319d04a563071d146bfa3f1673 + version: 2.2.16 + resolution: "nwsapi@npm:2.2.16" + checksum: 10/1e5e086cdd4ca4a45f414d37f49bf0ca81d84ed31c6871ac68f531917d2910845db61f77c6d844430dc90fda202d43fce9603024e74038675de95229eb834dba languageName: node linkType: hard