From 7a1af89c4f9c05b0f5088b606306522c2ff6cf69 Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Fri, 27 Aug 2021 14:26:22 -0700 Subject: [PATCH] Migrate ApprovalController to BaseControllerV2 (#555) This PR migrates the `ApprovalController` to `BaseControllerV2`. Its functionality is exactly the same as before, with the exception of some name changes for clarity, and the addition of controller messenger actions. The method `resolve` has been renamed to `accept`, which is a breaking change. --- src/approval/ApprovalController.test.js | 38 ++- src/approval/ApprovalController.test.ts | 153 +++++++++-- src/approval/ApprovalController.ts | 321 +++++++++++++++++------- 3 files changed, 379 insertions(+), 133 deletions(-) diff --git a/src/approval/ApprovalController.test.js b/src/approval/ApprovalController.test.js index eccfc80488b..a88ecb95e8f 100644 --- a/src/approval/ApprovalController.test.js +++ b/src/approval/ApprovalController.test.js @@ -1,27 +1,24 @@ const { errorCodes } = require('eth-rpc-errors'); +const { ControllerMessenger } = require('../ControllerMessenger'); const { ApprovalController } = require('./ApprovalController'); -const defaultConfig = { - showApprovalRequest: () => undefined, -}; +function getRestrictedMessenger() { + const controllerMessenger = new ControllerMessenger(); + const messenger = controllerMessenger.getRestricted({ + name: 'ApprovalController', + }); + return messenger; +} const getApprovalController = () => - new ApprovalController({ ...defaultConfig }); + new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest: () => undefined, + }); const STORE_KEY = 'pendingApprovals'; describe('ApprovalController: Input Validation', () => { - describe('constructor', () => { - it('throws on invalid input', () => { - expect(() => new ApprovalController({})).toThrow( - getInvalidShowApprovalRequestError(), - ); - expect( - () => new ApprovalController({ showApprovalRequest: 'bar' }), - ).toThrow(getInvalidShowApprovalRequestError()); - }); - }); - describe('add', () => { it('validates input', () => { const approvalController = getApprovalController(); @@ -109,6 +106,9 @@ describe('ApprovalController: Input Validation', () => { expect(() => approvalController.has({ type: true })).toThrow( getInvalidHasTypeError(), ); + expect(() => + approvalController.has({ origin: 'foo', type: true }), + ).toThrow(getInvalidHasTypeError()); }); }); @@ -118,7 +118,7 @@ describe('ApprovalController: Input Validation', () => { let approvalController; beforeEach(() => { - approvalController = new ApprovalController({ ...defaultConfig }); + approvalController = getApprovalController(); }); it('deletes entry', () => { @@ -162,10 +162,6 @@ describe('ApprovalController: Input Validation', () => { // helpers -function getInvalidShowApprovalRequestError() { - return getError('Must specify function showApprovalRequest.'); -} - function getInvalidIdError() { return getError('Must specify non-empty string id.', errorCodes.rpc.internal); } @@ -201,7 +197,7 @@ function getInvalidTypeError(code) { } function getInvalidHasParamsError() { - return getError('Must specify non-empty string id, origin, or type.'); + return getError('Must specify a valid combination of id, origin, and type.'); } function getApprovalCountParamsError() { diff --git a/src/approval/ApprovalController.test.ts b/src/approval/ApprovalController.test.ts index f0fb97a3232..fb5fcc55f8e 100644 --- a/src/approval/ApprovalController.test.ts +++ b/src/approval/ApprovalController.test.ts @@ -1,14 +1,33 @@ import { errorCodes } from 'eth-rpc-errors'; import sinon from 'sinon'; -import { ApprovalController } from './ApprovalController'; +import { ControllerMessenger } from '../ControllerMessenger'; +import { + ApprovalController, + ApprovalControllerActions, + ApprovalControllerEvents, + ApprovalControllerMessenger, +} from './ApprovalController'; const STORE_KEY = 'pendingApprovals'; const TYPE = 'TYPE'; -const defaultConfig = { - showApprovalRequest: () => undefined, -}; +const controllerName = 'ApprovalController'; + +function getRestrictedMessenger() { + const controllerMessenger = new ControllerMessenger< + ApprovalControllerActions, + ApprovalControllerEvents + >(); + const messenger = controllerMessenger.getRestricted< + typeof controllerName, + never, + never + >({ + name: 'ApprovalController', + }); + return messenger; +} describe('approval controller', () => { const clock = sinon.useFakeTimers(1); @@ -16,9 +35,14 @@ describe('approval controller', () => { describe('add', () => { let approvalController: ApprovalController; + let showApprovalRequest: sinon.SinonSpy; beforeEach(() => { - approvalController = new ApprovalController({ ...defaultConfig }); + showApprovalRequest = sinon.spy(); + approvalController = new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest, + }); }); it('adds correctly specified entry', () => { @@ -31,7 +55,13 @@ describe('approval controller', () => { approvalController.has({ origin: 'bar.baz', type: TYPE }), ).toStrictEqual(true); expect(approvalController.state[STORE_KEY]).toStrictEqual({ - foo: { id: 'foo', origin: 'bar.baz', time: 1, type: TYPE }, + foo: { + id: 'foo', + origin: 'bar.baz', + requestData: null, + time: 1, + type: TYPE, + }, }); }); @@ -121,7 +151,7 @@ describe('approval controller', () => { it('addAndShowApprovalRequest', () => { const showApprovalSpy = sinon.spy(); const approvalController = new ApprovalController({ - ...defaultConfig, + messenger: getRestrictedMessenger(), showApprovalRequest: showApprovalSpy, }); @@ -138,11 +168,15 @@ describe('approval controller', () => { describe('get', () => { it('gets entry', () => { - const approvalController = new ApprovalController({ ...defaultConfig }); + 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', origin: 'bar.baz', + requestData: null, type: 'myType', time: 1, }); @@ -154,7 +188,10 @@ describe('approval controller', () => { let addWithCatch: (args: any) => void; beforeEach(() => { - approvalController = new ApprovalController({ ...defaultConfig }); + approvalController = new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest: sinon.spy(), + }); addWithCatch = (args: any) => approvalController.add(args).catch(() => undefined); }); @@ -252,7 +289,10 @@ describe('approval controller', () => { describe('getTotalApprovalCount', () => { it('gets the total approval count', () => { - const approvalController = new ApprovalController({ ...defaultConfig }); + const approvalController = new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest: sinon.spy(), + }); expect(approvalController.getTotalApprovalCount()).toStrictEqual(0); const addWithCatch = (args: any) => @@ -279,7 +319,10 @@ describe('approval controller', () => { let approvalController: ApprovalController; beforeEach(() => { - approvalController = new ApprovalController({ ...defaultConfig }); + approvalController = new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest: sinon.spy(), + }); }); it('returns true for existing entry by id', () => { @@ -351,7 +394,10 @@ describe('approval controller', () => { let deleteSpy: sinon.SinonSpy; beforeEach(() => { - approvalController = new ApprovalController({ ...defaultConfig }); + approvalController = new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest: sinon.spy(), + }); // TODO: Stop using private methods in tests deleteSpy = sinon.spy(approvalController as any, '_delete'); numDeletions = 0; @@ -365,7 +411,7 @@ describe('approval controller', () => { origin: 'bar.baz', type: 'myType', }); - approvalController.resolve('foo', 'success'); + approvalController.accept('foo', 'success'); const result = await approvalPromise; expect(result).toStrictEqual('success'); @@ -386,12 +432,12 @@ describe('approval controller', () => { type: 'myType2', }); - approvalController.resolve('foo2', 'success2'); + approvalController.accept('foo2', 'success2'); let result = await approvalPromise2; expect(result).toStrictEqual('success2'); - approvalController.resolve('foo1', 'success1'); + approvalController.accept('foo1', 'success1'); result = await approvalPromise1; expect(result).toStrictEqual('success1'); @@ -399,7 +445,7 @@ describe('approval controller', () => { }); it('throws on unknown id', () => { - expect(() => approvalController.resolve('foo')).toThrow( + expect(() => approvalController.accept('foo')).toThrow( getIdNotFoundError('foo'), ); expect(deleteSpy.callCount).toStrictEqual(numDeletions); @@ -412,7 +458,10 @@ describe('approval controller', () => { let deleteSpy: sinon.SinonSpy; beforeEach(() => { - approvalController = new ApprovalController({ ...defaultConfig }); + approvalController = new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest: sinon.spy(), + }); // TODO: Stop using private methods in tests deleteSpy = sinon.spy(approvalController as any, '_delete'); numDeletions = 0; @@ -459,9 +508,12 @@ describe('approval controller', () => { }); }); - describe('resolve and reject', () => { - it('resolves and rejects multiple approval promises out of order', async () => { - const approvalController = new ApprovalController({ ...defaultConfig }); + 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', @@ -484,7 +536,7 @@ describe('approval controller', () => { type: 'myType4', }); - approvalController.resolve('foo2', 'success2'); + approvalController.accept('foo2', 'success2'); let result = await promise2; expect(result).toStrictEqual('success2'); @@ -500,7 +552,7 @@ describe('approval controller', () => { ); expect(approvalController.has({ origin: 'bar.baz' })).toStrictEqual(true); - approvalController.resolve('foo1', 'success1'); + approvalController.accept('foo1', 'success1'); result = await promise1; expect(result).toStrictEqual('success1'); @@ -515,7 +567,10 @@ describe('approval controller', () => { let approvalController: ApprovalController; beforeEach(() => { - approvalController = new ApprovalController({ ...defaultConfig }); + approvalController = new ApprovalController({ + messenger: getRestrictedMessenger(), + showApprovalRequest: sinon.spy(), + }); }); it('does nothing if state is already empty', () => { @@ -538,13 +593,61 @@ describe('approval controller', () => { expect(rejectSpy.callCount).toStrictEqual(2); }); }); + + describe('actions', () => { + it('addApprovalRequest: shouldShowRequest = true', async () => { + const messenger = new ControllerMessenger< + ApprovalControllerActions, + ApprovalControllerEvents + >(); + const showApprovalSpy = sinon.spy(); + + const approvalController = new ApprovalController({ + messenger: messenger.getRestricted({ + name: controllerName, + }) as ApprovalControllerMessenger, + showApprovalRequest: showApprovalSpy, + }); + + messenger.call( + 'ApprovalController:addRequest', + { id: 'foo', origin: 'bar.baz', type: TYPE }, + true, + ); + expect(showApprovalSpy.calledOnce).toStrictEqual(true); + expect(approvalController.has({ id: 'foo' })).toStrictEqual(true); + }); + + it('addApprovalRequest: shouldShowRequest = false', async () => { + const messenger = new ControllerMessenger< + ApprovalControllerActions, + ApprovalControllerEvents + >(); + const showApprovalSpy = sinon.spy(); + + const approvalController = new ApprovalController({ + messenger: messenger.getRestricted({ + name: controllerName, + }) as ApprovalControllerMessenger, + showApprovalRequest: showApprovalSpy, + }); + + messenger.call( + 'ApprovalController:addRequest', + { id: 'foo', origin: 'bar.baz', type: TYPE }, + false, + ); + expect(showApprovalSpy.notCalled).toStrictEqual(true); + expect(approvalController.has({ id: 'foo' })).toStrictEqual(true); + }); + }); }); // helpers function getIdCollisionError(id: string) { return getError( - `Approval with id '${id}' already exists.`, + `Approval request with id '${id}' already exists.`, errorCodes.rpc.internal, ); } @@ -555,7 +658,7 @@ function getOriginTypeCollisionError(origin: string, type = TYPE) { } function getIdNotFoundError(id: string) { - return getError(`Approval with id '${id}' not found.`); + return getError(`Approval request with id '${id}' not found.`); } function getError(message: string, code?: number) { diff --git a/src/approval/ApprovalController.ts b/src/approval/ApprovalController.ts index 88ed1af6169..a35ecfdf132 100644 --- a/src/approval/ApprovalController.ts +++ b/src/approval/ApprovalController.ts @@ -1,21 +1,23 @@ -import { nanoid } from 'nanoid'; +import type { Patch } from 'immer'; import { ethErrors } from 'eth-rpc-errors'; -import { BaseController, BaseConfig, BaseState } from '../BaseController'; +import { nanoid } from 'nanoid'; + +import { BaseController, Json } from '../BaseControllerV2'; +import type { RestrictedControllerMessenger } from '../ControllerMessenger'; -const APPROVALS_STORE_KEY = 'pendingApprovals'; -const APPROVAL_COUNT_STORE_KEY = 'pendingApprovalCount'; +const controllerName = 'ApprovalController'; type ApprovalPromiseResolve = (value?: unknown) => void; type ApprovalPromiseReject = (error?: Error) => void; -type RequestData = Record; +type ApprovalRequestData = Record | null; -interface ApprovalCallbacks { +type ApprovalCallbacks = { resolve: ApprovalPromiseResolve; reject: ApprovalPromiseReject; -} +}; -export interface Approval { +export type ApprovalRequest = { /** * The ID of the approval request. */ @@ -38,25 +40,100 @@ export interface Approval { /** * Additional data associated with the request. + * TODO:TS4.4 make optional */ - requestData?: RequestData; -} + requestData: RequestData; +}; -export interface ApprovalConfig extends BaseConfig { - showApprovalRequest: () => void; -} +type ShowApprovalRequest = () => void | Promise; -export interface ApprovalState extends BaseState { - [APPROVALS_STORE_KEY]: { [approvalId: string]: Approval }; - [APPROVAL_COUNT_STORE_KEY]: number; -} +export type ApprovalControllerState = { + pendingApprovals: Record>>; + pendingApprovalCount: number; +}; + +const stateMetadata = { + pendingApprovals: { persist: false, anonymous: true }, + pendingApprovalCount: { persist: false, anonymous: false }, +}; const getAlreadyPendingMessage = (origin: string, type: string) => `Request of type '${type}' already pending for origin ${origin}. Please wait.`; -const defaultState: ApprovalState = { - [APPROVALS_STORE_KEY]: {}, - [APPROVAL_COUNT_STORE_KEY]: 0, +const getDefaultState = (): ApprovalControllerState => { + return { + pendingApprovals: {}, + pendingApprovalCount: 0, + }; +}; + +export type GetApprovalsState = { + type: `${typeof controllerName}:getState`; + handler: () => ApprovalControllerState; +}; + +export type ClearApprovalRequests = { + type: `${typeof controllerName}:clearRequests`; + handler: () => void; +}; + +type AddApprovalOptions = { + id?: string; + origin: string; + type: string; + requestData?: Record; +}; + +export type AddApprovalRequest = { + type: `${typeof controllerName}:addRequest`; + handler: ( + opts: AddApprovalOptions, + shouldShowRequest: boolean, + ) => ReturnType; +}; + +export type HasApprovalRequest = { + type: `${typeof controllerName}:hasRequest`; + handler: ApprovalController['has']; +}; + +export type AcceptRequest = { + type: `${typeof controllerName}:acceptRequest`; + handler: ApprovalController['accept']; +}; + +export type RejectRequest = { + type: `${typeof controllerName}:rejectRequest`; + handler: ApprovalController['reject']; +}; + +export type ApprovalControllerActions = + | GetApprovalsState + | ClearApprovalRequests + | AddApprovalRequest + | HasApprovalRequest + | AcceptRequest + | RejectRequest; + +export type ApprovalStateChange = { + type: `${typeof controllerName}:stateChange`; + payload: [ApprovalControllerState, Patch[]]; +}; + +export type ApprovalControllerEvents = ApprovalStateChange; + +export type ApprovalControllerMessenger = RestrictedControllerMessenger< + typeof controllerName, + ApprovalControllerActions, + ApprovalControllerEvents, + never, + never +>; + +type ApprovalControllerOptions = { + messenger: ApprovalControllerMessenger; + showApprovalRequest: ShowApprovalRequest; + state?: Partial; }; /** @@ -69,8 +146,9 @@ const defaultState: ApprovalState = { * is approved or denied, respectively. */ export class ApprovalController extends BaseController< - ApprovalConfig, - ApprovalState + typeof controllerName, + ApprovalControllerState, + ApprovalControllerMessenger > { private _approvals: Map; @@ -83,18 +161,58 @@ export class ApprovalController extends BaseController< * @param opts.showApprovalRequest - Function for opening the UI such that * the request can be displayed to the user. */ - constructor(config: ApprovalConfig, state?: ApprovalState) { - const { showApprovalRequest } = config; - if (typeof showApprovalRequest !== 'function') { - throw new Error('Must specify function showApprovalRequest.'); - } - - super(config, state || defaultState); + constructor({ + messenger, + showApprovalRequest, + state = {}, + }: ApprovalControllerOptions) { + super({ + name: controllerName, + metadata: stateMetadata, + messenger, + state: { ...getDefaultState(), ...state }, + }); this._approvals = new Map(); this._origins = new Map(); this._showApprovalRequest = showApprovalRequest; - this.initialize(); + this.registerMessageHandlers(); + } + + /** + * Constructor helper for registering this controller's messaging system + * actions. + */ + private registerMessageHandlers(): void { + this.messagingSystem.registerActionHandler( + `${controllerName}:clearRequests` as const, + this.clear.bind(this), + ); + + this.messagingSystem.registerActionHandler( + `${controllerName}:addRequest` as const, + (opts: AddApprovalOptions, shouldShowRequest: boolean) => { + if (shouldShowRequest) { + return this.addAndShowApprovalRequest(opts); + } + return this.add(opts); + }, + ); + + this.messagingSystem.registerActionHandler( + `${controllerName}:hasRequest` as const, + this.has.bind(this), + ); + + this.messagingSystem.registerActionHandler( + `${controllerName}:acceptRequest` as const, + this.accept.bind(this), + ); + + this.messagingSystem.registerActionHandler( + `${controllerName}:rejectRequest` as const, + this.reject.bind(this), + ); } /** @@ -113,12 +231,7 @@ export class ApprovalController extends BaseController< * if any. * @returns The approval promise. */ - addAndShowApprovalRequest(opts: { - id?: string; - origin: string; - type: string; - requestData?: RequestData; - }): Promise { + addAndShowApprovalRequest(opts: AddApprovalOptions): Promise { const promise = this._add( opts.origin, opts.type, @@ -145,12 +258,7 @@ export class ApprovalController extends BaseController< * if any. * @returns The approval promise. */ - add(opts: { - id?: string; - origin: string; - type: string; - requestData?: RequestData; - }): Promise { + add(opts: AddApprovalOptions): Promise { return this._add(opts.origin, opts.type, opts.id, opts.requestData); } @@ -160,9 +268,8 @@ export class ApprovalController extends BaseController< * @param id - The id of the approval request. * @returns The approval request data associated with the id. */ - get(id: string): Approval | undefined { - const info = this.state[APPROVALS_STORE_KEY][id]; - return info ? { ...info } : undefined; + get(id: string): ApprovalRequest | undefined { + return this.state.pendingApprovals[id]; } /** @@ -195,7 +302,7 @@ export class ApprovalController extends BaseController< // Only "type" was specified let count = 0; - for (const approval of Object.values(this.state[APPROVALS_STORE_KEY])) { + for (const approval of Object.values(this.state.pendingApprovals)) { if (approval.type === _type) { count += 1; } @@ -208,9 +315,50 @@ export class ApprovalController extends BaseController< * origins. */ getTotalApprovalCount(): number { - return this.state[APPROVAL_COUNT_STORE_KEY]; + return this.state.pendingApprovalCount; } + // These signatures create a meaningful difference when this method is called. + /* eslint-disable @typescript-eslint/unified-signatures */ + /** + * Checks if there's a pending approval request with the given ID. + * + * @param opts - Options bag. + * @param opts.id - The ID to check for. + * @returns `true` if a matching approval is found, and `false` otherwise. + */ + has(opts: { id: string }): boolean; + + /** + * Checks if there's any pending approval request with the given origin. + * + * @param opts - Options bag. + * @param opts.origin - The origin to check for. + * @returns `true` if a matching approval is found, and `false` otherwise. + */ + has(opts: { origin: string }): boolean; + + /** + * Checks if there's any pending approval request with the given type. + * + * @param opts - Options bag. + * @param opts.type - The type to check for. + * @returns `true` if a matching approval is found, and `false` otherwise. + */ + has(opts: { type: string }): boolean; + + /** + * Checks if there's any pending approval request with the given origin and + * type. + * + * @param opts - Options bag. + * @param opts.origin - The origin to check for. + * @param opts.type - The type to check for. + * @returns `true` if a matching approval is found, and `false` otherwise. + */ + has(opts: { origin: string; type: string }): boolean; + /* eslint-enable @typescript-eslint/unified-signatures */ + /** * Checks if there's a pending approval request per the given parameters. * At least one parameter must be specified. An error will be thrown if the @@ -236,31 +384,33 @@ export class ApprovalController extends BaseController< return this._approvals.has(id); } + if (_type && typeof _type !== 'string') { + throw new Error('May not specify non-string type.'); + } + if (origin) { if (typeof origin !== 'string') { throw new Error('May not specify non-string origin.'); } // Check origin and type pair if type also specified - if (_type && typeof _type === 'string') { + if (_type) { return Boolean(this._origins.get(origin)?.has(_type)); } return this._origins.has(origin); } if (_type) { - if (typeof _type !== 'string') { - throw new Error('May not specify non-string type.'); - } - - for (const approval of Object.values(this.state[APPROVALS_STORE_KEY])) { + for (const approval of Object.values(this.state.pendingApprovals)) { if (approval.type === _type) { return true; } } return false; } - throw new Error('Must specify non-empty string id, origin, or type.'); + throw new Error( + 'Must specify a valid combination of id, origin, and type.', + ); } /** @@ -270,7 +420,7 @@ export class ApprovalController extends BaseController< * @param id - The id of the approval request. * @param value - The value to resolve the approval promise with. */ - resolve(id: string, value?: unknown): void { + accept(id: string, value?: unknown): void { this._deleteApprovalAndGetCallbacks(id).resolve(value); } @@ -297,7 +447,7 @@ export class ApprovalController extends BaseController< this.reject(id, rejectionError); } this._origins.clear(); - this.update(defaultState, true); + this.update(() => getDefaultState()); } /** @@ -313,7 +463,7 @@ export class ApprovalController extends BaseController< origin: string, type: string, id: string = nanoid(), - requestData?: RequestData, + requestData?: Record, ): Promise { this._validateAddParams(id, origin, type, requestData); @@ -343,13 +493,13 @@ export class ApprovalController extends BaseController< id: string, origin: string, type: string, - requestData?: RequestData, + requestData?: Record, ): void { let errorMessage = null; if (!id || typeof id !== 'string') { errorMessage = 'Must specify non-empty string id.'; } else if (this._approvals.has(id)) { - errorMessage = `Approval with id '${id}' already exists.`; + errorMessage = `Approval request with id '${id}' already exists.`; } else if (!origin || typeof origin !== 'string') { errorMessage = 'Must specify non-empty string origin.'; } else if (!type || typeof type !== 'string') { @@ -395,25 +545,23 @@ export class ApprovalController extends BaseController< id: string, origin: string, type: string, - requestData?: RequestData, + requestData?: Record, ): void { - const approval: Approval = { id, origin, type, time: Date.now() }; - if (requestData) { - approval.requestData = requestData; - } - - const approvals = { - ...this.state[APPROVALS_STORE_KEY], - [id]: approval, + const approval: ApprovalRequest | null> = { + id, + origin, + type, + time: Date.now(), + requestData: requestData || null, }; - this.update( - { - [APPROVALS_STORE_KEY]: approvals, - [APPROVAL_COUNT_STORE_KEY]: Object.keys(approvals).length, - }, - true, - ); + this.update((draftState) => { + // Typecast: ts(2589) + draftState.pendingApprovals[id] = approval as any; + draftState.pendingApprovalCount = Object.keys( + draftState.pendingApprovals, + ).length; + }); } /** @@ -427,23 +575,22 @@ export class ApprovalController extends BaseController< private _delete(id: string): void { this._approvals.delete(id); - const approvals = this.state[APPROVALS_STORE_KEY]; - const { origin, type } = approvals[id]; + // This method is only called after verifying that the approval with the + // specified id exists. + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const { origin, type } = this.state.pendingApprovals[id]!; (this._origins.get(origin) as Set).delete(type); if (this._isEmptyOrigin(origin)) { this._origins.delete(origin); } - const newApprovals = { ...approvals }; - delete newApprovals[id]; - this.update( - { - [APPROVALS_STORE_KEY]: newApprovals, - [APPROVAL_COUNT_STORE_KEY]: Object.keys(newApprovals).length, - }, - true, - ); + this.update((draftState) => { + delete draftState.pendingApprovals[id]; + draftState.pendingApprovalCount = Object.keys( + draftState.pendingApprovals, + ).length; + }); } /** @@ -457,7 +604,7 @@ export class ApprovalController extends BaseController< private _deleteApprovalAndGetCallbacks(id: string): ApprovalCallbacks { const callbacks = this._approvals.get(id); if (!callbacks) { - throw new Error(`Approval with id '${id}' not found.`); + throw new Error(`Approval request with id '${id}' not found.`); } this._delete(id);