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

[composable-controller] Implement type validation of constructor options #4213

Open
MajorLift opened this issue Apr 25, 2024 · 5 comments
Open
Labels
bug Something isn't working team-wallet-framework

Comments

@MajorLift
Copy link
Contributor

MajorLift commented Apr 25, 2024

Requirements

When initializing a ComposableController class, given a ComposableControllerState type:

  1. A type error should be raised if the list of child controllers specified in the controllers array constructor option does not exactly match the list of controllers included in the ComposableControllerState type.

  2. A type error should be raised if the controller-messenger instance passed into the messenger constructor option has an event allowlist that does not include all of the stateChange events for the list of controllers in the ComposableControllerState type.

References

@desi
Copy link
Contributor

desi commented Apr 25, 2024

@MajorLift Can you update the ticket to include the solutions you have tried for number 2 above and the roadblocks you hit so that others have that context?

@desi
Copy link
Contributor

desi commented Apr 25, 2024

Seems like we probably need some spikes to figure out how to solve or approach. So waiting to estimate until we have a chance to do that.

@MajorLift
Copy link
Contributor Author

MajorLift commented May 1, 2024

  • controllers:

    • Validating with a union type consisting of the child controllers allows controllers lists that are incomplete.
    • Validating with a tuple type consisting of the child controllers forces us to pass in a controllers list that matches the order of the tuple type, not just the contents.
      • Because the tuple type is likely to be derived from a union type, its order cannot necessarily be determined based on the order of controllers presented in the ComposableControllerState type.
  • messenger:

    • Given an event allowlist T, and U s.t. U <: T, TypeScript does not recognize that RestrictedControllerMessenger<..., U> <: RestrictedControllerMessenger<..., T>.
    • Wrap class constructor into a factory function that takes the event allowlist as a parameter, and validate that parameter?

@MajorLift
Copy link
Contributor Author

Following up on the last bullet point in the above comment, the wallet framework proposal also suggests refactoring the ComposableController into a compose function that returns a class instance that composes the input child controllers.

This should make it easier to validate the input params of the compose function, as we can let TypeScript use the input arguments as sources of inference for generic params, and then apply generic constraints.

Here's an example of this concept in action.

@MajorLift MajorLift added the bug Something isn't working label Jun 12, 2024
MajorLift added a commit that referenced this issue Aug 20, 2024
… safeguards (#4467)

## Overview

This commit fixes issues with the `ComposableController` class's
interface, and its logic for validating V1 and V2 controllers.

These changes will enable `ComposableController` to function correctly
downstream in the Mobile Engine, and eventually the Wallet Framework
POC.

## Explanation

The previous approach of generating mock controller classes from the
`ComposableControllerState` input using the `GetChildControllers` was
flawed, because the mock controllers would always be incomplete,
complicating attempts at validation.

Instead, we now rely on the downstream consumer to provide both a
composed type of state schemas (`ComposableControllerState`) and a type
union of the child controller instances (`ChildControllers`). For
example, in mobile, we can use (with some adjustments) `EngineState` for
the former, and `Controllers[keyof Controllers]` for the latter.

The validation logic for V1 controllers has also been updated. Due to
breaking changes made to the private properties of `BaseControllerV1`
(#3959), mobile V1 controllers
relying on versions prior to these changes were introduced were
incompatible with the up-to-date `BaseControllerV1` version that the
composable-controller package references.

In this commit, the validator type `BaseControllerV1Instance` filters
out the problematic private properties by using the `PublicInterface`
type. Because the public API of `BaseControllerV1` has been relatively
constant, this removes the type errors that previously occurred in
mobile when passing V1 controllers into `ComposableController`.

## References

- Closes #4448
- Contributes to #4213
- Next steps MetaMask/metamask-mobile#10073
  - See MetaMask/metamask-mobile#10011

## Changelog

### `@metamask/composable-controller` (major)

### Changed

- **BREAKING:** Add two required generic parameters to the
`ComposableController` class: `ComposedControllerState` (constrained by
`LegacyComposableControllerStateConstraint`) and `ChildControllers`
(constrained by `ControllerInstance`)
([#4467](#4467)).
- **BREAKING:** The type guard `isBaseController` now validates that the
input has an object-type property named `metadata` in addition to its
existing checks.
- **BREAKING:** The type guard `isBaseControllerV1` now validates that
the input has object-type properties `config`, `state`, and
function-type property `subscribe`, in addition to its existing checks.
- **BREAKING:** Narrow `LegacyControllerStateConstraint` type from
`BaseState | StateConstraint` to `BaseState & object | StateConstraint`.
- Add an optional generic parameter `ControllerName` to the
`RestrictedControllerMessengerConstraint` type, which extends `string`
and defaults to `string`.
 
### Fixed

- **BREAKING:** The `ComposableController` class raises a type error if
a non-controller with no `state` property is passed into the
`ChildControllers` generic parameter or the `controllers` constructor
option.
- Previously, a runtime error was thrown at class instantiation with no
type-level enforcement.
- When the `ComposableController` class is instantiated, its messenger
now attempts to subscribe to all child controller `stateChange` events
that are included in the messenger's events allowlist.
- This was always the expected behavior, but a bug introduced in
`@metamask/[email protected]` caused `stateChange` event
subscriptions to fail.
- `isBaseController` and `isBaseControllerV1` no longer return false
negatives.
- The `instanceof` operator is no longer used to validate that the input
is a subclass of `BaseController` or `BaseControllerV1`.
- The `ChildControllerStateChangeEvents` type checks that the child
controller's state extends from the `StateConstraintV1` type instead of
from `Record<string, unknown>`.
([#4467](#4467))
- V1 controllers define their state types using the `interface` keyword,
which are incompatible with `Record<string, unknown>` by default. This
resulted in `ChildControllerStateChangeEvents` failing to generate
`stateChange` events for V1 controllers and returning `never`.

## 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
Copy link
Contributor Author

MajorLift commented Sep 11, 2024

Priority analysis:

  • If ComposableController is only used internally: Nice-to-have.
  • If ComposableController is included in the Wallet Framework: Should at least aim for partial improvements.

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