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

New "approve" screen doesn't let you copy the address being approved #7615

Closed
haydenadams opened this issue Dec 2, 2019 · 11 comments
Closed
Labels
needs-design Needs design support. team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead type-enhancement type-security

Comments

@haydenadams
Copy link

Describe the bug
Address being approved is truncated and can not be copied.

To Reproduce
Steps to reproduce the behavior:

  1. Do an approve transaction
  2. Look at transaction details
  3. Try and figure out how to get the address being approved, other than parsing raw transaction data

Expected behavior
A little button that allows you to copy the address.

Screenshots
image

Browser details (please complete the following information):

  • OS: OS X
  • Browser - Chrome
  • MetaMask Version - Latest
@danfinlay
Copy link
Contributor

@MetaMask/design ^ good opportunity to enhance this design. Anywhere we show an address is an opportunity to do all the things we do for an address.

@cjeria
Copy link
Contributor

cjeria commented Dec 4, 2019

It appears this was spec'd in the initial designs but wasn't implemented and was missed during QA. See mockup below. This is the icon I'd suggest we use for the copy to clipboard. I'd also advise making the entire address string clickable + tool tip "copy to clipboard". cc @danjm @whymarrh

image

@cjeria
Copy link
Contributor

cjeria commented Dec 9, 2019

@danjm consolidating design feedback for Approve screen designs here.

  1. The current approve screen account details row has some text-overflow bugs. The proposed solution is to show a header and balance of the specific token being approved. Here's a link to the updated design.

image

  1. The expanded transaction fee detail section has a few px's more padding. It should be flush with the top section at 24px padding:
    image

@danfinlay
Copy link
Contributor

We also got some feedback today, which I agree with, that the top of the approve screen should not refer to the domain, but to the contract address actually being granted the permission. It should also have its name drawn from the address book or reverse-resolved from ENS if possible (not just hex or the word "contract" for "to").

This is to match the reality of what is being trusted: The smart contract, not the domain. Might also want to add a link to its block-explorer page.

prompt

@cjeria
Copy link
Contributor

cjeria commented Dec 10, 2019

We also got some feedback today, which I agree with, that the top of the approve screen should not refer to the domain, but to the contract address actually being granted the permission.

@danfinlay based on our lengthy design discussions, the goal was to make the Approve screen overall more human-readable and reduce confusion for the user.

Since the user is interacting with a site/dapp at the very moment that the Approve function is called, it makes more usability sense to call out the domain in the header for the reason that it provides the user a more direct context and association with the dapp/site they are interacting with.

We were also very conscious about making it simple to access the contract address, which is just one level deep under the transaction details. So we feel confident that this design meets the requirements that satisfy our usability goals with pro users' needs with minimal friction. That said, I don't feel it's necessary to swap out the domain name with the contract address.

cc @bdresser @rachelcope @omnat

@danfinlay
Copy link
Contributor

@danfinlay based on our lengthy design discussions, the goal was to make the Approve screen overall more human-readable and reduce confusion for the user.

The length of the discussion is irrelevant. Long discussions can reach wrong conclusions.

The word we're showing there is not representative of what is being permitted, and without the presence of the actual address, we are not giving the tools to correctly review this.

We were also very conscious about making it simple to access the contract address, which is just one level deep under the transaction details. So we feel confident that this design meets the requirements

Currently this address is concatenated, and does not allow copying, so this requirement is not yet met:

Screen Shot 2019-12-10 at 2 55 38 PM

It would also be nice to use the address book and/or ENS to render a rich name here when available.

@cjeria
Copy link
Contributor

cjeria commented Dec 18, 2019

Dan pointed out that Etherscan now has dedicated smart contract pages . We may want to consider linking users to these smart contract dapp pages from the Approve screen. We may also want to take inspiration from how they display the smart contract address for our approve screen (bubble up the smart contract address). cc @rachelcope

@amsimoes
Copy link

amsimoes commented Nov 7, 2020

Guys, any update on this?

It isn't possible yet to check the contract requesting approval, being useless actually...

Screenshot 2020-11-07 at 22 30 22

Is there any workaround to check the contract apart from confirming the TX? Like in console/inspector?

Screenshot 2020-11-07 at 22 31 46

@cjeria

Thanks!

@Gudahtt Gudahtt self-assigned this Jan 7, 2021
@jacobc-eth jacobc-eth added needs-design Needs design support. and removed design feedback labels Apr 13, 2021
@dzid26
Copy link

dzid26 commented Aug 16, 2021

Seems like a easy fix.
It's a shame one still cannot easily verify address of a contract they interact with.

Yes, this is a security issue.

@Gudahtt Gudahtt removed their assignment Aug 16, 2021
@jedcli
Copy link

jedcli commented Nov 13, 2021

I double the comment by @dzid26 on top. It is really frustrating issue you can't check a contract before confirming a transaction.

@bschorchit
Copy link

Closing as this has been solved. There's a copy button currently in prod.

@bschorchit bschorchit added the team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead label Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-design Needs design support. team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead type-enhancement type-security
Projects
None yet
Development

No branches or pull requests

10 participants