Use messenger actions instead of KeyringController as constructor p…

…aram (#1593)

packages/signature-controller/package.json

@@ -40,6 +40,7 @@
   },
   "devDependencies": {
     "@metamask/auto-changelog": "^3.1.0",
+    "@metamask/keyring-controller": "^7.4.0",
     "@types/jest": "^27.4.1",
     "deepmerge": "^4.2.2",
     "jest": "^27.5.1",

packages/signature-controller/src/SignatureController.test.ts

@@ -117,12 +117,6 @@ const createMessageManagerMock = <T>(prototype?: any): jest.Mocked<T> => {
   }) as jest.Mocked<T>;
 };
 
-const createKeyringControllerMock = () => ({
-  signMessage: jest.fn(),
-  signPersonalMessage: jest.fn(),
-  signTypedMessage: jest.fn(),
-});
-
 describe('SignatureController', () => {
   let signatureController: SignatureController;
 
@@ -148,14 +142,28 @@ describe('SignatureController', () => {
     error: jest.fn(),
   };
   const messengerMock = createMessengerMock();
-  const keyringControllerMock = createKeyringControllerMock();
   const getAllStateMock = jest.fn();
   const securityProviderRequestMock = jest.fn();
   const isEthSignEnabledMock = jest.fn();
   const getCurrentChainIdMock = jest.fn();
   const keyringErrorMessageMock = 'Keyring Error';
   const keyringErrorMock = new Error(keyringErrorMessageMock);
 
+  const mockMessengerAction = (
+    action: string,
+    callback: (actionName: string, ...args: any[]) => any,
+  ) => {
+    messengerMock.call.mockImplementation((actionName, ...rest) => {
+      if (actionName === action) {
+        return callback(actionName, ...rest);
+      }
+
+      return Promise.resolve({
+        resultCallbacks: resultCallbacksMock,
+      });
+    });
+  };
+
   beforeEach(() => {
     jest.resetAllMocks();
     jest.spyOn(console, 'info').mockImplementation(() => undefined);
@@ -176,7 +184,6 @@ describe('SignatureController', () => {
 
     signatureController = new SignatureController({
       messenger: messengerMock,
-      keyringController: keyringControllerMock,
       getAllState: getAllStateMock,
       securityProviderRequest: securityProviderRequestMock,
       isEthSignEnabled: isEthSignEnabledMock,
@@ -371,8 +378,9 @@ describe('SignatureController', () => {
         undefined,
       );
 
-      expect(messengerMock.call).toHaveBeenCalledTimes(1);
-      expect(messengerMock.call).toHaveBeenCalledWith(
+      expect(messengerMock.call).toHaveBeenCalledTimes(2);
+      expect(messengerMock.call).toHaveBeenNthCalledWith(
+        1,
         'ApprovalController:addRequest',
         {
           id: messageIdMock,
@@ -386,9 +394,9 @@ describe('SignatureController', () => {
     });
 
     it('throws if cannot get signature', async () => {
-      (keyringControllerMock as any).signMessage.mockRejectedValueOnce(
-        keyringErrorMock,
-      );
+      mockMessengerAction('KeyringController:signMessage', async () => {
+        throw keyringErrorMock;
+      });
       const listenerMock = jest.fn();
       signatureController.hub.on(`${messageIdMock}:signError`, listenerMock);
 
@@ -404,6 +412,7 @@ describe('SignatureController', () => {
       expect(listenerMock).toHaveBeenCalledWith({
         error,
       });
+      expect(messengerMock.call).toHaveBeenCalledTimes(2);
       expect(error.message).toBe(keyringErrorMessageMock);
       expect(messageManagerMock.rejectMessage).toHaveBeenCalledTimes(1);
       expect(messageManagerMock.rejectMessage).toHaveBeenCalledWith(
@@ -443,7 +452,7 @@ describe('SignatureController', () => {
         undefined,
       );
 
-      expect(messengerMock.call).toHaveBeenCalledTimes(1);
+      expect(messengerMock.call).toHaveBeenCalledTimes(2);
       expect(messengerMock.call).toHaveBeenCalledWith(
         'ApprovalController:addRequest',
         {
@@ -455,6 +464,11 @@ describe('SignatureController', () => {
         },
         true,
       );
+      expect(messengerMock.call).toHaveBeenNthCalledWith(
+        2,
+        'KeyringController:signPersonalMessage',
+        messageParamsWithoutIdMock,
+      );
     });
 
     it('throws if approval rejected', async () => {
@@ -471,17 +485,25 @@ describe('SignatureController', () => {
     });
 
     it('throws if cannot get signature', async () => {
-      (keyringControllerMock as any).signPersonalMessage.mockRejectedValueOnce(
-        keyringErrorMock,
-      );
+      mockMessengerAction('KeyringController:signPersonalMessage', async () => {
+        throw keyringErrorMock;
+      });
+
       const error: any = await getError(
         async () =>
           await signatureController.newUnsignedPersonalMessage(
             messageParamsMock,
             requestMock,
           ),
       );
+
+      expect(messengerMock.call).toHaveBeenCalledTimes(2);
       expect(error.message).toBe(keyringErrorMessageMock);
+      expect(messengerMock.call).toHaveBeenNthCalledWith(
+        2,
+        'KeyringController:signPersonalMessage',
+        messageParamsWithoutIdMock,
+      );
       expect(personalMessageManagerMock.rejectMessage).toHaveBeenCalledTimes(1);
       expect(personalMessageManagerMock.rejectMessage).toHaveBeenCalledWith(
         messageIdMock,
@@ -522,8 +544,9 @@ describe('SignatureController', () => {
         versionMock,
       );
 
-      expect(messengerMock.call).toHaveBeenCalledTimes(1);
-      expect(messengerMock.call).toHaveBeenCalledWith(
+      expect(messengerMock.call).toHaveBeenCalledTimes(2);
+      expect(messengerMock.call).toHaveBeenNthCalledWith(
+        1,
         'ApprovalController:addRequest',
         {
           id: messageIdMock,
@@ -534,6 +557,12 @@ describe('SignatureController', () => {
         },
         true,
       );
+      expect(messengerMock.call).toHaveBeenNthCalledWith(
+        2,
+        'KeyringController:signTypedMessage',
+        messageParamsWithoutIdMock,
+        versionMock,
+      );
     });
 
     it('does not set as signed, messages with deferSetAsSigned', async () => {
@@ -575,10 +604,11 @@ describe('SignatureController', () => {
         { parseJsonData: true },
       );
 
-      expect(keyringControllerMock.signTypedMessage).toHaveBeenCalledTimes(1);
-      expect(keyringControllerMock.signTypedMessage).toHaveBeenCalledWith(
+      expect(messengerMock.call).toHaveBeenNthCalledWith(
+        2,
+        'KeyringController:signTypedMessage',
         { ...messageParamsMock2, data: jsonData, deferSetAsSigned: false },
-        { version: 'V2' },
+        'V2',
       );
     });
 
@@ -598,9 +628,9 @@ describe('SignatureController', () => {
     });
 
     it('throws if cannot get signature', async () => {
-      keyringControllerMock.signTypedMessage.mockRejectedValueOnce(
-        keyringErrorMock,
-      );
+      mockMessengerAction('KeyringController:signTypedMessage', async () => {
+        throw keyringErrorMock;
+      });
       typedMessageManagerMock.addUnapprovedMessage.mockResolvedValue(
         messageIdMock,
       );

packages/signature-controller/src/SignatureController.ts

@@ -6,6 +6,12 @@ import type {
 import type { RestrictedControllerMessenger } from '@metamask/base-controller';
 import { BaseControllerV2 } from '@metamask/base-controller';
 import { ApprovalType, ORIGIN_METAMASK } from '@metamask/controller-utils';
+import type {
+  KeyringControllerSignMessageAction,
+  KeyringControllerSignPersonalMessageAction,
+  KeyringControllerSignTypedMessageAction,
+  SignTypedDataVersion,
+} from '@metamask/keyring-controller';
 import type {
   MessageParams,
   MessageParamsMetamask,
@@ -72,7 +78,11 @@ type SignatureControllerState = {
   unapprovedTypedMessagesCount: number;
 };
 
-type AllowedActions = AddApprovalRequest;
+type AllowedActions =
+  | AddApprovalRequest
+  | KeyringControllerSignMessageAction
+  | KeyringControllerSignPersonalMessageAction
+  | KeyringControllerSignTypedMessageAction;
 
 type TypedMessageSigningOptions = {
   parseJsonData: boolean;
@@ -100,20 +110,8 @@ export type SignatureControllerMessenger = RestrictedControllerMessenger<
   never
 >;
 
-export interface KeyringController {
-  signMessage: (messsageParams: MessageParams) => Promise<string>;
-  signPersonalMessage: (
-    messsageParams: PersonalMessageParams,
-  ) => Promise<string>;
-  signTypedMessage: (
-    messsageParams: TypedMessageParams,
-    options: { version: string | undefined },
-  ) => Promise<string>;
-}
-
 export type SignatureControllerOptions = {
   messenger: SignatureControllerMessenger;
-  keyringController: KeyringController;
   isEthSignEnabled: () => boolean;
   getAllState: () => unknown;
   securityProviderRequest?: (
@@ -133,8 +131,6 @@ export class SignatureController extends BaseControllerV2<
 > {
   hub: EventEmitter;
 
-  #keyringController: KeyringController;
-
   #isEthSignEnabled: () => boolean;
 
   #getAllState: () => any;
@@ -150,15 +146,13 @@ export class SignatureController extends BaseControllerV2<
    *
    * @param options - The controller options.
    * @param options.messenger - The restricted controller messenger for the sign controller.
-   * @param options.keyringController - An instance of a keyring controller used to perform the signing operations.
    * @param options.isEthSignEnabled - Callback to return true if eth_sign is enabled.
    * @param options.getAllState - Callback to retrieve all user state.
    * @param options.securityProviderRequest - A function for verifying a message, whether it is malicious or not.
    * @param options.getCurrentChainId - A function for retrieving the current chainId.
    */
   constructor({
     messenger,
-    keyringController,
     isEthSignEnabled,
     getAllState,
     securityProviderRequest,
@@ -171,7 +165,6 @@ export class SignatureController extends BaseControllerV2<
       state: getDefaultState(),
     });
 
-    this.#keyringController = keyringController;
     this.#isEthSignEnabled = isEthSignEnabled;
     this.#getAllState = getAllState;
 
@@ -525,7 +518,10 @@ export class SignatureController extends BaseControllerV2<
       ApprovalType.EthSign,
       msgParams,
       async (cleanMsgParams) =>
-        await this.#keyringController.signMessage(cleanMsgParams),
+        await this.messagingSystem.call(
+          'KeyringController:signMessage',
+          cleanMsgParams,
+        ),
     );
   }
 
@@ -542,7 +538,10 @@ export class SignatureController extends BaseControllerV2<
       ApprovalType.PersonalSign,
       msgParams,
       async (cleanMsgParams) =>
-        await this.#keyringController.signPersonalMessage(cleanMsgParams),
+        await this.messagingSystem.call(
+          'KeyringController:signPersonalMessage',
+          cleanMsgParams,
+        ),
     );
   }
 
@@ -570,11 +569,10 @@ export class SignatureController extends BaseControllerV2<
           ? this.#removeJsonData(cleanMsgParams, version as string)
           : cleanMsgParams;
 
-        return await this.#keyringController.signTypedMessage(
+        return await this.messagingSystem.call(
+          'KeyringController:signTypedMessage',
           finalMessageParams,
-          {
-            version,
-          },
+          version as SignTypedDataVersion,
         );
       },
     );

packages/signature-controller/tsconfig.build.json

@@ -17,6 +17,9 @@
     },
     {
       "path": "../message-manager/tsconfig.build.json"
+    },
+    {
+      "path": "../keyring-controller/tsconfig.build.json"
     }
   ],
   "include": ["../../types", "./src"]

packages/signature-controller/tsconfig.json

@@ -15,6 +15,9 @@
     },
     {
       "path": "../message-manager"
+    },
+    {
+      "path": "../keyring-controller"
     }
   ],
   "include": ["../../types", "./src"]

yarn.lock

@@ -1790,7 +1790,7 @@ __metadata:
   languageName: unknown
   linkType: soft
 
-"@metamask/keyring-controller@workspace:packages/keyring-controller":
+"@metamask/keyring-controller@^7.4.0, @metamask/keyring-controller@workspace:packages/keyring-controller":
   version: 0.0.0-use.local
   resolution: "@metamask/keyring-controller@workspace:packages/keyring-controller"
   dependencies:
@@ -2114,6 +2114,7 @@ __metadata:
     "@metamask/auto-changelog": ^3.1.0
     "@metamask/base-controller": ^3.2.1
     "@metamask/controller-utils": ^4.3.2
+    "@metamask/keyring-controller": ^7.4.0
     "@metamask/message-manager": ^7.3.1
     "@metamask/utils": ^6.2.0
     "@types/jest": ^27.4.1