From fd0773500ea5d60b1833136620010e4b4cb3e449 Mon Sep 17 00:00:00 2001
From: Frederik Bolding <frederik.bolding@gmail.com>
Date: Fri, 9 Feb 2024 12:35:57 +0100
Subject: [PATCH] Simplify `wallet_requestPermissions` implementation (#3911)

## Explanation

The type for the RPC method hook `requestPermissionsForOrigin` did not
previously match the implementation
`PermissionController.requestPermission`. This PR rectifies that and
thus simplifies the implementation of the RPC method by removing
effectively dead code.

Previously we were passing in a string as the optional options bag.
Since this options bag has sane defaults, those were always used:
https://github.com/MetaMask/core/blob/main/packages/permission-controller/src/PermissionController.ts#L1884

With this change, we just always use the options bag defaults and can
remove complexity from the RPC implementation.

## Changelog

I'm not sure this warrants a changelog entry tbh.


## 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
---
 .../rpc-methods/requestPermissions.test.ts    | 42 +------------------
 .../src/rpc-methods/requestPermissions.ts     | 17 +-------
 2 files changed, 3 insertions(+), 56 deletions(-)

diff --git a/packages/permission-controller/src/rpc-methods/requestPermissions.test.ts b/packages/permission-controller/src/rpc-methods/requestPermissions.test.ts
index 279a6502eaf..e935e021a74 100644
--- a/packages/permission-controller/src/rpc-methods/requestPermissions.test.ts
+++ b/packages/permission-controller/src/rpc-methods/requestPermissions.test.ts
@@ -41,7 +41,7 @@ describe('requestPermissions RPC method', () => {
 
     expect(response.result).toStrictEqual(['a', 'b', 'c']);
     expect(mockRequestPermissionsForOrigin).toHaveBeenCalledTimes(1);
-    expect(mockRequestPermissionsForOrigin).toHaveBeenCalledWith({}, '1');
+    expect(mockRequestPermissionsForOrigin).toHaveBeenCalledWith({});
   });
 
   it('returns an error if requestPermissionsForOrigin rejects', async () => {
@@ -90,45 +90,7 @@ describe('requestPermissions RPC method', () => {
       serializeError(expectedError, { shouldIncludeStack: false }),
     );
     expect(mockRequestPermissionsForOrigin).toHaveBeenCalledTimes(1);
-    expect(mockRequestPermissionsForOrigin).toHaveBeenCalledWith({}, '1');
-  });
-
-  it('returns an error if the request has an invalid id', async () => {
-    const { implementation } = requestPermissionsHandler;
-    const mockRequestPermissionsForOrigin = jest.fn();
-
-    const engine = new JsonRpcEngine();
-    engine.push((req, res, next, end) =>
-      // TODO: Replace `any` with type
-      // eslint-disable-next-line @typescript-eslint/no-explicit-any
-      implementation(req as any, res as any, next, end, {
-        requestPermissionsForOrigin: mockRequestPermissionsForOrigin,
-      }),
-    );
-
-    for (const invalidId of ['', null, {}]) {
-      const req = {
-        jsonrpc: '2.0',
-        id: invalidId,
-        method: 'arbitraryName',
-        params: [], // doesn't matter
-      };
-
-      const expectedError = rpcErrors
-        .invalidRequest({
-          message: 'Invalid request: Must specify a valid id.',
-          data: { request: { ...req } },
-        })
-        .serialize();
-      delete expectedError.stack;
-
-      // TODO: Replace `any` with type
-      // eslint-disable-next-line @typescript-eslint/no-explicit-any
-      const response: any = await engine.handle(req as any);
-      delete response.error.stack;
-      expect(response.error).toStrictEqual(expectedError);
-      expect(mockRequestPermissionsForOrigin).not.toHaveBeenCalled();
-    }
+    expect(mockRequestPermissionsForOrigin).toHaveBeenCalledWith({});
   });
 
   it('returns an error if the request params are invalid', async () => {
diff --git a/packages/permission-controller/src/rpc-methods/requestPermissions.ts b/packages/permission-controller/src/rpc-methods/requestPermissions.ts
index 1976eb2934d..f89143ed813 100644
--- a/packages/permission-controller/src/rpc-methods/requestPermissions.ts
+++ b/packages/permission-controller/src/rpc-methods/requestPermissions.ts
@@ -1,6 +1,5 @@
 import { isPlainObject } from '@metamask/controller-utils';
 import type { JsonRpcEngineEndCallback } from '@metamask/json-rpc-engine';
-import { rpcErrors } from '@metamask/rpc-errors';
 import type { JsonRpcRequest, PendingJsonRpcResponse } from '@metamask/utils';
 
 import { invalidParams } from '../errors';
@@ -22,7 +21,6 @@ export const requestPermissionsHandler: PermittedHandlerExport<
 
 type RequestPermissions = (
   requestedPermissions: RequestedPermissions,
-  id: string,
 ) => Promise<
   [Record<string, PermissionConstraint>, { id: string; origin: string }]
 >;
@@ -49,19 +47,7 @@ async function requestPermissionsImplementation(
   end: JsonRpcEngineEndCallback,
   { requestPermissionsForOrigin }: RequestPermissionsHooks,
 ): Promise<void> {
-  const { id, params } = req;
-
-  if (
-    (typeof id !== 'number' && typeof id !== 'string') ||
-    (typeof id === 'string' && !id)
-  ) {
-    return end(
-      rpcErrors.invalidRequest({
-        message: 'Invalid request: Must specify a valid id.',
-        data: { request: req },
-      }),
-    );
-  }
+  const { params } = req;
 
   if (!Array.isArray(params) || !isPlainObject(params[0])) {
     return end(invalidParams({ data: { request: req } }));
@@ -70,7 +56,6 @@ async function requestPermissionsImplementation(
   const [requestedPermissions] = params;
   const [grantedPermissions] = await requestPermissionsForOrigin(
     requestedPermissions,
-    String(id),
   );
 
   // `wallet_requestPermission` is specified to return an array.