diff --git a/README.md b/README.md index 06fb06e2385..ed7f97862c6 100644 --- a/README.md +++ b/README.md @@ -305,6 +305,7 @@ linkStyle default opacity:0.5 assets_controllers --> polling_controller; assets_controllers --> preferences_controller; assets_controllers --> profile_sync_controller; + assets_controllers --> remote_feature_flag_controller; assets_controllers --> storage_service; assets_controllers --> transaction_controller; authenticated_user_storage --> base_data_service; diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 6cebb1d05a9..f0ad59a4904 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** `DeFiPositionsControllerV2.fetchDeFiPositions` now polls while any selected account has `processingDefiPositions: true`, updating state only when every account is ready, invalidating the balances cache between attempts, sharing one in-flight promise per selected-account + `vsCurrency` key (so fast switches can join an earlier matching poll), and stopping on request failure or the max attempt limit ([#9711](https://github.com/MetaMask/core/pull/9711)) + - Clients must allow and delegate `RemoteFeatureFlagController:getState` on the `DeFiPositionsControllerV2` messenger. - Bump `@metamask/transaction-controller` from `^69.4.0` to `^69.5.0` ([#9780](https://github.com/MetaMask/core/pull/9780)) ## [110.1.1] diff --git a/packages/assets-controllers/package.json b/packages/assets-controllers/package.json index 855a4960276..8575729b941 100644 --- a/packages/assets-controllers/package.json +++ b/packages/assets-controllers/package.json @@ -82,6 +82,7 @@ "@metamask/polling-controller": "^16.0.9", "@metamask/preferences-controller": "^23.1.0", "@metamask/profile-sync-controller": "^29.0.0", + "@metamask/remote-feature-flag-controller": "^5.0.0", "@metamask/rpc-errors": "^7.0.2", "@metamask/snaps-controllers": "^19.0.0", "@metamask/snaps-sdk": "^11.0.0", diff --git a/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2-method-action-types.ts b/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2-method-action-types.ts index 12c145da251..b2a4f67f065 100644 --- a/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2-method-action-types.ts +++ b/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2-method-action-types.ts @@ -6,18 +6,30 @@ import type { DeFiPositionsControllerV2 } from './DeFiPositionsControllerV2.js'; /** - * Fetches DeFi positions for the selected account group. Each account key in - * a ready response replaces that account's state (other accounts stay). - * Accounts still indexing (`processingDefiPositions`) are skipped so prior - * state is kept for them. No-ops when disabled or when the group has no + * Fetches DeFi positions for the selected account group. State is updated only + * when every selected account in the response is ready (none report + * `processingDefiPositions`); each account key in that response replaces that + * account's state (other accounts stay). While any account is still indexing, + * prior state is kept and the method polls (invalidating the balances cache + * between attempts) until all selected accounts are ready, the attempt limit + * is reached, or a request fails. Concurrent calls for the same selected + * accounts and `vsCurrency` share one in-flight promise; calls for a different + * selection or fiat currency start a new fetch and leave prior polls running + * so a later switch back can join them. When a successful ready response + * required more than one attempt, or when polling hits the attempt limit + * while still processing, reports to Sentry via `messenger.captureException` + * (error names `DeFiPositionsV2FetchAttempts` / + * `DeFiPositionsV2ProcessingPollExhausted`) so poll limits can be tuned. + * No-ops when disabled or when the group has no * supported accounts. Caching / spam prevention is handled by the apiClient * TanStack Query cache (keyed by accounts + query options including - * `vsCurrency`). Pass `{ forceRefresh: true }` to bypass the cache (e.g. - * pull-to-refresh). + * `vsCurrency`). Pass `{ forceRefresh: true }` to bypass the cache on the + * first attempt (e.g. pull-to-refresh). * * @param options - Optional fetch modifiers. - * @param options.forceRefresh - When true, bypass the apiClient cache and - * fetch immediately. + * @param options.forceRefresh - When true, bypass the apiClient cache on the + * first attempt and fetch immediately. + * @returns Resolves when the fetch (and any processing polls) finish. */ export type DeFiPositionsControllerV2FetchDeFiPositionsAction = { type: `DeFiPositionsControllerV2:fetchDeFiPositions`; diff --git a/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2.test.ts b/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2.test.ts index dfd6919ac9b..512273cd0b8 100644 --- a/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2.test.ts +++ b/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2.test.ts @@ -18,6 +18,7 @@ import type { MessengerEvents, MockAnyNamespace, } from '@metamask/messenger'; +import type { FeatureFlags } from '@metamask/remote-feature-flag-controller'; import { createMockInternalAccount } from '../../../accounts-controller/tests/mocks.js'; import { DEFI_SUPPORTED_NETWORKS } from './build-defi-balances-query.js'; @@ -27,6 +28,11 @@ import { getDefaultDeFiPositionsControllerV2State, } from './DeFiPositionsControllerV2.js'; +/** Mirrors the internal defaults in `defi-controller-v2-feature-flag.ts`. */ +const DEFI_CONTROLLER_V2_FEATURE_FLAG = 'defiControllerV2'; +const DEFAULT_PROCESSING_POLL_INTERVAL_MS = 5_000; +const DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS = 5; + const EVM_ADDRESS = '0x0000000000000000000000000000000000000001'; const SOLANA_ADDRESS = 'So11111111111111111111111111111111111111112'; @@ -125,34 +131,58 @@ function buildMockBalancesResponse( }; } +/** + * Builds a processing-only balances response for the EVM account. + * + * @returns A v6 balances response with `processingDefiPositions: true`. + */ +function buildProcessingBalancesResponse(): V6BalancesResponse { + return buildMockBalancesResponse({ + accounts: [ + { + accountId: `eip155:0:${EVM_ADDRESS}`, + processingDefiPositions: true, + balances: [], + }, + ], + }); +} + /** * Sets up the V2 controller with the given configuration. * * @param config - Configuration for the mock setup. * @param config.isEnabled - Whether the controller is enabled. * @param config.getVsCurrency - Fiat currency getter. + * @param config.remoteFeatureFlags - Remote feature flags returned by + * `RemoteFeatureFlagController:getState` (defaults to empty). * @param config.mockGroupAccounts - Accounts returned for the selected group. * @param config.getGroupAccounts - Getter for the selected group accounts * (preferred when the selection changes between fetches). * @param config.mockFetchV6MultiAccountBalances - Mock API fetch function. + * @param config.captureException - Mock Sentry capture function. * @param config.state - Initial controller state. * @returns The controller instance and mocks. */ function setupController({ isEnabled = (): boolean => true, getVsCurrency = (): string => 'USD', + remoteFeatureFlags = {}, mockGroupAccounts = GROUP_ACCOUNTS, getGroupAccounts, mockFetchV6MultiAccountBalances = jest .fn() .mockResolvedValue(buildMockBalancesResponse()), + captureException = jest.fn(), state, }: { isEnabled?: () => boolean; getVsCurrency?: () => string; + remoteFeatureFlags?: FeatureFlags; mockGroupAccounts?: InternalAccount[]; getGroupAccounts?: () => InternalAccount[]; mockFetchV6MultiAccountBalances?: jest.Mock; + captureException?: jest.Mock; state?: Partial>; } = {}): { controller: DeFiPositionsControllerV2; @@ -163,15 +193,26 @@ function setupController({ RootMessenger >; mockFetchV6MultiAccountBalances: jest.Mock; + mockInvalidateQueries: jest.Mock; + mockGetV6MultiAccountBalancesQueryOptions: jest.Mock; + mockCaptureException: jest.Mock; } { const messenger: RootMessenger = new Messenger({ namespace: MOCK_ANY_NAMESPACE, + captureException, }); messenger.registerActionHandler( 'AccountTreeController:getAccountsFromSelectedAccountGroup', () => getGroupAccounts?.() ?? mockGroupAccounts, ); + messenger.registerActionHandler( + 'RemoteFeatureFlagController:getState', + () => ({ + remoteFeatureFlags, + cacheTimestamp: 0, + }), + ); const controllerMessenger = new Messenger< 'DeFiPositionsControllerV2', @@ -184,12 +225,25 @@ function setupController({ }); messenger.delegate({ messenger: controllerMessenger, - actions: ['AccountTreeController:getAccountsFromSelectedAccountGroup'], + actions: [ + 'AccountTreeController:getAccountsFromSelectedAccountGroup', + 'RemoteFeatureFlagController:getState', + ], }); + const mockInvalidateQueries = jest.fn().mockResolvedValue(undefined); + const mockGetV6MultiAccountBalancesQueryOptions = jest + .fn() + .mockReturnValue({ queryKey: ['accounts', 'balances', 'v6', 'mock'] }); + const apiClient = { accounts: { fetchV6MultiAccountBalances: mockFetchV6MultiAccountBalances, + getV6MultiAccountBalancesQueryOptions: + mockGetV6MultiAccountBalancesQueryOptions, + queryClient: { + invalidateQueries: mockInvalidateQueries, + }, }, } as unknown as ApiPlatformClient; @@ -205,11 +259,15 @@ function setupController({ controller, controllerMessenger, mockFetchV6MultiAccountBalances, + mockInvalidateQueries, + mockGetV6MultiAccountBalancesQueryOptions, + mockCaptureException: captureException, }; } describe('DeFiPositionsControllerV2', () => { afterEach(() => { + jest.useRealTimers(); jest.restoreAllMocks(); }); @@ -248,7 +306,11 @@ describe('DeFiPositionsControllerV2', () => { }); it('fetches positions and stores them keyed by internal account ID', async () => { - const { controller, mockFetchV6MultiAccountBalances } = setupController(); + const { + controller, + mockFetchV6MultiAccountBalances, + mockInvalidateQueries, + } = setupController(); await controller.fetchDeFiPositions(); @@ -266,6 +328,7 @@ describe('DeFiPositionsControllerV2', () => { }, {}, ); + expect(mockInvalidateQueries).not.toHaveBeenCalled(); expect(controller.state.allDeFiPositionsV2['evm-account-id']).toHaveLength( 1, @@ -378,36 +441,125 @@ describe('DeFiPositionsControllerV2', () => { }); }); - it('keeps prior state for accounts still indexing DeFi positions', async () => { - const { controller, mockFetchV6MultiAccountBalances } = setupController({ + it('polls until processing accounts become ready and keeps prior state meanwhile', async () => { + jest.useFakeTimers(); + + const { + controller, + mockFetchV6MultiAccountBalances, + mockInvalidateQueries, + } = setupController({ mockFetchV6MultiAccountBalances: jest .fn() .mockResolvedValueOnce(buildMockBalancesResponse()) - .mockResolvedValueOnce( - buildMockBalancesResponse({ - accounts: [ - { - accountId: `eip155:0:${EVM_ADDRESS}`, - processingDefiPositions: true, - balances: [], - }, - ], - }), - ), + .mockResolvedValueOnce(buildProcessingBalancesResponse()) + .mockResolvedValueOnce(buildMockBalancesResponse()), }); await controller.fetchDeFiPositions(); const cached = controller.state.allDeFiPositionsV2['evm-account-id']; expect(cached).toHaveLength(1); - await controller.fetchDeFiPositions({ forceRefresh: true }); - expect(controller.state.allDeFiPositionsV2['evm-account-id']).toBe(cached); + const secondFetch = controller.fetchDeFiPositions({ forceRefresh: true }); + await Promise.resolve(); expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(2); + expect(controller.state.allDeFiPositionsV2['evm-account-id']).toBe(cached); + expect(mockInvalidateQueries).toHaveBeenCalledTimes(1); + + await jest.advanceTimersByTimeAsync(DEFAULT_PROCESSING_POLL_INTERVAL_MS); + await secondFetch; + + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(3); + expect(mockFetchV6MultiAccountBalances).toHaveBeenLastCalledWith( + expect.any(Array), + expect.objectContaining({ vsCurrency: 'usd' }), + { staleTime: 0 }, + ); + expect(controller.state.allDeFiPositionsV2['evm-account-id']).toHaveLength( + 1, + ); + expect(controller.state.allDeFiPositionsV2['evm-account-id']).not.toBe( + cached, + ); + }); + + it('does not report attempt count to Sentry when the first fetch succeeds', async () => { + const { controller, mockCaptureException } = setupController(); + + await controller.fetchDeFiPositions(); + + expect(mockCaptureException).not.toHaveBeenCalled(); + }); + + it('reports attempt count to Sentry when positions become ready after polling', async () => { + jest.useFakeTimers(); + + const { controller, mockCaptureException } = setupController({ + mockFetchV6MultiAccountBalances: jest + .fn() + .mockResolvedValueOnce(buildProcessingBalancesResponse()) + .mockResolvedValueOnce(buildProcessingBalancesResponse()) + .mockResolvedValueOnce(buildMockBalancesResponse()), + }); + + const fetchPromise = controller.fetchDeFiPositions(); + await Promise.resolve(); + expect(mockCaptureException).not.toHaveBeenCalled(); + + await jest.advanceTimersByTimeAsync(DEFAULT_PROCESSING_POLL_INTERVAL_MS); + await Promise.resolve(); + expect(mockCaptureException).not.toHaveBeenCalled(); + + await jest.advanceTimersByTimeAsync(DEFAULT_PROCESSING_POLL_INTERVAL_MS); + await fetchPromise; + + expect(mockCaptureException).toHaveBeenCalledTimes(1); + expect(mockCaptureException).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'DeFiPositionsV2FetchAttempts', + message: + 'DeFiPositionsControllerV2: positions ready after 3 attempt(s)', + }), + ); }); - it('updates ready accounts while skipping ones still indexing', async () => { + it('reports to Sentry when polling hits the max limit while still processing', async () => { + jest.useFakeTimers(); + + const mockFetchV6MultiAccountBalances = jest + .fn() + .mockResolvedValue(buildProcessingBalancesResponse()); + const { controller, mockCaptureException } = setupController({ + mockFetchV6MultiAccountBalances, + }); + + const fetchPromise = controller.fetchDeFiPositions(); + + for (let i = 0; i < DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS - 1; i++) { + await Promise.resolve(); + await jest.advanceTimersByTimeAsync(DEFAULT_PROCESSING_POLL_INTERVAL_MS); + } + + await fetchPromise; + + expect(mockCaptureException).toHaveBeenCalledTimes(1); + expect(mockCaptureException).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'DeFiPositionsV2ProcessingPollExhausted', + message: `DeFiPositionsControllerV2: still processing after ${DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS} attempt(s)`, + }), + ); + }); + + it('keeps prior state for all accounts until every account is ready', async () => { + jest.useFakeTimers(); + const solanaAccountId = `solana:${SolScope.Mainnet.split(':')[1]}:${SOLANA_ADDRESS}`; - const { controller } = setupController({ + const { + controller, + mockInvalidateQueries, + mockFetchV6MultiAccountBalances, + } = setupController({ mockGroupAccounts: GROUP_ACCOUNTS_WITH_SOLANA, mockFetchV6MultiAccountBalances: jest .fn() @@ -459,27 +611,325 @@ describe('DeFiPositionsControllerV2', () => { }, ], }), + ) + .mockResolvedValueOnce( + buildMockBalancesResponse({ + accounts: [ + { + accountId: `eip155:0:${EVM_ADDRESS}`, + balances: buildMockBalancesResponse().accounts[0].balances, + }, + { + accountId: solanaAccountId, + balances: [], + }, + ], + }), ), }); await controller.fetchDeFiPositions(); const evmPositions = controller.state.allDeFiPositionsV2['evm-account-id']; + const solanaPositions = + controller.state.allDeFiPositionsV2['solana-account-id']; expect(evmPositions).toHaveLength(1); - expect( - controller.state.allDeFiPositionsV2['solana-account-id'], - ).toHaveLength(1); + expect(solanaPositions).toHaveLength(1); - await controller.fetchDeFiPositions({ forceRefresh: true }); + const secondFetch = controller.fetchDeFiPositions({ forceRefresh: true }); + await Promise.resolve(); - // Still-indexing EVM account keeps prior positions; ready Solana clears. + // Mixed ready/processing response: keep prior state for every account. expect(controller.state.allDeFiPositionsV2['evm-account-id']).toBe( evmPositions, ); + expect(controller.state.allDeFiPositionsV2['solana-account-id']).toBe( + solanaPositions, + ); + expect(mockInvalidateQueries).toHaveBeenCalledTimes(1); + + await jest.advanceTimersByTimeAsync(DEFAULT_PROCESSING_POLL_INTERVAL_MS); + await secondFetch; + + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(3); + expect(controller.state.allDeFiPositionsV2['evm-account-id']).toHaveLength( + 1, + ); + expect(controller.state.allDeFiPositionsV2['evm-account-id']).not.toBe( + evmPositions, + ); expect( controller.state.allDeFiPositionsV2['solana-account-id'], ).toStrictEqual([]); }); + it('stops polling after the max attempt limit while still processing', async () => { + jest.useFakeTimers(); + + const mockFetchV6MultiAccountBalances = jest + .fn() + .mockResolvedValue(buildProcessingBalancesResponse()); + + const { controller, mockInvalidateQueries } = setupController({ + mockFetchV6MultiAccountBalances, + }); + + const fetchPromise = controller.fetchDeFiPositions(); + + for (let i = 0; i < DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS - 1; i++) { + await Promise.resolve(); + await jest.advanceTimersByTimeAsync(DEFAULT_PROCESSING_POLL_INTERVAL_MS); + } + + await fetchPromise; + + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes( + DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS, + ); + expect(mockInvalidateQueries).toHaveBeenCalledTimes( + DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS, + ); + expect(controller.state.allDeFiPositionsV2).toStrictEqual({}); + }); + + it('uses maxAttempts and pollInterval from the defiControllerV2 remote flag', async () => { + jest.useFakeTimers(); + + const remoteMaxAttempts = 3; + const remotePollInterval = 1_000; + const mockFetchV6MultiAccountBalances = jest + .fn() + .mockResolvedValue(buildProcessingBalancesResponse()); + + const { controller, mockInvalidateQueries } = setupController({ + mockFetchV6MultiAccountBalances, + remoteFeatureFlags: { + [DEFI_CONTROLLER_V2_FEATURE_FLAG]: { + maxAttempts: remoteMaxAttempts, + pollInterval: remotePollInterval, + }, + }, + }); + + const fetchPromise = controller.fetchDeFiPositions(); + + for (let i = 0; i < remoteMaxAttempts - 1; i++) { + await Promise.resolve(); + await jest.advanceTimersByTimeAsync(remotePollInterval); + } + + await fetchPromise; + + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes( + remoteMaxAttempts, + ); + expect(mockInvalidateQueries).toHaveBeenCalledTimes(remoteMaxAttempts); + expect(controller.state.allDeFiPositionsV2).toStrictEqual({}); + }); + + it('shares one in-flight promise across concurrent fetchDeFiPositions calls', async () => { + jest.useFakeTimers(); + + let resolveFetch!: (value: V6BalancesResponse) => void; + const mockFetchV6MultiAccountBalances = jest.fn().mockImplementation( + () => + new Promise((resolve) => { + resolveFetch = resolve; + }), + ); + + const { controller } = setupController({ + mockFetchV6MultiAccountBalances, + }); + + const first = controller.fetchDeFiPositions(); + const second = controller.fetchDeFiPositions({ forceRefresh: true }); + + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(1); + + resolveFetch(buildMockBalancesResponse()); + await Promise.all([first, second]); + + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(1); + }); + + it('starts a new fetch when vsCurrency changes during an in-flight call', async () => { + let vsCurrency = 'USD'; + + let resolveUsdFetch!: (value: V6BalancesResponse) => void; + const mockFetchV6MultiAccountBalances = jest + .fn() + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveUsdFetch = resolve; + }), + ) + .mockResolvedValueOnce(buildMockBalancesResponse()); + + const { controller } = setupController({ + getVsCurrency: () => vsCurrency, + mockFetchV6MultiAccountBalances, + }); + + const usdFetch = controller.fetchDeFiPositions(); + await Promise.resolve(); + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(1); + expect(mockFetchV6MultiAccountBalances).toHaveBeenLastCalledWith( + expect.any(Array), + expect.objectContaining({ vsCurrency: 'usd' }), + {}, + ); + + vsCurrency = 'EUR'; + const eurFetch = controller.fetchDeFiPositions({ forceRefresh: true }); + await Promise.resolve(); + + // Different fiat currency must not join the USD in-flight promise. + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(2); + expect(mockFetchV6MultiAccountBalances).toHaveBeenLastCalledWith( + expect.any(Array), + expect.objectContaining({ vsCurrency: 'eur' }), + { staleTime: 0 }, + ); + + resolveUsdFetch(buildMockBalancesResponse()); + await Promise.all([usdFetch, eurFetch]); + + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(2); + }); + + it('starts a new fetch when selection changes during an in-flight call', async () => { + const otherEvmAddress = '0x0000000000000000000000000000000000000002'; + const otherEvmAccount = createMockInternalAccount({ + id: 'evm-account-id-2', + address: otherEvmAddress, + type: EthAccountType.Eoa, + }); + let groupAccounts: InternalAccount[] = GROUP_ACCOUNTS; + + let resolveFirstFetch!: (value: V6BalancesResponse) => void; + const mockFetchV6MultiAccountBalances = jest + .fn() + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirstFetch = resolve; + }), + ) + .mockResolvedValueOnce( + buildMockBalancesResponse({ + accounts: [ + { + accountId: `eip155:0:${otherEvmAddress}`, + balances: buildMockBalancesResponse().accounts[0].balances, + }, + ], + }), + ); + + const { controller } = setupController({ + getGroupAccounts: () => groupAccounts, + mockFetchV6MultiAccountBalances, + }); + + const firstFetch = controller.fetchDeFiPositions(); + await Promise.resolve(); + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(1); + expect(mockFetchV6MultiAccountBalances.mock.calls[0][0]).toContain( + `eip155:0:${EVM_ADDRESS}`, + ); + + groupAccounts = [otherEvmAccount]; + const secondFetch = controller.fetchDeFiPositions({ forceRefresh: true }); + await Promise.resolve(); + + // Different selection does not join the in-flight promise. + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(2); + expect(mockFetchV6MultiAccountBalances).toHaveBeenLastCalledWith( + expect.arrayContaining([`eip155:0:${otherEvmAddress}`]), + expect.objectContaining({ vsCurrency: 'usd' }), + { staleTime: 0 }, + ); + + resolveFirstFetch(buildMockBalancesResponse()); + await Promise.all([firstFetch, secondFetch]); + + // Prior poll may still write the old group; new fetch writes the new group. + expect(controller.state.allDeFiPositionsV2['evm-account-id']).toHaveLength( + 1, + ); + expect( + controller.state.allDeFiPositionsV2['evm-account-id-2'], + ).toHaveLength(1); + }); + + it('rejoins an in-flight fetch when switching back to the same accounts', async () => { + const otherEvmAddress = '0x0000000000000000000000000000000000000002'; + const otherEvmAccount = createMockInternalAccount({ + id: 'evm-account-id-2', + address: otherEvmAddress, + type: EthAccountType.Eoa, + }); + let groupAccounts: InternalAccount[] = GROUP_ACCOUNTS; + + let resolveFirstFetch!: (value: V6BalancesResponse) => void; + let resolveSecondFetch!: (value: V6BalancesResponse) => void; + const mockFetchV6MultiAccountBalances = jest + .fn() + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirstFetch = resolve; + }), + ) + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveSecondFetch = resolve; + }), + ); + + const { controller } = setupController({ + getGroupAccounts: () => groupAccounts, + mockFetchV6MultiAccountBalances, + }); + + const firstFetch = controller.fetchDeFiPositions(); + await Promise.resolve(); + + groupAccounts = [otherEvmAccount]; + const secondFetch = controller.fetchDeFiPositions(); + await Promise.resolve(); + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(2); + + groupAccounts = GROUP_ACCOUNTS; + const thirdFetch = controller.fetchDeFiPositions(); + + // Switched back to the first group — join its still-in-flight promise. + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(2); + + resolveFirstFetch(buildMockBalancesResponse()); + resolveSecondFetch( + buildMockBalancesResponse({ + accounts: [ + { + accountId: `eip155:0:${otherEvmAddress}`, + balances: [], + }, + ], + }), + ); + await Promise.all([firstFetch, secondFetch, thirdFetch]); + + expect(mockFetchV6MultiAccountBalances).toHaveBeenCalledTimes(2); + expect(controller.state.allDeFiPositionsV2['evm-account-id']).toHaveLength( + 1, + ); + expect( + controller.state.allDeFiPositionsV2['evm-account-id-2'], + ).toStrictEqual([]); + }); + it('merges fetched accounts into state without clearing other accounts', async () => { const otherEvmAddress = '0x0000000000000000000000000000000000000002'; const otherEvmAccount = createMockInternalAccount({ diff --git a/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2.ts b/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2.ts index e03457e8f98..d913972cd02 100644 --- a/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2.ts +++ b/packages/assets-controllers/src/DeFiPositionsController/DeFiPositionsControllerV2.ts @@ -7,8 +7,11 @@ import type { } from '@metamask/base-controller'; import type { ApiPlatformClient } from '@metamask/core-backend'; import type { Messenger } from '@metamask/messenger'; +import type { RemoteFeatureFlagControllerGetStateAction } from '@metamask/remote-feature-flag-controller'; import { buildDeFiBalancesQuery } from './build-defi-balances-query.js'; +import type { DeFiBalancesQuery } from './build-defi-balances-query.js'; +import { getProcessingPollConfig } from './defi-controller-v2-feature-flag.js'; import type { DeFiPositionsControllerV2MethodActions } from './DeFiPositionsControllerV2-method-action-types.js'; import type { DeFiPositionsByAccount } from './group-defi-positions-v6.js'; import { groupDeFiPositionsV6 } from './group-defi-positions-v6.js'; @@ -17,6 +20,14 @@ const controllerName = 'DeFiPositionsControllerV2'; const MESSENGER_EXPOSED_METHODS = ['fetchDeFiPositions'] as const; +/** + * @param ms - Milliseconds to wait. + * @returns A promise that resolves after `ms`. + */ +function delay(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + export type DeFiPositionsControllerV2State = { /** * DeFi positions keyed by internal MetaMask account ID (`InternalAccount.id`, @@ -72,7 +83,8 @@ export type DeFiPositionsControllerV2Events = * The external actions available to the {@link DeFiPositionsControllerV2}. */ export type AllowedActions = - AccountTreeControllerGetAccountsFromSelectedAccountGroupAction; + | AccountTreeControllerGetAccountsFromSelectedAccountGroupAction + | RemoteFeatureFlagControllerGetStateAction; /** * The external events available to the {@link DeFiPositionsControllerV2}. @@ -97,7 +109,23 @@ export type DeFiPositionsControllerV2Messenger = Messenger< * * Deduplication and freshness are handled by the shared TanStack Query cache on * {@link ApiPlatformClient} (balances default `staleTime` is 1 minute). Pass - * `{ forceRefresh: true }` to bypass that cache for pull-to-refresh. + * `{ forceRefresh: true }` to bypass that cache on the first attempt (e.g. + * pull-to-refresh); later processing polls always bypass the cache. + * + * When the API reports `processingDefiPositions` for any account, this + * controller polls until indexing finishes or the attempt limit is reached, + * and only then processes and writes state — mixed ready/processing responses + * leave prior state untouched. Concurrent calls for the same selected accounts + * and `vsCurrency` share one in-flight promise so the UI can treat the pending + * promise as a loading signal. Calls for a different selection or fiat currency + * start their own fetch and leave any prior poll running, so switching back can + * join an in-flight fetch for that group and currency. + * + * Processing-poll `maxAttempts` / `pollInterval` are read via + * {@link getProcessingPollConfig} from the `defiControllerV2` remote feature + * flag (`RemoteFeatureFlagController:getState`), falling back to built-in + * defaults when unset or invalid. Clients must delegate that action to this + * controller's messenger. */ export class DeFiPositionsControllerV2 extends BaseController< typeof controllerName, @@ -110,6 +138,14 @@ export class DeFiPositionsControllerV2 extends BaseController< readonly #getVsCurrency: () => string; + /** + * In-flight fetches keyed by selected DeFi-queryable account IDs plus + * `vsCurrency`. Concurrent callers for the same selection and currency share + * a promise; different selections or fiat currencies keep independent + * fetches so fast switching can join an earlier matching poll. + */ + readonly #inFlightFetches = new Map>(); + /** * @param options - Constructor options. * @param options.messenger - The controller messenger. @@ -152,18 +188,30 @@ export class DeFiPositionsControllerV2 extends BaseController< } /** - * Fetches DeFi positions for the selected account group. Each account key in - * a ready response replaces that account's state (other accounts stay). - * Accounts still indexing (`processingDefiPositions`) are skipped so prior - * state is kept for them. No-ops when disabled or when the group has no + * Fetches DeFi positions for the selected account group. State is updated only + * when every selected account in the response is ready (none report + * `processingDefiPositions`); each account key in that response replaces that + * account's state (other accounts stay). While any account is still indexing, + * prior state is kept and the method polls (invalidating the balances cache + * between attempts) until all selected accounts are ready, the attempt limit + * is reached, or a request fails. Concurrent calls for the same selected + * accounts and `vsCurrency` share one in-flight promise; calls for a different + * selection or fiat currency start a new fetch and leave prior polls running + * so a later switch back can join them. When a successful ready response + * required more than one attempt, or when polling hits the attempt limit + * while still processing, reports to Sentry via `messenger.captureException` + * (error names `DeFiPositionsV2FetchAttempts` / + * `DeFiPositionsV2ProcessingPollExhausted`) so poll limits can be tuned. + * No-ops when disabled or when the group has no * supported accounts. Caching / spam prevention is handled by the apiClient * TanStack Query cache (keyed by accounts + query options including - * `vsCurrency`). Pass `{ forceRefresh: true }` to bypass the cache (e.g. - * pull-to-refresh). + * `vsCurrency`). Pass `{ forceRefresh: true }` to bypass the cache on the + * first attempt (e.g. pull-to-refresh). * * @param options - Optional fetch modifiers. - * @param options.forceRefresh - When true, bypass the apiClient cache and - * fetch immediately. + * @param options.forceRefresh - When true, bypass the apiClient cache on the + * first attempt and fetch immediately. + * @returns Resolves when the fetch (and any processing polls) finish. */ async fetchDeFiPositions(options?: { forceRefresh?: boolean; @@ -175,58 +223,137 @@ export class DeFiPositionsControllerV2 extends BaseController< const selectedAccounts = this.messenger.call( 'AccountTreeController:getAccountsFromSelectedAccountGroup', ); + const query = buildDeFiBalancesQuery(selectedAccounts); + const vsCurrency = this.#getVsCurrency().toLowerCase(); + // Include vsCurrency so a fiat change does not join a poll priced in the + // previous currency (TanStack also keys the balances cache on vsCurrency). + const inFlightKey = [ + ...[...query.internalAccountIdByCaip.keys()].sort(), + vsCurrency, + ].join('\0'); - const { networks, internalAccountIdByCaip } = - buildDeFiBalancesQuery(selectedAccounts); + const existing = this.#inFlightFetches.get(inFlightKey); + if (existing) { + await existing; + return; + } + + // Same-key callers join this promise instead of replacing it, so cleanup + // can always delete without checking identity. + const fetchPromise = this.#fetchDeFiPositions( + options, + query, + vsCurrency, + ).finally(() => { + this.#inFlightFetches.delete(inFlightKey); + }); + this.#inFlightFetches.set(inFlightKey, fetchPromise); + await fetchPromise; + } + + async #fetchDeFiPositions( + options: { forceRefresh?: boolean } | undefined, + { networks, internalAccountIdByCaip }: DeFiBalancesQuery, + vsCurrency: string, + ): Promise { if (internalAccountIdByCaip.size === 0 || networks.length === 0) { return; } const accountIds = [...internalAccountIdByCaip.keys()]; - const vsCurrency = this.#getVsCurrency().toLowerCase(); + const queryOptions = { + networks, + includeDeFiBalances: true, + forceFetchDeFiPositions: true, + includePrices: true, + vsCurrency, + }; - try { - const response = - await this.#apiClient.accounts.fetchV6MultiAccountBalances( + // Resolve once per fetch so a mid-poll remote-flag change cannot stretch + // or shrink this poll sequence inconsistently. + const { maxAttempts, pollInterval } = getProcessingPollConfig( + this.messenger, + ); + + for (let attempt = 0; attempt < maxAttempts; attempt++) { + // First attempt respects forceRefresh; later polls always bypass cache + // so we do not spin on a stale processing snapshot. + const fetchOptions = { + ...(options?.forceRefresh || attempt > 0 ? { staleTime: 0 } : {}), + }; + + let response; + try { + response = await this.#apiClient.accounts.fetchV6MultiAccountBalances( accountIds, - { - networks, - includeDeFiBalances: true, - forceFetchDeFiPositions: true, - includePrices: true, - vsCurrency, - }, - { - // staleTime: 0 makes TanStack treat the cache as stale for this call. - ...(options?.forceRefresh ? { staleTime: 0 } : {}), - }, + queryOptions, + fetchOptions, ); - - // Skip accounts still indexing — their balances are not a valid snapshot. - const readyAccounts = response.accounts.filter( - (account) => !account.processingDefiPositions, - ); - if (readyAccounts.length === 0) { + } catch (error) { + // Soft-fail so prior state stays and the in-flight promise settles. + console.error('Failed to fetch DeFi positions', error); return; } - const positionsByAccount = groupDeFiPositionsV6( - { ...response, accounts: readyAccounts }, - internalAccountIdByCaip, + const stillProcessing = response.accounts.some( + (account) => account.processingDefiPositions, ); - // Last valid response wins per ready account; processing / other accounts - // stay untouched. - this.update((state) => { - for (const [accountId, positions] of Object.entries( - positionsByAccount, - )) { - state.allDeFiPositionsV2[accountId] = positions; + // Only process and write when every account is ready so a partial + // response cannot clear or overwrite positions for accounts that are + // still indexing. + if (!stillProcessing) { + const positionsByAccount = groupDeFiPositionsV6( + response, + internalAccountIdByCaip, + ); + + this.update((state) => { + for (const [accountId, positions] of Object.entries( + positionsByAccount, + )) { + state.allDeFiPositionsV2[accountId] = positions; + } + }); + + // Report how many attempts were needed (only when polling was + // required) so Sentry can inform remote-flag / default poll-limit + // tuning without flooding on first-try successes. + const attemptsTaken = attempt + 1; + if (attemptsTaken > 1) { + const multipleAttemptsError = new Error( + `DeFiPositionsControllerV2: positions ready after ${attemptsTaken} attempt(s)`, + ); + multipleAttemptsError.name = 'DeFiPositionsV2FetchAttempts'; + this.messenger.captureException?.(multipleAttemptsError); } + return; + } + + const { queryKey } = + this.#apiClient.accounts.getV6MultiAccountBalancesQueryOptions( + accountIds, + queryOptions, + fetchOptions, + ); + await this.#apiClient.accounts.queryClient.invalidateQueries({ + queryKey, }); - } catch (error) { - console.error('Failed to fetch DeFi positions', error); + + const isLastAttempt = attempt >= maxAttempts - 1; + if (isLastAttempt) { + // Report exhausted polls so Sentry can inform remote-flag / default + // poll-limit tuning when indexing never finishes in time. + const multipleAttemptsError = new Error( + `DeFiPositionsControllerV2: still processing after ${maxAttempts} attempt(s)`, + ); + multipleAttemptsError.name = 'DeFiPositionsV2ProcessingPollExhausted'; + this.messenger.captureException?.(multipleAttemptsError); + return; + } + + await delay(pollInterval); } } } diff --git a/packages/assets-controllers/src/DeFiPositionsController/defi-controller-v2-feature-flag.test.ts b/packages/assets-controllers/src/DeFiPositionsController/defi-controller-v2-feature-flag.test.ts new file mode 100644 index 00000000000..8178b40a0c9 --- /dev/null +++ b/packages/assets-controllers/src/DeFiPositionsController/defi-controller-v2-feature-flag.test.ts @@ -0,0 +1,104 @@ +import { getProcessingPollConfig } from './defi-controller-v2-feature-flag.js'; + +/** Mirrors the internal defaults in `defi-controller-v2-feature-flag.ts`. */ +const DEFAULT_PROCESSING_POLL_INTERVAL_MS = 5_000; +const DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS = 5; +const DEFI_CONTROLLER_V2_FEATURE_FLAG = 'defiControllerV2'; + +/** + * @param remoteFeatureFlags - Remote feature flags to return from getState. + * @returns A minimal messenger stub for `getProcessingPollConfig`. + */ +function buildMessenger(remoteFeatureFlags: Record): { + call: jest.Mock; +} { + return { + call: jest.fn().mockReturnValue({ + remoteFeatureFlags, + cacheTimestamp: 0, + }), + }; +} + +describe('getProcessingPollConfig', () => { + it('returns defaults when the remote flag is missing', () => { + const messenger = buildMessenger({}); + + expect(getProcessingPollConfig(messenger)).toStrictEqual({ + maxAttempts: DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS, + pollInterval: DEFAULT_PROCESSING_POLL_INTERVAL_MS, + }); + expect(messenger.call).toHaveBeenCalledWith( + 'RemoteFeatureFlagController:getState', + ); + }); + + it('returns defaults when the remote flag is malformed', () => { + const messenger = buildMessenger({ + [DEFI_CONTROLLER_V2_FEATURE_FLAG]: 'not-an-object', + }); + + expect(getProcessingPollConfig(messenger)).toStrictEqual({ + maxAttempts: DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS, + pollInterval: DEFAULT_PROCESSING_POLL_INTERVAL_MS, + }); + }); + + it('resolves maxAttempts and pollInterval from the remote flag', () => { + const messenger = buildMessenger({ + [DEFI_CONTROLLER_V2_FEATURE_FLAG]: { + enabled: true, + maxAttempts: 3, + pollInterval: 1000, + }, + }); + + expect(getProcessingPollConfig(messenger)).toStrictEqual({ + maxAttempts: 3, + pollInterval: 1000, + }); + }); + + it('floors maxAttempts and falls back for non-positive or non-finite values', () => { + expect( + getProcessingPollConfig( + buildMessenger({ + [DEFI_CONTROLLER_V2_FEATURE_FLAG]: { + maxAttempts: 2.9, + pollInterval: 2500, + }, + }), + ), + ).toStrictEqual({ + maxAttempts: 2, + pollInterval: 2500, + }); + + expect( + getProcessingPollConfig( + buildMessenger({ + [DEFI_CONTROLLER_V2_FEATURE_FLAG]: { + maxAttempts: 0, + pollInterval: Number.NaN, + }, + }), + ), + ).toStrictEqual({ + maxAttempts: DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS, + pollInterval: DEFAULT_PROCESSING_POLL_INTERVAL_MS, + }); + + expect( + getProcessingPollConfig( + buildMessenger({ + [DEFI_CONTROLLER_V2_FEATURE_FLAG]: { + maxAttempts: 2, + }, + }), + ), + ).toStrictEqual({ + maxAttempts: 2, + pollInterval: DEFAULT_PROCESSING_POLL_INTERVAL_MS, + }); + }); +}); diff --git a/packages/assets-controllers/src/DeFiPositionsController/defi-controller-v2-feature-flag.ts b/packages/assets-controllers/src/DeFiPositionsController/defi-controller-v2-feature-flag.ts new file mode 100644 index 00000000000..6237b632375 --- /dev/null +++ b/packages/assets-controllers/src/DeFiPositionsController/defi-controller-v2-feature-flag.ts @@ -0,0 +1,120 @@ +import type { RemoteFeatureFlagControllerState } from '@metamask/remote-feature-flag-controller'; +import type { Json } from '@metamask/utils'; + +/** + * Remote feature flag key for DeFi Positions Controller V2 (camelCase, as + * stored by RemoteFeatureFlagController after client-config resolution). + */ +const DEFI_CONTROLLER_V2_FEATURE_FLAG = 'defiControllerV2'; + +/** Delay between polls while Accounts API reports DeFi indexing in progress. */ +const DEFAULT_PROCESSING_POLL_INTERVAL_MS = 5_000; + +/** + * Maximum fetch attempts (including the first) while any account still has + * `processingDefiPositions: true`. After this, the call resolves without + * updating state, so prior positions are kept for every selected account. + */ +const DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS = 5; + +/** + * Resolved `defiControllerV2` remote feature flag shape used for processing + * poll overrides. `enabled` is read by clients for gating; the controller + * only consumes `maxAttempts` / `pollInterval`. + */ +type DeFiControllerV2FeatureFlag = { + enabled?: boolean; + maxAttempts?: number; + pollInterval?: number; +}; + +/** + * Optional processing-poll overrides from {@link DeFiControllerV2FeatureFlag}. + * Missing or non-positive values fall back to + * {@link DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS} / + * {@link DEFAULT_PROCESSING_POLL_INTERVAL_MS}. + */ +type DeFiPositionsControllerV2ProcessingPollConfig = { + maxAttempts?: number; + pollInterval?: number; +}; + +/** + * Resolved positive integer processing-poll limits. + */ +type ResolvedProcessingPollConfig = { + maxAttempts: number; + pollInterval: number; +}; + +/** + * Messenger surface needed to read the DeFi V2 remote feature flag. + */ +type GetProcessingPollConfigMessenger = { + call: ( + actionType: 'RemoteFeatureFlagController:getState', + ) => RemoteFeatureFlagControllerState; +}; + +/** + * @param config - Optional remote poll overrides. + * @returns Resolved positive integer max attempts and poll interval ms. + */ +function resolveProcessingPollConfig( + config?: DeFiPositionsControllerV2ProcessingPollConfig | null, +): ResolvedProcessingPollConfig { + const maxAttempts = + typeof config?.maxAttempts === 'number' && + Number.isFinite(config.maxAttempts) && + config.maxAttempts > 0 + ? Math.floor(config.maxAttempts) + : DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS; + const pollInterval = + typeof config?.pollInterval === 'number' && + Number.isFinite(config.pollInterval) && + config.pollInterval > 0 + ? config.pollInterval + : DEFAULT_PROCESSING_POLL_INTERVAL_MS; + + return { maxAttempts, pollInterval }; +} + +/** + * Narrows a remote feature-flag JSON value to the DeFi V2 poll config fields. + * + * @param flag - Raw flag value from RemoteFeatureFlagController state. + * @returns Poll config fields when present, otherwise `undefined`. + */ +function parseDeFiControllerV2FeatureFlag( + flag: Json | undefined, +): DeFiPositionsControllerV2ProcessingPollConfig | undefined { + if (!flag || typeof flag !== 'object' || Array.isArray(flag)) { + return undefined; + } + + const { maxAttempts, pollInterval } = flag as DeFiControllerV2FeatureFlag; + return { maxAttempts, pollInterval }; +} + +/** + * Reads `defiControllerV2` from RemoteFeatureFlagController and returns + * resolved processing-poll limits. Missing or invalid flag values fall back to + * {@link DEFAULT_PROCESSING_POLL_MAX_ATTEMPTS} / + * {@link DEFAULT_PROCESSING_POLL_INTERVAL_MS}. + * + * @param messenger - Messenger that can call + * `RemoteFeatureFlagController:getState`. + * @returns Positive integer max attempts and poll interval ms. + */ +export function getProcessingPollConfig( + messenger: GetProcessingPollConfigMessenger, +): ResolvedProcessingPollConfig { + const { remoteFeatureFlags } = messenger.call( + 'RemoteFeatureFlagController:getState', + ); + return resolveProcessingPollConfig( + parseDeFiControllerV2FeatureFlag( + remoteFeatureFlags?.[DEFI_CONTROLLER_V2_FEATURE_FLAG], + ), + ); +} diff --git a/packages/assets-controllers/tsconfig.build.json b/packages/assets-controllers/tsconfig.build.json index f440265759a..6e3978c1457 100644 --- a/packages/assets-controllers/tsconfig.build.json +++ b/packages/assets-controllers/tsconfig.build.json @@ -59,6 +59,9 @@ }, { "path": "../profile-sync-controller/tsconfig.build.json" + }, + { + "path": "../remote-feature-flag-controller/tsconfig.build.json" } ], "include": ["../../types", "./src"], diff --git a/packages/assets-controllers/tsconfig.json b/packages/assets-controllers/tsconfig.json index 695e5d7c051..7a5d296a792 100644 --- a/packages/assets-controllers/tsconfig.json +++ b/packages/assets-controllers/tsconfig.json @@ -58,6 +58,9 @@ }, { "path": "../profile-sync-controller" + }, + { + "path": "../remote-feature-flag-controller" } ], "include": ["../../types", "./src", "../../tests"] diff --git a/yarn.lock b/yarn.lock index 848ac8c2fff..36864b777c5 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6065,6 +6065,7 @@ __metadata: "@metamask/preferences-controller": "npm:^23.1.0" "@metamask/profile-sync-controller": "npm:^29.0.0" "@metamask/providers": "npm:^22.1.0" + "@metamask/remote-feature-flag-controller": "npm:^5.0.0" "@metamask/rpc-errors": "npm:^7.0.2" "@metamask/snaps-controllers": "npm:^19.0.0" "@metamask/snaps-sdk": "npm:^11.0.0"