From 4aeb5df55bdb8cd7955611a8ec57b2c7a6e45110 Mon Sep 17 00:00:00 2001 From: Mikhail Mikheev Date: Thu, 9 Apr 2020 18:21:52 +0400 Subject: [PATCH] v1.9.4 (#754) * Bug: Invalid V in signature with eth_sign (#728) * Fix invalid V with metamask/ledger * DONT FORGET TO REVERT BEFORE MERGING: test deployment * DONT FORGET TO REVERT BEFORE MERGING 2: test deployment * Revert "DONT FORGET TO REVERT BEFORE MERGING 2: test deployment" This reverts commit 8331f2a78f7fc8f53eb893899f16edd8238c68ff. * Revert "DONT FORGET TO REVERT BEFORE MERGING: test deployment" This reverts commit 03b81e31820ce4fe078a7131c2f0caa2af4870ac. * BUG: Only injected providers are cached as last used provider (#733) * cache every used provider, not only injected one * package json update * (Fix) Adapt app to back-end changes (#736) * refactor: Set success status to `201` (CREATED) * refactor: return `null` when there's no latestTx * (Fix) Transaction not automatically executed (#716) * feature: action/reducer to UPDATE_SAFE_NONCE * refactor: when processing txs returned from backend, extract latest tx nonce value and store it in the safe's state * chore: update `yarn.lock` * refactor: `UPDATE_SAFE_THRESHOLD` and `UPDATE_SAFE_NONCE` discarded in favor of `UPDATE_SAFE` * refactor: use `SAFE_REDUCER_ID` constant * refactor: remove `updateSafeNonce` file * (Fix) Change the order of the upgrade methods lookup (#740) * fix: change the order of the upgrade methods lookup The `isUpgradeTransaction` method was looking for the methods in an wrong order (#599). The proper order was set in #610, but `isUpgradeTransaction` wasn't updated. * fix: contract upgrade version lookup * Feature: Use eth_sign for hardware wallets connected via onboard.js (#742) * Use eth_sign for hardware wallets * install onboard.js with fix from forked repo * rebuild yarn.lock to fix cached onboard * update bnc-onboard * update package json (#743) * (Fix) Properly decode threshold value in tx details (#749) * fix: Display new threshold value when changing its value There was a typo for the `changeThreshold` action fixes #746 * refactor: use a constant for safe methods names * fix: check for `decimals` method in transferredTokens (#748) Previously we were looking for `decimals` hash in the contract `code`. There are some contracts like USDC who happen to be behind a FiatTokenProxy, making it upgradable. By directly calling the `decimals()` method, we interact with the contract and can be sure that the `decimals()` method is present. fixes #678 * Bug #747: Don't use getLastTxNonce to fetch safe nonce (#750) * don't use getLastTxNonce to fetch safe nonce * fetch safe nonce in checkAndUpdateSafe * checkAndUpdateSafe refactor * remove nonce update logic from UPDATE_SAFE reducer * handle the case when localSafe returns undefined * handle the case when localSafe returns undefined in buildTransactionFrom * bump package json version to 1.9.4 Co-authored-by: Fernando --- package.json | 2 +- src/logic/contracts/methodIds.js | 15 +++-- src/logic/tokens/store/actions/fetchTokens.js | 3 +- src/logic/tokens/utils/tokenHelpers.js | 12 ++++ .../ExpandedTx/TxDescription/index.jsx | 9 +-- .../ExpandedTx/TxDescription/utils.js | 9 +-- src/routes/safe/container/actions.js | 4 +- src/routes/safe/container/index.jsx | 4 +- src/routes/safe/store/actions/fetchSafe.js | 57 +++++++++++-------- .../safe/store/actions/fetchTransactions.js | 40 ++----------- src/routes/safe/store/reducer/safe.js | 8 --- 11 files changed, 76 insertions(+), 87 deletions(-) diff --git a/package.json b/package.json index 48b1f3bd2d..6c32f5b70b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "safe-react", - "version": "1.9.3", + "version": "1.9.4", "description": "Allowing crypto users manage funds in a safer way", "homepage": "https://github.com/gnosis/safe-react#readme", "bugs": { diff --git a/src/logic/contracts/methodIds.js b/src/logic/contracts/methodIds.js index 2ea3becc3a..a946cf3857 100644 --- a/src/logic/contracts/methodIds.js +++ b/src/logic/contracts/methodIds.js @@ -39,11 +39,18 @@ import { getWeb3 } from '~/logic/wallets/getWeb3' // { name: "getTransactionHash", id: "0xd8d11f78" } // ] +export const SAFE_METHODS_NAMES = { + ADD_OWNER_WITH_THRESHOLD: 'addOwnerWithThreshold', + CHANGE_THRESHOLD: 'changeThreshold', + REMOVE_OWNER: 'removeOwner', + SWAP_OWNER: 'swapOwner', +} + const METHOD_TO_ID = { - '0xe318b52b': 'swapOwner', - '0x0d582f13': 'addOwnerWithThreshold', - '0xf8dc5dd9': 'removeOwner', - '0x694e80c3': 'changeThreshold', + '0xe318b52b': SAFE_METHODS_NAMES.SWAP_OWNER, + '0x0d582f13': SAFE_METHODS_NAMES.ADD_OWNER_WITH_THRESHOLD, + '0xf8dc5dd9': SAFE_METHODS_NAMES.REMOVE_OWNER, + '0x694e80c3': SAFE_METHODS_NAMES.CHANGE_THRESHOLD, } export const decodeParamsFromSafeMethod = async (data: string) => { diff --git a/src/logic/tokens/store/actions/fetchTokens.js b/src/logic/tokens/store/actions/fetchTokens.js index d1b82e709e..1a5baa5c6f 100644 --- a/src/logic/tokens/store/actions/fetchTokens.js +++ b/src/logic/tokens/store/actions/fetchTokens.js @@ -12,8 +12,7 @@ import { fetchTokenList } from '~/logic/tokens/api' import { type TokenProps, makeToken } from '~/logic/tokens/store/model/token' import { tokensSelector } from '~/logic/tokens/store/selectors' import { getWeb3 } from '~/logic/wallets/getWeb3' -import { type GlobalState } from '~/store' -import { store } from '~/store/index' +import { type GlobalState, store } from '~/store' import { ensureOnce } from '~/utils/singleton' const createStandardTokenContract = async () => { diff --git a/src/logic/tokens/utils/tokenHelpers.js b/src/logic/tokens/utils/tokenHelpers.js index c12d3287b2..6ae0061339 100644 --- a/src/logic/tokens/utils/tokenHelpers.js +++ b/src/logic/tokens/utils/tokenHelpers.js @@ -1,4 +1,5 @@ // @flow +import ERC20Detailed from '@openzeppelin/contracts/build/contracts/ERC20Detailed.json' import { List } from 'immutable' import logo from '~/assets/icons/icon_etherTokens.svg' @@ -56,6 +57,17 @@ export const isAddressAToken = async (tokenAddress: string) => { return call !== '0x' } +export const hasDecimalsMethod = async (address: string): Promise => { + try { + const web3 = getWeb3() + const token = new web3.eth.Contract(ERC20Detailed.abi, address) + await token.methods.decimals().call() + return true + } catch (e) { + return false + } +} + export const isTokenTransfer = (data: string, value: number): boolean => !!data && data.substring(0, 10) === '0xa9059cbb' && value === 0 diff --git a/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/index.jsx b/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/index.jsx index 8940eff4b3..dd09fcec82 100644 --- a/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/index.jsx +++ b/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/index.jsx @@ -10,6 +10,7 @@ import Bold from '~/components/layout/Bold' import LinkWithRef from '~/components/layout/Link' import Paragraph from '~/components/layout/Paragraph' import { getNameFromAddressBook } from '~/logic/addressBook/utils' +import { SAFE_METHODS_NAMES } from '~/logic/contracts/methodIds' import { shortVersionOf } from '~/logic/wallets/ethAddresses' import OwnerAddressTableCell from '~/routes/safe/components/Settings/ManageOwners/OwnerAddressTableCell' import { getTxAmount } from '~/routes/safe/components/Transactions/TxsTable/columns' @@ -120,7 +121,7 @@ const NewThreshold = ({ newThreshold }: { newThreshold: string }) => ( ) const SettingsDescription = ({ action, addedOwner, newThreshold, removedOwner }: DescriptionDescProps) => { - if (action === 'removeOwner' && removedOwner && newThreshold) { + if (action === SAFE_METHODS_NAMES.REMOVE_OWNER && removedOwner && newThreshold) { return ( <> @@ -129,11 +130,11 @@ const SettingsDescription = ({ action, addedOwner, newThreshold, removedOwner }: ) } - if (action === 'changedThreshold' && newThreshold) { + if (action === SAFE_METHODS_NAMES.CHANGE_THRESHOLD && newThreshold) { return } - if (action === 'addOwnerWithThreshold' && addedOwner && newThreshold) { + if (action === SAFE_METHODS_NAMES.ADD_OWNER_WITH_THRESHOLD && addedOwner && newThreshold) { return ( <> @@ -142,7 +143,7 @@ const SettingsDescription = ({ action, addedOwner, newThreshold, removedOwner }: ) } - if (action === 'swapOwner' && removedOwner && addedOwner) { + if (action === SAFE_METHODS_NAMES.SWAP_OWNER && removedOwner && addedOwner) { return ( <> diff --git a/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.js b/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.js index 111e62d0d0..9c78435245 100644 --- a/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.js +++ b/src/routes/safe/components/Transactions/TxsTable/ExpandedTx/TxDescription/utils.js @@ -1,4 +1,5 @@ // @flow +import { SAFE_METHODS_NAMES } from '~/logic/contracts/methodIds' import { getWeb3 } from '~/logic/wallets/getWeb3' import { type Transaction } from '~/routes/safe/store/models/transaction' @@ -51,15 +52,15 @@ export const getTxData = (tx: Transaction): DecodedTxData => { if (tx.decodedParams) { txData.action = tx.decodedParams.methodName - if (txData.action === 'removeOwner') { + if (txData.action === SAFE_METHODS_NAMES.REMOVE_OWNER) { txData.removedOwner = tx.decodedParams.args[1] txData.newThreshold = tx.decodedParams.args[2] - } else if (txData.action === 'changeThreshold') { + } else if (txData.action === SAFE_METHODS_NAMES.CHANGE_THRESHOLD) { txData.newThreshold = tx.decodedParams.args[0] - } else if (txData.action === 'addOwnerWithThreshold') { + } else if (txData.action === SAFE_METHODS_NAMES.ADD_OWNER_WITH_THRESHOLD) { txData.addedOwner = tx.decodedParams.args[0] txData.newThreshold = tx.decodedParams.args[1] - } else if (txData.action === 'swapOwner') { + } else if (txData.action === SAFE_METHODS_NAMES.SWAP_OWNER) { txData.removedOwner = tx.decodedParams.args[1] txData.addedOwner = tx.decodedParams.args[2] } diff --git a/src/routes/safe/container/actions.js b/src/routes/safe/container/actions.js index 9eb078befe..4118b9fb74 100644 --- a/src/routes/safe/container/actions.js +++ b/src/routes/safe/container/actions.js @@ -29,7 +29,7 @@ export type Actions = { fetchLatestMasterContractVersion: typeof fetchLatestMasterContractVersion, activateTokensByBalance: typeof activateTokensByBalance, activateAssetsByBalance: typeof activateAssetsByBalance, - checkAndUpdateSafeOwners: typeof checkAndUpdateSafe, + checkAndUpdateSafe: typeof checkAndUpdateSafe, fetchCurrencyValues: typeof fetchCurrencyValues, loadAddressBook: typeof loadAddressBookFromStorage, updateAddressBookEntry: typeof updateAddressBookEntry, @@ -50,7 +50,7 @@ export default { fetchEtherBalance, fetchLatestMasterContractVersion, fetchCurrencyValues, - checkAndUpdateSafeOwners: checkAndUpdateSafe, + checkAndUpdateSafe, loadAddressBook: loadAddressBookFromStorage, updateAddressBookEntry, addViewedSafe, diff --git a/src/routes/safe/container/index.jsx b/src/routes/safe/container/index.jsx index 279b680aac..8f2cce7b75 100644 --- a/src/routes/safe/container/index.jsx +++ b/src/routes/safe/container/index.jsx @@ -121,14 +121,14 @@ class SafeView extends React.Component { checkForUpdates() { const { activeTokens, - checkAndUpdateSafeOwners, + checkAndUpdateSafe, fetchEtherBalance, fetchTokenBalances, fetchTransactions, safe, safeUrl, } = this.props - checkAndUpdateSafeOwners(safeUrl) + checkAndUpdateSafe(safeUrl) fetchTokenBalances(safeUrl, activeTokens) fetchEtherBalance(safe) fetchTransactions(safeUrl) diff --git a/src/routes/safe/store/actions/fetchSafe.js b/src/routes/safe/store/actions/fetchSafe.js index e0d6775719..76b21bb1ff 100644 --- a/src/routes/safe/store/actions/fetchSafe.js +++ b/src/routes/safe/store/actions/fetchSafe.js @@ -67,40 +67,47 @@ export const checkAndUpdateSafe = (safeAdd: string) => async (dispatch: ReduxDis // Check if the owner's safe did change and update them const [gnosisSafe, localSafe] = await Promise.all([getGnosisSafeInstanceAt(safeAddress), getLocalSafe(safeAddress)]) - const remoteOwners = await gnosisSafe.getOwners() + const [remoteOwners, remoteNonce, remoteThreshold] = await Promise.all([ + gnosisSafe.getOwners(), + gnosisSafe.nonce(), + gnosisSafe.getThreshold(), + ]) // Converts from [ { address, ownerName} ] to address array - const localOwners = localSafe.owners.map((localOwner) => localOwner.address) - const localThreshold = localSafe.threshold + const localOwners = localSafe ? localSafe.owners.map((localOwner) => localOwner.address) : undefined + const localThreshold = localSafe ? localSafe.threshold : undefined + const localNonce = localSafe ? localSafe.nonce : undefined - // Updates threshold values - const remoteThreshold = await gnosisSafe.getThreshold() - localSafe.threshold = remoteThreshold.toNumber() + if (localNonce !== remoteNonce.toNumber()) { + dispatch(updateSafe({ address: safeAddress, nonce: remoteNonce.toNumber() })) + } if (localThreshold !== remoteThreshold.toNumber()) { dispatch(updateSafe({ address: safeAddress, threshold: remoteThreshold.toNumber() })) } // If the remote owners does not contain a local address, we remove that local owner - localOwners.forEach((localAddress) => { - const remoteOwnerIndex = remoteOwners.findIndex((remoteAddress) => sameAddress(remoteAddress, localAddress)) - if (remoteOwnerIndex === -1) { - dispatch(removeSafeOwner({ safeAddress, ownerAddress: localAddress })) - } - }) + if (localOwners) { + localOwners.forEach((localAddress) => { + const remoteOwnerIndex = remoteOwners.findIndex((remoteAddress) => sameAddress(remoteAddress, localAddress)) + if (remoteOwnerIndex === -1) { + dispatch(removeSafeOwner({ safeAddress, ownerAddress: localAddress })) + } + }) - // If the remote has an owner that we don't have locally, we add it - remoteOwners.forEach((remoteAddress) => { - const localOwnerIndex = localOwners.findIndex((localAddress) => sameAddress(remoteAddress, localAddress)) - if (localOwnerIndex === -1) { - dispatch( - addSafeOwner({ - safeAddress, - ownerAddress: remoteAddress, - ownerName: 'UNKNOWN', - }), - ) - } - }) + // If the remote has an owner that we don't have locally, we add it + remoteOwners.forEach((remoteAddress) => { + const localOwnerIndex = localOwners.findIndex((localAddress) => sameAddress(remoteAddress, localAddress)) + if (localOwnerIndex === -1) { + dispatch( + addSafeOwner({ + safeAddress, + ownerAddress: remoteAddress, + ownerName: 'UNKNOWN', + }), + ) + } + }) + } } // eslint-disable-next-line consistent-return diff --git a/src/routes/safe/store/actions/fetchTransactions.js b/src/routes/safe/store/actions/fetchTransactions.js index 06d35f0ab3..9d505c5ca7 100644 --- a/src/routes/safe/store/actions/fetchTransactions.js +++ b/src/routes/safe/store/actions/fetchTransactions.js @@ -15,8 +15,8 @@ import { getLocalSafe } from '~/logic/safe/utils' import { getTokenInfos } from '~/logic/tokens/store/actions/fetchTokens' import { ALTERNATIVE_TOKEN_ABI } from '~/logic/tokens/utils/alternativeAbi' import { - DECIMALS_METHOD_HASH, SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH, + hasDecimalsMethod, isMultisendTransaction, isTokenTransfer, isUpgradeTransaction, @@ -25,7 +25,6 @@ import { ZERO_ADDRESS, sameAddress } from '~/logic/wallets/ethAddresses' import { EMPTY_DATA } from '~/logic/wallets/ethTransactions' import { getWeb3 } from '~/logic/wallets/getWeb3' import { addCancellationTransactions } from '~/routes/safe/store/actions/addCancellationTransactions' -import updateSafe from '~/routes/safe/store/actions/updateSafe' import { makeConfirmation } from '~/routes/safe/store/models/confirmation' import { type IncomingTransaction, makeIncomingTransaction } from '~/routes/safe/store/models/incomingTransaction' import { makeOwner } from '~/routes/safe/store/models/owner' @@ -75,14 +74,14 @@ type IncomingTxServiceModel = { } export const buildTransactionFrom = async (safeAddress: string, tx: TxServiceModel): Promise => { - const { owners } = await getLocalSafe(safeAddress) + const localSafe = await getLocalSafe(safeAddress) const confirmations = List( tx.confirmations.map((conf: ConfirmationServiceModel) => { let ownerName = 'UNKNOWN' - if (owners) { - const storedOwner = owners.find((owner) => sameAddress(conf.owner, owner.address)) + if (localSafe && localSafe.owners) { + const storedOwner = localSafe.owners.find((owner) => sameAddress(conf.owner, owner.address)) if (storedOwner) { ownerName = storedOwner.name @@ -102,7 +101,7 @@ export const buildTransactionFrom = async (safeAddress: string, tx: TxServiceMod const code = tx.to ? await web3.eth.getCode(tx.to) : '' const isERC721Token = code.includes(SAFE_TRANSFER_FROM_WITHOUT_DATA_HASH) || - (isTokenTransfer(tx.data, Number(tx.value)) && !code.includes(DECIMALS_METHOD_HASH)) + (isTokenTransfer(tx.data, Number(tx.value)) && !(await hasDecimalsMethod(tx.to))) const isSendTokenTx = !isERC721Token && isTokenTransfer(tx.data, Number(tx.value)) const isMultiSendTx = isMultisendTransaction(tx.data, Number(tx.value)) const isUpgradeTx = isMultiSendTx && isUpgradeTransaction(tx.data) @@ -349,45 +348,16 @@ export const loadSafeIncomingTransactions = async (safeAddress: string) => { return Map().set(safeAddress, List(incomingTxsRecord)) } -/** - * Returns nonce from the last tx returned by the server or defaults to 0 - * @param outgoingTxs - * @returns {number|*} - */ -const getLastTxNonce = (outgoingTxs) => { - if (!outgoingTxs) { - return 0 - } - - const mostRecentNonce = outgoingTxs.get(0).nonce - - // if nonce is null, then we are in front of the creation-tx - if (mostRecentNonce === null) { - const tx = outgoingTxs.get(1) - - if (tx) { - // if there's other tx than the creation one, we return its nonce - return tx.nonce - } else { - return 0 - } - } - - return mostRecentNonce -} - export default (safeAddress: string) => async (dispatch: ReduxDispatch) => { web3 = await getWeb3() const transactions: SafeTransactionsType | undefined = await loadSafeTransactions(safeAddress) if (transactions) { const { cancel, outgoing } = transactions - const nonce = getLastTxNonce(outgoing && outgoing.get(safeAddress)) batch(() => { dispatch(addCancellationTransactions(cancel)) dispatch(addTransactions(outgoing)) - dispatch(updateSafe({ address: safeAddress, nonce })) }) } diff --git a/src/routes/safe/store/reducer/safe.js b/src/routes/safe/store/reducer/safe.js index eb4ba4eabd..879d81dd76 100644 --- a/src/routes/safe/store/reducer/safe.js +++ b/src/routes/safe/store/reducer/safe.js @@ -48,14 +48,6 @@ export default handleActions( [UPDATE_SAFE]: (state: SafeReducerState, action: ActionType): SafeReducerState => { const safe = action.payload const safeAddress = safe.address - const isNonceUpdate = safe.nonce !== undefined - - if (isNonceUpdate && safe.nonce <= state.getIn([SAFE_REDUCER_ID, safeAddress, 'nonce'])) { - // update only when nonce is greater than the one already stored - // this will prevent undesired changes in the safe's state when - // txs pagination is implemented - return state - } return state.updateIn([SAFE_REDUCER_ID, safeAddress], (prevSafe) => prevSafe.merge(safe)) },