Page MenuHomePhabricator

[Cashtab] Ignore function for tokens - pt 1 of 2
Changes PlannedPublic

Authored by emack on Jan 4 2024, 13:10.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Summary

At the moment there is no mitigation against an address being innundated with spam/scam eTokens (only a matter of time once market picks up). There is the option for the user to burn those tokens but that action incurs a transaction fee and the user shouldn't have to pay for this privilege. The existing token blacklist is also not appropriate for this purpose as it needs to be customized for each user and the potentially multiple wallets in each user's Cashtab instance.

This diff adds an option for the user to flag specific tokens in their wallet to ignore, which are stored in localForage and filtered at rendering time in TokenList.js.

There'll be a part 2 to this diff which will expand on a UI that allows the user to browse through the list of ignored tokens and choose to un-ignore specific ones.

Test Plan
  • npm test
  • npm start
  • Open Cashtab on a browser that has not interacted with this diff (i.e. localStorage does not contain the 'allTokensToIgnore' object
  • Navigate to the eTokens page, click on an existing eToken, scroll to the bottom and click "Hide [token ticker]". Ensure a "[token ticker] added to ignore list" notification is displayed
  • Examine localForage (Ctrl+shift+I -> Application-> Storage -> IndexedDb -> localForage -> keyvaluepairs) and ensure it now contains a new 'allTokensToIgnore' object containing this address and a 'tokenIdsToIgnore' array within this object containing this token id.
  • Navigate to the eTokens page again and ensure the recently hidden token is now hidden from the list.
  • Hide another token, ensure the same notification is displayed and this 2nd token being hidden has been added to the 'tokenIdsToIgnore' array for this address in localForage.
  • Hide all tokens in wallet and ensure no tokens are displayed on the eTokens page with no errors.
  • Manually delete the 'allTokensToIgnore' object from localForage and ensure all tokens are now displayed again on the eTokens page.
  • Open cashtab in a fresh new browser with no existing cache whatsoever. Create new wallet. Fund it. Create new token. Hide it and observe the same outcome as above.
  • npm run extension and observe the same outcome in the extension.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
hideToken
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26282
Build 52134: Build Diffcashtab-tests
Build 52133: arc lint + arc unit

Event Timeline

emack requested review of this revision.Jan 4 2024, 13:10
bytesofman requested changes to this revision.Jan 4 2024, 17:19
bytesofman added inline comments.
cashtab/src/components/Etokens/Etokens.js
12 ↗(On Diff #43871)

I have seen it a couple times during testing now where, for an old wallet, the address is stored with bitcoincash: prefix. It doesn't really matter for (most) cases since the legacy address is used.

In this case though it does matter.

This error should not be propagated -- instead we should have a diff that migrates all these legacy storage wallets to ecash format.

This revision now requires changes to proceed.Jan 4 2024, 17:19

Rebase to master and correcting expect() statement

Rebased to legacy address mitigation in D15084, token component rejig in D15122 and myriad of other diffs landed this week

Now that D15084 is landed, removing the now-redundant prefix conversions

There'll be a part 2 to this diff which will expand on a UI that allows the user to browse through the list of ignored tokens and choose to un-ignore specific ones.

This is a useful feature but also quite complicated. It is nice to (finally) get a localforage mock in the codebase, but the unit tests need to make better use of it.

Now that tokens are organized alphabetically, is there really high demand for this diff? Users can just burn tokens they don't want to see. Probably only have a handful of users who are bothered to surplus tokens.

Why go through the trouble of hiding them? If you don't want them, just burn them.

I'm concerned this is more technical debt than its worth. Niche feature (with alternatives) and high complexity.

More valuable -- deprecate localforage and just use the indexedDb APIs. This would simplify unit testing and get rid of a large dependency. Probably difficult to do given it would require some sort of migration. But it would make Cashtab more robust, testable, and lean. It would also make me less wary about updating our local storage model for low-impact changes.

In general we should be focused on "what is the highest impact feature I can build?" -- I really don't think this is it. This is quite time intensive, maybe somebody asked for it and it would be high impact to them ... but they won't be maintaining it through all other Cashtab improvements and they might not even use it.

Off the top of my head, stuff that would be higher impact

  • Extension / website integration improvements
  • New webapps that use the extension
  • etoken trading using swap
  • memo protocol on ecash
  • removing tech debt in Cashtab (refactor usewallet, deprecate localforage, deprecate slp-mdm for our own lib)
  • push notifications in cashtab
  • etoken to reward cashtab users for using cashtab (similar to coingecko candies)

imo there is so much out there. we are under-resourced and need to be careful about these "it needs to be done" diffs that, perhaps, should not be done before higher impact opportunities.

cashtab/src/hooks/__tests__/useWallet.test.js
468

this is the only useful test here.

Testing the getTokensToIgnoreFromLocalStorage is just testing the mock

In general -- testing useWallet functions is an integration test. We have to confirm that an expected state change happened.

So, we don't want unit test type tests. We want more a user story. "User is able to ignore a token. (test that local storage was updated and that the get method returns the expected updated value).", "User is able to unignore a token (test that local storage was updated, get method returns new expected value)

This revision now requires changes to proceed.Jan 12 2024, 21:33

probably much easier to implement and with similar impact: just hide tokens that don't have an icon

https://reviews.bitcoinabc.org/T2237

emack planned changes to this revision.Mar 2 2024, 13:33