Skip to content

Commit

Permalink
remove redundant perDomainNetwork state (#3989)
Browse files Browse the repository at this point in the history
## Explanation

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?
* Are there any changes whose purpose might not obvious to those
unfamiliar with the domain?
* If your primary goal was to update one package but you found you had
to update another one along the way, why did you do so?
* If you had to upgrade a dependency, why did you do so?
-->

See
[here](https://app.zenhub.com/workspaces/wallet-api-platform-63bee08a4e3b9d001108416e/issues/gh/metamask/metamask-planning/2143)
for more details


## References

<!--
Are there any issues that this pull request is tied to? Are there other
links that reviewers should consult to understand these changes better?

For example:

* Fixes #12345
* Related to #67890
-->

## 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/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
  • Loading branch information
BelfordZ authored Feb 27, 2024
1 parent f7a0c13 commit 17c52d9
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 72 deletions.
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;
});
}
}
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

0 comments on commit 17c52d9

Please sign in to comment.