Use new assets state in TransactionPayController (behind feature flag)#8163
Open
Use new assets state in TransactionPayController (behind feature flag)#8163
Conversation
bergarces
force-pushed
the
transaction-pay-controller-new-assets-state
branch
from
1a70549 to
1e09ec5
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Contributor
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Contributor
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
bergarces
commented
Mar 11, 2026
Contributor
Author
There was a problem hiding this comment.
Once the feature flag is gone for good. We will come here to remove the old references to assets-controllers and remove the branching logic that the feature flag requires.
bergarces
changed the title
salimtb
reviewed
Mar 11, 2026
| */ | ||
| export function getAssetsUnifyStateFeature( | ||
| messenger: TransactionPayControllerMessenger, | ||
| ): boolean { |
Contributor
There was a problem hiding this comment.
QQ if we want to test this , what is the way to turn this to true ?
Contributor
There was a problem hiding this comment.
Currently only controlled by remote feature flags, unsure how much work would be required if we want to force to true.
Contributor
Author
There was a problem hiding this comment.
By doing black magic cjs updates in the client.
Contributor
Author
There was a problem hiding this comment.
With that said, if that cjs change is done with a patch and directly at the feature flag controller, it can be used to test selector changes too.
salimtb
previously approved these changes
Mar 11, 2026
Contributor
salimtb
left a comment
salimtb
left a comment
There was a problem hiding this comment.
the changes LGTM and works as expected on the local build
Prithpal-Sooriya
previously approved these changes
Mar 11, 2026
bergarces
dismissed stale reviews from Prithpal-Sooriya and salimtb
via
e2f2a16
packages/transaction-pay-controller/src/utils/feature-flags.test.ts
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
matthewwalsh0
requested changes
Mar 12, 2026
| * @param minRequiredVersion - The minimum required version. | ||
| * @returns True if the app version satisfies the minimum required version, false otherwise. | ||
| */ | ||
| function hasMinimumRequiredVersion( |
Member
There was a problem hiding this comment.
If this is a new feature flag, can this not be achieved by the version support in RemoteFeatureFlagController, so you just add a versions property to the root of the feature flag value in LaunchDarkly?
Contributor
Author
There was a problem hiding this comment.
I'll look into this. I'm all for letting feature flag controller handle the version logic.
Contributor
Author
There was a problem hiding this comment.
Done. It's now using the built-in versioning.
| * @param messenger - Controller messenger. | ||
| * @returns True if the assets unify state feature is enabled, false otherwise. | ||
| */ | ||
| export function getAssetsUnifyStateFeature( |
Member
There was a problem hiding this comment.
Why do we need to pivot on a feature flag at this point?
Could we not test the new controller for a time with standard client flows, and refactor this in one go when stable?
I'm naturally concerned given the risk to key in-development flows like MUSD, Perps, and Predict.
Contributor
Author
There was a problem hiding this comment.
We cannot migrate in one go. There is a very complex network of selectors using other selectors that depend on our state, and we want to be able to work in manageable PR sizes.
What we have done so far is create a root selector for every single legacy piece of state and make it return the old state if the flag is off, and the new state in the shape of the old if the flag is on. We have hunted every single reference to legacy stated and replaced it with those selectors (in extension). We are now doing the same for the two controllers that fetch from those controller states as well.
Once that is done, we can safely turn the feature flag on in dev for QA to test. Once we are confident there are no regressions we can then start turning it on in other environments.
Not only that, but before we turn it on in dev we need to ensure all fixtures and tests that use legacy state also have the new controller state, otherwise those tests will fail.
This would have been very difficult to achieve in one PR without feature flags.
| let getAccountsByChainId; | ||
| if (assetsUnifyStateFeatureEnabled) { | ||
| const assetsControllerState = messenger.call( | ||
| 'AssetsController:getStateForTransactionPay', |
Member
There was a problem hiding this comment.
Why are we coupling actions to other controllers?
Would we not just want a single standard state model for consistency, that we then expose for all callers?
Does it not defeat the benefit of a clean new controller for asset state, if we then add complexity through alternate formats of the same data?
Contributor
Author
There was a problem hiding this comment.
We could return AssetsController state, but then we would also have to add dependencies here to AccountsController (actually not that one as they are deprecating it, but I'm not even sure what they want us to use nowadays), as the AssetsController state stores balances based on the accountId, not the account address (so that it is consistent for both evm and non-evm).
Since the functions here seem to be evm-specific and use the account address, that means we would have to do a fetch and a lookup here from the address to the accountId so that we can access the new state.
So that's why we have favoured a thin layer that does all the transformations at our end and delivers them in the same shape as they are now so that the same logic can be reused. Easier to review and less risks of introducing a lot of new logic in an already pretty huge controller migration.
Member
There was a problem hiding this comment.
I agree that does sound far more complex, if we'd have to correlate balances and accounts in here.
But maybe you could make that "view" still generic and decoupled with a name such as getStateEvm, getStateLegacy, or getStateByAddress so other controllers with only EVM or address usage requirements could benefit also?
It's no impact to us if you're happy to maintain it, just wanted to clarify the intent 👍
Contributor
Author
There was a problem hiding this comment.
If we had a lot of controllers that depended on it that should have been the approach, but there were only two (Bridge, which is already done and TransactionPay), so we had actions tailored to them directly. We are happy to maintain it like this.
Part of the intent is also to discourage the use of those actions in new code, as it'd be best to use the new state as it is.
With client selectors, since they are used everywhere, we have just generic ones for each piece of state, which we plan to mark as deprecated as soon as the feature flag is on and stable.
| | AccountTrackerControllerState['accountsByChainId'] | ||
| | undefined => | ||
| messenger.call('AccountTrackerController:getState')?.accountsByChainId; | ||
| } |
Member
There was a problem hiding this comment.
To clarify, this should be negligible though as just a synchronous state read?
| * @param account - Address of the account. | ||
| * @returns The token balance as a BigNumber. | ||
| */ | ||
| export function getAllTokenBalances( |
Member
There was a problem hiding this comment.
Harmless to remove if not used, but TokenBalancesController:getState should be negligible in performance cost as it's not asynchronous, nor is there any logic associated with it, so it's just an object read but indirected through the messenger callbacks?
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Contributor
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
Uses the new assets controller state in TransactionPayController. With the feature flag off, it should not require any additional changes.
The messenger requires two new actions, one to determine the current version of the app for feature flag filtering, and another to fetch state from the new AssetsController.
Preview PR with the breaking changes.
Extension: MetaMask/metamask-extension#40712 (extension is several major versions behind, we are only applying breaking changes from this PR)
Mobile: MetaMask/metamask-mobile#27359
References
Checklist
Note
Medium Risk
Changes how
TransactionPayControllerreads token metadata, balances, and pricing by optionally switching toAssetsController:getStateForTransactionPaybehind a remote feature flag, which could affect quote calculations if state shapes or flag gating are incorrect. Also introduces a breaking messenger permission change requiring consumers to allow the new AssetsController action.Overview
Adds a new remote feature flag check (
assetsUnifyState, version1) and, when enabled, switchesgetTokenBalance,getTokenInfo, andgetTokenFiatRateto source balances/metadata/prices fromAssetsController:getStateForTransactionPayinstead of calling multiple legacy controllergetStateactions.Updates typing, tests, and mocks to support the new messenger action (breaking
AllowedActionschange), adds@metamask/assets-controlleras a dependency, and wires TS project references accordingly.Written by Cursor Bugbot for commit 1f5463f. This will update automatically on new commits. Configure here.