Skip to content

Remove default approval controller type #321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rekmarks merged 3 commits into from
Jan 19, 2021
Merged

Remove default approval controller type #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73aea59
Remove default approval controller type
rekmarks Dec 17, 2020
7e3ab75
fixup! Remove default approval controller type
rekmarks Dec 17, 2020
5754f8e
Remove references to default type in messages and errors
rekmarks Dec 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 7 additions & 17 deletions src/approval/ApprovalController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ export interface Approval {
}

export interface ApprovalConfig extends BaseConfig {
defaultApprovalType: string;
showApprovalRequest: () => void;
}

Expand All @@ -70,8 +69,6 @@ const defaultState: ApprovalState = { [APPROVALS_STORE_KEY]: {}, [APPROVAL_COUNT
*/
export class ApprovalController extends BaseController<ApprovalConfig, ApprovalState> {

public readonly defaultApprovalType: string;

private _approvals: Map<string, ApprovalCallbacks>;

private _origins: Map<string, Set<string>>;
Expand All @@ -80,22 +77,17 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS

/**
* @param opts - Options bag
* @param opts.defaultApprovalType - The default type for approvals.
* @param opts.showApprovalRequest - Function for opening the UI such that
* the request can be displayed to the user.
*/
constructor(config: ApprovalConfig, state?: ApprovalState) {
const { defaultApprovalType, showApprovalRequest } = config;
if (!defaultApprovalType || typeof defaultApprovalType !== 'string') {
throw new Error('Must specify non-empty string defaultApprovalType.');
}
const { showApprovalRequest } = config;
if (typeof showApprovalRequest !== 'function') {
throw new Error('Must specify function showApprovalRequest.');
}

super(config, state || defaultState);

this.defaultApprovalType = defaultApprovalType;
this._approvals = new Map();
this._origins = new Map();
this._showApprovalRequest = showApprovalRequest;
Expand All @@ -113,16 +105,15 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
* @param opts.id - The id of the approval request. A random id will be
* generated if none is provided.
* @param opts.origin - The origin of the approval request.
* @param opts.type - The type associated with the approval request. The
* default type will be used if no type is specified.
* @param opts.type - The type associated with the approval request.
* @param opts.requestData - Additional data associated with the request,
* if any.
* @returns The approval promise.
*/
addAndShowApprovalRequest(opts: {
id?: string;
origin: string;
type?: string;
type: string;
requestData?: RequestData;
}): Promise<unknown> {
const promise = this._add(opts.origin, opts.type, opts.id, opts.requestData);
Expand All @@ -141,16 +132,15 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
* @param opts.id - The id of the approval request. A random id will be
* generated if none is provided.
* @param opts.origin - The origin of the approval request.
* @param opts.type - The type associated with the approval request. The
* default type will be used if no type is specified.
* @param opts.type - The type associated with the approval request.
* @param opts.requestData - Additional data associated with the request,
* if any.
* @returns The approval promise.
*/
add(opts: {
id?: string;
origin: string;
type?: string;
type: string;
requestData?: RequestData;
}): Promise<unknown> {
return this._add(opts.origin, opts.type, opts.id, opts.requestData);
Expand Down Expand Up @@ -315,7 +305,7 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
*/
private _add(
origin: string,
type: string = this.defaultApprovalType,
type: string,
id: string = nanoid(),
requestData?: RequestData,
): Promise<unknown> {
Expand Down Expand Up @@ -357,7 +347,7 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
} else if (!origin || typeof origin !== 'string') {
errorMessage = 'Must specify non-empty string origin.';
} else if (!type || typeof type !== 'string') {
errorMessage = 'May not specify empty or non-string type.';
errorMessage = 'Must specify non-empty string type.';
} else if (requestData && (
typeof requestData !== 'object' || Array.isArray(requestData)
)) {
Expand Down
31 changes: 10 additions & 21 deletions tests/ApprovalController.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ const { errorCodes } = require('eth-rpc-errors');
const ApprovalController = require('../dist/approval/ApprovalController').default;

const defaultConfig = {
defaultApprovalType: 'DEFAULT_TYPE',
showApprovalRequest: () => undefined,
};

Expand All @@ -13,16 +12,8 @@ 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(
getInvalidShowApprovalRequestError(),
);
expect(() => new ApprovalController({ defaultApprovalType: 'foo', showApprovalRequest: 'bar' })).toThrow(
expect(() => new ApprovalController({})).toThrow(getInvalidShowApprovalRequestError());
expect(() => new ApprovalController({ showApprovalRequest: 'bar' })).toThrow(
getInvalidShowApprovalRequestError(),
);
});
Expand Down Expand Up @@ -50,6 +41,7 @@ describe('ApprovalController: Input Validation', () => {
approvalController.add({
id: 'foo',
origin: 'bar.baz',
type: 'type',
requestData: 'foo',
}),
).toThrow(getInvalidRequestDataError());
Expand All @@ -60,7 +52,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();

Expand Down Expand Up @@ -103,25 +95,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);
Expand All @@ -138,10 +131,6 @@ describe('ApprovalController: Input Validation', () => {

// helpers

function getInvalidDefaultTypeError() {
return getError('Must specify non-empty string defaultApprovalType.');
}

function getInvalidShowApprovalRequestError() {
return getError('Must specify function showApprovalRequest.');
}
Expand Down Expand Up @@ -171,7 +160,7 @@ function getInvalidRequestDataError() {
}

function getInvalidTypeError(code) {
return getError('May not specify empty or non-string type.', code);
return getError('Must specify non-empty string type.', code);
}

function getInvalidHasParamsError() {
Expand Down
Loading