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

[5.3] Show amount being approved by default on approval screens #4269

Merged
merged 10 commits into from
Jun 16, 2022

Conversation

blackdevelopa
Copy link
Contributor

Description

This PR should show the proposed approval/custom spend limit on the approval screen

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Progresses #4148

@github-actions
Copy link
Contributor

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.

@blackdevelopa blackdevelopa force-pushed the improvement/245-show-token-approval-amount branch from fef9423 to a92ed29 Compare May 11, 2022 14:38
@blackdevelopa blackdevelopa marked this pull request as ready for review May 11, 2022 14:38
@blackdevelopa blackdevelopa requested a review from a team as a code owner May 11, 2022 14:38
@blackdevelopa blackdevelopa self-assigned this May 11, 2022
@blackdevelopa blackdevelopa added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Code Impact - Low Minor code change that can safely applied to the codebase Priority - Medium Task with medium priority labels May 11, 2022
@blackdevelopa blackdevelopa changed the title Show amount being approved by default on approval screens [5.2] Show amount being approved by default on approval screens May 11, 2022
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@sethkfman sethkfman added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board labels May 11, 2022
@sethkfman
Copy link
Contributor

@blackdevelopa if this ticket is ready. I would move it into QA

@blackdevelopa
Copy link
Contributor Author

blackdevelopa commented May 12, 2022

@blackdevelopa if this ticket is ready. I would move it into QA

I am actually waiting on Saya for a design update on the approval screen before moving this to QA

Ready for QA.

@mobularay mobularay changed the title [5.2] Show amount being approved by default on approval screens [5.3] Show amount being approved by default on approval screens May 16, 2022
@blackdevelopa blackdevelopa force-pushed the improvement/245-show-token-approval-amount branch from 2584673 to dcc25ea Compare May 18, 2022 09:38
@plasmacorral plasmacorral added Priority - High Task with high priority and removed Priority - Medium Task with medium priority labels Jun 1, 2022
@sethkfman sethkfman added the Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing label Jun 1, 2022
@plasmacorral plasmacorral added the QA in Progress QA has started on the feature. label Jun 2, 2022
@plasmacorral
Copy link
Contributor

Currently hash a2ddea4 is not showing these updates when using the MetaMask swaps button on Binance network in Android 11, 12 or iOS 15.5.

Screen Shot 2022-06-06 at 12 54 00 PM

Please make these changes in the swaps flow as well.

@plasmacorral plasmacorral added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels Jun 6, 2022
@plasmacorral
Copy link
Contributor

One more note here: In this branch the "proposed approval limit" is showing as a long number with commas that includes a whole bunch of zeros. (please forgive my terribly out of focus screenshot, but you can at least see all the zeros)
Screen Shot 2022-06-07 at 10 53 32 AM

Typically in production, this proposed approval limit value is the numeric equivalent of ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff which is equal to 115792089237316195423570985008687907853269984665640564039457584007913129639935.

@blackdevelopa blackdevelopa force-pushed the improvement/245-show-token-approval-amount branch from a2ddea4 to d1e5672 Compare June 8, 2022 07:54
@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jun 9, 2022
@plasmacorral plasmacorral added QA in Progress QA has started on the feature. QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA in Progress QA has started on the feature. labels Jun 9, 2022
@plasmacorral
Copy link
Contributor

plasmacorral commented Jun 9, 2022

I am still not seeing the change in d1e5672 to more prominently feature the approval amount when using MetaMask swaps on Polygon/Binance chains.

iOS 15.5
iPhone 13 mini

@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jun 10, 2022
@blackdevelopa
Copy link
Contributor Author

Adding this comment for the Swap flow and removing the QA issues found label. https://consensys.slack.com/archives/C03BGLXT03E/p1654878051963769?thread_ts=1654184068.226649&cid=C03BGLXT03E

@plasmacorral
Copy link
Contributor

plasmacorral commented Jun 10, 2022

Android 12
Pixel 3a

Screen Shot 2022-06-10 at 6 11 08 PM

It seems that the part of the modal where this new information is presented is not scroll-able. This actually serves to blind android users operating in split screen mode. With this change as tested in d1e5672 we actually no longer present even a mention of the spending allowance amount or provide an option to edit within the approval modal. The exception being, that if the user taps "View details" they can view but not edit the token spending allowance.

The experience on the split screen using pancakeswap.finance is tough to say the least, I still found it surprising how many android users were recently encountering the app closure in android split screen. While this may be a relatively small segment of users, the implications of these changes create what I believe to be a merge blocker. We should not further obfuscate the spending approval for this small segment of users in our effort to expand visibility and enable informed consent around spending approvals.

A possible solution would be to support scrolling for the entire approval modal, rather than just the bottom as we do today.

Reproduction steps:

  1. Open another app on the device that you will later use for split screen
  2. Hold or acquire non-native asset on the BNB network on the address under test
  3. Do NOT have any existing token spending approvals for the non-native asset under test that is greater than the volume of the asset currently held or equal to 115792089237316195423570985008687907853269984665640564039457584007913129639935
  4. Launch MetaMask mobile client that is already setup
  5. Tap the burger menu and access the browser
  6. Navigate to https://Chainlist.org
  7. Tap CTA to connect wallet
  8. Tap CTA to add BNB network
  9. Go to https://pancakeswap.finance and connect wallet
  10. Let dapp change network if you did not already do that on Chainlist
  11. Tap trade on the bottom left corner of the pancake UI
  12. Select a non-native token for trading so you will need to provide token approval
  13. Attempt to trade an amount that is larger than any existing token spending allowance set on chain for the address under test
  14. Tap the "enable (asset)" CTA on Pancakeswap
  15. Note the changes to include the spending approval amount on the approval modal
  16. Dismiss or reject the approval modal
  17. Swipe up from the bottom of the screen to access android app drawer
  18. Swipe around to to feature the app you opened in step one and long press its icon
  19. Select "split screen"
  20. Tap to select MetaMask mobile as your second app for split screen
  21. Tap the "enable (asset)" CTA on Pancakeswap
  22. Note that token spending allowance AMOUNT is not shown
  23. Note that CTA to edit token spending allowance is not shown
  24. Note that modal is not scroll-able to present this information to the user
  25. Tap "View details" to see the only place the token spending allowance is displayed
  26. Note that token spending allowance is unable to be edited in view details, or at all in split screen

@plasmacorral plasmacorral added blocker QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jun 10, 2022
@mobularay mobularay added the release-5.3.0 Issue or pull request that will be included in release 5.3.0 label Jun 14, 2022
@omnat
Copy link
Contributor

omnat commented Jun 14, 2022

Per the discussion, let's not consider this issue as a release blocker; i.e., the issue of approval amount not visible when user is in split screen mode.

@sethkfman will write a ticket for upcoming sprint to disable split screen mode unless user is on a tablet (which is when split screen is likely more common and real estate not an issue for hiding consent visibility).

cc @plasmacorral @jakehaugen @gantunesr

@plasmacorral plasmacorral added QA in Progress QA has started on the feature. and removed blocker QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jun 14, 2022
@plasmacorral
Copy link
Contributor

plasmacorral commented Jun 14, 2022

This is a non-blocking observation that may merit further investigation or may simply be a matter of my connectivity challenges today.

Android 11
Samsung A515f

https://recordit.co/oA3w4FoPyE
Screen Shot 2022-06-14 at 6 44 20 PM

I have been experiencing connectivity issues today, but this "Give permission to access your [missing {{token symbol}} value]?" hung for nearly 3 seconds before the Token and spending approval amount was resolved.

This was for a non-native asset that had not yet been imported into MetaMask.

Reproduction steps 1-14 cited in my earlier comments were used to get here.

@plasmacorral plasmacorral added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Jun 14, 2022
@sethkfman sethkfman merged commit a6249d9 into main Jun 16, 2022
@sethkfman sethkfman deleted the improvement/245-show-token-approval-amount branch June 16, 2022 16:44
@github-actions github-actions bot locked and limited conversation to collaborators Jun 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Code Impact - Low Minor code change that can safely applied to the codebase Priority - High Task with high priority QA Passed A successful QA run through has been done release-5.3.0 Issue or pull request that will be included in release 5.3.0 Sev1-high An issue that may have caused fund loss or access to wallet in the past & may still be ongoing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants