Skip to content
This repository has been archived by the owner on Jan 15, 2021. It is now read-only.

Deterministic sorting #1513

Merged
merged 13 commits into from
Oct 9, 2020
1 change: 1 addition & 0 deletions src/const.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ export const INPUT_PRECISION_SIZE = 6
export const WETH_ADDRESS_MAINNET = '0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2'
export const WETH_ADDRESS_RINKEBY = '0xc778417E063141139Fce010982780140Aa0cD5Ab'
export const WXDAI_ADDRESS_XDAI = '0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d'
export const WETH_ADDRESS_XDAI = '0x6A023CCd1ff6F2045C3309768eAd9E68F978f6e1'

export const ORDER_BOOK_HOPS_DEFAULT = 2
export const ORDER_BOOK_HOPS_MAX = 2
Expand Down
62 changes: 57 additions & 5 deletions src/services/factories/tokenList.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Copy link
Contributor

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.

? 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
It won't be a race condition in our case because this microtask is scheduled before await Promise.all, but still a bad approach.


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
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, no 😢
This is like 3 loops more than necessary


function _sortTokens(networkId: number, tokens: TokenDetails[]): TokenDetails[] {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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,
Expand All @@ -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
}
}

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 tokenList out of that. One might be inclined to treat tokenDetails as unchanged after this line, though we don't do it.

Just writing

_sortTokens(networkId, tokenDetails)

explicitly shows that tokenDetails was mutated


// 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 String.localeCompare (or Intl.collator.compare if you fancy that)

}

async function updateTokens(networkId: number): Promise<void> {
Expand Down
1 change: 1 addition & 0 deletions src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export enum Network {
}

export interface TokenDetails extends TokenDex {
label: string
disabled?: boolean
override?: TokenOverride
}
Expand Down