Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/assets-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Bump `@metamask/transaction-controller` from `^69.5.1` to `^69.5.2` ([#9823](https://github.com/MetaMask/core/pull/9823))

### Fixed

- Never serve Accounts API balances from the TanStack cache in any flow (forced refresh, polling, websocket-triggered fetches): `AccountsApiDataSource` now invalidates cached balance queries before every fetch and fetches with `staleTime: 0, gcTime: 0`, so v5 and v6 balance requests always hit the network and their results are never retained for reuse — previously non-forced polls could reuse balances cached for up to 60s and even `forceUpdate: true` only shrank the cache window to 100ms, so `getAssets(..., { forceUpdate: true })` (pull-to-refresh, post-swap/post-transaction refresh) could still return stale cached balances

## [13.1.2]

### Changed
Expand Down
3 changes: 3 additions & 0 deletions packages/assets-controller/src/AssetsController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1409,6 +1409,7 @@ describe('AssetsController', () => {
}),
fetchV6MultiAccountBalances,
fetchV5MultiAccountBalances,
invalidateBalances: jest.fn().mockResolvedValue(undefined),
},
} as unknown as ApiPlatformClient;

Expand Down Expand Up @@ -3044,6 +3045,7 @@ describe('AssetsController', () => {
partialSupport: [],
}),
fetchV5MultiAccountBalances,
invalidateBalances: jest.fn().mockResolvedValue(undefined),
},
} as unknown as ApiPlatformClient;

Expand Down Expand Up @@ -3363,6 +3365,7 @@ describe('AssetsController', () => {
balances: [],
unprocessedNetworks: [],
}),
invalidateBalances: jest.fn().mockResolvedValue(undefined),
},
} as unknown as ApiPlatformClient;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ type MockApiClient = {
fetchV2SupportedNetworks: jest.Mock;
fetchV5MultiAccountBalances: jest.Mock;
fetchV6MultiAccountBalances: jest.Mock;
invalidateBalances: jest.Mock;
};
};

Expand Down Expand Up @@ -87,6 +88,7 @@ function createMockApiClient(
unprocessedNetworks,
unprocessedIncludeAssetIds: [],
}),
invalidateBalances: jest.fn().mockResolvedValue(undefined),
},
};
}
Expand Down Expand Up @@ -519,25 +521,74 @@ describe('AccountsApiDataSource', () => {
expect(apiClient.accounts.fetchV5MultiAccountBalances).toHaveBeenCalledWith(
[`eip155:1:${MOCK_ADDRESS}`],
undefined,
undefined,
{ staleTime: 0, gcTime: 0 },
);

controller.destroy();
});

it('uses a short-lived TanStack cache window when forceUpdate is true', async () => {
const { controller, apiClient } = await setupController();
// Balances must never be served from the TanStack cache in any flow.
it.each([
{ forceUpdate: undefined },
{ forceUpdate: false },
{ forceUpdate: true },
])(
'always invalidates the balances cache and fetches with no-cache options (v5, forceUpdate: $forceUpdate)',
async ({ forceUpdate }) => {
const { controller, apiClient } = await setupController();

await controller.fetch(createDataRequest({ forceUpdate: true }));
await controller.fetch(createDataRequest({ forceUpdate }));

expect(apiClient.accounts.fetchV5MultiAccountBalances).toHaveBeenCalledWith(
[`eip155:1:${MOCK_ADDRESS}`],
undefined,
{ staleTime: 100, gcTime: 100 },
);
expect(apiClient.accounts.invalidateBalances).toHaveBeenCalledTimes(1);
expect(
apiClient.accounts.fetchV5MultiAccountBalances,
).toHaveBeenCalledWith([`eip155:1:${MOCK_ADDRESS}`], undefined, {
staleTime: 0,
gcTime: 0,
});
// Invalidation must precede the fetch so a fresh cache entry cannot be
// reused.
expect(
apiClient.accounts.invalidateBalances.mock.invocationCallOrder[0],
).toBeLessThan(
apiClient.accounts.fetchV5MultiAccountBalances.mock
.invocationCallOrder[0],
);

controller.destroy();
});
controller.destroy();
},
);

it.each([
{ forceUpdate: undefined },
{ forceUpdate: false },
{ forceUpdate: true },
])(
'always invalidates the balances cache and fetches with no-cache options (v6, forceUpdate: $forceUpdate)',
async ({ forceUpdate }) => {
const { controller, apiClient } = await setupController({
remoteFeatureFlags: { assetsAccountsApiV6: { value: true } },
});

await controller.fetch(createDataRequest({ forceUpdate }));

expect(apiClient.accounts.invalidateBalances).toHaveBeenCalledTimes(1);
expect(
apiClient.accounts.fetchV6MultiAccountBalances,
).toHaveBeenCalledWith([`eip155:1:${MOCK_ADDRESS}`], undefined, {
staleTime: 0,
gcTime: 0,
});
expect(
apiClient.accounts.invalidateBalances.mock.invocationCallOrder[0],
).toBeLessThan(
apiClient.accounts.fetchV6MultiAccountBalances.mock
.invocationCallOrder[0],
);

controller.destroy();
},
);

it('fetch processes balance response', async () => {
const balances = [
Expand Down Expand Up @@ -696,11 +747,10 @@ describe('AccountsApiDataSource', () => {

expect(
apiClient.accounts.fetchV6MultiAccountBalances,
).toHaveBeenCalledWith(
[`eip155:1:${MOCK_ADDRESS}`],
undefined,
undefined,
);
).toHaveBeenCalledWith([`eip155:1:${MOCK_ADDRESS}`], undefined, {
staleTime: 0,
gcTime: 0,
});
expect(
apiClient.accounts.fetchV5MultiAccountBalances,
).not.toHaveBeenCalled();
Expand Down Expand Up @@ -861,11 +911,10 @@ describe('AccountsApiDataSource', () => {

expect(
apiClient.accounts.fetchV6MultiAccountBalances,
).toHaveBeenCalledWith(
[`eip155:1:${MOCK_ADDRESS}`],
undefined,
undefined,
);
).toHaveBeenCalledWith([`eip155:1:${MOCK_ADDRESS}`], undefined, {
staleTime: 0,
gcTime: 0,
});

controller.destroy();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -427,9 +427,13 @@ export class AccountsApiDataSource extends AbstractDataSource<
return response;
}

const fetchOptions = request.forceUpdate
? { staleTime: 100, gcTime: 100 }
: undefined;
// Balances must never be served from the TanStack cache in any flow
// (polling, forced refresh, websocket-triggered): invalidate cached
// balance queries first (fetchQuery refetches invalidated queries
// regardless of freshness), then fetch with staleTime/gcTime 0 so the
// response is neither read back nor retained.
const fetchOptions = { staleTime: 0, gcTime: 0 };

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.

passing this to 0 now , we can also consider removing it

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.

Reverts this PR:
#9591

Worth double checking - I think instead of caching, we should attempt debouncing. But open to discuss

await this.#apiClient.accounts.invalidateBalances();

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.

staleTime/gcTime only apply to this one call, old entries in the shared cache keep their 60s freshness. Invalidating first marks them stale for all consumers and forces an unconditional refetch, guaranteeing balances never come from cache.

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.

Yes, 100% agreed here


// Feature-flagged: v6 endpoint with a fallback to legacy v5. The flag is
// read here (not cached) so a runtime toggle can revert v6 -> v5.
Expand Down Expand Up @@ -482,13 +486,15 @@ export class AccountsApiDataSource extends AbstractDataSource<
* Fetch balances from the legacy v5 endpoint and process them.
*
* @param accountIds - CAIP-10 account IDs to fetch balances for.
* @param fetchOptions - Cache/fetch options (e.g. force update settings).
* @param fetchOptions - No-cache fetch options (balances are never cached).
* @param fetchOptions.staleTime - Time in ms before cached data is stale.
* @param fetchOptions.gcTime - Time in ms before cached data is removed.
* @param request - The original data request containing accounts to map.
* @returns Unprocessed networks and processed asset balances by account.
*/
async #fetchV5Balances(
accountIds: string[],
fetchOptions: { staleTime: number; gcTime: number } | undefined,
fetchOptions: { staleTime: number; gcTime: number },
request: DataRequest,
): Promise<{
unprocessedNetworks: string[];
Expand Down Expand Up @@ -519,13 +525,15 @@ export class AccountsApiDataSource extends AbstractDataSource<
* Fetch balances from the v6 endpoint and process them.
*
* @param accountIds - CAIP-10 account IDs to fetch balances for.
* @param fetchOptions - Cache/fetch options (e.g. force update settings).
* @param fetchOptions - No-cache fetch options (balances are never cached).
* @param fetchOptions.staleTime - Time in ms before cached data is stale.
* @param fetchOptions.gcTime - Time in ms before cached data is removed.
* @param request - The original data request containing accounts to map.
* @returns Unprocessed networks and processed asset balances by account.
*/
async #fetchV6Balances(
accountIds: string[],
fetchOptions: { staleTime: number; gcTime: number } | undefined,
fetchOptions: { staleTime: number; gcTime: number },
request: DataRequest,
): Promise<{
unprocessedNetworks: string[];
Expand Down
156 changes: 156 additions & 0 deletions packages/core-backend/src/api/accounts/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,162 @@ describe('AccountsApiClient', () => {
});
expect(mockFetch).not.toHaveBeenCalled();
});

describe('balances cache behavior', () => {
const v5Response = (balance: string): V5BalancesResponse => ({
count: 1,
unprocessedNetworks: [],
balances: [
{
object: 'token',
symbol: 'ETH',
name: 'Ethereum',
type: 'native',
decimals: 18,
assetId: 'eip155:1/slip44:60',
balance,
accountId: 'eip155:1:0x123',
},
],
});

const v6Response = (balance: string): V6BalancesResponse => ({
unprocessedNetworks: [],
unprocessedIncludeAssetIds: [],
accounts: [
{
accountId: 'eip155:1:0x123',
balances: [
{
category: 'token',
assetId: 'eip155:1/slip44:60',
name: 'Ethereum',
symbol: 'ETH',
decimals: 18,
balance,
},
],
processingDefiPositions: false,
},
],
});

it('reuses a fresh cached v5 response when no fetch options are passed', async () => {
mockFetch.mockResolvedValueOnce(createMockResponse(v5Response('1')));

const first = await client.accounts.fetchV5MultiAccountBalances([
'eip155:1:0x123',
]);
const second = await client.accounts.fetchV5MultiAccountBalances([
'eip155:1:0x123',
]);

expect(mockFetch).toHaveBeenCalledTimes(1);
expect(second).toStrictEqual(first);
});

it('refetches v5 balances over the network after invalidateBalances with no-cache options, even when a fresh cache entry exists', async () => {
mockFetch
.mockResolvedValueOnce(createMockResponse(v5Response('1')))
.mockResolvedValueOnce(createMockResponse(v5Response('2')));

await client.accounts.fetchV5MultiAccountBalances(['eip155:1:0x123']);
await client.accounts.invalidateBalances();
const result = await client.accounts.fetchV5MultiAccountBalances(
['eip155:1:0x123'],
undefined,
{ staleTime: 0, gcTime: 0 },
);

expect(mockFetch).toHaveBeenCalledTimes(2);
expect(result.balances[0]?.balance).toBe('2');
});

it('never reuses cached v5 balances across consecutive fetches with no-cache options', async () => {
mockFetch
.mockResolvedValueOnce(createMockResponse(v5Response('1')))
.mockResolvedValueOnce(createMockResponse(v5Response('2')))
.mockResolvedValueOnce(createMockResponse(v5Response('3')));

const noCache = { staleTime: 0, gcTime: 0 };
await client.accounts.fetchV5MultiAccountBalances(
['eip155:1:0x123'],
undefined,
noCache,
);
await client.accounts.fetchV5MultiAccountBalances(
['eip155:1:0x123'],
undefined,
noCache,
);
const result = await client.accounts.fetchV5MultiAccountBalances(
['eip155:1:0x123'],
undefined,
noCache,
);

expect(mockFetch).toHaveBeenCalledTimes(3);
expect(result.balances[0]?.balance).toBe('3');
});

it('reuses a fresh cached v6 response when no fetch options are passed', async () => {
mockFetch.mockResolvedValueOnce(createMockResponse(v6Response('1')));

const first = await client.accounts.fetchV6MultiAccountBalances([
'eip155:1:0x123',
]);
const second = await client.accounts.fetchV6MultiAccountBalances([
'eip155:1:0x123',
]);

expect(mockFetch).toHaveBeenCalledTimes(1);
expect(second).toStrictEqual(first);
});

it('refetches v6 balances over the network after invalidateBalances with no-cache options, even when a fresh cache entry exists', async () => {
mockFetch
.mockResolvedValueOnce(createMockResponse(v6Response('1')))
.mockResolvedValueOnce(createMockResponse(v6Response('2')));

await client.accounts.fetchV6MultiAccountBalances(['eip155:1:0x123']);
await client.accounts.invalidateBalances();
const result = await client.accounts.fetchV6MultiAccountBalances(
['eip155:1:0x123'],
undefined,
{ staleTime: 0, gcTime: 0 },
);

expect(mockFetch).toHaveBeenCalledTimes(2);
expect(result.accounts[0]?.balances[0]?.balance).toBe('2');
});

it('never reuses cached v6 balances across consecutive fetches with no-cache options', async () => {
mockFetch
.mockResolvedValueOnce(createMockResponse(v6Response('1')))
.mockResolvedValueOnce(createMockResponse(v6Response('2')))
.mockResolvedValueOnce(createMockResponse(v6Response('3')));

const noCache = { staleTime: 0, gcTime: 0 };
await client.accounts.fetchV6MultiAccountBalances(
['eip155:1:0x123'],
undefined,
noCache,
);
await client.accounts.fetchV6MultiAccountBalances(
['eip155:1:0x123'],
undefined,
noCache,
);
const result = await client.accounts.fetchV6MultiAccountBalances(
['eip155:1:0x123'],
undefined,
noCache,
);

expect(mockFetch).toHaveBeenCalledTimes(3);
expect(result.accounts[0]?.balances[0]?.balance).toBe('3');
});
});
});

describe('Transactions', () => {
Expand Down
Loading