Token Discovery - Add untrusted assets visibility toggler to settings #3250

hyphenized · 2023-04-04T16:50:05Z

Adds a toggler to show/hide untrusted assets to the settings page

To Test

Testing Env

SUPPORT_ASSET_TRUST=true

Latest build: extension-builds-3250 (as of Fri, 12 May 2023 12:17:34 GMT).

background/redux-slices/ui.ts

0xDaedalus · 2023-04-11T15:18:41Z

This looks good to me just need to add the migration

Shadowfiend · 2023-04-15T02:24:04Z

Quick Q here: I'm puzzled as to how this interacts with the show/hide hidden assets collapsing in the asset list. Is a hidden asset different from an untrusted asset? How does the user find out the distinction? Or is this work meant to supersede that work?

kkosiorowska

Currently, after separating hidden assets, the toggler is not doing anything.

Screen.Recording.2023-04-18.at.09.36.56.mov

hyphenized · 2023-04-18T16:37:35Z

Quick Q here: I'm puzzled as to how this interacts with the show/hide hidden assets collapsing in the asset list. Is a hidden asset different from an untrusted asset? How does the user find out the distinction? Or is this work meant to supersede that work?

Hidden assets may be asset balances that are dust, assets that have not been marked as trusted or a combination of the former (assets market as trusted but which balances are considered dust). Does that makes sense? cc @VladUXUI

@kkosiorowska I've fixed the toggler, please give it another look when you can.

kkosiorowska

It looks really good ⚡️Let's still wait for Vlad's response.

kkosiorowska · 2023-04-19T10:55:55Z

background/redux-slices/selectors/accountsSelectors.ts

@@ -71,12 +72,54 @@ const shouldForciblyDisplayAsset = (
  return isDoggo || isNetworkBaseAsset(assetAmount.asset)
 }

+// Hides dust, untrusted assets and missing amounts.
+export function isAssetAmountVisible(


Non-blocking comment
I think this could be moved to a separate file with utils for accounts. In that case, the test file can also be split into two. As for nfts-utils.ts.

background/redux-slices/selectors/accountsSelectors.ts

kkosiorowska · 2023-04-19T11:17:02Z

background/redux-slices/selectors/accountsSelectors.ts

+      hideDust,
+      hideUntrustedAssets


Non-blocking comment

I wonder if it's better to make it one object with settings for assets and pass it here. What I mean is that I would like to avoid increasing the number of parameters for the function.

Agree, we don't need the to pass the current network as a parameter anymore either

background/redux-slices/selectors/accountsSelectors.ts

Simplified the logic to handle the extra filters & added some tests

VladUXUI · 2023-04-20T06:54:38Z

Hidden assets may be asset balances that are dust, assets that have not been marked as trusted or a combination of the former (assets market as trusted but which balances are considered dust). Does that makes sense? cc @VladUXUI

This was not my initial thinking. But i like the idea. Initially i thought as dust and untrusted to be totally separate. Since dust has it's own UX of showing up in the list with the toggle.

But i think i like the idea of having them all in the same place.
So let's go with it.

This also means that if the toggle is off, all the dust assets will move to the main list right?

kkosiorowska · 2023-04-20T08:17:07Z

This also means that if the toggle is off, all the dust assets will move to the main list right?

If we are talking about a toggle for dust. As far as I can see, yes.

VladUXUI · 2023-04-20T11:00:06Z

Couple of things here on the UX side (although uncertain what to actually comment on)
1 - If i marked an asset as trusted, it shouldn't have the yellow Attention icon anymore
2 - Trust/untrust is missing from the token details page

- Hide button now renders "close" if the toggle for hiding untrusted assets hasn't been turned on - Hide button now renders "Keep hidden" if asset has already been marked as untrusted

jagodarybacka

once I trust asset I can no longer untrust it, Hide asset is doing nothing
on single asset page I can see Asset not verified for tokens from token lists

0xDaedalus

I'd like to requests 2 e2e tests here, lets test the following

For a token that is initially trusted:

Set token to untrusted
Token displays as untrusted in Wallet
Set it back to trusted
Token displays as trusted in Wallet

For a token that is initially untrusted:

Set token to trusted
Token displays as trusted in Wallet
Set it back to untrusted
Token displays as untrusted in Wallet

hyphenized · 2023-05-08T14:44:15Z

once I trust asset I can no longer untrust it, Hide asset is doing nothing

Thanks for the review, fixed!

on single asset page I can see Asset not verified for tokens from token lists

Not sure if this is the correct behaviour, could you confirm @VladUXUI ?

Shadowfiend · 2023-05-08T14:46:11Z

on single asset page I can see Asset not verified for tokens from token lists
Not sure if this is the correct behaviour

As I absorb this feature into my brain—why would we see Asset not verified for a token list asset? We have baseline trust for token list assets, right? And Asset not verified is a textual reminder that an asset is not trusted, correct?

VladUXUI · 2023-05-09T08:06:34Z

Token list assets should't have any verified or unverified behaviour/status. As these are already verified and trusted by us

jagodarybacka

Hide button was fixed but I wonder - do we want to implement everything in this PR or not? For example I can see in figma that send and swap buttons are changing state between disabled and enabled depending on asset state

Also once we trust given asset - should we display this popup once again if we are switching between hidden/shown asset?
"Trust asset" button is not triggering that popup but "warning" icon on assets list is triggering that popup every time.

Screen.Recording.2023-05-09.at.13.41.10.mov

jagodarybacka

Some ideas and improvements

jagodarybacka · 2023-05-10T10:34:57Z

background/redux-slices/selectors/accountsSelectors.ts

+  const isTrusted =
+    !isCustomAsset || assetAmount.asset.metadata?.trusted === true
+
+  const isSmartContractAmount = isSmartContractFungibleAsset(assetAmount.asset)


Suggested change

const isSmartContractAmount = isSmartContractFungibleAsset(assetAmount.asset)

const isSmartContractAsset = isSmartContractFungibleAsset(assetAmount.asset)

jagodarybacka · 2023-05-10T10:40:44Z

background/redux-slices/selectors/accountsSelectors.ts

+  if (isForciblyDisplayed) {
+    return true
+  }
+
+  if (!isPresent) {
+    return false
+  }
+


non blocking - I would rather return early, there is no point defining 4 other variables if we already know the result

jagodarybacka · 2023-05-10T10:41:33Z

background/redux-slices/selectors/accountsSelectors.ts

      ) {
        acc.combinedAssetAmounts.push(assetAmount)
-      } else if (isPresent) {
+      } else {
+        // FIXME: Assets with very low values should still be displayed as dust e.g. "<0,01"


Do we need to create an issue for that?

jagodarybacka · 2023-05-10T10:53:29Z

background/redux-slices/tests/accounts.unit.test.ts

+        expect(
+          isAssetAmountVisible(
+            createCompleteAssetAmount(
+              createSmartContractAsset({ metadata: {} }),


Whats the default if metadata is empty? is it trusted or untrusted?

No metadata means no token lists so this is an unverified, untrusted asset

jagodarybacka · 2023-05-10T10:55:16Z

background/redux-slices/ui.ts

@@ -24,12 +25,14 @@ export type UIState = {
  selectedAccount: AddressOnNetwork
  showingActivityDetailID: string | null
  initializationLoadingTimeExpired: boolean
+  // FIXME: Move these settings to preferences service db


Can we add a plumbing task for it? I think it won't happen if we won't plan it

jagodarybacka · 2023-05-10T11:01:04Z

ui/components/Wallet/AssetWarningSlideUp.tsx

+        {asset.metadata?.discoveryTx && (
+          <li>
+            <div className="left">{t("discoveryTx")}</div>
+            <div className="right">


Can we use SharedAddress component here?

We could but SharedAddress has some logic specific to name resolution.

ui/pages/SingleAsset.tsx

Assets set as hidden should not display the unverified asset warning icon

kkosiorowska · 2023-05-24T08:17:00Z

There has been a change in logic for trusted/untrusted assets. The current status of this feature can be found in #3394. Let's close it.

hyphenized added this to the 🚓 Untrusted Tokens Management milestone Apr 4, 2023

hyphenized requested a review from a team April 4, 2023 16:50

hyphenized self-assigned this Apr 4, 2023

kkosiorowska reviewed Apr 5, 2023

View reviewed changes

background/redux-slices/ui.ts Outdated Show resolved Hide resolved

hyphenized force-pushed the show-me-the-money branch from a180448 to 63b9e6e Compare April 14, 2023 12:39

kkosiorowska reviewed Apr 18, 2023

View reviewed changes

kkosiorowska reviewed Apr 19, 2023

View reviewed changes

hyphenized added 7 commits April 19, 2023 16:44

Update redux slice with trusted assets visibility state

d85b2ad

Add trusted/untrusted assets visibility toggler to settings

feadcb5

Add ui settings state migration

c6e8848

Add complete asset amount factory

e5a0d42

Refactor untrusted asset toggler state

78d282b

Refactor asset amount visibility logic

b83df64

Simplified the logic to handle the extra filters & added some tests

Prevent displaying near zero balances in hidden assets list

e956630

hyphenized force-pushed the show-me-the-money branch from 813690c to e956630 Compare April 19, 2023 15:42

Base automatically changed from trust-issues to main April 19, 2023 21:31

hyphenized added 3 commits April 20, 2023 00:08

Merge branch 'main' into show-me-the-money

651498a

Feature flag untrusted assets visibility toggler

879c7b4

Simplify feature flagged settings list items

55edbf1

hyphenized added 5 commits April 21, 2023 10:46

Hide warning on trusted assets

73c7116

Check for trust status in unverified asset slide up

4c33bf9

- Hide button now renders "close" if the toggle for hiding untrusted assets hasn't been turned on - Hide button now renders "Keep hidden" if asset has already been marked as untrusted

Allow trusting/untrusting assets within the single asset page

3e69f7f

Hide warning for assets that have been marked as trusted/untrusted once

901fe59

Merge branch 'main' into show-me-the-money

96c4f4c

hyphenized added 3 commits April 21, 2023 17:20

Update isUntrustedAsset tests

acedb3a

Update SharedAssetInput tests

6a83666

Fix untrusted asset check

0c422ec

hyphenized force-pushed the show-me-the-money branch from b13d8e2 to 0c422ec Compare April 25, 2023 11:23

jagodarybacka requested changes May 3, 2023

View reviewed changes

0xDaedalus suggested changes May 3, 2023

View reviewed changes

kkosiorowska mentioned this pull request May 8, 2023

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

Closed

4 tasks

hyphenized added 3 commits May 8, 2023 14:48

Fix toggler buttons

7e8f33f

Persist transaction source data for assets found through token discovery

4badbf5

Display origin transaction for discovered assets when available

e9009f2

Do not display unverified asset status for token list assets

105bd79

jagodarybacka reviewed May 9, 2023

View reviewed changes

Disable send/swap actions for untrusted assets

4a151f1

jagodarybacka requested changes May 10, 2023

View reviewed changes

hyphenized and others added 5 commits May 11, 2023 11:53

Fix tokenlist assets visibility

68209e4

Assets set as hidden should not display the unverified asset warning icon

Fix positioning for asset warning slide up actions

f20cd5f

Refactor asset trust toggler

e696a70

Refactor variable name

4a7cc3d

Merge branch 'main' into show-me-the-money

ac31218

kkosiorowska self-assigned this May 16, 2023

kkosiorowska closed this May 24, 2023

kkosiorowska deleted the show-me-the-money branch June 14, 2023 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Token Discovery - Add untrusted assets visibility toggler to settings #3250

Token Discovery - Add untrusted assets visibility toggler to settings #3250

hyphenized commented Apr 4, 2023 •

edited by kkosiorowska

Loading

0xDaedalus commented Apr 11, 2023

Shadowfiend commented Apr 15, 2023

kkosiorowska left a comment

hyphenized commented Apr 18, 2023

kkosiorowska left a comment

kkosiorowska Apr 19, 2023

kkosiorowska Apr 19, 2023

hyphenized Apr 19, 2023

VladUXUI commented Apr 20, 2023

kkosiorowska commented Apr 20, 2023

VladUXUI commented Apr 20, 2023

jagodarybacka left a comment •

edited

Loading

0xDaedalus left a comment

hyphenized commented May 8, 2023

Shadowfiend commented May 8, 2023

VladUXUI commented May 9, 2023

jagodarybacka left a comment

jagodarybacka left a comment

jagodarybacka May 10, 2023

jagodarybacka May 10, 2023

jagodarybacka May 10, 2023

jagodarybacka May 10, 2023

hyphenized May 11, 2023

jagodarybacka May 10, 2023

jagodarybacka May 10, 2023

hyphenized May 11, 2023

kkosiorowska commented May 24, 2023

	const isSmartContractAmount = isSmartContractFungibleAsset(assetAmount.asset)
	const isSmartContractAsset = isSmartContractFungibleAsset(assetAmount.asset)

Token Discovery - Add untrusted assets visibility toggler to settings #3250

Token Discovery - Add untrusted assets visibility toggler to settings #3250

Conversation

hyphenized commented Apr 4, 2023 • edited by kkosiorowska Loading

To Test

Testing Env

0xDaedalus commented Apr 11, 2023

Shadowfiend commented Apr 15, 2023

kkosiorowska left a comment

Choose a reason for hiding this comment

hyphenized commented Apr 18, 2023

kkosiorowska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladUXUI commented Apr 20, 2023

kkosiorowska commented Apr 20, 2023

VladUXUI commented Apr 20, 2023

jagodarybacka left a comment • edited Loading

Choose a reason for hiding this comment

0xDaedalus left a comment

Choose a reason for hiding this comment

hyphenized commented May 8, 2023

Shadowfiend commented May 8, 2023

VladUXUI commented May 9, 2023

jagodarybacka left a comment

Choose a reason for hiding this comment

jagodarybacka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkosiorowska commented May 24, 2023

hyphenized commented Apr 4, 2023 •

edited by kkosiorowska

Loading

jagodarybacka left a comment •

edited

Loading