extension
extension copied to clipboard
tahowallet
:eye: Trust but verify
Add a warning in the wallet asset list when interacting with assets whose metadata has been inferred from transaction history.
To test
- [ ] Load a read-only account with tokens that aren't on our lists
- [ ] As historical activities are loaded, additional token balances should be discovered
- [ ] Tokens that aren't on our lists should show a warning
Refs #1021, #1893
Latest build: extension-builds-1840 (as of Thu, 12 Jan 2023 13:33:46 GMT).
@Shadowfiend would love your thoughts on how to make this consistent with your plans in https://github.com/tallycash/extension/pull/1788
I'd like to add an "internal" token list for each user in a future PR, and use that to track custom assets going forward. But in the meantime, this PR should be enough to show discovered assets and warn people of scams... except they won't currently show up at all with after this change
https://github.com/tallycash/extension/pull/1788/files#diff-b0e4a7036b74ef74bb2061264a7470aa9dc941d5bb0b326556464bb28480fc80L328
We already have the internal list--that is what the custom asset stuff is I believe. This todo explains a reasonable path to adding the relevant metadata there:
https://github.com/tallycash/extension/blob/6a69598bdab150ee1c0cebc3b81e05d6369a0fa6/background/services/indexing/index.ts#L294-L299
~~I think we have started using custom assets for assets with 0 balance at some point.~~
Don't comment based on vague memories and from mobile :) we use custom assets during the discovery in indexing service #453 as it should be used.
Taking a different tack: this PR shows a warning for any asset in the asset list that isn't on a token list.
It appears that these assets are no longer auto-discovered from the asset transfer call as of #1788... given that we have a warning now, should I revert part of that PR here?
Yes, I think this is the way.
Nit: I wouldn't revert that change but reintroduce a modified version according to this comment
Nit2: The definition of baselineTrustedAsset will also need to be updated.
I think two paths we can take here and it's ultimately a product decision. We can nuke the baseline trust concept, always display the warning for non-token list assets without a way to mark them trusted, then separately add a warning dismissal path that adds the token to the custom token list. Or we can land this, invisible because those assets are never added, and then fill out the trust concept as per the TODO and attach the warnings to that.
Either way works for me.
I think the former is the shorter path, but will confirm.
Getting to more token balances in the wallet sooner is going to be a big win for users.
@mr-michael calling your attention here: merging this PR means we'll pick up all / nearly all token balances at the risk of including scam tokens, though they'll have clear and obvious warnings.
My thought is we merge this, then do the next bit (allowing tokens to be flagged as trusted or ignored) in a future PR / release based on other priorities. I think that's the right call because it immediately improves the UX for new wallets. It can degrade UX a bit for older wallets like mine with a bunch of spam tokens, but most users seem to be onboarding with new accounts.
@Shadowfiend @greg-nagy I opened https://github.com/tallycash/extension/issues/1893 to continue the asset trust discussion and unify the two approaches.
Found the issue (took forever!), fixed the issue, added regression tests. Should be ready for another look @jagodarybacka ✅
Should be good for another look - note that AVAX on polygon is displayed as untrusted because it is indeed not on the official polygon tokenlist that we use. Otherwise, network base assets erroneously showing as untrusted should be fixed.
For reference - reproduction steps:
https://user-images.githubusercontent.com/20949277/212067677-8278e920-e1bf-4464-94e8-1ca59df03d10.mov
After account is fully loaded it seems to be fixed:
Problem here is because MATIC token doesn't contain coinType property and because of it it is not considered base asset.