metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Published 20 hours ago • verified publisher icon MetaMask

ConfirmPage: update UI for setApproveForAll transactions to better warn users of the allowances

Open digiwand opened this issue 2 years ago • 8 comments

Explanation

In https://github.com/MetaMask/metamask-extension/pull/15496, @cygaar proposed improving the warning message on the ConfirmPage for setApprovalForAll transaction types and gathered feedback to support this improvement.

Based on an internal dialog here, we will be updating the "Confirm" button to red, and including an error type Dialog to better warn users that they are interacting with a setApprovalForAll transaction. This is a temporary improvement while a redesign is on the way. More details are explained in https://github.com/MetaMask/metamask-extension/pull/15496 and the internal dialog.

More Information

See: https://github.com/MetaMask/metamask-extension/pull/15496 See: Internal Dialog

Screenshots/Screencaps

Before

Screenshot 2022-08-10 at 3 17 20 AM

After

Screenshot 2022-08-10 at 6 06 42 PM Screenshot 2022-08-10 at 6 07 04 PM

Manual Testing Steps

  • send a setApprovalForAll transaction and observe a red, danger-primary "Confirm" button
  • send any other transaction and observe a blue, primary "Confirm" button

+ If there are functional changes:

  • [ ] Manual testing complete & passed
  • [ ] "Extension QA Board" label has been applied

digiwand avatar Aug 09 '22 17:08 digiwand

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

github-actions[bot] avatar Aug 09 '22 17:08 github-actions[bot]

Great to see, thanks for updating!

cygaar avatar Aug 09 '22 19:08 cygaar

Builds ready [a5fe3c6]
Page Load Metrics (2218 ± 84 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint952163238442212
domContentLoaded19012507218618187
load19012507221817584
domInteractive19012507218618187

highlights:

storybook

metamaskbot avatar Aug 09 '22 20:08 metamaskbot

The language This should never be your default included in the warning might throw some users off, since almost no dapps will present users with a secondary option (say, to approve a single tokenId). I would find this language confusing. I think we should make the risks known and present it with the appropriate gravity of warning without suggesting that the user is missing some less risky option that very mainstream dapps likely don't offer. Open to being convinced otherwise though! cc @tayvano tagging you since I know you have strong opinions about security and the language we use to convey it.

@adonesky1 what do you think about: "You're giving access to all your current and future <NFTname> NFTs. The other party might be able to transfer them from your wallet without further notice. Proceed with caution." cc: @SayaGT @coreyjanssen

bschorchit avatar Aug 09 '22 21:08 bschorchit

How about: "By clicking approve you are granting access to all of the NFTs you currently own on this contract and any NFTs on this contract you may acquire in the future, until you revoke this approval. The party to whom you're granting approval will be able to transfer your NFTs from your wallet without further notice. Proceed with caution" something like that... away from my computer right not so grammar could use some tweaking

adonesky1 avatar Aug 09 '22 21:08 adonesky1

How about: "By clicking approve you are granting access to all of the NFTs you currently own on this contract and any NFTs on this contract you may acquire in the future, until you revoke this approval. The party to whom you're granting approval will be able to transfer your NFTs from your wallet without further notice. Proceed with caution" something like that... away from my computer right not so grammar could use some tweaking

Sounds great to me!

bschorchit avatar Aug 09 '22 21:08 bschorchit

Great call out and suggestion @adonesky1! Looks good to me. The first message was copied from a mock supplied in this Slack thread.

I moved one comma and kept the rest the same:

By clicking approve, you are granting access to all of the NFTs you currently own on this contract and any NFTs on this contract you may acquire in the future until you revoke this approval. The party to whom you're granting approval will be able to transfer your NFTs from your wallet without further notice. Proceed with caution.

Updated: https://github.com/MetaMask/metamask-extension/pull/15512/commits/5e0b148eee0a4d1a2a34366c7ebac34e7172fd8e

--

In regards to keeping or omitting the punctuation on the last sentence, we could do that in a follow-up PR if this PR gets approved before the final decision here. ref: https://github.com/MetaMask/metamask-extension/pull/15512#discussion_r941765206

digiwand avatar Aug 10 '22 10:08 digiwand

Builds ready [ac14058]
Page Load Metrics (1818 ± 116 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint862401103517
domContentLoaded159325361808234113
load159425361818242116
domInteractive159325361808234113

highlights:

storybook

metamaskbot avatar Aug 10 '22 11:08 metamaskbot

Apologies for not seeing this convo until today! (thanks @bschorchit!) Made a few edits to the proposed copy. Lmk if this works!

You're granting access to all the NFTs on this contract, including any you might own in the future. The party on the other end of this approval can transfer NFTs from your wallet at any time without asking you. Proceed with caution.

coreyjanssen avatar Aug 24 '22 23:08 coreyjanssen

Hi @coreyjanssen,

No problem! I like this update, and I'm up for updating this. One thing that I would miss from the copy that has been merged

By clicking approve, you are granting access to all of the NFTs you currently own on this contract and any NFTs on this contract you may acquire in the future until you revoke this approval. The party to whom you're granting approval will be able to transfer your NFTs from your wallet without further notice. Proceed with caution.

is the mention that this contract is able to access the NFTs until you revoke this approval.. Is this something we could add in somehow?

cc: @adonesky1 @bschorchit

digiwand avatar Aug 28 '22 03:08 digiwand

@digiwand sure thing! how's this?

You're granting access to all the NFTs on this contract, including any you might own in the future. The party on the other end can transfer NFTs from your wallet at any time without asking you until you revoke this approval. Proceed with caution.

coreyjanssen avatar Sep 05 '22 20:09 coreyjanssen

Awesome!!! I think this is an A1 update! Thanks @coreyjanssen! :D

Updated here: https://github.com/MetaMask/metamask-extension/pull/15744

digiwand avatar Sep 06 '22 11:09 digiwand