-
Notifications
You must be signed in to change notification settings - Fork 41
Deterministic sorting #1513
Deterministic sorting #1513
Changes from all commits
346b1d0
5b8e19c
9da34bb
421ddf2
c8c2941
11a7eeb
760dedb
b8ce9b5
e6782f2
f349ded
e046689
dcfcd3c
8ff5c71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,12 @@ import { TokenList } from 'api/tokenList/TokenListApi' | |
import { SubscriptionCallback } from 'api/tokenList/Subscriptions' | ||
import { ExchangeApi } from 'api/exchange/ExchangeApi' | ||
import { TcrApi } from 'api/tcr/TcrApi' | ||
import { TokenDetails, Command } from 'types' | ||
import { TokenDetails, Command, Network } from 'types' | ||
import { logDebug, notEmpty, retry } from 'utils' | ||
|
||
import { TokenFromErc20Params } from './' | ||
import { TokenErc20 } from '@gnosis.pm/dex-js' | ||
import { safeTokenName, TokenErc20 } from '@gnosis.pm/dex-js' | ||
import { WETH_ADDRESS_MAINNET, WETH_ADDRESS_RINKEBY, WETH_ADDRESS_XDAI, WXDAI_ADDRESS_XDAI } from 'const' | ||
|
||
export function getTokensFactory(factoryParams: { | ||
tokenListApi: TokenList | ||
|
@@ -175,18 +176,56 @@ export function getTokensFactory(factoryParams: { | |
const tokenDetailsPromises: (Promise<TokenDetails | undefined> | TokenDetails)[] = [] | ||
addressToIdMap.forEach((id, tokenAddress) => { | ||
// Resolve the details using the config, otherwise fetch the token | ||
const token: TokenDetails | undefined | Promise<TokenDetails | undefined> = tokensConfigMap.has(tokenAddress) | ||
? tokensConfigMap.get(tokenAddress) | ||
const token: undefined | Promise<TokenDetails | undefined> = tokensConfigMap.has(tokenAddress) | ||
? Promise.resolve(tokensConfigMap.get(tokenAddress)) | ||
: _fetchToken(networkId, id, tokenAddress) | ||
|
||
if (token) { | ||
// Add a label for convenience | ||
token.then((token) => { | ||
if (token) { | ||
token.label = safeTokenName(token) | ||
} | ||
return token | ||
}) | ||
Comment on lines
+185
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we don't await this specific promise, it could be a race condition |
||
|
||
tokenDetailsPromises.push(token) | ||
} | ||
}) | ||
|
||
return (await Promise.all(tokenDetailsPromises)).filter(notEmpty) | ||
} | ||
|
||
function _moveTokenToHeadOfArray(tokenAddress: string, tokenList: TokenDetails[]): TokenDetails[] { | ||
const tokenIndex = tokenList.findIndex((t) => t.address === tokenAddress) | ||
if (tokenIndex !== -1) { | ||
const token = tokenList[tokenIndex] | ||
tokenList.splice(tokenIndex, 1) | ||
|
||
return [token, ...tokenList] | ||
} | ||
|
||
return tokenList | ||
} | ||
Comment on lines
+199
to
+209
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, no 😢 |
||
|
||
function _sortTokens(networkId: number, tokens: TokenDetails[]): TokenDetails[] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can do the same in one pass. |
||
// Sort tokens | ||
let tokensSorted = tokens.sort(_tokenComparer) | ||
|
||
// Make sure wxDAI and WETH are the first tokens | ||
switch (networkId) { | ||
case Network.Mainnet: | ||
return _moveTokenToHeadOfArray(WETH_ADDRESS_MAINNET, tokensSorted) | ||
case Network.Rinkeby: | ||
return _moveTokenToHeadOfArray(WETH_ADDRESS_RINKEBY, tokensSorted) | ||
case Network.xDAI: | ||
tokensSorted = _moveTokenToHeadOfArray(WETH_ADDRESS_XDAI, tokensSorted) | ||
return _moveTokenToHeadOfArray(WXDAI_ADDRESS_XDAI, tokensSorted) | ||
Comment on lines
+222
to
+223
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, no no 😭 |
||
default: | ||
return tokensSorted | ||
} | ||
} | ||
|
||
async function _fetchToken( | ||
networkId: number, | ||
tokenId: number, | ||
|
@@ -206,6 +245,7 @@ export function getTokensFactory(factoryParams: { | |
return { | ||
...partialToken, | ||
id: tokenId, | ||
label: '', // Label is not nullable for convenience, but it's added later. This adds a default for making TS happy | ||
} | ||
} | ||
|
||
|
@@ -218,8 +258,20 @@ export function getTokensFactory(factoryParams: { | |
// Get token details for each filtered token | ||
const tokenDetails = await _fetchTokenDetails(networkId, filteredAddressesAndIds, tokensConfig) | ||
|
||
// Sort tokens | ||
const tokenList = _sortTokens(networkId, tokenDetails) | ||
Comment on lines
+261
to
+262
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is misleading, Sort is mutative, so we don't get a new Just writing _sortTokens(networkId, tokenDetails) explicitly shows that |
||
|
||
// Persist it | ||
tokenListApi.persistTokens({ networkId, tokenList: tokenDetails }) | ||
tokenListApi.persistTokens({ networkId, tokenList }) | ||
} | ||
|
||
function _tokenComparer(a: TokenDetails, b: TokenDetails): number { | ||
if (a.label < b.label) { | ||
return -1 | ||
} else if (a.label > b.label) { | ||
return 1 | ||
} | ||
return 0 | ||
Comment on lines
+269
to
+274
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. considering what a lable may be (unicode string)and for simpicity, I recommend |
||
} | ||
|
||
async function updateTokens(networkId: number): Promise<void> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we in the future call unresolved vars
promised<varName>
? I find it easier to read.