Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Untrusted assets should not block the addition of custom tokens #3491

Merged
merged 17 commits into from
Jul 8, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion background/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ import {
OneTimeAnalyticsEvent,
} from "./lib/posthog"
import {
isVerifiedAsset,
isBuiltInNetworkBaseAsset,
isSameAsset,
} from "./redux-slices/utils/asset-utils"
Expand Down Expand Up @@ -1894,14 +1895,17 @@ export default class Main extends BaseService<never> {
const mainCurrencyAmount = convertedAssetAmount
? assetAmountToDesiredDecimals(convertedAssetAmount, 2)
: undefined
// Existing assets are those that are verified by default or by the user.
// This check allows the user to add an asset that is on the unverified list.
const exists = cachedAsset ? isVerifiedAsset(cachedAsset) : false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const exists = cachedAsset ? isVerifiedAsset(cachedAsset) : false
const shouldDisplay = cachedAsset && isVerifiedAsset(cachedAsset)

The logic and naming seems a little clearer like this, I think, but I'm not 100% sure it's all accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


return {
...assetData,
balance: Number.parseFloat(
utils.formatUnits(assetData.amount, assetData.asset.decimals)
),
mainCurrencyAmount,
exists: !!cachedAsset,
exists,
}
}

Expand Down
4 changes: 2 additions & 2 deletions background/redux-slices/selectors/0xSwapSelectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { selectCurrentNetwork } from "./uiSelectors"
import { SwappableAsset, isSmartContractFungibleAsset } from "../../assets"
import { sameNetwork } from "../../networks"
import {
canBeUsedForTransaction,
isVerifiedAsset,
isBuiltInNetworkBaseAsset,
} from "../utils/asset-utils"
import { RootState } from ".."
Expand All @@ -29,7 +29,7 @@ export const selectSwapBuyAssets = createSelector(
): asset is SwappableAsset & {
recentPrices: SingleAssetState["recentPrices"]
} => {
if (!canBeUsedForTransaction(asset)) {
if (!isVerifiedAsset(asset)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're stacking enough conditions here that most of this function is noise. I wonder if we wouldn't be better served rewriting it as:

  return isVerifiedAsset(asset) && (
    // Only list assets for the current network.
    isBuiltInNetworkBaseAsset(asset, currentNetwork) ||
    (isSmartContractFungibleAsset(asset) && sameNetwork(asset.homeNetwork, currentNetwork))
  )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return false
}
if (isSmartContractFungibleAsset(asset)) {
Expand Down
28 changes: 17 additions & 11 deletions background/redux-slices/utils/asset-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,23 @@ export function isUnverifiedAssetByUser(asset: AnyAsset | undefined): boolean {
return false
}

/**
* Check if an asset is verified.
* The asset can be verified by us when it is trusted by default.
* Untrusted asset can be manually verified by the user.
*
* Only verified assets can take part in wallet actions.
* By actions is meant:
* - doing an swap with this asset
* - sending this asset to another address
*/
export function isVerifiedAsset(asset: AnyAsset): boolean {
if (!isEnabled(FeatureFlags.SUPPORT_UNVERIFIED_ASSET)) {
return true
}
return isUntrustedAsset(asset) ? !isUnverifiedAssetByUser(asset) : true
Copy link
Contributor

@Shadowfiend Shadowfiend Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't isUnverifiedAssetByUser already take isUntrustedAsset into account?

const baseAsset = isNetworkBaseAsset(asset)
const isUntrusted = isUntrustedAsset(asset)
return !baseAsset && isUntrusted

I think if we dig through and do a bunch of really ugly K Maps of this1, we can simplify this assertion to:

  return isNetworkBaseAsset(asset) || !isUntrustedAsset(asset)

I'm having a lot of difficulty reasoning my way through these though and I think it's because we're using a lot of negatives, and we've also muddled three concepts I want to circle back to: baseline trusted assets, trusted assets, and verified assets. I think we can define a very clear, stackable set of conditions that are easier to reason about with these:

  • Baseline trusted means “the wallet ~trusts this”—i.e., the asset is in a token list OR the asset is a network base asset. Currently !isUnverifiedAssetByUser sort of tries to do this.
  • Verified means the user has explicitly verified the asset.
  • Trusted means the asset is baseline trusted OR verified. Currently that is what isVerifiedAsset reports.

In this scenario the negatives also have very clear meanings as the opposites of the above (!...):

  • Baseline untrusted means the asset is !baseline trusted—i.e. “the wallet does not trust this without an explicit user signal”. Currently this is roughly equivalent to isUntrustedAsset. It could still be verified.
  • Unverified means the asset is !verified—i.e., the user has not explicitly verified the asset. It can still be baseline trusted.
  • Untrusted means the asset is !trusted—i.e. the asset is neither baseline trusted NOR verified. Currently this is roughly equivalent to isUnverifiedAssetByUser.

I strongly believe these three concepts are enough to handle all of what we're trying to address, and can all be stated in terms of positives (verified, trusted, baselineTrusted) most of the time and only as negatives untrusted, untrustedByUser, etc) when we need to check the negative. They also feel straightforward to unit test, explain, and document.

Do you agree? Have I missed an edge case? I found about 4 while I was writing and rewriting this comment, so I wouldn't be surprised if I missed another 😅

Footnotes

  1. image

Copy link
Contributor

@Shadowfiend Shadowfiend Jun 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here:

  • Wanted to clarify the broader rework (the new terms) probably doesn't need to block this PR.
  • I did miss an edge case: if verified is set to false (rather than unset), an asset should not be trusted*, regardless of whether it's baseline trusted, because that means the user explicitly marked it not verified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been brilliantly explained. Let's simplify this logic to make it clear and unambiguous. PR with a new concept #3528

}

type AssetType = "base" | "erc20"

export type AssetID = `${AssetType}/${string}`
Expand All @@ -391,17 +408,6 @@ export const getAssetID = (
return `erc20/${asset.contractAddress}`
}

/**
* Assets that are untrusted and have not been verified by the user
* should not be swapped or sent.
*/
export function canBeUsedForTransaction(asset: AnyAsset): boolean {
if (!isEnabled(FeatureFlags.SUPPORT_UNVERIFIED_ASSET)) {
return true
}
return isUntrustedAsset(asset) ? !isUnverifiedAssetByUser(asset) : true
}

// FIXME Unify once asset similarity code is unified.
export function isSameAsset(asset1?: AnyAsset, asset2?: AnyAsset): boolean {
if (typeof asset1 === "undefined" || typeof asset2 === "undefined") {
Expand Down
45 changes: 8 additions & 37 deletions background/services/indexing/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ import {
normalizeEVMAddress,
sameEVMAddress,
} from "../../lib/utils"
import {
getActiveAssetsByAddressForNetwork,
getAssetsByAddress,
shouldRefreshKnownAsset,
} from "./utils"

// Transactions seen within this many blocks of the chain tip will schedule a
// token refresh sooner than the standard rate.
Expand Down Expand Up @@ -75,29 +80,6 @@ interface Events extends ServiceLifecycleEvents {
removeAssetData: SmartContractFungibleAsset
}

const getAssetsByAddress = (assets: SmartContractFungibleAsset[]) => {
const activeAssetsByAddress = assets.reduce((agg, t) => {
const newAgg = {
...agg,
}
newAgg[t.contractAddress.toLowerCase()] = t
return newAgg
}, {} as { [address: string]: SmartContractFungibleAsset })

return activeAssetsByAddress
}

const getActiveAssetsByAddressForNetwork = (
network: EVMNetwork,
activeAssetsToTrack: SmartContractFungibleAsset[]
) => {
const networkActiveAssets = activeAssetsToTrack.filter(
(asset) => asset.homeNetwork.chainID === network.chainID
)

return getAssetsByAddress(networkActiveAssets)
}

/**
* IndexingService is responsible for pulling and maintaining all application-
* level "indexing" data — things like fungible token balances and NFTs, as well
Expand Down Expand Up @@ -700,20 +682,9 @@ export default class IndexingService extends BaseService<Events> {
normalizedAddress
)

if (knownAsset) {
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
}
if (knownAsset && !shouldRefreshKnownAsset(knownAsset, metadata)) {
await this.addAssetToTrack(knownAsset)
return knownAsset
}

let customAsset = await this.db.getCustomAssetByAddressAndNetwork(
Expand Down
68 changes: 68 additions & 0 deletions background/services/indexing/utils/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { SmartContractFungibleAsset } from "../../../assets"
import { EVMNetwork } from "../../../networks"
import {
isUntrustedAsset,
isVerifiedAsset,
} from "../../../redux-slices/utils/asset-utils"
import { HexString } from "../../../types"

export const getAssetsByAddress = (
assets: SmartContractFungibleAsset[]
): {
[address: string]: SmartContractFungibleAsset
} => {
const activeAssetsByAddress = assets.reduce((agg, t) => {
const newAgg = {
...agg,
}
newAgg[t.contractAddress.toLowerCase()] = t
return newAgg
}, {} as { [address: string]: SmartContractFungibleAsset })

return activeAssetsByAddress
}

export const getActiveAssetsByAddressForNetwork = (
network: EVMNetwork,
activeAssetsToTrack: SmartContractFungibleAsset[]
): {
[address: string]: SmartContractFungibleAsset
} => {
const networkActiveAssets = activeAssetsToTrack.filter(
(asset) => asset.homeNetwork.chainID === network.chainID
)

return getAssetsByAddress(networkActiveAssets)
}

export function shouldRefreshKnownAsset(
asset: SmartContractFungibleAsset,
metadata: {
discoveryTxHash?: {
[address: HexString]: HexString
}
verified?: boolean
}
): boolean {
const newDiscoveryTxHash = metadata?.discoveryTxHash
const addressForDiscoveryTxHash = newDiscoveryTxHash
? Object.keys(newDiscoveryTxHash)[0]
: undefined
const existingDiscoveryTxHash = addressForDiscoveryTxHash
? asset.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
// Additionally, discovery tx Hash should only be added for untrusted assets.
const allowAddDiscoveryTxHash =
isUntrustedAsset(asset) && !(!newDiscoveryTxHash || existingDiscoveryTxHash)

// Refresh a known unverified asset if it has been manually imported.
// This check allows the user to add an asset from the unverified list.
const isManuallyImported = metadata?.verified
const allowVerifyAssetByManualImport =
!isVerifiedAsset(asset) && isManuallyImported

return allowVerifyAssetByManualImport || allowAddDiscoveryTxHash
}
115 changes: 115 additions & 0 deletions background/services/indexing/utils/tests/index.unit.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
import { shouldRefreshKnownAsset } from ".."
import { createSmartContractAsset } from "../../../../tests/factories"
import * as featureFlags from "../../../../features"

const ADDRESS = "0x0000000000000000000000000000000000000000"
const DISCOVERY_TX_HASH = "0x0000000000000000000000000000000000000000"
const METADATA_TX = {
discoveryTxHash: {
[ADDRESS]: DISCOVERY_TX_HASH,
},
}
const METADATA_VERIFIED = {
verified: true,
}

const TRUSTED_ASSET = createSmartContractAsset()
const UNTRUSTED_ASSET = createSmartContractAsset({
metadata: { tokenLists: [] },
})

describe("IndexingService utils", () => {
describe("shouldRefreshKnownAsset", () => {
beforeAll(() => {
jest.spyOn(featureFlags, "isEnabled").mockImplementation(() => true)
})

it("Refresh the untrusted asset if manually imported", () => {
const shouldBeRefreshed = shouldRefreshKnownAsset(
UNTRUSTED_ASSET,
METADATA_VERIFIED
)

expect(shouldBeRefreshed).toBe(true)
})

it("Not refresh the untrusted asset if not manually imported", () => {
const shouldBeRefreshed = shouldRefreshKnownAsset(UNTRUSTED_ASSET, {})

expect(shouldBeRefreshed).toBe(false)
})

it("Not refresh the trusted asset if not manually imported", () => {
const shouldBeRefreshed = shouldRefreshKnownAsset(TRUSTED_ASSET, {})

expect(shouldBeRefreshed).toBe(false)
})

// This state is not quite possible in the app because the user is not able to manually import a trusted asset that exists.
it("Not refresh the trusted asset if manually imported", () => {
const shouldBeRefreshed = shouldRefreshKnownAsset(
TRUSTED_ASSET,
METADATA_VERIFIED
)

expect(shouldBeRefreshed).toBe(false)
})

it("Refresh the untrusted asset if it does not already have a discovered tx hash", () => {
const shouldBeRefreshed = shouldRefreshKnownAsset(
UNTRUSTED_ASSET,
METADATA_TX
)

expect(shouldBeRefreshed).toBe(true)
})

it("Not refresh the trusted asset if it does not already have a discovered tx hash", () => {
const shouldBeRefreshed = shouldRefreshKnownAsset(
TRUSTED_ASSET,
METADATA_TX
)

expect(shouldBeRefreshed).toBe(false)
})

it("Not refresh the untrusted asset if it does already have a discovered tx hash", () => {
const asset = {
...UNTRUSTED_ASSET,
metadata: { ...UNTRUSTED_ASSET, ...METADATA_TX },
}
const shouldBeRefreshed = shouldRefreshKnownAsset(asset, METADATA_TX)

expect(shouldBeRefreshed).toBe(false)
})

it("Not refresh the trusted asset if it does already have a discovered tx hash", () => {
const asset = {
...TRUSTED_ASSET,
metadata: { ...TRUSTED_ASSET, ...METADATA_TX },
}
const shouldBeRefreshed = shouldRefreshKnownAsset(asset, METADATA_TX)

expect(shouldBeRefreshed).toBe(false)
})

it("Not refresh the trusted asset if discovered tx hash is not specified", () => {
const asset = {
...TRUSTED_ASSET,
metadata: { ...TRUSTED_ASSET, ...METADATA_TX },
}

expect(shouldRefreshKnownAsset(TRUSTED_ASSET, {})).toBe(false)
expect(shouldRefreshKnownAsset(asset, {})).toBe(false)
})
it("Not refresh the untrusted asset if discovered tx hash is not specified", () => {
const asset = {
...UNTRUSTED_ASSET,
metadata: { ...UNTRUSTED_ASSET, ...METADATA_TX },
}

expect(shouldRefreshKnownAsset(UNTRUSTED_ASSET, {})).toBe(false)
expect(shouldRefreshKnownAsset(asset, {})).toBe(false)
})
})
})
4 changes: 2 additions & 2 deletions ui/pages/Send.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
} from "@tallyho/tally-background/redux-slices/assets"
import { CompleteAssetAmount } from "@tallyho/tally-background/redux-slices/accounts"
import {
canBeUsedForTransaction,
isVerifiedAsset,
enrichAssetAmountWithMainCurrencyValues,
} from "@tallyho/tally-background/redux-slices/utils/asset-utils"
import { useHistory, useLocation } from "react-router-dom"
Expand Down Expand Up @@ -112,7 +112,7 @@ export default function Send(): ReactElement {
(assetAmount): assetAmount is CompleteAssetAmount<FungibleAsset> =>
isFungibleAssetAmount(assetAmount) &&
assetAmount.decimalAmount > 0 &&
canBeUsedForTransaction(assetAmount.asset)
isVerifiedAsset(assetAmount.asset)
)
const assetPricePoint = useBackgroundSelector((state) =>
selectAssetPricePoint(state.assets, selectedAsset, mainCurrencySymbol)
Expand Down
4 changes: 2 additions & 2 deletions ui/utils/swap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { debounce, DebouncedFunc } from "lodash"
import { useState, useRef, useCallback } from "react"
import { CompleteAssetAmount } from "@tallyho/tally-background/redux-slices/accounts"
import {
canBeUsedForTransaction,
isVerifiedAsset,
isSameAsset,
} from "@tallyho/tally-background/redux-slices/utils/asset-utils"
import { useBackgroundDispatch, useBackgroundSelector } from "../hooks"
Expand Down Expand Up @@ -271,7 +271,7 @@ export function getOwnedSellAssetAmounts(
(isSmartContractFungibleAsset(assetAmount.asset) ||
assetAmount.asset.symbol === currentNetwork.baseAsset.symbol) &&
assetAmount.decimalAmount > 0 &&
canBeUsedForTransaction(assetAmount.asset)
isVerifiedAsset(assetAmount.asset)
) ?? []
)
}
Expand Down