[pull] main from MetaMask:main#569
Merged
pull[bot] merged 3 commits intoReality2byte:mainfrom May 7, 2026
Merged
Conversation
…ecoding (#8723) ## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> Follow-up to #8130. Extends the first-time-interaction recipient-decoding fix to ERC721/ERC1155 `safeTransferFrom` transfers, which were not covered by the original change and silently suppress the first-time-interaction warning when sending NFTs to a new recipient. ## Problem `getEffectiveRecipient` in `first-time-interaction.ts` only treated `tokenMethodTransfer` and `tokenMethodTransferFrom` as token-transfer types. For `tokenMethodSafeTransferFrom` (ERC721 `safeTransferFrom(from,to,tokenId)`, ERC721 `safeTransferFrom(from,to,tokenId,data)`, and ERC1155 `safeTransferFrom`/`safeBatchTransferFrom`), it fell through to `txParams.to` — i.e. the NFT contract address — instead of decoding the actual recipient. This caused two failure paths, both suppressing the warning: 1. **API call uses contract as recipient.** `getAccountAddressRelationship` was queried with the NFT contract address as `to`. Any popular collection returns `count > 0`, so `isFirstTimeInteraction = false` and the alert never fires. 2. **Existing-transaction short-circuit matches on contract identity.** Once a user had any prior `safeTransferFrom` to the same NFT contract, `existingTransactions.find()` matched on contract address and skipped the API call entirely, even when the new transfer's recipient was a never-before-seen address. Net effect: the first-time-interaction warning was silently suppressed for NFT transfers — including transfers to lookalike/attacker addresses. ## Solution Add `TransactionType.tokenMethodSafeTransferFrom` to `TOKEN_TRANSFER_TYPES` so `getEffectiveRecipient` decodes the recipient from `txParams.data`. The existing `parsed?.args?._to ?? parsed?.args?.to ?? to` fallback chain already handles all three relevant ABI shapes (ERC721 3-arg, ERC721 4-arg with `bytes`, and ERC1155 `safeTransferFrom`/`safeBatchTransferFrom`). Comments updated to reflect that the rationale applies to ERC20/ERC721/ERC1155, not just ERC20. ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> Fixes https://consensyssoftware.atlassian.net/browse/CONF-1350 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes first-time-interaction recipient matching for NFT `safeTransferFrom` transfers, which impacts when security warnings are shown/skipped and could affect user-facing alerts. Scope is small and covered by new unit tests for multiple `safeTransferFrom` argument shapes and existing-transaction de-duplication behavior. > > **Overview** > Fixes first-time-interaction checks for NFT/token `safeTransferFrom` by treating `TransactionType.tokenMethodSafeTransferFrom` as a token-transfer type and decoding the *effective recipient* from `txParams.data` instead of using the token contract address. > > Adds unit tests covering recipient extraction for ERC721 (3-arg and 4-arg) and ERC1155 `safeTransferFrom`, plus ensures the existing-transaction short-circuit compares decoded recipients (not just the contract) for `safeTransferFrom`. Updates the package changelog to document the behavior change. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5c03cea. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
… added (#8714) ## Explanation `TransactionPayController.parseRequiredTokens` returns an empty array when token info or fiat rates are unavailable. Previously the only retry trigger was a `txParams.data` change, leaving in-flight transactions deadlocked when clients gate calldata edits on having a resolved required token. This change subscribes to asset state changes (or `AssetsController` when the unify flag is enabled) and re-parses required tokens for in-flight transactions whose tokens are still empty. ## References ## Changelog <!-- THIS SECTION IS NO LONGER NEEDED. The process for updating the changelog has changed. Please review the pull request guidelines: https://github.com/MetaMask/core/blob/main/docs/contributing.md#pull-request-guidelines --> ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "**BREAKING**" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Introduces new messenger event subscriptions (and expands `AllowedEvents`), which is a breaking integration change for consumers and can affect runtime behavior via additional state-change handling. > > **Overview** > Ensures in-flight transactions retry `parseRequiredTokens` when asset data becomes available by adding `subscribeAssetChanges` and wiring it into `TransactionPayController` to re-run token parsing on rate/token state updates (or `AssetsController` updates when unify-state is enabled). > > Renames `pollTransactionChanges` to `subscribeTransactionChanges`, updates controller/tests accordingly, and expands `AllowedEvents` with the required asset/rate state-change events (**BREAKING**: consumers must grant these events). Adds extra diagnostics in `required-tokens` when calldata decoding or token metadata/rates are missing. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit a25d13a. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Major release of `@metamask/transaction-pay-controller`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Although the diff is mostly version/changelog updates, this is a major bump for `@metamask/transaction-pay-controller` and may break consumers (new required `AllowedEvents` permissions). > > **Overview** > Cuts release `966.0.0` and publishes `@metamask/transaction-pay-controller` `22.0.0`. > > Updates the `transaction-pay-controller` changelog to record **breaking** messenger event-permission additions for asset-state-driven required-token re-parsing, dependency bumps, and a fix to ensure the fiat strategy can be selected/quoted. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 132d606. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )