Skip to content

Commit

Permalink
fix: add SelectedNetworkController setNetworkClientId useRequestQue…
Browse files Browse the repository at this point in the history 
…uePreference guard (#4388)

## Explanation

Our previous SelectedNetworkController fix that [added a guard to
permission state changes](#4368)
was not sufficient. This PR properly addresses the underlying issue of
setting networkClientId for domains when the `useRequestQueuePreference`
flag is false by moving the guard into the `setNetworkClientId()` method
itself

## References

* Fixes: MetaMask/metamask-extension#25097

## 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`

- **FIXED**: `setNetworkClientId()` will now result in a noop if
`useRequestQueuePreference` is false


## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: Alex Donesky <[email protected]>
  • Loading branch information
@jiexi @adonesky1
jiexi and adonesky1 authored Jun 10, 2024
1 parent e1a71e5 commit 1900a9d
Show file tree
Hide file tree
Showing 2 changed files with 158 additions and 76 deletions.
10 changes: 5 additions & 5 deletions packages/selected-network-controller/src/SelectedNetworkController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,7 @@ export class SelectedNetworkController extends BaseController<
path[0] === 'subjects' && path[1] !== undefined;
if (isChangingSubject && typeof path[1] === 'string') {
const domain = path[1];
if (
op === 'add' &&
this.state.domains[domain] === undefined &&
this.#useRequestQueuePreference
) {
if (op === 'add' && this.state.domains[domain] === undefined) {
this.setNetworkClientIdForDomain(
domain,
this.messagingSystem.call('NetworkController:getState')
Expand Down Expand Up @@ -311,6 +307,10 @@ export class SelectedNetworkController extends BaseController<
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 Down
224 changes: 153 additions & 71 deletions packages/selected-network-controller/tests/SelectedNetworkController.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,36 +225,77 @@ describe('SelectedNetworkController', () => {
});
});

describe('It updates domain state when the network controller state changes', () => {
describe('when a networkClient is deleted from the network controller state', () => {
it('updates the networkClientId for domains which were previously set to the deleted networkClientId', () => {
const { controller, messenger } = setup({
state: {
domains: {
metamask: 'goerli',
'example.com': 'test-network-client-id',
'test.com': 'test-network-client-id',
describe('networkController:stateChange', () => {
describe('when useRequestQueuePreference is false', () => {
describe('when a networkClient is deleted from the network controller state', () => {
it('does not update the networkClientId for domains which were previously set to the deleted networkClientId', () => {
const { controller, messenger } = setup({
state: {
// normally there would not be any domains in state if useRequestQueuePreference is false
domains: {
metamask: 'goerli',
'example.com': 'test-network-client-id',
'test.com': 'test-network-client-id',
},
},
},
});

messenger.publish(
'NetworkController:stateChange',
{
providerConfig: { chainId: '0x5', ticker: 'ETH', type: 'goerli' },
selectedNetworkClientId: 'goerli',
networkConfigurations: {},
networksMetadata: {},
},
[
{
op: 'remove',
path: ['networkConfigurations', 'test-network-client-id'],
},
],
);
expect(controller.state.domains).toStrictEqual({
metamask: 'goerli',
'example.com': 'test-network-client-id',
'test.com': 'test-network-client-id',
});
});
});
});

messenger.publish(
'NetworkController:stateChange',
{
providerConfig: { chainId: '0x5', ticker: 'ETH', type: 'goerli' },
selectedNetworkClientId: 'goerli',
networkConfigurations: {},
networksMetadata: {},
},
[
describe('when useRequestQueuePreference is true', () => {
describe('when a networkClient is deleted from the network controller state', () => {
it('updates the networkClientId for domains which were previously set to the deleted networkClientId', () => {
const { controller, messenger } = setup({
state: {
domains: {
metamask: 'goerli',
'example.com': 'test-network-client-id',
'test.com': 'test-network-client-id',
},
},
useRequestQueuePreference: true,
});

messenger.publish(
'NetworkController:stateChange',
{
op: 'remove',
path: ['networkConfigurations', 'test-network-client-id'],
providerConfig: { chainId: '0x5', ticker: 'ETH', type: 'goerli' },
selectedNetworkClientId: 'goerli',
networkConfigurations: {},
networksMetadata: {},
},
],
);
expect(controller.state.domains['example.com']).toBe('goerli');
expect(controller.state.domains['test.com']).toBe('goerli');
[
{
op: 'remove',
path: ['networkConfigurations', 'test-network-client-id'],
},
],
);
expect(controller.state.domains['example.com']).toBe('goerli');
expect(controller.state.domains['test.com']).toBe('goerli');
});
});
});
});
Expand All @@ -263,42 +304,42 @@ describe('SelectedNetworkController', () => {
afterEach(() => {
jest.clearAllMocks();
});
it('should throw an error when passed "metamask" as domain arg', () => {
const { controller } = setup();
expect(() => {
controller.setNetworkClientIdForDomain('metamask', 'mainnet');
}).toThrow(
'NetworkClientId for domain "metamask" cannot be set on the SelectedNetworkController',
);
expect(controller.state.domains.metamask).toBeUndefined();
});

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: {
domains: {
'1.com': 'mainnet',
'2.com': 'mainnet',
'3.com': 'mainnet',
},
it('skips setting the networkClientId for the passed in domain', () => {
const { controller } = setup({
state: {
domains: {
'1.com': 'mainnet',
'2.com': 'mainnet',
'3.com': 'mainnet',
},
});
const domains = ['1.com', '2.com', '3.com'];
const networkClientIds = ['1', '2', '3'];
},
});
const domains = ['1.com', '2.com', '3.com'];
const networkClientIds = ['1', '2', '3'];

domains.forEach((domain, i) =>
controller.setNetworkClientIdForDomain(domain, networkClientIds[i]),
);
domains.forEach((domain, i) =>
controller.setNetworkClientIdForDomain(domain, networkClientIds[i]),
);

expect(controller.state.domains['1.com']).toBe('1');
expect(controller.state.domains['2.com']).toBe('2');
expect(controller.state.domains['3.com']).toBe('3');
expect(controller.state.domains).toStrictEqual({
'1.com': 'mainnet',
'2.com': 'mainnet',
'3.com': 'mainnet',
});
});
});

describe('when the useRequestQueue is true', () => {
it('should throw an error when passed "metamask" as domain arg', () => {
const { controller } = setup({ useRequestQueuePreference: true });
expect(() => {
controller.setNetworkClientIdForDomain('metamask', 'mainnet');
}).toThrow(
'NetworkClientId for domain "metamask" cannot be set on the SelectedNetworkController',
);
expect(controller.state.domains.metamask).toBeUndefined();
});
describe('when the requesting domain is a snap (starts with "npm:" or "local:"', () => {
it('skips setting the networkClientId for the passed in domain', () => {
const { controller, mockHasPermissions } = setup({
Expand Down Expand Up @@ -377,6 +418,7 @@ describe('SelectedNetworkController', () => {
it('throw an error and does not set the networkClientId for the passed in domain', () => {
const { controller, mockHasPermissions } = setup({
state: { domains: {} },
useRequestQueuePreference: true,
});
mockHasPermissions.mockReturnValue(false);

Expand Down Expand Up @@ -742,31 +784,71 @@ describe('SelectedNetworkController', () => {
});

describe('Constructor checks for domains in permissions', () => {
it('should set networkClientId for domains not already in state', async () => {
const getSubjectNamesMock = ['newdomain.com'];
const { controller } = setup({
state: { domains: {} },
getSubjectNames: getSubjectNamesMock,
describe('when useRequestQueuePreference is true', () => {
it('should set networkClientId for domains not already in state', async () => {
const { controller } = setup({
state: {
domains: {
'existingdomain.com': 'initialNetworkId',
},
},
getSubjectNames: ['newdomain.com'],
useRequestQueuePreference: true,
});

expect(controller.state.domains).toStrictEqual({
'newdomain.com': 'mainnet',
'existingdomain.com': 'initialNetworkId',
});
});

// Now, 'newdomain.com' should have the selectedNetworkClientId set
expect(controller.state.domains['newdomain.com']).toBe('mainnet');
it('should not modify domains already in state', async () => {
const { controller } = setup({
state: {
domains: {
'existingdomain.com': 'initialNetworkId',
},
},
getSubjectNames: ['existingdomain.com'],
useRequestQueuePreference: true,
});

expect(controller.state.domains).toStrictEqual({
'existingdomain.com': 'initialNetworkId',
});
});
});

it('should not modify domains already in state', async () => {
const { controller } = setup({
state: {
domains: {
'existingdomain.com': 'initialNetworkId',
describe('when useRequestQueuePreference is false', () => {
it('should not set networkClientId for new domains', async () => {
const { controller } = setup({
state: {
domains: {
'existingdomain.com': 'initialNetworkId',
},
},
},
getSubjectNames: ['existingdomain.com'],
getSubjectNames: ['newdomain.com'],
});

expect(controller.state.domains).toStrictEqual({
'existingdomain.com': 'initialNetworkId',
});
});

// The 'existingdomain.com' should retain its initial networkClientId
expect(controller.state.domains['existingdomain.com']).toBe(
'initialNetworkId',
);
it('should not modify domains already in state', async () => {
const { controller } = setup({
state: {
domains: {
'existingdomain.com': 'initialNetworkId',
},
},
getSubjectNames: ['existingdomain.com'],
});

expect(controller.state.domains).toStrictEqual({
'existingdomain.com': 'initialNetworkId',
});
});
});
});

Expand Down

0 comments on commit 1900a9d

Please sign in to comment.