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

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 26281
Build 52132: Build Diff	cashtab-tests
Build 52131: arc lint + arc unit

Event Timeline

emack created this revision.Jan 4 2024, 13:10

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 4 2024, 13:10

emack requested review of this revision.Jan 4 2024, 13:10

Harbormaster completed remote builds in B26139: Diff 43871.Jan 4 2024, 13:19

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

emack mentioned this in D15084: [Cashtab] Additional mitigation for legacy wallet addresses.Jan 5 2024, 06:38

emack mentioned this in D15104: [Cashtab] Remove faulty unit tests.Jan 6 2024, 00:47

Rebase to master and correcting expect() statement

Pending D15084

Harbormaster completed remote builds in B26199: Diff 43963.Jan 6 2024, 00:53

emack mentioned this in D15122: [Cashtab] Better component organization and routing for etokens.Jan 10 2024, 00:44

emack mentioned this in rABCf57103ac9621: [Cashtab] Additional mitigation for legacy wallet addresses.Jan 11 2024, 23:45

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

Harbormaster completed remote builds in B26281: Diff 44082.Jan 12 2024, 00:11

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

Harbormaster completed remote builds in B26282: Diff 44083.Jan 12 2024, 00:41

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 ↗	(On Diff #44083)	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)