warn user when sending from different account #8601

brad-decker · 2020-05-15T17:25:21Z

task: Warn the user when the "from" account of confirmation is different from the selected account

Trello card: https://trello.com/c/ZijoEYIz
Figma: https://www.figma.com/file/CvD7nhQVeSEB1QuSy3XhtY/oCap-%2F-Login-Per-Site?node-id=2783%3A144

Screenshot

ui/app/components/ui/sender-to-recipient/sender-to-recipient.component.js

brad-decker · 2020-05-15T17:33:24Z

Warn the user when the "from" account of a confirmation is different from the selected account

metamaskbot · 2020-05-18T16:56:25Z

Builds ready [cad1c2a]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (623 ± 44 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	32	83	38	11	5
		domContentLoaded	351	745	621	92	44
		load	352	746	623	92	44
		domInteractive	351	744	621	92	44

Gudahtt

Looks good aside from the missing checksummedSenderAddress prop!

ui/app/components/ui/icon/info-icon.component.js

ui/app/components/ui/sender-to-recipient/sender-to-recipient.component.js

ui/app/components/ui/sender-to-recipient/index.scss

app/_locales/en/messages.json

ui/app/hooks/useDappAuthorizationCheck.js

ui/app/components/ui/sender-to-recipient/sender-to-recipient.component.js

metamaskbot · 2020-05-19T21:46:38Z

Builds ready [654efd8]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (659 ± 68 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	110	48	23	11
		domContentLoaded	342	917	658	141	68
		load	344	920	659	141	68
		domInteractive	342	917	657	141	68

metamaskbot · 2020-05-20T16:05:05Z

Builds ready [468d5ec]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (626 ± 61 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	32	87	43	16	8
		domContentLoaded	377	802	624	127	61
		load	379	803	626	127	61
		domInteractive	377	802	624	127	61

brad-decker · 2020-05-20T16:06:47Z

@Gudahtt could use a re-review when you have time, included a commit breakdown in the description.

metamaskbot · 2020-05-20T21:15:30Z

Builds ready [08d94ee]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (627 ± 70 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	33	86	43	11	5
		domContentLoaded	353	863	625	146	70
		load	354	865	627	146	70
		domInteractive	352	863	624	146	70

rekmarks

Looks great, and I really like the refactor!

However, the address "copied to clipboard" tooltip doesn't appear to be working correctly. Notice in the recording how the sender tooltip changes without clicking, then both tooltips display Copied! regardless of what I click.

Screen Recording

rachelcope · 2020-05-20T21:39:20Z

What about updating the copy to something a little more concise?

"Is this the correct account? It's different from the currently selected account in your wallet"

@brad-decker @Gudahtt

brad-decker · 2020-05-20T22:25:27Z

@rekmarks -- Thanks for finding that! The current experience on develop is actually pretty broken. First off, the hover state only works half of the time for the recipient address hover... only if you go from sender tooltip active -> move the mouse over directly to the recipient. If you enter the recipient address from anywhere else it doesn't work. Furthermore, clicking it doesn't update the message to say "Copied!" because it doesn't actually trigger a set state call.

To address this:

updated react-tippy
added an offset prop to tooltip-v2 (because the reason for flakey hover seems to be related to the tooltip trying to open partially offscreen?)
separate state for recipient and sender address copying
Move more of the logic around in the split up components so that state doesn't need to be passed anywhere.

all of this is isolated to the latest commit.

metamaskbot · 2020-05-20T22:42:49Z

Builds ready [2253f80]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (633 ± 57 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	32	78	41	12	6
		domContentLoaded	361	845	632	118	57
		load	363	847	633	118	57
		domInteractive	361	844	631	118	57

brad-decker · 2020-05-21T14:47:43Z

@rachelcope made the copy change.

metamaskbot · 2020-05-21T15:11:21Z

Builds ready [f48f070]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (644 ± 61 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	32	105	51	22	11
		domContentLoaded	357	821	642	126	61
		load	358	823	644	126	61
		domInteractive	357	821	642	126	61

metamaskbot · 2020-05-21T18:04:55Z

Builds ready [8dd1e78]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (673 ± 43 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	58	38	7	4
		domContentLoaded	373	784	671	90	43
		load	375	786	673	90	43
		domInteractive	373	783	671	90	43

package.json

rekmarks

LGTM!

metamaskbot · 2020-05-21T20:02:07Z

Builds ready [9fefb4b]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (559 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	104	42	18	9
		domContentLoaded	344	700	558	125	60
		load	346	702	559	125	60
		domInteractive	344	700	558	125	60

Gudahtt

LGTM!

brad-decker requested a review from Gudahtt May 15, 2020 17:25

brad-decker commented May 15, 2020

View reviewed changes

ui/app/components/ui/sender-to-recipient/sender-to-recipient.component.js Outdated Show resolved Hide resolved

brad-decker commented May 15, 2020

View reviewed changes

ui/app/components/ui/sender-to-recipient/sender-to-recipient.component.js Outdated Show resolved Hide resolved

brad-decker force-pushed the alert-account-mismatch branch 2 times, most recently from e319370 to cad1c2a Compare May 15, 2020 17:31

brad-decker marked this pull request as draft May 18, 2020 20:56

brad-decker removed the request for review from Gudahtt May 18, 2020 20:56

Gudahtt requested changes May 19, 2020

View reviewed changes

brad-decker force-pushed the alert-account-mismatch branch from cad1c2a to 9028060 Compare May 19, 2020 18:19

brad-decker commented May 19, 2020

View reviewed changes

app/_locales/en/messages.json Outdated Show resolved Hide resolved

brad-decker force-pushed the alert-account-mismatch branch 2 times, most recently from 6b99573 to b027e3e Compare May 19, 2020 18:24

brad-decker commented May 19, 2020

View reviewed changes

ui/app/hooks/useDappAuthorizationCheck.js Outdated Show resolved Hide resolved

brad-decker force-pushed the alert-account-mismatch branch from b027e3e to 654efd8 Compare May 19, 2020 18:32

Gudahtt reviewed May 19, 2020

View reviewed changes

ui/app/components/ui/sender-to-recipient/sender-to-recipient.component.js Outdated Show resolved Hide resolved

brad-decker force-pushed the alert-account-mismatch branch from 02d2cd3 to 468d5ec Compare May 20, 2020 15:44

brad-decker marked this pull request as ready for review May 20, 2020 16:02

brad-decker requested a review from a team as a code owner May 20, 2020 16:02

brad-decker requested a review from Gudahtt May 20, 2020 16:06

brad-decker force-pushed the alert-account-mismatch branch from 468d5ec to 08d94ee Compare May 20, 2020 20:45

rekmarks requested changes May 20, 2020

View reviewed changes

brad-decker requested a review from rekmarks May 20, 2020 22:25

brad-decker force-pushed the alert-account-mismatch branch 2 times, most recently from 91ba841 to 39c6412 Compare May 21, 2020 17:27

brad-decker added 5 commits May 21, 2020 12:40

refactor to functional components, drop proxyquire

046a281

add new info icon component

ae5cd21

Add new account mismatch warning component

3a6a39e

include scss from new components

27c1b2b

add new warning component to conf screens

77b2ff7

brad-decker force-pushed the alert-account-mismatch branch from 39c6412 to 8dd1e78 Compare May 21, 2020 17:41

Gudahtt reviewed May 21, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

brad-decker added 3 commits May 21, 2020 14:32

fix test

f6e4d14

change message copy

72260d8

removing new instances of useContext(I18nContext)

9fefb4b

brad-decker force-pushed the alert-account-mismatch branch from 8dd1e78 to 9fefb4b Compare May 21, 2020 19:32

rekmarks approved these changes May 21, 2020

View reviewed changes

brad-decker requested a review from Gudahtt May 21, 2020 19:41

Gudahtt approved these changes May 21, 2020

View reviewed changes

brad-decker merged commit 389df9d into develop May 21, 2020

brad-decker deleted the alert-account-mismatch branch May 21, 2020 20:21

tmashuang mentioned this pull request Nov 19, 2020

No sender and recipient addresses shown when sending a token in full screen mode #9920

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

warn user when sending from different account #8601

warn user when sending from different account #8601

brad-decker commented May 15, 2020 •

edited

Loading

brad-decker commented May 15, 2020

metamaskbot commented May 18, 2020

Gudahtt left a comment

metamaskbot commented May 19, 2020

metamaskbot commented May 20, 2020

brad-decker commented May 20, 2020

metamaskbot commented May 20, 2020

rekmarks left a comment •

edited

Loading

rachelcope commented May 20, 2020

brad-decker commented May 20, 2020

metamaskbot commented May 20, 2020

brad-decker commented May 21, 2020

metamaskbot commented May 21, 2020

metamaskbot commented May 21, 2020

rekmarks left a comment

metamaskbot commented May 21, 2020

Gudahtt left a comment

warn user when sending from different account #8601

warn user when sending from different account #8601

Conversation

brad-decker commented May 15, 2020 • edited Loading

brad-decker commented May 15, 2020

metamaskbot commented May 18, 2020

Gudahtt left a comment

Choose a reason for hiding this comment

metamaskbot commented May 19, 2020

metamaskbot commented May 20, 2020

brad-decker commented May 20, 2020

metamaskbot commented May 20, 2020

rekmarks left a comment • edited Loading

Choose a reason for hiding this comment

rachelcope commented May 20, 2020

brad-decker commented May 20, 2020

metamaskbot commented May 20, 2020

brad-decker commented May 21, 2020

metamaskbot commented May 21, 2020

metamaskbot commented May 21, 2020

rekmarks left a comment

Choose a reason for hiding this comment

metamaskbot commented May 21, 2020

Gudahtt left a comment

Choose a reason for hiding this comment

brad-decker commented May 15, 2020 •

edited

Loading

rekmarks left a comment •

edited

Loading