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

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": {

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,

src/routes/safe/container/index.jsx

@@ -121,14 +121,14 @@ class SafeView extends React.Component<Props, State> {
   checkForUpdates() {
     const {
       activeTokens,
-      checkAndUpdateSafeOwners,
+      checkAndUpdateSafe,
       fetchEtherBalance,
       fetchTokenBalances,
       fetchTransactions,
       safe,
       safeUrl,
     } = this.props
-    checkAndUpdateSafeOwners(safeUrl)
+    checkAndUpdateSafe(safeUrl)
     fetchTokenBalances(safeUrl, activeTokens)
     fetchEtherBalance(safe)
     fetchTransactions(safeUrl)

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

src/routes/safe/store/actions/fetchTransactions.js

@@ -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<Transaction> => {
-  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
@@ -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<GlobalState>) => {
   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 }))
     })
   }
 

src/routes/safe/store/reducer/safe.js

@@ -48,14 +48,6 @@ export default handleActions<SafeReducerState, *>(
     [UPDATE_SAFE]: (state: SafeReducerState, action: ActionType<Function>): 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))
     },