diff --git a/packages/assets-controller/CHANGELOG.md b/packages/assets-controller/CHANGELOG.md index 737848319c..ad35f5f1fd 100644 --- a/packages/assets-controller/CHANGELOG.md +++ b/packages/assets-controller/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `RpcDataSource` no longer writes `{ amount: "NaN" }` entries into `assetsBalance` when state holds malformed native metadata ([#8781](https://github.com/MetaMask/core/pull/8781)) + - Existing native metadata in `assetsInfo` is now only reused when its `decimals` field is a finite, non-negative number (via a new `#hasValidDecimals` guard). Stale entries like `{ type: 'native', decimals: null, name: '', symbol: '' }` or `{ decimals: -1, ... }` are replaced with the chain-status stub (`{ type: 'native', symbol/name: chainStatus.nativeCurrency, decimals: 18 }`) so balance conversion has a usable `decimals` and consumers never see a negative `decimals` in `assetsInfo`. + - `decimals: 0` is treated as valid; `name` and `symbol` are not required. + - Decimals resolution in `#handleBalanceUpdate` and the manual-fetch path no longer relies on `??` to fall through from state to pipeline metadata. A new `#pickValidDecimals` helper picks the first source whose `decimals` is finite and non-negative, so a stale `decimals: NaN` (or `decimals: -1`) in state can no longer shadow the chain-status stub's `decimals: 18` and silently produce `amount: '0'` while `assetsInfo` reports `decimals: 18`. + - `#convertToHumanReadable` now defensively returns `'0'` when `decimals` isn't a finite non-negative number or when the raw balance can't be parsed, matching the existing safe fallback used in the error path. + ## [7.1.1] ### Changed diff --git a/packages/assets-controller/src/data-sources/RpcDataSource.test.ts b/packages/assets-controller/src/data-sources/RpcDataSource.test.ts index 6686e096df..df834db6f8 100644 --- a/packages/assets-controller/src/data-sources/RpcDataSource.test.ts +++ b/packages/assets-controller/src/data-sources/RpcDataSource.test.ts @@ -1553,6 +1553,225 @@ describe('RpcDataSource', () => { }); }); + describe('native metadata validation (#hasValidDecimals)', () => { + const NATIVE_ASSET_ID = 'eip155:1/slip44:60' as Caip19AssetId; + + const subscribeAndEmit = async ( + stateNativeMeta: unknown, + onAssetsUpdate: jest.Mock, + ): Promise => { + let balanceUpdateCallback: + | ((result: BalanceFetchResult) => void | Promise) + | null = null; + jest + .spyOn(BalanceFetcher.prototype, 'setOnBalanceUpdate') + .mockImplementation(function (this: BalanceFetcher, callback) { + balanceUpdateCallback = callback; + }); + + await withController( + { + actionHandlerOverrides: { + 'AssetsController:getState': () => ({ + ...getDefaultAssetsControllerState(), + assetsInfo: { [NATIVE_ASSET_ID]: stateNativeMeta }, + }), + }, + }, + async ({ controller }) => { + await controller.subscribe({ + request: createDataRequest(), + subscriptionId: 'test-sub', + isUpdate: false, + onAssetsUpdate, + }); + await balanceUpdateCallback?.( + createBalanceFetchResult({ + balances: [ + { + assetId: NATIVE_ASSET_ID, + balance: '1000000000000000000', + } as BalanceFetchResult['balances'][0], + ], + }), + ); + }, + ); + }; + + it('falls back to chain-status stub when state native metadata has null decimals', async () => { + const onAssetsUpdate = jest.fn(); + await subscribeAndEmit( + { + aggregators: ['dynamic'], + decimals: null, + name: '', + symbol: '', + type: 'native', + }, + onAssetsUpdate, + ); + + expect(onAssetsUpdate).toHaveBeenCalled(); + const [response] = onAssetsUpdate.mock.calls[0]; + expect(response.assetsInfo?.[NATIVE_ASSET_ID]).toStrictEqual({ + type: 'native', + symbol: 'ETH', + name: 'ETH', + decimals: 18, + }); + expect( + response.assetsBalance?.[MOCK_ACCOUNT_ID]?.[NATIVE_ASSET_ID]?.amount, + ).toBe('1'); + }); + + it('falls back to chain-status stub when state native metadata has NaN decimals', async () => { + const onAssetsUpdate = jest.fn(); + await subscribeAndEmit( + { + decimals: Number.NaN, + name: 'ETH', + symbol: 'ETH', + type: 'native', + }, + onAssetsUpdate, + ); + + const [response] = onAssetsUpdate.mock.calls[0]; + expect(response.assetsInfo?.[NATIVE_ASSET_ID]?.decimals).toBe(18); + // Regression: `decimals: NaN` in state must not bypass the pipeline's + // valid `decimals: 18` via `??`, otherwise the amount silently becomes + // '0' even though `assetsInfo` reports the correct decimals. + expect( + response.assetsBalance?.[MOCK_ACCOUNT_ID]?.[NATIVE_ASSET_ID]?.amount, + ).toBe('1'); + }); + + it('keeps existing native metadata when decimals is 0 (valid)', async () => { + const onAssetsUpdate = jest.fn(); + const existing = { + decimals: 0, + name: '', + symbol: '', + type: 'native' as const, + }; + await subscribeAndEmit(existing, onAssetsUpdate); + + const [response] = onAssetsUpdate.mock.calls[0]; + expect(response.assetsInfo?.[NATIVE_ASSET_ID]).toStrictEqual(existing); + }); + + it('keeps existing native metadata when name/symbol are missing but decimals is valid', async () => { + const onAssetsUpdate = jest.fn(); + const existing = { + decimals: 18, + type: 'native' as const, + }; + await subscribeAndEmit(existing, onAssetsUpdate); + + const [response] = onAssetsUpdate.mock.calls[0]; + expect(response.assetsInfo?.[NATIVE_ASSET_ID]).toStrictEqual(existing); + }); + + it('falls back to chain-status stub when state native metadata has negative decimals', async () => { + const onAssetsUpdate = jest.fn(); + await subscribeAndEmit( + { + decimals: -1, + name: 'X', + symbol: 'X', + type: 'native', + }, + onAssetsUpdate, + ); + + const [response] = onAssetsUpdate.mock.calls[0]; + // Regression: `#hasValidDecimals` previously accepted negative + // decimals (only checked `Number.isFinite`), so consumers saw stale + // `decimals: -1` in `assetsInfo` while the balance silently became + // `'0'`. The metadata guard must reject negatives so the chain-status + // stub takes over and the balance resolves correctly. + expect(response.assetsInfo?.[NATIVE_ASSET_ID]).toStrictEqual({ + type: 'native', + symbol: 'ETH', + name: 'ETH', + decimals: 18, + }); + expect( + response.assetsBalance?.[MOCK_ACCOUNT_ID]?.[NATIVE_ASSET_ID]?.amount, + ).toBe('1'); + }); + }); + + describe('convertToHumanReadable guards', () => { + const NATIVE_ASSET_ID = 'eip155:1/slip44:60' as Caip19AssetId; + + const runFetchWithStateMetadata = async ( + stateNativeMeta: unknown, + rawBalance: string, + ): Promise<{ + onAssetsUpdate: jest.Mock; + }> => { + let balanceUpdateCallback: + | ((result: BalanceFetchResult) => void | Promise) + | null = null; + jest + .spyOn(BalanceFetcher.prototype, 'setOnBalanceUpdate') + .mockImplementation(function (this: BalanceFetcher, callback) { + balanceUpdateCallback = callback; + }); + + const onAssetsUpdate = jest.fn(); + await withController( + { + actionHandlerOverrides: { + 'AssetsController:getState': () => ({ + ...getDefaultAssetsControllerState(), + assetsInfo: { [NATIVE_ASSET_ID]: stateNativeMeta }, + }), + }, + }, + async ({ controller }) => { + await controller.subscribe({ + request: createDataRequest(), + subscriptionId: 'test-sub', + isUpdate: false, + onAssetsUpdate, + }); + await balanceUpdateCallback?.( + createBalanceFetchResult({ + balances: [ + { + assetId: NATIVE_ASSET_ID, + balance: rawBalance, + } as BalanceFetchResult['balances'][0], + ], + }), + ); + }, + ); + + return { onAssetsUpdate }; + }; + + it('returns "0" amount when raw balance is not a number', async () => { + const { onAssetsUpdate } = await runFetchWithStateMetadata( + { + decimals: 18, + name: 'ETH', + symbol: 'ETH', + type: 'native', + }, + 'not-a-number', + ); + + const [response] = onAssetsUpdate.mock.calls[0]; + expect( + response.assetsBalance?.[MOCK_ACCOUNT_ID]?.[NATIVE_ASSET_ID]?.amount, + ).toBe('0'); + }); + }); + describe('handleDetectionUpdate (via callback)', () => { it('invokes onAssetsUpdate when TokenDetector callback runs', async () => { let detectionUpdateCallback: diff --git a/packages/assets-controller/src/data-sources/RpcDataSource.ts b/packages/assets-controller/src/data-sources/RpcDataSource.ts index a2bc1b4563..2bcb742c65 100644 --- a/packages/assets-controller/src/data-sources/RpcDataSource.ts +++ b/packages/assets-controller/src/data-sources/RpcDataSource.ts @@ -346,12 +346,30 @@ export class RpcDataSource extends AbstractDataSource< /** * Convert a raw balance to human-readable format using decimals. * + * Returns `'0'` when either input is invalid (e.g. `decimals` is `null`, + * `NaN`, negative or non-finite, or `rawBalance` cannot be parsed as a + * number). Defaulting to a fixed decimals value would silently produce + * wrong amounts; `'0'` keeps state safe and never lets `NaN` leak in. + * * @param rawBalance - The raw balance string. * @param decimals - The number of decimals for the token. - * @returns The human-readable balance string. + * @returns The human-readable balance string, or `'0'` when inputs are invalid. */ #convertToHumanReadable(rawBalance: string, decimals: number): string { + if (!Number.isFinite(decimals) || decimals < 0) { + log('Invalid decimals — defaulting balance to "0"', { + rawBalance, + decimals, + }); + return '0'; + } + const rawAmount = new BigNumberJS(rawBalance); + if (!rawAmount.isFinite()) { + log('Invalid raw balance — defaulting to "0"', { rawBalance, decimals }); + return '0'; + } + const divisor = new BigNumberJS(10).pow(decimals); return rawAmount.dividedBy(divisor).toFixed(); } @@ -383,7 +401,7 @@ export class RpcDataSource extends AbstractDataSource< // enriched by the price/info API with image, description, etc. // Only emit a minimal stub when there's nothing in state yet, // so we don't clobber that richer metadata on every balance refresh. - if (existingMeta) { + if (this.#hasValidDecimals(existingMeta)) { assetsInfo[balance.assetId] = existingMeta; } else { const chainStatus = this.#chainStatuses[chainId]; @@ -406,6 +424,50 @@ export class RpcDataSource extends AbstractDataSource< return assetsInfo; } + /** + * Type guard for metadata whose `decimals` is safe to use for balance + * conversion. + * + * Mirrors the validity rules in `#convertToHumanReadable` (finite and + * non-negative). Keeping these in sync ensures that whenever the metadata + * guard accepts a value, the balance guard will also accept it — so we + * never end up emitting metadata with `decimals: -1` while silently + * defaulting the balance to `'0'`. + * + * @param metadata - The metadata to check. + * @returns `true` if `decimals` is a finite, non-negative number. + */ + #hasValidDecimals( + metadata: AssetMetadata | undefined, + ): metadata is AssetMetadata { + return Boolean( + metadata && Number.isFinite(metadata.decimals) && metadata.decimals >= 0, + ); + } + + /** + * Pick the first valid `decimals` value from a list of metadata sources. + * + * `??` only short-circuits on `null`/`undefined`, so a stale state entry + * with `decimals: NaN` would otherwise win over a later source that holds + * a correct value (e.g. the chain-status stub produced by + * `#collectMetadataForBalances`). This helper treats `NaN`, negative, and + * non-finite values as missing so the next source can supply a usable one. + * + * @param metadatas - Metadata candidates in priority order. + * @returns The first finite `decimals` value, or `undefined` if none are valid. + */ + #pickValidDecimals( + ...metadatas: (AssetMetadata | undefined)[] + ): number | undefined { + for (const metadata of metadatas) { + if (this.#hasValidDecimals(metadata)) { + return metadata.decimals; + } + } + return undefined; + } + /** * Handle balance update from BalanceFetcher. * @@ -436,7 +498,7 @@ export class RpcDataSource extends AbstractDataSource< for (const balance of normalizedBalances) { const stateMetadata = existingMetadata[balance.assetId]; const pipelineMetadata = assetsInfo[balance.assetId]; - const decimals = stateMetadata?.decimals ?? pipelineMetadata?.decimals; + const decimals = this.#pickValidDecimals(stateMetadata, pipelineMetadata); if (decimals === undefined) { continue; @@ -1012,8 +1074,10 @@ export class RpcDataSource extends AbstractDataSource< for (const balance of normalizedBalances) { const stateMetadata = existingMetadata[balance.assetId]; const pipelineMetadata = assetsInfo[balance.assetId]; - let decimals: number | undefined = - stateMetadata?.decimals ?? pipelineMetadata?.decimals; + let decimals: number | undefined = this.#pickValidDecimals( + stateMetadata, + pipelineMetadata, + ); if (decimals === undefined) { const parsed = parseCaipAssetType(balance.assetId); @@ -1054,7 +1118,7 @@ export class RpcDataSource extends AbstractDataSource< // nothing is in state yet, so we don't clobber that richer metadata. const existingNativeMeta = this.#getExistingAssetsMetadata()[nativeAssetId]; - if (existingNativeMeta) { + if (this.#hasValidDecimals(existingNativeMeta)) { assetsInfo[nativeAssetId] = existingNativeMeta; } else { const chainStatus = this.#chainStatuses[chainId];