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] Controller constructors accept messengers with incomplete allowlists #4414

Open
MajorLift opened this issue Jun 12, 2024 · 0 comments
Assignees
Labels
bug Something isn't working team-wallet-framework

Comments

@MajorLift
Copy link
Contributor

MajorLift commented Jun 12, 2024

Problem

BaseControllerV2 and its subclass controllers currently accept messengers as constructor options even if they are defined with incomplete or empty action/event allowlists.

Type errors are raised when the omitted actions/events are invoked. Runtime errors are also thrown when the call, subscribe, unsubscribe methods are invoked (or any method that uses the #isAllowedAction, #isAllowedEvent methods). However, these errors are produced by the messenger, not the controller.

Controller constructors do correctly raise type errors for messengers that allow actions or events that are not included in the controller's allowlists.

Repro

/**
 * Controller accepts messenger with "empty" allowlists
 */

const emptyAllowlistControllerMessenger = 
  new ControllerMessenger<never, never>() // ControllerMessenger can be defined with any superset of the RestrictedControllerMessenger's empty allowlists
const emptyAllowlistMessenger = emptyAllowlistControllerMessenger.getRestricted({
    name: '',
    allowedActions: [],
    allowedEvents: [],
  })
  
const preferencesController = new PreferencesController({
  messenger: emptyAllowlistMessenger, // No type or runtime error!
})

/**
 * Controller accepts messenger with "incomplete" allowlists
 */

type TokenRatesControllerAllowedActions = 
  | PreferencesControllerGetStateAction 
  | NetworkControllerGetStateAction
  | NetworkControllerGetNetworkClientByIdAction 
  | TokensControllerGetStateAction
type TokenRatesControllerAllowedEvents = 
  | TokensControllerStateChangeEvent 
  | PreferencesControllerStateChangeEvent 
  | NetworkControllerStateChangeEvent

const incompleteAllowlistControllerMessenger = new ControllerMessenger<
  Exclude<TokenRatesControllerAllowedActions, NetworkControllerGetNetworkClientByIdAction>, 
  Exclude<TokenRatesControllerAllowedEvents, NetworkControllerGetStateAction>
>() // ControllerMessenger can be defined with any superset of the RestrictedControllerMessenger's incomplete allowlists
const incompleteAllowlistMessenger = incompleteAllowlistControllerMessenger.getRestricted({
    name: 'TokenRatesController',
    allowedActions: [
      // 'NetworkController:getNetworkClientById',
      'NetworkController:getState',
      'PreferencesController:getState',
      'TokensController:getState',
    ],
    allowedEvents: [
      // 'NetworkController:stateChange',
      'PreferencesController:stateChange',
      'TokensController:stateChange',
    ],
  })

const incompleteTokenRatesController = new TokenRatesController({
  messenger: incompleteAllowlistMessenger, // No type or runtime error!
  tokenPricesService: buildMockTokenPricesService(),
})

Playground link

Acceptance Criteria

Controllers inheriting from BaseControllerV2 must raise a type and/or runtime error if they are initialized with a messenger that does not explicitly allow ALL of the actions or events in the controller's allowlists.

References

@MajorLift MajorLift added bug Something isn't working team-wallet-framework labels Jun 12, 2024
@MajorLift MajorLift mentioned this issue Jun 12, 2024
3 tasks
@kanthesha kanthesha self-assigned this Jun 20, 2024
@kanthesha kanthesha removed their assignment Jun 28, 2024
@MajorLift MajorLift self-assigned this Jul 1, 2024
@MajorLift MajorLift changed the title [base-controller] Messengers with incomplete allowlists are accepted by controller constructors [base-controller] Controller constructors accept messengers with incomplete allowlists Jul 18, 2024
@MajorLift MajorLift mentioned this issue Jul 25, 2024
3 tasks
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

No branches or pull requests

2 participants