Fix custom asset price fetching #3508

hyphenized · 2023-06-30T02:51:29Z

~~Fixes price fetching of assets that were first discovered as custom assets and then retrieved from token lists~~ Now in Don't treat assets in the token list as unverified #3534.
~~Fixes a bug that caused token list assets to have a discovery transaction hash and to also be included as custom assets.~~
Enables price fetching for all trusted assets: base network, token list, and custom assets that have been verified.

To Test

Verify that some custom assets have price information (an easy way to test this would be to remove all token lists from preference service defaults and reinstall the wallet)

Latest build: extension-builds-3508 (as of Thu, 13 Jul 2023 06:55:02 GMT).

An asset might be discovered and tracked as a custom asset before being included in a token list for different reasons. Consequently, local asset cache, which includes token list and custom assets, should be checked before determining if a custom asset needs be excluded from price fetching.

Moved default networks, base assets and RPCs to a static property so it's easier to override networks used during tests

Since the addition of custom networks, default network settings are no longer setup at chain service initialization, but, at chain database. Since these defaults are now exposed through a static property on the class we can update these tests to stub and select which networks we want to use on a given test suite.

kkosiorowska

I tested the solution and it looks like the prices load correctly after changing the branch. 🚀 In my case, it took quite a long time to reload the prices. I left some small thoughts.

kkosiorowska · 2023-07-04T07:43:07Z

background/redux-slices/utils/asset-utils.ts

+export function isTokenListAsset(asset?: AnyAsset): boolean {
+  return !!asset?.metadata?.tokenLists?.length
+}
+


kkosiorowska · 2023-07-04T07:51:56Z

background/redux-slices/utils/asset-utils.ts

+/**
+ * Returns a string that can be used as a unique identifier for an asset
+ */
+export const getFullAssetID = (


Non-blocking comment
Could we extend the description of the function in more detail? At first sight, it is a bit unclear in my opinion (What I have in mind is why we have two functions determining the id of an asset). I checked that getAssetID is used when determining token balances which makes sense and therefore does not return a unique id. I think it might be worth expanding on the description of these functions.

This is still used for balances, so we'd need a migration to remove it completely. The difference is that getFullAssetID includes the chainID. This wasn't necessary at the time since balances are already indexed by chainID. However, we can remove this in #3526

I would say it would be clearer if we could rename it to something like getAssetIDwithChainID or so - agreed that full is not explaining much and if someone will decide to use it elsewhere then it can be confusing why this and not the other one. But if we plan to remove one of the functions soon then it's fine.

Removed from this PR in favor of improving it in #3526

background/services/chain/db.ts

kkosiorowska · 2023-07-04T08:07:03Z

background/services/chain/tests/index.integration.test.ts

@@ -57,15 +57,15 @@ describe("ChainService", () => {

      const initializeBaseAssets = sandbox.spy(
        chainServiceInstance.db,
-        "initializeBaseAssets"
+        "initializeBaseAssets" as keyof ChainDatabase


hmm I wonder how we can improve this. 🤔 I'm thinking about using something like below. But I'm not sure what is better.

type ChainDatabaseExternalized = Omit<ChainDatabase, ""> & { initializeBaseAssets: () => Promise<void> } type ChainServiceExternalized = Omit<ChainService, ""> & { db: ChainDatabaseExternalized }

Would be easier to just ditch access modifiers in favour of dangling underscores cc @Shadowfiend

If we're needing private access for integration tests we are either writing bad tests or not providing correct access modifiers. My guess is we're writing bad integration tests (because integration tests are notoriously difficult to write at the correct abstraction level).

I'll try to look in more detail tomorrow. In the meantime, I both agree that the instinct to avoid as is correct, and I think we can bear the as in the context of the test since it has lower implication on production behavior.

background/services/indexing/index.ts

kkosiorowska · 2023-07-04T08:16:36Z

background/services/indexing/index.ts

-      const newDiscoveryTxHash = metadata?.discoveryTxHash
-      const addressForDiscoveryTxHash = newDiscoveryTxHash
-        ? Object.keys(newDiscoveryTxHash)[0]
-        : undefined
-      const existingDiscoveryTxHash = addressForDiscoveryTxHash
-        ? knownAsset.metadata?.discoveryTxHash?.[addressForDiscoveryTxHash]
-        : undefined
-      // If the discovery tx hash is not specified
-      // or if it already exists in the asset, do not update the asset
-      if (!newDiscoveryTxHash || existingDiscoveryTxHash) {
-        await this.addAssetToTrack(knownAsset)
-        return knownAsset
-      }
+      await this.addAssetToTrack(knownAsset)


This logic updated the discovery tx hash for unverified assets that were retrieved earlier and did not have it set. Let's leave it as it is and fix the issue of price fetching. And I'll come back to this in the next PR for unverified assets.

Please take a look at this #3534

jagodarybacka · 2023-07-04T10:36:59Z

background/redux-slices/utils/asset-utils.ts

+export function isTokenListAsset(asset?: AnyAsset): boolean {
+  return !!asset?.metadata?.tokenLists?.length
+}
+
 /**
 * Check if the asset has a list of tokens.
 * Assets that do not have it are considered untrusted.
 *
 */
 export function isUntrustedAsset(asset: AnyAsset | undefined): boolean {


Why are we allowing undefined here? Seems weird, if we wouldn't allow undefined then this function would be redundant to isTokenListAsset

This pattern allows us to do isUntrustedAsset(mightBeUndefined) instead of mightBeUndefined && isUntrustedAsset(...) within components.

I know but still, it seems like additional logic this function shouldn't be responsible for, also the function name and comment above don't indicate that we have this kind of logic here.

Yeah also this makes it harder to reuse this function as !isUntrustedAsset to determine trust because if something is undefined it will return true

jagodarybacka · 2023-07-04T11:05:26Z

background/services/chain/db.ts

@@ -39,6 +39,12 @@ type AccountAssetTransferLookup = {
 // TODO keep track of transaction replacement / nonce invalidation

 export class ChainDatabase extends Dexie {
+  static defaultSettings = {


Why do we need this?

It's much easier to stub this during testing if it's exposed as a property on the class

This reverts commit 8a6d55b.

kkosiorowska · 2023-07-07T13:45:40Z

background/services/indexing/index.ts

+        // Only filter custom assets which do not appear in token lists
+        !isTokenListAsset(
+          this.getKnownSmartContractAsset(
+            asset.homeNetwork,
+            asset.contractAddress
+          )
+        )


I'm not sure how best to solve this. Because #3491 improves the logic of trusted assets and is also in review. But we should also fetch prices for untrusted but verified assets. We will probably fix this in the next PR when both changes are merged.

All trusted assets should get price-checked, whether they were on token lists or not (I made this remark in 3491 as well).

What I'm not 100% sure about is—does that mean this PR is unnecessary?

This PR was made to address the same issue in #3534 which caused price fetching problems for token list assets.

Since we should fetch prices for verified assets, it makes sense that this fix should address that now too.

I think I've made a bit of a mess by opening 3534. The changes have started to overlap and I have therefore separated this into a separate PR so that there are no problems with testing the fix either.

I think the whole thing should be done in this PR. I will close my PR(3534) and put this issue in your hands if you don't mind.

Shadowfiend · 2023-07-11T11:33:01Z

background/services/indexing/index.ts

    const customAssets = await this.db.getActiveCustomAssetsByNetworks(
      trackedNetworks
    )

-    const customAssetsById = new Set(customAssets.map(getAssetId))
+    const customAssetsById = new Set(customAssets.map(getFullAssetID))

    // Filter all assets based on supported networks
    const activeAssetsToTrack = assetsToTrack.filter((asset) => {
      // Skip custom assets


Ok I have found the most specific version of my confusion! The answer may be due to recent changes making this unnecessary.

The question is: why are we skipping custom assets here at all? Any custom assets? We're burning a lot of logic where it feels like we could just be filtering by isTrusted and calling it a day--no looking up the custom asset list, no adding a new id type, etc--just assetsToTrack.filter((asset) => isTrusted(asset) && trackedNetworks.some((trackedNetwork) => isAssetOnNetwork(asset, trackedNetwork))).

@kkosiorowska also just updated a TODO on CustomAssetListItem that seems like it could be nuked with that change.

The question is: why are we skipping custom assets here at all? Any custom assets? We're burning a lot of logic where it feels like we could just be filtering by isTrusted and calling it a day--no looking up the custom asset list, no adding a new id type, etc--just assetsToTrack.filter((asset) => isTrusted(asset) && trackedNetworks.some((trackedNetwork) => isAssetOnNetwork(asset, trackedNetwork))).

This change was made before the decision to start fetching prices for verified assets

Amazing!

EDIT: By which I mean I'm looking forward to code being deleted 😁

This includes custom assets that have been marked as verified

This util can be further improved and tested in a different PR

`getKnownSmartContractAsset` already returns tracked assets

Shadowfiend

I haven't had a chance to do extensive testing here, but everything about this seems right and everything about it smells right, so I suggest we do some light smoke testing merge and then let the release testing pick everything else up.

Shadowfiend · 2023-07-11T20:52:31Z

background/redux-slices/utils/asset-utils.ts

+type ChainID = string
+
+export type FullAssetID = `${ChainID}/${AssetID}`
+
+/**
+ * Returns a string that can be used as an identifier for an asset
+ * TODO: This should be removed in favour of getFullAssetID; Base
+ * assets should not use symbol in their identifier
+ */


This all still seems tied to the FullAssetID stuff that got ripped out. Not a huge deal and not blocking, just flagging it.

kkosiorowska · 2023-07-13T11:18:31Z

Enables price fetching for all trusted assets: base network, token list, and custom assets that have been verified.

@hyphenized This point wasn't marked as being done. It seems that after verification we still don't have prices for such an asset. I'm not sure what the next steps are for this and whether we shouldn't create a task to not lose this.

@jagodarybacka

## 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).

hyphenized added 3 commits June 29, 2023 21:10

Re-use asset ids and utilities within indexing service

058de82

Do not check discovery tx hashes for token list assets

8a6d55b

hyphenized force-pushed the relative-value branch from 268e92d to 0bdd11e Compare July 3, 2023 01:23

hyphenized added 2 commits July 3, 2023 22:15

Change initializers and defaults visibility

fe3a483

Moved default networks, base assets and RPCs to a static property so it's easier to override networks used during tests

hyphenized force-pushed the relative-value branch from 4bcf4d7 to 3c6fd59 Compare July 4, 2023 03:27

hyphenized changed the title ~~Custom assets cleanup~~ Fix custom asset price fetching Jul 4, 2023

hyphenized marked this pull request as ready for review July 4, 2023 03:38

hyphenized requested a review from a team July 4, 2023 03:47

Merge branch 'main' into relative-value

a57e453

kkosiorowska reviewed Jul 4, 2023

View reviewed changes

jagodarybacka reviewed Jul 4, 2023

View reviewed changes

hyphenized added 2 commits July 4, 2023 17:36

Refactor to reuse utils

da9b25c

Revert "Do not check discovery tx hashes for token list assets"

256e014

This reverts commit 8a6d55b.

kkosiorowska reviewed Jul 7, 2023

View reviewed changes

hyphenized mentioned this pull request Jul 7, 2023

Don't treat assets in the token list as unverified #3534

Closed

5 tasks

kkosiorowska mentioned this pull request Jul 11, 2023

Refactor of terms for verified/unverified assets #3528

Merged

10 tasks

Shadowfiend reviewed Jul 11, 2023

View reviewed changes

hyphenized marked this pull request as draft July 11, 2023 13:59

hyphenized added 3 commits July 11, 2023 15:00

Merge remote-tracking branch 'origin/main' into relative-value

ab73493

Enable price fetching for trusted assets

8e4a74f

This includes custom assets that have been marked as verified

Remove getFullAssetId

ab058c4

This util can be further improved and tested in a different PR

hyphenized marked this pull request as ready for review July 11, 2023 20:18

Re-use sameNetwork util, remove duplicate check

af8f4d3

`getKnownSmartContractAsset` already returns tracked assets

hyphenized force-pushed the relative-value branch from 3012e20 to af8f4d3 Compare July 11, 2023 20:26

Shadowfiend approved these changes Jul 12, 2023

View reviewed changes

kkosiorowska enabled auto-merge July 13, 2023 06:46

Merge branch 'main' into relative-value

13e6c9c

kkosiorowska merged commit 23b8fa3 into main Jul 13, 2023
5 checks passed

kkosiorowska deleted the relative-value branch July 13, 2023 06:49

kkosiorowska mentioned this pull request Jul 13, 2023

v0.42.0 #3549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix custom asset price fetching #3508

Fix custom asset price fetching #3508

hyphenized commented Jun 30, 2023 •

edited by kkosiorowska

Loading

kkosiorowska left a comment

kkosiorowska Jul 4, 2023

kkosiorowska Jul 4, 2023

hyphenized Jul 4, 2023

jagodarybacka Jul 6, 2023

hyphenized Jul 11, 2023

kkosiorowska Jul 4, 2023

hyphenized Jul 4, 2023

Shadowfiend Jul 5, 2023

kkosiorowska Jul 4, 2023

kkosiorowska Jul 7, 2023

jagodarybacka Jul 4, 2023

hyphenized Jul 4, 2023

jagodarybacka Jul 6, 2023

hyphenized Jul 6, 2023

jagodarybacka Jul 4, 2023

hyphenized Jul 4, 2023

kkosiorowska Jul 7, 2023

Shadowfiend Jul 8, 2023

hyphenized Jul 10, 2023

kkosiorowska Jul 11, 2023 •

edited

Loading

Shadowfiend Jul 11, 2023

hyphenized Jul 11, 2023

Shadowfiend Jul 11, 2023 •

edited

Loading

Shadowfiend left a comment •

edited

Loading

Shadowfiend Jul 11, 2023

kkosiorowska commented Jul 13, 2023

Fix custom asset price fetching #3508

Fix custom asset price fetching #3508

Conversation

hyphenized commented Jun 30, 2023 • edited by kkosiorowska Loading

To Test

kkosiorowska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkosiorowska Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Shadowfiend left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkosiorowska commented Jul 13, 2023

hyphenized commented Jun 30, 2023 •

edited by kkosiorowska

Loading

kkosiorowska Jul 11, 2023 •

edited

Loading

Shadowfiend Jul 11, 2023 •

edited

Loading

Shadowfiend left a comment •

edited

Loading