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
+17 −14
Merged

Add approval controller count state #306

Changes from 1 commit
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
Next Next commit
Add approval controller count state
@rekmarks
rekmarks committed Nov 11, 2020
commit 5fbcec16479a4c22b44a658503dbfe27fd17a561
36 changes: 21 additions & 15 deletions src/approval/ApprovalController.ts
View file
Original file line number Diff line number Diff line change
@@ -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;

@@ -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
@@ -65,10 +67,12 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
super(config, state || defaultState);

this._approvals = new Map();

this._origins = new Map();

this._showApprovalRequest = config.showApprovalRequest;

this.defaultConfig = config;
this.defaultState = defaultState;
this.initialize();
}

/**
@@ -134,7 +138,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;
@@ -185,7 +189,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];
}

/**
@@ -248,7 +252,7 @@ export class ApprovalController extends BaseController<ApprovalConfig, ApprovalS
this.reject(id, rejectionError);
}
this._origins.clear();
this.update(defaultState, true);
this.update(this.defaultState, true);
}

/**
@@ -359,10 +363,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);
}

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

const state = this.state[STORE_KEY];
const approvalState = this.state[APPROVALS_STORE_KEY];
const {
origin,
type = NO_TYPE,
} = state[id];
} = approvalState[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 newApprovalState = { ...approvalState };
delete newApprovalState[id];
this.update({
[STORE_KEY]: newState,
[APPROVALS_STORE_KEY]: newApprovalState,
[APPROVAL_COUNT_STORE_KEY]: this.state[APPROVAL_COUNT_STORE_KEY] - 1,
}, true);
}
}