From 11d176fde69b3b40bbd6f8da5afaa826ad3dbc4d Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Thu, 17 Dec 2020 21:34:43 -0800 Subject: [PATCH] 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 | 22 +-- ui/app/store/actions.js | 231 +++++++++++++++++-------------- 2 files changed, 143 insertions(+), 110 deletions(-) diff --git a/test/unit/ui/app/actions.spec.js b/test/unit/ui/app/actions.spec.js index 09ae2acba..5e9ad3909 100644 --- a/test/unit/ui/app/actions.spec.js +++ b/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) => { diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index 474cdda67..1cbf2bcdc 100644 --- a/ui/app/store/actions.js +++ b/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(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()) } - 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(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(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(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(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(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(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()) - const token = global.eth.contract(abi).at(tokenAddress) - token.transfer(toAddress, addHexPrefix(amount), txData).catch((err) => { + + try { + const token = global.eth.contract(abi).at(tokenAddress) + const txPromise = token.transfer(toAddress, addHexPrefix(amount), txData) + dispatch(showConfTxPage()) dispatch(hideLoadingIndication()) - dispatch(displayWarning(err.message)) - }) - dispatch(showConfTxPage()) + 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) => { - background.updateTransaction(txData, (err) => { - dispatch(updateTransactionParams(txData.id, txData.txParams)) - if (err) { - dispatch(txError(err)) - dispatch(goHome()) - log.error(err.message) - reject(err) - return - } + try { + await promisifiedBackground.updateTransaction(txData) + } catch (error) { + dispatch(updateTransactionParams(txData.id, txData.txParams)) + dispatch(hideLoadingIndication()) + dispatch(txError(error)) + dispatch(goHome()) + log.error(error.message) + throw error + } - resolve(txData) - }) - }) - .then(() => updateMetamaskStateFromBackground()) - .then((newState) => dispatch(updateMetamaskState(newState))) - .then(() => { - dispatch(showConfTxPage({ id: txData.id })) - dispatch(hideLoadingIndication()) - return txData - }) + try { + dispatch(updateTransactionParams(txData.id, txData.txParams)) + const newState = await updateMetamaskStateFromBackground() + dispatch(updateMetamaskState(newState)) + dispatch(showConfTxPage({ id: txData.id })) + 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) => { - if (err) { - reject(err) + background.cancelTransaction(txData.id, (error) => { + if (error) { + reject(error) return } @@ -1050,6 +1071,10 @@ export function cancelTx(txData) { return txData }) + .catch((error) => { + dispatch(hideLoadingIndication()) + throw error + }) } } @@ -1061,34 +1086,38 @@ export function cancelTx(txData) { export function cancelTxs(txDataList) { return async (dispatch) => { dispatch(showLoadingIndication()) - const txIds = txDataList.map(({ id }) => id) - const cancellations = txIds.map( - (id) => - new Promise((resolve, reject) => { - background.cancelTransaction(id, (err) => { - if (err) { - reject(err) - return - } - - resolve() - }) - }), - ) - await Promise.all(cancellations) - const newState = await updateMetamaskStateFromBackground() - dispatch(updateMetamaskState(newState)) - dispatch(clearSend()) + try { + const txIds = txDataList.map(({ id }) => id) + const cancellations = txIds.map( + (id) => + new Promise((resolve, reject) => { + background.cancelTransaction(id, (err) => { + if (err) { + reject(err) + return + } + + resolve() + }) + }), + ) - txIds.forEach((id) => { - dispatch(completedTx(id)) - }) + await Promise.all(cancellations) - dispatch(hideLoadingIndication()) + const newState = await updateMetamaskStateFromBackground() + dispatch(updateMetamaskState(newState)) + dispatch(clearSend()) - if (getEnvironmentType() === ENVIRONMENT_TYPE_NOTIFICATION) { - global.platform.closeCurrentWindow() + txIds.forEach((id) => { + dispatch(completedTx(id)) + }) + } finally { + if (getEnvironmentType() === ENVIRONMENT_TYPE_NOTIFICATION) { + global.platform.closeCurrentWindow() + } else { + dispatch(hideLoadingIndication()) + } } } } @@ -1235,11 +1264,11 @@ export function setSelectedAddress(address) { try { await _setSelectedAddress(dispatch, address) } catch (error) { - dispatch(hideLoadingIndication()) dispatch(displayWarning(error.message)) return + } finally { + dispatch(hideLoadingIndication()) } - dispatch(hideLoadingIndication()) } } @@ -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(hideLoadingIndication()) + dispatch({ type: actionConstants.SHOW_ACCOUNT_DETAIL, value: address, @@ -2024,13 +2054,13 @@ export function setCompletedOnboarding() { try { await promisifiedBackground.completeOnboarding() + dispatch(completeOnboarding()) } catch (err) { dispatch(displayWarning(err.message)) throw err + } finally { + dispatch(hideLoadingIndication()) } - - dispatch(completeOnboarding()) - dispatch(hideLoadingIndication()) } } @@ -2179,20 +2209,19 @@ export function setIpfsGateway(val) { export function updateCurrentLocale(key) { return async (dispatch) => { dispatch(showLoadingIndication()) - await loadRelativeTimeFormatLocaleData(key) - return fetchLocale(key).then((localeMessages) => { - log.debug(`background.setCurrentLocale`) - background.setCurrentLocale(key, (err, textDirection) => { - if (err) { - dispatch(hideLoadingIndication()) - dispatch(displayWarning(err.message)) - return - } - switchDirection(textDirection) - dispatch(setCurrentLocale(key, localeMessages)) - dispatch(hideLoadingIndication()) - }) - }) + + try { + await loadRelativeTimeFormatLocaleData(key) + const localeMessages = await fetchLocale(key) + const textDirection = await promisifiedBackground.setCurrentLocale(key) + await switchDirection(textDirection) + dispatch(setCurrentLocale(key, localeMessages)) + } catch (error) { + dispatch(displayWarning(error.message)) + return + } finally { + dispatch(hideLoadingIndication()) + } } }