From a714ffb4d080632006d2d54954ed0d788680b4b8 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 13 Jul 2023 20:15:59 -0230 Subject: [PATCH] Prevent bug where incorrect symbol used for token rates (#1496) There is a bug present in the TokenRatesController where a network switch can result in token rates being requested for the wrong network. This can happen when the network state update event comes before currency rate controller state update event. The controller only works correctly if the currency rate controller state update event happens first. Instead of getting the ticker from the currency rate controller, we now get it directly from the network controller. This simplifies the controller and ensures that we always use the currency symbol corresponding with the current chain ID. --- .../src/TokenRatesController.test.ts | 27 +++++-------------- .../src/TokenRatesController.ts | 14 ++-------- 2 files changed, 9 insertions(+), 32 deletions(-) diff --git a/packages/assets-controllers/src/TokenRatesController.test.ts b/packages/assets-controllers/src/TokenRatesController.test.ts index bf3c484cf55..657ced4bb8b 100644 --- a/packages/assets-controllers/src/TokenRatesController.test.ts +++ b/packages/assets-controllers/src/TokenRatesController.test.ts @@ -122,7 +122,6 @@ describe('TokenRatesController', () => { chainId: toHex(1), ticker: NetworksTicker.mainnet, onTokensStateChange: sinon.stub(), - onCurrencyRateStateChange: sinon.stub(), onNetworkStateChange: sinon.stub(), }); expect(controller.state).toStrictEqual({ @@ -135,7 +134,6 @@ describe('TokenRatesController', () => { chainId: toHex(1), ticker: NetworksTicker.mainnet, onTokensStateChange: sinon.stub(), - onCurrencyRateStateChange: sinon.stub(), onNetworkStateChange: sinon.stub(), }); expect(controller.config).toStrictEqual({ @@ -153,7 +151,6 @@ describe('TokenRatesController', () => { chainId: toHex(1), ticker: NetworksTicker.mainnet, onTokensStateChange: sinon.stub(), - onCurrencyRateStateChange: sinon.stub(), onNetworkStateChange: sinon.stub(), }); expect(() => console.log(controller.tokens)).toThrow( @@ -170,7 +167,6 @@ describe('TokenRatesController', () => { chainId: toHex(1), ticker: NetworksTicker.mainnet, onTokensStateChange: jest.fn(), - onCurrencyRateStateChange: jest.fn(), onNetworkStateChange: jest.fn(), }, { @@ -194,7 +190,6 @@ describe('TokenRatesController', () => { chainId: toHex(1), ticker: NetworksTicker.mainnet, onTokensStateChange: sinon.stub(), - onCurrencyRateStateChange: sinon.stub(), onNetworkStateChange: sinon.stub(), }, { @@ -214,7 +209,6 @@ describe('TokenRatesController', () => { chainId: toHex(1), ticker: NetworksTicker.mainnet, onTokensStateChange: sinon.stub(), - onCurrencyRateStateChange: sinon.stub(), onNetworkStateChange: sinon.stub(), }, { interval: 1337 }, @@ -249,7 +243,6 @@ describe('TokenRatesController', () => { chainId: toHex(1), ticker: NetworksTicker.mainnet, onTokensStateChange: (listener) => tokensController.subscribe(listener), - onCurrencyRateStateChange: sinon.stub(), onNetworkStateChange: (listener) => messenger.subscribe('NetworkController:stateChange', listener), }, @@ -278,7 +271,6 @@ describe('TokenRatesController', () => { chainId: toHex(1), ticker: NetworksTicker.mainnet, onTokensStateChange: sinon.stub(), - onCurrencyRateStateChange: sinon.stub(), onNetworkStateChange: sinon.stub(), }, { interval: 10 }, @@ -301,14 +293,12 @@ describe('TokenRatesController', () => { const onTokensStateChange = sinon.stub().callsFake((listener) => { tokenStateChangeListener = listener; }); - const onCurrencyRateStateChange = sinon.stub(); const onNetworkStateChange = sinon.stub(); const controller = new TokenRatesController( { chainId: toHex(1), ticker: NetworksTicker.mainnet, onTokensStateChange, - onCurrencyRateStateChange, onNetworkStateChange, }, { interval: 10 }, @@ -324,19 +314,17 @@ describe('TokenRatesController', () => { expect(updateExchangeRatesStub.callCount).toStrictEqual(2); }); - it('should update exchange rates when native currency changes', async () => { - let currencyRateStateChangeListener: (state: any) => void; + it('should update exchange rates when ticker changes', async () => { + let networkStateChangeListener: (state: any) => void; const onTokensStateChange = sinon.stub(); - const onCurrencyRateStateChange = sinon.stub().callsFake((listener) => { - currencyRateStateChangeListener = listener; + const onNetworkStateChange = sinon.stub().callsFake((listener) => { + networkStateChangeListener = listener; }); - const onNetworkStateChange = sinon.stub(); const controller = new TokenRatesController( { chainId: toHex(1), ticker: NetworksTicker.mainnet, onTokensStateChange, - onCurrencyRateStateChange, onNetworkStateChange, }, { interval: 10 }, @@ -347,7 +335,9 @@ describe('TokenRatesController', () => { 'updateExchangeRates', ); // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - currencyRateStateChangeListener!({ nativeCurrency: 'dai' }); + networkStateChangeListener!({ + providerConfig: { chainId: toHex(1), ticker: 'dai' }, + }); // FIXME: This is now being called twice expect(updateExchangeRatesStub.callCount).toStrictEqual(2); }); @@ -387,7 +377,6 @@ describe('TokenRatesController', () => { chainId: toHex(137), ticker: 'MATIC', onTokensStateChange, - onCurrencyRateStateChange: sinon.stub(), onNetworkStateChange, }, { interval: 10 }, @@ -451,7 +440,6 @@ describe('TokenRatesController', () => { ticker: NetworksTicker.mainnet, onTokensStateChange, onNetworkStateChange, - onCurrencyRateStateChange: sinon.stub(), }, { interval: 10 }, ); @@ -525,7 +513,6 @@ describe('TokenRatesController', () => { ticker: NetworksTicker.mainnet, onTokensStateChange, onNetworkStateChange: sinon.stub(), - onCurrencyRateStateChange: sinon.stub(), }, { interval: 10 }, ); diff --git a/packages/assets-controllers/src/TokenRatesController.ts b/packages/assets-controllers/src/TokenRatesController.ts index 14ca8629fe4..cd57ed846b9 100644 --- a/packages/assets-controllers/src/TokenRatesController.ts +++ b/packages/assets-controllers/src/TokenRatesController.ts @@ -14,7 +14,6 @@ import { import type { NetworkState } from '@metamask/network-controller'; import { fetchExchangeRate as fetchNativeExchangeRate } from './crypto-compare'; import type { TokensState } from './TokensController'; -import type { CurrencyRateState } from './CurrencyRateController'; /** * @type CoinGeckoResponse @@ -170,7 +169,6 @@ export class TokenRatesController extends BaseController< * @param options.chainId - The chain ID of the current network. * @param options.ticker - The ticker for the current network. * @param options.onTokensStateChange - Allows subscribing to token controller state changes. - * @param options.onCurrencyRateStateChange - Allows subscribing to currency rate controller state changes. * @param options.onNetworkStateChange - Allows subscribing to network state changes. * @param config - Initial options used to configure this controller. * @param state - Initial state to set on this controller. @@ -180,7 +178,6 @@ export class TokenRatesController extends BaseController< chainId: initialChainId, ticker: initialTicker, onTokensStateChange, - onCurrencyRateStateChange, onNetworkStateChange, }: { chainId: Hex; @@ -188,9 +185,6 @@ export class TokenRatesController extends BaseController< onTokensStateChange: ( listener: (tokensState: TokensState) => void, ) => void; - onCurrencyRateStateChange: ( - listener: (currencyRateState: CurrencyRateState) => void, - ) => void; onNetworkStateChange: ( listener: (networkState: NetworkState) => void, ) => void; @@ -220,14 +214,10 @@ export class TokenRatesController extends BaseController< this.configure({ tokens: [...tokens, ...detectedTokens] }); }); - onCurrencyRateStateChange((currencyRateState) => { - this.configure({ nativeCurrency: currencyRateState.nativeCurrency }); - }); - onNetworkStateChange(({ providerConfig }) => { - const { chainId } = providerConfig; + const { chainId, ticker } = providerConfig; this.update({ contractExchangeRates: {} }); - this.configure({ chainId }); + this.configure({ chainId, nativeCurrency: ticker }); }); this.poll(); }