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.

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

Add web3 shim usage notification #10039

Merged
merged 11 commits into from
Dec 10, 2020
Merged

Add web3 shim usage notification #10039

merged 11 commits into from
Dec 10, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Dec 10, 2020

Implements the web3 shim usage notification, using the AlertController and HomeNotification. The new notification has been tested in Firefox and Chrome.

The web3 shim usage logging state now lives in the alert controller. Its lifecycle is coterminous with the extension background process, as it's erased on boot. The notification is shown in the popup only if the current website has any permissions and attempted to use web3. The user can:

  1. Ignore the notification
  2. Dismiss the notification. This will hide the notification for the current website until the extension is restarted.
  3. Select "Don't show this again", and dismiss the notification. This will hide the notification for all websites until the user re-enables it in settings. A tooltip indicates this possibility to the user.

In sum, the following changes were made:

  • Moves the alert controller constants to shared/constants/alerts, adds new constants
  • Adds new messages
  • Adds relevant state and methods to the alert controller
  • Fixes alert controller state initialization bug
    • The persisted would overwrite defaultState.alertEnabledness rather than merging it, meaning that new alerts wouldn't be added to state. This hasn't bitten us because the only other alert added since the controller was created can't be toggled.
  • Adds relevant selectors and actions in the UI
  • Adds a checkbox to the HomeNotification component, which is used for disabling the alert
  • Adds a link to a support article (yet to be published) in the web3 shim usage notification
  • Deletes app/images/icons/connect.svg, which we missed deleting in Disable console in contentscript #10040

@rekmarks rekmarks added DO-NOT-MERGE Pull requests that should not be merged 2020-breaking-changes labels Dec 10, 2020
@metamaskbot
Copy link
Collaborator

Builds ready [26c9536]
Page Load Metrics (597 ± 24 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint327847115
domContentLoaded5307145965124
load5327155975024
domInteractive5307135965124

@rekmarks rekmarks force-pushed the web3-usage-notification branch from 26c9536 to d555463 Compare December 10, 2020 20:05
@metamaskbot
Copy link
Collaborator

Builds ready [b7b3d3f]
Page Load Metrics (550 ± 11 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32604594
domContentLoaded5115885482211
load5135895502211
domInteractive5115885482211

unconnectedAccountAlertShownOrigins: {},
web3ShimUsageOrigins: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: should you just spread ...defaultState before alertEnabledness and remove the empty objects here? That way if default state changes in the future it is easily extensible here? No strong opinions on this, it is safe the way it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, in retrospect -- do we want persistence for web3ShimUsageOrigins? Then we'd want to spread initState.web3ShimUsageOrigins into the state declaration

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't want to persist web3ShimUsageOrigins, because we want dismissed web3 shim usage notifications to reappear if the extension is restarted (assuming the notification is enabled), and storing the origins could contribute to state bloat over time.

Nevertheless, I followed your suggestion about spreading defaultState first, it's better!

@rekmarks rekmarks dismissed a stale review via 661dd5f December 10, 2020 21:33
@metamaskbot
Copy link
Collaborator

Builds ready [995f518]
Page Load Metrics (507 ± 29 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31614494
domContentLoaded3065575066029
load3085595076129
domInteractive3065565056029

@rekmarks rekmarks removed the DO-NOT-MERGE Pull requests that should not be merged label Dec 10, 2020
@rekmarks rekmarks marked this pull request as ready for review December 10, 2020 22:23
@rekmarks rekmarks requested a review from a team as a code owner December 10, 2020 22:23
@rekmarks rekmarks requested a review from brad-decker December 10, 2020 22:23
ui/app/store/actions.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [6fd5a07]
Page Load Metrics (601 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint348051136
domContentLoaded5017505996933
load5067516016933
domInteractive5017495996933

Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

LGTM

@metamaskbot
Copy link
Collaborator

Builds ready [4cb4bfa]
Page Load Metrics (545 ± 12 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint326045105
domContentLoaded5086195432412
load5106205452412
domInteractive5086195432412

@rekmarks rekmarks merged commit 54e9c53 into develop Dec 10, 2020
@rekmarks rekmarks deleted the web3-usage-notification branch December 10, 2020 23:40
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants