Deterministic sorting #1513
Deterministic sorting #1513
Conversation
Co-authored-by: Velenir <Velenir@users.noreply.github.com>
Co-authored-by: Velenir <Velenir@users.noreply.github.com>
| @@ -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) | |||
There was a problem hiding this comment.
nit: can we in the future call unresolved vars promised<varName>? I find it easier to read.
| token.then((token) => { | ||
| if (token) { | ||
| token.label = safeTokenName(token) | ||
| } | ||
| return token | ||
| }) |
There was a problem hiding this comment.
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.
| if (a.label < b.label) { | ||
| return -1 | ||
| } else if (a.label > b.label) { | ||
| return 1 | ||
| } | ||
| return 0 |
There was a problem hiding this comment.
considering what a lable may be (unicode string)and for simpicity, I recommend String.localeCompare (or Intl.collator.compare if you fancy that)
| 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 | ||
| } |
There was a problem hiding this comment.
Please, no 😢
This is like 3 loops more than necessary
| // Sort tokens | ||
| const tokenList = _sortTokens(networkId, tokenDetails) |
There was a problem hiding this comment.
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
| tokensSorted = _moveTokenToHeadOfArray(WETH_ADDRESS_XDAI, tokensSorted) | ||
| return _moveTokenToHeadOfArray(WXDAI_ADDRESS_XDAI, tokensSorted) |
| return tokenList | ||
| } | ||
|
|
||
| function _sortTokens(networkId: number, tokens: TokenDetails[]): TokenDetails[] { |
There was a problem hiding this comment.
Can do the same in one pass.
Solves the issue that the sorting is not deterministic.
Changing the implementation on how we get the tokens exposed that our order is not deterministic.
When we changed how we iterate and compose the final list of tokens, it moved our token orders.
safeTokenNameNOTE:
labelwas added, not only for sorting. I think that it's just nice to have already a not nullable displayable name for debugging and for the UI. We use this function over and over in many parts of the app. This PR doesn't do the refactor to take advantage of this new property. There will be a separate issue for that