Reject popup confirmations on close #12643

ritave · 2021-11-10T12:09:14Z

github-actions · 2021-11-10T12:09:32Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

ritave · 2021-11-10T12:11:09Z

I have read the CLA Document and I hereby sign the CLA

metamaskbot · 2021-11-10T18:52:09Z

Builds ready [2f275ef]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (665 ± 17 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	626	771	663	34	16
		domContentLoaded	628	772	664	35	17
		load	628	772	665	35	17
		domInteractive	628	772	664	35	17

highlights:

storybook

ui/pages/settings/advanced-tab/advanced-tab.container.js

rekmarks · 2021-11-10T19:34:00Z

@Gudahtt if all pending confirmations are rejected on notification popup close, how should we handle metrics? The ApprovalController probably needs a controller messenger event for this purpose, given the current implementation.

Gudahtt · 2021-11-10T19:56:48Z

Good point.

Right now our "on reject" metrics are in the UI. We can move them into the tx-state-manager to preserve them with this approach. And similarly for the "message manager"-style classes, we can have them dispatch metrics on reject as well. The state manager for the message seems like a better place than the UI to dispatch such a metric anyway.

As for the confirmations managed by the approval controller, I am not sure why we'd want to dispatch metrics directly in that controller. It doesn't know what these approvals are for, it doesn't have the context. The one controller using that right now is the permissions controller, so to take that as an example, we could dispatch the rejection metric event in rejectPermissionsRequest. That's where we have all of the context about what is being rejected.

ui/pages/confirmation/confirmation.js

ui/components/app/signature-request/signature-request.component.js

ui/components/app/signature-request-original/signature-request-original.component.js

ui/components/app/dropdowns/network-dropdown.js

app/scripts/lib/notification-manager.js

metamaskbot · 2021-11-11T12:11:50Z

Builds ready [cedd25f]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (685 ± 34 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	514	843	681	71	34
		domContentLoaded	501	846	682	73	35
		load	513	847	685	71	34
		domInteractive	501	846	682	73	35

highlights:

storybook

ui/pages/confirm-encryption-public-key/confirm-encryption-public-key.component.js

app/scripts/lib/message-manager.js

app/scripts/background.js

metamaskbot · 2021-11-11T18:07:30Z

Builds ready [f564ddb]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7

Page Load Metrics (768 ± 37 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	283	922	416	209	100
		domContentLoaded	693	986	767	78	38
		load	696	986	768	78	37
		domInteractive	693	986	767	78	38

highlights:

storybook

app/scripts/lib/message-manager.js

Gudahtt

This mostly looks good! One more set of changes left, for the remaining message managers.

ui/components/app/signature-request/signature-request.component.js

metamaskbot · 2021-11-15T14:28:14Z

Builds ready [ecfc773]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7

Page Load Metrics (771 ± 37 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	243	800	400	175	84
		domContentLoaded	699	983	770	76	37
		load	701	983	771	76	37
		domInteractive	699	983	770	76	37

highlights:

storybook

app/scripts/background.js

Gudahtt

LGTM!

metamaskbot · 2021-11-15T15:41:02Z

Builds ready [0434fdc]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7

Page Load Metrics (840 ± 36 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	301	932	520	250	120
		domContentLoaded	728	974	839	75	36
		load	731	974	840	74	36
		domInteractive	728	974	839	75	36

highlights:

storybook

ritave force-pushed the ritave/close-confirm branch 2 times, most recently from 2b52c51 to ce0f540 Compare November 10, 2021 17:52

rekmarks reviewed Nov 10, 2021

View reviewed changes

ui/pages/settings/advanced-tab/advanced-tab.container.js Outdated Show resolved Hide resolved

rekmarks requested a review from Gudahtt November 10, 2021 19:33

Gudahtt reviewed Nov 10, 2021

View reviewed changes

ui/pages/confirmation/confirmation.js Outdated Show resolved Hide resolved

Gudahtt reviewed Nov 10, 2021

View reviewed changes

ritave force-pushed the ritave/close-confirm branch from 2f275ef to cedd25f Compare November 11, 2021 11:35

Gudahtt reviewed Nov 11, 2021

View reviewed changes

ui/pages/confirm-encryption-public-key/confirm-encryption-public-key.component.js Outdated Show resolved Hide resolved

Gudahtt reviewed Nov 11, 2021

View reviewed changes

app/scripts/lib/message-manager.js Outdated Show resolved Hide resolved

app/scripts/background.js Outdated Show resolved Hide resolved

ritave changed the title ~~[WIP] Reject popup confirmations on close~~ Reject popup confirmations on close Nov 11, 2021

ritave marked this pull request as ready for review November 11, 2021 16:23

ritave requested a review from a team as a code owner November 11, 2021 16:23

ritave added 10 commits November 11, 2021 18:30

Background clears confirmations on popup close

69edd1e

[WIP] Remove clearing confirmations through UI

ffde446

Confirmations are now rejected instead of cleared

5bd6f13

Erased commented out code

0a1eec9

Fix linter errors

738f8b9

Changes after code review

7fa77ac

Moved metrics events from onWindowUnload to background

e7d9339

PR review fixes

107f880

Added abillity to add reason to rejection of messages

f9dd2c5

Fix prettier

f564ddb

ritave force-pushed the ritave/close-confirm branch from 2986657 to f564ddb Compare November 11, 2021 17:30

Gudahtt reviewed Nov 12, 2021

View reviewed changes

app/scripts/lib/message-manager.js Show resolved Hide resolved

Gudahtt reviewed Nov 12, 2021

View reviewed changes

ui/components/app/signature-request/signature-request.component.js Show resolved Hide resolved

ritave added 2 commits November 15, 2021 12:54

Added type metrics event to signature cancel

fa1b75e

Fix test

ecfc773

ritave self-assigned this Nov 15, 2021

Gudahtt reviewed Nov 15, 2021

View reviewed changes

app/scripts/background.js Outdated Show resolved Hide resolved

The uncofirmed transactions are now cleared even if Metamask is locked

0434fdc

Gudahtt approved these changes Nov 15, 2021

View reviewed changes

danjm approved these changes Nov 15, 2021

View reviewed changes

ritave merged commit a323a5f into develop Nov 15, 2021

ritave deleted the ritave/close-confirm branch November 15, 2021 16:13

github-actions bot locked and limited conversation to collaborators Nov 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject popup confirmations on close #12643

Reject popup confirmations on close #12643

ritave commented Nov 10, 2021

github-actions bot commented Nov 10, 2021 •

edited

Loading

ritave commented Nov 10, 2021

metamaskbot commented Nov 10, 2021

rekmarks commented Nov 10, 2021

Gudahtt commented Nov 10, 2021

metamaskbot commented Nov 11, 2021

metamaskbot commented Nov 11, 2021

Gudahtt left a comment

metamaskbot commented Nov 15, 2021

Gudahtt left a comment

metamaskbot commented Nov 15, 2021

Reject popup confirmations on close #12643

Reject popup confirmations on close #12643

Conversation

ritave commented Nov 10, 2021

github-actions bot commented Nov 10, 2021 • edited Loading

ritave commented Nov 10, 2021

metamaskbot commented Nov 10, 2021

highlights:

rekmarks commented Nov 10, 2021

Gudahtt commented Nov 10, 2021

metamaskbot commented Nov 11, 2021

highlights:

metamaskbot commented Nov 11, 2021

highlights:

Gudahtt left a comment

Choose a reason for hiding this comment

metamaskbot commented Nov 15, 2021

highlights:

Gudahtt left a comment

Choose a reason for hiding this comment

metamaskbot commented Nov 15, 2021

highlights:

github-actions bot commented Nov 10, 2021 •

edited

Loading