-
-
Notifications
You must be signed in to change notification settings - Fork 280
feat(wallet): wire ApprovalController into default initialization
#8953
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.
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
Changes from all commits
5ce2ab7
5db5ae5
5f9fd20
dd0c5ef
53fc74a
bf7de2b
5e15773
f32320c
d816a60
3a5030b
343634d
b9f3b1a
a9dad92
e533d67
ce8d061
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| import { ApprovalController } from '@metamask/approval-controller'; | ||
| import { ApprovalType } from '@metamask/controller-utils'; | ||
| import { Messenger } from '@metamask/messenger'; | ||
|
|
||
| import { defaultConfigurations } from '../../defaults'; | ||
| import type { | ||
| DefaultActions, | ||
| DefaultEvents, | ||
| RootMessenger, | ||
| } from '../../defaults'; | ||
| import { approvalController } from './approval-controller'; | ||
|
|
||
| /** | ||
| * Creates a root messenger for use in tests. | ||
| * | ||
| * @returns A root messenger. | ||
| */ | ||
| function getRootMessenger(): RootMessenger<DefaultActions, DefaultEvents> { | ||
| return new Messenger({ namespace: 'Root' }); | ||
| } | ||
|
|
||
| describe('approvalController', () => { | ||
| it('is registered as a default initialization configuration', () => { | ||
| // Proves the controller is part of the default ensemble that `initialize()` | ||
| // wires, without constructing a `Wallet` (which keeps this PR independent of | ||
| // the constructor-options shape). | ||
| expect(Object.values(defaultConfigurations)).toContain(approvalController); | ||
| }); | ||
|
|
||
| it('initializes an ApprovalController with default state', () => { | ||
| const messenger = approvalController.getMessenger(getRootMessenger()); | ||
|
|
||
| const instance = approvalController.init({ | ||
| state: undefined, | ||
| messenger, | ||
| options: {}, | ||
| }); | ||
|
|
||
| expect(instance).toBeInstanceOf(ApprovalController); | ||
| expect(instance.state).toStrictEqual({ | ||
| pendingApprovals: {}, | ||
| pendingApprovalCount: 0, | ||
| approvalFlows: [], | ||
| }); | ||
| }); | ||
|
|
||
| it('forwards the provided state to the controller', () => { | ||
| const messenger = approvalController.getMessenger(getRootMessenger()); | ||
|
|
||
| const instance = approvalController.init({ | ||
| state: { | ||
| pendingApprovals: {}, | ||
| pendingApprovalCount: 3, | ||
| approvalFlows: [], | ||
| }, | ||
| messenger, | ||
| options: {}, | ||
| }); | ||
|
|
||
| expect(instance.state.pendingApprovalCount).toBe(3); | ||
| }); | ||
|
|
||
| it('uses the provided showApprovalRequest callback', () => { | ||
| const messenger = approvalController.getMessenger(getRootMessenger()); | ||
| const showApprovalRequest = jest.fn(); | ||
|
|
||
| const instance = approvalController.init({ | ||
| state: undefined, | ||
| messenger, | ||
| options: { showApprovalRequest }, | ||
| }); | ||
|
|
||
| instance.startFlow(); | ||
|
|
||
| expect(showApprovalRequest).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('defaults showApprovalRequest to a no-op when omitted', () => { | ||
| const messenger = approvalController.getMessenger(getRootMessenger()); | ||
|
|
||
| const instance = approvalController.init({ | ||
| state: undefined, | ||
| messenger, | ||
| options: {}, | ||
| }); | ||
|
|
||
| expect(() => instance.startFlow()).not.toThrow(); | ||
| }); | ||
|
|
||
| // Pins the exact default exclusion set (independent of the source constant): | ||
| // each of these types must allow multiple pending requests from one origin. | ||
| // The pending promises never settle here; `.catch` only marks them handled. | ||
| it.each([ | ||
| ApprovalType.PersonalSign, | ||
| ApprovalType.EthSignTypedData, | ||
| ApprovalType.Transaction, | ||
| ApprovalType.WatchAsset, | ||
| ApprovalType.EthGetEncryptionPublicKey, | ||
| ApprovalType.EthDecrypt, | ||
| ])('excludes %s from rate limiting by default', (type) => { | ||
| const messenger = approvalController.getMessenger(getRootMessenger()); | ||
|
|
||
| const instance = approvalController.init({ | ||
| state: undefined, | ||
| messenger, | ||
| options: {}, | ||
| }); | ||
|
|
||
| instance.add({ origin: 'metamask.io', type }).catch(() => undefined); | ||
| instance.add({ origin: 'metamask.io', type }).catch(() => undefined); | ||
|
|
||
| // Both requests are queued rather than the second being rejected. | ||
| expect(instance.state.pendingApprovalCount).toBe(2); | ||
| }); | ||
|
|
||
| it('honors a custom typesExcludedFromRateLimiting list that overrides the default', () => { | ||
| const messenger = approvalController.getMessenger(getRootMessenger()); | ||
|
|
||
| const instance = approvalController.init({ | ||
| state: undefined, | ||
| messenger, | ||
| // Empty override: nothing is excluded, not even the default types. | ||
| options: { typesExcludedFromRateLimiting: [] }, | ||
| }); | ||
|
|
||
| instance | ||
| .add({ origin: 'metamask.io', type: ApprovalType.Transaction }) | ||
| .catch(() => undefined); | ||
|
|
||
| // A second request of the same origin and type is now rate-limited. | ||
| expect(() => | ||
| instance.add({ origin: 'metamask.io', type: ApprovalType.Transaction }), | ||
| ).toThrow('already pending'); | ||
| }); | ||
|
|
||
| it('exposes its actions through the root messenger', () => { | ||
| const rootMessenger = getRootMessenger(); | ||
| const messenger = approvalController.getMessenger(rootMessenger); | ||
|
|
||
| approvalController.init({ state: undefined, messenger, options: {} }); | ||
|
|
||
| expect(rootMessenger.call('ApprovalController:getState')).toStrictEqual({ | ||
| pendingApprovals: {}, | ||
| pendingApprovalCount: 0, | ||
| approvalFlows: [], | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { | ||
| ApprovalController, | ||
| ApprovalControllerMessenger, | ||
| } from '@metamask/approval-controller'; | ||
| import { ApprovalType } from '@metamask/controller-utils'; | ||
| import { Messenger } from '@metamask/messenger'; | ||
|
|
||
| import { InitializationConfiguration } from '../../types'; | ||
|
|
||
| /** | ||
| * Approval types that are exempt from per-origin rate limiting, so more than one | ||
| * request of the same type can be pending at once. These are the common EVM | ||
| * types: signing, transactions, watch-asset, and encryption. | ||
| * | ||
| * Clients can replace this list via | ||
| * `instanceOptions.approvalController.typesExcludedFromRateLimiting` — the | ||
| * extension and mobile pass their own. `snap_dialog` will be added here once the | ||
| * wallet wires `SnapController`. | ||
| */ | ||
| const DEFAULT_TYPES_EXCLUDED_FROM_RATE_LIMITING = [ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How did you pick these?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are the six EVM I used that as the platform-agnostic baseline and dropped the two client-specific string entries — the smart-transaction status type (whose value even differs between extension and mobile) and Happy to trim the default to just the intersection (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's have someone from the confirmations team sanity check that 👍
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks fine to me, as long as it's the superset of the two, we were meaning to align mobile with extension.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though I guess the problem with that would be the client specific smart TX string
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thought about this more and kept the default as the EVM baseline (no
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally we didn't have the divergence in the smart TX string, then we could use a superset and not have overrides. But up to @matthewwalsh0 whether he thinks that is worth the trouble right now.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can double-check both clients and remove in future once we know the superset is safe? |
||
| ApprovalType.PersonalSign, | ||
| ApprovalType.EthSignTypedData, | ||
| ApprovalType.Transaction, | ||
| ApprovalType.WatchAsset, | ||
| ApprovalType.EthGetEncryptionPublicKey, | ||
| ApprovalType.EthDecrypt, | ||
| ]; | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| export const approvalController: InitializationConfiguration< | ||
| ApprovalController, | ||
| ApprovalControllerMessenger | ||
| > = { | ||
| name: 'ApprovalController', | ||
| init: ({ state, messenger, options }) => | ||
| new ApprovalController({ | ||
| state, | ||
| messenger, | ||
| showApprovalRequest: | ||
| options.showApprovalRequest ?? ((): void => undefined), | ||
| typesExcludedFromRateLimiting: | ||
| options.typesExcludedFromRateLimiting ?? | ||
| DEFAULT_TYPES_EXCLUDED_FROM_RATE_LIMITING, | ||
| }), | ||
| getMessenger: (parent) => | ||
| new Messenger({ | ||
| namespace: 'ApprovalController', | ||
| parent, | ||
| }), | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import type { | ||
| ApprovalControllerOptions, | ||
| ShowApprovalRequest, | ||
| } from '@metamask/approval-controller'; | ||
|
|
||
| /** | ||
| * Per-instance options for the wallet's `ApprovalController`. Both fields are | ||
| * optional; see the controller's `init` for the defaults applied when omitted. | ||
| */ | ||
| export type ApprovalControllerInstanceOptions = { | ||
| /** | ||
| * Callback that surfaces a pending approval request to the user. Defaults to | ||
| * a no-op so the controller works headlessly. | ||
| */ | ||
| showApprovalRequest?: ShowApprovalRequest; | ||
| /** | ||
| * Approval types exempt from per-origin rate limiting. Defaults to a baseline | ||
| * of EVM approval types. | ||
| */ | ||
| typesExcludedFromRateLimiting?: NonNullable< | ||
| ApprovalControllerOptions['typesExcludedFromRateLimiting'] | ||
| >; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| export { approvalController } from './approval-controller/approval-controller'; | ||
| export { keyringController } from './keyring-controller'; | ||
| export { storageService } from './storage-service'; |
Uh oh!
There was an error while loading. Please reload this page.