Skip to content

Commit

Permalink
Fix: Keep proxies in sync across "Select networks for each site" togg…
Browse files Browse the repository at this point in the history
…ling on/off (#4130)

## Explanation

This PR fixes a bug where the proxies used to manage per dapp selected
network connections do not stay in sync when the user toggles on or off
the "Select networks for each site" preference (which turns request
queueing on and off):
<img width="367" alt="Screenshot 2024-04-12 at 10 54 52 AM"
src="https://github.com/MetaMask/core/assets/34557516/31f6acef-da3e-4ff4-b402-efdde1c6c7bb">

This PR fixes this bug by subscribing to `PreferenceController` changes
and, when the "Select networks for each site" toggle is flipped
repointing the proxy from the (nested) globally selected network proxy
to its own independently managed proxy, and when flipped off repoint the
proxy to the globally selected network proxy.

To test: pull into [this extension
PR](https://github.com/MetaMask/metamask-extension/compare/ad/fix/queue-toggle-proxies?expand=1)
-> you'll need to link to a local build of selected-network-controller
with these commits.

## Changelog

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

- **ADD**: **BREAKING:** A parameter `useRequestQueuePreference` which
should point to the current preferences state for useRequestQueue is now
required by the constructor
- **ADD**: An `onPreferencesStateChange` argument that should subscribe
to `PreferencesController` state changes and call a callback with the
updated state is now a required parameter in the constructor options
object.
- **REMOVED** - The `getUseRequestQueue` parameter is no longer accepted
by the constructor.

## 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
  • Loading branch information
adonesky1 authored Apr 16, 2024
1 parent bdeb992 commit 894460f
Show file tree
Hide file tree
Showing 2 changed files with 236 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,13 @@ export type SelectedNetworkControllerMessenger = RestrictedControllerMessenger<
AllowedEvents['type']
>;

export type GetUseRequestQueue = () => boolean;

export type SelectedNetworkControllerOptions = {
state?: SelectedNetworkControllerState;
messenger: SelectedNetworkControllerMessenger;
getUseRequestQueue: GetUseRequestQueue;
useRequestQueuePreference: boolean;
onPreferencesStateChange: (
listener: (preferencesState: { useRequestQueue: boolean }) => void,
) => void;
domainProxyMap: Map<Domain, NetworkProxy>;
};

Expand All @@ -116,21 +117,23 @@ export class SelectedNetworkController extends BaseController<
> {
#domainProxyMap: Map<Domain, NetworkProxy>;

#getUseRequestQueue: GetUseRequestQueue;
#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.getUseRequestQueue - feature flag for perDappNetwork & request queueing features
* @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(),
getUseRequestQueue,
useRequestQueuePreference,
onPreferencesStateChange,
domainProxyMap,
}: SelectedNetworkControllerOptions) {
super({
Expand All @@ -139,7 +142,7 @@ export class SelectedNetworkController extends BaseController<
messenger,
state,
});
this.#getUseRequestQueue = getUseRequestQueue;
this.#useRequestQueuePreference = useRequestQueuePreference;
this.#domainProxyMap = domainProxyMap;
this.#registerMessageHandlers();

Expand Down Expand Up @@ -201,6 +204,21 @@ 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 @@ -262,6 +280,23 @@ 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,
Expand All @@ -284,7 +319,7 @@ export class SelectedNetworkController extends BaseController<
getNetworkClientIdForDomain(domain: Domain): NetworkClientId {
const { selectedNetworkClientId: metamaskSelectedNetworkClientId } =
this.messagingSystem.call('NetworkController:getState');
if (!this.#getUseRequestQueue()) {
if (!this.#useRequestQueuePreference) {
return metamaskSelectedNetworkClientId;
}
return this.state.domains[domain] ?? metamaskSelectedNetworkClientId;
Expand All @@ -297,11 +332,11 @@ export class SelectedNetworkController extends BaseController<
* @returns The proxy and block tracker proxies.
*/
getProviderAndBlockTracker(domain: Domain): NetworkProxy {
const networkClientId = this.state.domains[domain];
const networkClientId = this.getNetworkClientIdForDomain(domain);
let networkProxy = this.#domainProxyMap.get(domain);
if (networkProxy === undefined) {
let networkClient;
if (networkClientId === undefined) {
if (networkClientId === undefined || !this.#useRequestQueuePreference) {
networkClient = this.messagingSystem.call(
'NetworkController:getSelectedNetworkClient',
);
Expand All @@ -322,7 +357,6 @@ export class SelectedNetworkController extends BaseController<
};
this.#domainProxyMap.set(domain, networkProxy);
}

return networkProxy;
}
}
Loading

0 comments on commit 894460f

Please sign in to comment.