From 73aea59662f1c2f323969411f2de48bf7383202d Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Thu, 17 Dec 2020 00:06:47 -0800 Subject: [PATCH 1/3] Remove default approval controller type --- src/approval/ApprovalController.ts | 16 +--- tests/ApprovalController.test.js | 27 +++---- tests/ApprovalController.test.ts | 125 +++++++++-------------------- 3 files changed, 52 insertions(+), 116 deletions(-) diff --git a/src/approval/ApprovalController.ts b/src/approval/ApprovalController.ts index 9d4775736b8..a42a8015b87 100644 --- a/src/approval/ApprovalController.ts +++ b/src/approval/ApprovalController.ts @@ -44,7 +44,6 @@ export interface Approval { } export interface ApprovalConfig extends BaseConfig { - defaultApprovalType: string; showApprovalRequest: () => void; } @@ -70,8 +69,6 @@ const defaultState: ApprovalState = { [APPROVALS_STORE_KEY]: {}, [APPROVAL_COUNT */ export class ApprovalController extends BaseController { - public readonly defaultApprovalType: string; - private _approvals: Map; private _origins: Map>; @@ -80,22 +77,17 @@ export class ApprovalController extends BaseController { const promise = this._add(opts.origin, opts.type, opts.id, opts.requestData); @@ -150,7 +142,7 @@ export class ApprovalController extends BaseController { return this._add(opts.origin, opts.type, opts.id, opts.requestData); @@ -315,7 +307,7 @@ export class ApprovalController extends BaseController { diff --git a/tests/ApprovalController.test.js b/tests/ApprovalController.test.js index 2e5318c5923..8e5b815c79f 100644 --- a/tests/ApprovalController.test.js +++ b/tests/ApprovalController.test.js @@ -2,7 +2,6 @@ const { errorCodes } = require('eth-rpc-errors'); const ApprovalController = require('../dist/approval/ApprovalController').default; const defaultConfig = { - defaultApprovalType: 'DEFAULT_TYPE', showApprovalRequest: () => undefined, }; @@ -13,16 +12,10 @@ const STORE_KEY = 'pendingApprovals'; describe('ApprovalController: Input Validation', () => { describe('constructor', () => { it('throws on invalid input', () => { - expect(() => new ApprovalController({})).toThrow(getInvalidDefaultTypeError()); - expect(() => new ApprovalController({ showApprovalRequest: () => undefined })).toThrow( - getInvalidDefaultTypeError(), - ); - expect(() => new ApprovalController({ defaultApprovalType: 2 })).toThrow(getInvalidDefaultTypeError()); - - expect(() => new ApprovalController({ defaultApprovalType: 'foo' })).toThrow( + expect(() => new ApprovalController({})).toThrow( getInvalidShowApprovalRequestError(), ); - expect(() => new ApprovalController({ defaultApprovalType: 'foo', showApprovalRequest: 'bar' })).toThrow( + expect(() => new ApprovalController({ showApprovalRequest: 'bar' })).toThrow( getInvalidShowApprovalRequestError(), ); }); @@ -50,6 +43,7 @@ describe('ApprovalController: Input Validation', () => { approvalController.add({ id: 'foo', origin: 'bar.baz', + type: 'type', requestData: 'foo', }), ).toThrow(getInvalidRequestDataError()); @@ -60,7 +54,7 @@ describe('ApprovalController: Input Validation', () => { it('returns undefined for non-existing entry', () => { const approvalController = getApprovalController(); - approvalController.add({ id: 'foo', origin: 'bar.baz' }); + approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'type' }); expect(approvalController.get('fizz')).toBeUndefined(); @@ -103,25 +97,26 @@ describe('ApprovalController: Input Validation', () => { }); it('deletes entry', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz' }); + approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'type' }); approvalController._delete('foo'); expect( !approvalController.has({ id: 'foo' }) && + !approvalController.has({ type: 'type' }) && !approvalController.has({ origin: 'bar.baz' }) && !approvalController.state[STORE_KEY].foo, ).toEqual(true); }); it('deletes one entry out of many without side-effects', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz' }); - approvalController.add({ id: 'fizz', origin: 'bar.baz', type: 'myType' }); + approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'type1' }); + approvalController.add({ id: 'fizz', origin: 'bar.baz', type: 'type2' }); approvalController._delete('fizz'); expect( - !approvalController.has({ id: 'fizz' }) && !approvalController.has({ origin: 'bar.baz', type: 'myType' }), + !approvalController.has({ id: 'fizz' }) && !approvalController.has({ origin: 'bar.baz', type: 'type2' }), ).toEqual(true); expect(approvalController.has({ id: 'foo' }) && approvalController.has({ origin: 'bar.baz' })).toEqual(true); @@ -138,10 +133,6 @@ describe('ApprovalController: Input Validation', () => { // helpers -function getInvalidDefaultTypeError() { - return getError('Must specify non-empty string defaultApprovalType.'); -} - function getInvalidShowApprovalRequestError() { return getError('Must specify function showApprovalRequest.'); } diff --git a/tests/ApprovalController.test.ts b/tests/ApprovalController.test.ts index daddab38021..e49e29ccc47 100644 --- a/tests/ApprovalController.test.ts +++ b/tests/ApprovalController.test.ts @@ -5,10 +5,9 @@ const sinon = require('sinon'); const STORE_KEY = 'pendingApprovals'; -const DEFAULT_TYPE = 'DEFAULT_TYPE'; +const TYPE = 'TYPE'; const defaultConfig = { - defaultApprovalType: DEFAULT_TYPE, showApprovalRequest: () => undefined, }; @@ -24,57 +23,47 @@ describe('approval controller', () => { }); it('adds correctly specified entry', () => { - expect(() => approvalController.add({ id: 'foo', origin: 'bar.baz' })).not.toThrow(); + expect(() => approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE })).not.toThrow(); expect(approvalController.has({ id: 'foo' })).toEqual(true); + expect(approvalController.has({ origin: 'bar.baz', type: TYPE })).toEqual(true); expect(approvalController.state[STORE_KEY]).toEqual({ - foo: { id: 'foo', origin: 'bar.baz', time: 1, type: DEFAULT_TYPE }, + foo: { id: 'foo', origin: 'bar.baz', time: 1, type: TYPE }, }); }); it('adds id if non provided', () => { - expect(() => approvalController.add({ id: undefined, origin: 'bar.baz' })).not.toThrow(); + expect(() => approvalController.add({ id: undefined, origin: 'bar.baz', type: TYPE })).not.toThrow(); const id = Object.keys(approvalController.state[STORE_KEY])[0]; expect(id && typeof id === 'string').toBeTruthy(); }); - it('adds correctly specified entry with custom type', () => { - expect(() => approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' })).not.toThrow(); - - expect(approvalController.has({ id: 'foo' })).toEqual(true); - expect(approvalController.has({ origin: 'bar.baz', type: 'myType' })).toEqual(true); - expect(approvalController.state[STORE_KEY]).toEqual({ - foo: { id: 'foo', origin: 'bar.baz', type: 'myType', time: 1 }, - }); - }); - it('adds correctly specified entry with request data', () => { expect(() => approvalController.add({ id: 'foo', origin: 'bar.baz', - type: undefined, + type: 'myType', requestData: { foo: 'bar' }, }), ).not.toThrow(); expect(approvalController.has({ id: 'foo' })).toEqual(true); expect(approvalController.has({ origin: 'bar.baz' })).toEqual(true); + expect(approvalController.has({ type: 'myType' })).toEqual(true); expect(approvalController.state[STORE_KEY].foo.requestData).toEqual({ foo: 'bar' }); }); it('adds multiple entries for same origin with different types and ids', () => { const ORIGIN = 'bar.baz'; - expect(() => approvalController.add({ id: 'foo1', origin: ORIGIN })).not.toThrow(); - expect(() => approvalController.add({ id: 'foo2', origin: ORIGIN, type: 'myType1' })).not.toThrow(); - expect(() => approvalController.add({ id: 'foo3', origin: ORIGIN, type: 'myType2' })).not.toThrow(); + expect(() => approvalController.add({ id: 'foo1', origin: ORIGIN, type: 'myType1' })).not.toThrow(); + expect(() => approvalController.add({ id: 'foo2', origin: ORIGIN, type: 'myType2' })).not.toThrow(); expect( approvalController.has({ id: 'foo1' }) && - approvalController.has({ id: 'foo3' }) && - approvalController.has({ id: 'foo3' }), + approvalController.has({ id: 'foo2' }), ).toEqual(true); expect( approvalController.has({ origin: ORIGIN }) && @@ -84,20 +73,12 @@ describe('approval controller', () => { }); it('throws on id collision', () => { - expect(() => approvalController.add({ id: 'foo', origin: 'bar.baz' })).not.toThrow(); - - expect(() => approvalController.add({ id: 'foo', origin: 'fizz.buzz' })).toThrow(getIdCollisionError('foo')); - }); - - it('throws on origin and default type collision', () => { - expect(() => approvalController.add({ id: 'foo', origin: 'bar.baz' })).not.toThrow(); + expect(() => approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE })).not.toThrow(); - expect(() => approvalController.add({ id: 'foo1', origin: 'bar.baz' })).toThrow( - getOriginTypeCollisionError('bar.baz'), - ); + expect(() => approvalController.add({ id: 'foo', origin: 'fizz.buzz', type: TYPE })).toThrow(getIdCollisionError('foo')); }); - it('throws on origin and custom type collision', () => { + it('throws on origin and type collision', () => { expect(() => approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' })).not.toThrow(); expect(() => approvalController.add({ id: 'foo1', origin: 'bar.baz', type: 'myType' })).toThrow( @@ -127,21 +108,9 @@ describe('approval controller', () => { }); describe('get', () => { - let approvalController: ApprovalController; - - beforeEach(() => { - approvalController = new ApprovalController({ ...defaultConfig }); - }); - - it('gets entry with default type', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz' }); - - expect(approvalController.get('foo')).toEqual({ id: 'foo', origin: 'bar.baz', time: 1, type: DEFAULT_TYPE }); - }); - - it('gets entry with custom type', () => { + it('gets entry', () => { + const approvalController = new ApprovalController({ ...defaultConfig }); approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' }); - expect(approvalController.get('foo')).toEqual({ id: 'foo', origin: 'bar.baz', type: 'myType', time: 1 }); }); }); @@ -156,25 +125,25 @@ describe('approval controller', () => { }); it('gets the count when specifying origin and type', () => { - addWithCatch({ id: '1', origin: 'origin1' }); + addWithCatch({ id: '1', origin: 'origin1', type: TYPE }); addWithCatch({ id: '2', origin: 'origin1', type: 'type1' }); addWithCatch({ id: '3', origin: 'origin2', type: 'type1' }); - expect(approvalController.getApprovalCount({ origin: 'origin1', type: DEFAULT_TYPE })).toEqual(1); + expect(approvalController.getApprovalCount({ origin: 'origin1', type: TYPE })).toEqual(1); expect(approvalController.getApprovalCount({ origin: 'origin1', type: 'type1' })).toEqual(1); expect(approvalController.getApprovalCount({ origin: 'origin1', type: 'type2' })).toEqual(0); - expect(approvalController.getApprovalCount({ origin: 'origin2', type: DEFAULT_TYPE })).toEqual(0); + expect(approvalController.getApprovalCount({ origin: 'origin2', type: TYPE })).toEqual(0); expect(approvalController.getApprovalCount({ origin: 'origin2', type: 'type1' })).toEqual(1); expect(approvalController.getApprovalCount({ origin: 'origin2', type: 'type2' })).toEqual(0); - expect(approvalController.getApprovalCount({ origin: 'origin3', type: DEFAULT_TYPE })).toEqual(0); + expect(approvalController.getApprovalCount({ origin: 'origin3', type: TYPE })).toEqual(0); expect(approvalController.getApprovalCount({ origin: 'origin3', type: 'type1' })).toEqual(0); expect(approvalController.getApprovalCount({ origin: 'origin3', type: 'type2' })).toEqual(0); }); it('gets the count when specifying origin only', () => { - addWithCatch({ id: '1', origin: 'origin1' }); + addWithCatch({ id: '1', origin: 'origin1', type: 'type0' }); addWithCatch({ id: '2', origin: 'origin1', type: 'type1' }); addWithCatch({ id: '3', origin: 'origin2', type: 'type1' }); @@ -186,13 +155,10 @@ describe('approval controller', () => { }); it('gets the count when specifying type only', () => { - addWithCatch({ id: '1', origin: 'origin1' }); addWithCatch({ id: '2', origin: 'origin1', type: 'type1' }); addWithCatch({ id: '3', origin: 'origin2', type: 'type1' }); addWithCatch({ id: '4', origin: 'origin2', type: 'type2' }); - expect(approvalController.getApprovalCount({ type: DEFAULT_TYPE })).toEqual(1); - expect(approvalController.getApprovalCount({ type: 'type1' })).toEqual(2); expect(approvalController.getApprovalCount({ type: 'type2' })).toEqual(1); @@ -208,7 +174,7 @@ describe('approval controller', () => { const addWithCatch = (args: any) => approvalController.add(args).catch(() => undefined); - addWithCatch({ id: '1', origin: 'origin1' }); + addWithCatch({ id: '1', origin: 'origin1', type: 'type0' }); expect(approvalController.getTotalApprovalCount()).toEqual(1); addWithCatch({ id: '2', origin: 'origin1', type: 'type1' }); @@ -233,49 +199,43 @@ describe('approval controller', () => { }); it('returns true for existing entry by id', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz' }); + approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE }); expect(approvalController.has({ id: 'foo' })).toEqual(true); }); it('returns true for existing entry by origin', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz' }); + approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE }); expect(approvalController.has({ origin: 'bar.baz' })).toEqual(true); }); - it('returns true for existing entry by origin and custom type', () => { + it('returns true for existing entry by origin and type', () => { approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' }); expect(approvalController.has({ origin: 'bar.baz', type: 'myType' })).toEqual(true); }); - it('returns true for existing default type', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz' }); - - expect(approvalController.has({ type: approvalController.defaultApprovalType })).toEqual(true); - }); - - it('returns true for existing custom type', () => { + it('returns true for existing type', () => { approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' }); expect(approvalController.has({ type: 'myType' })).toEqual(true); }); it('returns false for non-existing entry by id', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz' }); + approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE }); expect(approvalController.has({ id: 'fizz' })).toEqual(false); }); it('returns false for non-existing entry by origin', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz' }); + approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE }); expect(approvalController.has({ origin: 'fizz.buzz' })).toEqual(false); }); it('returns false for non-existing entry by existing origin and non-existing type', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz' }); + approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE }); expect(approvalController.has({ origin: 'bar.baz', type: 'myType' })).toEqual(false); }); @@ -286,16 +246,10 @@ describe('approval controller', () => { expect(approvalController.has({ origin: 'fizz.buzz', type: 'myType' })).toEqual(false); }); - it('returns false for non-existing entry by default type', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' }); - - expect(approvalController.has({ type: approvalController.defaultApprovalType })).toEqual(false); - }); - - it('returns false for non-existing entry by custom type', () => { - approvalController.add({ id: 'foo', origin: 'bar.baz' }); + it('returns false for non-existing entry by type', () => { + approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType1' }); - expect(approvalController.has({ type: 'myType' })).toEqual(false); + expect(approvalController.has({ type: 'myType2' })).toEqual(false); }); }); @@ -313,7 +267,7 @@ describe('approval controller', () => { it('resolves approval promise', async () => { numDeletions = 1; - const approvalPromise = approvalController.add({ id: 'foo', origin: 'bar.baz' }); + const approvalPromise = approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' }); approvalController.resolve('foo', 'success'); const result = await approvalPromise; @@ -324,7 +278,7 @@ describe('approval controller', () => { it('resolves multiple approval promises out of order', async () => { numDeletions = 2; - const approvalPromise1 = approvalController.add({ id: 'foo1', origin: 'bar.baz' }); + const approvalPromise1 = approvalController.add({ id: 'foo1', origin: 'bar.baz', type: 'myType1' }); const approvalPromise2 = approvalController.add({ id: 'foo2', origin: 'bar.baz', type: 'myType2' }); approvalController.resolve('foo2', 'success2'); @@ -359,7 +313,7 @@ describe('approval controller', () => { it('rejects approval promise', async () => { numDeletions = 1; - const approvalPromise = approvalController.add({ id: 'foo', origin: 'bar.baz' }).catch((error) => { + const approvalPromise = approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE }).catch((error) => { expect(error).toMatchObject(getError('failure')); }); @@ -371,7 +325,7 @@ describe('approval controller', () => { it('rejects multiple approval promises out of order', async () => { numDeletions = 2; - const rejectionPromise1 = approvalController.add({ id: 'foo1', origin: 'bar.baz' }).catch((error) => { + const rejectionPromise1 = approvalController.add({ id: 'foo1', origin: 'bar.baz', type: TYPE }).catch((error) => { expect(error).toMatchObject(getError('failure1')); }); const rejectionPromise2 = approvalController @@ -398,9 +352,9 @@ describe('approval controller', () => { it('resolves and rejects multiple approval promises out of order', async () => { const approvalController = new ApprovalController({ ...defaultConfig }); - const promise1 = approvalController.add({ id: 'foo1', origin: 'bar.baz' }); + const promise1 = approvalController.add({ id: 'foo1', origin: 'bar.baz', type: TYPE }); const promise2 = approvalController.add({ id: 'foo2', origin: 'bar.baz', type: 'myType2' }); - const promise3 = approvalController.add({ id: 'foo3', origin: 'fizz.buzz' }).catch((error) => { + const promise3 = approvalController.add({ id: 'foo3', origin: 'fizz.buzz', type: TYPE }).catch((error) => { expect(error).toMatchObject(getError('failure3')); }); const promise4 = approvalController.add({ id: 'foo4', origin: 'bar.baz', type: 'myType4' }).catch((error) => { @@ -444,14 +398,13 @@ describe('approval controller', () => { it('deletes existing entries', async () => { const rejectSpy = sinon.spy(approvalController, 'reject'); - approvalController.add({ id: 'foo1', origin: 'bar.baz' }).catch((_error) => undefined); approvalController.add({ id: 'foo2', origin: 'bar.baz', type: 'myType' }).catch((_error) => undefined); approvalController.add({ id: 'foo3', origin: 'fizz.buzz', type: 'myType' }).catch((_error) => undefined); approvalController.clear(); expect(approvalController.state[STORE_KEY]).toEqual({}); - expect(rejectSpy.callCount).toEqual(3); + expect(rejectSpy.callCount).toEqual(2); }); }); }); @@ -462,7 +415,7 @@ function getIdCollisionError(id: string) { return getError(`Approval with id '${id}' already exists.`, errorCodes.rpc.internal); } -function getOriginTypeCollisionError(origin: string, type = DEFAULT_TYPE) { +function getOriginTypeCollisionError(origin: string, type = TYPE) { const message = `Request of type '${type}' already pending for origin ${origin}. Please wait.`; return getError(message, errorCodes.rpc.resourceUnavailable); } From 7e3ab7519fba3645e5c92fbb99665f3a971b423c Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Thu, 17 Dec 2020 00:13:03 -0800 Subject: [PATCH 2/3] fixup! Remove default approval controller type --- tests/ApprovalController.test.js | 4 +--- tests/ApprovalController.test.ts | 9 ++++----- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/ApprovalController.test.js b/tests/ApprovalController.test.js index 8e5b815c79f..095a4d949ae 100644 --- a/tests/ApprovalController.test.js +++ b/tests/ApprovalController.test.js @@ -12,9 +12,7 @@ const STORE_KEY = 'pendingApprovals'; describe('ApprovalController: Input Validation', () => { describe('constructor', () => { it('throws on invalid input', () => { - expect(() => new ApprovalController({})).toThrow( - getInvalidShowApprovalRequestError(), - ); + expect(() => new ApprovalController({})).toThrow(getInvalidShowApprovalRequestError()); expect(() => new ApprovalController({ showApprovalRequest: 'bar' })).toThrow( getInvalidShowApprovalRequestError(), ); diff --git a/tests/ApprovalController.test.ts b/tests/ApprovalController.test.ts index e49e29ccc47..4f55c698aa6 100644 --- a/tests/ApprovalController.test.ts +++ b/tests/ApprovalController.test.ts @@ -61,10 +61,7 @@ describe('approval controller', () => { expect(() => approvalController.add({ id: 'foo1', origin: ORIGIN, type: 'myType1' })).not.toThrow(); expect(() => approvalController.add({ id: 'foo2', origin: ORIGIN, type: 'myType2' })).not.toThrow(); - expect( - approvalController.has({ id: 'foo1' }) && - approvalController.has({ id: 'foo2' }), - ).toEqual(true); + expect(approvalController.has({ id: 'foo1' }) && approvalController.has({ id: 'foo2' })).toEqual(true); expect( approvalController.has({ origin: ORIGIN }) && approvalController.has({ origin: ORIGIN, type: 'myType1' }) && @@ -75,7 +72,9 @@ describe('approval controller', () => { it('throws on id collision', () => { expect(() => approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE })).not.toThrow(); - expect(() => approvalController.add({ id: 'foo', origin: 'fizz.buzz', type: TYPE })).toThrow(getIdCollisionError('foo')); + expect(() => approvalController.add({ id: 'foo', origin: 'fizz.buzz', type: TYPE })).toThrow( + getIdCollisionError('foo'), + ); }); it('throws on origin and type collision', () => { From 5754f8ea62c85d0b9ac7b5a1c0b1327dbdf5b885 Mon Sep 17 00:00:00 2001 From: Erik Marks Date: Thu, 17 Dec 2020 00:28:56 -0800 Subject: [PATCH 3/3] Remove references to default type in messages and errors --- src/approval/ApprovalController.ts | 8 +++----- tests/ApprovalController.test.js | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/approval/ApprovalController.ts b/src/approval/ApprovalController.ts index a42a8015b87..a3c5a453976 100644 --- a/src/approval/ApprovalController.ts +++ b/src/approval/ApprovalController.ts @@ -105,8 +105,7 @@ export class ApprovalController extends BaseController