Skip to content

feat: replace assets state references in Engine#29727

Merged
bergarces merged 1 commit into
mainfrom
replace-assets-state-references-engine
May 5, 2026
Merged

feat: replace assets state references in Engine#29727
bergarces merged 1 commit into
mainfrom
replace-assets-state-references-engine

Conversation

@bergarces

@bergarces bergarces commented May 5, 2026

Copy link
Copy Markdown
Contributor

Description

The new AssetsController is being introduced to replace most controllers from @metamask/assets-controllers. This is one of many PRs that replace direct access to legacy state with selectors that, using a feature flag, handle the transition between the legacy state and the new state when the flag is turned on.

Changelog

CHANGELOG entry: null

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/ASSETS-2827

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

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.

Note

Medium Risk
Updates a core balance-calculation path to source data from Redux selectors instead of controller state, which could change results depending on feature-flagged state shape. Risk is mitigated by updating unit tests, but errors would affect fiat balance display across networks.

Overview
Refactors Engine.getTotalEvmFiatAccountBalance to stop reading asset/currency/token data directly from Engine controller instances and instead pull accountsByChainId, token lists/balances, market data, and currency rates via assets-migration selectors off store.getState().

Updates Engine.test.ts to mock store.getState() and provide the expected engine.backgroundState slices for the balance-calculation test cases (no balances, ETH only, ETH+tokens, ETH+staked ETH+tokens).

Reviewed by Cursor Bugbot for commit 4993f4a. Bugbot is set up for automated code reviews on this repo. Configure here.

@bergarces bergarces requested a review from a team as a code owner May 5, 2026 12:31
@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown
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.

@bergarces bergarces marked this pull request as draft May 5, 2026 12:31
@github-actions github-actions Bot added the size-M label May 5, 2026
@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeWalletPlatform, SmokeConfirmations, SmokeAccounts, SmokeSwap, SmokeStake
  • Selected Performance tags: @PerformanceAssetLoading, @PerformanceAccountList
  • Risk Level: high
  • AI Confidence: 82%
click to see 🤖 AI reasoning details

E2E Test Selection:
The changes in Engine.ts refactor the getTotalEvmFiatAccountBalance method to use Redux selectors from assets-migration instead of directly accessing controller state. This is a significant architectural change:

  1. What changed: The method now reads from the Redux store via createDeepEqualSelector selectors (getCurrencyRateControllerCurrentCurrency, getAccountTrackerControllerAccountsByChainId, getTokenRatesControllerMarketData, getTokensControllerAllTokens, getTokenBalancesControllerTokenBalances) instead of directly from controller instances.

  2. Why it's high risk: The assets-migration selectors are feature-flag-gated (via selectIsAssetsUnifyStateEnabled). When the flag is enabled, they return data from the new unified AssetsController instead of legacy controllers. This means balance calculations could return different values depending on the feature flag state, potentially affecting all surfaces that display EVM balances.

  3. Affected surfaces:

    • SmokeConfirmations: useAccountInfo.ts calls getTotalEvmFiatAccountBalance() directly for account info rows in confirmation screens (personal_sign, typed data, contract interactions, etc.)
    • SmokeWalletPlatform: useMultichainBalances/utils.ts calls this for wallet home balance display and transaction history
    • SmokeAccounts: useAccounts hook uses this for account selector balance display
    • SmokeSwap: BridgeView uses this balance calculation
    • SmokeStake: Staking confirmation screens use this balance
  4. Test file changes: Engine.test.ts was updated to mock store.getState instead of passing state directly to Engine.init, confirming the architectural shift to Redux-based state reading.

  5. Dependent tags: SmokeSwap and SmokeStake require SmokeConfirmations per tag descriptions (already included).

Performance Test Selection:
The refactoring changes direct controller state access to Redux selectors using createDeepEqualSelector. This introduces selector memoization overhead and potentially changes the data flow for balance calculations. @PerformanceAssetLoading is relevant because token balances, currency rates, and market data are now fetched via selectors from Redux store rather than directly from controllers - this could affect balance loading times. @PerformanceAccountList is relevant because the account selector uses getTotalEvmFiatAccountBalance for each account's balance display, and the new selector-based approach with deep equality checks could impact rendering performance when many accounts are displayed.

View GitHub Actions results

@bergarces bergarces marked this pull request as ready for review May 5, 2026 12:36
@sonarqubecloud

sonarqubecloud Bot commented May 5, 2026

Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For assets state, we are no longer reading data directly from the controller state, but we are using store.getState() instead. That's why the state changes have been moved to that function mock.

@juanmigdr juanmigdr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@Cal-L Cal-L left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@bergarces bergarces added this pull request to the merge queue May 5, 2026
Merged via the queue into main with commit 4e82ec3 May 5, 2026
203 of 209 checks passed
@bergarces bergarces deleted the replace-assets-state-references-engine branch May 5, 2026 15:46
@github-actions github-actions Bot locked and limited conversation to collaborators May 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants