fix: Apply major ComposableController API updates that fix broken functionality
#10441
Conversation
|
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. |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@metamask/[email protected] |
MajorLift
force-pushed
the
jongsun/fix-composable-controller
branch
6 times, most recently
from
815f836
to
992908f
Compare
MajorLift
added
the
No QA Needed
MajorLift
force-pushed
the
jongsun/fix-composable-controller
branch
3 times, most recently
from
6209b1f
to
a519205
Compare
app/core/Engine.ts
Outdated
| this.datamodel = new ComposableController< | ||
| EngineState, | ||
| Controllers[keyof Controllers] | ||
| >({ | ||
| controllers, |
There was a problem hiding this comment.
The controllers argument no longer requires a ts-expect-error directive, because its type is supplied via generic argument from the source-of-truth for the controllers array (i.e. Controllers).
app/core/Engine.ts
Outdated
| // @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' | ||
| messenger: this.controllerMessenger.getRestricted({ |
There was a problem hiding this comment.
This ts-expect-error directive is required due to the @metamask/base-controller version mismatch issue that is affecting most of the controller instantiations in the Engine class.
I've put up a preview branch that demonstrates that pinning base-controller to the latest version using yarn resolutions resolves this issue: #10374.
In practice, this directive will be removed as the controller versions in mobile are upgraded.
app/core/Engine.ts
Outdated
| /** | ||
| * V1/V2 controllers incorrectly defined with a `messagingSystem` that is missing its `stateChange` event. | ||
| * ! These `stateChange` events must be included in the datamodel's events allowlist. | ||
| * TODO: Upstream fixes in the source packages are required for the following controllers. | ||
| */ | ||
| // @ts-expect-error BaseControllerV2, messenger defined without `stateChange` event type | ||
| 'AuthenticationController:stateChange', | ||
| // @ts-expect-error BaseControllerV2, messenger defined without `stateChange` event type | ||
| 'LoggingController:stateChange', | ||
| // @ts-expect-error BaseControllerV1, has `messagingSystem` but as private field, messenger defined without `stateChange` event type | ||
| 'NftController:stateChange', | ||
| // @ts-expect-error BaseControllerV2, messenger defined without `stateChange` event type | ||
| 'NotificationServicesController:stateChange', | ||
| // @ts-expect-error BaseControllerV2, messenger defined without `stateChange` event type | ||
| 'PhishingController:stateChange', | ||
| // @ts-expect-error BaseControllerV2, messenger defined without `stateChange` event type | ||
| 'PPOMController:stateChange', | ||
| // @ts-expect-error BaseControllerV2, messenger defined without `stateChange` event type | ||
| 'SnapsRegistry:stateChange', | ||
| // @ts-expect-error BaseControllerV2, `TokenBalancesControllerState` import error | ||
| 'TokenBalancesController:stateChange', | ||
| // @ts-expect-error BaseControllerV2, messenger defined without `stateChange` event type | ||
| 'UserStorageController:stateChange', |
There was a problem hiding this comment.
These are stateChange events that need to be included in this allowlist, but are throwing errors because of misdefined controllers or messenger typing in their upstream source packages.
I have documented the causes in the comments, and the preview branch demonstrates working fixes for these that use yarn patches.
The wallet framework team has created a ticket to address these issues: MetaMask/core#4579. While the updated controllers are being released, the yarn patch fixes could also be cherry-picked from the preview branch.
Here, I have applied ts-expect-error directives to ensure that the ComposableControllerMessenger is defined with the correct allowlist at runtime, even if the associated types are not yet fully defined.
app/core/Engine.ts
Outdated
| /** | ||
| * V1 controllers that should be excluded from the datamodel's events allowlist for now. | ||
| * TODO: Each of these events should be added to the allowlist once its controller is migrated to V2. | ||
| */ | ||
| // 'AccountTrackerController:stateChange', // StaticIntervalPollingControllerV1, no `messagingSystem` | ||
| // 'AddressBookController:stateChange', // BaseControllerV1, no `messagingSystem` | ||
| // 'SmartTransactionsController:stateChange', // StaticIntervalPollingControllerV1, no `messagingSystem` | ||
| // 'SwapsController:stateChange', // BaseControllerV1, no `messagingSystem` | ||
| // 'TokenRatesController:stateChange', // StaticIntervalPollingControllerV1, no `messagingSystem` |
There was a problem hiding this comment.
These are stateChange events that eventually need to be included in this allowlist, but are blocked by their controllers lacking a messagingSystem.
These controllers have been upgraded to V2 or are currently in the process of being upgraded. These events should be uncommented as mobile migrates to using the upgraded V2 versions.
The end result of these changes can be seen in the preview branch: https://github.com/MetaMask/metamask-mobile/pull/10374/files#diff-1a656ef42f595bb9c6c1251b29808fd70d31da7a3e1a6e9be85574ad866dc1b0R1508-R1536
| // TODO: Replace "any" with type | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| datamodel: any; | ||
| datamodel: ComposableController<EngineState, Controllers[keyof Controllers]>; |
There was a problem hiding this comment.
This was causing implicit any types to spread throughout the file. See below.
| currencyRates: Object.fromEntries( | ||
| Object.entries(CurrencyRateController.currencyRates).map(([k, v]) => [ | ||
| k, | ||
| { ...v, conversionRate: v.conversionRate ?? 0 }, | ||
| ]), | ||
| ), |
There was a problem hiding this comment.
This CurrencyRateController object has been implicitly typed as any since this year-old change, along with all of the state objects destructured in the preceding lines (https://github.com/MetaMask/metamask-mobile/pull/10441/files#diff-1a656ef42f595bb9c6c1251b29808fd70d31da7a3e1a6e9be85574ad866dc1b0R2001-R2033).
Because of this, when the CurrencyRateController class's state type was significantly altered in a breaking change introduced by @metamask/[email protected], the resulting type error was suppressed, causing this runtime bug to go undetected.
The type error is now restored, as a side effect of the any typing of the datamodel class field being fixed.
Previously, there was only one top-level conversionRate property in the CurrencyRateController object that was being modified. I have fixed the logic so that all nested conversionRate properties for all currencies are normalized to 0 if they have a null value. Please let me know if this isn't the expected behavior.
There was a problem hiding this comment.
I added a test for this change, but sonarcloud is still complaining that lines 2099 and 2101 are uncovered: e107dc3
There was a problem hiding this comment.
Sonarcloud issue fixed here: 0b70f89
| // Interfaces are incompatible with our controllers and data types by default. | ||
| // Adding an index signature fixes this, but at the cost of widening the type unnecessarily. | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-definitions | ||
| export type EngineState = { |
There was a problem hiding this comment.
MajorLift
force-pushed
the
jongsun/fix-composable-controller
branch
2 times, most recently
from
f501427
to
2c0475a
Compare
MajorLift
force-pushed
the
jongsun/fix-composable-controller
branch
2 times, most recently
from
8e8fc98
to
2a010d3
Compare
MajorLift
force-pushed
the
jongsun/fix-composable-controller
branch
from
908c281
to
73a608b
Compare
…e` entry in composable controller event allowlist
|
|
@MajorLift Is this PR still relevant? It looks like maybe, but it has also been open for a while I'm noticing, so I'm not sure if it needs to be updated or something. |
Yes still relevant I am taking over this work |
|
…n Global{Actions,Events}, ComposableController event allowlist
MajorLift
added
Run Smoke E2E
|
MajorLift
force-pushed
the
jongsun/fix-composable-controller
branch
from
49f4ae0
to
24d9936
Compare
MajorLift
added
Run Smoke E2E
|
…ler` actions and events
MajorLift
added
Run Smoke E2E
|
Description
This commit updates
@metamask/composable-controllerto^9.0.0.This involves fixing bugs outlined in #10073, and applying the major changes to the
ComposableControllerAPI that has accumulated between the intervening versions.Blocked by
ComposableControllerupgrade to enable handling of non-controllers: [composable-controller] Enable handling of input that includes non-controllers with empty state core#4444Changelog
Changed
@metamask/composable-controllerfrom^3.0.0to^9.0.0(#10441)ComposableControllerclass constructor optionmessengerwith aRestrictedControllerMessengerinstance derived from thecontrollerMessengerclass field instance ofEngine, instead of passing in thecontrollerMessengerinstance directly.messengerinstance passed intoComposableControllerconstructor fromGlobalActions['type']to an empty union.messengerinstance passed intoComposableControllerconstructor fromGlobalEvents['type']to a union of thestateChangeevents of all controllers included in theEngineStatetype.EngineStateinterface to use type alias syntax to ensure compatibility with types used in MetaMask controllers (#10441)Fixed
Engineclassdatamodelfield fromanytoComposableController<EngineState, Controllers[keyof Controllers]>(#10441)Engineclassstategetter method now normalizesnullinto 0 for theconversionRatevalues of all native currencies keyed in thecurrencyRatesobject ofCurrencyRatesControllerState(#10441)EngineclasscontrollerMessengerso that the its allowlists include all internal actions and events of the controllers defined inEngineState(#10441)ComposableControllercan be instantiated with any collection of child controller instances, regardless of theBaseControllerpackage version the child controller inherits from (#10441)BaseControllerV1versions.controllersconstructor option.Related issues
ComposableControllerinstantiation and functionality is broken #10073@metamask/[email protected]release.@metamask/composable-controllerfrom^3.0.0to^6.0.2#10011Manual testing steps
Screenshots/Recordings
Pre-merge author checklist
Pre-merge reviewer checklist