Skip to content

remove redundant perDomainNetwork state #3989

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.

Sign up for GitHub

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

Merged
adonesky1 merged 2 commits into from
Feb 27, 2024
Merged

remove redundant perDomainNetwork state #3989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
745c9df
remove redundant perDomainNetwork state
BelfordZ Feb 27, 2024
a733642
lint fix
BelfordZ Feb 27, 2024
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
33 changes: 12 additions & 21 deletions packages/selected-network-controller/src/SelectedNetworkController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -46,12 +42,6 @@ export const SelectedNetworkControllerEventTypes = {

export type SelectedNetworkControllerState = {
domains: Record<Domain, NetworkClientId>;
/**
* 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 = {
Expand Down Expand Up @@ -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 = {
Expand All @@ -120,23 +113,28 @@ export class SelectedNetworkController extends BaseController<
> {
#proxies = new Map<Domain, NetworkProxy>();

#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,
metadata: stateMetadata,
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)
Expand Down Expand Up @@ -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;
Expand All @@ -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];
Expand All @@ -308,11 +306,4 @@ export class SelectedNetworkController extends BaseController<

return networkProxy;
}

setPerDomainNetwork(enabled: boolean) {
this.update((state) => {
state.perDomainNetwork = enabled;
return state;
});
}
}
76 changes: 25 additions & 51 deletions packages/selected-network-controller/tests/SelectedNetworkController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { createEventEmitterProxy } from '@metamask/swappable-obj-proxy';
import type {
AllowedActions,
AllowedEvents,
GetUseRequestQueue,
SelectedNetworkControllerActions,
SelectedNetworkControllerEvents,
SelectedNetworkControllerMessenger,
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -139,6 +142,7 @@ const setup = ({
const controller = new SelectedNetworkController({
messenger: selectedNetworkControllerMessenger,
state,
getUseRequestQueue,
});
return {
controller,
Expand All @@ -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,
});
});
});
Expand All @@ -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',
Expand Down Expand Up @@ -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',
Expand All @@ -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';
Expand All @@ -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';

Expand All @@ -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,
});

Expand All @@ -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(
Expand All @@ -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';
Expand All @@ -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',
Expand All @@ -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');
Expand All @@ -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();
Expand All @@ -392,60 +395,32 @@ 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(() => {
controller.getProviderAndBlockTracker('test.com');
}).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: {},
},
});

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();
Expand All @@ -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: {} }, [
Expand All @@ -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,
});

Expand All @@ -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',
},
Expand Down