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

Make custom tokens visible in the asset list even if they have a balance of zero #3313

Closed
wants to merge 6 commits into from

Conversation

kkosiorowska
Copy link
Contributor

@kkosiorowska kkosiorowska commented Apr 26, 2023

Closes #3273

What

Currently, when a user adds a token manually, it will not show up in the list of assets when the balance is zero. We should display an asset on the list even when its balance is empty.

UI

Screenshot 2023-04-26 at 14 20 43

Screenshot 2023-04-26 at 14 20 59

To Test

  • add a custom token whose balance is zero (0x841fad6eae12c286d1fd18d1d525dffa75c7effe for Fandom network)
  • the asset should appear in the list of assets
  • hide dust in the settings, the token should be displayed on the list of hidden assets
  • make sure that we do not show other assets that have a balance of zero

Latest build: extension-builds-3313 (as of Wed, 14 Jun 2023 15:42:00 GMT).

Karolina Kosiorowska added 2 commits April 26, 2023 14:15
* hide assets with balances under $2
* display custom assets when balance is 0
@kkosiorowska kkosiorowska self-assigned this Apr 26, 2023
@kkosiorowska kkosiorowska marked this pull request as ready for review April 26, 2023 12:33
Copy link
Contributor

@jagodarybacka jagodarybacka left a comment

Choose a reason for hiding this comment

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

Some improvements ⬇️

Also - on other networks trusted and untrusted assets switched sections:

Screen.Recording.2023-05-03.at.11.52.19.mov

background/redux-slices/utils/asset-utils.ts Outdated Show resolved Hide resolved
background/redux-slices/selectors/accountsSelectors.ts Outdated Show resolved Hide resolved
background/redux-slices/accounts.ts Outdated Show resolved Hide resolved
Comment on lines 312 to 317
if (
updatedAccountBalance.assetAmount.amount === 0n &&
existingAccountData.balances[updatedAssetSymbol] === undefined &&
!isBuiltInNetworkBaseAsset(asset, network) // add base asset even if balance is 0
existingAccountData.balances[updatedAssetSymbol] === undefined
) {
return
// add base asset even if balance is 0 or is a custom asset
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we combine these two ifs?

@kkosiorowska
Copy link
Contributor Author

Let's wait with this PR until we merge this one.

@hyphenized
Copy link
Contributor

Let's wait with this PR until we merge this one.

Good catch! The conditionals for token visibility have already been refactored there but we can also cherry pick some of that work here if we need to push this out faster

@0xDaedalus
Copy link
Contributor

Lets add a couple of unit tests here as well - we should just need two: One for computeCombinedAssetAmountsData and one for updateAccountBalance

@jagodarybacka
Copy link
Contributor

@kkosiorowska Is this PR still valid and we should merge it at some point or not?

@kkosiorowska
Copy link
Contributor Author

kkosiorowska commented Jun 6, 2023

@kkosiorowska Is this PR still valid and we should merge it at some point or not?

@jagodarybacka PR was blocked by verified/unverified tokens. It needs to be updated and tested again. I should be looking into it this week.

@VladUXUI
Copy link
Contributor

I don't think this is a good idea fwiw.
If i sell my asset, how do i hide the dust from it?

@kkosiorowska
Copy link
Contributor Author

kkosiorowska commented Jun 14, 2023

I don't think this is a good idea fwiw. If i sell my asset, how do i hide the dust from it?

We are able to force a newly added asset to be displayed (when the balance is 0). In the case where we sell an asset and its balance is close to zero we are able to hide it.

@Shadowfiend
Copy link
Contributor

Shadowfiend commented Jun 15, 2023

I don't think this is a good idea fwiw. If i sell my asset, how do i hide the dust from it?

We are able to force a newly added asset to be displayed (when the balance is 0). In the case where we sell an asset and its balance is close to zero we are able to hide it.

I think if we've implemented, in the custom token add page, the message that the asset will be added but hidden due to having 0 balance + hide dust being enabled, then we should avoid getting to this level of contortion in the code, because it's going to have the potential to get real nasty. e.g. when do we consider an asset to be ”new“ and subject to this? Do we have to wait for balance to go over 0 and then drop back under it? Are we instead saying the asset is visible at exactly 0 but not at fractionally over 0? Etc.

EDIT to say: if it's not clear, I'm basically for killing this PR completely, BUT I think it's ultimately a call for Vlad/Haley to make 😁

!isBuiltInNetworkBaseAsset(asset, network) // add base asset even if balance is 0
// add base asset even if balance is 0 or is a custom asset
!(
isUntrustedAsset(asset) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

If the asset already exists it will likely be part of a token list assets and not a custom asset, in that case this won't trigger right?

@kkosiorowska
Copy link
Contributor Author

EDIT to say: if it's not clear, I'm basically for killing this PR completely, BUT I think it's ultimately a call for Vlad/Haley to make 😁

It seems to me that Vlad has noticed an important issue. I don't think this PR adds much value for the user. I'm okay with closing it.

@VladUXUI Please let us know what you think.

@VladUXUI
Copy link
Contributor

Yeah, let's close. I think this will add more confusion to user and not enough value. Seeing 0 token assets isn't useful for many people. Also as a user i will rarely add a 0 balance asset just for the fun of it

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.

Tokens that are added manually should be visible in the asset list even if they have a balance of zero.
6 participants