Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: change queued-request-controller methods array params to callbacks #4423

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('QueuedRequestController', () => {
it('can be instantiated with default values', () => {
const options: QueuedRequestControllerOptions = {
messenger: buildQueuedRequestControllerMessenger(),
methodsRequiringNetworkSwitch: [],
shouldRequestSwitchNetwork: () => false,
clearPendingConfirmations: jest.fn(),
};

Expand Down Expand Up @@ -62,7 +62,7 @@ describe('QueuedRequestController', () => {
await firstRequest;
});

it('switches network if a request comes in for a different network client and the method is in the methodsRequiringNetworkSwitch param', async () => {
it('switches network if a request comes in for a different network client and shouldRequestSwitchNetwork returns true', async () => {
const mockSetActiveNetwork = jest.fn();
const { messenger } = buildControllerMessenger({
networkControllerGetState: jest.fn().mockReturnValue({
Expand All @@ -81,7 +81,8 @@ describe('QueuedRequestController', () => {
);
const controller = buildQueuedRequestController({
messenger: buildQueuedRequestControllerMessenger(messenger),
methodsRequiringNetworkSwitch: ['method_requiring_network_switch'],
shouldRequestSwitchNetwork: ({ method }) =>
method === 'method_requiring_network_switch',
clearPendingConfirmations: jest.fn(),
});

Expand All @@ -98,7 +99,7 @@ describe('QueuedRequestController', () => {
);
});

it('does not switch networks if the method is not in the methodsRequiringNetworkSwitch param', async () => {
it('does not switch networks if shouldRequestSwitchNetwork returns false', async () => {
const mockSetActiveNetwork = jest.fn();
const { messenger } = buildControllerMessenger({
networkControllerGetState: jest.fn().mockReturnValue({
Expand All @@ -117,11 +118,12 @@ describe('QueuedRequestController', () => {
);
const controller = buildQueuedRequestController({
messenger: buildQueuedRequestControllerMessenger(messenger),
methodsRequiringNetworkSwitch: [],
shouldRequestSwitchNetwork: ({ method }) =>
method === 'method_requiring_network_switch',
});

await controller.enqueueRequest(
{ ...buildRequest(), method: 'not_in_methodsRequiringNetworkSwitch' },
{ ...buildRequest(), method: 'not_requiring_network_switch' },
() => new Promise((resolve) => setTimeout(resolve, 10)),
);

Expand Down Expand Up @@ -537,7 +539,8 @@ describe('QueuedRequestController', () => {
});
const controller = buildQueuedRequestController({
messenger: buildQueuedRequestControllerMessenger(messenger),
methodsRequiringNetworkSwitch: ['method_requiring_network_switch'],
shouldRequestSwitchNetwork: ({ method }) =>
method === 'method_requiring_network_switch',
});

await expect(() =>
Expand Down Expand Up @@ -572,7 +575,8 @@ describe('QueuedRequestController', () => {
});
const controller = buildQueuedRequestController({
messenger: buildQueuedRequestControllerMessenger(messenger),
methodsRequiringNetworkSwitch: ['method_requiring_network_switch'],
shouldRequestSwitchNetwork: ({ method }) =>
method === 'method_requiring_network_switch',
});
const firstRequest = controller.enqueueRequest(
{
Expand Down Expand Up @@ -626,7 +630,8 @@ describe('QueuedRequestController', () => {
});
const controller = buildQueuedRequestController({
messenger: buildQueuedRequestControllerMessenger(messenger),
methodsRequiringNetworkSwitch: ['method_requiring_network_switch'],
shouldRequestSwitchNetwork: ({ method }) =>
method === 'method_requiring_network_switch',
});
const firstRequest = controller.enqueueRequest(
{
Expand Down Expand Up @@ -679,7 +684,8 @@ describe('QueuedRequestController', () => {
});
const controller = buildQueuedRequestController({
messenger: buildQueuedRequestControllerMessenger(messenger),
methodsRequiringNetworkSwitch: ['method_requiring_network_switch'],
shouldRequestSwitchNetwork: ({ method }) =>
method === 'method_requiring_network_switch',
});
const firstRequest = controller.enqueueRequest(
{
Expand Down Expand Up @@ -810,7 +816,8 @@ describe('QueuedRequestController', () => {

const options: QueuedRequestControllerOptions = {
messenger: buildQueuedRequestControllerMessenger(messenger),
methodsRequiringNetworkSwitch: ['eth_sendTransaction'],
shouldRequestSwitchNetwork: ({ method }) =>
method === 'eth_sendTransaction',
clearPendingConfirmations: jest.fn(),
};

Expand Down Expand Up @@ -891,7 +898,8 @@ describe('QueuedRequestController', () => {

const options: QueuedRequestControllerOptions = {
messenger: buildQueuedRequestControllerMessenger(messenger),
methodsRequiringNetworkSwitch: ['eth_sendTransaction'],
shouldRequestSwitchNetwork: ({ method }) =>
method === 'eth_sendTransaction',
clearPendingConfirmations: jest.fn(),
};

Expand Down Expand Up @@ -1027,7 +1035,7 @@ function buildQueuedRequestController(
): QueuedRequestController {
const options: QueuedRequestControllerOptions = {
messenger: buildQueuedRequestControllerMessenger(),
methodsRequiringNetworkSwitch: [],
shouldRequestSwitchNetwork: () => false,
clearPendingConfirmations: jest.fn(),
...overrideOptions,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ export type QueuedRequestControllerMessenger = RestrictedControllerMessenger<

export type QueuedRequestControllerOptions = {
messenger: QueuedRequestControllerMessenger;
methodsRequiringNetworkSwitch: string[];
shouldRequestSwitchNetwork: (
request: QueuedRequestMiddlewareJsonRpcRequest,
) => boolean;
clearPendingConfirmations: () => void;
};

Expand Down Expand Up @@ -136,14 +138,16 @@ export class QueuedRequestController extends BaseController<
#processingRequestCount = 0;

/**
* This is a list of methods that require the globally selected network
* to match the dapp selected network before being processed. These can
* This is a function that returns true if a request requires the globally selected
* network to match the dapp selected network before being processed. These can
* be for UI/UX reasons where the currently selected network is displayed
* in the confirmation even though it will be submitted on the correct
* network for the dapp. It could also be that a method expects the
* globally selected network to match some value in the request params itself.
*/
readonly #methodsRequiringNetworkSwitch: string[];
readonly #shouldRequestSwitchNetwork: (
request: QueuedRequestMiddlewareJsonRpcRequest,
) => boolean;

#clearPendingConfirmations: () => void;

Expand All @@ -152,12 +156,12 @@ export class QueuedRequestController extends BaseController<
*
* @param options - Controller options.
* @param options.messenger - The restricted controller messenger that facilitates communication with other controllers.
* @param options.methodsRequiringNetworkSwitch - A list of methods that require the globally selected network to match the dapp selected network.
* @param options.shouldRequestSwitchNetwork - A function that returns if a request requires the globally selected network to match the dapp selected network.
* @param options.clearPendingConfirmations - A function that will clear all the pending confirmations.
*/
constructor({
messenger,
methodsRequiringNetworkSwitch,
shouldRequestSwitchNetwork,
clearPendingConfirmations,
}: QueuedRequestControllerOptions) {
super({
Expand All @@ -171,7 +175,7 @@ export class QueuedRequestController extends BaseController<
messenger,
state: { queuedRequestCount: 0 },
});
this.#methodsRequiringNetworkSwitch = methodsRequiringNetworkSwitch;
this.#shouldRequestSwitchNetwork = shouldRequestSwitchNetwork;
this.#clearPendingConfirmations = clearPendingConfirmations;
this.#registerMessageHandlers();
}
Expand Down Expand Up @@ -346,7 +350,7 @@ export class QueuedRequestController extends BaseController<
this.#updateQueuedRequestCount();

await waitForDequeue;
} else if (this.#methodsRequiringNetworkSwitch.includes(request.method)) {
} else if (this.#shouldRequestSwitchNetwork(request)) {
// Process request immediately
// Requires switching network now if necessary
await this.#switchNetworkIfNecessary();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,13 @@ describe('createQueuedRequestMiddleware', () => {
expect(mockEnqueueRequest).not.toHaveBeenCalled();
});

it('enqueues the request if the method is in the methodsWithConfirmation param', async () => {
it('enqueues the request if shouldEnqueueRest returns true', async () => {
const mockEnqueueRequest = getMockEnqueueRequest();
const middleware = buildQueuedRequestMiddleware({
enqueueRequest: mockEnqueueRequest,
useRequestQueue: () => true,
methodsWithConfirmation: ['method_with_confirmation'],
shouldEnqueueRequest: ({ method }) =>
method === 'method_with_confirmation',
});
const request = {
...getRequestDefaults(),
Expand Down Expand Up @@ -167,7 +168,7 @@ describe('createQueuedRequestMiddleware', () => {
.fn()
.mockRejectedValue(new Error('enqueuing error')),
useRequestQueue: () => true,
methodsWithConfirmation: ['method_should_be_enqueued'],
shouldEnqueueRequest: () => true,
});
const request = {
...getRequestDefaults(),
Expand All @@ -191,7 +192,7 @@ describe('createQueuedRequestMiddleware', () => {
.fn()
.mockRejectedValue(new Error('enqueuing error')),
useRequestQueue: () => true,
methodsWithConfirmation: ['method_should_be_enqueued'],
shouldEnqueueRequest: () => true,
});
const request = {
...getRequestDefaults(),
Expand Down Expand Up @@ -271,7 +272,7 @@ function buildQueuedRequestMiddleware(
const options = {
enqueueRequest: getMockEnqueueRequest(),
useRequestQueue: () => false,
methodsWithConfirmation: [],
shouldEnqueueRequest: () => false,
...overrideOptions,
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,24 +39,26 @@ function hasRequiredMetadata(
* @param options - Configuration options.
* @param options.enqueueRequest - A method for enqueueing a request.
* @param options.useRequestQueue - A function that determines if the request queue feature is enabled.
* @param options.methodsWithConfirmation - A list of methods that can cause a confirmation to be presented to the user.
* @param options.shouldEnqueueRequest - A function that returns if a request should be handled by the QueuedRequestController.
* @returns The JSON-RPC middleware that manages queued requests.
*/
export const createQueuedRequestMiddleware = ({
enqueueRequest,
useRequestQueue,
methodsWithConfirmation,
shouldEnqueueRequest,
}: {
enqueueRequest: QueuedRequestController['enqueueRequest'];
useRequestQueue: () => boolean;
methodsWithConfirmation: string[];
shouldEnqueueRequest: (
request: QueuedRequestMiddlewareJsonRpcRequest,
) => boolean;
}): JsonRpcMiddleware<JsonRpcParams, Json> => {
return createAsyncMiddleware(async (req: JsonRpcRequest, res, next) => {
hasRequiredMetadata(req);

// if the request queue feature is turned off, or this method is not a confirmation method
// bypass the queue completely
if (!useRequestQueue() || !methodsWithConfirmation.includes(req.method)) {
if (!useRequestQueue() || !shouldEnqueueRequest(req)) {
return await next();
}

Expand Down
Loading