1
0
mirror of https://github.com/kremalicious/metamask-extension.git synced 2024-12-23 01:39:44 +01:00

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.
This commit is contained in:
Erik Marks 2020-12-17 21:34:43 -08:00 committed by GitHub
parent d2b6376c3d
commit 4b766fa538
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 142 additions and 109 deletions

View File

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

View File

@ -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()
})
}),
)
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
}
await Promise.all(cancellations)
const newState = await updateMetamaskStateFromBackground()
dispatch(updateMetamaskState(newState))
dispatch(clearSend())
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())
}
}
}