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

Verify Contract Details #5327

Merged
merged 24 commits into from
Mar 13, 2023
Merged

Verify Contract Details #5327

merged 24 commits into from
Mar 13, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Dec 1, 2022

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
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 marked this pull request as ready for review December 13, 2022 15:51
@blackdevelopa blackdevelopa added team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Dec 15, 2022
@bschorchit bschorchit removed the team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead label Jan 18, 2023
@blackdevelopa blackdevelopa added the team-confirmations-secure-ux-PR PR from the confirmations team label Jan 24, 2023
Copy link
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.

Copy link
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!

app/constants/address.ts Outdated Show resolved Hide resolved
Copy link
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
@seaona seaona added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed release-6.2.0 labels Mar 6, 2023
@seaona
Copy link
Contributor

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
Copy link

bschorchit commented Mar 9, 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

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
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
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

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
Copy link
Contributor

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

@blackdevelopa
Copy link
Contributor Author

Thank you @tommasini. Good catch.

Copy link
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

🚀🚀🚀

@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
Contributor

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.
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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants