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.

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

[base-controller] Fix getRestricted not inferring allowlists as empty, when they are omitted in both function and generic arguments #4033

Closed
MajorLift opened this issue Mar 7, 2024 · 0 comments · Fixed by #4035
Assignees
Labels
bug Something isn't working team-wallet-framework

Comments

@MajorLift
Copy link
Contributor

MajorLift commented Mar 7, 2024

References

Description

The issue is observed when calling the ControllerMessenger class method getRestricted.

Repro steps

  1. The allowlists are omitted in both the getRestricted generic and function params.
  2. The returned restricted controller-messenger is passed in directly as the messenger option in the controller constructor.

Following these steps prevents the Allowed{Action,Event} generic defaults (never) from being applied. Allowed{Action,Event} are inferred as their generic constraints instead (NotNamespacedBy<...>).

Currently known fixes

  1. Explicitly passing in the allowlists either as generic or function params.
  2. [unexpected behavior] Assigning the restricted messenger to a variable first before passing it into the controller constructor.

Objective

  • Find a fix that is internal to getRestricted or RestrictedControllerMessenger, and doesn't require the consumer to take actions at the call site.
  • As a fallback option, add an entry to changelog that describes this issue and conveys in detail the steps consumers must take to use getRestricted without experiencing unexpected behavior.

Details

Error

Example A

Error: Type 'RestrictedControllerMessenger<"NetworkController", NetworkControllerGetStateAction | NetworkControllerGetProviderConfigAction | ... 19 more ... | TransactionControllerGetStateAction, NetworkControllerStateChangeEvent | ... 20 more ... | TransactionControllerUnapprovedTransactionAddedEvent, "ApprovalController:getSta...' is not assignable to type 'NetworkControllerMessenger'.
Types of property '#allowedActions' are incompatible.
Type '("ApprovalController:getState" | "ApprovalController:clearRequests" | "ApprovalController:addRequest" | "ApprovalController:hasRequest" | "ApprovalController:acceptRequest" | ... 7 more ... | "TransactionController:getState")[]' is not assignable to type 'never[]'.
Type 'string' is not assignable to type 'never'.ts(2322)

  • NotNamespacedBy, NarrowToAllowed are working correctly.
  • There doesn't seem to be anything wrong with the generic params of getRestricted or RestrictedControllerMessenger constructor.
   const unrestrictedMessenger: UnrestrictedControllerMessenger =
     new ControllerMessenger();
   const networkController = new NetworkController({
     messenger: unrestrictedMessenger.getRestricted({
       name: 'NetworkController',
     }),

Example B

No error, but full allowlist is inferred despite being omitted from generic + function arguments.

In this case, the code is relying on this bug to implement a SelectedNetworkControllerMessenger while conveniently sidestepping the need to supply allowlists.

This is dangerous behavior (especially at runtime) that should not be possible. The code should be breaking from the lack of explicitly supplied function-param allowlists.

  • From SelectedNetworkMiddleware.test.ts:

🚫 (method) ControllerMessenger<SelectedNetworkControllerActions | AllowedActions, SelectedNetworkControllerStateChangeEvent | AllowedEvents>.getRestricted<"SelectedNetworkController", "NetworkController:getNetworkClientById" | "NetworkController:getState" | "PermissionController:hasPermissions" | "PermissionController:getSubjectNames", "NetworkController:stateChange" | "PermissionController:stateChange">({ name, allowedActions, allowedEvents, }: {
...;
}): RestrictedControllerMessenger<...>

const buildMessenger = () => {
  return new ControllerMessenger<
    SelectedNetworkControllerActions | AllowedActions,
    SelectedNetworkControllerEvents | AllowedEvents
  >();
};
...
const messenger = buildMessenger();
const middleware = createSelectedNetworkMiddleware(
  messenger.getRestricted({
    name: 'SelectedNetworkController',
  }),
);

✅ const restrictedMessenger: RestrictedControllerMessenger<"SelectedNetworkController", SelectedNetworkControllerGetSelectedNetworkStateAction | SelectedNetworkControllerGetNetworkClientIdForDomainAction | SelectedNetworkControllerSetNetworkClientIdForDomainAction, SelectedNetworkControllerStateChangeEvent, never, never>

...
const restrictedMessenger = messenger.getRestricted({
  name: 'SelectedNetworkController',
});
const middleware = createSelectedNetworkMiddleware(restrictedMessenger);

Example C

Same behavior as Example B once this diff is applied:

diff --git a/packages/gas-fee-controller/src/GasFeeController.test.ts b/packages/gas-fee-controller/src/GasFeeController.test.ts
index d198f4b98..62a692751 100644
--- a/packages/gas-fee-controller/src/GasFeeController.test.ts
+++ b/packages/gas-fee-controller/src/GasFeeController.test.ts
@@ -66,14 +66,10 @@ const setupNetworkController = async ({
   state: Partial<NetworkState>;
   clock: sinon.SinonFakeTimers;
 }) => {
-  const restrictedMessenger = unrestrictedMessenger.getRestricted({
-    name: 'NetworkController',
-    allowedActions: [],
-    allowedEvents: [],
-  });
-
   const networkController = new NetworkController({
-    messenger: restrictedMessenger,
+    messenger: unrestrictedMessenger.getRestricted({
+      name: 'NetworkController',
+    }),
     state,
     infuraProjectId: '123',
     trackMetaMetricsEvent: jest.fn(),

Fixes

  • Explicitly pass never as generic params
   const unrestrictedMessenger: UnrestrictedControllerMessenger =
     new ControllerMessenger();
-  const networkControllerMessenger = unrestrictedMessenger.getRestricted({
-    name: 'NetworkController',
-  });
   const networkController = new NetworkController({
-    messenger: networkControllerMessenger,
+    messenger: unrestrictedMessenger.getRestricted<
+      'NetworkController',
+      never,
+      never
+    >({
+      name: 'NetworkController',
+    }),
  • Explicitly pass empty array as function params
   const unrestrictedMessenger: UnrestrictedControllerMessenger =
     new ControllerMessenger();
-  const networkControllerMessenger = unrestrictedMessenger.getRestricted({
-    name: 'NetworkController',
-  });
   const networkController = new NetworkController({
-    messenger: networkControllerMessenger,
+    messenger: unrestrictedMessenger.getRestricted({
+      name: 'NetworkController',
+      allowedActions: [],
+      allowedEvents: [],
+    }),
  • Assign restricted messenger to its own variable first before passing into controller constructor
  const unrestrictedMessenger: UnrestrictedControllerMessenger =
    new ControllerMessenger();
+ const networkControllerMessenger = unrestrictedMessenger.getRestricted({
+   name: 'NetworkController',
+ });
  const networkController = new NetworkController({
-   messenger: unrestrictedMessenger.getRestricted({
-     name: 'NetworkController',
-   }),
+   messenger: networkControllerMessenger,
@MajorLift MajorLift added bug Something isn't working team-wallet-framework labels Mar 7, 2024
@MajorLift MajorLift self-assigned this Mar 7, 2024
@MajorLift MajorLift changed the title [base-controller] Fix getRestricted not inferring omitted allowed{Actions,Events} as never[] when used with unrestrictedControllerMessenger pattern [base-controller] Fix getRestricted not inferring allowlists as empty, when they are omitted in both function and generic arguments Mar 8, 2024
MajorLift added a commit that referenced this issue Mar 8, 2024
…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
MajorLift added a commit to MetaMask/metamask-mobile that referenced this issue Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working team-wallet-framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant