diff --git a/eslint-suppressions.json b/eslint-suppressions.json index 7dd9a6e62bd..1b6d03705b9 100644 --- a/eslint-suppressions.json +++ b/eslint-suppressions.json @@ -270,20 +270,12 @@ "count": 3 } }, - "packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts": { - "@typescript-eslint/explicit-function-return-type": { - "count": 1 - } - }, "packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts": { "@typescript-eslint/explicit-function-return-type": { - "count": 3 + "count": 2 }, "@typescript-eslint/naming-convention": { "count": 1 - }, - "@typescript-eslint/no-misused-promises": { - "count": 2 } }, "packages/assets-controllers/src/MultichainBalancesController/MultichainBalancesController.test.ts": { diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 8ddb069002b..ff35ae99ca6 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fix stale `accountsAssets` reference in `MultichainAssetsRatesController` causing incorrect conversion rates for non-EVM chains when currency is changed ([#7424](https://github.com/MetaMask/core/pull/7424)) + - `MultichainAssetsRatesController` now dynamically fetches fresh state instead of caching a reference that becomes stale due to Immer's immutable updates - **BREAKING:** `NftDetectionController` now calls a new function `NftController:addNfts` that reduces API calls to bulk-scan to batch multiple NFTs URLs ([#7411](https://github.com/MetaMask/core/pull/7411)) - Added decimal precision (default 9dp) for `CurrencyRateController` `conversionRate` and `conversionRate` properties. ([#7324](https://github.com/MetaMask/core/pull/7324)) - This fixes any BigNumber conversion errors due to exceeding the 15 significant digit limit diff --git a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts index b73b1b83e0d..8d643f7325e 100644 --- a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts +++ b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.test.ts @@ -1,4 +1,5 @@ import { deriveStateFromMetadata } from '@metamask/base-controller'; +import type { CaipAssetType } from '@metamask/keyring-api'; import { SolScope } from '@metamask/keyring-api'; import { SolMethod } from '@metamask/keyring-api'; import { SolAccountType } from '@metamask/keyring-api'; @@ -146,7 +147,11 @@ const setupController = ({ ConstructorParameters[0] >; accountsAssets?: InternalAccount[]; -} = {}) => { +} = {}): { + controller: MultichainAssetsRatesController; + messenger: RootMessenger; + updateSpy: jest.SpyInstance; +} => { const messenger: RootMessenger = new Messenger({ namespace: MOCK_ANY_NAMESPACE, }); @@ -1106,6 +1111,172 @@ describe('MultichainAssetsRatesController', () => { }); }); + describe('dynamic asset fetching', () => { + it('should fetch rates for assets added after controller initialization', async () => { + const messenger: RootMessenger = new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); + + // Initially, MultichainAssetsController has no assets + let multichainAssets: Record = {}; + + messenger.registerActionHandler( + 'MultichainAssetsController:getState', + () => ({ + accountsAssets: multichainAssets, + assetsMetadata: { + 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501': { + name: 'Solana', + symbol: 'SOL', + fungible: true, + iconUrl: 'https://example.com/solana.png', + units: [{ symbol: 'SOL', name: 'Solana', decimals: 9 }], + }, + }, + allIgnoredAssets: {}, + }), + ); + + messenger.registerActionHandler( + 'AccountsController:listMultichainAccounts', + () => [ + { + id: 'account1', + address: 'sol-address-1', + options: {}, + methods: [SolMethod.SignMessage, SolMethod.SignTransaction], + type: SolAccountType.DataAccount, + metadata: { + name: 'Test Solana Account', + importTime: Date.now(), + keyring: { + type: KeyringTypes.snap, + }, + snap: { + name: 'Test Snap', + id: 'test-snap', + enabled: true, + }, + }, + scopes: [SolScope.Mainnet], + }, + ], + ); + + messenger.registerActionHandler( + 'AccountsController:getSelectedMultichainAccount', + () => ({ + id: 'account1', + address: 'sol-address-1', + options: {}, + methods: [SolMethod.SignMessage, SolMethod.SignTransaction], + type: SolAccountType.DataAccount, + metadata: { + name: 'Test Solana Account', + importTime: Date.now(), + keyring: { + type: KeyringTypes.snap, + }, + snap: { + name: 'Test Snap', + id: 'test-snap', + enabled: true, + }, + }, + scopes: [SolScope.Mainnet], + }), + ); + + messenger.registerActionHandler( + 'CurrencyRateController:getState', + () => ({ + currentCurrency: 'USD', + currencyRates: {}, + }), + ); + + const snapHandler = jest.fn().mockResolvedValue({ + conversionRates: { + 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501': { + 'swift:0/iso4217:USD': { + rate: '150.00', + conversionTime: Date.now(), + }, + }, + }, + }); + + messenger.registerActionHandler( + 'SnapController:handleRequest', + snapHandler, + ); + + const multichainAssetsRatesControllerMessenger = new Messenger< + 'MultichainAssetsRatesController', + AllMultichainAssetsRateControllerActions, + AllMultichainAssetsRateControllerEvents, + RootMessenger + >({ + namespace: 'MultichainAssetsRatesController', + parent: messenger, + }); + + messenger.delegate({ + messenger: multichainAssetsRatesControllerMessenger, + actions: [ + 'MultichainAssetsController:getState', + 'AccountsController:listMultichainAccounts', + 'AccountsController:getSelectedMultichainAccount', + 'CurrencyRateController:getState', + 'SnapController:handleRequest', + ], + events: [ + 'KeyringController:lock', + 'KeyringController:unlock', + 'AccountsController:accountAdded', + 'CurrencyRateController:stateChange', + 'MultichainAssetsController:accountAssetListUpdated', + ], + }); + + jest + .spyOn(KeyringClient.prototype, 'listAccountAssets') + .mockResolvedValue([]); + + const controller = new MultichainAssetsRatesController({ + messenger: multichainAssetsRatesControllerMessenger, + }); + + // Initial fetch should return empty because no assets exist yet + await controller.updateAssetsRates(); + expect(controller.state.conversionRates).toStrictEqual({}); + expect(snapHandler).not.toHaveBeenCalled(); + + // Simulate new wallet import: MultichainAssetsController now has assets + multichainAssets = { + account1: ['solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501'], + }; + + jest + .spyOn(KeyringClient.prototype, 'listAccountAssets') + .mockResolvedValue([ + 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501', + ]); + + // Fetch again - should now pick up the new assets + await controller.updateAssetsRates(); + + // Verify that rates were fetched for the newly added asset + expect(snapHandler).toHaveBeenCalled(); + expect(controller.state.conversionRates).toMatchObject({ + 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501': { + rate: '150.00', + currency: 'swift:0/iso4217:USD', + }, + }); + }); + }); + describe('metadata', () => { it('includes expected state in debug snapshots', () => { const { controller } = setupController(); diff --git a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts index eec4daccb7b..ac0c6f65fe4 100644 --- a/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts +++ b/packages/assets-controllers/src/MultichainAssetsRatesController/MultichainAssetsRatesController.ts @@ -43,7 +43,6 @@ import type { import type { MultichainAssetsControllerGetStateAction, MultichainAssetsControllerAccountAssetListUpdatedEvent, - MultichainAssetsControllerState, } from '../MultichainAssetsController'; /** @@ -203,8 +202,6 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro #currentCurrency: CurrencyRateState['currentCurrency']; - readonly #accountsAssets: MultichainAssetsControllerState['accountsAssets']; - #isUnlocked = true; /** @@ -244,16 +241,13 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro this.#isUnlocked = true; }); - ({ accountsAssets: this.#accountsAssets } = this.messenger.call( - 'MultichainAssetsController:getState', - )); - ({ currentCurrency: this.#currentCurrency } = this.messenger.call( 'CurrencyRateController:getState', )); this.messenger.subscribe( 'CurrencyRateController:stateChange', + // eslint-disable-next-line @typescript-eslint/no-misused-promises async (currentCurrency: string) => { this.#currentCurrency = currentCurrency; await this.updateAssetsRates(); @@ -264,6 +258,7 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro this.messenger.subscribe( 'MultichainAssetsController:accountAssetListUpdated', + // eslint-disable-next-line @typescript-eslint/no-misused-promises async ({ assets }) => { const newAccountAssets = Object.entries(assets).map( ([accountId, { added }]) => ({ @@ -370,7 +365,7 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro async updateAssetsRates(): Promise { const releaseLock = await this.#mutex.acquire(); - return (async () => { + return (async (): Promise => { if (!this.isActive) { return; } @@ -683,7 +678,12 @@ export class MultichainAssetsRatesController extends StaticIntervalPollingContro * @returns An array of CAIP-19 assets. */ #getAssetsForAccount(accountId: string): CaipAssetType[] { - return this.#accountsAssets?.[accountId] ?? []; + // Always fetch fresh state - MultichainAssetsController uses Immer which creates + // new object references on every update, so caching would become stale. + const { accountsAssets } = this.messenger.call( + 'MultichainAssetsController:getState', + ); + return accountsAssets?.[accountId] ?? []; } /**