metamask-extension
metamask-extension copied to clipboard
MetaMask
fix: Can't call setApprovalForAll on certain NFTs
Description
Fixes an issue where users can't list certain NFTs because MetaMask crashes with:
Invalid data; should be 'approve' method, but instead is 'setApprovalForAll'
The repro we have for this, is when MetaMask wrongly categorizes an NFT as an ERC20 token. I don't have a good repro of how this occurs. Some could be grandfathered in before standard detection / guards were added. But it's possible it can still happen.
EDIT: ERC404 tokens are one repro. These kinds of tokens can be treated as fungible and non-fungible. But it also occurred before that standard existed.
When MetaMask receives the request to setApprovalForAll for the NFT, it mistakenly enters an ERC20 approval flow and crashes. This doesn't make much sense, because the setApprovalForAll function signature only appears on ERC721/ERC1155, not ERC20.
My fix is to not enter the ERC20 approval flow when the function signature is setApprovalForAll, and to use the NFT approval page instead. This relies on what the dapp prompted instead of our categorization of the token standard.
Related issues
Fixes: https://github.com/MetaMask/metamask-extension/issues/18507
Manual testing steps
TODO trying to repro naturally
Screenshots/Recordings
Before
After
https://github.com/MetaMask/metamask-extension/assets/3500406/d8342357-0eaf-41fd-bac2-71b58d3a34df
Pre-merge author checklist
- [ ] I’ve followed MetaMask Coding Standards.
- [ ] I've clearly explained what problem this PR is solving and how it is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] 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.
- [ ] I’ve properly set the pull request status:
- [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft".
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.
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.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Builds ready [fd53422]
- builds: chrome, firefox
- builds (beta): chrome
- builds (flask): chrome, firefox
- builds (MMI): chrome, firefox
- builds (test): chrome, firefox
- builds (test-flask): chrome, firefox
- build viz: Build System
- mv3: Background Module Init Stats
- mv3: UI Init Stats
- mv3: Module Load Stats
- mv3: Bundle Size Stats
- mv2: E2e Actions Stats
- code coverage: Report
- storybook: Storybook
- typescript migration: Dashboard
- all artifacts
Page Load Metrics (832 ± 39 ms)
| Platform | Page | Metric | Min (ms) | Max (ms) | Average (ms) | StandardDeviation (ms) | MarginOfError (ms) |
|---|---|---|---|---|---|---|---|
| Chrome | Home | firstPaint | 93 | 296 | 126 | 43 | 21 |
| domContentLoaded | 10 | 205 | 28 | 41 | 20 | ||
| load | 706 | 1015 | 832 | 80 | 39 | ||
| domInteractive | 10 | 205 | 28 | 41 | 20 |
Bundle size diffs
- background: 0 Bytes (0.00%)
- ui: 3 Bytes (0.00%)
- common: 0 Bytes (0.00%)
Codecov Report
Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.
Project coverage is 68.46%. Comparing base (
0b52d3d) to head (fd53422). Report is 225 commits behind head on develop.
:exclamation: Current head fd53422 differs from pull request most recent head 3750ae1. Consider uploading reports for the commit 3750ae1 to get more accurate results
| Files | Patch % | Lines |
|---|---|---|
| ui/pages/confirm-approve/confirm-approve.js | 0.00% | 4 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## develop #22822 +/- ##
===========================================
- Coverage 68.48% 68.46% -0.03%
===========================================
Files 1088 1088
Lines 42886 42828 -58
Branches 11418 11397 -21
===========================================
- Hits 29370 29318 -52
+ Misses 13516 13510 -6
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
If you need to repro, just launch any "ERC404" type of NFT, which is fundamentally broken at many levels but still getting support from the ecosystem due to hype, and it will fall into this trap guaranteed.
Builds ready [3750ae1]
- builds: chrome, firefox
- builds (beta): chrome
- builds (flask): chrome, firefox
- builds (MMI): chrome, firefox
- builds (test): chrome, firefox
- builds (test-flask): chrome, firefox
- build viz: Build System
- mv3: Background Module Init Stats
- mv3: UI Init Stats
- mv3: Module Load Stats
- mv3: Bundle Size Stats
- mv2: E2e Actions Stats
- code coverage: Report
- storybook: Storybook
- typescript migration: Dashboard
- all artifacts
Page Load Metrics (744 ± 13 ms)
| Platform | Page | Metric | Min (ms) | Max (ms) | Average (ms) | StandardDeviation (ms) | MarginOfError (ms) |
|---|---|---|---|---|---|---|---|
| Chrome | Home | firstPaint | 83 | 132 | 97 | 10 | 5 |
| domContentLoaded | 14 | 33 | 16 | 4 | 2 | ||
| load | 716 | 831 | 744 | 26 | 13 | ||
| domInteractive | 14 | 33 | 16 | 4 | 2 |
Bundle size diffs
- background: 0 Bytes (0.00%)
- ui: 3 Bytes (0.00%)
- common: 0 Bytes (0.00%)
Any movement on this if it fixed https://github.com/MetaMask/metamask-extension/issues/18507 it would be a great PR to get over the line, plenty of folk stuck on the approval bug for old NFT collections and MM browser
