From c92351b1c5d4c21e0d841a350dbfabb7b92040ae Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Fri, 26 May 2023 11:54:32 +0100 Subject: [PATCH 1/5] adds startFlow and enfFlow methods --- .../src/ApprovalController.test.ts | 100 ++++++++++++++++-- .../src/ApprovalController.ts | 77 +++++++++++++- packages/approval-controller/src/errors.ts | 16 +++ 3 files changed, 181 insertions(+), 12 deletions(-) diff --git a/packages/approval-controller/src/ApprovalController.test.ts b/packages/approval-controller/src/ApprovalController.test.ts index ae1ac7c6c6..b3536fc927 100644 --- a/packages/approval-controller/src/ApprovalController.test.ts +++ b/packages/approval-controller/src/ApprovalController.test.ts @@ -6,9 +6,12 @@ import { ApprovalControllerActions, ApprovalControllerEvents, ApprovalControllerMessenger, + ApprovalFlowOptions, } from './ApprovalController'; +import { EndInvalidFlowError, NoApprovalFlowsError } from './errors'; -const STORE_KEY = 'pendingApprovals'; +const PENDING_APPROVALS_STORE_KEY = 'pendingApprovals'; +const APPROVAL_FLOWS_STORE_KEY = 'approvalFlows'; const TYPE = 'TYPE'; @@ -113,7 +116,9 @@ describe('approval controller', () => { approvalController.has({ origin: 'bar.baz', type: TYPE }), ).toStrictEqual(true); - expect(approvalController.state[STORE_KEY]).toStrictEqual({ + expect( + approvalController.state[PENDING_APPROVALS_STORE_KEY], + ).toStrictEqual({ foo: { id: 'foo', origin: 'bar.baz', @@ -134,7 +139,9 @@ describe('approval controller', () => { }), ).not.toThrow(); - const id = Object.keys(approvalController.state[STORE_KEY])[0]; + const id = Object.keys( + approvalController.state[PENDING_APPROVALS_STORE_KEY], + )[0]; expect(id && typeof id === 'string').toStrictEqual(true); }); @@ -151,9 +158,9 @@ describe('approval controller', () => { expect(approvalController.has({ id: 'foo' })).toStrictEqual(true); expect(approvalController.has({ origin: 'bar.baz' })).toStrictEqual(true); expect(approvalController.has({ type: 'myType' })).toStrictEqual(true); - expect(approvalController.state[STORE_KEY].foo.requestData).toStrictEqual( - { foo: 'bar' }, - ); + expect( + approvalController.state[PENDING_APPROVALS_STORE_KEY].foo.requestData, + ).toStrictEqual({ foo: 'bar' }); }); it('adds correctly specified entry with request state', () => { @@ -170,7 +177,7 @@ describe('approval controller', () => { expect(approvalController.has({ origin: 'bar.baz' })).toStrictEqual(true); expect(approvalController.has({ type: 'myType' })).toStrictEqual(true); expect( - approvalController.state[STORE_KEY].foo.requestState, + approvalController.state[PENDING_APPROVALS_STORE_KEY].foo.requestState, ).toStrictEqual({ foo: 'bar' }); }); @@ -802,7 +809,9 @@ describe('approval controller', () => { approvalController.clear(new EthereumRpcError(1, 'clear')); - expect(approvalController.state[STORE_KEY]).toStrictEqual({}); + expect( + approvalController.state[PENDING_APPROVALS_STORE_KEY], + ).toStrictEqual({}); expect(rejectSpy.callCount).toStrictEqual(2); }); @@ -882,7 +891,7 @@ describe('approval controller', () => { !approvalController.has({ id: 'foo' }) && !approvalController.has({ type: 'type' }) && !approvalController.has({ origin: 'bar.baz' }) && - !approvalController.state[STORE_KEY].foo, + !approvalController.state[PENDING_APPROVALS_STORE_KEY].foo, ).toStrictEqual(true); }); @@ -981,6 +990,79 @@ describe('approval controller', () => { }); }); }); + + describe('startFlow', () => { + let approvalController: ApprovalController; + let showApprovalRequest: jest.Mock; + + beforeEach(() => { + showApprovalRequest = jest.fn(); + approvalController = new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest, + }); + }); + + it.each([ + ['no options passed', undefined], + ['partial options passed', {}], + ['options passed', { id: 'id' }], + ])( + 'adds flow to state and calls showApprovalRequest with %s', + (_, approvalFlowOptions?: ApprovalFlowOptions) => { + const result = approvalController.startFlow(approvalFlowOptions); + + const expectedFlow = { + id: approvalFlowOptions?.id ?? expect.any(String), + }; + expect(result).toStrictEqual(expectedFlow); + expect(showApprovalRequest).toHaveBeenCalledTimes(1); + expect(approvalController.state[APPROVAL_FLOWS_STORE_KEY]).toHaveLength( + 1, + ); + expect( + approvalController.state[APPROVAL_FLOWS_STORE_KEY][0], + ).toStrictEqual(expectedFlow); + }, + ); + }); + + describe('endFlow', () => { + let approvalController: ApprovalController; + let showApprovalRequest: jest.Mock; + + beforeEach(() => { + showApprovalRequest = jest.fn(); + approvalController = new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest, + }); + }); + + it('fails to end flow if no flow exists', () => { + expect(() => approvalController.endFlow('id')).toThrow( + NoApprovalFlowsError, + ); + }); + + it('fails to end flow if id does not correspond the current flow', () => { + approvalController.startFlow({ id: 'id' }); + + expect(() => approvalController.endFlow('wrong-id')).toThrow( + EndInvalidFlowError, + ); + }); + + it('ends flow if id corresponds with the current flow', () => { + approvalController.startFlow({ id: 'id' }); + + approvalController.endFlow('id'); + + expect(approvalController.state[APPROVAL_FLOWS_STORE_KEY]).toHaveLength( + 0, + ); + }); + }); }); // helpers diff --git a/packages/approval-controller/src/ApprovalController.ts b/packages/approval-controller/src/ApprovalController.ts index ac315cb044..343e879270 100644 --- a/packages/approval-controller/src/ApprovalController.ts +++ b/packages/approval-controller/src/ApprovalController.ts @@ -5,8 +5,12 @@ import { BaseControllerV2, RestrictedControllerMessenger, } from '@metamask/base-controller'; -import { Json } from '@metamask/utils'; -import { ApprovalRequestNotFoundError } from './errors'; +import { Json, OptionalField } from '@metamask/utils'; +import { + ApprovalRequestNotFoundError, + EndInvalidFlowError, + NoApprovalFlowsError, +} from './errors'; const controllerName = 'ApprovalController'; @@ -57,14 +61,22 @@ export type ApprovalRequest = { type ShowApprovalRequest = () => void | Promise; +type ApprovalFlow = { + id: string; +}; + +export type ApprovalFlowState = ApprovalFlow; + export type ApprovalControllerState = { pendingApprovals: Record>>; pendingApprovalCount: number; + approvalFlows: ApprovalFlowState[]; }; const stateMetadata = { pendingApprovals: { persist: false, anonymous: true }, pendingApprovalCount: { persist: false, anonymous: false }, + approvalFlows: { persist: false, anonymous: false }, }; const getAlreadyPendingMessage = (origin: string, type: string) => @@ -74,6 +86,7 @@ const getDefaultState = (): ApprovalControllerState => { return { pendingApprovals: {}, pendingApprovalCount: 0, + approvalFlows: [], }; }; @@ -128,6 +141,20 @@ export type UpdateRequestState = { handler: ApprovalController['updateRequestState']; }; +export type ApprovalFlowOptions = OptionalField; + +export type ApprovalFlowStartResult = ApprovalFlow; + +export type StartFlow = { + type: `${typeof controllerName}:startFlow`; + handler: ApprovalController['startFlow']; +}; + +export type EndFlow = { + type: `${typeof controllerName}:endFlow`; + handler: ApprovalController['endFlow']; +}; + export type ApprovalControllerActions = | GetApprovalsState | ClearApprovalRequests @@ -135,7 +162,9 @@ export type ApprovalControllerActions = | HasApprovalRequest | AcceptRequest | RejectRequest - | UpdateRequestState; + | UpdateRequestState + | StartFlow + | EndFlow; export type ApprovalStateChange = { type: `${typeof controllerName}:stateChange`; @@ -250,6 +279,16 @@ export class ApprovalController extends BaseControllerV2< `${controllerName}:updateRequestState` as const, this.updateRequestState.bind(this), ); + + this.messagingSystem.registerActionHandler( + `${controllerName}:startFlow` as const, + this.startFlow.bind(this), + ); + + this.messagingSystem.registerActionHandler( + `${controllerName}:endFlow` as const, + this.endFlow.bind(this), + ); } /** @@ -476,6 +515,38 @@ export class ApprovalController extends BaseControllerV2< }); } + startFlow(opts: ApprovalFlowOptions = {}): ApprovalFlowStartResult { + const id = opts.id ?? nanoid(); + const finalOptions = { id }; + + this.update((draftState) => { + draftState.approvalFlows.push(finalOptions); + }); + + this._showApprovalRequest(); + + return { id }; + } + + endFlow(flowId: string) { + if (!this.state.approvalFlows.length) { + throw new NoApprovalFlowsError(); + } + + const currentFlow = this.state.approvalFlows.slice(-1)[0]; + + if (flowId !== currentFlow.id) { + throw new EndInvalidFlowError( + flowId, + this.state.approvalFlows.map((flow) => flow.id), + ); + } + + this.update((draftState) => { + draftState.approvalFlows.pop(); + }); + } + /** * Implementation of add operation. * diff --git a/packages/approval-controller/src/errors.ts b/packages/approval-controller/src/errors.ts index 7389d351b1..d42f359d9f 100644 --- a/packages/approval-controller/src/errors.ts +++ b/packages/approval-controller/src/errors.ts @@ -3,3 +3,19 @@ export class ApprovalRequestNotFoundError extends Error { super(`Approval request with id '${id}' not found.`); } } + +export class NoApprovalFlowsError extends Error { + constructor() { + super(`No approval flows found.`); + } +} + +export class EndInvalidFlowError extends Error { + constructor(id: string, flowIds: string[]) { + super( + `Attempted to end flow with id '${id}' which does not match current flow with id '${ + flowIds.slice(-1)[0] + }'. All Flows: ${flowIds.join(', ')}`, + ); + } +} From b7050ac817a989b9381be1c814d3ffa87c0a693a Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Fri, 26 May 2023 12:11:55 +0100 Subject: [PATCH 2/5] remove sinon --- .../src/ApprovalController.test.ts | 177 ++++-------------- 1 file changed, 34 insertions(+), 143 deletions(-) diff --git a/packages/approval-controller/src/ApprovalController.test.ts b/packages/approval-controller/src/ApprovalController.test.ts index b3536fc927..4b79a37004 100644 --- a/packages/approval-controller/src/ApprovalController.test.ts +++ b/packages/approval-controller/src/ApprovalController.test.ts @@ -1,5 +1,4 @@ import { errorCodes, EthereumRpcError } from 'eth-rpc-errors'; -import * as sinon from 'sinon'; import { ControllerMessenger } from '@metamask/base-controller'; import { ApprovalController, @@ -38,26 +37,18 @@ function getRestrictedMessenger() { } describe('approval controller', () => { - beforeEach(() => { - sinon.useFakeTimers(1); - }); + let approvalController: ApprovalController; + let showApprovalRequest: jest.Mock; - afterEach(() => { - sinon.restore(); + beforeEach(() => { + showApprovalRequest = jest.fn(); + approvalController = new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest, + }); }); describe('add', () => { - let approvalController: ApprovalController; - let showApprovalRequest: sinon.SinonSpy; - - beforeEach(() => { - showApprovalRequest = sinon.spy(); - approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest, - }); - }); - it('validates input', () => { expect(() => approvalController.add({ id: null, origin: 'bar.baz' } as any), @@ -124,7 +115,7 @@ describe('approval controller', () => { origin: 'bar.baz', requestData: null, requestState: null, - time: 1, + time: expect.any(Number), type: TYPE, }, }); @@ -260,12 +251,6 @@ describe('approval controller', () => { // otherwise tested by 'add' above describe('addAndShowApprovalRequest', () => { it('addAndShowApprovalRequest', () => { - const showApprovalSpy = sinon.spy(); - const approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: showApprovalSpy, - }); - const result = approvalController.addAndShowApprovalRequest({ id: 'foo', origin: 'bar.baz', @@ -273,16 +258,12 @@ describe('approval controller', () => { requestData: { foo: 'bar' }, }); expect(result instanceof Promise).toStrictEqual(true); - expect(showApprovalSpy.calledOnce).toStrictEqual(true); + expect(showApprovalRequest).toHaveBeenCalledTimes(1); }); }); describe('get', () => { it('gets entry', () => { - const approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' }); expect(approvalController.get('foo')).toStrictEqual({ id: 'foo', @@ -290,16 +271,11 @@ describe('approval controller', () => { requestData: null, requestState: null, type: 'myType', - time: 1, + time: expect.any(Number), }); }); it('returns undefined for non-existing entry', () => { - const approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); - approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'type' }); expect(approvalController.get('fizz')).toBeUndefined(); @@ -311,15 +287,9 @@ describe('approval controller', () => { }); describe('getApprovalCount', () => { - let approvalController: ApprovalController; let addWithCatch: (args: any) => void; beforeEach(() => { - approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); - addWithCatch = (args: any) => approvalController.add(args).catch(() => undefined); }); @@ -441,7 +411,7 @@ describe('approval controller', () => { it('gets the count when specifying origin and type with type excluded from rate limiting', () => { approvalController = new ApprovalController({ messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), + showApprovalRequest, typesExcludedFromRateLimiting: [TYPE], }); @@ -456,10 +426,6 @@ describe('approval controller', () => { describe('getTotalApprovalCount', () => { it('gets the total approval count', () => { - const approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); expect(approvalController.getTotalApprovalCount()).toStrictEqual(0); const addWithCatch = (args: any) => @@ -482,9 +448,9 @@ describe('approval controller', () => { }); it('gets the total approval count with type excluded from rate limiting', () => { - const approvalController = new ApprovalController({ + approvalController = new ApprovalController({ messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), + showApprovalRequest, typesExcludedFromRateLimiting: ['type0'], }); expect(approvalController.getTotalApprovalCount()).toStrictEqual(0); @@ -507,15 +473,6 @@ describe('approval controller', () => { }); describe('has', () => { - let approvalController: ApprovalController; - - beforeEach(() => { - approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); - }); - it('validates input', () => { expect(() => approvalController.has()).toThrow( getInvalidHasParamsError(), @@ -606,17 +563,12 @@ describe('approval controller', () => { }); describe('resolve', () => { - let approvalController: ApprovalController; let numDeletions: number; - let deleteSpy: sinon.SinonSpy; + let deleteSpy: jest.SpyInstance; beforeEach(() => { - approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); // TODO: Stop using private methods in tests - deleteSpy = sinon.spy(approvalController as any, '_delete'); + deleteSpy = jest.spyOn(approvalController as any, '_delete'); numDeletions = 0; }); @@ -632,7 +584,7 @@ describe('approval controller', () => { const result = await approvalPromise; expect(result).toStrictEqual('success'); - expect(deleteSpy.callCount).toStrictEqual(numDeletions); + expect(deleteSpy).toHaveBeenCalledTimes(numDeletions); }); it('resolves multiple approval promises out of order', async () => { @@ -658,29 +610,24 @@ describe('approval controller', () => { result = await approvalPromise1; expect(result).toStrictEqual('success1'); - expect(deleteSpy.callCount).toStrictEqual(numDeletions); + expect(deleteSpy).toHaveBeenCalledTimes(numDeletions); }); it('throws on unknown id', () => { expect(() => approvalController.accept('foo')).toThrow( getIdNotFoundError('foo'), ); - expect(deleteSpy.callCount).toStrictEqual(numDeletions); + expect(deleteSpy).toHaveBeenCalledTimes(numDeletions); }); }); describe('reject', () => { - let approvalController: ApprovalController; let numDeletions: number; - let deleteSpy: sinon.SinonSpy; + let deleteSpy: jest.SpyInstance; beforeEach(() => { - approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); // TODO: Stop using private methods in tests - deleteSpy = sinon.spy(approvalController as any, '_delete'); + deleteSpy = jest.spyOn(approvalController as any, '_delete'); numDeletions = 0; }); @@ -693,7 +640,7 @@ describe('approval controller', () => { }); approvalController.reject('foo', new Error('failure')); await expect(approvalPromise).rejects.toThrow('failure'); - expect(deleteSpy.callCount).toStrictEqual(numDeletions); + expect(deleteSpy).toHaveBeenCalledTimes(numDeletions); }); it('rejects multiple approval promises out of order', async () => { @@ -714,24 +661,19 @@ describe('approval controller', () => { approvalController.reject('foo1', new Error('failure1')); await expect(rejectionPromise2).rejects.toThrow('failure2'); await expect(rejectionPromise1).rejects.toThrow('failure1'); - expect(deleteSpy.callCount).toStrictEqual(numDeletions); + expect(deleteSpy).toHaveBeenCalledTimes(numDeletions); }); it('throws on unknown id', () => { expect(() => approvalController.reject('foo', new Error('bar'))).toThrow( getIdNotFoundError('foo'), ); - expect(deleteSpy.callCount).toStrictEqual(numDeletions); + expect(deleteSpy).toHaveBeenCalledTimes(numDeletions); }); }); describe('accept and reject', () => { it('accepts and rejects multiple approval promises out of order', async () => { - const approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); - const promise1 = approvalController.add({ id: 'foo1', origin: 'bar.baz', @@ -781,15 +723,6 @@ describe('approval controller', () => { }); describe('clear', () => { - let approvalController: ApprovalController; - - beforeEach(() => { - approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); - }); - it('does nothing if state is already empty', () => { expect(() => approvalController.clear(new EthereumRpcError(1, 'clear')), @@ -797,7 +730,7 @@ describe('approval controller', () => { }); it('deletes existing entries', async () => { - const rejectSpy = sinon.spy(approvalController, 'reject'); + const rejectSpy = jest.spyOn(approvalController, 'reject'); approvalController .add({ id: 'foo2', origin: 'bar.baz', type: 'myType' }) @@ -812,7 +745,7 @@ describe('approval controller', () => { expect( approvalController.state[PENDING_APPROVALS_STORE_KEY], ).toStrictEqual({}); - expect(rejectSpy.callCount).toStrictEqual(2); + expect(rejectSpy).toHaveBeenCalledTimes(2); }); it('rejects existing entries with a caller-specified error', async () => { @@ -830,15 +763,6 @@ describe('approval controller', () => { }); describe('updateRequestState', () => { - let approvalController: ApprovalController; - - beforeEach(() => { - approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); - }); - it('updates the request state of a given approval request', () => { approvalController .add({ @@ -873,15 +797,6 @@ describe('approval controller', () => { // they are heavily dependent upon it. // TODO: Stop using private methods in tests describe('_delete', () => { - let approvalController: ApprovalController; - - beforeEach(() => { - approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest: sinon.spy(), - }); - }); - it('deletes entry', () => { approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'type' }); @@ -919,13 +834,12 @@ describe('approval controller', () => { ApprovalControllerActions, ApprovalControllerEvents >(); - const showApprovalSpy = sinon.spy(); - const approvalController = new ApprovalController({ + approvalController = new ApprovalController({ messenger: messenger.getRestricted({ name: controllerName, }) as ApprovalControllerMessenger, - showApprovalRequest: showApprovalSpy, + showApprovalRequest, }); messenger.call( @@ -933,7 +847,7 @@ describe('approval controller', () => { { id: 'foo', origin: 'bar.baz', type: TYPE }, true, ); - expect(showApprovalSpy.calledOnce).toStrictEqual(true); + expect(showApprovalRequest).toHaveBeenCalledTimes(1); expect(approvalController.has({ id: 'foo' })).toStrictEqual(true); }); @@ -942,13 +856,12 @@ describe('approval controller', () => { ApprovalControllerActions, ApprovalControllerEvents >(); - const showApprovalSpy = sinon.spy(); - const approvalController = new ApprovalController({ + approvalController = new ApprovalController({ messenger: messenger.getRestricted({ name: controllerName, }) as ApprovalControllerMessenger, - showApprovalRequest: showApprovalSpy, + showApprovalRequest, }); messenger.call( @@ -956,7 +869,7 @@ describe('approval controller', () => { { id: 'foo', origin: 'bar.baz', type: TYPE }, false, ); - expect(showApprovalSpy.notCalled).toStrictEqual(true); + expect(showApprovalRequest).toHaveBeenCalledTimes(0); expect(approvalController.has({ id: 'foo' })).toStrictEqual(true); }); @@ -966,11 +879,11 @@ describe('approval controller', () => { ApprovalControllerEvents >(); - const approvalController = new ApprovalController({ + approvalController = new ApprovalController({ messenger: messenger.getRestricted({ name: controllerName, }) as ApprovalControllerMessenger, - showApprovalRequest: sinon.spy(), + showApprovalRequest, }); approvalController.add({ @@ -992,17 +905,6 @@ describe('approval controller', () => { }); describe('startFlow', () => { - let approvalController: ApprovalController; - let showApprovalRequest: jest.Mock; - - beforeEach(() => { - showApprovalRequest = jest.fn(); - approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest, - }); - }); - it.each([ ['no options passed', undefined], ['partial options passed', {}], @@ -1028,17 +930,6 @@ describe('approval controller', () => { }); describe('endFlow', () => { - let approvalController: ApprovalController; - let showApprovalRequest: jest.Mock; - - beforeEach(() => { - showApprovalRequest = jest.fn(); - approvalController = new ApprovalController({ - messenger: getRestrictedMessenger(), - showApprovalRequest, - }); - }); - it('fails to end flow if no flow exists', () => { expect(() => approvalController.endFlow('id')).toThrow( NoApprovalFlowsError, From 4f206c156613a3913c8da361f072403ac22345e8 Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Fri, 26 May 2023 12:15:48 +0100 Subject: [PATCH 3/5] jsdoc --- .../approval-controller/src/ApprovalController.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/approval-controller/src/ApprovalController.ts b/packages/approval-controller/src/ApprovalController.ts index 343e879270..9a430bca73 100644 --- a/packages/approval-controller/src/ApprovalController.ts +++ b/packages/approval-controller/src/ApprovalController.ts @@ -515,6 +515,13 @@ export class ApprovalController extends BaseControllerV2< }); } + /** + * Starts a new approval flow. + * + * @param opts - Options bag. + * @param opts.id - The id of the approval flow. + * @returns The object containing the approval flow id. + */ startFlow(opts: ApprovalFlowOptions = {}): ApprovalFlowStartResult { const id = opts.id ?? nanoid(); const finalOptions = { id }; @@ -528,6 +535,11 @@ export class ApprovalController extends BaseControllerV2< return { id }; } + /** + * Ends the current approval flow. + * + * @param flowId - The id of the approval flow to end. + */ endFlow(flowId: string) { if (!this.state.approvalFlows.length) { throw new NoApprovalFlowsError(); From f399d4c2d3bdfc56117d7fc8ed531c8fa608652a Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Wed, 7 Jun 2023 13:08:05 +0100 Subject: [PATCH 4/5] prevent clear from removing approval flows --- .../approval-controller/src/ApprovalController.test.ts | 10 ++++++++++ packages/approval-controller/src/ApprovalController.ts | 5 ++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/approval-controller/src/ApprovalController.test.ts b/packages/approval-controller/src/ApprovalController.test.ts index f9bf9f67f9..96199855a8 100644 --- a/packages/approval-controller/src/ApprovalController.test.ts +++ b/packages/approval-controller/src/ApprovalController.test.ts @@ -857,6 +857,16 @@ describe('approval controller', () => { new EthereumRpcError(1000, 'foo'), ); }); + + it('does not clear approval flows', async () => { + approvalController.startFlow(); + + approvalController.clear(new EthereumRpcError(1, 'clear')); + + expect(approvalController.state[APPROVAL_FLOWS_STORE_KEY]).toHaveLength( + 1, + ); + }); }); describe('updateRequestState', () => { diff --git a/packages/approval-controller/src/ApprovalController.ts b/packages/approval-controller/src/ApprovalController.ts index b7ca9c74f4..b8d62aadbf 100644 --- a/packages/approval-controller/src/ApprovalController.ts +++ b/packages/approval-controller/src/ApprovalController.ts @@ -627,7 +627,10 @@ export class ApprovalController extends BaseControllerV2< this.reject(id, rejectionError); } this._origins.clear(); - this.update(() => getDefaultState()); + this.update((draftState) => { + draftState.pendingApprovals = {}; + draftState.pendingApprovalCount = 0; + }); } /** From f8117ef9c1ec680dd9bd5c218c8fff93a5eb67f1 Mon Sep 17 00:00:00 2001 From: Bernardo Garces Chapero Date: Fri, 9 Jun 2023 09:14:50 +0100 Subject: [PATCH 5/5] end flow options bag --- .../src/ApprovalController.test.ts | 10 +++++----- .../approval-controller/src/ApprovalController.ts | 15 +++++++++------ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/packages/approval-controller/src/ApprovalController.test.ts b/packages/approval-controller/src/ApprovalController.test.ts index 96199855a8..68a33be934 100644 --- a/packages/approval-controller/src/ApprovalController.test.ts +++ b/packages/approval-controller/src/ApprovalController.test.ts @@ -5,7 +5,7 @@ import { ApprovalControllerActions, ApprovalControllerEvents, ApprovalControllerMessenger, - ApprovalFlowOptions, + StartFlowOptions, } from './ApprovalController'; import { ApprovalRequestNoResultSupportError, @@ -1018,7 +1018,7 @@ describe('approval controller', () => { ['options passed', { id: 'id' }], ])( 'adds flow to state and calls showApprovalRequest with %s', - (_, approvalFlowOptions?: ApprovalFlowOptions) => { + (_, approvalFlowOptions?: StartFlowOptions) => { const result = approvalController.startFlow(approvalFlowOptions); const expectedFlow = { @@ -1038,7 +1038,7 @@ describe('approval controller', () => { describe('endFlow', () => { it('fails to end flow if no flow exists', () => { - expect(() => approvalController.endFlow('id')).toThrow( + expect(() => approvalController.endFlow({ id: 'id' })).toThrow( NoApprovalFlowsError, ); }); @@ -1046,7 +1046,7 @@ describe('approval controller', () => { it('fails to end flow if id does not correspond the current flow', () => { approvalController.startFlow({ id: 'id' }); - expect(() => approvalController.endFlow('wrong-id')).toThrow( + expect(() => approvalController.endFlow({ id: 'wrong-id' })).toThrow( EndInvalidFlowError, ); }); @@ -1054,7 +1054,7 @@ describe('approval controller', () => { it('ends flow if id corresponds with the current flow', () => { approvalController.startFlow({ id: 'id' }); - approvalController.endFlow('id'); + approvalController.endFlow({ id: 'id' }); expect(approvalController.state[APPROVAL_FLOWS_STORE_KEY]).toHaveLength( 0, diff --git a/packages/approval-controller/src/ApprovalController.ts b/packages/approval-controller/src/ApprovalController.ts index b8d62aadbf..e234f4726d 100644 --- a/packages/approval-controller/src/ApprovalController.ts +++ b/packages/approval-controller/src/ApprovalController.ts @@ -194,10 +194,12 @@ export type AddResult = { resultCallbacks?: AcceptResultCallbacks; }; -export type ApprovalFlowOptions = OptionalField; +export type StartFlowOptions = OptionalField; export type ApprovalFlowStartResult = ApprovalFlow; +export type EndFlowOptions = Pick; + export type StartFlow = { type: `${typeof controllerName}:startFlow`; handler: ApprovalController['startFlow']; @@ -659,7 +661,7 @@ export class ApprovalController extends BaseControllerV2< * @param opts.id - The id of the approval flow. * @returns The object containing the approval flow id. */ - startFlow(opts: ApprovalFlowOptions = {}): ApprovalFlowStartResult { + startFlow(opts: StartFlowOptions = {}): ApprovalFlowStartResult { const id = opts.id ?? nanoid(); const finalOptions = { id }; @@ -675,18 +677,19 @@ export class ApprovalController extends BaseControllerV2< /** * Ends the current approval flow. * - * @param flowId - The id of the approval flow to end. + * @param opts - Options bag. + * @param opts.id - The id of the approval flow that will be finished. */ - endFlow(flowId: string) { + endFlow({ id }: EndFlowOptions) { if (!this.state.approvalFlows.length) { throw new NoApprovalFlowsError(); } const currentFlow = this.state.approvalFlows.slice(-1)[0]; - if (flowId !== currentFlow.id) { + if (id !== currentFlow.id) { throw new EndInvalidFlowError( - flowId, + id, this.state.approvalFlows.map((flow) => flow.id), ); }