Skip to content

fix: token sorting #262

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: token sorting #262

wants to merge 1 commit into from

Conversation

meeh0w
Copy link
Member

@meeh0w meeh0w commented May 13, 2025

Changes

  • Fixes token sorting

Testing

Verify tokens are sorted as expected: native token first, then the highest fiat value, then highest balance (if fiat value is unknown), then alphabetically.

Screenshots:

image

Checklist for the author

  • I've covered new/modified business logic with Jest test cases.
  • I've tested the changes myself before sending it to code review and QA.

@meeh0w meeh0w added the DO NOT MERGE This PR is not meant to be merged in its current state label May 13, 2025
@meeh0w meeh0w changed the title Fix/token sorting fix: token sorting May 13, 2025
@meeh0w meeh0w force-pushed the fix/token-sorting branch from 9e7178a to 9b0602c Compare May 16, 2025 17:50
@meeh0w meeh0w changed the base branch from chore/new-app to main May 16, 2025 17:50
@meeh0w meeh0w removed the DO NOT MERGE This PR is not meant to be merged in its current state label May 16, 2025
Comment on lines +29 to +61
const [nativeTokens, tokensWithCurrency, tokensWithoutCurrency] =
tokens.reduce<
[
DisplayToken<TokenWithBalance & { type: TokenType.NATIVE }>[],
DisplayToken<TokenWithBalance & { balanceInCurrency: number }>[],
DisplayToken<TokenWithBalance>[],
]
>(
([natives, withCurrencyValue, withoutCurrencyValue], displayToken) => {
if (isNativeToken(displayToken)) {
return [
[...natives, displayToken],
withCurrencyValue,
withoutCurrencyValue,
];
}

if (hasCurrencyValue(displayToken)) {
return [
natives,
[...withCurrencyValue, displayToken],
withoutCurrencyValue,
];
}

return [
natives,
withCurrencyValue,
[...withoutCurrencyValue, displayToken],
];
},
[[], [], []],
);

Choose a reason for hiding this comment

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

Please consider to use lodash groupby method instead
https://lodash.com/docs#groupBy

Choose a reason for hiding this comment

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

Or, have you tried to use Lodash's orderBy function?
It should be much cleaner

Comment on lines +39 to +43
return [
[...natives, displayToken],
withCurrencyValue,
withoutCurrencyValue,
];

Choose a reason for hiding this comment

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

You shouldn't return a new accumulator value, because it litters the memory like crazy.
Original accumulator should be mutated instead

Choose a reason for hiding this comment

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

tokens.reduce(acc, token => {
  const [natives, withCurrentValue, withoutCurrentValue] = acc;
  if (isNativeToken(token)) {
     natives.push(token)
  }

  ....

  return acc
})
    

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants