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 hiding unverified assets #3708

Merged
merged 5 commits into from
Dec 21, 2023
Merged

Fix hiding unverified assets #3708

merged 5 commits into from
Dec 21, 2023

Conversation

jagodarybacka
Copy link
Contributor

@jagodarybacka jagodarybacka commented Dec 15, 2023

Resolves #3675

What has been done

The problem was that although we were removing custom assets correctly from the redux and marking them as removed in the databese then they were added back with next balances update.

  • added removed filed to metadata type for consistency
  • fixed the issue: filter out balance update against a list of removed custom assets

Testing

Try to reproduce steps from the linked issue. The item that was removed shouldn't go back anymore even after reload.

Latest build: extension-builds-3708 (as of Thu, 21 Dec 2023 09:27:55 GMT).

@Shadowfiend
Copy link
Contributor

Shadowfiend commented Dec 15, 2023

One path we might consider here is not using hideAsset for “Don't show” in the verification modal. Semantically, “removing” is not the same as “not trusting”—feels like we should keep the asset and mark it explicitly unverified (setting verified: false). That would also let us in the future show the user a warning if they tried to manually add it (e.g. “you previously marked this asset as untrusted, are you sure you want to add it?”).

EDIT to add: also, of course, it's unclear why we're showing an asset automatically/through asset discovery that was previously marked as explicitly removed...

@jagodarybacka
Copy link
Contributor Author

Ok so the problem seems to be caused by removing assets from redux:

  • user hides asset via "Don't show" button
  • asset is marked removed in the indexing/customAssets database
  • later there can be an update on balances that adds that hidden asset to the redux once again without checking in the database what was the status of a given asset

We need to either make sure we are not adding removed assets back to the redux or we should just include the information about the asset removed status in the redux state and filter it out in the selector. Will see what will be easier.

I see the problem with naming here - untrusted, hidden and removed are mixed up but let's fix it later

To avoid hidden custom assets reappearing after balances update
we need to filter them out while balances are being deserialized.
@jagodarybacka jagodarybacka self-assigned this Dec 18, 2023
@jagodarybacka jagodarybacka marked this pull request as ready for review December 18, 2023 14:00
@michalinacienciala
Copy link
Contributor

Unfortunately the problem still occurs:

Screen.Recording.2023-12-18.at.15.27.50.mov

Tested on extension-builds-3708 (as of Fri, 15 Dec 2023 13:31:07 GMT).

@michalinacienciala
Copy link
Contributor

michalinacienciala commented Dec 18, 2023

Unfortunately the problem still occurs:

Screen.Recording.2023-12-18.at.15.27.50.mov
Tested on extension-builds-3708 (as of Fri, 15 Dec 2023 13:31:07 GMT).

Hmm, I just noticed that the build I was testing on was built 3 days ago... Looks the artifact did not update after the recent commits... Will investigate why...

EDIT: Ok, the new artifact was produced, but the bot and Jagoda were editing the PR description at a similar time and Jagoda has overwritten the bot's change by mistake:
image

I will restore the bot's change and will test the PR on the latest build.

@michalinacienciala
Copy link
Contributor

What is the expected behavior for a case where an asset is removed via the Don't show option and then user tries to add it back manually?

@jagodarybacka
Copy link
Contributor Author

@michalinacienciala If I remember correctly such asset should reappear as verified

@michalinacienciala
Copy link
Contributor

michalinacienciala commented Dec 18, 2023

@michalinacienciala If I remember correctly such asset should reappear as verified

Yeah, that what I thought as well. But I hit a problem with adding a previously removed asset in this scenario:

  1. Remove hidden asset (in my case it was LOOKSDROP on bnbspider.crypto). Asset removed (OK)
  2. Add the removed asset. Asset added (OK)
  3. Remove the asset again. Asset removed (at least that's how it looks) (OK)
  4. Try adding the asset again - Taho shows that asset already exists and does not allow for adding it again. Looking on the assets list, the asset is not visible. (NOOK)
Screen.Recording.2023-12-18.at.16.55.58.mov

Btw, I also noticed that the balance of the LOOKSDROP asset is wrong (we show 80 on the asset list, but when trying to import the asset, balance is shown as 800 (which is in line with Etherscan). I will investigate that independently of this PR.

@jagodarybacka
Copy link
Contributor Author

Interesting, seems like another bug but will see tomorrow if I can quickly solve it in one PR.

@jagodarybacka
Copy link
Contributor Author

The problem mentioned above is different than the one we solved here (and not trivial to solve) so let's fix it in a separate PR.

@xpaczka xpaczka merged commit 8bc0d63 into main Dec 21, 2023
6 checks passed
@xpaczka xpaczka deleted the cant-remove-assets branch December 21, 2023 09:19
@xpaczka xpaczka mentioned this pull request Dec 27, 2023
xpaczka added a commit that referenced this pull request Dec 29, 2023
## What's Changed
* Enable swaps on Sepolia testnet by @michalinacienciala in
#3710
* Prevent read-only address submission when validating input by @xpaczka
in #3705
* Support video avatars by @xpaczka in
#3700
* Replace `customStyle` property with `style` by @xpaczka in
#3715
* Show tooltips on hovering over icons with no labels by @xpaczka in
#3711
* Minor UI issues by @xpaczka in
#3716
* Fix hiding unverified assets by @jagodarybacka in
#3708
* v0.54.0 by @xpaczka in
#3706
* chore(ui): typo fix by @IssouChancla in
#3717
* Switch e2e tests run on Goerli to Sepolia by @michalinacienciala in
#3719

## New Contributors
* @xpaczka made their first contribution in
#3705
* @IssouChancla made their first contribution in
#3717

**Full Changelog**:
v0.54.0...v0.55.0

Latest build:
[extension-builds-3720](https://github.com/tahowallet/extension/suites/19343332470/artifacts/1136165985)
(as of Wed, 27 Dec 2023 09:28:52 GMT).
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.

Removed assets reappear on the token list
5 participants