Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add chainId support to TokenRatesController #476

Merged
merged 13 commits into from
Jul 5, 2021
Merged

Add chainId support to TokenRatesController #476

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
84ac2fd
Initial proposal for chainId support of TokenRatesController
wachunei May 24, 2021
b720274
Default to ethereum mainnet chain
wachunei May 24, 2021
efddcdf
Get chainSlugIdentifier on update
wachunei May 25, 2021
8ad4be4
Merge branch 'main' into feature/token-rate-chainid
wachunei Jun 2, 2021
f3524f6
Add supportedChains and chainCache
wachunei Jun 5, 2021
4830c33
Cache response instead of individual slugs
wachunei Jun 11, 2021
1774012
Merge branch 'main' into feature/token-rate-chainid
wachunei Jun 17, 2021
0db47bd
Update interval test
wachunei Jun 23, 2021
204ff43
Merge branch 'main' into feature/token-rate-chainid
wachunei Jun 23, 2021
bca002a
Use 6 hours threshold
wachunei Jun 24, 2021
e4bc0d1
Merge branch 'main' into feature/token-rate-chainid
wachunei Jul 1, 2021
1aa1bfb
Rename COINGECKO_API to CoinGeckoApi
wachunei Jul 2, 2021
120f654
Merge branch 'main' into feature/token-rate-chainid
wachunei Jul 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 68 additions & 10 deletions src/assets/TokenRatesController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,53 @@ import { AssetsController } from './AssetsController';
import { AssetsContractController } from './AssetsContractController';

const COINGECKO_HOST = 'https://api.coingecko.com';
const COINGECKO_PATH = '/api/v3/simple/token_price/ethereum';
const COINGECKO_ETH_PATH = '/api/v3/simple/token_price/ethereum';
const COINGECKO_BSC_PATH = '/api/v3/simple/token_price/binance-smart-chain';
const COINGECKO_ASSETS_PATH = '/api/v3/asset_platforms';

describe('TokenRatesController', () => {
beforeEach(() => {
nock(COINGECKO_HOST)
.get(COINGECKO_ASSETS_PATH)
.reply(200, [
{
id: 'binance-smart-chain',
chain_identifier: 56,
name: 'Binance Smart Chain',
shortname: 'BSC',
},
{
id: 'ethereum',
chain_identifier: 1,
name: 'Ethereum',
shortname: '',
},
])
.get(
`${COINGECKO_PATH}?contract_addresses=0x89d24A6b4CcB1B6fAA2625fE562bDD9a23260359,0xfoO&vs_currencies=eth`,
`${COINGECKO_ETH_PATH}?contract_addresses=0x89d24A6b4CcB1B6fAA2625fE562bDD9a23260359,0xfoO&vs_currencies=eth`,
)
.reply(200, {
'0x89d24a6b4ccb1b6faa2625fe562bdd9a23260359': { eth: 0.00561045 },
})
.get(`${COINGECKO_PATH}?contract_addresses=0xfoO&vs_currencies=eth`)
.get(`${COINGECKO_ETH_PATH}?contract_addresses=0xfoO&vs_currencies=eth`)
.reply(200, {})
.get(`${COINGECKO_PATH}?contract_addresses=bar&vs_currencies=eth`)
.get(`${COINGECKO_ETH_PATH}?contract_addresses=bar&vs_currencies=eth`)
.reply(200, {})
.get(`${COINGECKO_PATH}?contract_addresses=0xfoO&vs_currencies=gno`)
.get(`${COINGECKO_ETH_PATH}?contract_addresses=0xfoO&vs_currencies=gno`)
.reply(200, {})
.get(
`${COINGECKO_BSC_PATH}?contract_addresses=0x89d24A6b4CcB1B6fAA2625fE562bDD9a23260359,0xfoO&vs_currencies=eth`,
)
.reply(200, {
'0x89d24a6b4ccb1b6faa2625fe562bdd9a23260359': { eth: 0.00561045 },
})
.get(`${COINGECKO_BSC_PATH}?contract_addresses=0xfoO&vs_currencies=eth`)
.reply(200, {})
.get(`${COINGECKO_BSC_PATH}?contract_addresses=bar&vs_currencies=eth`)
.reply(200, {})
.get(`${COINGECKO_BSC_PATH}?contract_addresses=0xfoO&vs_currencies=gno`)
.reply(200, {})

.persist();

nock('https://min-api.cryptocompare.com')
Expand All @@ -40,19 +70,25 @@ describe('TokenRatesController', () => {
const controller = new TokenRatesController({
onAssetsStateChange: stub(),
onCurrencyRateStateChange: stub(),
onNetworkStateChange: stub(),
});
expect(controller.state).toStrictEqual({
contractExchangeRates: {},
chainSlugIdentifier: 'ethereum',
});
expect(controller.state).toStrictEqual({ contractExchangeRates: {} });
});

it('should initialize with the default config', () => {
const controller = new TokenRatesController({
onAssetsStateChange: stub(),
onCurrencyRateStateChange: stub(),
onNetworkStateChange: stub(),
});
expect(controller.config).toStrictEqual({
disabled: false,
interval: 180000,
nativeCurrency: 'eth',
chainId: '1',
tokens: [],
});
});
Expand All @@ -61,6 +97,7 @@ describe('TokenRatesController', () => {
const controller = new TokenRatesController({
onAssetsStateChange: stub(),
onCurrencyRateStateChange: stub(),
onNetworkStateChange: stub(),
});
expect(() => console.log(controller.tokens)).toThrow(
'Property only used for setting',
Expand All @@ -71,7 +108,11 @@ describe('TokenRatesController', () => {
await new Promise<void>((resolve) => {
const mock = stub(TokenRatesController.prototype, 'fetchExchangeRate');
new TokenRatesController(
{ onAssetsStateChange: stub(), onCurrencyRateStateChange: stub() },
{
onAssetsStateChange: stub(),
onCurrencyRateStateChange: stub(),
onNetworkStateChange: stub(),
},
{
interval: 10,
tokens: [{ address: 'bar', decimals: 0, symbol: '' }],
Expand All @@ -89,7 +130,11 @@ describe('TokenRatesController', () => {

it('should not update rates if disabled', async () => {
const controller = new TokenRatesController(
{ onAssetsStateChange: stub(), onCurrencyRateStateChange: stub() },
{
onAssetsStateChange: stub(),
onCurrencyRateStateChange: stub(),
onNetworkStateChange: stub(),
},
{
interval: 10,
},
Expand All @@ -103,7 +148,11 @@ describe('TokenRatesController', () => {
it('should clear previous interval', async () => {
const mock = stub(global, 'clearTimeout');
const controller = new TokenRatesController(
{ onAssetsStateChange: stub(), onCurrencyRateStateChange: stub() },
{
onAssetsStateChange: stub(),
onCurrencyRateStateChange: stub(),
onNetworkStateChange: stub(),
},
{ interval: 1337 },
);
await new Promise<void>((resolve) => {
Expand Down Expand Up @@ -133,6 +182,7 @@ describe('TokenRatesController', () => {
{
onAssetsStateChange: (listener) => assets.subscribe(listener),
onCurrencyRateStateChange: stub(),
onNetworkStateChange: (listener) => network.subscribe(listener),
},
{ interval: 10 },
);
Expand All @@ -156,7 +206,11 @@ describe('TokenRatesController', () => {

it('should handle balance not found in API', async () => {
const controller = new TokenRatesController(
{ onAssetsStateChange: stub(), onCurrencyRateStateChange: stub() },
{
onAssetsStateChange: stub(),
onCurrencyRateStateChange: stub(),
onNetworkStateChange: stub(),
},
{ interval: 10 },
);
stub(controller, 'fetchExchangeRate').throws({
Expand All @@ -176,10 +230,12 @@ describe('TokenRatesController', () => {
assetStateChangeListener = listener;
});
const onCurrencyRateStateChange = stub();
const onNetworkStateChange = stub();
const controller = new TokenRatesController(
{
onAssetsStateChange,
onCurrencyRateStateChange,
onNetworkStateChange,
},
{ interval: 10 },
);
Expand All @@ -196,10 +252,12 @@ describe('TokenRatesController', () => {
const onCurrencyRateStateChange = stub().callsFake((listener) => {
currencyRateStateChangeListener = listener;
});
const onNetworkStateChange = stub();
const controller = new TokenRatesController(
{
onAssetsStateChange,
onCurrencyRateStateChange,
onNetworkStateChange,
},
{ interval: 10 },
);
Expand Down
104 changes: 85 additions & 19 deletions src/assets/TokenRatesController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { toChecksumAddress } from 'ethereumjs-util';
import BaseController, { BaseConfig, BaseState } from '../BaseController';
import { safelyExecute, handleFetch } from '../util';

import type { NetworkState } from '../network/NetworkController';
import type { AssetsState } from './AssetsController';
import type { CurrencyRateState } from './CurrencyRateController';

Expand Down Expand Up @@ -46,9 +47,14 @@ export interface Token {
export interface TokenRatesConfig extends BaseConfig {
interval: number;
nativeCurrency: string;
chainId: string;
tokens: Token[];
}

interface ContractExchangeRates {
[address: string]: number | undefined;
}

/**
* @type TokenRatesState
*
Expand All @@ -57,7 +63,8 @@ export interface TokenRatesConfig extends BaseConfig {
* @property contractExchangeRates - Hash of token contract addresses to exchange rates
*/
export interface TokenRatesState extends BaseState {
contractExchangeRates: { [address: string]: number };
contractExchangeRates: ContractExchangeRates;
chainSlugIdentifier: string | null | undefined;
}

/**
Expand All @@ -72,8 +79,21 @@ export class TokenRatesController extends BaseController<

private tokenList: Token[] = [];

private getPricingURL(query: string) {
return `https://api.coingecko.com/api/v3/simple/token_price/ethereum?${query}`;
private getPricingURL(chainSlugIdentifier: string, query: string) {
return `https://api.coingecko.com/api/v3/simple/token_price/${chainSlugIdentifier}?${query}`;
}

private async getChainSlugIdentifier(
chainId: string,
): Promise<string | undefined> {
const platforms: [
{ id: string; chain_identifier: number | null },
] = await handleFetch('https://api.coingecko.com/api/v3/asset_platforms');
const chain = platforms.find(
({ chain_identifier }) =>
chain_identifier !== null && String(chain_identifier) === chainId,
);
return chain?.id;
}

/**
Expand All @@ -94,13 +114,17 @@ export class TokenRatesController extends BaseController<
{
onAssetsStateChange,
onCurrencyRateStateChange,
onNetworkStateChange,
}: {
onAssetsStateChange: (
listener: (assetState: AssetsState) => void,
) => void;
onCurrencyRateStateChange: (
listener: (currencyRateState: CurrencyRateState) => void,
) => void;
onNetworkStateChange: (
listener: (networkState: NetworkState) => void,
) => void;
},
config?: Partial<TokenRatesConfig>,
state?: Partial<TokenRatesState>,
Expand All @@ -110,9 +134,13 @@ export class TokenRatesController extends BaseController<
disabled: true,
interval: 180000,
nativeCurrency: 'eth',
chainId: '1',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not have a hard-coded default network - we should find out for sure what the initial network is. This could be disastrous, if this controller thinks the user is on a different network until he first switch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda just as bad to do this with the native currency as well to be honest, though that's a pre-existing issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done in order to provide backwards compatibility, since the controller was implicitly always hard coded to chainId: 1 (because ethereum was the hardcoded endpoint).

Will look into how we should handle this properly then.

Copy link
Member

@Gudahtt Gudahtt Jun 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've handled this elsewhere by passing in the current network state as a constructor parameter, or passing in a "getNetworkState" function which can be used to check the initial state.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, will look it up!

tokens: [],
};
this.defaultState = { contractExchangeRates: {} };
this.defaultState = {
contractExchangeRates: {},
chainSlugIdentifier: undefined,
};
this.initialize();
this.configure({ disabled: false }, false, false);
onAssetsStateChange((assetsState) => {
Expand All @@ -121,6 +149,10 @@ export class TokenRatesController extends BaseController<
onCurrencyRateStateChange((currencyRateState) => {
this.configure({ nativeCurrency: currencyRateState.nativeCurrency });
});
onNetworkStateChange(({ provider }) => {
const { chainId } = provider;
this.configure({ chainId });
});
this.poll();
}

Expand All @@ -138,6 +170,14 @@ export class TokenRatesController extends BaseController<
}, this.config.interval);
}

set chainId(_chainId: string) {
!this.disabled && safelyExecute(() => this.updateExchangeRates());
}

get chainId() {
throw new Error('Property only used for setting');
}

Comment on lines +220 to +227
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When chainId changes, run update if not disabled.

/**
* Sets a new token list to track prices
*
Expand All @@ -157,11 +197,15 @@ export class TokenRatesController extends BaseController<
/**
* Fetches a pairs of token address and native currency
*
* @param chainSlugIdentifier - Chain string identifier
* @param query - Query according to tokens in tokenList and native currency
* @returns - Promise resolving to exchange rates for given pairs
*/
async fetchExchangeRate(query: string): Promise<CoinGeckoResponse> {
return handleFetch(this.getPricingURL(query));
async fetchExchangeRate(
chainSlugIdentifier: string,
query: string,
): Promise<CoinGeckoResponse> {
return handleFetch(this.getPricingURL(chainSlugIdentifier, query));
}

/**
Expand All @@ -170,21 +214,43 @@ export class TokenRatesController extends BaseController<
* @returns Promise resolving when this operation completes
*/
async updateExchangeRates() {
if (this.tokenList.length === 0) {
if (this.tokenList.length === 0 || this.disabled) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was called directly, nothing was stopping it from being run. What is the intended behavior originally: should disable just disable tokenList/chainId changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the disabled property was used in this controller 🤔

Like many aspects of the BaseController, some controllers are written to use it and some ignore it, or break when you try to use it. The config is similar - it's built to be changeable at runtime, but many controllers are written assuming that never happens, and they break if you try it.

It doesn't hurt to add a check for disabled here but I'm not sure it helps either, unless I'm missing something. This awkwardness with the API is one of the things BaseControllerV2 is meant to address.

return;
}
const newContractExchangeRates: { [address: string]: number } = {};
const { nativeCurrency } = this.config;
const pairs = this.tokenList.map((token) => token.address).join(',');
const query = `contract_addresses=${pairs}&vs_currencies=${nativeCurrency.toLowerCase()}`;
const prices = await this.fetchExchangeRate(query);
this.tokenList.forEach((token) => {
const address = toChecksumAddress(token.address);
const price = prices[token.address.toLowerCase()];
newContractExchangeRates[address] = price
? price[nativeCurrency.toLowerCase()]
: 0;
});
const { nativeCurrency, chainId } = this.config;
const { chainSlugIdentifier } = this.state;

let chainSlug;

if (!chainSlugIdentifier) {
try {
chainSlug = await this.getChainSlugIdentifier(chainId);
if (!chainSlug) {
this.update({ chainSlugIdentifier: null });
}
} catch {
this.update({ chainSlugIdentifier: undefined });
}
}
Copy link
Member Author

@wachunei wachunei May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the main question.

Since now there is an extra chainId parameter, we need to hit two endpoints

  1. Get the chain slug given the chain id
  2. Use the chain slug to recover the prices, like it was done before

Should we "remember" the slug? There are three cases:

  1. The request fails
  2. The request is successful but the requested chain id is not listed on the api
  3. The request is successful and we have a slug we can use

If we don't have the slug stored, this will always make two request when fetching prices.
If we store the slug, we must be able to define when to invalidate it in case the API changes, for example: it supports a new chain which we stored as unsupported

Copy link
Contributor

@adonesky1 adonesky1 May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we store the slug when we are able to get a valid response and then wrap the fetchExchangeRate in a try/catch in case the slug becomes invalid and attempt to refetch it if that's the case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we manage the case the slug does not exist and later it does? Example: We would store it as null, so we don't even attempt to fetch the rates, but if the slug is supported in the future, how would we invalidate that null?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm not understanding but if the chainSlugIdentifier is null we try will try to fetch it again as the code stands? Its not ideal because we are not able to cut out that extra network request by storing it as unsupported but I can't think of a way around that right now - unless they only update the api when they change the endpoint (v3 -> v4)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we are doing on swaps-controller to hold values for a certain amount of time is to store pairs (value, timestamp) and compare timestamp, which represents the last time value was fetched, to a time threshold.

This way if value is null but Date.now() - timestamp is less than a threshold, we consider the network is not supported, otherwise we attempt to fetch the slug and update the pair (value, timestamp = Date.now()).

The same works when value is a-network-slug that might become unsupported, but we check only after we are over the time threshold.

Does it make sense?

Copy link
Contributor

@adonesky1 adonesky1 May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that makes sense. So we're weighing the trade-offs of optimizing for fewer network requests and, if we choose to go that route, possibly not fetching exchange rates when they are available until the unsupported flag expires according to whatever time threshold we set. I guess I don't have a strong intuition of which way to go with this. What do you think @Gudahtt?


const newContractExchangeRates: ContractExchangeRates = {};
if (!chainSlug) {
this.tokenList.forEach((token) => {
const address = toChecksumAddress(token.address);
newContractExchangeRates[address] = undefined;
});
} else {
const pairs = this.tokenList.map((token) => token.address).join(',');
Copy link
Contributor

@adonesky1 adonesky1 Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] @wachunei I know you didn't name this const but it seems slightly misnamed to me? It's a list of tokens that are each one side of a pair with the native currency right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is just a concatenation of token addresses joined by a comma, from something like [{ address: '0x1...', ...restOfTokenObject1 }, { address: '0x2...', ...restOfTokenObject2 }] into "0x1...,0x2...", which is what CoinGecko takes in as query param for the API call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree is not a great name! But you know, naming stuff is hard hahahaha

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pairs should be addresses, sorry for necroposting

const query = `contract_addresses=${pairs}&vs_currencies=${nativeCurrency.toLowerCase()}`;
const prices = await this.fetchExchangeRate(chainSlug, query);
this.tokenList.forEach((token) => {
const address = toChecksumAddress(token.address);
const price = prices[token.address.toLowerCase()];
newContractExchangeRates[address] = price
? price[nativeCurrency.toLowerCase()]
: 0;
});
}
this.update({ contractExchangeRates: newContractExchangeRates });
}
}
Expand Down