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.

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

[7] fix(tokens): fix tokens list loading state #3201

Merged

Conversation

shoom3301
Copy link
Collaborator

@shoom3301 shoom3301 commented Oct 10, 2023

Summary

This is a final PR where I add all fixes after code-review / QA.
I also added test plan for the whole refactoring below:

To Test

  1. Token list
  • add/import (by URL, by ensName)
  • remove
  • toggle
  • view details
  • update (TokensListsUpdater updates it every 6 hours)
  • search a list is not presented already
  • search a list is presented already
  1. User added tokens
  • add/import
  • remove
  • clear all
  1. Favourite tokens
  • toggle
  • reset to defaults
  1. Unsupported tokens
  • AST token on Gnosis chain should be marked with "unsupported" tag in the list
  • TODO: since an unsupported token may become supported, we should periodically reset the state
  1. Select a token
  • all active tokens should be displayed (including user-added and favourite)
  • tokens should be sorted by balance
  • search a token by address
  • search a token by symbol
  • import token
  • fast select by pressing Enter
  1. Auto import token
  1. Tokens
  • tokens with decimals=0
  • permittable tokens should have a tag in tokens list
  • native token should be displayed first in the list
  • add to Metamask
  • token logo displaying in the orders table
  • token logo displaying in the activities list
  1. Other
  • google analytics events: libs/analytics/src/events/listEvents.ts

TODO

  1. Add a migration to clean up old localStorage state
  2. Add a migration for user-added tokens from old to the new state

@shoom3301 shoom3301 self-assigned this Oct 10, 2023
@vercel
Copy link

vercel bot commented Oct 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
swap-dev ✅ Ready (Inspect) Visit Preview Oct 24, 2023 2:25pm

🌃 Cosmos ↗︎

@shoom3301 shoom3301 force-pushed the refactor/tokens-post-review-fixes branch from 418081f to 840af3f Compare October 10, 2023 10:06
@anxolin
Copy link
Contributor

anxolin commented Oct 18, 2023

Thanks for the diagrams in the library!

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Nice to finally test everything!
Some comments on the code.

As for testing:

There's no indication of list loading, when searching for a new one:

Screen.Recording.2023-10-19.at.11.25.07.mov

When removing a token that's currently selected, I immediately get a prompt to add the token back

Screen.Recording.2023-10-19.at.11.27.56.mov

GNO has a square logo? I think we used to round them in the UI.

Screenshot 2023-10-19 at 11 28 52

ETH disappears from the list sometimes, apparently while it's loading.

Screen.Recording.2023-10-19.at.11.30.58.mov

There's a test item to reset favourite to defaults. How do I do that?

import { useCallback } from 'react'
import { useIsUnsupportedToken } from './useIsUnsupportedToken'

type NullishAddress = string | null | undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume you don't want to use Nullish because that's in the main app and it would create a circular dependency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You know my mind :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would like to move common types to some a new lib in the future

@@ -1,20 +1,16 @@
import { useUnsupportedTokens } from './useUnsupportedTokens'
import { useCallback } from 'react'

export function useIsUnsupportedToken() {
type NullishAddress = string | null | undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Repeated type. What about move it to the token app types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't you mind if I do it in the next iteration?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, no problem

libs/tokens/src/utils/getTokenLogoUrls.ts Outdated Show resolved Hide resolved
*/
export function tokenMapToListWithLogo(tokenMap: TokensMap): TokenWithLogo[] {
return Object.values(tokenMap)
.sort((a, b) => (a.symbol > b.symbol ? 1 : -1))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it mater it they are lower cased for the comparison?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see any difference, maybe because the majority of tokens have uppercased symbols

@shoom3301
Copy link
Collaborator Author

There's no indication of list loading, when searching for a new one:

@alfetopito added the loader: 6a7a227

image

@shoom3301
Copy link
Collaborator Author

When removing a token that's currently selected, I immediately get a prompt to add the token back

This is not a new issue (exiting on prod), I would like to fix it in the next iteration if you don't mind

@shoom3301
Copy link
Collaborator Author

GNO has a square logo? I think we used to round them in the UI.

@alfetopito thanks! Fixed

@shoom3301
Copy link
Collaborator Author

There's a test item to reset favourite to defaults. How do I do that?

@alfetopito sorry, I didn't add the scenario:

  1. Add some tokens to favourites

  2. Open tokens page and select "Favourite tokens"

image
  1. Click "Reset favourites"
image
  1. The list of the favourite tokens should be reset to defaults

@shoom3301
Copy link
Collaborator Author

ETH disappears from the list sometimes, apparently while it's loading.

@alfetopito hmmm, I can't reproduce it now. Could you give steps on how to get it?

@alfetopito
Copy link
Collaborator

ETH disappears from the list sometimes, apparently while it's loading.

@alfetopito hmmm, I can't reproduce it now. Could you give steps on how to get it?

I don't know exactly what are the steps.
I am connected with MM on mainnet and I can see it very easily.
I recorded it now with the console logs open:

Screen.Recording.2023-10-23.at.11.40.31.mov

Maybe try to slow down your connection a bit?

Watching the network calls, the ETH price seems to be displayed only when the blocknative call completes:
image

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Tested again, only the ETH disappearing from the list issue remains

@shoom3301
Copy link
Collaborator Author

@alfetopito thank you! Fixed

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

It's working \o/

Comment on lines -12 to +16
if (getIsNativeToken(a) || getIsNativeToken(b)) return 1
if (aIsNative || bIsNative) {
return aIsNative ? -1 : 1
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ha, I knew it!

@shoom3301 shoom3301 merged commit 9c4bfdf into refactor/tokens-remove-legacy Oct 26, 2023
6 of 7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 26, 2023
@alfetopito alfetopito deleted the refactor/tokens-post-review-fixes branch October 26, 2023 17:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants