-
Notifications
You must be signed in to change notification settings - Fork 394
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
Untrusted assets should not block the addition of custom tokens #3491
Conversation
- Simplification of the update logic when an asset already exists. - Create a function to check whether the asset is verified. - Addition of comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions on simplifying boolean logic here—notably, there's a few other places in these code paths but outside of this PR where we should also ruthlessly simplify the boolean logic, e.g.:
if (!isRemoved || (isRemoved && isVerified)) { |
background/main.ts
Outdated
@@ -1894,14 +1895,17 @@ export default class Main extends BaseService<never> { | |||
const mainCurrencyAmount = convertedAssetAmount | |||
? assetAmountToDesiredDecimals(convertedAssetAmount, 2) | |||
: undefined | |||
// Existing assets are those that are verified by default or by the user. | |||
// This check allows the user to add an asset that is on the unverified list. | |||
const exists = cachedAsset ? isVerifiedAsset(cachedAsset) : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const exists = cachedAsset ? isVerifiedAsset(cachedAsset) : false | |
const shouldDisplay = cachedAsset && isVerifiedAsset(cachedAsset) |
The logic and naming seems a little clearer like this, I think, but I'm not 100% sure it's all accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -29,7 +29,7 @@ export const selectSwapBuyAssets = createSelector( | |||
): asset is SwappableAsset & { | |||
recentPrices: SingleAssetState["recentPrices"] | |||
} => { | |||
if (!canBeUsedForTransaction(asset)) { | |||
if (!isVerifiedAsset(asset)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're stacking enough conditions here that most of this function is noise. I wonder if we wouldn't be better served rewriting it as:
return isVerifiedAsset(asset) && (
// Only list assets for the current network.
isBuiltInNetworkBaseAsset(asset, currentNetwork) ||
(isSmartContractFungibleAsset(asset) && sameNetwork(asset.homeNetwork, currentNetwork))
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!isEnabled(FeatureFlags.SUPPORT_UNVERIFIED_ASSET)) { | ||
return true | ||
} | ||
return isUntrustedAsset(asset) ? !isUnverifiedAssetByUser(asset) : true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't isUnverifiedAssetByUser
already take isUntrustedAsset
into account?
extension/background/redux-slices/utils/asset-utils.ts
Lines 371 to 374 in 546f8c5
const baseAsset = isNetworkBaseAsset(asset) | |
const isUntrusted = isUntrustedAsset(asset) | |
return !baseAsset && isUntrusted |
I think if we dig through and do a bunch of really ugly K Maps of this1, we can simplify this assertion to:
return isNetworkBaseAsset(asset) || !isUntrustedAsset(asset)
I'm having a lot of difficulty reasoning my way through these though and I think it's because we're using a lot of negatives, and we've also muddled three concepts I want to circle back to: baseline trusted assets, trusted assets, and verified assets. I think we can define a very clear, stackable set of conditions that are easier to reason about with these:
- Baseline trusted means “the wallet ~trusts this”—i.e., the asset is in a token list OR the asset is a network base asset. Currently
!isUnverifiedAssetByUser
sort of tries to do this. - Verified means the user has explicitly verified the asset.
- Trusted means the asset is baseline trusted OR verified. Currently that is what
isVerifiedAsset
reports.
In this scenario the negatives also have very clear meanings as the opposites of the above (!...
):
- Baseline untrusted means the asset is
!baseline trusted
—i.e. “the wallet does not trust this without an explicit user signal”. Currently this is roughly equivalent toisUntrustedAsset
. It could still be verified. - Unverified means the asset is
!verified
—i.e., the user has not explicitly verified the asset. It can still be baseline trusted. - Untrusted means the asset is
!trusted
—i.e. the asset is neither baseline trusted NOR verified. Currently this is roughly equivalent toisUnverifiedAssetByUser
.
I strongly believe these three concepts are enough to handle all of what we're trying to address, and can all be stated in terms of positives (verified
, trusted
, baselineTrusted
) most of the time and only as negatives untrusted
, untrustedByUser
, etc) when we need to check the negative. They also feel straightforward to unit test, explain, and document.
Do you agree? Have I missed an edge case? I found about 4 while I was writing and rewriting this comment, so I wouldn't be surprised if I missed another 😅
Footnotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things here:
- Wanted to clarify the broader rework (the new terms) probably doesn't need to block this PR.
- I did miss an edge case: if
verified
is set tofalse
(rather than unset), an asset should not be trusted*, regardless of whether it's baseline trusted, because that means the user explicitly marked it not verified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been brilliantly explained. Let's simplify this logic to make it clear and unambiguous. PR with a new concept #3528
When a user adds an unverified asset manually it should not bring up a dust warning. This token should be displayed in the list even is dust.
- Minor changes to the comments - Removal of the `discoveryTxHash` refresh logic. It is no longer needed. The change was to update the `discoveryTxHash` after the data migration, where `discoveryTxHash` was removed for all custom assets. This also resulted in the wrong adding of a `discoveryTxHash` for the asset in the token list. The change solves this issue.
After manually adding a token from the list of unverified assets, it will be displayed normally, even though it is dust. Therefore, the dust warning should be displayed when the asset does not yet exist in the wallet.
Improvements have been done to parts of the logic. Terminology changes and simplifications will be included in the #3528. Simplification of the asset removal logic has reported in #3533, and will be resolved in the near future. The PR is ready to be reviewed and tested again. One note we currently don't display prices for untrusted tokens. There is a discussion on this topic on discord. EDIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of notes, can be followed up in #3528. Let's ship it 🚢
// Only show prices for trusted assets | ||
isUntrusted || | ||
// Only show prices for trusted or verified assets | ||
!isVerifiedOrTrustedAsset(assetAmount.asset) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this and #3528 land in the same release so we don't have to worry about differences, as that PR also handles this.
@@ -399,7 +392,7 @@ export default function SettingsAddCustomAsset(): ReactElement { | |||
{t("submit")} | |||
</SharedButton> | |||
</div> | |||
{shouldDisplayAsset && ( | |||
{shouldDisplayAsset ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… I feel like shouldDisplayAsset
isn't telling me what this boolean actually means 🙈
But let's rework that in #3528 since we have to touch the isVerifiedOrTrusted
stuff anyway.
## What Currently, some of the terms and conditions for untrusted assets have become unclear. This should be simplified and described. There are three concepts baseline trusted assets, trusted assets, and verified assets. - Baseline trusted means “the wallet ~trusts this”—i.e., the asset is in a token list OR the asset is a network base asset. - Verified means the user has explicitly verified the asset. - Trusted means the asset is baseline trusted OR verified. The negatives: - Baseline untrusted means the asset is !baseline trusted—i.e. “the wallet does not trust this without an explicit user signal”. - Unverified means the asset is !verified—i.e., the user has not explicitly verified the asset. It can still be baseline trusted. - Untrusted means the asset is !trusted—i.e. the asset is neither baseline trusted NOR verified. More information can be found [here](#3491 (comment)). ## Testing The changes should not affect the behavior of the app, it should be the same as before the changes. Wallet page - [x] Only trusted assets should be on the list on the wallet page. - [x] Enable the display of unverified assets in the settings. Only unverified assets have verification options on the wallet page. - [x] Untrusted assets should not be on the list of send assets. - [x] Untrusted assets should not be on the list of assets to be swapped. - [x] Verify the untrusted asset. The asset should appear on the list of assets to be swapped and send. - [x] Warning slide-up menu for unverified/verified is showing the correct buttons. Single asset page - [x] The basic network asset - visible send and swap buttons - [x] Token list asset - visible send and swap buttons - [x] Untrusted asset - visible verify status label and button - [x] Verified asset - visible send and swap buttons and verify status label Feature flag disabled - `SUPPORT_UNVERIFIED_ASSET=false` - [ ] Untrusted assets should be on the list of send assets. - [ ] Untrusted assets should be on the list of assets to be swapped. - [ ] Enable the display of unverified assets in the settings. Only unverified assets have a verification warning on the wallet page. - [ ] All assets should have visible send and swap buttons on the single asset page Latest build: [extension-builds-3528](https://github.com/tahowallet/extension/suites/14211360622/artifacts/797370444) (as of Tue, 11 Jul 2023 12:05:49 GMT).
## What's Changed * Add private key onboarding flow by @jagodarybacka in #3119 * Private key JSON import by @jagodarybacka in #3177 * Allow export of private keys and mnemonics by @jagodarybacka in #3248 * Export private key form by @jagodarybacka in #3255 * Unlock screen for the account backup by @kkosiorowska in #3257 * Show mnemonic menu by @jagodarybacka in #3259 * Fix background blur issue by @jagodarybacka in #3265 * Account backup UI fixes by @jagodarybacka in #3270 * Fix unhiding removed accounts by @jagodarybacka in #3282 * New error for incorrectly decrypted JSON file by @jagodarybacka in #3293 * Export private keys from HD wallet addresses by @jagodarybacka in #3253 * Refactor keyring redux slice to remove `importing` field by @jagodarybacka in #3309 * 📚 Accounts backup by @kkosiorowska in #3252 * Catch Enter keypress on Unlock screen by @jagodarybacka in #3355 * Rename `keyring` to `internal signer` and other improvements by @jagodarybacka in #3331 * 🗝 QA - Accounts backup and private key import by @jagodarybacka in #3266 * Remove private key signers if they are replaced by accounts from HD wallet by @jagodarybacka in #3377 * RFB 4: One-Off Keyring Design by @Shadowfiend in #3372 * Copy to clipboard warning by @kkosiorowska in #3488 * Allow setting custom auto-lock timer by @hyphenized in #3477 * Use Argon2 for encrypted vaults by @jagodarybacka in #3502 * 👑 Private keys import and accounts backup by @jagodarybacka in #3089 * Untrusted assets should not block the addition of custom tokens by @kkosiorowska in #3491 * Flip updated dApp connections flag by @Shadowfiend in #3492 * v0.41.0 by @Shadowfiend in #3531 * Switch to a given network if adding a network that is already added. by @0xDaedalus in #3154 * Remove waiting for Loading Doggo component in E2E tests by @jagodarybacka in #3541 * Squeeze content to better fit on Swaps page by @jagodarybacka in #3542 * Refactor of terms for verified/unverified assets by @kkosiorowska in #3528 * Fix ChainList styling by @fulldecent in #3547 * Update release checklist by @jagodarybacka in #3548 * Fix custom asset price fetching by @hyphenized in #3508 * Sticky Defaults: Make Taho-as-default replace MetaMask in almost all cases by @Shadowfiend in #3546 ## New Contributors * @fulldecent made their first contribution in #3547 **Full Changelog**: v0.41.0...v0.42.0 Latest build: [extension-builds-3549](https://github.com/tahowallet/extension/suites/14268975651/artifacts/801826435) (as of Thu, 13 Jul 2023 09:51:56 GMT).
Closes #3467
What
Currently, the user isn't able to add an asset that is in the untrusted list. We display a message about the existence of the token and block the button. We should allow users to add an asset that is in the untrusted list.
Changes
UI
Screen.Recording.2023-06-21.at.13.52.33.mov
Testing
testertesting.eth
0x841fad6eae12c286d1fd18d1d525dffa75c7effe
Latest build: extension-builds-3491 (as of Sat, 08 Jul 2023 04:51:18 GMT).