From 0c5bd6db6923e6de027c32c97df012cc8996561a Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 7 Nov 2024 06:33:08 +0900 Subject: [PATCH] address PR comments --- .../SnapInterfaceController.test.tsx | 129 +++++++++--------- .../src/interface/SnapInterfaceController.ts | 48 ++----- 2 files changed, 73 insertions(+), 104 deletions(-) diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.test.tsx b/packages/snaps-controllers/src/interface/SnapInterfaceController.test.tsx index 921860e5f2..5a6c693a5c 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.test.tsx +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.test.tsx @@ -70,8 +70,6 @@ describe('SnapInterfaceController', () => { }, }, }); - - controller.destroy(); }); describe('constructor', () => { @@ -105,8 +103,6 @@ describe('SnapInterfaceController', () => { }, }, }); - - controller.destroy(); }); }); @@ -116,7 +112,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -153,7 +150,6 @@ describe('SnapInterfaceController', () => { expect(content).toStrictEqual(getJsxElementFromComponent(components)); expect(state).toStrictEqual({ foo: { bar: null } }); - controller.destroy(); }); it('can create a new interface from JSX', async () => { @@ -161,7 +157,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -203,7 +200,6 @@ describe('SnapInterfaceController', () => { expect(content).toStrictEqual(element); expect(state).toStrictEqual({ foo: { bar: null } }); - controller.destroy(); }); it('supports providing interface context', async () => { @@ -211,7 +207,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -238,7 +235,6 @@ describe('SnapInterfaceController', () => { expect(content).toStrictEqual(element); expect(context).toStrictEqual({ foo: 'bar' }); - controller.destroy(); }); it('supports providing an interface content type', async () => { @@ -246,7 +242,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -273,7 +270,6 @@ describe('SnapInterfaceController', () => { ); expect(contentType).toStrictEqual(ContentType.Notification); - controller.destroy(); }); it('throws if interface context is too large', async () => { @@ -286,7 +282,8 @@ describe('SnapInterfaceController', () => { .mockReturnValueOnce(1) .mockReturnValueOnce(10_000_000); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -306,7 +303,6 @@ describe('SnapInterfaceController', () => { { foo: 'a'.repeat(1_000_000) }, ), ).rejects.toThrow('A Snap interface context may not be larger than 1 MB'); - controller.destroy(); }); it('throws if a link is on the phishing list', async () => { @@ -326,7 +322,8 @@ describe('SnapInterfaceController', () => { () => ({ result: true, type: 'all' }), ); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -356,8 +353,6 @@ describe('SnapInterfaceController', () => { 'PhishingController:testOrigin', 'https://foo.bar/', ); - - controller.destroy(); }); it('throws if a JSX link is on the phishing list', async () => { @@ -377,7 +372,8 @@ describe('SnapInterfaceController', () => { () => ({ result: true, type: 'all' }), ); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -407,7 +403,6 @@ describe('SnapInterfaceController', () => { 'PhishingController:testOrigin', 'https://foo.bar/', ); - controller.destroy(); }); it('throws if UI content is too large', async () => { @@ -419,7 +414,8 @@ describe('SnapInterfaceController', () => { jest.mocked(getJsonSizeUnsafe).mockReturnValueOnce(11_000_000); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -432,7 +428,6 @@ describe('SnapInterfaceController', () => { components, ), ).rejects.toThrow('A Snap UI may not be larger than 10 MB.'); - controller.destroy(); }); it('throws if JSX UI content is too large', async () => { @@ -444,7 +439,8 @@ describe('SnapInterfaceController', () => { jest.mocked(getJsonSizeUnsafe).mockReturnValueOnce(11_000_000); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -461,7 +457,6 @@ describe('SnapInterfaceController', () => { element, ), ).rejects.toThrow('A Snap UI may not be larger than 10 MB.'); - controller.destroy(); }); it('throws if text content is too large', async () => { @@ -471,7 +466,8 @@ describe('SnapInterfaceController', () => { false, ); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -484,7 +480,6 @@ describe('SnapInterfaceController', () => { components, ), ).rejects.toThrow('The text in a Snap UI may not be larger than 50 kB.'); - controller.destroy(); }); }); @@ -494,7 +489,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -516,7 +512,6 @@ describe('SnapInterfaceController', () => { ); expect(content).toStrictEqual(getJsxElementFromComponent(components)); - controller.destroy(); }); it('throws if the snap requesting the interface is not the one that created it', async () => { @@ -524,7 +519,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -546,7 +542,6 @@ describe('SnapInterfaceController', () => { id, ), ).toThrow(`Interface not created by foo.`); - controller.destroy(); }); it('throws if the interface does not exist', () => { @@ -554,7 +549,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -565,7 +561,6 @@ describe('SnapInterfaceController', () => { 'test', ), ).toThrow(`Interface with id 'test' not found.`); - controller.destroy(); }); }); @@ -575,7 +570,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -610,7 +606,6 @@ describe('SnapInterfaceController', () => { expect(content).toStrictEqual(getJsxElementFromComponent(newContent)); expect(state).toStrictEqual({ foo: { baz: null } }); - controller.destroy(); }); it('can update an interface using JSX', async () => { @@ -618,7 +613,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -659,7 +655,6 @@ describe('SnapInterfaceController', () => { expect(content).toStrictEqual(newElement); expect(state).toStrictEqual({ foo: { baz: null } }); - controller.destroy(); }); it('can update an interface and context', async () => { @@ -667,7 +662,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -713,7 +709,6 @@ describe('SnapInterfaceController', () => { expect(content).toStrictEqual(getJsxElementFromComponent(newContent)); expect(state).toStrictEqual({ foo: { baz: null } }); expect(interfaceContext).toStrictEqual(newContext); - controller.destroy(); }); it('does not replace context if none is provided', async () => { @@ -721,7 +716,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -764,7 +760,6 @@ describe('SnapInterfaceController', () => { expect(content).toStrictEqual(getJsxElementFromComponent(newContent)); expect(state).toStrictEqual({ foo: { baz: null } }); expect(interfaceContext).toStrictEqual(context); - controller.destroy(); }); it('throws if a link is on the phishing list', async () => { @@ -784,7 +779,8 @@ describe('SnapInterfaceController', () => { () => ({ result: true, type: 'all' }), ); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -826,7 +822,6 @@ describe('SnapInterfaceController', () => { 'PhishingController:testOrigin', 'https://foo.bar/', ); - controller.destroy(); }); it('throws if a JSX link is on the phishing list', async () => { @@ -846,7 +841,8 @@ describe('SnapInterfaceController', () => { () => ({ result: true, type: 'all' }), ); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -892,7 +888,6 @@ describe('SnapInterfaceController', () => { 'PhishingController:testOrigin', 'https://foo.bar/', ); - controller.destroy(); }); it('throws if UI content is too large', async () => { @@ -905,7 +900,8 @@ describe('SnapInterfaceController', () => { .mockReturnValueOnce(1) .mockReturnValueOnce(11_000_000); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -930,7 +926,6 @@ describe('SnapInterfaceController', () => { newContent, ), ).rejects.toThrow('A Snap UI may not be larger than 10 MB.'); - controller.destroy(); }); it('throws if JSX UI content is too large', async () => { @@ -943,7 +938,8 @@ describe('SnapInterfaceController', () => { .mockReturnValueOnce(1) .mockReturnValueOnce(11_000_000); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -975,7 +971,6 @@ describe('SnapInterfaceController', () => { newElement, ), ).rejects.toThrow('A Snap UI may not be larger than 10 MB.'); - controller.destroy(); }); it('throws if text content is too large', async () => { @@ -983,7 +978,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -1008,7 +1004,6 @@ describe('SnapInterfaceController', () => { newContent, ), ).rejects.toThrow('The text in a Snap UI may not be larger than 50 kB.'); - controller.destroy(); }); it('throws if the interface does not exist', async () => { @@ -1016,7 +1011,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -1030,7 +1026,6 @@ describe('SnapInterfaceController', () => { content, ), ).rejects.toThrow("Interface with id 'foo' not found."); - controller.destroy(); }); it('throws if the interface is updated by another snap', async () => { @@ -1038,7 +1033,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -1063,7 +1059,6 @@ describe('SnapInterfaceController', () => { newContent, ), ).rejects.toThrow('Interface not created by foo.'); - controller.destroy(); }); }); @@ -1073,7 +1068,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -1100,7 +1096,6 @@ describe('SnapInterfaceController', () => { ); expect(state).toStrictEqual(newState); - controller.destroy(); }); it('updates the interface state with a file', async () => { @@ -1108,7 +1103,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -1150,7 +1146,6 @@ describe('SnapInterfaceController', () => { ); expect(state).toStrictEqual(newState); - controller.destroy(); }); }); @@ -1160,7 +1155,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -1181,7 +1177,6 @@ describe('SnapInterfaceController', () => { id, ), ).toThrow(`Interface with id '${id}' not found.`); - controller.destroy(); }); }); @@ -1191,7 +1186,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -1227,7 +1223,6 @@ describe('SnapInterfaceController', () => { ); expect(await approvalPromise).toBe('bar'); - controller.destroy(); }); it('throws if the interface does not exist', async () => { @@ -1235,7 +1230,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -1259,7 +1255,6 @@ describe('SnapInterfaceController', () => { 'bar', ), ).rejects.toThrow(`Interface with id 'foo' not found.`); - controller.destroy(); }); it('throws if the interface is resolved by another snap', async () => { @@ -1267,7 +1262,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -1304,7 +1300,6 @@ describe('SnapInterfaceController', () => { 'bar', ), ).rejects.toThrow('Interface not created by baz.'); - controller.destroy(); }); it('throws if the interface has no approval request', async () => { @@ -1312,7 +1307,8 @@ describe('SnapInterfaceController', () => { const controllerMessenger = getRestrictedSnapInterfaceControllerMessenger(rootMessenger); - const controller = new SnapInterfaceController({ + // eslint-disable-next-line no-new + new SnapInterfaceController({ messenger: controllerMessenger, }); @@ -1344,7 +1340,6 @@ describe('SnapInterfaceController', () => { 'bar', ), ).rejects.toThrow(`Approval request with id '${id}' not found.`); - controller.destroy(); }); }); }); diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts index 3c6ce61059..3cc50bd317 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts @@ -95,7 +95,7 @@ export type SnapInterfaceControllerStateChangeEvent = >; type NotificationListUpdatedEvent = { - type: `${'NotificationServicesController'}:notificationsListUpdated`; + type: 'NotificationServicesController:notificationsListUpdated'; payload: [Record[]]; }; @@ -128,8 +128,6 @@ export type SnapInterfaceControllerArgs = { state?: SnapInterfaceControllerState; }; -const subscriptions = new WeakMap(); - /** * Use this controller to manage snaps UI interfaces using RPC method hooks. */ @@ -163,12 +161,9 @@ export class SnapInterfaceController extends BaseController< state: { interfaces: {}, ...state }, }); - /* eslint-disable @typescript-eslint/unbound-method */ - subscriptions.set(this, this.#_onNotificationsListUpdated.bind(this)); - this.messagingSystem.subscribe( 'NotificationServicesController:notificationsListUpdated', - subscriptions.get(this), + this.#onNotificationsListUpdated.bind(this), ); this.#registerMessageHandlers(); @@ -430,7 +425,7 @@ export class SnapInterfaceController extends BaseController< ); } - #_onNotificationsListUpdated(notificationsList: Record[]) { + #onNotificationsListUpdated(notificationsList: Record[]) { const snapNotificationsWithInterface = notificationsList.filter( (notification) => { return notification.type === 'snap' && notification.data?.detailedView; @@ -443,36 +438,15 @@ export class SnapInterfaceController extends BaseController< ), ); - const updatedState = Object.entries(this.state.interfaces).reduce< - Record - >((newState, [id, snapInterface]) => { - if (snapInterface.contentType === ContentType.Notification) { - if (interfaceIdSet.has(id)) { - newState[id] = snapInterface; - } - } else { - newState[id] = snapInterface; - } - return newState; - }, {}); - this.update((state) => { - // @ts-expect-error - TS2589: Type instantiation is excessively deep and - // possibly infinite. - state.interfaces = updatedState; + Object.entries(state.interfaces).forEach(([id, snapInterface]) => { + if ( + snapInterface.contentType === ContentType.Notification && + !interfaceIdSet.has(id) + ) { + delete state.interfaces[id]; + } + }); }); } - - /** - * Run controller teardown process and unsubscribe from notification service controller events. - */ - destroy() { - super.destroy(); - - /* eslint-disable @typescript-eslint/unbound-method */ - this.messagingSystem.unsubscribe( - 'NotificationServicesController:notificationsListUpdated', - subscriptions.get(this), - ); - } }