(Fix) Support CryptoKitties NFTs (#1574)

* create `logic/collectibles/utils` file and move all the
 NFT-related helper functions into it

`generateERC721TransferTxData` will decide whether the method
 to transfer an NFT will be `transfer` or `safeTransferFrom`,
 based on preset conditions where CryptoKitties tokens is taken
 as an exception.

Also, `transfer` was used instead of `transferFrom`
 because `transferFrom` is not implemented in the
 rinkeby version, and was the method used as a
 fallback before.

- moved `SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH` const
- moved `isSendERC721Transaction` function
- moved `getERC721Symbol` function
- moved `isERC721Contract` function
- created `getTransferMethodByContractAddress` along with `CK_ADDRESS` const
- created `generateERC721TransferTxData` function
- refactored `ReviewCollectible` component to use `generateERC721TransferTxData`
- updated tests

* remove `ENS_ADDRESS` constant as it's not used

* add unmock of collectibles/utils

* add tests for `getTransferMethodByContractAddress`

src/logic/collectibles/utils/__tests__/getTransferMethodByContractAddress.test.ts

@@ -0,0 +1,68 @@
+import { getTransferMethodByContractAddress } from 'src/logic/collectibles/utils'
+
+jest.mock('src/config', () => {
+  // Require the original module to not be mocked...
+  const originalModule = jest.requireActual('src/config')
+
+  return {
+    __esModule: true, // Use it when dealing with esModules
+    ...originalModule,
+    getNetworkId: jest.fn().mockReturnValue(4),
+  }
+})
+
+describe('getTransferMethodByContractAddress', () => {
+  const config = require('src/config')
+
+  afterAll(() => {
+    jest.unmock('src/config')
+  })
+
+  it(`should return "transfer" method, if CK address is provided for MAINNET`, () => {
+    // Given
+    config.getNetworkId.mockReturnValue(1)
+    const contractAddress = '0x06012c8cf97bead5deae237070f9587f8e7a266d'
+
+    // When
+    const selectedMethod = getTransferMethodByContractAddress(contractAddress)
+
+    // Then
+    expect(selectedMethod).toBe('transfer')
+  })
+
+  it(`should return "transfer" method, if CK address is provided for RINKEBY`, () => {
+    // Given
+    config.getNetworkId.mockReturnValue(4)
+    const contractAddress = '0x16baf0de678e52367adc69fd067e5edd1d33e3bf'
+
+    // When
+    const selectedMethod = getTransferMethodByContractAddress(contractAddress)
+
+    // Then
+    expect(selectedMethod).toBe('transfer')
+  })
+
+  it(`should return "0x42842e0e" method, if CK address is provided any other network`, () => {
+    // Given
+    config.getNetworkId.mockReturnValue(100)
+    const contractAddress = '0x06012c8cf97bead5deae237070f9587f8e7a266d'
+
+    // When
+    const selectedMethod = getTransferMethodByContractAddress(contractAddress)
+
+    // Then
+    expect(selectedMethod).toBe('0x42842e0e')
+  })
+
+  it(`should return "0x42842e0e" method, if non-CK address is provided`, () => {
+    // Given
+    config.getNetworkId.mockReturnValue(4)
+    const contractAddress = '0x57f1887a8BF19b14fC0dF6Fd9B2acc9Af147eA85'
+
+    // When
+    const selectedMethod = getTransferMethodByContractAddress(contractAddress)
+
+    // Then
+    expect(selectedMethod).toBe('0x42842e0e')
+  })
+})

src/logic/collectibles/utils/index.ts

@@ -0,0 +1,131 @@
+import { getNetworkId } from 'src/config'
+import { ETHEREUM_NETWORK } from 'src/config/networks/network.d'
+import { nftAssetsListAddressesSelector } from 'src/logic/collectibles/store/selectors'
+import { TxServiceModel } from 'src/logic/safe/store/actions/transactions/fetchTransactions/loadOutgoingTransactions'
+import { TOKEN_TRANSFER_METHODS_NAMES } from 'src/logic/safe/store/models/types/transactions.d'
+import { getERC721TokenContract, getStandardTokenContract } from 'src/logic/tokens/store/actions/fetchTokens'
+import { sameAddress } from 'src/logic/wallets/ethAddresses'
+import { CollectibleTx } from 'src/routes/safe/components/Balances/SendModal/screens/ReviewCollectible'
+import { store } from 'src/store'
+import { sameString } from 'src/utils/strings'
+
+// CryptoKitties Contract Addresses by network
+// This is an exception made for a popular NFT that's not ERC721 standard-compatible,
+//  so we can allow the user to transfer the assets by using `transferFrom` instead of
+//  the standard `safeTransferFrom` method.
+export const CK_ADDRESS = {
+  [ETHEREUM_NETWORK.MAINNET]: '0x06012c8cf97bead5deae237070f9587f8e7a266d',
+  [ETHEREUM_NETWORK.RINKEBY]: '0x16baf0de678e52367adc69fd067e5edd1d33e3bf',
+}
+
+// safeTransferFrom(address,address,uint256)
+export const SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH = '42842e0e'
+
+/**
+ * Verifies that a tx received by the transaction service is an ERC721 token-related transaction
+ * @param {TxServiceModel} tx
+ * @returns boolean
+ */
+export const isSendERC721Transaction = (tx: TxServiceModel): boolean => {
+  let hasERC721Transfer = false
+
+  if (tx.dataDecoded && sameString(tx.dataDecoded.method, TOKEN_TRANSFER_METHODS_NAMES.SAFE_TRANSFER_FROM)) {
+    hasERC721Transfer = tx.dataDecoded.parameters.findIndex((param) => sameString(param.name, 'tokenId')) !== -1
+  }
+
+  // Note: this is only valid with our current case (client rendering), if we move to server side rendering we need to refactor this
+  const state = store.getState()
+  const knownAssets = nftAssetsListAddressesSelector(state)
+  return knownAssets.includes(tx.to) || hasERC721Transfer
+}
+
+/**
+ * Returns the symbol of the provided ERC721 contract
+ * @param {string} contractAddress
+ * @returns Promise<string>
+ */
+export const getERC721Symbol = async (contractAddress: string): Promise<string> => {
+  let tokenSymbol = 'UNKNOWN'
+
+  try {
+    const ERC721token = await getERC721TokenContract()
+    const tokenInstance = await ERC721token.at(contractAddress)
+    tokenSymbol = tokenInstance.symbol()
+  } catch (err) {
+    // If the contract address is an ENS token contract, we know that the ERC721 standard is not proper implemented
+    // The method symbol() is missing
+    const ENS_TOKEN_CONTRACT = '0x57f1887a8BF19b14fC0dF6Fd9B2acc9Af147eA85'
+    if (sameAddress(contractAddress, ENS_TOKEN_CONTRACT)) {
+      return 'ENS'
+    }
+    console.error(`Failed to retrieve token symbol for ERC721 token ${contractAddress}`)
+  }
+
+  return tokenSymbol
+}
+
+/**
+ * Verifies if the provided contract is a valid ERC721
+ * @param {string} contractAddress
+ * @returns boolean
+ */
+export const isERC721Contract = async (contractAddress: string): Promise<boolean> => {
+  const ERC721Token = await getStandardTokenContract()
+  let isERC721 = false
+
+  try {
+    await ERC721Token.at(contractAddress)
+    isERC721 = true
+  } catch (error) {
+    console.warn('Asset not found')
+  }
+
+  return isERC721
+}
+
+/**
+ * Returns a method identifier based on the asset specified and the current network
+ * @param {string} contractAddress
+ * @returns string
+ */
+export const getTransferMethodByContractAddress = (contractAddress: string): string => {
+  if (sameAddress(contractAddress, CK_ADDRESS[getNetworkId()])) {
+    // on mainnet `transferFrom` seems to work fine but we can assure that `transfer` will work on both networks
+    // so that's the reason why we're falling back to `transfer` for CryptoKitties
+    return 'transfer'
+  }
+
+  return `0x${SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH}`
+}
+
+/**
+ * Builds the encodedABI data for the transfer of an NFT token
+ * @param {CollectibleTx} tx
+ * @param {string} safeAddress
+ * @returns Promise<string>
+ */
+export const generateERC721TransferTxData = async (
+  tx: CollectibleTx,
+  safeAddress: string | undefined,
+): Promise<string> => {
+  if (!safeAddress) {
+    throw new Error('Failed to build NFT transfer tx data. SafeAddress is not defined.')
+  }
+
+  const methodToCall = getTransferMethodByContractAddress(tx.assetAddress)
+  let transferParams = [tx.recipientAddress, tx.nftTokenId]
+  let NFTTokenContract
+
+  if (methodToCall.includes(SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH)) {
+    // we add the `from` param for the `safeTransferFrom` method call
+    transferParams = [safeAddress, ...transferParams]
+    NFTTokenContract = await getERC721TokenContract()
+  } else {
+    // we fallback to an ERC20 Token contract whose ABI implements the `transfer` method
+    NFTTokenContract = await getStandardTokenContract()
+  }
+
+  const tokenInstance = await NFTTokenContract.at(tx.assetAddress)
+
+  return tokenInstance.contract.methods[methodToCall](...transferParams).encodeABI()
+}

src/logic/safe/store/actions/__tests__/transactionHelpers.test.ts

@@ -273,9 +273,11 @@ describe('isOutgoingTransaction', () => {
   })
 })
 
+jest.mock('src/logic/collectibles/utils')
 jest.mock('src/logic/tokens/utils/tokenHelpers')
 describe('isCustomTransaction', () => {
   afterAll(() => {
+    jest.unmock('src/logic/collectibles/utils')
     jest.unmock('src/logic/tokens/utils/tokenHelpers')
   })
   it('It should return true if Is outgoing transaction, is not an erc20 transaction, not an upgrade transaction and not and erc721 transaction', async () => {
@@ -292,10 +294,11 @@ describe('isCustomTransaction', () => {
     })
     knownTokens.set('0x00Df91984582e6e96288307E9c2f20b38C8FeCE9', token)
 
+    const collectiblesHelpers = require('src/logic/collectibles/utils')
     const txHelpers = require('src/logic/tokens/utils/tokenHelpers')
 
     txHelpers.isSendERC20Transaction.mockImplementationOnce(() => false)
-    txHelpers.isSendERC721Transaction.mockImplementationOnce(() => false)
+    collectiblesHelpers.isSendERC721Transaction.mockImplementationOnce(() => false)
 
     // when
     const result = await isCustomTransaction(transaction, safeAddress)
@@ -303,7 +306,7 @@ describe('isCustomTransaction', () => {
     // then
     expect(result).toBe(true)
     expect(txHelpers.isSendERC20Transaction).toHaveBeenCalled()
-    expect(txHelpers.isSendERC721Transaction).toHaveBeenCalled()
+    expect(collectiblesHelpers.isSendERC721Transaction).toHaveBeenCalled()
   })
   it('It should return true if is outgoing transaction, is not SendERC20Transaction, is not isUpgradeTransaction and not isSendERC721Transaction', async () => {
     // given
@@ -319,10 +322,11 @@ describe('isCustomTransaction', () => {
     })
     knownTokens.set('0x00Df91984582e6e96288307E9c2f20b38C8FeCE9', token)
 
+    const collectiblesHelpers = require('src/logic/collectibles/utils')
     const txHelpers = require('src/logic/tokens/utils/tokenHelpers')
 
     txHelpers.isSendERC20Transaction.mockImplementationOnce(() => false)
-    txHelpers.isSendERC721Transaction.mockImplementationOnce(() => false)
+    collectiblesHelpers.isSendERC721Transaction.mockImplementationOnce(() => false)
 
     // when
     const result = await isCustomTransaction(transaction, safeAddress)
@@ -330,7 +334,7 @@ describe('isCustomTransaction', () => {
     // then
     expect(result).toBe(true)
     expect(txHelpers.isSendERC20Transaction).toHaveBeenCalled()
-    expect(txHelpers.isSendERC721Transaction).toHaveBeenCalled()
+    expect(collectiblesHelpers.isSendERC721Transaction).toHaveBeenCalled()
   })
   it('It should return false if is outgoing transaction, not SendERC20Transaction, isUpgradeTransaction and not isSendERC721Transaction', async () => {
     // given
@@ -347,10 +351,11 @@ describe('isCustomTransaction', () => {
     })
     knownTokens.set('0x00Df91984582e6e96288307E9c2f20b38C8FeCE9', token)
 
+    const collectiblesHelpers = require('src/logic/collectibles/utils')
     const txHelpers = require('src/logic/tokens/utils/tokenHelpers')
 
     txHelpers.isSendERC20Transaction.mockImplementationOnce(() => true)
-    txHelpers.isSendERC721Transaction.mockImplementationOnce(() => false)
+    collectiblesHelpers.isSendERC721Transaction.mockImplementationOnce(() => false)
 
     // when
     const result = await isCustomTransaction(transaction, safeAddress)
@@ -373,10 +378,11 @@ describe('isCustomTransaction', () => {
     })
     knownTokens.set('0x00Df91984582e6e96288307E9c2f20b38C8FeCE9', token)
 
+    const collectiblesHelpers = require('src/logic/collectibles/utils')
     const txHelpers = require('src/logic/tokens/utils/tokenHelpers')
 
     txHelpers.isSendERC20Transaction.mockImplementationOnce(() => false)
-    txHelpers.isSendERC721Transaction.mockImplementationOnce(() => true)
+    collectiblesHelpers.isSendERC721Transaction.mockImplementationOnce(() => true)
 
     // when
     const result = await isCustomTransaction(transaction, safeAddress)
@@ -384,7 +390,7 @@ describe('isCustomTransaction', () => {
     // then
     expect(result).toBe(false)
     expect(txHelpers.isSendERC20Transaction).toHaveBeenCalled()
-    expect(txHelpers.isSendERC721Transaction).toHaveBeenCalled()
+    expect(collectiblesHelpers.isSendERC721Transaction).toHaveBeenCalled()
   })
 })
 

src/logic/safe/store/actions/transactions/utils/transactionHelpers.ts

@@ -1,11 +1,7 @@
 import { List } from 'immutable'
 import { getNetworkInfo } from 'src/config'
-import {
+import { getERC20DecimalsAndSymbol, isSendERC20Transaction } from 'src/logic/tokens/utils/tokenHelpers'
-  getERC20DecimalsAndSymbol,
+import { getERC721Symbol, isSendERC721Transaction } from 'src/logic/collectibles/utils'
-  getERC721Symbol,
-  isSendERC20Transaction,
-  isSendERC721Transaction,
-} from 'src/logic/tokens/utils/tokenHelpers'
 import { sameAddress, ZERO_ADDRESS } from 'src/logic/wallets/ethAddresses'
 import { EMPTY_DATA } from 'src/logic/wallets/ethTransactions'
 import { makeConfirmation } from 'src/logic/safe/store/models/confirmation'

src/logic/tokens/utils/__tests__/tokenHelpers.test.ts

@@ -1,5 +1,6 @@
 import { makeToken } from 'src/logic/tokens/store/model/token'
-import { getERC20DecimalsAndSymbol, isERC721Contract, isTokenTransfer } from 'src/logic/tokens/utils/tokenHelpers'
+import { getERC20DecimalsAndSymbol, isTokenTransfer } from 'src/logic/tokens/utils/tokenHelpers'
+import { isERC721Contract } from 'src/logic/collectibles/utils'
 import { getMockedTxServiceModel } from 'src/test/utils/safeHelper'
 import { fromTokenUnit, toTokenUnit } from 'src/logic/tokens/utils/humanReadableValue'
 

src/logic/tokens/utils/tokenHelpers.ts

@@ -2,22 +2,13 @@ import { AbiItem } from 'web3-utils'
 
 import { getNetworkInfo } from 'src/config'
 import generateBatchRequests from 'src/logic/contracts/generateBatchRequests'
-import {
+import { getTokenInfos } from 'src/logic/tokens/store/actions/fetchTokens'
-  getStandardTokenContract,
+import { isSendERC721Transaction } from 'src/logic/collectibles/utils'
-  getTokenInfos,
-  getERC721TokenContract,
-} from 'src/logic/tokens/store/actions/fetchTokens'
 import { makeToken, Token } from 'src/logic/tokens/store/model/token'
 import { ALTERNATIVE_TOKEN_ABI } from 'src/logic/tokens/utils/alternativeAbi'
 import { web3ReadOnly as web3 } from 'src/logic/wallets/getWeb3'
 import { isEmptyData } from 'src/logic/safe/store/actions/transactions/utils/transactionHelpers'
 import { TxServiceModel } from 'src/logic/safe/store/actions/transactions/fetchTransactions/loadOutgoingTransactions'
-import { sameString } from 'src/utils/strings'
-import { TOKEN_TRANSFER_METHODS_NAMES } from 'src/logic/safe/store/models/types/transactions.d'
-import { store } from 'src/store'
-import { nftAssetsListAddressesSelector } from 'src/logic/collectibles/store/selectors'
-
-export const SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH = '42842e0e'
 
 export const getEthAsToken = (balance: string | number): Token => {
   const { nativeCoin } = getNetworkInfo()
@@ -45,37 +36,6 @@ export const isTokenTransfer = (tx: TxServiceModel): boolean => {
   return !isEmptyData(tx.data) && tx.data?.substring(0, 10) === '0xa9059cbb' && Number(tx.value) === 0
 }
 
-export const isSendERC721Transaction = (tx: TxServiceModel): boolean => {
-  let hasERC721Transfer = false
-
-  if (tx.dataDecoded && sameString(tx.dataDecoded.method, TOKEN_TRANSFER_METHODS_NAMES.SAFE_TRANSFER_FROM)) {
-    hasERC721Transfer = tx.dataDecoded.parameters.findIndex((param) => sameString(param.name, 'tokenId')) !== -1
-  }
-
-  // Note: this is only valid with our current case (client rendering), if we move to server side rendering we need to refactor this
-  const state = store.getState()
-  const knownAssets = nftAssetsListAddressesSelector(state)
-  return knownAssets.includes(tx.to) || hasERC721Transfer
-}
-
-export const getERC721Symbol = async (contractAddress: string): Promise<string> => {
-  let tokenSymbol = 'UNKNOWN'
-  try {
-    const ERC721token = await getERC721TokenContract()
-    const tokenInstance = await ERC721token.at(contractAddress)
-    tokenSymbol = tokenInstance.symbol()
-  } catch (err) {
-    // If the contract address is an ENS token contract, we know that the ERC721 standard is not proper implemented
-    // The method symbol() is missing
-    const ENS_TOKEN_CONTRACT = '0x57f1887a8BF19b14fC0dF6Fd9B2acc9Af147eA85'
-    if (sameString(contractAddress, ENS_TOKEN_CONTRACT)) {
-      return 'ENS'
-    }
-    console.error(`Failed to retrieve token symbol for ERC721 token ${contractAddress}`)
-  }
-  return tokenSymbol
-}
-
 export const getERC20DecimalsAndSymbol = async (
   tokenAddress: string,
 ): Promise<{ decimals: number; symbol: string }> => {
@@ -115,17 +75,3 @@ export const isSendERC20Transaction = async (tx: TxServiceModel): Promise<boolea
 
   return isSendTokenTx
 }
-
-export const isERC721Contract = async (contractAddress: string): Promise<boolean> => {
-  const ERC721Token = await getStandardTokenContract()
-  let isERC721 = false
-
-  try {
-    await ERC721Token.at(contractAddress)
-    isERC721 = true
-  } catch (error) {
-    console.warn('Asset not found')
-  }
-
-  return isERC721
-}

src/routes/safe/components/Balances/SendModal/screens/ReviewCollectible/index.tsx

@@ -20,13 +20,12 @@ import createTransaction from 'src/logic/safe/store/actions/createTransaction'
 import { safeSelector } from 'src/logic/safe/store/selectors'
 import { TX_NOTIFICATION_TYPES } from 'src/logic/safe/transactions'
 import { estimateTxGasCosts } from 'src/logic/safe/transactions/gas'
-import { getERC721TokenContract } from 'src/logic/tokens/store/actions/fetchTokens'
 import { formatAmount } from 'src/logic/tokens/utils/formatAmount'
-import { SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH } from 'src/logic/tokens/utils/tokenHelpers'
 import SafeInfo from 'src/routes/safe/components/Balances/SendModal/SafeInfo'
 import { setImageToPlaceholder } from 'src/routes/safe/components/Balances/utils'
 import { sm } from 'src/theme/variables'
 import { textShortener } from 'src/utils/strings'
+import { generateERC721TransferTxData } from 'src/logic/collectibles/utils'
 
 import ArrowDown from '../assets/arrow-down.svg'
 
@@ -67,14 +66,8 @@ const ReviewCollectible = ({ onClose, onPrev, tx }: Props): React.ReactElement =
 
     const estimateGas = async () => {
       try {
-        const methodToCall = `0x${SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH}`
+        const txData = await generateERC721TransferTxData(tx, safeAddress)
-        const transferParams = [tx.recipientAddress, tx.nftTokenId]
+        const estimatedGasCosts = await estimateTxGasCosts(safeAddress ?? '', tx.recipientAddress, txData)
-        const params = [safeAddress, ...transferParams]
-        const ERC721Token = await getERC721TokenContract()
-        const tokenInstance = await ERC721Token.at(tx.assetAddress)
-        const txData = tokenInstance.contract.methods[methodToCall](...params).encodeABI()
-
-        const estimatedGasCosts = await estimateTxGasCosts(safeAddress as string, tx.recipientAddress, txData)
         const gasCosts = fromTokenUnit(estimatedGasCosts, nativeCoin.decimals)
         const formattedGasCosts = formatAmount(gasCosts)
 
@@ -92,7 +85,7 @@ const ReviewCollectible = ({ onClose, onPrev, tx }: Props): React.ReactElement =
     return () => {
       isCurrent = false
     }
-  }, [safeAddress, tx.assetAddress, tx.nftTokenId, tx.recipientAddress])
+  }, [safeAddress, tx])
 
   const submitTx = async () => {
     try {

src/routes/safe/components/Balances/Tokens/screens/AddCustomAsset/validators.ts

@@ -1,5 +1,5 @@
 import memoize from 'lodash.memoize'
-import { isERC721Contract } from 'src/logic/tokens/utils/tokenHelpers'
+import { isERC721Contract } from 'src/logic/collectibles/utils'
 import { sameAddress } from 'src/logic/wallets/ethAddresses'
 
 // eslint-disable-next-line