From ca732001bfb68e0f440c28dda8acd22c7f5f3c75 Mon Sep 17 00:00:00 2001 From: Agustin Pane Date: Tue, 10 Nov 2020 17:09:41 -0300 Subject: [PATCH] (Fix) - #1561 Outgoing instead of custom tx for sending collectibles (#1567) * Refactor getTxData * Add SAFE_TRANSFER_FROM in SAFE_METHODS_NAMES * Adds check on isSendERC721Transaction for erc721 send * Adds TOKEN_TRANSFER_METHODS_NAMES types * Replace type SAFE_TRANSFER_FROM * Fix import * Adds nftAssetsListAddressSelector * Remove txCode and knownTokens from isSendERC721Transaction Now it directly checks agains the list of nftAssets on the store * Refactor ENS_TOKEN_CONTRACT usage check * Add TODO * Add return for ENS symbol Co-authored-by: Daniel Sanchez Co-authored-by: Fernando --- .../collectibles/store/selectors/index.ts | 4 + .../__tests__/transactionHelpers.test.ts | 14 +- .../loadOutgoingTransactions.ts | 14 +- .../transactions/utils/transactionHelpers.ts | 27 +-- .../safe/store/models/types/transactions.d.ts | 8 +- src/logic/tokens/utils/tokenHelpers.ts | 38 ++-- .../ExpandedTx/TxDescription/utils.ts | 199 ++++++++++++------ 7 files changed, 179 insertions(+), 125 deletions(-) diff --git a/src/logic/collectibles/store/selectors/index.ts b/src/logic/collectibles/store/selectors/index.ts index 1c5e6412..d6737a6a 100644 --- a/src/logic/collectibles/store/selectors/index.ts +++ b/src/logic/collectibles/store/selectors/index.ts @@ -16,6 +16,10 @@ export const nftAssetsListSelector = createSelector(nftAssets, (assets): NFTAsse return assets ? Object.values(assets) : [] }) +export const nftAssetsListAddressesSelector = createSelector(nftAssetsListSelector, (assets): string[] => { + return Array.from(new Set(assets.map((nftAsset) => nftAsset.address))) +}) + export const availableNftAssetsAddresses = createSelector(nftTokensSelector, (userNftTokens): string[] => { return Array.from(new Set(userNftTokens.map((nftToken) => nftToken.assetAddress))) }) diff --git a/src/logic/safe/store/actions/__tests__/transactionHelpers.test.ts b/src/logic/safe/store/actions/__tests__/transactionHelpers.test.ts index 6fb7a15d..43a76e6a 100644 --- a/src/logic/safe/store/actions/__tests__/transactionHelpers.test.ts +++ b/src/logic/safe/store/actions/__tests__/transactionHelpers.test.ts @@ -281,7 +281,6 @@ describe('isCustomTransaction', () => { it('It should return true if Is outgoing transaction, is not an erc20 transaction, not an upgrade transaction and not and erc721 transaction', async () => { // given const transaction = getMockedTxServiceModel({ to: safeAddress2, value: '0', data: 'test' }) - const txCode = '' const knownTokens = Map & Readonly>() const token = makeToken({ address: '0x00Df91984582e6e96288307E9c2f20b38C8FeCE9', @@ -299,7 +298,7 @@ describe('isCustomTransaction', () => { txHelpers.isSendERC721Transaction.mockImplementationOnce(() => false) // when - const result = await isCustomTransaction(transaction, txCode, safeAddress, knownTokens) + const result = await isCustomTransaction(transaction, safeAddress) // then expect(result).toBe(true) @@ -309,7 +308,6 @@ describe('isCustomTransaction', () => { it('It should return true if is outgoing transaction, is not SendERC20Transaction, is not isUpgradeTransaction and not isSendERC721Transaction', async () => { // given const transaction = getMockedTxServiceModel({ to: safeAddress2, value: '0', data: 'test' }) - const txCode = '' const knownTokens = Map & Readonly>() const token = makeToken({ address: '0x00Df91984582e6e96288307E9c2f20b38C8FeCE9', @@ -327,7 +325,7 @@ describe('isCustomTransaction', () => { txHelpers.isSendERC721Transaction.mockImplementationOnce(() => false) // when - const result = await isCustomTransaction(transaction, txCode, safeAddress, knownTokens) + const result = await isCustomTransaction(transaction, safeAddress) // then expect(result).toBe(true) @@ -338,7 +336,6 @@ describe('isCustomTransaction', () => { // given const upgradeTxData = `0x8d80ff0a000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000f200dfa693da0d16f5e7e78fdcbede8fc6ebea44f1cf000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000247de7edef000000000000000000000000d5d82b6addc9027b22dca772aa68d5d74cdbdf4400dfa693da0d16f5e7e78fdcbede8fc6ebea44f1cf00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000024f08a032300000000000000000000000034cfac646f301356faa8b21e94227e3583fe3f5f0000000000000000000000000000` const transaction = getMockedTxServiceModel({ to: safeAddress2, value: '0', data: upgradeTxData }) - const txCode = '' const knownTokens = Map & Readonly>() const token = makeToken({ address: '0x00Df91984582e6e96288307E9c2f20b38C8FeCE9', @@ -356,7 +353,7 @@ describe('isCustomTransaction', () => { txHelpers.isSendERC721Transaction.mockImplementationOnce(() => false) // when - const result = await isCustomTransaction(transaction, txCode, safeAddress, knownTokens) + const result = await isCustomTransaction(transaction, safeAddress) // then expect(result).toBe(false) @@ -365,7 +362,6 @@ describe('isCustomTransaction', () => { it('It should return false if is outgoing transaction, is not SendERC20Transaction, not isUpgradeTransaction and isSendERC721Transaction', async () => { // given const transaction = getMockedTxServiceModel({ to: safeAddress2, value: '0', data: 'test' }) - const txCode = '' const knownTokens = Map & Readonly>() const token = makeToken({ address: '0x00Df91984582e6e96288307E9c2f20b38C8FeCE9', @@ -383,7 +379,7 @@ describe('isCustomTransaction', () => { txHelpers.isSendERC721Transaction.mockImplementationOnce(() => true) // when - const result = await isCustomTransaction(transaction, txCode, safeAddress, knownTokens) + const result = await isCustomTransaction(transaction, safeAddress) // then expect(result).toBe(false) @@ -839,11 +835,9 @@ describe('buildTx', () => { const txResult = await buildTx({ cancellationTxs, currentUser: userAddress, - knownTokens, outgoingTxs, safe: safeInstance, tx: transaction, - txCode: undefined, }) // then diff --git a/src/logic/safe/store/actions/transactions/fetchTransactions/loadOutgoingTransactions.ts b/src/logic/safe/store/actions/transactions/fetchTransactions/loadOutgoingTransactions.ts index 4dca08fb..60e5e6ad 100644 --- a/src/logic/safe/store/actions/transactions/fetchTransactions/loadOutgoingTransactions.ts +++ b/src/logic/safe/store/actions/transactions/fetchTransactions/loadOutgoingTransactions.ts @@ -1,7 +1,6 @@ import { fromJS, List, Map } from 'immutable' import generateBatchRequests from 'src/logic/contracts/generateBatchRequests' -import { TOKEN_REDUCER_ID, TokenState } from 'src/logic/tokens/store/reducer/tokens' import { web3ReadOnly } from 'src/logic/wallets/getWeb3' import { PROVIDER_REDUCER_ID } from 'src/logic/wallets/store/reducer/provider' import { buildTx, isCancelTransaction } from 'src/logic/safe/store/actions/transactions/utils/transactionHelpers' @@ -9,7 +8,6 @@ import { SAFE_REDUCER_ID } from 'src/logic/safe/store/reducer/safe' import { store } from 'src/store' import fetchTransactions from 'src/logic/safe/store/actions/transactions/fetchTransactions/fetchTransactions' import { Transaction, TransactionTypes } from 'src/logic/safe/store/models/types/transaction' -import { Token } from 'src/logic/tokens/store/model/token' import { SafeRecord } from 'src/logic/safe/store/models/safe' import { DataDecoded } from 'src/logic/safe/store/models/types/transactions.d' @@ -65,7 +63,6 @@ export type OutgoingTxs = { export type BatchProcessTxsProps = OutgoingTxs & { currentUser?: string - knownTokens: Map safe: SafeRecord } @@ -138,7 +135,6 @@ const batchRequestContractCode = (transactions: TxServiceModel[]): Promise => { +export const isCustomTransaction = async (tx: TxServiceModel, safeAddress?: string): Promise => { const isOutgoing = isOutgoingTransaction(tx, safeAddress) - const isErc20 = await isSendERC20Transaction(tx, txCode, knownTokens) + const isErc20 = await isSendERC20Transaction(tx) const isUpgrade = isUpgradeTransaction(tx) - const isErc721 = isSendERC721Transaction(tx, txCode, knownTokens) + const isErc721 = isSendERC721Transaction(tx) return isOutgoing && !isErc20 && !isUpgrade && !isErc721 } @@ -232,27 +225,24 @@ export const calculateTransactionType = (tx: Transaction): TransactionTypeValues export type BuildTx = BatchProcessTxsProps & { tx: TxServiceModel - txCode?: string } export const buildTx = async ({ cancellationTxs, currentUser, - knownTokens, outgoingTxs, safe, tx, - txCode, }: BuildTx): Promise => { const safeAddress = safe.address const { nativeCoin } = getNetworkInfo() const isModifySettingsTx = isModifySettingsTransaction(tx, safeAddress) const isTxCancelled = isTransactionCancelled(tx, outgoingTxs, cancellationTxs) - const isSendERC721Tx = isSendERC721Transaction(tx, txCode, knownTokens) - const isSendERC20Tx = await isSendERC20Transaction(tx, txCode, knownTokens) + const isSendERC721Tx = isSendERC721Transaction(tx) + const isSendERC20Tx = await isSendERC20Transaction(tx) const isMultiSendTx = isMultiSendTransaction(tx) const isUpgradeTx = isUpgradeTransaction(tx) - const isCustomTx = await isCustomTransaction(tx, txCode, safeAddress, knownTokens) + const isCustomTx = await isCustomTransaction(tx, safeAddress) const isCancellationTx = isCancelTransaction(tx, safeAddress) const refundParams = await getRefundParams(tx, getERC20DecimalsAndSymbol) const decodedParams = getDecodedParams(tx) @@ -323,7 +313,6 @@ export type TxToMock = TxArgs & { } export const mockTransaction = (tx: TxToMock, safeAddress: string, state: AppReduxState): Promise => { - const knownTokens: Map = state[TOKEN_REDUCER_ID] const safe = safeSelector(state) const cancellationTxs = safeCancellationTransactionsSelector(state) const outgoingTxs = safeTransactionsSelector(state) @@ -335,11 +324,9 @@ export const mockTransaction = (tx: TxToMock, safeAddress: string, state: AppRed return buildTx({ cancellationTxs, currentUser: undefined, - knownTokens, outgoingTxs, safe, tx: (tx as unknown) as TxServiceModel, - txCode: EMPTY_DATA, }) } diff --git a/src/logic/safe/store/models/types/transactions.d.ts b/src/logic/safe/store/models/types/transactions.d.ts index a9fcc709..b606d49b 100644 --- a/src/logic/safe/store/models/types/transactions.d.ts +++ b/src/logic/safe/store/models/types/transactions.d.ts @@ -241,7 +241,13 @@ export const METHOD_TO_ID = { export type SafeMethods = typeof SAFE_METHODS_NAMES[keyof typeof SAFE_METHODS_NAMES] -type TokenMethods = 'transfer' | 'transferFrom' | 'safeTransferFrom' +export const TOKEN_TRANSFER_METHODS_NAMES = { + TRANSFER: 'transfer', + TRANSFER_FROM: 'transferFrom', + SAFE_TRANSFER_FROM: 'safeTransferFrom', +} as const + +type TokenMethods = typeof TOKEN_TRANSFER_METHODS_NAMES[keyof typeof TOKEN_TRANSFER_METHODS_NAMES] type SafeDecodedParams = { [key in SafeMethods]?: Record diff --git a/src/logic/tokens/utils/tokenHelpers.ts b/src/logic/tokens/utils/tokenHelpers.ts index b7bb4b2f..ef2f7852 100644 --- a/src/logic/tokens/utils/tokenHelpers.ts +++ b/src/logic/tokens/utils/tokenHelpers.ts @@ -8,11 +8,14 @@ import { getERC721TokenContract, } from 'src/logic/tokens/store/actions/fetchTokens' import { makeToken, Token } from 'src/logic/tokens/store/model/token' -import { TokenState } from 'src/logic/tokens/store/reducer/tokens' 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' @@ -42,14 +45,17 @@ 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, txCode?: string, knownTokens?: TokenState): boolean => { - // "0x57f1887a8BF19b14fC0dF6Fd9B2acc9Af147eA85" - ens token contract, includes safeTransferFrom - // but no proper ERC721 standard implemented - return ( - (txCode?.includes(SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH) && - tx.to !== '0x57f1887a8BF19b14fC0dF6Fd9B2acc9Af147eA85') || - (isTokenTransfer(tx) && !knownTokens?.get(tx.to)) - ) +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 => { @@ -59,6 +65,12 @@ export const getERC721Symbol = async (contractAddress: string): Promise 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 @@ -89,12 +101,8 @@ export const getERC20DecimalsAndSymbol = async ( return tokenInfo } -export const isSendERC20Transaction = async ( - tx: TxServiceModel, - txCode?: string, - knownTokens?: TokenState, -): Promise => { - let isSendTokenTx = !isSendERC721Transaction(tx, txCode, knownTokens) && isTokenTransfer(tx) +export const isSendERC20Transaction = async (tx: TxServiceModel): Promise => { + let isSendTokenTx = !isSendERC721Transaction(tx) && isTokenTransfer(tx) if (isSendTokenTx) { const { decimals, symbol } = await getERC20DecimalsAndSymbol(tx.to) diff --git a/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.ts b/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.ts index d204b863..2ba34f22 100644 --- a/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.ts +++ b/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.ts @@ -29,77 +29,142 @@ interface TxData { upgradeTx?: boolean } -export const getTxData = (tx: Transaction): TxData => { +const getTxDataForModifySettingsTxs = (tx: Transaction): TxData => { const txData: TxData = {} - if (tx.decodedParams) { - if (tx.isTokenTransfer) { - const { to } = tx.decodedParams.transfer || {} - txData.recipient = to - txData.isTokenTransfer = true - } else if (tx.isCollectibleTransfer) { - const { safeTransferFrom, transfer, transferFrom } = tx.decodedParams - const { to, value } = safeTransferFrom || transferFrom || transfer || {} - txData.recipient = to - txData.tokenId = value - txData.isCollectibleTransfer = true - } else if (tx.modifySettingsTx) { - txData.recipient = tx.recipient - txData.modifySettingsTx = true + if (!tx.modifySettingsTx || !tx.decodedParams) { + return txData + } - if (tx.decodedParams[SAFE_METHODS_NAMES.REMOVE_OWNER]) { - const { _threshold, owner } = tx.decodedParams[SAFE_METHODS_NAMES.REMOVE_OWNER] - txData.action = SAFE_METHODS_NAMES.REMOVE_OWNER - txData.removedOwner = owner - txData.newThreshold = _threshold - } else if (tx.decodedParams[SAFE_METHODS_NAMES.CHANGE_THRESHOLD]) { - const { _threshold } = tx.decodedParams[SAFE_METHODS_NAMES.CHANGE_THRESHOLD] - txData.action = SAFE_METHODS_NAMES.CHANGE_THRESHOLD - txData.newThreshold = _threshold - } else if (tx.decodedParams[SAFE_METHODS_NAMES.ADD_OWNER_WITH_THRESHOLD]) { - const { _threshold, owner } = tx.decodedParams[SAFE_METHODS_NAMES.ADD_OWNER_WITH_THRESHOLD] - txData.action = SAFE_METHODS_NAMES.ADD_OWNER_WITH_THRESHOLD - txData.addedOwner = owner - txData.newThreshold = _threshold - } else if (tx.decodedParams[SAFE_METHODS_NAMES.SWAP_OWNER]) { - const { newOwner, oldOwner } = tx.decodedParams[SAFE_METHODS_NAMES.SWAP_OWNER] - txData.action = SAFE_METHODS_NAMES.SWAP_OWNER - txData.removedOwner = oldOwner - txData.addedOwner = newOwner - } else if (tx.decodedParams[SAFE_METHODS_NAMES.ENABLE_MODULE]) { - const { module } = tx.decodedParams[SAFE_METHODS_NAMES.ENABLE_MODULE] - txData.action = SAFE_METHODS_NAMES.ENABLE_MODULE - txData.module = module - } else if (tx.decodedParams[SAFE_METHODS_NAMES.DISABLE_MODULE]) { - const { module } = tx.decodedParams[SAFE_METHODS_NAMES.DISABLE_MODULE] - txData.action = SAFE_METHODS_NAMES.DISABLE_MODULE - txData.module = module - } - } else if (tx.multiSendTx) { - txData.recipient = tx.recipient - txData.data = tx.data - txData.customTx = true - } else { - txData.recipient = tx.recipient - txData.data = tx.data - txData.customTx = true - } - } else if (tx.customTx) { - txData.recipient = tx.recipient - txData.data = tx.data - txData.customTx = true - } else if (Number(tx.value) > 0) { - txData.recipient = tx.recipient - } else if (tx.isCancellationTx) { - txData.cancellationTx = true - } else if (tx.creationTx) { - txData.creationTx = true - } else if (tx.upgradeTx) { - txData.upgradeTx = true - txData.data = `The contract of this Safe is upgraded to Version ${getSafeVersion(tx.data)}` - } else { - txData.recipient = tx.recipient + txData.recipient = tx.recipient + txData.modifySettingsTx = true + + if (tx.decodedParams[SAFE_METHODS_NAMES.REMOVE_OWNER]) { + const { _threshold, owner } = tx.decodedParams[SAFE_METHODS_NAMES.REMOVE_OWNER] + txData.action = SAFE_METHODS_NAMES.REMOVE_OWNER + txData.removedOwner = owner + txData.newThreshold = _threshold + + return txData + } + if (tx.decodedParams[SAFE_METHODS_NAMES.CHANGE_THRESHOLD]) { + const { _threshold } = tx.decodedParams[SAFE_METHODS_NAMES.CHANGE_THRESHOLD] + txData.action = SAFE_METHODS_NAMES.CHANGE_THRESHOLD + txData.newThreshold = _threshold + return txData + } + if (tx.decodedParams[SAFE_METHODS_NAMES.ADD_OWNER_WITH_THRESHOLD]) { + const { _threshold, owner } = tx.decodedParams[SAFE_METHODS_NAMES.ADD_OWNER_WITH_THRESHOLD] + txData.action = SAFE_METHODS_NAMES.ADD_OWNER_WITH_THRESHOLD + txData.addedOwner = owner + txData.newThreshold = _threshold + return txData + } + + if (tx.decodedParams[SAFE_METHODS_NAMES.SWAP_OWNER]) { + const { newOwner, oldOwner } = tx.decodedParams[SAFE_METHODS_NAMES.SWAP_OWNER] + txData.action = SAFE_METHODS_NAMES.SWAP_OWNER + txData.removedOwner = oldOwner + txData.addedOwner = newOwner + return txData + } + + if (tx.decodedParams[SAFE_METHODS_NAMES.ENABLE_MODULE]) { + const { module } = tx.decodedParams[SAFE_METHODS_NAMES.ENABLE_MODULE] + txData.action = SAFE_METHODS_NAMES.ENABLE_MODULE + txData.module = module + return txData + } + + if (tx.decodedParams[SAFE_METHODS_NAMES.DISABLE_MODULE]) { + const { module } = tx.decodedParams[SAFE_METHODS_NAMES.DISABLE_MODULE] + txData.action = SAFE_METHODS_NAMES.DISABLE_MODULE + txData.module = module + return txData } return txData } + +const getTxDataForTxsWithDecodedParams = (tx: Transaction): TxData => { + const txData: TxData = {} + + if (!tx.decodedParams) { + return txData + } + + if (tx.isTokenTransfer) { + const { to } = tx.decodedParams.transfer || {} + txData.recipient = to + txData.isTokenTransfer = true + return txData + } + + if (tx.isCollectibleTransfer) { + const { safeTransferFrom, transfer, transferFrom } = tx.decodedParams + const { to, value } = safeTransferFrom || transferFrom || transfer || {} + txData.recipient = to + txData.tokenId = value + txData.isCollectibleTransfer = true + + return txData + } + + if (tx.modifySettingsTx) { + return getTxDataForModifySettingsTxs(tx) + } + + if (tx.multiSendTx) { + txData.recipient = tx.recipient + txData.data = tx.data + txData.customTx = true + return txData + } + + txData.recipient = tx.recipient + txData.data = tx.data + txData.customTx = true + + return txData +} + +// @todo (agustin) this function does not makes much sense +// it should be refactored to simplify unnecessary if's checks and re-asigning props to the txData object +export const getTxData = (tx: Transaction): TxData => { + const txData: TxData = {} + + if (tx.decodedParams) { + return getTxDataForTxsWithDecodedParams(tx) + } + + if (tx.customTx) { + txData.recipient = tx.recipient + txData.data = tx.data + txData.customTx = true + return txData + } + if (Number(tx.value) > 0) { + txData.recipient = tx.recipient + return txData + } + + if (tx.isCancellationTx) { + txData.cancellationTx = true + return txData + } + + if (tx.creationTx) { + txData.creationTx = true + return txData + } + + if (tx.upgradeTx) { + txData.upgradeTx = true + txData.data = `The contract of this Safe is upgraded to Version ${getSafeVersion(tx.data)}` + + return txData + } + txData.recipient = tx.recipient + + return txData +}