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]: Simulations should handle minting NFTs #24251

Closed
dbrans opened this issue Apr 25, 2024 · 1 comment · Fixed by #24306
Closed

[BUG]: Simulations should handle minting NFTs #24251

dbrans opened this issue Apr 25, 2024 · 1 comment · Fixed by #24306
Assignees
Labels
INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. release-11.17.0 Issue or pull request that will be included in release 11.17.0

Comments

@dbrans
Copy link
Contributor

dbrans commented Apr 25, 2024

Background

In simulations on the client, if a transaction simulation indicates a change in the user's balance for a non-native token , we rerun the simulation but sandwich the original transaction between two other transactions that check the user's balance before and after the transaction for the given token.

This "sandwich" method is thought to be more reliable than the original simulation's trace logs.

Problem

In the case of a transaction that would mint an NFT, the token id does not exist before the transaction runs and checking the user's balance for a non-existing token id will revert.

This is causing simulation errors on the client because the client is not anticipating a revert since the original transaction did not revert.

Furthermore, since Sentinel correctly reports the revert, this issue is contributing to a current large error-rate discrepancy between client and server (double-digit percent vs half-a-percent).


Here is what a revert looks like for a token ID that does not exist yet:

{
  "jsonrpc": "2.0",
  "result": {
    "transactions": [
      {
        "error": "execution reverted: ERC721: invalid token ID",
        "return": "0x08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000184552433732313a20696e76616c696420746f6b656e2049440000000000000000",
        "status": "0x0"
      }
    ],
    "blockNumber": "0xd02556",
    "id": "0bbcda56-afaa-479a-ab46-4171cc33ef13"
  },
  "id": "5"
}
@dbrans dbrans self-assigned this Apr 25, 2024
@metamaskbot metamaskbot added the INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. label Apr 25, 2024
@dbrans
Copy link
Contributor Author

dbrans commented Apr 25, 2024

This is only a problem for ERC721 where we check the user's balance using ownerOf.

dbrans added a commit that referenced this issue Apr 30, 2024
…on (#24306)

## **Description**
Update transaction-controller core package to bring in this fix to NFT
mint simulation: MetaMask/core#4217

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24306?quickstart=1)

## **Related issues**

Fixes: #24251

## **Manual testing steps**

1. Visit https://www.fxhash.xyz/generative/slug/g-l-y-p-h 
2. Try minting the nft
3. Simulation should work and show the nft in the "You receive" section.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

(Simulation Failed message)

### **After**


![image](https://github.com/MetaMask/metamask-extension/assets/507015/ded0173a-088c-4f12-b3b6-09b823db7756)

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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.
@metamaskbot metamaskbot added the release-11.17.0 Issue or pull request that will be included in release 11.17.0 label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. release-11.17.0 Issue or pull request that will be included in release 11.17.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants