BREAKING: Refactor RPC method params and add tests (#922)

* Refactor RPC method params and add tests

Fix jest conf after rebase

Add manageState tests and refactor it

Update jest config after rebase

Add refactoring for invoke snap method

Add some overall refactoring

Refactor update state validation process and add more tests

* Fix coverage config after rebase

* Refactor storage limit param

packages/rpc-methods/jest.config.js

@@ -5,10 +5,10 @@ module.exports = deepmerge(baseConfig, {
   coveragePathIgnorePatterns: ['./src/index.ts'],
   coverageThreshold: {
     global: {
-      branches: 82.85,
-      functions: 80.85,
-      lines: 59.77,
-      statements: 59.77,
+      branches: 87.01,
+      functions: 87.03,
+      lines: 78.96,
+      statements: 78.96,
     },
   },
   testTimeout: 2500,

packages/rpc-methods/src/permitted/invokeSnapSugar.test.ts

@@ -0,0 +1,90 @@
+import {
+  JsonRpcEngineEndCallback,
+  JsonRpcEngineNextCallback,
+  JsonRpcRequest,
+} from '@metamask/types';
+import { ethErrors } from 'eth-rpc-errors';
+import { getValidatedParams, invokeSnapSugar } from './invokeSnapSugar';
+
+describe('wallet_invokeSnap', () => {
+  describe('invokeSnapSugar', () => {
+    it('invokes snap with next()', () => {
+      const req: JsonRpcRequest<unknown> = {
+        id: 'some-id',
+        jsonrpc: '2.0',
+        method: 'wallet_invokeSnap',
+        params: {
+          snapId: 'npm:@metamask/example-snap',
+          request: { method: 'hello' },
+        },
+      };
+      const _res: unknown = {};
+      const next: JsonRpcEngineNextCallback = jest
+        .fn()
+        .mockResolvedValueOnce(true);
+      const end: JsonRpcEngineEndCallback = jest.fn();
+
+      invokeSnapSugar(req, _res, next, end);
+
+      expect(next).toHaveBeenCalledTimes(1);
+    });
+
+    it('ends with an error if params are invalid', () => {
+      const req: JsonRpcRequest<unknown> = {
+        id: 'some-id',
+        jsonrpc: '2.0',
+        method: 'wallet_invokeSnap',
+        params: {
+          snapId: undefined,
+          request: [],
+        },
+      };
+      const _res: unknown = {};
+      const next: JsonRpcEngineNextCallback = jest
+        .fn()
+        .mockResolvedValueOnce(true);
+      const end: JsonRpcEngineEndCallback = jest.fn();
+
+      invokeSnapSugar(req, _res, next, end);
+
+      expect(end).toHaveBeenCalledWith(
+        ethErrors.rpc.invalidParams({
+          message: 'Must specify a valid snap ID.',
+        }),
+      );
+      expect(next).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('getValidatedParams', () => {
+    it('throws an error if the params is not an object', () => {
+      expect(() => getValidatedParams([])).toThrow(
+        'Expected params to be a single object.',
+      );
+    });
+
+    it('throws an error if the snap ID is missing from params object', () => {
+      expect(() =>
+        getValidatedParams({ snapId: undefined, request: {} }),
+      ).toThrow('Must specify a valid snap ID.');
+    });
+
+    it('throws an error if the request is not a plain object', () => {
+      expect(() =>
+        getValidatedParams({ snapId: 'snap-id', request: [] }),
+      ).toThrow('Expected request to be a single object.');
+    });
+
+    it('returns valid parameters', () => {
+      expect(
+        getValidatedParams({
+          snapId: 'npm:@metamask/example-snap',
+          request: { method: 'hello' },
+        }),
+      ).toStrictEqual({
+        snapId: 'npm:@metamask/example-snap',
+        request: { method: 'hello' },
+      });
+    });
+  });
+});

packages/rpc-methods/src/permitted/invokeSnapSugar.ts

@@ -8,6 +8,11 @@ import {
 import { isObject } from '@metamask/utils';
 import { SNAP_PREFIX } from '@metamask/snaps-utils';
 
+export type InvokeSnapSugarArgs = {
+  snapId: string;
+  request: JsonRpcRequest<unknown>;
+};
+
 /**
  * `wallet_invokeSnap` attempts to invoke an RPC method of the specified Snap.
  */
@@ -33,25 +38,51 @@ export const invokeSnapSugarHandler: PermittedHandlerExport<
  * @returns Nothing.
  * @throws If the params are invalid.
  */
-function invokeSnapSugar(
+export function invokeSnapSugar(
   req: JsonRpcRequest<unknown>,
   _res: unknown,
   next: JsonRpcEngineNextCallback,
   end: JsonRpcEngineEndCallback,
 ): void {
-  if (
-    !Array.isArray(req.params) ||
-    typeof req.params[0] !== 'string' ||
-    !isObject(req.params[1])
-  ) {
-    return end(
-      ethErrors.rpc.invalidParams({
-        message: 'Must specify a string snap ID and a plain request object.',
-      }),
-    );
+  let params: InvokeSnapSugarArgs;
+  try {
+    params = getValidatedParams(req.params);
+  } catch (error) {
+    return end(error);
   }
 
-  req.method = SNAP_PREFIX + req.params[0];
-  req.params = [req.params[1]];
+  req.method = SNAP_PREFIX + params.snapId;
+  req.params = params.request;
   return next();
 }
+
+/**
+ * Validates the wallet_invokeSnap method `params` and returns them cast to the correct
+ * type. Throws if validation fails.
+ *
+ * @param params - The unvalidated params object from the method request.
+ * @returns The validated method parameter object.
+ */
+export function getValidatedParams(params: unknown): InvokeSnapSugarArgs {
+  if (!isObject(params)) {
+    throw ethErrors.rpc.invalidParams({
+      message: 'Expected params to be a single object.',
+    });
+  }
+
+  const { snapId, request } = params;
+
+  if (!snapId || typeof snapId !== 'string' || snapId === '') {
+    throw ethErrors.rpc.invalidParams({
+      message: 'Must specify a valid snap ID.',
+    });
+  }
+
+  if (!isObject(request)) {
+    throw ethErrors.rpc.invalidParams({
+      message: 'Expected request to be a single object.',
+    });
+  }
+
+  return params as InvokeSnapSugarArgs;
+}

packages/rpc-methods/src/restricted/invokeSnap.test.ts

@@ -48,7 +48,7 @@ describe('implementation', () => {
     await implementation({
       context: { origin: MOCK_ORIGIN },
       method: `wallet_snap_${MOCK_SNAP_ID}`,
-      params: [{ method: 'foo', params: {} }],
+      params: { method: 'foo', params: {} },
     });
 
     expect(hooks.getSnap).toHaveBeenCalledTimes(1);
@@ -73,7 +73,7 @@ describe('implementation', () => {
       implementation({
         context: { origin: MOCK_ORIGIN },
         method: `wallet_snap_${MOCK_SNAP_ID}`,
-        params: [{ method: 'foo', params: {} }],
+        params: { method: 'foo', params: {} },
       }),
     ).rejects.toThrow(
       `The snap "${MOCK_SNAP_ID}" is not installed. This is a bug, please report it.`,
@@ -92,7 +92,7 @@ describe('implementation', () => {
       implementation({
         context: { origin: MOCK_ORIGIN },
         method: `wallet_snap_${MOCK_SNAP_ID}`,
-        params: [{}],
+        params: {},
       }),
     ).rejects.toThrow(
       'Must specify a valid JSON-RPC request object as single parameter.',

packages/rpc-methods/src/restricted/invokeSnap.ts

@@ -92,12 +92,11 @@ export function getInvokeSnapImplementation({
   handleSnapRpcRequest,
 }: InvokeSnapMethodHooks) {
   return async function invokeSnap(
-    options: RestrictedMethodOptions<[Record<string, Json>]>,
+    options: RestrictedMethodOptions<Record<string, Json>>,
   ): Promise<Json> {
-    const { params = [], method, context } = options;
-    const rawRequest = params[0];
+    const { params = {}, method, context } = options;
 
-    const request = { jsonrpc: '2.0', id: nanoid(), ...rawRequest };
+    const request = { jsonrpc: '2.0', id: nanoid(), ...params };
 
     if (!isJsonRpcRequest(request)) {
       throw ethErrors.rpc.invalidParams({