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

[Bug] Verify third party details causes to watch the asset undesirably #7178

Closed
seaona opened this issue Sep 8, 2023 · 2 comments
Closed
Assignees
Labels
Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead type-bug Something isn't working

Comments

@seaona
Copy link
Contributor

seaona commented Sep 8, 2023

Describe the bug
Whenever you are Approving a token, if you click on Verify third party details the following behaviour happens:

  • The token is added to the wallet. I wouldn't expect checking 3rd party details to add the token to the wallet. Seems a bug cc @bschorchit
  • In the case of an ERC20, the token is added correctly
  • In the case of an ERC721, the token is added as if it was an ERC20, making it appear on the Tokens tab (not the NFT tab) and with a weird amount. Furthermore, the token is displayed with the name ERC20 Token which is incorrect ,and seems related to this other bug [Bug] Approve an ERC721 token displays as ERC20 token as the contract name fallback #6273

Screenshots

verify-3rd-party-details.mp4

To Reproduce

  1. Go to the test dapp
  2. Deploy an ERC721 token
  3. Click Approve
  4. Click Third party details
  5. Cancel
  6. Go to the Home wallet
  7. See how an ERC20 token is now added to your wallet

Expected behavior
My expectation would be that no token is added at all, whenever we check the third party details.
In case it's added, it should be added correctly (under NFTs) and with the correct name, not ERC20

Smartphone (please complete the following information):

  • Device: Pixel 6
  • OS: Android
  • App Version : prod

to be added after bug submission by internal support / PM
Severity

  • How critical is the impact of this bug on a user?
  • Add stats if available on % of customers impacted
  • Is this visible to all users?
  • Is this tech debt?
@seaona seaona added type-bug Something isn't working team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead labels Sep 8, 2023
@seaona seaona changed the title [Bug] Verify third party details causes to watch the asset undesiredly [Bug] Verify third party details causes to watch the asset undesirably Sep 8, 2023
@bschorchit bschorchit added the Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking label Sep 11, 2023
@blackdevelopa blackdevelopa self-assigned this Oct 6, 2023
@blackdevelopa
Copy link
Contributor

Hey @seaona, I think we should separate this issue into two different issues? Wdyt?

The first should fix the bug that adds the token when only trying to verify third party details. We have a fix here. It prevents calling addToken on the approve screen.

The second issue should then enable watching erc721 token on mobile which I don't think we currently support.

PS: [Bug] Approve an ERC721 token displays as ERC20 token as the contract name fallback is fixed here

@seaona
Copy link
Contributor Author

seaona commented Oct 9, 2023

hi @blackdevelopa thanks for your comment, this makes sense to me 👍 About the second issue;

The second issue should then enable watching erc721 token on mobile which I don't think we currently support.

I think adding support to watch ERC721 falls under Mobile Core team, so I think we can leave that part outside Confirmations scope. cc @bschorchit

blackdevelopa added a commit that referenced this issue Oct 10, 2023
…#7410)

## **Description**
When approving a token and you click on `Verify third party details` it
adds the token to the wallet. This PR prevents calling `addToken` on the
token approve screen.

## **Manual testing steps**
1. Go to the test dapp
2. Deploy an ERC721/ERC20 token
3. Click Approve
4. Click Verify Third party details
5. Cancel
6. Go to the Home wallet
7. You shouldn't see a new token added to your wallet.

## **Screenshots/Recordings**

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

Bitrise:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/e072c203-f63d-4115-bf86-768408fe083e

### **Before**


https://github.com/MetaMask/metamask-mobile/assets/54408225/00e92cf0-948f-47d4-bb7a-b604d361279d

### **After**



https://github.com/MetaMask/metamask-mobile/assets/29962968/cba721d4-8b69-49f9-a1ae-ed22f1fa7ad4


## **Related issues**

_Fixes #7178 

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained:
  - [x] What problem this PR is solving.
  - [x] How this problem was solved.
  - [x] How reviewers can test my changes.
- [x] I’ve indicated what issue this PR is linked to: Fixes #???
- [x] I’ve included tests if applicable.
- [x] I’ve documented any added code.
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead type-bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants