From 17c52d99dc8585e3a4f9ed5b06c486d726a83b93 Mon Sep 17 00:00:00 2001 From: Zachary Belford Date: Tue, 27 Feb 2024 15:17:56 -0800 Subject: [PATCH] remove redundant perDomainNetwork state (#3989) ## Explanation See [here](https://app.zenhub.com/workspaces/wallet-api-platform-63bee08a4e3b9d001108416e/issues/gh/metamask/metamask-planning/2143) for more details ## References ## Changelog ### `@metamask/selected-network-controller` - **ADDED**: SelectedNetworkController constructor now takes a `getUseRequestQueue` function. - **REMOVED**: SelectedNetworkController no longer has a perDappNetwork feature flag saved in state. ## 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 --- .../src/SelectedNetworkController.ts | 33 +++----- .../tests/SelectedNetworkController.test.ts | 76 ++++++------------- 2 files changed, 37 insertions(+), 72 deletions(-) diff --git a/packages/selected-network-controller/src/SelectedNetworkController.ts b/packages/selected-network-controller/src/SelectedNetworkController.ts index 9093ae4633..99f1b33d16 100644 --- a/packages/selected-network-controller/src/SelectedNetworkController.ts +++ b/packages/selected-network-controller/src/SelectedNetworkController.ts @@ -20,13 +20,9 @@ export const controllerName = 'SelectedNetworkController'; const stateMetadata = { domains: { persist: true, anonymous: false }, - perDomainNetwork: { persist: true, anonymous: false }, }; -const getDefaultState = () => ({ - domains: {}, - perDomainNetwork: false, -}); +const getDefaultState = () => ({ domains: {} }); type Domain = string; @@ -46,12 +42,6 @@ export const SelectedNetworkControllerEventTypes = { export type SelectedNetworkControllerState = { domains: Record; - /** - * Feature flag to start returning networkClientId based on the domain. - * when the flag is false, the 'metamask' domain will always be used. - * defaults to false - */ - perDomainNetwork: boolean; }; export type SelectedNetworkControllerStateChangeEvent = { @@ -100,9 +90,12 @@ export type SelectedNetworkControllerMessenger = RestrictedControllerMessenger< AllowedEvents['type'] >; +export type GetUseRequestQueue = () => boolean; + export type SelectedNetworkControllerOptions = { state?: SelectedNetworkControllerState; messenger: SelectedNetworkControllerMessenger; + getUseRequestQueue: GetUseRequestQueue; }; export type NetworkProxy = { @@ -120,16 +113,20 @@ export class SelectedNetworkController extends BaseController< > { #proxies = new Map(); + #getUseRequestQueue: GetUseRequestQueue; + /** * 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 */ constructor({ messenger, state = getDefaultState(), + getUseRequestQueue, }: SelectedNetworkControllerOptions) { super({ name: controllerName, @@ -137,6 +134,7 @@ export class SelectedNetworkController extends BaseController< messenger, state, }); + this.#getUseRequestQueue = getUseRequestQueue; this.#registerMessageHandlers(); // this is fetching all the dapp permissions from the PermissionsController and looking for any domains that are not in domains state in this controller. Then we take any missing domains and add them to state here, setting it with the globally selected networkClientId (fetched from the NetworkController) @@ -267,7 +265,7 @@ export class SelectedNetworkController extends BaseController< getNetworkClientIdForDomain(domain: Domain): NetworkClientId { const { selectedNetworkClientId: metamaskSelectedNetworkClientId } = this.messagingSystem.call('NetworkController:getState'); - if (!this.state.perDomainNetwork) { + if (!this.#getUseRequestQueue()) { return metamaskSelectedNetworkClientId; } return this.state.domains[domain] ?? metamaskSelectedNetworkClientId; @@ -280,9 +278,9 @@ export class SelectedNetworkController extends BaseController< * @returns The proxy and block tracker proxies. */ getProviderAndBlockTracker(domain: Domain): NetworkProxy { - if (!this.state.perDomainNetwork) { + if (!this.#getUseRequestQueue()) { throw new Error( - 'Provider and BlockTracker should be fetched from NetworkController when perDomainNetwork is false', + 'Provider and BlockTracker should be fetched from NetworkController when useRequestQueue is false', ); } const networkClientId = this.state.domains[domain]; @@ -308,11 +306,4 @@ export class SelectedNetworkController extends BaseController< return networkProxy; } - - setPerDomainNetwork(enabled: boolean) { - this.update((state) => { - state.perDomainNetwork = enabled; - return state; - }); - } } diff --git a/packages/selected-network-controller/tests/SelectedNetworkController.test.ts b/packages/selected-network-controller/tests/SelectedNetworkController.test.ts index a452945634..d4dfcd897a 100644 --- a/packages/selected-network-controller/tests/SelectedNetworkController.test.ts +++ b/packages/selected-network-controller/tests/SelectedNetworkController.test.ts @@ -4,6 +4,7 @@ import { createEventEmitterProxy } from '@metamask/swappable-obj-proxy'; import type { AllowedActions, AllowedEvents, + GetUseRequestQueue, SelectedNetworkControllerActions, SelectedNetworkControllerEvents, SelectedNetworkControllerMessenger, @@ -90,10 +91,12 @@ const setup = ({ hasPermissions = true, getSubjectNames = [], state, + getUseRequestQueue = () => false, }: { hasPermissions?: boolean; state?: SelectedNetworkControllerState; getSubjectNames?: string[]; + getUseRequestQueue?: GetUseRequestQueue; } = {}) => { const mockProviderProxy = { setTarget: jest.fn(), @@ -139,6 +142,7 @@ const setup = ({ const controller = new SelectedNetworkController({ messenger: selectedNetworkControllerMessenger, state, + getUseRequestQueue, }); return { controller, @@ -158,19 +162,16 @@ describe('SelectedNetworkController', () => { const { controller } = setup(); expect(controller.state).toStrictEqual({ domains: {}, - perDomainNetwork: false, }); }); it('can be instantiated with a state', () => { const { controller } = setup({ state: { - perDomainNetwork: true, domains: { networkClientId: 'goerli' }, }, }); expect(controller.state).toStrictEqual({ domains: { networkClientId: 'goerli' }, - perDomainNetwork: true, }); }); }); @@ -180,7 +181,6 @@ describe('SelectedNetworkController', () => { it('updates the networkClientId for domains which were previously set to the deleted networkClientId', () => { const { controller, messenger } = setup({ state: { - perDomainNetwork: true, domains: { metamask: 'goerli', 'example.com': 'test-network-client-id', @@ -223,12 +223,11 @@ describe('SelectedNetworkController', () => { ); expect(controller.state.domains.metamask).toBeUndefined(); }); - describe('when the perDomainNetwork state is false', () => { + describe('when the useRequestQueue is false', () => { describe('when the requesting domain is not metamask', () => { it('updates the networkClientId for domain in state', () => { const { controller } = setup({ state: { - perDomainNetwork: false, domains: { '1.com': 'mainnet', '2.com': 'mainnet', @@ -250,12 +249,13 @@ describe('SelectedNetworkController', () => { }); }); - describe('when the perDomainNetwork state is true', () => { + describe('when the useRequestQueue is true', () => { describe('when the requesting domain has existing permissions', () => { it('sets the networkClientId for the passed in domain', () => { const { controller } = setup({ - state: { perDomainNetwork: true, domains: {} }, + state: { domains: {} }, hasPermissions: true, + getUseRequestQueue: () => true, }); const domain = 'example.com'; @@ -266,8 +266,9 @@ describe('SelectedNetworkController', () => { it('updates the provider and block tracker proxy when they already exist for the domain', () => { const { controller, mockProviderProxy } = setup({ - state: { perDomainNetwork: true, domains: {} }, + state: { domains: {} }, hasPermissions: true, + getUseRequestQueue: () => true, }); const initialNetworkClientId = '123'; @@ -294,7 +295,7 @@ describe('SelectedNetworkController', () => { describe('when the requesting domain does not have permissions', () => { it('throw an error and does not set the networkClientId for the passed in domain', () => { const { controller } = setup({ - state: { perDomainNetwork: true, domains: {} }, + state: { domains: {} }, hasPermissions: false, }); @@ -312,7 +313,7 @@ describe('SelectedNetworkController', () => { }); describe('getNetworkClientIdForDomain', () => { - describe('when the perDomainNetwork state is false', () => { + describe('when the useRequestQueue is false', () => { it('returns the selectedNetworkClientId from the NetworkController if not no networkClientId is set for requested domain', () => { const { controller } = setup(); expect(controller.getNetworkClientIdForDomain('example.com')).toBe( @@ -334,11 +335,12 @@ describe('SelectedNetworkController', () => { }); }); - describe('when the perDomainNetwork state is true', () => { + describe('when the useRequestQueue is true', () => { it('returns the networkClientId for the passed in domain, when a networkClientId has been set for the requested domain', () => { const { controller } = setup({ - state: { perDomainNetwork: true, domains: {} }, + state: { domains: {} }, hasPermissions: true, + getUseRequestQueue: () => true, }); const networkClientId1 = 'network5'; const networkClientId2 = 'network6'; @@ -352,8 +354,9 @@ describe('SelectedNetworkController', () => { it('returns the selectedNetworkClientId from the NetworkController when no networkClientId has been set for the domain requested', () => { const { controller } = setup({ - state: { perDomainNetwork: true, domains: {} }, + state: { domains: {} }, hasPermissions: true, + getUseRequestQueue: () => true, }); expect(controller.getNetworkClientIdForDomain('example.com')).toBe( 'mainnet', @@ -363,13 +366,13 @@ describe('SelectedNetworkController', () => { }); describe('getProviderAndBlockTracker', () => { - describe('when perDomainNetwork is true', () => { + describe('when useRequestQueue is true', () => { it('returns a proxy provider and block tracker when a networkClientId has been set for the requested domain', () => { const { controller } = setup({ state: { - perDomainNetwork: true, domains: {}, }, + getUseRequestQueue: () => true, }); controller.setNetworkClientIdForDomain('example.com', 'network7'); const result = controller.getProviderAndBlockTracker('example.com'); @@ -379,11 +382,11 @@ describe('SelectedNetworkController', () => { it('creates a new proxy provider and block tracker when there isnt one already', () => { const { controller } = setup({ state: { - perDomainNetwork: true, domains: { 'test.com': 'mainnet', }, }, + getUseRequestQueue: () => true, }); const result = controller.getProviderAndBlockTracker('test.com'); expect(result).toBeDefined(); @@ -392,9 +395,9 @@ describe('SelectedNetworkController', () => { it('throws and error when a networkClientId has not been set for the requested domain', () => { const { controller } = setup({ state: { - perDomainNetwork: true, domains: {}, }, + getUseRequestQueue: () => true, }); expect(() => { @@ -402,11 +405,10 @@ describe('SelectedNetworkController', () => { }).toThrow('NetworkClientId has not been set for the requested domain'); }); }); - describe('when perDomainNetwork is false', () => { + describe('when useRequestQueue is false', () => { it('throws and error when a networkClientId has been been set for the requested domain', () => { const { controller } = setup({ state: { - perDomainNetwork: false, domains: {}, }, }); @@ -414,38 +416,11 @@ describe('SelectedNetworkController', () => { expect(() => { controller.getProviderAndBlockTracker('test.com'); }).toThrow( - 'Provider and BlockTracker should be fetched from NetworkController when perDomainNetwork is false', + 'Provider and BlockTracker should be fetched from NetworkController when useRequestQueue is false', ); }); }); }); - - describe('setPerDomainNetwork', () => { - describe('when toggling from false to true', () => { - it('should update perDomainNetwork state to true', () => { - const { controller } = setup({ - state: { - perDomainNetwork: false, - domains: {}, - }, - }); - controller.setPerDomainNetwork(true); - expect(controller.state.perDomainNetwork).toBe(true); - }); - }); - describe('when toggling from true to false', () => { - it('should update perDomainNetwork state to false', () => { - const { controller } = setup({ - state: { - perDomainNetwork: true, - domains: {}, - }, - }); - controller.setPerDomainNetwork(false); - expect(controller.state.perDomainNetwork).toBe(false); - }); - }); - }); describe('When a permission is added or removed', () => { it('should add new domain to domains list on permission add', async () => { const { controller, messenger } = setup(); @@ -470,7 +445,7 @@ describe('SelectedNetworkController', () => { it('should remove domain from domains list on permission removal', async () => { const { controller, messenger } = setup({ - state: { perDomainNetwork: true, domains: { 'example.com': 'foo' } }, + state: { domains: { 'example.com': 'foo' } }, }); messenger.publish('PermissionController:stateChange', { subjects: {} }, [ @@ -488,7 +463,7 @@ describe('SelectedNetworkController', () => { it('should set networkClientId for domains not already in state', async () => { const getSubjectNamesMock = ['newdomain.com']; const { controller } = setup({ - state: { perDomainNetwork: true, domains: {} }, + state: { domains: {} }, getSubjectNames: getSubjectNamesMock, }); @@ -499,7 +474,6 @@ describe('SelectedNetworkController', () => { it('should not modify domains already in state', async () => { const { controller } = setup({ state: { - perDomainNetwork: true, domains: { 'existingdomain.com': 'initialNetworkId', },