Skip to content

Commit d405c7d

Browse files
authored
fix: force timeout after 30s token detection (#7106)
## Explanation ### What is the current state and why does it need to change? Currently, when `TokenDetectionController` uses the Accounts API to detect tokens, API calls can hang indefinitely if the API is slow, unresponsive, or experiencing network issues. This causes the entire token detection process to freeze without any fallback mechanism, resulting in a poor user experience where tokens are never detected. ### What is the solution and how does it work? This PR adds 30-second timeout protection for Accounts API calls in `TokenDetectionController`: - Added `ACCOUNTS_API_TIMEOUT_MS` constant (30000ms) to define the timeout threshold - Wrapped Accounts API calls with `Promise.race()` between the actual API call and a timeout promise - When timeout occurs, the promise rejects and is caught, triggering an automatic fallback to RPC-based token detection - Properly cleans up the timeout in a `finally` block to prevent memory leaks - Includes error logging for debugging timeout and failure events The timeout mechanism ensures that: 1. If the API responds within 30 seconds, detection proceeds normally via the API 2. If the API takes longer than 30 seconds, the timeout fires and RPC detection takes over 3. Users always get token detection results, either via API or RPC fallback ### Changes that might not be obvious The timeout is implemented in the `#attemptAccountAPIDetection` private method, which wraps the existing `#addDetectedTokensViaAPI` call. This ensures the timeout protection applies to all Accounts API token detection flows without requiring changes to the core detection logic. ## References - Improves reliability of token detection by preventing indefinite hangs - Ensures users always get token detection results through automatic RPC fallback - Related to ongoing work to improve Accounts API reliability and user experience ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - Added comprehensive timeout test using fake timers to simulate 30-second timeout scenario - Test verifies that API is called, timeout triggers, RPC fallback occurs, and tokens are successfully added - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - Added inline comments explaining timeout mechanism and cleanup - [x] I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary - Updated `CHANGELOG.md` with timeout protection fix - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes - Not applicable: This is an internal improvement with no breaking changes ## Technical Implementation Details ### Key Files Changed 1. **`TokenDetectionController.ts`** - Added `ACCOUNTS_API_TIMEOUT_MS` constant (30000ms) - Modified `#attemptAccountAPIDetection` method to implement timeout protection - Uses `Promise.race()` to race between API call and timeout promise - Includes proper cleanup in `finally` block to prevent memory leaks 2. **`TokenDetectionController.test.ts`** - Added test case: `'should timeout and fallback to RPC when Accounts API call takes longer than 30 seconds'` - Uses `sinon.useFakeTimers()` to simulate time advancement - Verifies complete flow: API call → timeout → RPC fallback → token addition ### Code Flow ``` detectTokens() ↓ #attemptAccountAPIDetection() ↓ Promise.race([ #addDetectedTokensViaAPI() ← actual API call timeout promise (30s) ← timeout protection ]) ↓ Success → continue with API results ↓ Timeout/Failure → fallback to RPC detection ↓ finally → cleanup timeout ``` ### Testing Strategy The test uses fake timers to simulate the 30-second timeout without actually waiting 30 seconds: 1. Mock API call to never resolve (simulates hanging request) 2. Start detection process 3. Advance fake timers by 30 seconds 4. Verify timeout triggered 5. Verify RPC fallback occurred 6. Verify tokens were successfully added ## Impact ### User Experience - **Before**: Token detection could hang indefinitely, leaving users without their tokens - **After**: Token detection always completes within 30 seconds, with automatic RPC fallback ### Performance - No performance impact on successful API calls (they complete normally) - Failed/slow API calls are cut off at 30 seconds instead of hanging forever - Memory leak prevention through proper timeout cleanup ### Reliability - Significantly improves reliability of token detection - Provides graceful degradation when Accounts API is experiencing issues - Maintains existing functionality while adding safety net <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Adds a 30s timeout to Accounts API token detection with RPC fallback, and refactors balance fetchers to return `unprocessedChainIds` so controllers can retry unsupported chains via RPC. > > - **Token Detection (`TokenDetectionController`)**: > - Add `ACCOUNTS_API_TIMEOUT_MS` (30s) and wrap API calls with `safelyExecuteWithTimeout`; on timeout/failure, fall back to RPC. > - Process Accounts API `unprocessedNetworks` and add those chains to RPC detection. > - **Balance Fetchers**: > - Change `fetch()` to return `{ balances, unprocessedChainIds }` in `AccountsApiBalanceFetcher` and `RpcBalanceFetcher`. > - Convert Accounts API `unprocessedNetworks` (decimal) to hex `unprocessedChainIds`; aggregate across batches. > - For `AccountTracker` RPC fetcher wrapper, filter out staked entries when `includeStakedAssets` is false while preserving `unprocessedChainIds`. > - **Controllers**: > - Update `AccountTrackerController` and `TokenBalancesController` to consume new fetcher result shape and re-queue `unprocessedChainIds` for fallback fetchers. > - **Tests & Misc**: > - Update tests across fetchers/controllers for new return type and timeout/fallback behavior; add coverage for missing currencies in `CurrencyRateController` responses. > - Update `CHANGELOG.md` to reflect timeout protection and unprocessed network handling. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 7a813ba. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 46b3468 commit d405c7d

12 files changed

+684
-123
lines changed

packages/assets-controllers/CHANGELOG.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
12+
- Add 30-second timeout protection for Accounts API calls in `TokenDetectionController` to prevent hanging requests ([#7106](https://github.com/MetaMask/core/pull/7106))
13+
- Prevents token detection from hanging indefinitely on slow or unresponsive API requests
14+
- Automatically falls back to RPC-based token detection when API call times out or fails
15+
- Includes error logging for debugging timeout and failure events
16+
- Handle `unprocessedNetworks` from Accounts API responses to ensure complete token detection coverage ([#7106](https://github.com/MetaMask/core/pull/7106))
17+
- When Accounts API returns networks it cannot process, those networks are automatically added to RPC detection
18+
- Applies to both `TokenDetectionController` and `TokenBalancesController`
19+
- Ensures all requested networks are processed even if API has partial support
20+
1021
## [88.0.0]
1122

1223
### Changed

packages/assets-controllers/src/AccountTrackerController.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -594,6 +594,57 @@ describe('AccountTrackerController', () => {
594594
},
595595
);
596596
});
597+
598+
it('should create account entry when applying staked balance without native balance (line 743)', async () => {
599+
// Mock returning staked balance for ADDRESS_1 and native balance for ADDRESS_2
600+
// but NO native balance for ADDRESS_1 - this tests the defensive check on line 743
601+
// Use lowercase addresses since queryAllAccounts: true uses lowercase
602+
mockedGetTokenBalancesForMultipleAddresses.mockResolvedValueOnce({
603+
tokenBalances: {
604+
'0x0000000000000000000000000000000000000000': {
605+
// Only ADDRESS_2 has native balance, ADDRESS_1 doesn't
606+
[ADDRESS_2]: new BN('100', 16),
607+
},
608+
},
609+
stakedBalances: {
610+
// ADDRESS_1 has staked balance but no native balance
611+
[ADDRESS_1]: new BN('2', 16), // 0x2
612+
[ADDRESS_2]: new BN('3', 16), // 0x3
613+
},
614+
});
615+
616+
await withController(
617+
{
618+
options: {
619+
includeStakedAssets: true,
620+
getStakedBalanceForChain: mockGetStakedBalanceForChain,
621+
},
622+
isMultiAccountBalancesEnabled: true,
623+
selectedAccount: ACCOUNT_1,
624+
listAccounts: [ACCOUNT_1, ACCOUNT_2],
625+
},
626+
async ({ controller, refresh }) => {
627+
await refresh(clock, ['mainnet'], true);
628+
629+
// Line 743 should have created an account entry with balance '0x0' for ADDRESS_1
630+
// when applying staked balance without a native balance entry
631+
expect(controller.state).toStrictEqual({
632+
accountsByChainId: {
633+
'0x1': {
634+
[CHECKSUM_ADDRESS_1]: {
635+
balance: '0x0', // Created by line 743 (defensive check)
636+
stakedBalance: '0x2',
637+
},
638+
[CHECKSUM_ADDRESS_2]: {
639+
balance: '0x100',
640+
stakedBalance: '0x3',
641+
},
642+
},
643+
},
644+
});
645+
},
646+
);
647+
});
597648
});
598649

599650
describe('with networkClientId', () => {

packages/assets-controllers/src/AccountTrackerController.ts

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -91,14 +91,19 @@ function createAccountTrackerRpcBalanceFetcher(
9191
},
9292

9393
async fetch(params) {
94-
const balances = await rpcBalanceFetcher.fetch(params);
94+
const result = await rpcBalanceFetcher.fetch(params);
9595

9696
if (!includeStakedAssets) {
9797
// Filter out staked balances from the results
98-
return balances.filter((balance) => balance.token === ZERO_ADDRESS);
98+
return {
99+
balances: result.balances.filter(
100+
(balance) => balance.token === ZERO_ADDRESS,
101+
),
102+
unprocessedChainIds: result.unprocessedChainIds,
103+
};
99104
}
100105

101-
return balances;
106+
return result;
102107
},
103108
};
104109
}
@@ -630,21 +635,38 @@ export class AccountTrackerController extends StaticIntervalPollingController<Ac
630635
}
631636

632637
try {
633-
const balances = await fetcher.fetch({
638+
const result = await fetcher.fetch({
634639
chainIds: supportedChains,
635640
queryAllAccounts,
636641
selectedAccount,
637642
allAccounts,
638643
});
639644

640-
if (balances && balances.length > 0) {
641-
aggregated.push(...balances);
645+
if (result.balances && result.balances.length > 0) {
646+
aggregated.push(...result.balances);
642647
// Remove chains that were successfully processed
643-
const processedChains = new Set(balances.map((b) => b.chainId));
648+
const processedChains = new Set(
649+
result.balances.map((b) => b.chainId),
650+
);
644651
remainingChains = remainingChains.filter(
645652
(chain) => !processedChains.has(chain),
646653
);
647654
}
655+
656+
// Add unprocessed chains back to remainingChains for next fetcher
657+
if (
658+
result.unprocessedChainIds &&
659+
result.unprocessedChainIds.length > 0
660+
) {
661+
// Only add chains that were originally requested and aren't already in remainingChains
662+
const currentRemainingChains = remainingChains;
663+
const chainsToAdd = result.unprocessedChainIds.filter(
664+
(chainId) =>
665+
supportedChains.includes(chainId) &&
666+
!currentRemainingChains.includes(chainId),
667+
);
668+
remainingChains.push(...chainsToAdd);
669+
}
648670
} catch (error) {
649671
console.warn(
650672
`Balance fetcher failed for chains ${supportedChains.join(', ')}: ${String(error)}`,

packages/assets-controllers/src/CurrencyRateController.test.ts

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -885,6 +885,100 @@ describe('CurrencyRateController', () => {
885885
controller.destroy();
886886
});
887887

888+
it('should set conversionDate to null when currency not found in price api response (lines 201-202)', async () => {
889+
jest.spyOn(global.Date, 'now').mockImplementation(() => getStubbedDate());
890+
891+
const messenger = getCurrencyRateControllerMessenger();
892+
893+
const tokenPricesService = buildMockTokenPricesService();
894+
895+
// Mock price API response where BNB is not included
896+
jest.spyOn(tokenPricesService, 'fetchExchangeRates').mockResolvedValue({
897+
eth: {
898+
name: 'Ether',
899+
ticker: 'eth',
900+
value: 1 / 1000,
901+
usd: 1 / 3000,
902+
currencyType: 'crypto',
903+
},
904+
// BNB is missing from the response
905+
});
906+
907+
const controller = new CurrencyRateController({
908+
messenger,
909+
state: { currentCurrency: 'xyz' },
910+
tokenPricesService,
911+
});
912+
913+
await controller.updateExchangeRate(['ETH', 'BNB']);
914+
915+
const conversionDate = getStubbedDate() / 1000;
916+
expect(controller.state).toStrictEqual({
917+
currentCurrency: 'xyz',
918+
currencyRates: {
919+
ETH: {
920+
conversionDate,
921+
conversionRate: 1000,
922+
usdConversionRate: 3000,
923+
},
924+
BNB: {
925+
conversionDate: null, // Line 201: rate === undefined
926+
conversionRate: null, // Line 202
927+
usdConversionRate: null,
928+
},
929+
},
930+
});
931+
932+
controller.destroy();
933+
});
934+
935+
it('should set conversionDate to null when currency not found in crypto compare response (lines 231-232)', async () => {
936+
jest.spyOn(global.Date, 'now').mockImplementation(() => getStubbedDate());
937+
const cryptoCompareHost = 'https://min-api.cryptocompare.com';
938+
nock(cryptoCompareHost)
939+
.get('/data/pricemulti?fsyms=ETH,BNB&tsyms=xyz')
940+
.reply(200, {
941+
ETH: { XYZ: 4000.42 },
942+
// BNB is missing from the response
943+
})
944+
.persist();
945+
946+
const messenger = getCurrencyRateControllerMessenger();
947+
const tokenPricesService = buildMockTokenPricesService();
948+
949+
// Make price API fail so it falls back to CryptoCompare
950+
jest
951+
.spyOn(tokenPricesService, 'fetchExchangeRates')
952+
.mockRejectedValue(new Error('Failed to fetch'));
953+
954+
const controller = new CurrencyRateController({
955+
messenger,
956+
state: { currentCurrency: 'xyz' },
957+
tokenPricesService,
958+
});
959+
960+
await controller.updateExchangeRate(['ETH', 'BNB']);
961+
962+
const conversionDate = getStubbedDate() / 1000;
963+
expect(controller.state).toStrictEqual({
964+
currentCurrency: 'xyz',
965+
currencyRates: {
966+
ETH: {
967+
conversionDate,
968+
conversionRate: 4000.42,
969+
usdConversionRate: null,
970+
},
971+
BNB: {
972+
conversionDate: null, // Line 231: rate === undefined
973+
conversionRate: null, // Line 232
974+
usdConversionRate: null,
975+
},
976+
},
977+
});
978+
979+
controller.destroy();
980+
});
981+
888982
describe('useExternalServices', () => {
889983
it('should not fetch exchange rates when useExternalServices is false', async () => {
890984
const fetchMultiExchangeRateStub = jest.fn();

packages/assets-controllers/src/TokenBalancesController.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1459,7 +1459,7 @@ describe('TokenBalancesController', () => {
14591459

14601460
// Mock the RPC balance fetcher's fetch method to verify the parameter
14611461
const mockRpcFetch = jest.spyOn(RpcBalanceFetcher.prototype, 'fetch');
1462-
mockRpcFetch.mockResolvedValueOnce([]);
1462+
mockRpcFetch.mockResolvedValueOnce({ balances: [] });
14631463

14641464
const { controller } = setupController({
14651465
config: {
@@ -1771,7 +1771,7 @@ describe('TokenBalancesController', () => {
17711771
// Mock empty aggregated results
17721772
const mockFetcher = {
17731773
supports: jest.fn().mockReturnValue(true),
1774-
fetch: jest.fn().mockResolvedValue([]), // Return empty array
1774+
fetch: jest.fn().mockResolvedValue({ balances: [] }), // Return empty result
17751775
};
17761776

17771777
// Replace the balance fetchers with our mock
@@ -4250,7 +4250,7 @@ describe('TokenBalancesController', () => {
42504250

42514251
// Mock AccountsApiBalanceFetcher to track when line 320 logic is executed
42524252
const mockSupports = jest.fn().mockReturnValue(true);
4253-
const mockApiFetch = jest.fn().mockResolvedValue([]);
4253+
const mockApiFetch = jest.fn().mockResolvedValue({ balances: [] });
42544254

42554255
const apiBalanceFetcher = jest.requireActual(
42564256
'./multi-chain-accounts-service/api-balance-fetcher',

packages/assets-controllers/src/TokenBalancesController.ts

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -653,21 +653,37 @@ export class TokenBalancesController extends StaticIntervalPollingController<{
653653
}
654654

655655
try {
656-
const balances = await fetcher.fetch({
656+
const result = await fetcher.fetch({
657657
chainIds: supportedChains,
658658
queryAllAccounts: queryAllAccounts ?? this.#queryAllAccounts,
659659
selectedAccount: selected as ChecksumAddress,
660660
allAccounts,
661661
});
662662

663-
if (balances && balances.length > 0) {
664-
aggregated.push(...balances);
663+
if (result.balances && result.balances.length > 0) {
664+
aggregated.push(...result.balances);
665665
// Remove chains that were successfully processed
666-
const processedChains = new Set(balances.map((b) => b.chainId));
666+
const processedChains = new Set(
667+
result.balances.map((b) => b.chainId),
668+
);
667669
remainingChains = remainingChains.filter(
668670
(chain) => !processedChains.has(chain),
669671
);
670672
}
673+
674+
// Add unprocessed chains back to remainingChains for next fetcher
675+
if (
676+
result.unprocessedChainIds &&
677+
result.unprocessedChainIds.length > 0
678+
) {
679+
const currentRemainingChains = remainingChains;
680+
const chainsToAdd = result.unprocessedChainIds.filter(
681+
(chainId) =>
682+
supportedChains.includes(chainId) &&
683+
!currentRemainingChains.includes(chainId),
684+
);
685+
remainingChains.push(...chainsToAdd);
686+
}
671687
} catch (error) {
672688
console.warn(
673689
`Balance fetcher failed for chains ${supportedChains.join(', ')}: ${String(error)}`,

0 commit comments

Comments
 (0)