From 2359062b62cf65f38b36ccb6bb33fa7d15ada1ae Mon Sep 17 00:00:00 2001 From: Dan Miller Date: Mon, 23 Jul 2018 22:20:06 -0230 Subject: [PATCH 1/5] UI confirm screen closes confirmation window on submit or cancel of a tx --- app/scripts/platforms/extension.js | 6 ++++++ app/scripts/ui.js | 8 ++++---- .../confirm-transaction-base.component.js | 19 +++++++++++++++---- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/app/scripts/platforms/extension.js b/app/scripts/platforms/extension.js index 901c26cab..bd4f7bd9f 100644 --- a/app/scripts/platforms/extension.js +++ b/app/scripts/platforms/extension.js @@ -14,6 +14,12 @@ class ExtensionPlatform { extension.tabs.create({ url }) } + closeCurrentWindow (cb) { + return extension.windows.getCurrent((windowDetails) => { + return extension.windows.remove(windowDetails.id) + }) + } + getVersion () { return extension.runtime.getManifest().version } diff --git a/app/scripts/ui.js b/app/scripts/ui.js index 9bf97be87..42703f029 100644 --- a/app/scripts/ui.js +++ b/app/scripts/ui.js @@ -64,16 +64,16 @@ async function start () { css = betaUIState ? NewMetaMaskUiCss() : OldMetaMaskUiCss() deleteInjectedCss = injectCss(css) } - if (state.appState.shouldClose) notificationManager.closePopup() + // if (state.appState.shouldClose) notificationManager.closePopup() }) }) function closePopupIfOpen (windowType) { - if (windowType !== ENVIRONMENT_TYPE_NOTIFICATION) { + // if (windowType !== ENVIRONMENT_TYPE_NOTIFICATION) { // should close only chrome popup - notificationManager.closePopup() - } + // notificationManager.closePopup() + // } } function displayCriticalError (err) { diff --git a/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js b/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js index e1bf2210f..2811e6157 100644 --- a/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js @@ -8,6 +8,9 @@ import { INSUFFICIENT_FUNDS_ERROR_KEY, TRANSACTION_ERROR_KEY, } from '../../../constants/error-keys' +import { + ENVIRONMENT_TYPE_NOTIFICATION, +} from '../../../../../app/scripts/lib/enums' export default class ConfirmTransactionBase extends Component { static contextTypes = { @@ -250,8 +253,12 @@ export default class ConfirmTransactionBase extends Component { } else { cancelTransaction(txData) .then(() => { - clearConfirmTransaction() - history.push(DEFAULT_ROUTE) + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION) { + return global.platform.closeCurrentWindow() + } else { + clearConfirmTransaction() + history.push(DEFAULT_ROUTE) + } }) } } @@ -264,8 +271,12 @@ export default class ConfirmTransactionBase extends Component { } else { sendTransaction(txData) .then(() => { - clearConfirmTransaction() - history.push(DEFAULT_ROUTE) + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION) { + return global.platform.closeCurrentWindow() + } else { + clearConfirmTransaction() + history.push(DEFAULT_ROUTE) + } }) } } From 152246f3b0862c8c5a8b42872852d236400c0e9d Mon Sep 17 00:00:00 2001 From: Alexander Tseung Date: Mon, 23 Jul 2018 20:25:04 -0700 Subject: [PATCH 2/5] Add close window support to signature requests. Move logic to actions --- ui/app/actions.js | 73 ++++++++++++++++--- .../confirm-transaction-base.component.js | 19 +---- ui/app/components/pages/home.js | 12 +-- ui/app/helpers/confirm-transaction/util.js | 6 ++ ui/app/selectors/confirm-transaction.js | 29 +++++++- 5 files changed, 108 insertions(+), 31 deletions(-) diff --git a/ui/app/actions.js b/ui/app/actions.js index 6c947fc35..7a8d9667d 100644 --- a/ui/app/actions.js +++ b/ui/app/actions.js @@ -10,6 +10,8 @@ const { const ethUtil = require('ethereumjs-util') const { fetchLocale } = require('../i18n-helper') const log = require('loglevel') +const { ENVIRONMENT_TYPE_NOTIFICATION } = require('../../app/scripts/lib/enums') +const { hasUnconfirmedTransactions } = require('./helpers/confirm-transaction/util') var actions = { _setBackgroundConnection: _setBackgroundConnection, @@ -743,7 +745,7 @@ function setCurrentCurrency (currencyCode) { function signMsg (msgData) { log.debug('action - signMsg') - return (dispatch) => { + return (dispatch, getState) => { dispatch(actions.showLoadingIndication()) return new Promise((resolve, reject) => { @@ -760,6 +762,12 @@ function signMsg (msgData) { } dispatch(actions.completedTx(msgData.metamaskId)) + + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION && + !hasUnconfirmedTransactions(getState())) { + return global.platform.closeCurrentWindow() + } + return resolve(msgData) }) }) @@ -768,7 +776,7 @@ function signMsg (msgData) { function signPersonalMsg (msgData) { log.debug('action - signPersonalMsg') - return dispatch => { + return (dispatch, getState) => { dispatch(actions.showLoadingIndication()) return new Promise((resolve, reject) => { @@ -785,6 +793,12 @@ function signPersonalMsg (msgData) { } dispatch(actions.completedTx(msgData.metamaskId)) + + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION && + !hasUnconfirmedTransactions(getState())) { + return global.platform.closeCurrentWindow() + } + return resolve(msgData) }) }) @@ -793,7 +807,7 @@ function signPersonalMsg (msgData) { function signTypedMsg (msgData) { log.debug('action - signTypedMsg') - return (dispatch) => { + return (dispatch, getState) => { dispatch(actions.showLoadingIndication()) return new Promise((resolve, reject) => { @@ -810,6 +824,12 @@ function signTypedMsg (msgData) { } dispatch(actions.completedTx(msgData.metamaskId)) + + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION && + !hasUnconfirmedTransactions(getState())) { + return global.platform.closeCurrentWindow() + } + return resolve(msgData) }) }) @@ -1003,7 +1023,7 @@ function clearSend () { function sendTx (txData) { log.info(`actions - sendTx: ${JSON.stringify(txData.txParams)}`) - return (dispatch) => { + return (dispatch, getState) => { log.debug(`actions calling background.approveTransaction`) background.approveTransaction(txData.id, (err) => { if (err) { @@ -1011,6 +1031,11 @@ function sendTx (txData) { return log.error(err.message) } dispatch(actions.completedTx(txData.id)) + + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION && + !hasUnconfirmedTransactions(getState())) { + return global.platform.closeCurrentWindow() + } }) } } @@ -1059,7 +1084,7 @@ function updateTransaction (txData) { function updateAndApproveTx (txData) { log.info('actions: updateAndApproveTx: ' + JSON.stringify(txData)) - return dispatch => { + return (dispatch, getState) => { log.debug(`actions calling background.updateAndApproveTx`) dispatch(actions.showLoadingIndication()) @@ -1084,6 +1109,12 @@ function updateAndApproveTx (txData) { dispatch(actions.clearSend()) dispatch(actions.completedTx(txData.id)) dispatch(actions.hideLoadingIndication()) + + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION && + !hasUnconfirmedTransactions(getState())) { + return global.platform.closeCurrentWindow() + } + return txData }) } @@ -1112,7 +1143,7 @@ function txError (err) { } function cancelMsg (msgData) { - return dispatch => { + return (dispatch, getState) => { dispatch(actions.showLoadingIndication()) return new Promise((resolve, reject) => { @@ -1126,6 +1157,12 @@ function cancelMsg (msgData) { } dispatch(actions.completedTx(msgData.id)) + + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION && + !hasUnconfirmedTransactions(getState())) { + return global.platform.closeCurrentWindow() + } + return resolve(msgData) }) }) @@ -1133,7 +1170,7 @@ function cancelMsg (msgData) { } function cancelPersonalMsg (msgData) { - return dispatch => { + return (dispatch, getState) => { dispatch(actions.showLoadingIndication()) return new Promise((resolve, reject) => { @@ -1147,6 +1184,12 @@ function cancelPersonalMsg (msgData) { } dispatch(actions.completedTx(id)) + + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION && + !hasUnconfirmedTransactions(getState())) { + return global.platform.closeCurrentWindow() + } + return resolve(msgData) }) }) @@ -1154,7 +1197,7 @@ function cancelPersonalMsg (msgData) { } function cancelTypedMsg (msgData) { - return dispatch => { + return (dispatch, getState) => { dispatch(actions.showLoadingIndication()) return new Promise((resolve, reject) => { @@ -1168,6 +1211,12 @@ function cancelTypedMsg (msgData) { } dispatch(actions.completedTx(id)) + + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION && + !hasUnconfirmedTransactions(getState())) { + return global.platform.closeCurrentWindow() + } + return resolve(msgData) }) }) @@ -1175,7 +1224,7 @@ function cancelTypedMsg (msgData) { } function cancelTx (txData) { - return dispatch => { + return (dispatch, getState) => { log.debug(`background.cancelTransaction`) dispatch(actions.showLoadingIndication()) @@ -1194,6 +1243,12 @@ function cancelTx (txData) { dispatch(actions.clearSend()) dispatch(actions.completedTx(txData.id)) dispatch(actions.hideLoadingIndication()) + + if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION && + !hasUnconfirmedTransactions(getState())) { + return global.platform.closeCurrentWindow() + } + return txData }) } diff --git a/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js b/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js index 2811e6157..e1bf2210f 100644 --- a/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js @@ -8,9 +8,6 @@ import { INSUFFICIENT_FUNDS_ERROR_KEY, TRANSACTION_ERROR_KEY, } from '../../../constants/error-keys' -import { - ENVIRONMENT_TYPE_NOTIFICATION, -} from '../../../../../app/scripts/lib/enums' export default class ConfirmTransactionBase extends Component { static contextTypes = { @@ -253,12 +250,8 @@ export default class ConfirmTransactionBase extends Component { } else { cancelTransaction(txData) .then(() => { - if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION) { - return global.platform.closeCurrentWindow() - } else { - clearConfirmTransaction() - history.push(DEFAULT_ROUTE) - } + clearConfirmTransaction() + history.push(DEFAULT_ROUTE) }) } } @@ -271,12 +264,8 @@ export default class ConfirmTransactionBase extends Component { } else { sendTransaction(txData) .then(() => { - if (global.METAMASK_UI_TYPE === ENVIRONMENT_TYPE_NOTIFICATION) { - return global.platform.closeCurrentWindow() - } else { - clearConfirmTransaction() - history.push(DEFAULT_ROUTE) - } + clearConfirmTransaction() + history.push(DEFAULT_ROUTE) }) } } diff --git a/ui/app/components/pages/home.js b/ui/app/components/pages/home.js index 38aa02dae..5e3fdc9af 100644 --- a/ui/app/components/pages/home.js +++ b/ui/app/components/pages/home.js @@ -27,19 +27,17 @@ const { NOTICE_ROUTE, } = require('../../routes') +const { unconfirmedTransactionsCountSelector } = require('../../selectors/confirm-transaction') + class Home extends Component { componentDidMount () { const { history, - unapprovedTxs = {}, - unapprovedMsgCount = 0, - unapprovedPersonalMsgCount = 0, - unapprovedTypedMessagesCount = 0, + unconfirmedTransactionsCount = 0, } = this.props // unapprovedTxs and unapproved messages - if (Object.keys(unapprovedTxs).length || - unapprovedTypedMessagesCount + unapprovedMsgCount + unapprovedPersonalMsgCount > 0) { + if (unconfirmedTransactionsCount > 0) { history.push(CONFIRM_TRANSACTION_ROUTE) } } @@ -167,6 +165,7 @@ Home.propTypes = { isPopup: PropTypes.bool, isMouseUser: PropTypes.bool, t: PropTypes.func, + unconfirmedTransactionsCount: PropTypes.number, } function mapStateToProps (state) { @@ -230,6 +229,7 @@ function mapStateToProps (state) { // state needed to get account dropdown temporarily rendering from app bar selected, + unconfirmedTransactionsCount: unconfirmedTransactionsCountSelector(state), } } diff --git a/ui/app/helpers/confirm-transaction/util.js b/ui/app/helpers/confirm-transaction/util.js index 1373d28df..f015b2bf5 100644 --- a/ui/app/helpers/confirm-transaction/util.js +++ b/ui/app/helpers/confirm-transaction/util.js @@ -16,6 +16,8 @@ import { conversionGreaterThan, } from '../../conversion-util' +import { unconfirmedTransactionsCountSelector } from '../../selectors/confirm-transaction' + export function getTokenData (data = {}) { return abiDecoder.decodeMethod(data) } @@ -131,3 +133,7 @@ export function convertTokenToFiat ({ conversionRate: totalExchangeRate, }) } + +export function hasUnconfirmedTransactions (state) { + return unconfirmedTransactionsCountSelector(state) > 0 +} diff --git a/ui/app/selectors/confirm-transaction.js b/ui/app/selectors/confirm-transaction.js index 54016a30e..8f8e0ea74 100644 --- a/ui/app/selectors/confirm-transaction.js +++ b/ui/app/selectors/confirm-transaction.js @@ -62,6 +62,34 @@ export const unconfirmedTransactionsHashSelector = createSelector( } ) +const unapprovedMsgCountSelector = state => state.metamask.unapprovedMsgCount +const unapprovedPersonalMsgCountSelector = state => state.metamask.unapprovedPersonalMsgCount +const unapprovedTypedMessagesCountSelector = state => state.metamask.unapprovedTypedMessagesCount + +export const unconfirmedTransactionsCountSelector = createSelector( + unapprovedTxsSelector, + unapprovedMsgCountSelector, + unapprovedPersonalMsgCountSelector, + unapprovedTypedMessagesCountSelector, + networkSelector, + ( + unapprovedTxs = {}, + unapprovedMsgCount = 0, + unapprovedPersonalMsgCount = 0, + unapprovedTypedMessagesCount = 0, + network + ) => { + const filteredUnapprovedTxIds = Object.keys(unapprovedTxs).filter(txId => { + const { metamaskNetworkId } = unapprovedTxs[txId] + return metamaskNetworkId === network + }) + + return filteredUnapprovedTxIds.length + unapprovedTypedMessagesCount + unapprovedMsgCount + + unapprovedPersonalMsgCount + } +) + + export const currentCurrencySelector = state => state.metamask.currentCurrency export const conversionRateSelector = state => state.metamask.conversionRate @@ -156,7 +184,6 @@ export const sendTokenTokenAmountAndToAddressSelector = createSelector( } ) - export const contractExchangeRateSelector = createSelector( contractExchangeRatesSelector, tokenAddressSelector, From cb8ea12db626e82b7230317fc74b6b80ce20f793 Mon Sep 17 00:00:00 2001 From: Dan Miller Date: Tue, 24 Jul 2018 11:24:36 -0230 Subject: [PATCH 3/5] Updates e2e beta tests to ensure that popup window closes after confirming a simple send. --- test/e2e/beta/metamask-beta-ui.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/beta/metamask-beta-ui.spec.js b/test/e2e/beta/metamask-beta-ui.spec.js index ee1aa0ff1..ca1977c5a 100644 --- a/test/e2e/beta/metamask-beta-ui.spec.js +++ b/test/e2e/beta/metamask-beta-ui.spec.js @@ -472,7 +472,7 @@ describe('MetaMask', function () { await confirmButton.click() await delay(regularDelayMs) - await closeAllWindowHandlesExcept(driver, [extension, dapp]) + await waitUntilXWindowHandles(driver, 2) await driver.switchTo().window(extension) await delay(regularDelayMs) }) From 64a82fd3dae66f33b0934d58e43e85df27fbfe1d Mon Sep 17 00:00:00 2001 From: Dan Miller Date: Tue, 24 Jul 2018 11:45:36 -0230 Subject: [PATCH 4/5] Uncomment closePopupIfOpen code. --- app/scripts/ui.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/scripts/ui.js b/app/scripts/ui.js index 42703f029..cbe15c92d 100644 --- a/app/scripts/ui.js +++ b/app/scripts/ui.js @@ -70,10 +70,10 @@ async function start () { function closePopupIfOpen (windowType) { - // if (windowType !== ENVIRONMENT_TYPE_NOTIFICATION) { + if (windowType !== ENVIRONMENT_TYPE_NOTIFICATION) { // should close only chrome popup - // notificationManager.closePopup() - // } + notificationManager.closePopup() + } } function displayCriticalError (err) { From a61227f224e37559c8d6e2c59441b0032633feaf Mon Sep 17 00:00:00 2001 From: Dan Miller Date: Tue, 24 Jul 2018 15:01:03 -0230 Subject: [PATCH 5/5] Remove unneeded code from i4829-close-notifications-from-ui branch. --- app/scripts/platforms/extension.js | 2 +- app/scripts/ui.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/scripts/platforms/extension.js b/app/scripts/platforms/extension.js index bd4f7bd9f..0803164e8 100644 --- a/app/scripts/platforms/extension.js +++ b/app/scripts/platforms/extension.js @@ -14,7 +14,7 @@ class ExtensionPlatform { extension.tabs.create({ url }) } - closeCurrentWindow (cb) { + closeCurrentWindow () { return extension.windows.getCurrent((windowDetails) => { return extension.windows.remove(windowDetails.id) }) diff --git a/app/scripts/ui.js b/app/scripts/ui.js index cbe15c92d..da100f928 100644 --- a/app/scripts/ui.js +++ b/app/scripts/ui.js @@ -64,7 +64,6 @@ async function start () { css = betaUIState ? NewMetaMaskUiCss() : OldMetaMaskUiCss() deleteInjectedCss = injectCss(css) } - // if (state.appState.shouldClose) notificationManager.closePopup() }) })