Skip to content

Commit

Permalink
Prevent bug where incorrect symbol used for token rates (#1496)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Gudahtt authored and MajorLift committed Oct 11, 2023
1 parent 2018899 commit a714ffb
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 32 deletions.
27 changes: 7 additions & 20 deletions packages/assets-controllers/src/TokenRatesController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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({
Expand All @@ -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(
Expand All @@ -170,7 +167,6 @@ describe('TokenRatesController', () => {
chainId: toHex(1),
ticker: NetworksTicker.mainnet,
onTokensStateChange: jest.fn(),
onCurrencyRateStateChange: jest.fn(),
onNetworkStateChange: jest.fn(),
},
{
Expand All @@ -194,7 +190,6 @@ describe('TokenRatesController', () => {
chainId: toHex(1),
ticker: NetworksTicker.mainnet,
onTokensStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange: sinon.stub(),
},
{
Expand All @@ -214,7 +209,6 @@ describe('TokenRatesController', () => {
chainId: toHex(1),
ticker: NetworksTicker.mainnet,
onTokensStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange: sinon.stub(),
},
{ interval: 1337 },
Expand Down Expand Up @@ -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),
},
Expand Down Expand Up @@ -278,7 +271,6 @@ describe('TokenRatesController', () => {
chainId: toHex(1),
ticker: NetworksTicker.mainnet,
onTokensStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange: sinon.stub(),
},
{ interval: 10 },
Expand All @@ -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 },
Expand All @@ -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 },
Expand All @@ -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);
});
Expand Down Expand Up @@ -387,7 +377,6 @@ describe('TokenRatesController', () => {
chainId: toHex(137),
ticker: 'MATIC',
onTokensStateChange,
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange,
},
{ interval: 10 },
Expand Down Expand Up @@ -451,7 +440,6 @@ describe('TokenRatesController', () => {
ticker: NetworksTicker.mainnet,
onTokensStateChange,
onNetworkStateChange,
onCurrencyRateStateChange: sinon.stub(),
},
{ interval: 10 },
);
Expand Down Expand Up @@ -525,7 +513,6 @@ describe('TokenRatesController', () => {
ticker: NetworksTicker.mainnet,
onTokensStateChange,
onNetworkStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
},
{ interval: 10 },
);
Expand Down
14 changes: 2 additions & 12 deletions packages/assets-controllers/src/TokenRatesController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -180,17 +178,13 @@ export class TokenRatesController extends BaseController<
chainId: initialChainId,
ticker: initialTicker,
onTokensStateChange,
onCurrencyRateStateChange,
onNetworkStateChange,
}: {
chainId: Hex;
ticker: string;
onTokensStateChange: (
listener: (tokensState: TokensState) => void,
) => void;
onCurrencyRateStateChange: (
listener: (currencyRateState: CurrencyRateState) => void,
) => void;
onNetworkStateChange: (
listener: (networkState: NetworkState) => void,
) => void;
Expand Down Expand Up @@ -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();
}
Expand Down

0 comments on commit a714ffb

Please sign in to comment.