feat: block MetaMask Pay submissions without quotes and handle no-op quotes cp-8.3.0#33194
Conversation
|
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. |
PR template — items to address before "Ready for review"Warnings — informational, address before merging:
See docs/readme/ready-for-review.md for the full Definition of Ready for Review. |
dan437
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…oller to 69.0.0 The new controller stores a no-op quote when a route needs no conversion. Use the TransactionPayStrategy.None enum directly now that it is available.
…ersions-without-quotes # Conflicts: # package.json # yarn.lock
…ests The publish guard now counts only executable quotes, matching the confirmation alerts which already filter no-op quotes. Direct routes are validated from controller state. Adds tests for the publish guard, the new alert legs, and the no-op quote metrics.
github-actions
Bot
added
size-L
risk:high
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning MetaMask internal reviewing guidelines:
|
MetaMask#9484) ## Explanation `TransactionPayController` stores an empty quotes array both when no conversion is needed and when a quote could not be fetched, so at publish time the two cases cannot be told apart. This PR makes the distinction explicit: - Store a no-op quote (`TransactionPayStrategy.None`) when a payment token is selected but the route needs no conversion. The no-op quote is built inside `getQuotes` when there are no quote requests, and is skipped when `isQuoteRequired` is set, since an empty request list then means source amounts could not be calculated. - No-op quotes are not routable: the quote refresher skips them, they are excluded from totals and the external-sign flag, and `TransactionPayPublishHook` filters them out before selecting a strategy. - Clients can rely on `quotes.length > 0` whenever MetaMask Pay is engaged, and enforce quote presence at publish time on their side. ## References - Mobile adoption: MetaMask/metamask-mobile#33194 ## 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 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) - [x] 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 quote shape and publish-time behavior (breaking for quote consumers) in the transaction pay path, but logic is guarded with filters and broad tests rather than touching auth or funds directly. > > **Overview** > Direct MetaMask Pay routes (payment token selected, no conversion) now persist a **no-op quote** with strategy `none` instead of an empty `quotes` array, so **“no conversion needed”** is distinguishable from **“quote still missing.”** > > `getQuotes` builds that marker via `buildNoOpQuote` when there are no quote requests and `isQuoteRequired` is false; fiat-only empty cases and quote-required flows still clear or omit it. **`TransactionPayStrategy.None` is not routable**—`isTransactionPayStrategy` excludes it, the publish hook skips execution, quote refresh/polling ignores only-no-op state, and fee totals / `hasQuotes` sync use **executable** quotes only. > > **Breaking:** downstream clients that treat any entry in `quotes` as a real pay quote must ignore or handle `strategy: 'none'`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 30646eb. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Withdraws with no preferred or last-used token intentionally leave paymentToken unset and default to a direct, same-token transfer (see getBestToken). The withdraw-not-initialised alert and the publish guard both required a payment token to be set even in this safe case, blocking legitimate withdraws that never engage the token picker. Both now only require isPostQuote (and, for the guard, no pending conversion) to allow direct submission. The real danger case, a different destination token chosen but not yet quoted, stays covered by the existing sourceAmounts-based checks. Also deduplicates yarn.lock entries left behind by the earlier merge.
…ersions-without-quotes # Conflicts: # yarn.lock
…ersions-without-quotes
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cefe5d2. Configure here.
…check Fiat-funded pay deposits (perps/predict/mUSD) auto-submit right after the on-ramp order is created, before any pay token is ever selected on the controller, so the pay-token-required publish guard always threw for them. Also fix a checksum-vs-lowercase mismatch that could mark an optional conversion as required and block a valid direct submission.
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: Performance Test Selection: |
|
⚡ Performance Test Results
✅ All tests passed · 5 tests · 1 device 📱 Devices tested (1)Android: Google Pixel 8 Pro (v14.0) ✅ Passed Tests (5)
Branch: |
metamask-ci
Bot
added
the
release-8.4.0
2 participants
Description
Mobile side of MetaMask/core#9484, which makes "no conversion needed" an explicit no-op quote and requires quotes at publish time for MetaMask Pay transactions.
@metamask/transaction-pay-controllerto^25.0.0(includes the no-op quote change) and@metamask/transaction-controllerto69.0.0(required by the new pay controller).validateRequiredQuotenow also requires quotes for post-quote withdraw types when the post-quote flag is enabled; direct routes are allowed only when the controller state confirms them.validateRequiredQuotealso covers pay deposit and conversion types (perps deposits, predict deposits, mUSD conversion): they may submit without a quote only when the user pays with the required token itself and no conversion is pending, or the deposit is funded by a fiat on-ramp order.useNoPayTokenQuotesAlert: withdraw flows where the pay config or destination token is not set on the controller yet, and pay deposit/conversion flows where neither a payment token nor a fiat payment method is set.none) are filtered everywhere: fee, duration, and step UI, metrics, and the publish guard. A no-op quote never counts as an executable quote; direct routes are validated from controller state instead, keeping the publish guard consistent with the confirmation alerts.mm_pay_quote_skipped; strategy and step totals count only executable quotes.Changelog
CHANGELOG entry: Improved quote validation for MetaMask Pay deposits, conversions and withdrawals
Related issues
Refs: MetaMask/core#9484
Manual testing steps
Screenshots/Recordings
N/A
Before
N/A
After
N/A
Pre-merge author checklist
Performance checks (if applicable)
trace()for usage andaddTokenfor an exampleFor performance guidelines and tooling, see the Performance Guide.
Pre-merge reviewer checklist
Note
High Risk
Changes transaction publish and confirmation blocking for MetaMask Pay funding paths; incorrect direct-route exceptions could still allow unfunded submissions or block legitimate flows.
Overview
Bumps
@metamask/transaction-pay-controllerto ^25.0.0 (no-op quotes for direct routes) and aligns mobile with stricter publish-time and confirmation-time Pay guards.No-op quotes (
strategy: none) are treated as non-executable everywhere: the quote selector filters them for UI,validateRequiredQuoteand metrics only count executable quotes, andmm_pay_quote_skippedis set when no-op quotes were present.Publish (
validateRequiredQuote) now also applies to pay-token-required deposits/conversions (perps/predict/mUSD, etc.) and post-quote withdraws when the feature flag is on. Submission without an executable quote is allowed only for validated direct routes (required token, no pending conversion), fiat orders withorderId, or post-quote withdraws withisPostQuoteand no pendingsourceAmounts—including withdraws with no destination token selected.Confirmation adds blocking
NoPayTokenQuotescases: withdraws with token selection enabled before Pay config is initialised (!isPostQuote), and pay-token-required types with no payment token and no fiat method—mirroring publish rules so users cannot confirm timing races that used to submit unfunded txs.Reviewed by Cursor Bugbot for commit cd7b38f. Bugbot is set up for automated code reviews on this repo. Configure here.