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

Clicking toAddress to add it to address book #6137

Merged
merged 29 commits into from
May 16, 2023
Merged

Clicking toAddress to add it to address book #6137

merged 29 commits into from
May 16, 2023

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Apr 6, 2023

Fixes: #6008

A reusable component is created to wrap child elements, the child elements when clicked will trigger add-to-address-book modal to open.

QA steps:
Add to address book works well at:

  • send page where to address is selected
  • confirm send page
  • any DAPP transaction

@jpuri jpuri added the team-confirmations-secure-ux-PR PR from the confirmations team label Apr 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2023

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.

@jpuri jpuri marked this pull request as ready for review April 7, 2023 06:53
@jpuri jpuri requested a review from a team as a code owner April 7, 2023 06:53
Copy link
Contributor

@blackdevelopa blackdevelopa left a comment

Choose a reason for hiding this comment

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

LGTM. Just left small nit

blackdevelopa
blackdevelopa previously approved these changes Apr 12, 2023
segun
segun previously approved these changes Apr 13, 2023
@jpuri jpuri added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 13, 2023
@seaona
Copy link
Contributor

seaona commented Apr 21, 2023

Address saved not rendered on ERC20 Token Transfers

Whenever I am sending an ERC20 token, I can see how clicking toAddress allows me to save the address, but whenever I save it, the Account name is not displayed. It is still displayed the address.

Repro

  1. Go to test dapp
  2. Click Create Token for deploying an ERC20 Token
  3. Add token to Wallet
  4. Click Transfer
  5. Click toAddress
  6. Save an account name
  7. See how the address is still displayed

This also happens if I try to Send the token from inside MetaMask instead of the dapp.
If I check the address book, I can see how the address has been saved correctly.

erc20-token-transfer-address-not-rendered.mp4

Add to Address Book does not open with ERC721 Sends

Whenever we are trying to send an ERC721 token, when I am on the Confirmation screen, clicking the To Address does not open the Add to Address Book modal.

erc721-address-not-open.webm

Since there is another issue related to SEnd erc721, it might have some effect on what I am seeing, so maybe after this issue is fixed, this one will be resolved

Repro

  1. Go to test dapp
  2. Deploy Collectibles contract
  3. Mint a token
  4. Go to the Wallet
  5. Click NFTs tab
  6. Add the NFT you just minted
  7. Click the NFT
  8. Click Send
  9. Add a recipient
  10. Proceed
  11. On the last confirmation screen, click To Address -- see nothing happens

Add to Address Book with ERC1155 Sends

Pending to test, as need there is an issue that should be resolved first, otherwise I cannot import any ERC1155 adn test it.

@seaona seaona removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 21, 2023
@jpuri
Copy link
Contributor Author

jpuri commented Apr 24, 2023

@seaona : PR is updated with fix, can you plz re-test.

The test should fix the issue for all transactions including 1155.

@jpuri jpuri added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 24, 2023
@seaona seaona removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 25, 2023
@Cal-L Cal-L dismissed stale reviews from segun and blackdevelopa via ebce34c May 15, 2023 18:13
@seaona
Copy link
Contributor

seaona commented May 16, 2023

@jpuri I see the issues fixed! I've checked the ERC721 in Mainnet since I couldn't import tokens manually due to the previous known issue (see ticked blocked by).
I could use the auto-detect NFTs for overcoming it. cc @bschorchit

@seaona seaona added QA Passed A successful QA run through has been done release-6.7.0 6.7.0 release tickets labels May 16, 2023
@jpuri jpuri requested review from segun and blackdevelopa May 16, 2023 13:16
Copy link
Contributor

@segun segun left a comment

Choose a reason for hiding this comment

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

LGTM

@seaona seaona merged commit f1922a8 into main May 16, 2023
15 checks passed
@seaona seaona deleted the add_to_address_book branch May 16, 2023 15:18
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2023
@cortisiko cortisiko added release-7.1.0 and removed release-6.7.0 6.7.0 release tickets labels May 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.1.0 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.

[Bug] Recipient Address component not clickable nor able to be added to the Address Book
5 participants