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

[DNM] Preview branch of complete ComposableController fix with no type error suppression #10374

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Jul 23, 2024

Description

This PR is not intended to be merged into main, but to serve as a preview for a complete implementation of @metamask/composable-controller@8.0.0 that doesn't rely on type error suppression.

Some commits may be cherry-picked into a new PR -- especially the controller patches (the wallet framework team will be applying the same changes to the core controllers).

To access newer yarn resolutions features and yarn patch, git protocol syntax), the package.json packageManager field is set to yarn v3.

This change is the cause of the CI failure during the "setup" step, and will be reverted in any PR that follows.

Fixes

All ts-expect-error, ts-ignore directives are removed by fixing the following errors:

@metamask/base-controller version mismatch errors in controller instantiations

Type 'RestrictedControllerMessenger<"ExampleController", ..., ..., ..., ...>' is not assignable to type 'ExampleControllerMessenger'.
  Property '#private' in type 'RestrictedControllerMessenger' refers to a different member that cannot be accessed from within type 'RestrictedControllerMessenger'.ts(2322)
  • These are fixed by pinning the @metamask/base-controller version to 6.0.2 (latest) with yarn resolutions.
  • Controllers updated to a version that uses an up-to-date @metamask/base-controller version, (e.g. @metamask/approval-controller is bumped to its latest version).
  • Upstream fixes in source package (e.g. @metamask/ppom-validator is updated to use @metamask/base-controller@6.0.2).

"stateChange event is not assignable..."

Type '"ExampleController:stateChange"' is not assignable to type '"SnapController:snapBlocked" | "SnapController:snapInstalled" | "SnapController:snapUninstalled" | "SnapController:snapInstallStarted" | "SnapController:snapInstallFailed" | ... 31 more ... | "TokensController:stateChange"'. Did you mean '"TokenListController:stateChange"'?ts(2820)

This error is caused by one of the following:

  1. messagingSystem is defined as a private field.
  • The messagingSystem class field of BaseControllerV2 was defined as a private field in @metamask/controllers@6.1.1 (#377) and has been set to protected since @metamask/controllers@8.0.0 (#378) (these versions are from 3 years ago before the core monorepo existed).
  • These controllers need to be updated to a version using an up-to-date @metamask/base-controller.
  1. The stateChange event type for the controller is not defined and/or the controller's messenger is defined without its stateChange event type (and there are no newer releases where these issues have been fixed)
  • Define and export a stateChange event type using the ControllerStateChangeEvent generic type from @metamask/base-controller.
  • Append this type to the ExampleControllerEvents type union.
  • Append this type union to the type union in the Events field of the controller's messenger type.
  • Add controller's actions/events to the Global{Actions,Events} type unions in the Engine class.

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Copy link

socket-security bot commented Jul 23, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@metamask-previews/address-book-controller@5.0.0-preview-aaea6b5 Transitive: network +11 4.48 MB metamaskbot
npm/@metamask-previews/assets-controllers@37.0.0-preview-aaea6b5 Transitive: environment, network +36 19.9 MB metamaskbot
npm/@metamask-previews/composable-controller@7.0.0-preview-b42590f None +3 971 kB metamaskbot
npm/@metamask/json-rpc-engine@8.0.2 None 0 189 kB metamaskbot
npm/@metamask/notification-services-controller@0.1.2 network Transitive: environment, filesystem +71 111 MB metamaskbot
npm/@metamask/phishing-controller@10.1.1 network +5 1.94 MB metamaskbot
npm/@metamask/polling-controller@8.0.0 None 0 123 kB metamaskbot
npm/@metamask/profile-sync-controller@0.1.4 network Transitive: environment, filesystem +23 5.34 MB metamaskbot
npm/@metamask/snaps-controllers@9.4.0 Transitive: environment, network +16 5 MB metamaskbot
npm/concat-stream@1.6.2 None 0 9.56 kB mafintosh
npm/long@4.0.0 None 0 177 kB dcode
npm/type-fest@0.6.0 None 0 29.6 kB sindresorhus

🚮 Removed packages: npm/@contentful/rich-text-html-renderer@16.6.4, npm/@firebase/analytics-compat@0.2.11, npm/@firebase/analytics@0.10.5, npm/@firebase/app-check-compat@0.3.12, npm/@firebase/app-check@0.8.5, npm/@firebase/app-compat@0.2.36, npm/@firebase/app@0.10.6, npm/@firebase/auth-compat@0.5.10, npm/@firebase/auth@1.7.5, npm/@firebase/database-compat@1.0.6, npm/@firebase/database@1.0.6, npm/@firebase/firestore-compat@0.3.33, npm/@firebase/firestore@4.6.4, npm/@metamask-previews/composable-controller@7.0.0-preview-2ca9038, npm/@metamask/address-book-controller@4.0.1, npm/@metamask/assets-controllers@30.0.0, npm/@metamask/json-rpc-engine@9.0.1, npm/@metamask/notification-services-controller@0.1.1, npm/@metamask/phishing-controller@9.0.2, npm/@metamask/polling-controller@6.0.2, npm/@metamask/ppom-validator@0.32.0, npm/@metamask/profile-sync-controller@0.1.3, npm/@metamask/snaps-controllers@9.2.0, npm/await-semaphore@0.1.3, npm/builtin-modules@3.3.0, npm/eslint-compat-utils@0.1.2, npm/eslint-plugin-es-x@7.5.0, npm/eslint-plugin-n@16.6.2, npm/eslint-visitor-keys@3.4.1, npm/firebase@10.12.3, npm/is-builtin-module@3.2.1, npm/type-fest@4.18.3

View full report↗︎

@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch from 8630e58 to b1f33b3 Compare July 23, 2024 00:16
@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch 2 times, most recently from 202e843 to e2625a2 Compare July 23, 2024 22:11
@MajorLift MajorLift requested a review from a team July 24, 2024 14:16
@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch from 9ca17c7 to 1de86a9 Compare July 26, 2024 18:05
@MajorLift MajorLift changed the base branch from upgrade-yarn-to-v3 to jongsun/fix-composable-controller July 26, 2024 18:05
@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch 2 times, most recently from dd53c9a to 76ef6e3 Compare July 26, 2024 18:32
@MajorLift MajorLift changed the base branch from jongsun/fix-composable-controller to upgrade-yarn-to-v3 July 26, 2024 18:32
Copy link

socket-security bot commented Jul 26, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/@metamask/obs-store@9.0.0, npm/@metamask/safe-event-emitter@3.1.1

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch from 76ef6e3 to bbd680f Compare July 26, 2024 19:18
@MajorLift MajorLift changed the title fix: Broken functionality of ComposableController, ComposableControllerMessenger fix: Preview branch of complete ComposableController fix with no type error suppression Jul 28, 2024
@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch from b141caf to 40d2d42 Compare July 29, 2024 16:23
@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch from 40d2d42 to dfe27b9 Compare July 29, 2024 16:33
@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch from dfe27b9 to ca177f4 Compare July 29, 2024 16:46
@MajorLift MajorLift added DO-NOT-MERGE Pull requests that should not be merged No QA Needed Apply this label when your PR does not need any QA effort. labels Jul 29, 2024
@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch from ca177f4 to 0250b07 Compare July 31, 2024 23:47
@@ -120,7 +121,16 @@
"redux-persist-filesystem-storage/react-native-blob-util": "^0.19.9",
"xmldom": "npm:@xmldom/xmldom@0.7.13",
"@metamask/metamask-eth-abis": "3.1.1",
"react-native/ws": "^6.2.3"
"react-native/ws": "^6.2.3",
"@metamask/snaps-controllers@^9.3.1": "patch:@metamask/snaps-controllers@npm%3A9.3.1#./.yarn/patches/@metamask-snaps-controllers-npm-9.3.1-6a141c6563.patch",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repo already supports patches without yarnv3 via patch-package, FYI.

package.json Outdated
@@ -110,6 +110,7 @@
]
},
"resolutions": {
"@metamask/base-controller": "6.0.2",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resolution should work under yarnv1 too?

Copy link
Contributor Author

@MajorLift MajorLift Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work fully, unfortunately.

Many of the nested resolutions fail with the following error message (the v1 docs warn about this, and also say that resolutions is a "fairly new feature"):

warning Resolution field "@metamask/base-controller@6.0.2" is incompatible with requested version "@metamask/base-controller@^5.0.2"

As a result, the base-controller version mismatch errors we see in almost every controller instantiation in the Engine class are not fixed when using yarn v1 resolutions (repro branch vs. current branch).

Type 'RestrictedControllerMessenger<"ExampleController", ..., ..., ..., ...>' is not assignable to type 'ExampleControllerMessenger'.
  Property '#private' in type 'RestrictedControllerMessenger' refers to a different member that cannot be accessed from within type 'RestrictedControllerMessenger'.ts(2322)

Copy link
Contributor Author

@MajorLift MajorLift Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the newer git protocol syntax doesn't seem to be supported in v1, which was inconvenient.

I think I might have given the impression that I would be upgrading the yarn version in mobile with this PR, which isn't the case since this branch isn't intended to be merged into main. I should probably put that at the top of the PR.😅

@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch from 4e2ec3b to 36b966e Compare August 1, 2024 21:38
@MajorLift MajorLift force-pushed the bump-composable-controller-from-3.0.0-to-8.0.0 branch from fe6b8f8 to 9e2449e Compare August 1, 2024 22:25
Comment on lines +1505 to +1520
Controllers[Exclude<keyof Controllers, keyof NonControllers>]
>({
controllers,
// @ts-expect-error Resolve/patch mismatch between base-controller versions
// ts(2322) - Property '#private' in type 'RestrictedControllerMessenger' refers to a different member that cannot be accessed from within type 'RestrictedControllerMessenger'
controllers: controllers.filter(
(
controller,
): controller is Controllers[Exclude<
keyof Controllers,
keyof NonControllers
>] =>
'state' in controller &&
controller.state !== undefined &&
Object.keys(controller.state).length > 0,
),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ComposableController now raises a type error if a non-controller lacking a state property is passed into the ChildControllers generic parameter or the controllers constructor option.

Here, I've assigned the responsibility of ensuring correct input to the call site. This is out of a preference for explicit code and less "magical" behavior on part of the ComposableController.

However, if this implementation seems too cumbersome, I could definitely look into implicitly handling this in the ComposableController constructor.

TypeScript v5.5 introduces inferred type predicates, which should make this much less verbose, so waiting for that is also an option.

Comment on lines 346 to 363
export type EngineState = {
AccountTrackerController: AccountTrackerState;
AddressBookController: AddressBookState;
AssetsContractController: BaseState;
NftController: NftState;
AccountTrackerController: AccountTrackerControllerState;
AddressBookController: AddressBookControllerState;
NftController: NftControllerState;
TokenListController: TokenListState;
CurrencyRateController: CurrencyRateState;
KeyringController: KeyringControllerState;
NetworkController: NetworkState;
PreferencesController: PreferencesState;
PhishingController: PhishingControllerState;
TokenBalancesController: TokenBalancesControllerState;
TokenRatesController: TokenRatesState;
TokenRatesController: TokenRatesControllerState;
TransactionController: TransactionControllerState;
SmartTransactionsController: SmartTransactionsControllerState;
SwapsController: SwapsState;
GasFeeController: GasFeeState;
TokensController: TokensState;
TokenDetectionController: BaseState;
NftDetectionController: BaseState;
TokensController: TokensControllerState;
///: BEGIN:ONLY_INCLUDE_IF(preinstalled-snaps,external-snaps)
Copy link
Contributor Author

@MajorLift MajorLift Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stateless non-controllers probably should be removed from EngineState:

  • These non-controllers do not have empty state objects or BaseState -- they lack a state property entirely.
  • Non-controllers can still be included in Controllers type, and any information needed by the Engine class can be derived from the full controller class stored in Controllers.

Edit:

In the interest of minimizing invasive changes and unnecessary diffs, I've reverted this commit for now and opted to exclude the non-controllers when EngineState is passed into ComposableController as a generic argument instead.

@MajorLift MajorLift changed the title [DO-NOT-MERGE] Preview branch of complete ComposableController fix with type error suppression removed [DO-NOT-MERGE] Preview branch of complete ComposableController fix with no type error suppression Aug 2, 2024
@MajorLift MajorLift changed the title [DO-NOT-MERGE] Preview branch of complete ComposableController fix with no type error suppression [DNM] Preview branch of complete ComposableController fix with no type error suppression Aug 2, 2024
MajorLift added a commit to MetaMask/core that referenced this pull request Aug 23, 2024
#4633)

## Explanation

Fixes controllers and messengers in the core repo that do not fulfill
their intended specifications correctly:

- [Define the `*:getState` action using the `ControllerGetStateAction`
utility
type](https://github.com/MetaMask/core/blob/add-controller-guidelines/docs/writing-controllers.md#define-the-getstate-action-using-the-controllergetstateaction-utility-type)
- [Define the `*:stateChange` event using the
`ControllerStateChangeEvent` utility
type](https://github.com/MetaMask/core/blob/add-controller-guidelines/docs/writing-controllers.md#define-the-statechange-event-using-the-controllerstatechangeevent-utility-type)
- [Define and export a type union for internal action
types](https://github.com/MetaMask/core/blob/add-controller-guidelines/docs/writing-controllers.md#define-and-export-a-type-union-for-internal-action-types)
- [Define and export a type union for internal event
types](https://github.com/MetaMask/core/blob/add-controller-guidelines/docs/writing-controllers.md#define-and-export-a-type-union-for-internal-event-types)
- [Define and export a type for the controller's
messenger](https://github.com/MetaMask/core/blob/add-controller-guidelines/docs/writing-controllers.md#define-and-export-a-type-for-the-controllers-messenger)

This also resolves downstream errors in mobile caused by
`composable-controller` expecting all of its child controllers to have a
`stateChange` event.
- See MetaMask/metamask-mobile#10374

## References

- Fixes #4579
- Blocks MetaMask/metamask-mobile#10441

## Changelog

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

```md
### Added

- Define and export new types: `LoggingControllerGetStateAction`, `LoggingControllerStateChangeEvent`, `LoggingControllerEvents` ([#4633](#4633))

### Changed

- **BREAKING:** `LoggingControllerMessenger` must allow internal events defined in the `LoggingControllerEvents` type ([#4633](#4633))
- `LoggingControllerActions` is widened to include the `LoggingController:getState` action ([#4633](#4633))
```

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

```md
### Added

- Define and export new types: `PhishingControllerGetStateAction`, `PhishingControllerStateChangeEvent`, `PhishingControllerEvents` ([#4633](#4633))

### Changed

- **BREAKING:** `PhishingControllerMessenger` must allow internal events defined in the `PhishingControllerEvents` type ([#4633](#4633))
- `PhishingControllerActions` is widened to include the `PhishingController:getState` action ([#4633](#4633))
```

### `@metamask/notification-services-controller` (minor)

```md
### Added

- Define and export new type `NotificationServicesControllerGetStateAction` ([#4633](#4633))

### Fixed

- Replace `getState` action in `NotificationServicesControllerActions` with correctly-defined `NotificationServicesControllerGetStateAction` type ([#4633](#4633))
```

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

```md
### Added

- Define and export new types: `AuthenticationControllerGetStateAction`, `AuthenticationControllerStateChangeEvent`, `Events` ([#4633](#4633))

### Changed

- **BREAKING:** `AuthenticationControllerMessenger` must allow internal events defined in the `Events` type ([#4633](#4633))
- `AuthenticationControllerActions` is widened to include the `AuthenticationController:getState` action ([#4633](#4633))
```

### `@metamask/user-storage-controller` (major)

```md
### Added

- Define and export new types: `UserStorageControllerGetStateAction`, `UserStorageControllerStateChangeEvent`, `Events` ([#4633](#4633))

### Changed

- **BREAKING:** `UserStorageControllerMessenger` must allow internal events defined in the `Events` type ([#4633](#4633))
- `UserStorageControllerActions` is widened to include the `UserStorageController:getState` action ([#4633](#4633))
```

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

This PR has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions.

@github-actions github-actions bot added the stale Issues that have not had activity in the last 90 days label Oct 31, 2024
Copy link
Contributor

github-actions bot commented Nov 7, 2024

This PR was closed because there has been no follow up activity in 7 days. Thank you for your contributions.

@github-actions github-actions bot closed this Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO-NOT-MERGE Pull requests that should not be merged No QA Needed Apply this label when your PR does not need any QA effort. stale Issues that have not had activity in the last 90 days team-wallet-framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants