metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Fix Issue #23801

Open ejwessel opened this issue 10 months ago • 8 comments

Description

#23801

Open in GitHub Codespaces

Related issues

Fixes: Screenshot 2024-04-20 at 3 07 07 PM

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • [ ] I’ve followed MetaMask Coding Standards.
  • [ ] I've completed the PR template to the best of my ability
  • [ ] I’ve included tests if applicable
  • [ ] I’ve documented my code using JSDoc format if applicable
  • [ ] I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

  • [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • [ ] 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.

ejwessel avatar Apr 20 '24 22:04 ejwessel

CLA Signature Action:

Thank you for your submission, we really appreciate it. We ask that you read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:

I have read the CLA Document and I hereby sign the CLA

By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to this repository.

0 out of 1 committers have signed the CLA. :x: @ejwessel

github-actions[bot] avatar Apr 20 '24 22:04 github-actions[bot]

I haven't tested yet but im wondering if similar issue can happen for ERC721?

sahar-fehri avatar Apr 23 '24 08:04 sahar-fehri

@sahar-fehri I haven't seen this issue with ERC721 yet. Good reminder! Will double-check.

sleepytanya avatar Apr 25 '24 21:04 sleepytanya

@sahar-fehri I can't send ERC721 - every attempt results in frozen MM (429 errors) before I can actually see the next screen.

https://github.com/MetaMask/metamask-extension/assets/104780023/50b2966e-e3d2-4572-9584-66c5144a021e

The fix is working great!

https://github.com/MetaMask/metamask-extension/assets/104780023/1b4d933c-d387-4394-9fcd-e515bb2f908c

May be it's worth asking @hesterbruikman what message we want to display: 'Insufficient tokens' or 'Cannot send negative or zero amount of tokens'.

sleepytanya avatar Apr 26 '24 00:04 sleepytanya

Thanks @sleepytanya ! The 429 you got is curious, i wasnt able to reproduce, and it seems okay for ERC721, the UI will display "balance: 1 token"

https://github.com/MetaMask/metamask-extension/assets/10994169/6b48e991-d475-4957-b205-cc2cafb313f5

sahar-fehri avatar May 02 '24 13:05 sahar-fehri

@sahar-fehri 429 Rate-Limiting errors are common for users coming through e.g. Tor or commercial VPN services. As an aside, in general we shouldn't assume availability of these endpoints and fail gracefully when they aren't available.

legobeat avatar May 02 '24 22:05 legobeat

Unrelated to my change, the tests are inconsistent and often fail when an address is selected or inserted as the recipient.

Common errors are:

-----Received an error from Chrome-----
SES_UNHANDLED_REJECTION: (TypeError#2)
----------End of Chrome error----------

-----Received an error from Chrome-----
TypeError#2: Cannot set properties of undefined (setting 'quotes')
----------End of Chrome error----------
-----Received an error from Chrome-----
(BigNumber Error#1)
----------End of Chrome error----------

-----Received an error from Chrome-----
BigNumber Error#1: new BigNumber() not a number: 2540be400
----------End of Chrome error----------

ejwessel avatar May 07 '24 15:05 ejwessel

Change was cherry picked out.

ejwessel avatar May 08 '24 17:05 ejwessel