Skip to content

Commit

Permalink
Simplify wallet_requestPermissions implementation (#3911)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
FrederikBolding authored Feb 9, 2024
1 parent 4433150 commit fd07735
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -22,7 +21,6 @@ export const requestPermissionsHandler: PermittedHandlerExport<

type RequestPermissions = (
requestedPermissions: RequestedPermissions,
id: string,
) => Promise<
[Record<string, PermissionConstraint>, { id: string; origin: string }]
>;
Expand All @@ -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 } }));
Expand All @@ -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.
Expand Down

0 comments on commit fd07735

Please sign in to comment.