Skip to content

Commit

Permalink
chore: Remove useRequestQueue toggle to turn on/off Per Dapp Select…
Browse files Browse the repository at this point in the history
…ed Network Feature (#4941)

## Explanation

The "Select networks for each site" preference toggle on the
experimental settings page has been live for many releases now since the
toggle has been turned on by default. We meant to remove it a while ago.

<img width="770" alt="Screenshot 2024-11-19 at 10 58 04 AM"
src="https://github.com/user-attachments/assets/aeca2483-c019-4f9f-b44e-520d10db9eee">

This PR removes the togglability of per dapp selected network from the
`SelectedNetworkController` and `QueuedRequestMiddleware`

The extension to integrate this change will also have to implement a
migration to remove this state.

Another reason we should remove this now is that having this setting
turned off is [causing a
bug](MetaMask/metamask-extension#28441) to
result with `wallet_switchEthereumChain` and the interaction between the
new chain permissions feature and the absence of queuing.

## References

Extension PR w/ preview builds of this PR (plus full removal of the
toggle): MetaMask/metamask-extension#28577

When integrated in the extension will resolve this issue:
MetaMask/metamask-extension#28441

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/queued-request-controller`

- **REMOVED**: `createQueuedRequestMiddleware` no longer expects a
`useRequestQueue` property in its param options.


### `@metamask/selected-network-controller`

- **REMOVED**: SelectedNetworkController constructure no longer expects
either `useRequestQueuePreference` or `onPreferencesStateChange` params

## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
- [ ] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes
  • Loading branch information
adonesky1 authored Nov 22, 2024
1 parent 1e23e90 commit 4814cf1
Show file tree
Hide file tree
Showing 4 changed files with 394 additions and 746 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ describe('createQueuedRequestMiddleware', () => {
const mockEnqueueRequest = getMockEnqueueRequest();
const middleware = buildQueuedRequestMiddleware({
enqueueRequest: mockEnqueueRequest,
useRequestQueue: () => true,
});

const request = {
Expand All @@ -105,7 +104,7 @@ describe('createQueuedRequestMiddleware', () => {
const mockEnqueueRequest = getMockEnqueueRequest();
const middleware = buildQueuedRequestMiddleware({
enqueueRequest: mockEnqueueRequest,
useRequestQueue: () => true,

shouldEnqueueRequest: ({ method }) =>
method === 'method_with_confirmation',
});
Expand Down Expand Up @@ -145,7 +144,6 @@ describe('createQueuedRequestMiddleware', () => {
it('calls next after a request is queued and processed', async () => {
const middleware = buildQueuedRequestMiddleware({
enqueueRequest: getMockEnqueueRequest(),
useRequestQueue: () => true,
});
const request = {
...getRequestDefaults(),
Expand All @@ -167,7 +165,7 @@ describe('createQueuedRequestMiddleware', () => {
enqueueRequest: jest
.fn()
.mockRejectedValue(new Error('enqueuing error')),
useRequestQueue: () => true,

shouldEnqueueRequest: () => true,
});
const request = {
Expand All @@ -191,7 +189,7 @@ describe('createQueuedRequestMiddleware', () => {
enqueueRequest: jest
.fn()
.mockRejectedValue(new Error('enqueuing error')),
useRequestQueue: () => true,

shouldEnqueueRequest: () => true,
});
const request = {
Expand Down Expand Up @@ -271,7 +269,6 @@ function buildQueuedRequestMiddleware(
) {
const options = {
enqueueRequest: getMockEnqueueRequest(),
useRequestQueue: () => false,
shouldEnqueueRequest: () => false,
...overrideOptions,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,27 +38,23 @@ 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.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,
shouldEnqueueRequest,
}: {
enqueueRequest: QueuedRequestController['enqueueRequest'];
useRequestQueue: () => boolean;
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() || !shouldEnqueueRequest(req)) {
// if this method is not a confirmation method bypass the queue completely
if (!shouldEnqueueRequest(req)) {
return await next();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,6 @@ export type SelectedNetworkControllerMessenger = RestrictedControllerMessenger<
export type SelectedNetworkControllerOptions = {
state?: SelectedNetworkControllerState;
messenger: SelectedNetworkControllerMessenger;
useRequestQueuePreference: boolean;
onPreferencesStateChange: (
listener: (preferencesState: { useRequestQueue: boolean }) => void,
) => void;
domainProxyMap: Map<Domain, NetworkProxy>;
};

Expand All @@ -124,23 +120,17 @@ export class SelectedNetworkController extends BaseController<
> {
#domainProxyMap: Map<Domain, NetworkProxy>;

#useRequestQueuePreference: boolean;

/**
* Construct a SelectedNetworkController controller.
*
* @param options - The controller options.
* @param options.messenger - The restricted controller messenger for the EncryptionPublicKey controller.
* @param options.state - The controllers initial state.
* @param options.useRequestQueuePreference - A boolean indicating whether to use the request queue preference.
* @param options.onPreferencesStateChange - A callback that is called when the preference state changes.
* @param options.domainProxyMap - A map for storing domain-specific proxies that are held in memory only during use.
*/
constructor({
messenger,
state = getDefaultState(),
useRequestQueuePreference,
onPreferencesStateChange,
domainProxyMap,
}: SelectedNetworkControllerOptions) {
super({
Expand All @@ -149,7 +139,6 @@ export class SelectedNetworkController extends BaseController<
messenger,
state,
});
this.#useRequestQueuePreference = useRequestQueuePreference;
this.#domainProxyMap = domainProxyMap;
this.#registerMessageHandlers();

Expand Down Expand Up @@ -247,21 +236,6 @@ export class SelectedNetworkController extends BaseController<
}
},
);

onPreferencesStateChange(({ useRequestQueue }) => {
if (this.#useRequestQueuePreference !== useRequestQueue) {
if (!useRequestQueue) {
// Loop through all domains and points each domain's proxy
// to the NetworkController's own proxy of the globally selected networkClient
Object.keys(this.state.domains).forEach((domain) => {
this.#unsetNetworkClientIdForDomain(domain);
});
} else {
this.#resetAllPermissionedDomains();
}
this.#useRequestQueuePreference = useRequestQueue;
}
});
}

#registerMessageHandlers(): void {
Expand Down Expand Up @@ -326,31 +300,10 @@ export class SelectedNetworkController extends BaseController<
);
}

// Loop through all domains and for those with permissions it points that domain's proxy
// to an unproxied instance of the globally selected network client.
// NOT the NetworkController's proxy of the globally selected networkClient
#resetAllPermissionedDomains() {
this.#domainProxyMap.forEach((_: NetworkProxy, domain: string) => {
const { selectedNetworkClientId } = this.messagingSystem.call(
'NetworkController:getState',
);
// can't use public setNetworkClientIdForDomain because it will throw an error
// rather than simply skip if the domain doesn't have permissions which can happen
// in this case since proxies are added for each site the user visits
if (this.#domainHasPermissions(domain)) {
this.#setNetworkClientIdForDomain(domain, selectedNetworkClientId);
}
});
}

setNetworkClientIdForDomain(
domain: Domain,
networkClientId: NetworkClientId,
) {
if (!this.#useRequestQueuePreference) {
return;
}

if (domain === METAMASK_DOMAIN) {
throw new Error(
`NetworkClientId for domain "${METAMASK_DOMAIN}" cannot be set on the SelectedNetworkController`,
Expand All @@ -373,9 +326,6 @@ export class SelectedNetworkController extends BaseController<
getNetworkClientIdForDomain(domain: Domain): NetworkClientId {
const { selectedNetworkClientId: metamaskSelectedNetworkClientId } =
this.messagingSystem.call('NetworkController:getState');
if (!this.#useRequestQueuePreference) {
return metamaskSelectedNetworkClientId;
}
return this.state.domains[domain] ?? metamaskSelectedNetworkClientId;
}

Expand Down Expand Up @@ -403,10 +353,7 @@ export class SelectedNetworkController extends BaseController<
let networkProxy = this.#domainProxyMap.get(domain);
if (networkProxy === undefined) {
let networkClient;
if (
this.#useRequestQueuePreference &&
this.#domainHasPermissions(domain)
) {
if (this.#domainHasPermissions(domain)) {
const networkClientId = this.getNetworkClientIdForDomain(domain);
networkClient = this.messagingSystem.call(
'NetworkController:getNetworkClientById',
Expand All @@ -416,10 +363,11 @@ export class SelectedNetworkController extends BaseController<
networkClient = this.messagingSystem.call(
'NetworkController:getSelectedNetworkClient',
);
if (networkClient === undefined) {
throw new Error('Selected network not initialized');
}
}
if (networkClient === undefined) {
throw new Error('Selected network not initialized');
}

networkProxy = {
provider: createEventEmitterProxy(networkClient.provider),
blockTracker: createEventEmitterProxy(networkClient.blockTracker, {
Expand Down
Loading

0 comments on commit 4814cf1

Please sign in to comment.