Skip to content
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

Add approval controller count state #306

Merged
merged 2 commits into from
Nov 13, 2020
Merged

Add approval controller count state #306

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fbcec1
Add approval controller count state
rekmarks Nov 11, 2020
2a49858
fixup! Add approval controller count state
rekmarks Nov 11, 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
31 changes: 17 additions & 14 deletions src/approval/ApprovalController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ import { ethErrors } from 'eth-rpc-errors';
import BaseController, { BaseConfig, BaseState } from '../BaseController';

const NO_TYPE = Symbol('NO_APPROVAL_TYPE');
const STORE_KEY = 'pendingApprovals';
const APPROVALS_STORE_KEY = 'pendingApprovals';
const APPROVAL_COUNT_STORE_KEY = 'pendingApprovalCount';

type ApprovalType = string | typeof NO_TYPE;

Expand Down Expand Up @@ -32,14 +33,15 @@ export interface ApprovalConfig extends BaseConfig {
}

export interface ApprovalState extends BaseState {
[STORE_KEY]: { [approvalId: string]: ApprovalInfo };
[APPROVALS_STORE_KEY]: { [approvalId: string]: ApprovalInfo };
[APPROVAL_COUNT_STORE_KEY]: number;
}

const getAlreadyPendingMessage = (origin: string, type: ApprovalType) => (
`Request ${type === NO_TYPE ? '' : `of type '${type}' `}already pending for origin ${origin}. Please wait.`
);

const defaultState = { [STORE_KEY]: {} };
const defaultState: ApprovalState = { [APPROVALS_STORE_KEY]: {}, [APPROVAL_COUNT_STORE_KEY]: 0 };

/**
* Controller for keeping track of pending approvals by id and/or origin and
Expand All @@ -65,10 +67,9 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
super(config, state || defaultState);

this._approvals = new Map();

this._origins = new Map();

this._showApprovalRequest = config.showApprovalRequest;
this.initialize();
}

/**
Expand Down Expand Up @@ -134,7 +135,7 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
* @returns The pending approval data associated with the id.
*/
get(id: string): ApprovalInfo | undefined {
const info = this.state[STORE_KEY][id];
const info = this.state[APPROVALS_STORE_KEY][id];
return info
? { ...info }
: undefined;
Expand Down Expand Up @@ -185,7 +186,7 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
* @returns The total pending approval count, for all types and origins.
*/
getTotalApprovalCount(): number {
return this._approvals.size;
return this.state[APPROVAL_COUNT_STORE_KEY];
}

/**
Expand Down Expand Up @@ -359,10 +360,11 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
}

this.update({
[STORE_KEY]: {
...this.state[STORE_KEY],
[APPROVALS_STORE_KEY]: {
...this.state[APPROVALS_STORE_KEY],
[id]: info,
},
[APPROVAL_COUNT_STORE_KEY]: this.state[APPROVAL_COUNT_STORE_KEY] + 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an example of the count getting out of sync, this could happen here if id is already in state. Then this would replace that entry, and falsely inflate the count by 1 per additional call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If consumers respect the interface of the controller and only use its public methods, it is impossible for that to happen (see _validateAddParams).

I decided to add the approval count to the state of this controller in order to make it consumable via our usual state subscription logic.

}, true);
}

Expand All @@ -378,22 +380,23 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
if (this._approvals.has(id)) {
this._approvals.delete(id);

const state = this.state[STORE_KEY];
const approvals = this.state[APPROVALS_STORE_KEY];
const {
origin,
type = NO_TYPE,
} = state[id];
} = approvals[id];

/* istanbul ignore next */
this._origins.get(origin)?.delete(type);
if (this._isEmptyOrigin(origin)) {
this._origins.delete(origin);
}

const newState = { ...state };
delete newState[id];
const newApprovals = { ...approvals };
delete newApprovals[id];
this.update({
[STORE_KEY]: newState,
[APPROVALS_STORE_KEY]: newApprovals,
[APPROVAL_COUNT_STORE_KEY]: this.state[APPROVAL_COUNT_STORE_KEY] - 1,
}, true);
}
}
Expand Down