From 56dbf6426fb70b7ffd94e375779e4f7c9c5147b9 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Fri, 10 Jan 2025 14:04:36 +0100 Subject: [PATCH 1/7] Add accepted and rejected events to ApprovalController --- .../src/ApprovalController.ts | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/packages/approval-controller/src/ApprovalController.ts b/packages/approval-controller/src/ApprovalController.ts index 6ea4dd0b631..3f69c38bb37 100644 --- a/packages/approval-controller/src/ApprovalController.ts +++ b/packages/approval-controller/src/ApprovalController.ts @@ -258,7 +258,10 @@ export type ApprovalStateChange = ControllerStateChangeEvent< ApprovalControllerState >; -export type ApprovalControllerEvents = ApprovalStateChange; +export type ApprovalControllerEvents = + | ApprovalApprovedEvent + | ApprovalRejectedEvent + | ApprovalStateChange; // Action Types @@ -325,6 +328,25 @@ export type ShowError = { handler: ApprovalController['error']; }; +export type ApprovalRejectedEvent = { + type: `${typeof controllerName}:rejected`; + payload: [ + { + approval: ApprovalRequest; + error: Error; + }, + ]; +}; + +export type ApprovalApprovedEvent = { + type: `${typeof controllerName}:accepted`; + payload: [ + { + approval: ApprovalRequest; + }, + ]; +}; + export type ApprovalControllerActions = | GetApprovalsState | ClearApprovalRequests @@ -729,6 +751,9 @@ export class ApprovalController extends BaseController< if (!requestDeleted) { this.#delete(id); } + this.messagingSystem.publish(`${controllerName}:accepted`, { + approval, + }); }); } @@ -740,9 +765,16 @@ export class ApprovalController extends BaseController< * @param error - The error to reject the approval promise with. */ reject(id: string, error: unknown): void { + const rejectedApproval = this.get( + id, + ) as ApprovalRequest; const callbacks = this.#getCallbacks(id); this.#delete(id); callbacks.reject(error); + this.messagingSystem.publish(`${controllerName}:rejected`, { + approval: rejectedApproval, + error: error as Error, + }); } /** From ae144301423307341419ece953d75a9323dc62f9 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Fri, 10 Jan 2025 14:05:50 +0100 Subject: [PATCH 2/7] Rename --- packages/approval-controller/src/ApprovalController.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/approval-controller/src/ApprovalController.ts b/packages/approval-controller/src/ApprovalController.ts index 3f69c38bb37..888fde6ee31 100644 --- a/packages/approval-controller/src/ApprovalController.ts +++ b/packages/approval-controller/src/ApprovalController.ts @@ -259,7 +259,7 @@ export type ApprovalStateChange = ControllerStateChangeEvent< >; export type ApprovalControllerEvents = - | ApprovalApprovedEvent + | ApprovalAcceptedEvent | ApprovalRejectedEvent | ApprovalStateChange; @@ -338,7 +338,7 @@ export type ApprovalRejectedEvent = { ]; }; -export type ApprovalApprovedEvent = { +export type ApprovalAcceptedEvent = { type: `${typeof controllerName}:accepted`; payload: [ { From e0611faf4d85f11a64a4b1527fdcf4ac58d667be Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Mon, 13 Jan 2025 13:30:29 +0100 Subject: [PATCH 3/7] Add unit tests for new added events --- .../src/ApprovalController.test.ts | 34 +++++++++++++++++-- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/packages/approval-controller/src/ApprovalController.test.ts b/packages/approval-controller/src/ApprovalController.test.ts index 1664571a21b..9dcb3510ce1 100644 --- a/packages/approval-controller/src/ApprovalController.test.ts +++ b/packages/approval-controller/src/ApprovalController.test.ts @@ -9,6 +9,7 @@ import type { AddApprovalOptions, ApprovalControllerActions, ApprovalControllerEvents, + ApprovalControllerMessenger, ErrorOptions, StartFlowOptions, SuccessOptions, @@ -243,15 +244,17 @@ function getRestrictedMessenger() { describe('approval controller', () => { let approvalController: ApprovalController; let showApprovalRequest: jest.Mock; + let messenger: ApprovalControllerMessenger; beforeEach(() => { nanoidMock.mockReturnValue('TestId'); jest.spyOn(global.console, 'info').mockImplementation(() => undefined); + messenger = getRestrictedMessenger(); showApprovalRequest = jest.fn(); approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), + messenger, showApprovalRequest, }); }); @@ -815,6 +818,9 @@ describe('approval controller', () => { describe('accept', () => { it('resolves approval promise', async () => { + const acceptedEvent = jest.fn(); + messenger.subscribe('ApprovalController:accepted', acceptedEvent); + const approvalPromise = approvalController.add({ id: 'foo', origin: 'bar.baz', @@ -826,6 +832,15 @@ describe('approval controller', () => { const result = await approvalPromise; expect(result).toBe('success'); + + expect(acceptedEvent.mock.calls[0][0]).toEqual({ + approval: expect.objectContaining({ + id: 'foo', + origin: 'bar.baz', + type: 'myType', + time: expect.any(Number), + }), + }); }); it('resolves multiple approval promises out of order', async () => { @@ -1067,13 +1082,26 @@ describe('approval controller', () => { describe('reject', () => { it('rejects approval promise', async () => { + const rejectedEvent = jest.fn(); + messenger.subscribe('ApprovalController:rejected', rejectedEvent); + + const rejectedError = new Error('failure'); const approvalPromise = approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE, }); - approvalController.reject('foo', new Error('failure')); - await expect(approvalPromise).rejects.toThrow('failure'); + approvalController.reject('foo', rejectedError); + await expect(approvalPromise).rejects.toThrow(rejectedError); + + expect(rejectedEvent.mock.calls[0][0]).toEqual({ + approval: expect.objectContaining({ + id: 'foo', + origin: 'bar.baz', + type: TYPE, + }), + error: rejectedError, + }); }); it('rejects multiple approval promises out of order', async () => { From 445749e5e85aa38ed7208659d937a384a35840b7 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Thu, 16 Jan 2025 08:25:02 +0100 Subject: [PATCH 4/7] Fix suggestions --- .../src/ApprovalController.test.ts | 55 ++++++++++++++----- .../src/ApprovalController.ts | 8 +-- 2 files changed, 45 insertions(+), 18 deletions(-) diff --git a/packages/approval-controller/src/ApprovalController.test.ts b/packages/approval-controller/src/ApprovalController.test.ts index 9dcb3510ce1..9518deec3e4 100644 --- a/packages/approval-controller/src/ApprovalController.test.ts +++ b/packages/approval-controller/src/ApprovalController.test.ts @@ -826,15 +826,29 @@ describe('approval controller', () => { origin: 'bar.baz', type: 'myType', }); - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/no-floating-promises + approvalController.accept('foo', 'success'); const result = await approvalPromise; expect(result).toBe('success'); + }); + + it('emits "accepted" event', async () => { + const acceptedEvent = jest.fn(); + messenger.subscribe('ApprovalController:accepted', acceptedEvent); + + const approvalPromise = approvalController.add({ + id: 'foo', + origin: 'bar.baz', + type: 'myType', + }); + + approvalController.accept('foo', 'success'); + + await approvalPromise; expect(acceptedEvent.mock.calls[0][0]).toEqual({ - approval: expect.objectContaining({ + request: expect.objectContaining({ id: 'foo', origin: 'bar.baz', type: 'myType', @@ -855,15 +869,11 @@ describe('approval controller', () => { type: 'myType2', }); - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.accept('foo2', 'success2'); let result = await approvalPromise2; expect(result).toBe('success2'); - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.accept('foo1', 'success1'); result = await approvalPromise1; @@ -1093,15 +1103,32 @@ describe('approval controller', () => { }); approvalController.reject('foo', rejectedError); await expect(approvalPromise).rejects.toThrow(rejectedError); + }); - expect(rejectedEvent.mock.calls[0][0]).toEqual({ - approval: expect.objectContaining({ - id: 'foo', - origin: 'bar.baz', - type: TYPE, - }), - error: rejectedError, + it('emits "rejected" event', async () => { + const rejectedEvent = jest.fn(); + messenger.subscribe('ApprovalController:rejected', rejectedEvent); + + const rejectedError = new Error('failure'); + const approvalPromise = approvalController.add({ + id: 'foo', + origin: 'bar.baz', + type: TYPE, }); + approvalController.reject('foo', rejectedError); + + try { + await approvalPromise; + } catch (error) { + expect(rejectedEvent.mock.calls[0][0]).toEqual({ + request: expect.objectContaining({ + id: 'foo', + origin: 'bar.baz', + type: TYPE, + }), + error: rejectedError, + }); + } }); it('rejects multiple approval promises out of order', async () => { diff --git a/packages/approval-controller/src/ApprovalController.ts b/packages/approval-controller/src/ApprovalController.ts index 888fde6ee31..5c1e5498a04 100644 --- a/packages/approval-controller/src/ApprovalController.ts +++ b/packages/approval-controller/src/ApprovalController.ts @@ -332,7 +332,7 @@ export type ApprovalRejectedEvent = { type: `${typeof controllerName}:rejected`; payload: [ { - approval: ApprovalRequest; + request: ApprovalRequest; error: Error; }, ]; @@ -342,7 +342,7 @@ export type ApprovalAcceptedEvent = { type: `${typeof controllerName}:accepted`; payload: [ { - approval: ApprovalRequest; + request: ApprovalRequest; }, ]; }; @@ -752,7 +752,7 @@ export class ApprovalController extends BaseController< this.#delete(id); } this.messagingSystem.publish(`${controllerName}:accepted`, { - approval, + request: approval, }); }); } @@ -772,7 +772,7 @@ export class ApprovalController extends BaseController< this.#delete(id); callbacks.reject(error); this.messagingSystem.publish(`${controllerName}:rejected`, { - approval: rejectedApproval, + request: rejectedApproval, error: error as Error, }); } From b0a038e220e811a8925e2fe7a435f3f1d1af5fa6 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Thu, 16 Jan 2025 08:34:39 +0100 Subject: [PATCH 5/7] Fix suggestions --- .../src/ApprovalController.test.ts | 54 ++++++++----------- 1 file changed, 23 insertions(+), 31 deletions(-) diff --git a/packages/approval-controller/src/ApprovalController.test.ts b/packages/approval-controller/src/ApprovalController.test.ts index 9518deec3e4..8b4c2b48406 100644 --- a/packages/approval-controller/src/ApprovalController.test.ts +++ b/packages/approval-controller/src/ApprovalController.test.ts @@ -244,17 +244,17 @@ function getRestrictedMessenger() { describe('approval controller', () => { let approvalController: ApprovalController; let showApprovalRequest: jest.Mock; - let messenger: ApprovalControllerMessenger; + let controllerMessenger: ApprovalControllerMessenger; beforeEach(() => { nanoidMock.mockReturnValue('TestId'); jest.spyOn(global.console, 'info').mockImplementation(() => undefined); - messenger = getRestrictedMessenger(); + controllerMessenger = getRestrictedMessenger(); showApprovalRequest = jest.fn(); approvalController = new ApprovalController({ - messenger, + messenger: controllerMessenger, showApprovalRequest, }); }); @@ -780,16 +780,12 @@ describe('approval controller', () => { }); it('returns false for non-existing entry by origin', () => { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE }); expect(approvalController.has({ origin: 'fizz.buzz' })).toBe(false); }); it('returns false for non-existing entry by existing origin and non-existing type', () => { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE }); expect( @@ -798,8 +794,6 @@ describe('approval controller', () => { }); it('returns false for non-existing entry by non-existing origin and existing type', () => { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' }); expect( @@ -808,8 +802,6 @@ describe('approval controller', () => { }); it('returns false for non-existing entry by type', () => { - // TODO: Either fix this lint violation or explain why it's necessary to ignore. - // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType1' }); expect(approvalController.has({ type: 'myType2' })).toBe(false); @@ -818,15 +810,14 @@ describe('approval controller', () => { describe('accept', () => { it('resolves approval promise', async () => { - const acceptedEvent = jest.fn(); - messenger.subscribe('ApprovalController:accepted', acceptedEvent); - const approvalPromise = approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType', }); + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.accept('foo', 'success'); const result = await approvalPromise; @@ -835,7 +826,7 @@ describe('approval controller', () => { it('emits "accepted" event', async () => { const acceptedEvent = jest.fn(); - messenger.subscribe('ApprovalController:accepted', acceptedEvent); + controllerMessenger.subscribe('ApprovalController:accepted', acceptedEvent); const approvalPromise = approvalController.add({ id: 'foo', @@ -843,6 +834,8 @@ describe('approval controller', () => { type: 'myType', }); + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.accept('foo', 'success'); await approvalPromise; @@ -869,11 +862,15 @@ describe('approval controller', () => { type: 'myType2', }); + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.accept('foo2', 'success2'); let result = await approvalPromise2; expect(result).toBe('success2'); + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.accept('foo1', 'success1'); result = await approvalPromise1; @@ -1092,9 +1089,6 @@ describe('approval controller', () => { describe('reject', () => { it('rejects approval promise', async () => { - const rejectedEvent = jest.fn(); - messenger.subscribe('ApprovalController:rejected', rejectedEvent); - const rejectedError = new Error('failure'); const approvalPromise = approvalController.add({ id: 'foo', @@ -1107,7 +1101,7 @@ describe('approval controller', () => { it('emits "rejected" event', async () => { const rejectedEvent = jest.fn(); - messenger.subscribe('ApprovalController:rejected', rejectedEvent); + controllerMessenger.subscribe('ApprovalController:rejected', rejectedEvent); const rejectedError = new Error('failure'); const approvalPromise = approvalController.add({ @@ -1117,18 +1111,16 @@ describe('approval controller', () => { }); approvalController.reject('foo', rejectedError); - try { - await approvalPromise; - } catch (error) { - expect(rejectedEvent.mock.calls[0][0]).toEqual({ - request: expect.objectContaining({ - id: 'foo', - origin: 'bar.baz', - type: TYPE, - }), - error: rejectedError, - }); - } + await expect(approvalPromise).rejects.toThrow(rejectedError); + + expect(rejectedEvent.mock.calls[0][0]).toEqual({ + request: expect.objectContaining({ + id: 'foo', + origin: 'bar.baz', + type: TYPE, + }), + error: rejectedError, + }); }); it('rejects multiple approval promises out of order', async () => { From 421bc4453f5eab7bdf974307cda63707d7f1ff20 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Thu, 16 Jan 2025 08:35:40 +0100 Subject: [PATCH 6/7] Remove unnecessary new line --- packages/approval-controller/src/ApprovalController.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/approval-controller/src/ApprovalController.test.ts b/packages/approval-controller/src/ApprovalController.test.ts index 8b4c2b48406..d45fe121fc4 100644 --- a/packages/approval-controller/src/ApprovalController.test.ts +++ b/packages/approval-controller/src/ApprovalController.test.ts @@ -815,7 +815,6 @@ describe('approval controller', () => { origin: 'bar.baz', type: 'myType', }); - // TODO: Either fix this lint violation or explain why it's necessary to ignore. // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.accept('foo', 'success'); From 6bedf45f6c6a8aacdd1e1de8fb668bc2441e1e27 Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Thu, 16 Jan 2025 08:43:57 +0100 Subject: [PATCH 7/7] Fix lint --- .../approval-controller/src/ApprovalController.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/approval-controller/src/ApprovalController.test.ts b/packages/approval-controller/src/ApprovalController.test.ts index d45fe121fc4..8ee6fcd0e54 100644 --- a/packages/approval-controller/src/ApprovalController.test.ts +++ b/packages/approval-controller/src/ApprovalController.test.ts @@ -780,12 +780,16 @@ describe('approval controller', () => { }); it('returns false for non-existing entry by origin', () => { + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE }); expect(approvalController.has({ origin: 'fizz.buzz' })).toBe(false); }); it('returns false for non-existing entry by existing origin and non-existing type', () => { + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE }); expect( @@ -794,6 +798,8 @@ describe('approval controller', () => { }); it('returns false for non-existing entry by non-existing origin and existing type', () => { + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' }); expect( @@ -802,6 +808,8 @@ describe('approval controller', () => { }); it('returns false for non-existing entry by type', () => { + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/no-floating-promises approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType1' }); expect(approvalController.has({ type: 'myType2' })).toBe(false);