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 2 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
83 changes: 66 additions & 17 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;
}

/**
Expand All @@ -72,8 +79,24 @@ 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 updateChainSlugIdentifier(chainId: string) {
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,
);
if (chain?.id) {
this.update({ chainSlugIdentifier: chain.id });
} else {
this.update({ chainSlugIdentifier: '' });
}
wachunei marked this conversation as resolved.
Show resolved Hide resolved
!this.disabled && safelyExecute(() => this.updateExchangeRates());
}

/**
Expand All @@ -94,13 +117,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 +137,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: 'ethereum',
};
this.initialize();
this.configure({ disabled: false }, false, false);
onAssetsStateChange((assetsState) => {
Expand All @@ -121,6 +152,11 @@ export class TokenRatesController extends BaseController<
onCurrencyRateStateChange((currencyRateState) => {
this.configure({ nativeCurrency: currencyRateState.nativeCurrency });
});
onNetworkStateChange(({ provider }) => {
const { chainId } = provider;
this.configure({ chainId });
this.updateChainSlugIdentifier(chainId);
});
this.poll();
}

Expand Down Expand Up @@ -157,11 +193,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 @@ -173,18 +213,27 @@ export class TokenRatesController extends BaseController<
if (this.tokenList.length === 0) {
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 { chainSlugIdentifier } = this.state;

const newContractExchangeRates: ContractExchangeRates = {};
if (!chainSlugIdentifier) {
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(chainSlugIdentifier, 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