Skip to content

Verify Contract Details#5327

Merged
seaona merged 24 commits intomainfrom
ft/verify_contract_details
Mar 13, 2023
Merged

Verify Contract Details#5327
seaona merged 24 commits intomainfrom
ft/verify_contract_details

Conversation

@blackdevelopa
Copy link
Copy Markdown
Contributor

@blackdevelopa blackdevelopa commented Dec 1, 2022 •
edited
Loading

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description
The aim of this code (PR) is to provide more insights before approving a transaction. There will now be the option to learn more about the contract and token involved and to verify these addresses, as well as add familiar addresses to the Contact list.
https://github.com/MetaMask/MetaMask-planning/issues/179

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions,
1. What is the reason for the change?
2. What is the improvement/solution?

See design

Screenshots/Recordings
Before
http://recordit.co/luMSaC7IrQ

After
http://recordit.co/ecgFtvxIox

On smaller device (SE)
http://recordit.co/bLxbYGJJyP

Todo:

  • Show a default (loading) token (image) state while fetching address details
  • Hide block explorer icon if block explorer url isn't provided
  • Show warning of potential fund loss when adding token address. See here

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

Test Case
Update in a bit

Progresses https://github.com/MetaMask/MetaMask-planning/issues/179

Checklist

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Dec 1, 2022

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 self-assigned this Dec 1, 2022
@blackdevelopa blackdevelopa marked this pull request as ready for review December 12, 2022 16:04
@blackdevelopa blackdevelopa requested a review from a team as a code owner December 12, 2022 16:04
@blackdevelopa blackdevelopa marked this pull request as draft December 12, 2022 16:12
@blackdevelopa blackdevelopa force-pushed the ft/verify_contract_details branch from c8722af to 64c357b Compare December 13, 2022 15:51
@blackdevelopa blackdevelopa marked this pull request as ready for review December 13, 2022 15:51
@blackdevelopa blackdevelopa force-pushed the ft/verify_contract_details branch from b2a5bc1 to ede833e Compare December 14, 2022 23:39
blackdevelopa
blackdevelopa commented Dec 14, 2022
View reviewed changes
@blackdevelopa blackdevelopa added team-confirmations-secure-ux-deprecated DEPRECATED: please use "team-confirmations" instead needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Dec 15, 2022
@blackdevelopa blackdevelopa force-pushed the ft/verify_contract_details branch from c4de4cd to ebc52c5 Compare January 12, 2023 10:40
jpuri
jpuri reviewed Jan 13, 2023
View reviewed changes
@bschorchit bschorchit removed the team-confirmations-secure-ux-deprecated DEPRECATED: please use "team-confirmations" instead label Jan 18, 2023
@blackdevelopa blackdevelopa added the team-confirmations-secure-ux-PR PR from the confirmations team label Jan 24, 2023
@blackdevelopa blackdevelopa force-pushed the ft/verify_contract_details branch from 7763f15 to 3b5d743 Compare February 7, 2023 08:38
tommasini
tommasini reviewed Feb 7, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

Hey, @blackdevelopa great job here!

Left some questions/suggestions.

tommasini
tommasini approved these changes Feb 7, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM!

@blackdevelopa blackdevelopa force-pushed the ft/verify_contract_details branch from b3c58d0 to ac619bb Compare February 14, 2023 05:52
jpuri
jpuri reviewed Feb 15, 2023
View reviewed changes
jpuri
jpuri approved these changes Feb 15, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

I added some NIT feedbacks.

PR is huge and touches important part of code. Thus QA is very important.

For complex changes smaller PRs with test coverage help a lot.

@blackdevelopa blackdevelopa 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) labels Feb 15, 2023
@blackdevelopa blackdevelopa force-pushed the ft/verify_contract_details branch from 9a57539 to 177e431 Compare February 15, 2023 20:00
@blackdevelopa blackdevelopa force-pushed the ft/verify_contract_details branch from ee61f13 to dc82fd7 Compare March 3, 2023 16:49
@seaona seaona added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed release-6.2.0 Issue or pull request that will be included in release 6.2.0 labels Mar 6, 2023
@seaona
Copy link
Copy Markdown
Member

seaona commented Mar 6, 2023

The issue above is fixed. Few things:

  • This PR needs to go over the dev review process again before being merged, so I've added the needs-red-review label
  • This PR cannot be introduced in the 6.2 release, as we need to fix the pending issues (the different tickets that @bschorchit created)
  • Once all issues are fixed, all the corresponding PRs should target the same release and we can add the release label

Let me know if this sounds good @bschorchit @blackdevelopa

@bschorchit bschorchit mentioned this pull request Mar 7, 2023
Closed
@bschorchit
Copy link
Copy Markdown
Contributor

bschorchit commented Mar 9, 2023 •
edited
Loading

The issue above is fixed. Few things:

  • This PR needs to go over the dev review process again before being merged, so I've added the needs-red-review label

Would you mind giving this a final re-review once you have a chance, @jpuri ? Once this is re-reviewed, we'll merge this PR and address the non-blocking issues found during QA as part of the tickets tagged above. 🧡

@jpuri
Copy link
Copy Markdown
Contributor

jpuri commented Mar 9, 2023

Looks good for merge 👍

cc @bschorchit , @blackdevelopa

@jpuri jpuri closed this Mar 9, 2023
@jpuri jpuri reopened this Mar 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 9, 2023
@jpuri
Copy link
Copy Markdown
Contributor

jpuri commented Mar 9, 2023

Plz ignore PR closing, that was accidental.

@blackdevelopa blackdevelopa removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 9, 2023
@bschorchit
Copy link
Copy Markdown
Contributor

bschorchit commented Mar 10, 2023

We're now following the practice of 2 reviews for confirmations mobile PRs, would you mind also giving this a re-review to check the latest changes @tommasini? Thank you 🧡

tommasini
tommasini requested changes Mar 10, 2023
View reviewed changes
@tommasini
Copy link
Copy Markdown
Contributor

tommasini commented Mar 10, 2023

Thanks for the tag @bschorchit!
Let me know your thoughts @blackdevelopa 🙌

@blackdevelopa
Copy link
Copy Markdown
Contributor Author

blackdevelopa commented Mar 10, 2023

Thank you @tommasini. Good catch.

tommasini
tommasini approved these changes Mar 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM! Awesome work! 🚀

@bschorchit
Copy link
Copy Markdown
Contributor

bschorchit commented Mar 10, 2023

🚀🚀🚀

@bschorchit bschorchit added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Mar 10, 2023
@seaona
Copy link
Copy Markdown
Member

seaona commented Mar 13, 2023

Looks good! I merge the PR.
Notice, for SetApprovalForAll and Revoke custom screens, we should also add the Verify Contract Details. At the moment, we don't have custom screens for these cases on Mobile, but whenever we add them, we should take this into account.

image

cc @bschorchit @blackdevelopa

@seaona seaona merged commit f46e93e into main Mar 13, 2023
@seaona seaona deleted the ft/verify_contract_details branch March 13, 2023 10:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jpuri jpuri jpuri approved these changes

@tommasini tommasini tommasini approved these changes

Assignees

@blackdevelopa blackdevelopa

Labels

needs-qa Any New Features that needs a full manual QA prior to being added to a release. QA'd but questions A QA run through has been done but you need clarification on minor issues you found team-confirmations-secure-ux-PR PR from the confirmations team unit test coverage confirmed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@blackdevelopa @seaona @bschorchit @jpuri @tommasini @cortisiko @hilvmason