Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cancel transaction on close of notification window #6340

Merged
merged 1 commit into from
Mar 25, 2019
Merged

Cancel transaction on close of notification window #6340

merged 1 commit into from
Mar 25, 2019

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Mar 25, 2019

fixes #3124

transactions:
Peek 2019-03-25 02-00

signature requests:
Peek 2019-03-25 02-32

@danjm danjm requested a review from whymarrh as a code owner March 25, 2019 05:11
@danjm danjm force-pushed the reject-tx-notification-close branch from 486566d to c9803ff Compare March 25, 2019 06:17
@metamaskbot
Copy link
Collaborator

Builds ready [c9803ff]: chrome, firefox, edge, opera

tmashuang
tmashuang approved these changes Mar 25, 2019
View reviewed changes
Copy link
Contributor

@tmashuang tmashuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@danjm
Copy link
Contributor Author

danjm commented Mar 25, 2019

This is good to merge unless @whymarrh does not like what I did in https://github.com/MetaMask/metamask-extension/pull/6340/files#diff-eebb93d0666faf7aa39f85d9f7efa804 :)

whymarrh
whymarrh approved these changes Mar 25, 2019
View reviewed changes
Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we can clean up the onbeforeunload afterwards

@danjm
Copy link
Contributor Author

danjm commented Mar 25, 2019 •
edited
Loading

Whymarrh and I agree that there is a more architecturally sound way to do this:

  • e.g. expose a function from the notification controller that can call a passed function on close
  • then expose that method to the front end via the metamask controller

The present solution works and is easily changed, so we are going to merge for now.

@danjm danjm merged commit 69f7968 into develop Mar 25, 2019
@danjm danjm deleted the reject-tx-notification-close branch March 26, 2019 14:15
danfinlay pushed a commit that referenced this pull request Mar 26, 2019
@tmashuang @danfinlay
Update master branch (#6296)
b921c8f 
* Fixes the 'Percentages row tracks and gutters' decpracation warning. (#6244)

* Fix missing this reference in addtoken button onclick method (#6245)

* Only call onRecipient and onSender methods if defined in sender-to-recipient (#6247)

* Prevent advanced gas input arrows from setting value to < 0 (#6248)

* Add privacy policy link to modal metrics opt-in (#6250)

* Handle undefined gas limits and prices in transaction-breakdown.component (#6246)

* Uppercase and context fixes on Spanish translation.

* development - enhancement for sourcemap validator tool (#6277)

* Nonmultiple notifications for batch txs

* No longer check network when validating checksum addresses

* Version 6.2.1 (#6251)

* Remove line rather than comment out

* Fixes the use of the browser back button on the reveal seed screen

* GABA: Integrate AddressBookController (#5847)

* gaba: integrate AddressBookController

* pin gaba version and update lockfile

* ci: Use cache version from circle environment var (#6286)

* Allow npm to update the package-lock.json file

* npm i -D ganache-core@2.5.3

* mascara - remove from project (#6283)

* circleci - disable npm dep cache (#6288)

* Centre all notification popups

* ui - add missing PropTypes (#6287)

* Improve Korean translations (#6268)

* ci: Use npm ci for fast(er) installs

* build - babel - move config to babelrc (#6284)

* Patch/bump version (#6294)

* Update Node minor version

* ci: Skip updating npm@6 as it is default

* Allow seed phrases with a trailing newline

* Centre the notification in the current window (#6307)

* Fixes popups not showing when screen size is odd (#6312)

* Fix typos in English messages (#6317)

* Add rollback script, move auto-changelog script (#6252)

* Bump gaba version to avoid broken eth-contract-metadata

* Fixing spelling of Ethereum in MetaMetrics copy (#6329)

* Stop reloading dapps on network change allowing dapps to decide if it should refresh or not (#6330)

* feat: `inpageProvider.autoRefreshOnNetworkChange` to allow dapps to control if it refreshes or not

* feat: check the `autoRefreshOnNetworkChange` before a refresh

* fix linting error

* fix: use `window.ethereum` now `web3.ethereum`

* Enable mobile sync (#6332)

* enable mobile sync

* remove mobile sync as a preference

* Fix typo

* Folder restructure (#6304)

* Remove ui/app/keychains/

* Remove ui/app/img/ (unused images)

* Move conversion-util to helpers/utils/

* Move token-util to helpers/utils/

* Move /helpers/*.js inside /helpers/utils/

* Move util tests inside /helpers/utils/

* Renameand move confirm-transaction/util.js to helpers/utils/

* Move higher-order-components to helpers/higher-order-components/

* Move infura-conversion.json to helpers/constants/

* Move all utility functions to helpers/utils/

* Move pages directory to top-level

* Move all constants to helpers/constants/

* Move metametrics inside helpers/

* Move app and root inside pages/

* Move routes inside helpers/

* Re-organize ducks/

* Move reducers to ducks/

* Move selectors inside selectors/

* Move test out of test folder

* Move action, reducer, store inside store/

* Move ui components inside ui/

* Move UI components inside ui/

* Move connected components inside components/app/

* Move i18n-helper inside helpers/

* Fix unit tests

* Fix unit test

* Move pages components

* Rename routes component

* Move reducers to ducks/index

* Fix bad path in unit test

* Hide gas price chart and prevent api call when not on ethereum networks. (#6300)

Add missing translations in gas customization modal

* Fix gas fee in the submitted step of the transaction details activity log. (#6301)

* Fix oversized loading overlay on gas customization modal. (#6326)

* Replaces the coinbase link in the deposit modal with one for wyre (#6302)

* New settings page rebased (#6333)

* New setting tab

* Add InfoTab

* Add Advanced tab

* Add Security Tab

* Finish mobile view

* Make new setting page responsive

* Fix linter

* Fix y scrolling

* Update link in network dropdown

* Fix e2e tests

* Remove duplicate translation key

* Resolve merge conflict

* Only change settings header in popup view.

* Place mobile-sync button in advanced-tab of settings

* Close transaction on close of notification window (#6340)

* Cancel error rebased (#6341)

* Check balance before showing cancel

* Fix linter

* Use existing helper methods for calculating increased cancel price

* Add tooltip for disabled button

* Lint fix for cancelError branch.

* Disabling of cancel button should account for value of tx.

* E2E - Dont close window notifications (#6349)

* Dont close window notifications

* Remove commented out lines in beta spec

* Don't include tx value in calculation of balance sufficiency for cancel button disabling. (#6346)

* enable privacy mode for first time users (#6347)

* Version 6.3.0 (#6350)
@danfinlay danfinlay mentioned this pull request Jun 18, 2019
Closed
@Gudahtt Gudahtt mentioned this pull request Jul 25, 2019
Merged
Gudahtt added a commit to Gudahtt/metamask-extension that referenced this pull request Jul 25, 2019
@Gudahtt
Override ownProps with state props in SignatureRequest
685560a 
The `accounts` prop of `SignatureRequest` was throwing a PropType
warning because `accounts` was an object instead of an array. It looks
like when the `mergeProps` function was added in MetaMask#6340, the ownProps
were accidentally set to override the state props.

The now ignored props have been removed from the parent `ConfirmTxScreen`
component as well. `conversionRate` was identical to the one retrieved
in `SignatureRequest`, and `selectedAddress` differed only in the
fallback behaviour when `state.metamask.selectedAddress` does not exist;
it will now default to the first account instead (as was the original
behavior, prior to MetaMask#6340).
Gudahtt added a commit that referenced this pull request Jul 26, 2019
@Gudahtt
Override ownProps with state props in SignatureRequest (#6911)
1f45798 
The `accounts` prop of `SignatureRequest` was throwing a PropType
warning because `accounts` was an object instead of an array. It looks
like when the `mergeProps` function was added in #6340, the ownProps
were accidentally set to override the state props.

The now ignored props have been removed from the parent `ConfirmTxScreen`
component as well. `conversionRate` was identical to the one retrieved
in `SignatureRequest`, and `selectedAddress` differed only in the
fallback behaviour when `state.metamask.selectedAddress` does not exist;
it will now default to the first account instead (as was the original
behavior, prior to #6340).
@Gudahtt Gudahtt mentioned this pull request Oct 31, 2019
Merged
Gudahtt added a commit that referenced this pull request Oct 31, 2019
@Gudahtt
Cleanup beforeunload handler after transaction is resolved
48277c2 
The notification window was updated to reject transactions upon close
in #6340. A handler that rejects the transaction was added to
`window.onbeforeunload`, and it was cleared in `actions.js` if it was
confirmed or rejected.

However, the `onbeforeunload` handler remained uncleared if the
transaction was resolved in another window. This results in the
transaction being rejected when the notification window closes, even
long after the transaction is submitted and confirmed. This has been
the cause of many problems with the Firefox e2e tests.

Instead the `onbeforeunload` handler is cleared in the
`componentWillUnmount` lifecycle function, alongside where it's set in
the first place. This ensures that it's correctly unset regardless
of how the transaction was resolved, and it better matches user
expectations.
Gudahtt added a commit that referenced this pull request Oct 31, 2019
@Gudahtt
Cleanup beforeunload handler after transaction is resolved (#7333)
fe28e0d 
* Cleanup beforeunload handler after transaction is resolved

The notification window was updated to reject transactions upon close
in #6340. A handler that rejects the transaction was added to
`window.onbeforeunload`, and it was cleared in `actions.js` if it was
confirmed or rejected.

However, the `onbeforeunload` handler remained uncleared if the
transaction was resolved in another window. This results in the
transaction being rejected when the notification window closes, even
long after the transaction is submitted and confirmed. This has been
the cause of many problems with the Firefox e2e tests.

Instead the `onbeforeunload` handler is cleared in the
`componentWillUnmount` lifecycle function, alongside where it's set in
the first place. This ensures that it's correctly unset regardless
of how the transaction was resolved, and it better matches user
expectations.

* Fix indentation and remove redundant export

The `run-all.sh` Bash script now uses consistent indentation, and is
consistent about only re-exporting the Ganache arguments when they
change.

* Ensure transactions are completed before checking balance

Various intermittent e2e test failures appear to be caused by React
re-rendering the transaction list during the test, as the transaction
goes from pending to confirmed. To avoid this race condition, the
transaction is now explicitly looked for in the confirmed transaction
list in each of the tests using this pattern.

* Enable all e2e tests on Firefox

The remaining tests that were disabled on Firefox now work correctly.
Only a few timing adjustments were needed.

* Update Firefox used in CI

Firefox v70 is now used on CI instead of v68. This necessitated
rewriting the function where the extension ID was obtained because the
Firefox extensions page was redesigned.
tmashuang pushed a commit that referenced this pull request Nov 4, 2019
@Gudahtt @tmashuang
Cleanup beforeunload handler after transaction is resolved (#7333)
d4db12d 
* Cleanup beforeunload handler after transaction is resolved

The notification window was updated to reject transactions upon close
in #6340. A handler that rejects the transaction was added to
`window.onbeforeunload`, and it was cleared in `actions.js` if it was
confirmed or rejected.

However, the `onbeforeunload` handler remained uncleared if the
transaction was resolved in another window. This results in the
transaction being rejected when the notification window closes, even
long after the transaction is submitted and confirmed. This has been
the cause of many problems with the Firefox e2e tests.

Instead the `onbeforeunload` handler is cleared in the
`componentWillUnmount` lifecycle function, alongside where it's set in
the first place. This ensures that it's correctly unset regardless
of how the transaction was resolved, and it better matches user
expectations.

* Fix indentation and remove redundant export

The `run-all.sh` Bash script now uses consistent indentation, and is
consistent about only re-exporting the Ganache arguments when they
change.

* Ensure transactions are completed before checking balance

Various intermittent e2e test failures appear to be caused by React
re-rendering the transaction list during the test, as the transaction
goes from pending to confirmed. To avoid this race condition, the
transaction is now explicitly looked for in the confirmed transaction
list in each of the tests using this pattern.

* Enable all e2e tests on Firefox

The remaining tests that were disabled on Firefox now work correctly.
Only a few timing adjustments were needed.

* Update Firefox used in CI

Firefox v70 is now used on CI instead of v68. This necessitated
rewriting the function where the extension ID was obtained because the
Firefox extensions page was redesigned.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmashuang tmashuang tmashuang approved these changes

@whymarrh whymarrh whymarrh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reject tx on tx window close.
4 participants
@danjm @metamaskbot @whymarrh @tmashuang