Tighten up loading indication logic (#10103)

Ensures that `hideLoadingIndication` is always called in all actions that call `showLoadingIndication`. It's unclear how many of these actions were failing to hide the loading indication, because other actions superset `hideLoadingIndication`. 

At the very least, `updateTransaction` was probably failing to hide the loading indication in the error case.

This PR also refactors a lot of actions to call `hideLoadingIndication` once in `finally` blocks as opposed to multiple times across `try` and `catch` blocks. We avoided making changes to functions using `Promise` methods, because `Promise.finally` is not supported by Waterfox, and it's not properly transpiled by Babel.

test/unit/ui/app/actions.spec.js

@@ -218,8 +218,8 @@ describe('Actions', function () {
 
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
-        { type: 'HIDE_LOADING_INDICATION' },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       submitPasswordSpy = sinon.stub(background, 'verifySeedPhrase')
@@ -368,8 +368,8 @@ describe('Actions', function () {
           type: 'SHOW_LOADING_INDICATION',
           value: 'This may take a while, please be patient.',
         },
-        { type: 'HIDE_LOADING_INDICATION' },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       importAccountWithStrategySpy = sinon.stub(
@@ -428,6 +428,7 @@ describe('Actions', function () {
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       try {
@@ -465,6 +466,7 @@ describe('Actions', function () {
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       try {
@@ -507,6 +509,7 @@ describe('Actions', function () {
           value: 'Looking for your Ledger...',
         },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       try {
@@ -545,6 +548,7 @@ describe('Actions', function () {
 
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
+        { type: 'HIDE_LOADING_INDICATION' },
         { type: 'DISPLAY_WARNING', value: 'error' },
       ]
 
@@ -581,8 +585,8 @@ describe('Actions', function () {
       const store = mockStore()
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
-        { type: 'HIDE_LOADING_INDICATION' },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       await store.dispatch(actions.setCurrentCurrency())
@@ -623,8 +627,8 @@ describe('Actions', function () {
       const store = mockStore()
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
-        { type: 'HIDE_LOADING_INDICATION' },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       signMessageSpy = sinon.stub(background, 'signMessage')
@@ -675,8 +679,8 @@ describe('Actions', function () {
       const store = mockStore()
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
-        { type: 'HIDE_LOADING_INDICATION' },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       signPersonalMessageSpy = sinon.stub(background, 'signPersonalMessage')
@@ -765,8 +769,8 @@ describe('Actions', function () {
       const store = mockStore()
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
-        { type: 'HIDE_LOADING_INDICATION' },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       try {
@@ -1005,8 +1009,8 @@ describe('Actions', function () {
       const store = mockStore()
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
-        { type: 'HIDE_LOADING_INDICATION' },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       await store.dispatch(actions.setSelectedAddress())
@@ -1044,8 +1048,8 @@ describe('Actions', function () {
       })
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
-        { type: 'HIDE_LOADING_INDICATION' },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
 
       await store.dispatch(actions.showAccountDetail())
@@ -1426,8 +1430,8 @@ describe('Actions', function () {
       const store = mockStore()
       const expectedActions = [
         { type: 'SHOW_LOADING_INDICATION', value: undefined },
-        { type: 'HIDE_LOADING_INDICATION' },
         { type: 'DISPLAY_WARNING', value: 'error' },
+        { type: 'HIDE_LOADING_INDICATION' },
       ]
       setCurrentLocaleSpy = sinon.stub(background, 'setCurrentLocale')
       setCurrentLocaleSpy.callsFake((_, callback) => {

ui/app/store/actions.js

@@ -121,12 +121,12 @@ export function createNewVaultAndGetSeedPhrase(password) {
     try {
       await createNewVault(password)
       const seedWords = await verifySeedPhrase()
-      dispatch(hideLoadingIndication())
       return seedWords
     } catch (error) {
-      dispatch(hideLoadingIndication())
       dispatch(displayWarning(error.message))
       throw new Error(error.message)
+    } finally {
+      dispatch(hideLoadingIndication())
     }
   }
 }
@@ -139,12 +139,12 @@ export function unlockAndGetSeedPhrase(password) {
       await submitPassword(password)
       const seedWords = await verifySeedPhrase()
       await forceUpdateMetamaskState(dispatch)
-      dispatch(hideLoadingIndication())
       return seedWords
     } catch (error) {
-      dispatch(hideLoadingIndication())
       dispatch(displayWarning(error.message))
       throw new Error(error.message)
+    } finally {
+      dispatch(hideLoadingIndication())
     }
   }
 }
@@ -209,12 +209,12 @@ export function requestRevealSeedWords(password) {
     try {
       await verifyPassword(password)
       const seedWords = await verifySeedPhrase()
-      dispatch(hideLoadingIndication())
       return seedWords
     } catch (error) {
-      dispatch(hideLoadingIndication())
       dispatch(displayWarning(error.message))
       throw new Error(error.message)
+    } finally {
+      dispatch(hideLoadingIndication())
     }
   }
 }
@@ -306,11 +306,12 @@ export function importNewAccount(strategy, args) {
       log.debug(`background.getState`)
       newState = await promisifiedBackground.getState()
     } catch (err) {
-      dispatch(hideLoadingIndication())
       dispatch(displayWarning(err.message))
       throw err
-    }
+    } finally {
       dispatch(hideLoadingIndication())
+    }
+
     dispatch(updateMetamaskState(newState))
     if (newState.selectedAddress) {
       dispatch({
@@ -335,11 +336,13 @@ export function addNewAccount() {
     } catch (error) {
       dispatch(displayWarning(error.message))
       throw error
+    } finally {
+      dispatch(hideLoadingIndication())
     }
+
     const newAccountAddress = Object.keys(newIdentities).find(
       (address) => !oldIdentities[address],
     )
-    dispatch(hideLoadingIndication())
     await forceUpdateMetamaskState(dispatch)
     return newAccountAddress
   }
@@ -360,9 +363,10 @@ export function checkHardwareStatus(deviceName, hdPath) {
       log.error(error)
       dispatch(displayWarning(error.message))
       throw error
+    } finally {
+      dispatch(hideLoadingIndication())
     }
 
-    dispatch(hideLoadingIndication())
     await forceUpdateMetamaskState(dispatch)
     return unlocked
   }
@@ -378,9 +382,10 @@ export function forgetDevice(deviceName) {
       log.error(error)
       dispatch(displayWarning(error.message))
       throw error
+    } finally {
+      dispatch(hideLoadingIndication())
     }
 
-    dispatch(hideLoadingIndication())
     await forceUpdateMetamaskState(dispatch)
   }
 }
@@ -403,10 +408,11 @@ export function connectHardware(deviceName, page, hdPath) {
       log.error(error)
       dispatch(displayWarning(error.message))
       throw error
-    }
+    } finally {
       dispatch(hideLoadingIndication())
-    await forceUpdateMetamaskState(dispatch)
+    }
 
+    await forceUpdateMetamaskState(dispatch)
     return accounts
   }
 }
@@ -421,6 +427,7 @@ export function unlockHardwareWalletAccount(index, deviceName, hdPath) {
         deviceName,
         hdPath,
         (err) => {
+          dispatch(hideLoadingIndication())
           if (err) {
             log.error(err)
             dispatch(displayWarning(err.message))
@@ -428,7 +435,6 @@ export function unlockHardwareWalletAccount(index, deviceName, hdPath) {
             return
           }
 
-          dispatch(hideLoadingIndication())
           resolve()
         },
       )
@@ -454,12 +460,13 @@ export function setCurrentCurrency(currencyCode) {
     try {
       data = await promisifiedBackground.setCurrentCurrency(currencyCode)
     } catch (error) {
-      dispatch(hideLoadingIndication())
       log.error(error.stack)
       dispatch(displayWarning(error.message))
       return
-    }
+    } finally {
       dispatch(hideLoadingIndication())
+    }
+
     dispatch({
       type: actionConstants.SET_CURRENT_FIAT,
       value: {
@@ -480,12 +487,13 @@ export function signMsg(msgData) {
     try {
       newState = await promisifiedBackground.signMessage(msgData)
     } catch (error) {
-      dispatch(hideLoadingIndication())
       log.error(error)
       dispatch(displayWarning(error.message))
       throw error
-    }
+    } finally {
       dispatch(hideLoadingIndication())
+    }
+
     dispatch(updateMetamaskState(newState))
     dispatch(completedTx(msgData.metamaskId))
     dispatch(closeCurrentNotificationWindow())
@@ -503,12 +511,13 @@ export function signPersonalMsg(msgData) {
     try {
       newState = await promisifiedBackground.signPersonalMessage(msgData)
     } catch (error) {
-      dispatch(hideLoadingIndication())
       log.error(error)
       dispatch(displayWarning(error.message))
       throw error
-    }
+    } finally {
       dispatch(hideLoadingIndication())
+    }
+
     dispatch(updateMetamaskState(newState))
     dispatch(completedTx(msgData.metamaskId))
     dispatch(closeCurrentNotificationWindow())
@@ -547,12 +556,13 @@ export function decryptMsg(decryptedMsgData) {
     try {
       newState = await promisifiedBackground.decryptMessage(decryptedMsgData)
     } catch (error) {
-      dispatch(hideLoadingIndication())
       log.error(error)
       dispatch(displayWarning(error.message))
       throw error
-    }
+    } finally {
       dispatch(hideLoadingIndication())
+    }
+
     dispatch(updateMetamaskState(newState))
     dispatch(completedTx(decryptedMsgData.metamaskId))
     dispatch(closeCurrentNotificationWindow())
@@ -570,12 +580,13 @@ export function encryptionPublicKeyMsg(msgData) {
     try {
       newState = await promisifiedBackground.encryptionPublicKey(msgData)
     } catch (error) {
-      dispatch(hideLoadingIndication())
       log.error(error)
       dispatch(displayWarning(error.message))
       throw error
-    }
+    } finally {
       dispatch(hideLoadingIndication())
+    }
+
     dispatch(updateMetamaskState(newState))
     dispatch(completedTx(msgData.metamaskId))
     dispatch(closeCurrentNotificationWindow())
@@ -593,12 +604,13 @@ export function signTypedMsg(msgData) {
     try {
       newState = await promisifiedBackground.signTypedMessage(msgData)
     } catch (error) {
-      dispatch(hideLoadingIndication())
       log.error(error)
       dispatch(displayWarning(error.message))
       throw error
-    }
+    } finally {
       dispatch(hideLoadingIndication())
+    }
+
     dispatch(updateMetamaskState(newState))
     dispatch(completedTx(msgData.metamaskId))
     dispatch(closeCurrentNotificationWindow())
@@ -788,14 +800,19 @@ export function updateSendEnsResolutionError(errorMessage) {
 }
 
 export function signTokenTx(tokenAddress, toAddress, amount, txData) {
-  return (dispatch) => {
+  return async (dispatch) => {
     dispatch(showLoadingIndication())
+
+    try {
       const token = global.eth.contract(abi).at(tokenAddress)
-    token.transfer(toAddress, addHexPrefix(amount), txData).catch((err) => {
+      const txPromise = token.transfer(toAddress, addHexPrefix(amount), txData)
-      dispatch(hideLoadingIndication())
-      dispatch(displayWarning(err.message))
-    })
       dispatch(showConfTxPage())
+      dispatch(hideLoadingIndication())
+      await txPromise
+    } catch (error) {
+      dispatch(hideLoadingIndication())
+      dispatch(displayWarning(error.message))
+    }
   }
 }
 
@@ -815,30 +832,29 @@ const updateMetamaskStateFromBackground = () => {
 }
 
 export function updateTransaction(txData, dontShowLoadingIndicator) {
-  return (dispatch) => {
+  return async (dispatch) => {
     !dontShowLoadingIndicator && dispatch(showLoadingIndication())
 
-    return new Promise((resolve, reject) => {
+    try {
-      background.updateTransaction(txData, (err) => {
+      await promisifiedBackground.updateTransaction(txData)
+    } catch (error) {
       dispatch(updateTransactionParams(txData.id, txData.txParams))
-        if (err) {
+      dispatch(hideLoadingIndication())
-          dispatch(txError(err))
+      dispatch(txError(error))
       dispatch(goHome())
-          log.error(err.message)
+      log.error(error.message)
-          reject(err)
+      throw error
-          return
     }
 
-        resolve(txData)
+    try {
-      })
+      dispatch(updateTransactionParams(txData.id, txData.txParams))
-    })
+      const newState = await updateMetamaskStateFromBackground()
-      .then(() => updateMetamaskStateFromBackground())
+      dispatch(updateMetamaskState(newState))
-      .then((newState) => dispatch(updateMetamaskState(newState)))
-      .then(() => {
       dispatch(showConfTxPage({ id: txData.id }))
-        dispatch(hideLoadingIndication())
       return txData
-      })
+    } finally {
+      dispatch(hideLoadingIndication())
+    }
   }
 }
 
@@ -950,6 +966,7 @@ export function cancelMsg(msgData) {
     } finally {
       dispatch(hideLoadingIndication())
     }
+
     dispatch(updateMetamaskState(newState))
     dispatch(completedTx(msgData.id))
     dispatch(closeCurrentNotificationWindow())
@@ -967,6 +984,7 @@ export function cancelPersonalMsg(msgData) {
     } finally {
       dispatch(hideLoadingIndication())
     }
+
     dispatch(updateMetamaskState(newState))
     dispatch(completedTx(msgData.id))
     dispatch(closeCurrentNotificationWindow())
@@ -984,6 +1002,7 @@ export function cancelDecryptMsg(msgData) {
     } finally {
       dispatch(hideLoadingIndication())
     }
+
     dispatch(updateMetamaskState(newState))
     dispatch(completedTx(msgData.id))
     dispatch(closeCurrentNotificationWindow())
@@ -1003,6 +1022,7 @@ export function cancelEncryptionPublicKeyMsg(msgData) {
     } finally {
       dispatch(hideLoadingIndication())
     }
+
     dispatch(updateMetamaskState(newState))
     dispatch(completedTx(msgData.id))
     dispatch(closeCurrentNotificationWindow())
@@ -1020,6 +1040,7 @@ export function cancelTypedMsg(msgData) {
     } finally {
       dispatch(hideLoadingIndication())
     }
+
     dispatch(updateMetamaskState(newState))
     dispatch(completedTx(msgData.id))
     dispatch(closeCurrentNotificationWindow())
@@ -1031,9 +1052,9 @@ export function cancelTx(txData) {
   return (dispatch) => {
     dispatch(showLoadingIndication())
     return new Promise((resolve, reject) => {
-      background.cancelTransaction(txData.id, (err) => {
+      background.cancelTransaction(txData.id, (error) => {
-        if (err) {
+        if (error) {
-          reject(err)
+          reject(error)
           return
         }
 
@@ -1050,6 +1071,10 @@ export function cancelTx(txData) {
 
         return txData
       })
+      .catch((error) => {
+        dispatch(hideLoadingIndication())
+        throw error
+      })
   }
 }
 
@@ -1061,6 +1086,8 @@ export function cancelTx(txData) {
 export function cancelTxs(txDataList) {
   return async (dispatch) => {
     dispatch(showLoadingIndication())
+
+    try {
       const txIds = txDataList.map(({ id }) => id)
       const cancellations = txIds.map(
         (id) =>
@@ -1077,6 +1104,7 @@ export function cancelTxs(txDataList) {
       )
 
       await Promise.all(cancellations)
+
       const newState = await updateMetamaskStateFromBackground()
       dispatch(updateMetamaskState(newState))
       dispatch(clearSend())
@@ -1084,11 +1112,12 @@ export function cancelTxs(txDataList) {
       txIds.forEach((id) => {
         dispatch(completedTx(id))
       })
-
+    } finally {
-    dispatch(hideLoadingIndication())
-
       if (getEnvironmentType() === ENVIRONMENT_TYPE_NOTIFICATION) {
         global.platform.closeCurrentWindow()
+      } else {
+        dispatch(hideLoadingIndication())
+      }
     }
   }
 }
@@ -1235,13 +1264,13 @@ export function setSelectedAddress(address) {
     try {
       await _setSelectedAddress(dispatch, address)
     } catch (error) {
-      dispatch(hideLoadingIndication())
       dispatch(displayWarning(error.message))
       return
-    }
+    } finally {
       dispatch(hideLoadingIndication())
     }
   }
+}
 
 export function showAccountDetail(address) {
   return async (dispatch, getState) => {
@@ -1270,11 +1299,12 @@ export function showAccountDetail(address) {
     try {
       await _setSelectedAddress(dispatch, address)
     } catch (error) {
-      dispatch(hideLoadingIndication())
       dispatch(displayWarning(error.message))
       return
-    }
+    } finally {
       dispatch(hideLoadingIndication())
+    }
+
     dispatch({
       type: actionConstants.SHOW_ACCOUNT_DETAIL,
       value: address,
@@ -2024,15 +2054,15 @@ export function setCompletedOnboarding() {
 
     try {
       await promisifiedBackground.completeOnboarding()
+      dispatch(completeOnboarding())
     } catch (err) {
       dispatch(displayWarning(err.message))
       throw err
-    }
+    } finally {
-
-    dispatch(completeOnboarding())
       dispatch(hideLoadingIndication())
     }
   }
+}
 
 export function completeOnboarding() {
   return {
@@ -2179,20 +2209,19 @@ export function setIpfsGateway(val) {
 export function updateCurrentLocale(key) {
   return async (dispatch) => {
     dispatch(showLoadingIndication())
+
+    try {
       await loadRelativeTimeFormatLocaleData(key)
-    return fetchLocale(key).then((localeMessages) => {
+      const localeMessages = await fetchLocale(key)
-      log.debug(`background.setCurrentLocale`)
+      const textDirection = await promisifiedBackground.setCurrentLocale(key)
-      background.setCurrentLocale(key, (err, textDirection) => {
+      await switchDirection(textDirection)
-        if (err) {
-          dispatch(hideLoadingIndication())
-          dispatch(displayWarning(err.message))
-          return
-        }
-        switchDirection(textDirection)
       dispatch(setCurrentLocale(key, localeMessages))
+    } catch (error) {
+      dispatch(displayWarning(error.message))
+      return
+    } finally {
       dispatch(hideLoadingIndication())
-      })
+    }
-    })
   }
 }