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

fix: filter out 0 liquidity assets and order by market cap #8175

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

NeOMakinG
Copy link
Collaborator

@NeOMakinG NeOMakinG commented Nov 22, 2024

Description

I noticed the scammy assets are coming from portal, they are added by 2 ways:

  • Asset generation (removed them by filtering 0 liquidity assets)
  • Upsert at runtime when building the portfolio (removed 0 liquidity assets too)
  • Sorting by market cap in the search too, so bigger market cap assets are upper in the three)
  • Avoid upserting asset we didn't manage to get market data for

Issue (if applicable)

closes #8139

Risk

Low

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

  • Try with demo mode, search for assets in the trade assets modal (USDC, USDT...)
    -- You shouldn't see any scammy tokens
  • Try with any other big wallets, likes shapeshiftdao.eth, or your own
    -- You shouldn't see any scammy tokens

Engineering

n/a

Operations

n/a

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

Before

image

After

image

@NeOMakinG NeOMakinG requested a review from a team as a code owner November 22, 2024 15:05
@NeOMakinG NeOMakinG marked this pull request as draft November 22, 2024 15:33
@NeOMakinG NeOMakinG marked this pull request as ready for review November 22, 2024 16:57
selectMarketDataUsd,
(assets, marketDataUsd) => {
// This looks weird but isn't - looks like we could use the sorted selectAssetsByMarketCap instead of selectAssets
// but we actually can't - this would rug the triple-sorting
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// but we actually can't - this would rug the triple-sorting
// but we actually can't - this would rug the double-sorting

Copy link
Member

@woodenfurniture woodenfurniture left a comment

Choose a reason for hiding this comment

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

@NeOMakinG a couple questions on this one:

  1. When the market data for the asset list is loading, what is the expected behavior? Should we hide the assets for safety, or what is that intended behavior?
  2. Is there a follow-up planned for asset sorting offline to avoid runtime cost of sorting by market cap?

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.

Bad UX with asset search in swapper.
2 participants