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

Improving warning text for eth_sign #12013

Merged
merged 2 commits into from
Sep 6, 2021
Merged

Improving warning text for eth_sign #12013

merged 2 commits into from
Sep 6, 2021

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Sep 3, 2021

Implements the first two steps for: #11337

Screen Shot 2021-09-03 at 11 22 51 AM

@ryanml ryanml requested a review from danfinlay September 3, 2021 18:27
@ryanml ryanml self-assigned this Sep 3, 2021
@ryanml ryanml requested a review from a team as a code owner September 3, 2021 18:27
@ryanml ryanml requested a review from adonesky1 September 3, 2021 18:27
@metamaskbot
Copy link
Collaborator

Builds ready [1461bb2]
Page Load Metrics (582 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint10078356115976
domContentLoaded38177956312862
load40178558212359
domInteractive38077956312862

@@ -1958,7 +1958,7 @@
"message": "Sign"
},
"signNotice": {
"message": "Signing this message can have \ndangerous side effects. Only sign messages from \nsites you fully trust with your entire account.\n This dangerous method will be removed in a future version. "
"message": "We are unable to estimate the result of this signature. This signature could potentially perform any operation on your account's behalf, including granting complete control of your account and all of its assets to the requesting site. Only sign this message if you know what you're doing or completely trust the requesting site."
Copy link
Contributor

@adonesky1 adonesky1 Sep 3, 2021

Choose a reason for hiding this comment

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

minor proposed change to copy : "We are unable to predict the result of this signature..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++

@ryanml ryanml requested a review from adonesky1 September 3, 2021 20:16
Copy link
Contributor

@adonesky1 adonesky1 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 [b755afb]
Page Load Metrics (493 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint9467043413465
domContentLoaded3616634719546
load3746714939445
domInteractive3616634719546

Copy link
Contributor

@NiranjanaBinoy NiranjanaBinoy left a comment

Choose a reason for hiding this comment

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

lgtm

@ryanml ryanml merged commit 449da1d into develop Sep 6, 2021
@ryanml ryanml deleted the fix-11337 branch September 6, 2021 17:28
@github-actions github-actions bot locked and limited conversation to collaborators Sep 6, 2021
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.

None yet

4 participants