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: Focus approval request UI when a request is queued #4456

Merged
merged 5 commits into from
Jun 24, 2024
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 @@ -28,6 +28,7 @@ describe('QueuedRequestController', () => {
messenger: buildQueuedRequestControllerMessenger(),
shouldRequestSwitchNetwork: () => false,
clearPendingConfirmations: jest.fn(),
showApprovalRequest: jest.fn(),
};

const controller = new QueuedRequestController(options);
Expand Down Expand Up @@ -184,6 +185,32 @@ describe('QueuedRequestController', () => {
await secondRequest;
});

it('focuses the existing approval request UI if a request from another origin is being processed', async () => {
const mockShowApprovalRequest = jest.fn();
const controller = buildQueuedRequestController({
showApprovalRequest: mockShowApprovalRequest,
});
// Trigger first request
const firstRequest = controller.enqueueRequest(
{ ...buildRequest(), origin: 'https://exampleorigin1.metamask.io' },
() => new Promise((resolve) => setTimeout(resolve, 10)),
);

const secondRequestNext = jest.fn();
const secondRequest = controller.enqueueRequest(
{ ...buildRequest(), origin: 'https://exampleorigin2.metamask.io' },
secondRequestNext,
);

// should focus the existing approval immediately after being queued
expect(mockShowApprovalRequest).toHaveBeenCalledTimes(1);

await firstRequest;
await secondRequest;

expect(mockShowApprovalRequest).toHaveBeenCalledTimes(1);
});

it('drains batch from queue when current batch finishes', async () => {
const controller = buildQueuedRequestController();
// Trigger first batch
Expand Down Expand Up @@ -819,6 +846,7 @@ describe('QueuedRequestController', () => {
shouldRequestSwitchNetwork: ({ method }) =>
method === 'eth_sendTransaction',
clearPendingConfirmations: jest.fn(),
showApprovalRequest: jest.fn(),
};

const controller = new QueuedRequestController(options);
Expand Down Expand Up @@ -901,6 +929,7 @@ describe('QueuedRequestController', () => {
shouldRequestSwitchNetwork: ({ method }) =>
method === 'eth_sendTransaction',
clearPendingConfirmations: jest.fn(),
showApprovalRequest: jest.fn(),
};

const controller = new QueuedRequestController(options);
Expand Down Expand Up @@ -1037,6 +1066,7 @@ function buildQueuedRequestController(
messenger: buildQueuedRequestControllerMessenger(),
shouldRequestSwitchNetwork: () => false,
clearPendingConfirmations: jest.fn(),
showApprovalRequest: jest.fn(),
...overrideOptions,
};

Expand Down
57 changes: 37 additions & 20 deletions packages/queued-request-controller/src/QueuedRequestController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export type QueuedRequestControllerOptions = {
request: QueuedRequestMiddlewareJsonRpcRequest,
) => boolean;
clearPendingConfirmations: () => void;
showApprovalRequest: () => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be optional?..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not? this controller will only be used on the extension AFAIK and I believe we will always want this hook passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's optional in the sense that this isn't required for the QueuedRequestController to perform it's core function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thats definitely true. I guess I think of the type as safeguard for accidental removal... though maybe that's not really true

};

/**
Expand Down Expand Up @@ -149,20 +150,33 @@ export class QueuedRequestController extends BaseController<
request: QueuedRequestMiddlewareJsonRpcRequest,
) => boolean;

/**
* This is a function that clears all pending confirmations across
* several controllers that may handle them.
*/
#clearPendingConfirmations: () => void;

/**
* This is a function that makes the confirmation notification view
* become visible and focused to the user
*/
#showApprovalRequest: () => void;

/**
* Construct a QueuedRequestController.
*
* @param options - Controller options.
* @param options.messenger - The restricted controller messenger that facilitates communication with other controllers.
* @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.
* @param options.showApprovalRequest - A function for opening the UI such that
* the existing request can be displayed to the user.
*/
constructor({
messenger,
shouldRequestSwitchNetwork,
clearPendingConfirmations,
showApprovalRequest,
}: QueuedRequestControllerOptions) {
super({
name: controllerName,
Expand All @@ -175,8 +189,10 @@ export class QueuedRequestController extends BaseController<
messenger,
state: { queuedRequestCount: 0 },
});

this.#shouldRequestSwitchNetwork = shouldRequestSwitchNetwork;
this.#clearPendingConfirmations = clearPendingConfirmations;
this.#showApprovalRequest = showApprovalRequest;
this.#registerMessageHandlers();
}

Expand Down Expand Up @@ -301,6 +317,25 @@ export class QueuedRequestController extends BaseController<
});
}

async #waitForDequeue(origin: string): Promise<void> {
const { promise, reject, resolve } = createDeferredPromise({
suppressUnhandledRejection: true,
});
this.#requestQueue.push({
origin,
processRequest: (error: unknown) => {
if (error) {
reject(error);
} else {
resolve();
}
},
});
this.#updateQueuedRequestCount();

return promise;
}

/**
* Enqueue a request to be processed in a batch with other requests from the same origin.
*
Expand Down Expand Up @@ -330,26 +365,8 @@ export class QueuedRequestController extends BaseController<
this.state.queuedRequestCount > 0 ||
this.#originOfCurrentBatch !== request.origin
) {
const {
promise: waitForDequeue,
reject,
resolve,
} = createDeferredPromise({
suppressUnhandledRejection: true,
});
this.#requestQueue.push({
origin: request.origin,
processRequest: (error: unknown) => {
if (error) {
reject(error);
} else {
resolve();
}
},
});
this.#updateQueuedRequestCount();

await waitForDequeue;
this.#showApprovalRequest();
await this.#waitForDequeue(request.origin);
} else if (this.#shouldRequestSwitchNetwork(request)) {
// Process request immediately
// Requires switching network now if necessary
Expand Down
Loading