[Beta] ERC20 approve() -- UI makes it look like tokens are being sent

[trakout]

When using approve() the full amount being approved is displayed (makes sense probably), but there is no indicator showing the fact that tokens are not yet being transferred (as opposed to approved for transfer).

Also in use-cases where an 'unlimited' approve is used (eg. RadarRelay), the max uint is displayed as an amount that runs off the edge of the viewport. This, coupled with the fact that there is no indication that this is technically not a transfer may be alarming to users.

Also related, users can use approve() without owning any amount of a token. Therefore the transaction will be successful even though the UI displays 'Insufficient tokens'.

Screenshots below. Used Chrome Nightly on Mac OS.

[2-am-zzz]

@danjm @alextsg

[danjm]

@trakout Some questions and responses (mostly for me to ensure I understand you correctly):
(1) I assume you are calling approve() from some other dapp, correct?
(2) You are proposing that we alter the words and visual indicators when a token contract approve() method is called? (For example, perhaps "Authorize" would be a better header than "Confirm" and "Please review your approval" a better subheader and maybe instead of an arrow between the two identicons, a https://fontawesome.com/icons/handshake?style=solid... this will actually go to our designer @cjeria first, but I am just giving you some examples to ensure I understand you correctly)
(3) Yes, we need to fix the amount running off the page. Thanks for notifying us of that.
(4) Did you get to your second screen shot by calling approve() and then clicking "Edit" on the screen with the "Confirm" header?
(5) Assuming your answer to for is yes, you are proposing that in the case of approve() calls, we should not display and insufficient amount error or disable the "Next" button on the basis of the users current token balance?

Thanks a lot for reporting this.

[trakout]

Hey @danjm! Sure:

1. Correct, calling approve() directly from a dapp.
2. Yes, this is a good example -- I think it's pretty much in line with the idea behind approve().
3. 👍
4. Yes, this is the edit screen you can get to off the Confirmation screen that pops up after attempting approve().
5. I think that's correct -- overall I think it's a good idea to suppress the amount field validation for approve(). There's cases where approval will be attempted for amounts of tokens a user does not have, or even will ever have (eg. larger than a token's totalSupply). There's ongoing discussion on this here (EIP 717).

Thanks!

[danjm]

@trakout Thanks for your replies. Fixing the display of large amounts will likely be resolved this week. Better support for approve() (to address the other issues raised here) will be done in our next sprint, which starts next week.

Thanks again for reporting this.

[cjeria]

@danjm I think we should create a new issue for the design of the 'approve()' screen. This should be part of a more strategic move towards designing call specific transaction screens.

[danfinlay]

We should only show the token transfer approval screen when the data field starts with 0xa9059cbb, which is the 4-byte signature of the transfer method.

[danfinlay]

I think we should create a new issue for the design of the 'approve()' screen.

This is beyond the scope of this issue, I'd rather we address other confirmation types as other issues, and ideally in a generic way when possible (like our rendering of contract method metadata).

[danjm]

Design being handled in MetaMask/Design#18

[danjm]

Closing in favour of MetaMask/Design#18 and #4505