Skip to content

Commit

Permalink
[base-controller] Make allowed{Actions,Events} required parameters …
Browse files Browse the repository at this point in the history
…of `getRestricted` (#4035)

## Explanation

This commit implements a fundamental solution to aligning runtime and
type-level behavior for `getRestricted`: removing the option to omit its
function parameters altogether.

### Impact

- This makes the expected runtime behavior of the method explicit and
predictable.
- This completely prevents type inference from deriving ambiguous
conclusions from the omission of function or generic parameters, or
otherwise exhibiting unexpected behavior.

The downsides of this approach are a bit more redundant code and
inconvenience, but having to write an extra empty array or two doesn't
seem like a terrible cost for getting unambiguous, explicit behavior.

### Caveat

Note that this solution does not directly resolve the observed bug that
motivated this ticket. It does make the bug irrelevant for
`getRestricted`, because passing in both allowlist params fixes the
issue, but in general, there may still be cases where generic default
arguments fail to apply.

See #4033 for repro steps and more details on this bug.

### Overview

- This change is implemented entirely in this commit:
9988d18
- All of the other diffs in the test files are simply applying this
breaking change.

## References

- Closes #4033

## Changelog

### `@metamask/base-controller`

#### Changed

- **BREAKING:** The `getRestricted` method of the `ControllerMessenger`
class now expects both `allowedActions` and `allowedEvents` as required
parameters.

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
  • Loading branch information
MajorLift authored Mar 8, 2024
1 parent 4e03239 commit 7ccd8e9
Show file tree
Hide file tree
Showing 25 changed files with 166 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ function getRestrictedMessenger() {
>();
return controllerMessenger.getRestricted({
name,
allowedActions: [],
allowedEvents: [],
});
}
const allAnnouncements: AnnouncementMap = {
Expand Down
15 changes: 11 additions & 4 deletions packages/approval-controller/src/ApprovalController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import type {
AddApprovalOptions,
ApprovalControllerActions,
ApprovalControllerEvents,
ApprovalControllerMessenger,
ErrorOptions,
StartFlowOptions,
SuccessOptions,
Expand Down Expand Up @@ -233,6 +232,8 @@ function getRestrictedMessenger() {
>();
const messenger = controllerMessenger.getRestricted({
name: 'ApprovalController',
allowedActions: [],
allowedEvents: [],
});
return messenger;
}
Expand Down Expand Up @@ -1147,7 +1148,9 @@ describe('approval controller', () => {
approvalController = new ApprovalController({
messenger: messenger.getRestricted({
name: controllerName,
}) as ApprovalControllerMessenger,
allowedActions: [],
allowedEvents: [],
}),
showApprovalRequest,
});

Expand All @@ -1169,7 +1172,9 @@ describe('approval controller', () => {
approvalController = new ApprovalController({
messenger: messenger.getRestricted({
name: controllerName,
}) as ApprovalControllerMessenger,
allowedActions: [],
allowedEvents: [],
}),
showApprovalRequest,
});

Expand All @@ -1191,7 +1196,9 @@ describe('approval controller', () => {
approvalController = new ApprovalController({
messenger: messenger.getRestricted({
name: controllerName,
}) as ApprovalControllerMessenger,
allowedActions: [],
allowedEvents: [],
}),
showApprovalRequest,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ async function setupAssetContractControllers() {
const messenger: NetworkControllerMessenger =
new ControllerMessenger().getRestricted({
name: 'NetworkController',
allowedActions: [],
allowedEvents: [],
});
const network = new NetworkController({
infuraProjectId: networkClientConfiguration.infuraProjectId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ function getRestrictedMessenger() {
>({
name,
allowedActions: ['NetworkController:getNetworkClientById'],
allowedEvents: [],
});
return messenger;
}
Expand Down
3 changes: 3 additions & 0 deletions packages/assets-controllers/src/NftController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ function setupController(

const approvalControllerMessenger = messenger.getRestricted({
name: 'ApprovalController',
allowedActions: [],
allowedEvents: [],
});

const approvalController = new ApprovalController({
Expand Down Expand Up @@ -163,6 +165,7 @@ function setupController(
>({
name: controllerName,
allowedActions: ['ApprovalController:addRequest'],
allowedEvents: [],
});

const preferencesStateChangeListeners: ((state: PreferencesState) => void)[] =
Expand Down
2 changes: 2 additions & 0 deletions packages/assets-controllers/src/TokensController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ describe('TokensController', () => {

approvalControllerMessenger = messenger.getRestricted({
name: 'ApprovalController',
allowedActions: [],
allowedEvents: [],
});

tokensControllerMessenger = messenger.getRestricted<
Expand Down
6 changes: 6 additions & 0 deletions packages/base-controller/src/BaseControllerV2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ function getCountMessenger(
}
return controllerMessenger.getRestricted({
name: countControllerName,
allowedActions: [],
allowedEvents: [],
});
}

Expand Down Expand Up @@ -147,6 +149,8 @@ function getMessagesMessenger(
}
return controllerMessenger.getRestricted({
name: messagesControllerName,
allowedActions: [],
allowedEvents: [],
});
}

Expand Down Expand Up @@ -1110,6 +1114,8 @@ describe('getPersistentState', () => {
>();
const visitorControllerMessenger = controllerMessenger.getRestricted({
name: visitorName,
allowedActions: [],
allowedEvents: [],
});
const visitorController = new VisitorController(
visitorControllerMessenger,
Expand Down
8 changes: 4 additions & 4 deletions packages/base-controller/src/ControllerMessenger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -430,15 +430,15 @@ export class ControllerMessenger<
AllowedEvent extends NotNamespacedBy<Namespace, Event['type']> = never,
>({
name,
allowedActions = [],
allowedEvents = [],
allowedActions,
allowedEvents,
}: {
name: Namespace;
allowedActions?: NotNamespacedBy<
allowedActions: NotNamespacedBy<
Namespace,
Extract<Action['type'], AllowedAction>
>[];
allowedEvents?: NotNamespacedBy<
allowedEvents: NotNamespacedBy<
Namespace,
Extract<Event['type'], AllowedEvent>
>[];
Expand Down
Loading

0 comments on commit 7ccd8e9

Please sign in to comment.