Skip to content

Commit 97c5aec

Browse files
rekmarksMajorLift
rekmarks
authored and
MajorLift
committed
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.
1 parent 6ef592a commit 97c5aec

File tree

3 files changed

+379
-133
lines changed

3 files changed

+379
-133
lines changed

‎src/approval/ApprovalController.test.js

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,24 @@
11
const { errorCodes } = require('eth-rpc-errors');
2+
const { ControllerMessenger } = require('../ControllerMessenger');
23
const { ApprovalController } = require('./ApprovalController');
34

4-
const defaultConfig = {
5-
showApprovalRequest: () => undefined,
6-
};
5+
function getRestrictedMessenger() {
6+
const controllerMessenger = new ControllerMessenger();
7+
const messenger = controllerMessenger.getRestricted({
8+
name: 'ApprovalController',
9+
});
10+
return messenger;
11+
}
712

813
const getApprovalController = () =>
9-
new ApprovalController({ ...defaultConfig });
14+
new ApprovalController({
15+
messenger: getRestrictedMessenger(),
16+
showApprovalRequest: () => undefined,
17+
});
1018

1119
const STORE_KEY = 'pendingApprovals';
1220

1321
describe('ApprovalController: Input Validation', () => {
14-
describe('constructor', () => {
15-
it('throws on invalid input', () => {
16-
expect(() => new ApprovalController({})).toThrow(
17-
getInvalidShowApprovalRequestError(),
18-
);
19-
expect(
20-
() => new ApprovalController({ showApprovalRequest: 'bar' }),
21-
).toThrow(getInvalidShowApprovalRequestError());
22-
});
23-
});
24-
2522
describe('add', () => {
2623
it('validates input', () => {
2724
const approvalController = getApprovalController();
@@ -109,6 +106,9 @@ describe('ApprovalController: Input Validation', () => {
109106
expect(() => approvalController.has({ type: true })).toThrow(
110107
getInvalidHasTypeError(),
111108
);
109+
expect(() =>
110+
approvalController.has({ origin: 'foo', type: true }),
111+
).toThrow(getInvalidHasTypeError());
112112
});
113113
});
114114

@@ -118,7 +118,7 @@ describe('ApprovalController: Input Validation', () => {
118118
let approvalController;
119119

120120
beforeEach(() => {
121-
approvalController = new ApprovalController({ ...defaultConfig });
121+
approvalController = getApprovalController();
122122
});
123123

124124
it('deletes entry', () => {
@@ -162,10 +162,6 @@ describe('ApprovalController: Input Validation', () => {
162162

163163
// helpers
164164

165-
function getInvalidShowApprovalRequestError() {
166-
return getError('Must specify function showApprovalRequest.');
167-
}
168-
169165
function getInvalidIdError() {
170166
return getError('Must specify non-empty string id.', errorCodes.rpc.internal);
171167
}
@@ -201,7 +197,7 @@ function getInvalidTypeError(code) {
201197
}
202198

203199
function getInvalidHasParamsError() {
204-
return getError('Must specify non-empty string id, origin, or type.');
200+
return getError('Must specify a valid combination of id, origin, and type.');
205201
}
206202

207203
function getApprovalCountParamsError() {

‎src/approval/ApprovalController.test.ts

Lines changed: 128 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,48 @@
11
import { errorCodes } from 'eth-rpc-errors';
22
import sinon from 'sinon';
3-
import { ApprovalController } from './ApprovalController';
3+
import { ControllerMessenger } from '../ControllerMessenger';
4+
import {
5+
ApprovalController,
6+
ApprovalControllerActions,
7+
ApprovalControllerEvents,
8+
ApprovalControllerMessenger,
9+
} from './ApprovalController';
410

511
const STORE_KEY = 'pendingApprovals';
612

713
const TYPE = 'TYPE';
814

9-
const defaultConfig = {
10-
showApprovalRequest: () => undefined,
11-
};
15+
const controllerName = 'ApprovalController';
16+
17+
function getRestrictedMessenger() {
18+
const controllerMessenger = new ControllerMessenger<
19+
ApprovalControllerActions,
20+
ApprovalControllerEvents
21+
>();
22+
const messenger = controllerMessenger.getRestricted<
23+
typeof controllerName,
24+
never,
25+
never
26+
>({
27+
name: 'ApprovalController',
28+
});
29+
return messenger;
30+
}
1231

1332
describe('approval controller', () => {
1433
const clock = sinon.useFakeTimers(1);
1534
afterAll(() => clock.restore());
1635

1736
describe('add', () => {
1837
let approvalController: ApprovalController;
38+
let showApprovalRequest: sinon.SinonSpy;
1939

2040
beforeEach(() => {
21-
approvalController = new ApprovalController({ ...defaultConfig });
41+
showApprovalRequest = sinon.spy();
42+
approvalController = new ApprovalController({
43+
messenger: getRestrictedMessenger(),
44+
showApprovalRequest,
45+
});
2246
});
2347

2448
it('adds correctly specified entry', () => {
@@ -31,7 +55,13 @@ describe('approval controller', () => {
3155
approvalController.has({ origin: 'bar.baz', type: TYPE }),
3256
).toStrictEqual(true);
3357
expect(approvalController.state[STORE_KEY]).toStrictEqual({
34-
foo: { id: 'foo', origin: 'bar.baz', time: 1, type: TYPE },
58+
foo: {
59+
id: 'foo',
60+
origin: 'bar.baz',
61+
requestData: null,
62+
time: 1,
63+
type: TYPE,
64+
},
3565
});
3666
});
3767

@@ -121,7 +151,7 @@ describe('approval controller', () => {
121151
it('addAndShowApprovalRequest', () => {
122152
const showApprovalSpy = sinon.spy();
123153
const approvalController = new ApprovalController({
124-
...defaultConfig,
154+
messenger: getRestrictedMessenger(),
125155
showApprovalRequest: showApprovalSpy,
126156
});
127157

@@ -138,11 +168,15 @@ describe('approval controller', () => {
138168

139169
describe('get', () => {
140170
it('gets entry', () => {
141-
const approvalController = new ApprovalController({ ...defaultConfig });
171+
const approvalController = new ApprovalController({
172+
messenger: getRestrictedMessenger(),
173+
showApprovalRequest: sinon.spy(),
174+
});
142175
approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' });
143176
expect(approvalController.get('foo')).toStrictEqual({
144177
id: 'foo',
145178
origin: 'bar.baz',
179+
requestData: null,
146180
type: 'myType',
147181
time: 1,
148182
});
@@ -154,7 +188,10 @@ describe('approval controller', () => {
154188
let addWithCatch: (args: any) => void;
155189

156190
beforeEach(() => {
157-
approvalController = new ApprovalController({ ...defaultConfig });
191+
approvalController = new ApprovalController({
192+
messenger: getRestrictedMessenger(),
193+
showApprovalRequest: sinon.spy(),
194+
});
158195
addWithCatch = (args: any) =>
159196
approvalController.add(args).catch(() => undefined);
160197
});
@@ -252,7 +289,10 @@ describe('approval controller', () => {
252289

253290
describe('getTotalApprovalCount', () => {
254291
it('gets the total approval count', () => {
255-
const approvalController = new ApprovalController({ ...defaultConfig });
292+
const approvalController = new ApprovalController({
293+
messenger: getRestrictedMessenger(),
294+
showApprovalRequest: sinon.spy(),
295+
});
256296
expect(approvalController.getTotalApprovalCount()).toStrictEqual(0);
257297

258298
const addWithCatch = (args: any) =>
@@ -279,7 +319,10 @@ describe('approval controller', () => {
279319
let approvalController: ApprovalController;
280320

281321
beforeEach(() => {
282-
approvalController = new ApprovalController({ ...defaultConfig });
322+
approvalController = new ApprovalController({
323+
messenger: getRestrictedMessenger(),
324+
showApprovalRequest: sinon.spy(),
325+
});
283326
});
284327

285328
it('returns true for existing entry by id', () => {
@@ -351,7 +394,10 @@ describe('approval controller', () => {
351394
let deleteSpy: sinon.SinonSpy;
352395

353396
beforeEach(() => {
354-
approvalController = new ApprovalController({ ...defaultConfig });
397+
approvalController = new ApprovalController({
398+
messenger: getRestrictedMessenger(),
399+
showApprovalRequest: sinon.spy(),
400+
});
355401
// TODO: Stop using private methods in tests
356402
deleteSpy = sinon.spy(approvalController as any, '_delete');
357403
numDeletions = 0;
@@ -365,7 +411,7 @@ describe('approval controller', () => {
365411
origin: 'bar.baz',
366412
type: 'myType',
367413
});
368-
approvalController.resolve('foo', 'success');
414+
approvalController.accept('foo', 'success');
369415

370416
const result = await approvalPromise;
371417
expect(result).toStrictEqual('success');
@@ -386,20 +432,20 @@ describe('approval controller', () => {
386432
type: 'myType2',
387433
});
388434

389-
approvalController.resolve('foo2', 'success2');
435+
approvalController.accept('foo2', 'success2');
390436

391437
let result = await approvalPromise2;
392438
expect(result).toStrictEqual('success2');
393439

394-
approvalController.resolve('foo1', 'success1');
440+
approvalController.accept('foo1', 'success1');
395441

396442
result = await approvalPromise1;
397443
expect(result).toStrictEqual('success1');
398444
expect(deleteSpy.callCount).toStrictEqual(numDeletions);
399445
});
400446

401447
it('throws on unknown id', () => {
402-
expect(() => approvalController.resolve('foo')).toThrow(
448+
expect(() => approvalController.accept('foo')).toThrow(
403449
getIdNotFoundError('foo'),
404450
);
405451
expect(deleteSpy.callCount).toStrictEqual(numDeletions);
@@ -412,7 +458,10 @@ describe('approval controller', () => {
412458
let deleteSpy: sinon.SinonSpy;
413459

414460
beforeEach(() => {
415-
approvalController = new ApprovalController({ ...defaultConfig });
461+
approvalController = new ApprovalController({
462+
messenger: getRestrictedMessenger(),
463+
showApprovalRequest: sinon.spy(),
464+
});
416465
// TODO: Stop using private methods in tests
417466
deleteSpy = sinon.spy(approvalController as any, '_delete');
418467
numDeletions = 0;
@@ -459,9 +508,12 @@ describe('approval controller', () => {
459508
});
460509
});
461510

462-
describe('resolve and reject', () => {
463-
it('resolves and rejects multiple approval promises out of order', async () => {
464-
const approvalController = new ApprovalController({ ...defaultConfig });
511+
describe('accept and reject', () => {
512+
it('accepts and rejects multiple approval promises out of order', async () => {
513+
const approvalController = new ApprovalController({
514+
messenger: getRestrictedMessenger(),
515+
showApprovalRequest: sinon.spy(),
516+
});
465517

466518
const promise1 = approvalController.add({
467519
id: 'foo1',
@@ -484,7 +536,7 @@ describe('approval controller', () => {
484536
type: 'myType4',
485537
});
486538

487-
approvalController.resolve('foo2', 'success2');
539+
approvalController.accept('foo2', 'success2');
488540

489541
let result = await promise2;
490542
expect(result).toStrictEqual('success2');
@@ -500,7 +552,7 @@ describe('approval controller', () => {
500552
);
501553
expect(approvalController.has({ origin: 'bar.baz' })).toStrictEqual(true);
502554

503-
approvalController.resolve('foo1', 'success1');
555+
approvalController.accept('foo1', 'success1');
504556

505557
result = await promise1;
506558
expect(result).toStrictEqual('success1');
@@ -515,7 +567,10 @@ describe('approval controller', () => {
515567
let approvalController: ApprovalController;
516568

517569
beforeEach(() => {
518-
approvalController = new ApprovalController({ ...defaultConfig });
570+
approvalController = new ApprovalController({
571+
messenger: getRestrictedMessenger(),
572+
showApprovalRequest: sinon.spy(),
573+
});
519574
});
520575

521576
it('does nothing if state is already empty', () => {
@@ -538,13 +593,61 @@ describe('approval controller', () => {
538593
expect(rejectSpy.callCount).toStrictEqual(2);
539594
});
540595
});
596+
597+
describe('actions', () => {
598+
it('addApprovalRequest: shouldShowRequest = true', async () => {
599+
const messenger = new ControllerMessenger<
600+
ApprovalControllerActions,
601+
ApprovalControllerEvents
602+
>();
603+
const showApprovalSpy = sinon.spy();
604+
605+
const approvalController = new ApprovalController({
606+
messenger: messenger.getRestricted({
607+
name: controllerName,
608+
}) as ApprovalControllerMessenger,
609+
showApprovalRequest: showApprovalSpy,
610+
});
611+
612+
messenger.call(
613+
'ApprovalController:addRequest',
614+
{ id: 'foo', origin: 'bar.baz', type: TYPE },
615+
true,
616+
);
617+
expect(showApprovalSpy.calledOnce).toStrictEqual(true);
618+
expect(approvalController.has({ id: 'foo' })).toStrictEqual(true);
619+
});
620+
621+
it('addApprovalRequest: shouldShowRequest = false', async () => {
622+
const messenger = new ControllerMessenger<
623+
ApprovalControllerActions,
624+
ApprovalControllerEvents
625+
>();
626+
const showApprovalSpy = sinon.spy();
627+
628+
const approvalController = new ApprovalController({
629+
messenger: messenger.getRestricted({
630+
name: controllerName,
631+
}) as ApprovalControllerMessenger,
632+
showApprovalRequest: showApprovalSpy,
633+
});
634+
635+
messenger.call(
636+
'ApprovalController:addRequest',
637+
{ id: 'foo', origin: 'bar.baz', type: TYPE },
638+
false,
639+
);
640+
expect(showApprovalSpy.notCalled).toStrictEqual(true);
641+
expect(approvalController.has({ id: 'foo' })).toStrictEqual(true);
642+
});
643+
});
541644
});
542645

543646
// helpers
544647

545648
function getIdCollisionError(id: string) {
546649
return getError(
547-
`Approval with id '${id}' already exists.`,
650+
`Approval request with id '${id}' already exists.`,
548651
errorCodes.rpc.internal,
549652
);
550653
}
@@ -555,7 +658,7 @@ function getOriginTypeCollisionError(origin: string, type = TYPE) {
555658
}
556659

557660
function getIdNotFoundError(id: string) {
558-
return getError(`Approval with id '${id}' not found.`);
661+
return getError(`Approval request with id '${id}' not found.`);
559662
}
560663

561664
function getError(message: string, code?: number) {

0 commit comments

Comments
 (0)