fix: Migrate AbstractMessageManager from BaseControllerV1 to BaseControllerV2

packages/message-manager/CHANGELOG.md

@@ -9,6 +9,10 @@ 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))
+- **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.
 - Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079))
 
 ## [11.0.3]

packages/message-manager/src/AbstractMessageManager.test.ts

@@ -62,7 +62,7 @@ class AbstractTestManager extends AbstractMessageManager<
   }
 }
 
-const mockMessenger = {
+const MOCK_MESSENGER = {
   clearEventSubscriptions: jest.fn(),
   publish: jest.fn(),
   registerActionHandler: jest.fn(),
@@ -75,9 +75,9 @@ const mockMessenger = {
   string
 >;
 
-const mockInitialOptions = {
+const MOCK_INITIAL_OPTIONS = {
   additionalFinishStatuses: undefined,
-  messenger: mockMessenger,
+  messenger: MOCK_MESSENGER,
   name: 'AbstractMessageManager' as const,
   securityProviderRequest: undefined,
 };
@@ -104,15 +104,15 @@ const mockMessageParams = { from, test: testData };
 
 describe('AbstractTestManager', () => {
   it('should set default state', () => {
-    const controller = new AbstractTestManager(mockInitialOptions);
+    const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
     expect(controller.state).toStrictEqual({
       unapprovedMessages: {},
       unapprovedMessagesCount: 0,
     });
   });
 
   it('should add a valid message', async () => {
-    const controller = new AbstractTestManager(mockInitialOptions);
+    const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
     await controller.addMessage({
       id: messageId,
       messageParams: {
@@ -136,7 +136,7 @@ describe('AbstractTestManager', () => {
   });
 
   it('should get all messages', async () => {
-    const controller = new AbstractTestManager(mockInitialOptions);
+    const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
     const message = {
       id: messageId,
       messageParams: {
@@ -170,7 +170,7 @@ describe('AbstractTestManager', () => {
       .fn()
       .mockResolvedValue(securityProviderResponseMock);
     const controller = new AbstractTestManager({
-      ...mockInitialOptions,
+      ...MOCK_INITIAL_OPTIONS,
       securityProviderRequest: securityProviderRequestMock,
     });
     await controller.addMessage({
@@ -200,7 +200,7 @@ describe('AbstractTestManager', () => {
   });
 
   it('should reject a message', async () => {
-    const controller = new AbstractTestManager(mockInitialOptions);
+    const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
     await controller.addMessage({
       id: messageId,
       messageParams: {
@@ -220,7 +220,7 @@ describe('AbstractTestManager', () => {
   });
 
   it('should sign a message', async () => {
-    const controller = new AbstractTestManager(mockInitialOptions);
+    const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
     await controller.addMessage({
       id: messageId,
       messageParams: {
@@ -242,7 +242,7 @@ describe('AbstractTestManager', () => {
 
   it('sets message to one of the allowed statuses', async () => {
     const controller = new AbstractTestManager({
-      ...mockInitialOptions,
+      ...MOCK_INITIAL_OPTIONS,
       additionalFinishStatuses: ['test-status'],
     });
     await controller.addMessage({
@@ -265,7 +265,7 @@ describe('AbstractTestManager', () => {
 
   it('should set a status to inProgress', async () => {
     const controller = new AbstractTestManager({
-      ...mockInitialOptions,
+      ...MOCK_INITIAL_OPTIONS,
       additionalFinishStatuses: ['test-status'],
     });
     await controller.addMessage({
@@ -301,7 +301,7 @@ describe('AbstractTestManager', () => {
       time: 123,
       type: 'eth_signTypedData',
     };
-    const controller = new AbstractTestManager(mockInitialOptions);
+    const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
     await controller.addMessage(firstMessage);
     await controller.addMessage(secondMessage);
     expect(controller.getUnapprovedMessagesCount()).toBe(2);
@@ -312,7 +312,7 @@ describe('AbstractTestManager', () => {
   });
 
   it('should approve message', async () => {
-    const controller = new AbstractTestManager(mockInitialOptions);
+    const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
     const firstMessage = { from: '0xfoO', test: testData };
     await controller.addMessage({
       id: messageId,
@@ -335,7 +335,7 @@ describe('AbstractTestManager', () => {
 
   describe('addRequestToMessageParams', () => {
     it('adds original request id and origin to messageParams', () => {
-      const controller = new AbstractTestManager(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
 
       const result = controller.addRequestToMessageParams(
         mockMessageParams,
@@ -352,7 +352,7 @@ describe('AbstractTestManager', () => {
 
   describe('createUnapprovedMessage', () => {
     it('creates a Message object with an unapproved status', () => {
-      const controller = new AbstractTestManager(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
 
       const result = controller.createUnapprovedMessage(
         mockMessageParams,
@@ -377,7 +377,7 @@ describe('AbstractTestManager', () => {
         emit: jest.fn(),
       }));
 
-      const controller = new AbstractTestManager(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
       await controller.addMessage({
         id: messageId,
         messageParams: { ...mockMessageParams },
@@ -395,7 +395,7 @@ describe('AbstractTestManager', () => {
     });
 
     it('throws an error if the message is not found', async () => {
-      const controller = new AbstractTestManager(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
 
       expect(() => controller.setMessageStatus(messageId, 'newstatus')).toThrow(
         'AbstractMessageManager: Message not found for id: 1.',
@@ -409,7 +409,7 @@ describe('AbstractTestManager', () => {
         emit: jest.fn(),
       }));
 
-      const controller = new AbstractTestManager(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
       await controller.addMessage({
         id: messageId,
         messageParams: { ...mockMessageParams },
@@ -423,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(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
       await controller.addMessage({
         id: messageId,
         messageParams: { ...mockMessageParams },
@@ -448,7 +447,7 @@ describe('AbstractTestManager', () => {
     });
 
     it('should throw an error if message is not found', () => {
-      const controller = new AbstractTestManager(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
 
       expect(() => controller.setMetadata(messageId, { foo: 'bar' })).toThrow(
         'AbstractMessageManager: Message not found for id: 1.',
@@ -458,7 +457,7 @@ describe('AbstractTestManager', () => {
 
   describe('waitForFinishStatus', () => {
     it('signs the message when status is "signed"', async () => {
-      const controller = new AbstractTestManager(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
       const promise = controller.waitForFinishStatus(
         {
           from: fromMock,
@@ -468,7 +467,7 @@ describe('AbstractTestManager', () => {
       );
 
       setTimeout(() => {
-        controller.hub.emit(`${messageIdMock}:finished`, {
+        controller.internalEvents.emit(`${messageIdMock}:finished`, {
           status: 'signed',
           rawSig: rawSigMock,
         });
@@ -478,7 +477,7 @@ describe('AbstractTestManager', () => {
     });
 
     it('rejects with an error when status is "rejected"', async () => {
-      const controller = new AbstractTestManager(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
       const promise = controller.waitForFinishStatus(
         {
           from: fromMock,
@@ -488,7 +487,7 @@ describe('AbstractTestManager', () => {
       );
 
       setTimeout(() => {
-        controller.hub.emit(`${messageIdMock}:finished`, {
+        controller.internalEvents.emit(`${messageIdMock}:finished`, {
           status: 'rejected',
         });
       }, 100);
@@ -499,7 +498,7 @@ describe('AbstractTestManager', () => {
     });
 
     it('rejects with an error when finishes with unknown status', async () => {
-      const controller = new AbstractTestManager(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
       const promise = controller.waitForFinishStatus(
         {
           from: fromMock,
@@ -509,7 +508,7 @@ describe('AbstractTestManager', () => {
       );
 
       setTimeout(() => {
-        controller.hub.emit(`${messageIdMock}:finished`, {
+        controller.internalEvents.emit(`${messageIdMock}:finished`, {
           status: 'unknown',
         });
       }, 100);
@@ -524,7 +523,7 @@ describe('AbstractTestManager', () => {
     });
 
     it('rejects with an error when finishes with errored status', async () => {
-      const controller = new AbstractTestManager(mockInitialOptions);
+      const controller = new AbstractTestManager(MOCK_INITIAL_OPTIONS);
       const promise = controller.waitForFinishStatus(
         {
           from: fromMock,
@@ -534,7 +533,7 @@ describe('AbstractTestManager', () => {
       );
 
       setTimeout(() => {
-        controller.hub.emit(`${messageIdMock}:finished`, {
+        controller.internalEvents.emit(`${messageIdMock}:finished`, {
           status: 'errored',
           error: 'error message',
         });
@@ -548,7 +547,21 @@ describe('AbstractTestManager', () => {
 
   describe('clearUnapprovedMessages', () => {
     it('clears the unapproved messages', () => {
-      const controller = new AbstractTestManager(mockInitialOptions);
+      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);
     });

packages/message-manager/src/AbstractMessageManager.ts

@@ -98,6 +98,11 @@ export type MessageManagerState<Message extends AbstractMessage> = {
   unapprovedMessagesCount: number;
 };
 
+export type UpdateBadgeEvent = {
+  type: `${string}:updateBadge`;
+  payload: [unusedPayload: string];
+};
+
 /**
  * A function for verifying a message, whether it is malicious or not
  */
@@ -106,7 +111,16 @@ export type SecurityProviderRequest = (
   messageType: string,
 ) => Promise<Json>;
 
-type AbstractMessageManagerOptions<
+/**
+ * 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,
@@ -115,7 +129,7 @@ type AbstractMessageManagerOptions<
   messenger: RestrictedControllerMessenger<
     string,
     Action,
-    Event,
+    Event | UpdateBadgeEvent,
     string,
     string
   >;
@@ -136,26 +150,22 @@ export abstract class AbstractMessageManager<
 > extends BaseController<
   string,
   MessageManagerState<Message>,
-  RestrictedControllerMessenger<string, Action, Event, string, string>
+  RestrictedControllerMessenger<
+    string,
+    Action,
+    Event | UpdateBadgeEvent,
+    string,
+    string
+  >
 > {
   protected messages: Message[];
 
   private readonly securityProviderRequest: SecurityProviderRequest | undefined;
 
   private readonly additionalFinishStatuses: string[];
 
-  hub: EventEmitter = new EventEmitter();
+  internalEvents = new EventEmitter();
 
-  /**
-   * Creates an AbstractMessageManager instance.
-   *
-   * @param options - Options for the AbstractMessageManager.
-   * @param options.additionalFinishStatuses - Optional list of statuses that are accepted to emit a finished event.
-   * @param options.messenger - Controller messaging system.
-   * @param options.name - The name of the manager.
-   * @param options.securityProviderRequest - A function for verifying a message, whether it is malicious or not.
-   * @param options.state - Initial state to set on this controller.
-   */
   constructor({
     additionalFinishStatuses,
     messenger,
@@ -235,7 +245,8 @@ export abstract class AbstractMessageManager<
       state.unapprovedMessagesCount = this.getUnapprovedMessagesCount();
     });
     if (emitUpdateBadge) {
-      this.hub.emit('updateBadge');
+      // Empty payload is used to satisfy event constraint for BaseControllerV2
+      this.messagingSystem.publish(`${this.name as string}:updateBadge`, '');
     }
   }
 
@@ -257,14 +268,17 @@ export abstract class AbstractMessageManager<
       status,
     };
     this.updateMessage(updatedMessage);
-    this.hub.emit(`${messageId}:${status}`, updatedMessage);
+    this.internalEvents.emit(`${messageId}:${status}`, updatedMessage);
     if (
       status === 'rejected' ||
       status === 'signed' ||
       status === 'errored' ||
       this.additionalFinishStatuses.includes(status)
     ) {
-      this.hub.emit(`${messageId}:finished`, updatedMessage);
+      this.internalEvents.emit(
+        `${messageId as string}:finished`,
+        updatedMessage,
+      );
     }
   }
 
@@ -516,7 +530,7 @@ export abstract class AbstractMessageManager<
   ): Promise<string> {
     const { metamaskId: messageId, ...messageParams } = messageParamsWithId;
     return new Promise((resolve, reject) => {
-      this.hub.once(
+      this.internalEvents.once(
         `${messageId as string}:finished`,
         (data: AbstractMessage) => {
           switch (data.status) {