Token Discovery - Remap redux asset balances #3195
Conversation
Fixes discovered tokens name collision
`assetIDs` are now checked against network base assets instead of built-in network assets alone, combined data now only merges asset amounts for trusted and base network assets
| /** | ||
| * All network base assets have a chainID property | ||
| */ | ||
| export function isNetworkBaseAsset(asset: AnyAsset): asset is NetworkBaseAsset { |
There was a problem hiding this comment.
I'm not sure about this change, as we already have a second function that is named isBuiltInNetworkBaseAsset and is checking much more things I'm afraid someone will make a mistake and will think that isNetworkBaseAsset does the same thing.
I assume that this change is because on custom networks we only have chainID as an indicator that this is a base asset? Can we think about a name that will make it clear which function should be used when? isAnyNetworkBaseAsset, isCustomNetworkBaseAsset? Lmk what you think 🤔
There was a problem hiding this comment.
You're right. Though since I could not find an instance of isBuiltInNetworkBaseAsset were we are still making use of those extra checks, I think we can replace it with this more generic alternative.
There was a problem hiding this comment.
I wonder about this. The names are slightly confusing. But however, functions isBuiltInNetworkBaseAsset and isNetworkBaseAsset take different parameters. This reduces the occurrence of mistakes. Therefore, it seems to me that it is not a big threat.
There was a problem hiding this comment.
I'm not sure if bnbspider holds duplicated WETH anymore.
Overall seems fine, assets are reloaded but they are present after a few minutes which is a little bit annoying and I'm afraid people can complain about it. Please @0xDaedalus take a look before merging.
|
Update with main broke types 🤔 |
|
@hyphenized Is this PR still valid and we want to merge it at some point or not really? |
AFAIK This addressed the issue of a collision between two assets named Paging @Shadowfiend for visibility |
This commit introduces E2E test that checks the functiolality of verified/unverified tokens. Following general steps are executed: 1. Import account 2. Enable `Show unverified assets` 3. Hide asset 4. Trust asset 5. Hide trusted asset A number of checks is performed during each step. The commit also introduces helper functions and adds `data-testid` attribute to a couple of DOM elements. As some of the code is similar or identical as in the #3418 PR which is yet not merged to `main`, some conflicts may arise and will need to be resolved before this change lands on `main`. Also some changes will need to be made once #3195 gets merged to `main`, as this PR solves a bug causing failures in the tests (the failing part of the test was temporarily commented out).
|
btw I recall we were meant to add another warning for these duplicated tokens, right? It looks very weird to have 3 AAVE tokens and none of them is the real one 🙈 cc @VladUXUI |
## What's Changed * Repeater Connection: Forward requests to new default when default is switched off during dApp connection flow by @Shadowfiend in #3462 * Auto-Not-So-Matic: Fix two matic.network URLs to polygon.technology by @Shadowfiend in #3483 * v0.38.0 by @kkosiorowska in #3480 * Case Dismissed: Forcibly show DAppConnectionInfoBar popover on first dApp connection by @Shadowfiend in #3464 * Add Hardhat Fork Functionality by @0xDaedalus in #3247 * Faded Jeans: Rename fadeIn class to fade_in by @Shadowfiend in #3485 * Fix issue for discovery transaction hash by @kkosiorowska in #3458 * Full Sweep: Drop the USE_UPDATED_SIGNING_UI feature flag by @Shadowfiend in #3475 * Token Discovery - Remap redux asset balances by @hyphenized in #3195 * Make specific warnings for adding custom asset by @kkosiorowska in #3478 * Run NFTs e2e tests on a controlled wallet address by @michalinacienciala in #3487 * v0.38.1 by @jagodarybacka in #3484 **Full Changelog**: v0.38.1...v0.39.0 Latest build: [extension-builds-3496](https://github.com/tahowallet/extension/suites/13792957673/artifacts/764738599) (as of Thu, 22 Jun 2023 14:27:32 GMT).
This commit introduces E2E test that checks the functiolality of verified/unverified tokens. Following general steps are executed: 1. Import account 2. Enable `Show unverified assets` 3. Hide asset 4. Trust asset 5. Hide trusted asset A number of checks is performed during each step. The commit also introduces helper functions and adds `data-testid` attribute to a couple of DOM elements. TODO: - [x] As some of the code is similar or identical as in the #3418 PR which is yet not merged to `main`, some conflicts may arise and will need to be resolved before this change lands on `main`. - [x] Also some changes will need to be made once #3195 gets merged to `main`, as this PR solves a bug causing failures in the tests (the failing part of the test was temporarily commented out). - [x] Add `E2E_TEST_ONLY_WALLET_JSON_BODY` and `E2E_TEST_ONLY_WALLET_JSON_PASSWORD` secrets in GitHub's settings. - [x] Wait for #3559 to be merged and merge main to feature branch (should fix the failing `e2e-tests` job) - [x] Handle TODOs in the code Latest build: [extension-builds-3472](https://github.com/tahowallet/extension/suites/15116290312/artifacts/863816729) (as of Tue, 15 Aug 2023 18:12:30 GMT).
Prevents tokens with the same symbol from overriding existing balances. Changes the way combined amounts data is computed so that balances displayed in overview for e.g. USDC on Mainnet and USDC on Polygon get bundled together only if they both are sourced from a token list (or are network base assets).
To Test
bnbspider2.cryptoLatest build: extension-builds-3195 (as of Mon, 19 Jun 2023 09:45:50 GMT).