fix(transaction-pay-controller): keep max quotes non-atomic for MFA keyrings by jpuri · Pull Request #11389 · MetaMask/core

jpuri · 2026-11-09T12:15:28Z

Explanation

getRelayMaxQuote promotes a non-atomic max quote back to atomic whenever the route turns out to be subsidized, to secure the better quote. That promotion is correct for its intended case and should stay — but it is wrong for flows where embedding the target calls is incorrect rather than merely suboptimal.

An atomic quote embeds the target calls, which requires a fresh ERC-7710 delegation signature per quote. Quotes refresh continuously while an amount screen is open. When the signing account's keyring requires multi-factor authentication, every one of those signatures costs the user an MFA confirmation, which makes the screen unusable.

The motivating case is MetaMask Mobile's Money Account deposits. Verified on device against this package: with a partial amount no money-account delegation is signed, but with Max on a subsidized route the quote is promoted and the per-refresh signature reappears.

Solution

The pay controller already resolves an account's keyring through KeyringController:getState in accountSupports7702, so it can answer "does this account require MFA to sign" itself. accountRequiresMfa (utils/keyring.ts) mirrors that helper, and getRelayMaxQuote skips both promotion paths when it returns true.

Only MPC Money Keyring is treated as MFA. The SRP-derived Money Keyring signs locally and keeps the existing atomic-max subsidy optimisation.

The account checked is `transaction.txParams.from`, not `request.from`

This is the one non-obvious part and the easiest thing to "simplify" incorrectly later.

The delegation is signed by the account the target calls execute from, which is the transaction's own from. getFrom (utils/quotes.ts:237) resolves request.from to accountOverride ?? txParams.from, and Money Account deposits set accountOverride to the funding EOA. So request.from is the payer — an ordinary HD keyring — while txParams.from is the money account whose keyring actually signs.

Reusing the resolved from, as accountSupports7702's call site does, would return false and leave the promotion firing for exactly the accounts this protects. There is a comment at the call site explaining this.

Why not a client flag

The first revision of this PR added an atomicForbidden field to TransactionConfig, plumbed through TransactionData and QuoteRequest. That was replaced following review feedback (#discussion_r4512819137): the keyring is already reachable from inside the controller, so the public API addition and its plumbing were unnecessary, and encapsulating it means no client can forget to set it. The net diff is now smaller than the flag version and requires no client change at all.

Compatibility

No public API change. Behaviour changes only for accounts held by an MPC keyring, which no client can currently select at runtime, so this is inert on merge and correct once MFA accounts ship.

Testing

Two cases added to the existing subsidized max Money Account promotion suite:

an MFA keyring holding txParams.from is not promoted (no getAmountData call, single quote fetch, request.atomic === false)
a non-MFA Money Keyring holding the same account still promotes, so the subsidy optimisation is not regressed

I verified the first test actually catches the regression rather than passing vacuously — neutering the guard to false && requiresMfa fails it with expected true to be false; restoring it passes.

Full package suite: 1801 tests pass, no type errors. lint:tsc:check, lint:oxlint, lint:misc, and changelog:validate report no issues in this package.

References

Jira: CONF-2157 — Enable non-atomic money account deposits if money account keyring is MFA
Consumer context (Mobile): feat(confirmations): route MFA money account deposits to the non-atomic path metamask-mobile#39222 — routes MFA money account deposits to the non-atomic path. With this change Mobile needs no additional work for the Max case.

Not a breaking change, so no consumer PRs are required to resolve breakage.

Checklist

I've updated the test suite for new or updated code as appropriate
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
I've introduced breaking changes in this PR and have prepared draft pull requests for clients and consumer packages to resolve them

Note

Medium Risk
Touches core quote atomic/delegation behavior and Relay max promotion; wrong account or keyring detection could disable atomic quotes or leave MFA users with per-refresh signatures.

Overview
When the transaction’s signing account (txParams.from, not the quote payer / accountOverride) is on an MPC Money Keyring, the controller now forces non-atomic quotes so quote refreshes do not require a delegation signature on every screen update.

resolveAtomic in updateQuotes clears the client’s atomic hint for MFA keyrings before any strategy runs. getRelayMaxQuote additionally skips subsidized max → atomic promotion when that same signing account requires MFA, so promotion cannot re-embed target calls after the hint was cleared.

Keyring helpers move from utils/7702.ts to utils/account.ts (accountRequiresMfa, shared hasKeyringOfType); KEYRING_TYPES_REQUIRING_MFA and KEYRING_TYPES_SUPPORTING_7702 live in constants.ts. No public API changes—behavior is internal and applies once MFA MPC accounts ship.

^{Reviewed by Cursor Bugbot for commit 98ef653. Bugbot is set up for automated code reviews on this repo. Configure here.}

`config.atomic` alone is a hint the Relay strategy may override. On a subsidised max route `getRelayMaxQuote` promotes the quote back to atomic, which reintroduces the per-quote delegation signature the MFA case exists to avoid. Device logs confirmed this: with a partial amount the money account signed nothing, but on Max it signed on every quote refresh. Set `atomicForbidden` alongside `atomic: false` for MFA keyrings, so Core skips both promotion paths. Only the MFA case sets it; the max-amount case keeps the existing hint, since promotion is desirable there. `resolveMoneyAccountDepositAtomic` now returns both values so the two settings stay derived in one place, and the TODO marks the dev-only override for removal once MFA Money Accounts are selectable at runtime. Requires `atomicForbidden` in @metamask/transaction-pay-controller (MetaMask/core#11389). Until that releases and the dependency is bumped, this will not typecheck against the declared ^30.4.3.

matthewwalsh0 · 2026-11-09T12:42:03Z

+  // that cannot sign a delegation per quote.
+  if (
+    isPostQuote ||
+    atomicForbidden ||


Don't we already check the transaction's from's keyring for EIP-7702 support, so could we do the same for MFA to disable atomic and keep it encapsulated in pay controller?

Good call — done in 9bb6aca. atomicForbidden and all its plumbing (TransactionConfig → TransactionData → QuoteRequest, 11 touch points in quotes.ts) are gone, replaced by accountRequiresMfa in utils/keyring.ts mirroring accountSupports7702.

…omic execution `TransactionConfig.atomic` carries two different meanings. For most flows it is a hard choice, but for max deposits under `payStrategies.relay.atomicMaxEnabled` it is only a subsidy hint: `getRelayMaxQuote` promotes a subsidized non-atomic max quote back to atomic. That promotion is wrong for flows where embedding the target calls is incorrect rather than merely suboptimal. An atomic quote needs a fresh delegation signature per quote, and quotes refresh continuously while an amount screen is open. When the target account's keyring requires multi-factor authentication, every one of those signatures costs the user an MFA confirmation. Add `atomicForbidden` as a hard constraint distinct from the `atomic` hint. When set, `getRelayMaxQuote` skips both promotion paths and always resolves to a non-atomic max quote. The existing atomic-max subsidy optimisation is unchanged for callers that do not set it.

…idden test The new test omitted the required `from` on the `getRelayQuotes` request, adding a 168th TS2741 to a file whose suppression entry allows 167. Supply `FROM_MOCK` rather than raising the suppression count, so the file does not accumulate another suppressed error.

…a client flag Replaces the `atomicForbidden` config flag with a keyring lookup inside the pay controller, per review feedback. `accountSupports7702` already resolves an account's keyring through `KeyringController:getState`, so the controller can answer "does this account require MFA to sign" itself rather than having every client plumb a flag through `TransactionConfig` -> `TransactionData` -> `QuoteRequest`. That removes the public API addition and all its plumbing, and means no client can forget to set it. One subtlety the flag hid: the delegation an atomic quote needs is signed by the account the target calls execute from, which is `transaction.txParams.from` — not `request.from`. Deposits funded by MetaMask Pay set `accountOverride` to the funding EOA, so `request.from` is the payer while `txParams.from` is the money account whose keyring actually signs. Reading the wrong one would leave the promotion firing for exactly the accounts this protects. Only `MPC Money Keyring` is treated as MFA. The SRP-derived `Money Keyring` signs locally and keeps the existing atomic-max subsidy optimisation.

The destructure from `getMessengerMock()` widens its mocks to `any`, so the two new `mockReturnValue` calls added two `no-unsafe-call` and two `no-unsafe-member-access` violations beyond this file's suppression counts. Re-type the keyring mock through a local alias rather than raising those counts, so new assertions do not add suppressed errors.

cursor

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ 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 725fdf8. Configure here.}

The changelog check requires Unreleased entries to link to the PR that introduced them. Mine was the only entry in the file without one.

The MFA guard forwarded the caller's `request` unchanged, but `processTransactions` only skips embedding the target calls when `atomic` is literally `false`. An unset or `true` hint therefore still embedded them and still requested a delegation signature per quote — the exact prompt the guard exists to prevent. Pass `{ ...request, atomic: false }`, matching the existing unsubsidized fallback a few lines below. The original test missed this because it reused `MONEY_ACCOUNT_MAX_REQUEST`, which already sets `atomic: false`, so the guard had nothing left to clear. It now omits the hint entirely and asserts the request body carries no `txs`, which is the property that actually matters. Verified it fails against the previous implementation.

The MFA check only ran inside `getRelayMaxQuote`, but non-max requests go through `getQuoteWithPostQuoteGasHandling` and never reach it. A partial-amount deposit from an MFA account therefore still embedded the target calls and still signed a delegation per quote. Move the check into `normalizeRequest`, which runs once per request ahead of both paths, so the quote is cleared to `atomic: false` regardless of amount. `relay-max` keeps one guard of its own: the subsidized branch would otherwise promote an already-cleared request straight back to atomic. Adds a non-max regression test alongside the max one. Both fail against the previous implementation.

matthewwalsh0 · 2026-11-10T11:43:10Z

+  // When that keyring requires MFA each quote refresh costs the user a
+  // confirmation, so atomic execution is never acceptable for those accounts.
+  // Applied here so it covers both the max and non-max quote paths.
+  const atomic = accountRequiresMfa(messenger, transaction.txParams?.from)


Could we do this check in quotes.ts when building the quote request, so it applies to all strategies including ServerStrategy?

Done — moved it into buildQuoteRequests's caller, updateQuotes in quotes.ts, as a resolveAtomic helper. It now resolves atomic once before the requests are built, so it applies to every strategy (Server, Across, Relay) and to both the max and non-max paths, and normalizeRequest is back to being Relay-shaped normalisation only.

I kept the defensive check in getRelayMaxQuote, since atomic promotion sets atomic: true regardless of the incoming hint, so clearing the hint upstream is not enough on its own.

matthewwalsh0 · 2026-11-10T11:44:15Z

+ *
+ * The SRP-derived `Money Keyring` signs locally and is deliberately absent.
+ */
+export const KEYRING_TYPES_REQUIRING_MFA: `${KeyringTypes}`[] = [


Minor, should this go in constants.ts?

Done — moved it to constants.ts, and moved the pre-existing KEYRING_TYPES_SUPPORTING_7702 alongside it while I was there, since the two belong together.

matthewwalsh0 · 2026-11-10T11:51:42Z

+ * @param account - The account address to check.
+ * @returns Whether signing with the account requires MFA.
+ */
+export function accountRequiresMfa(


Minor, as this very similar to accountSupports7702 in 7702.ts, could we move both of them into a account.ts?

Done — both now live in utils/account.ts, and utils/keyring.ts and utils/7702.ts are deleted. They also share a private hasKeyringOfType helper now, since the lookup was duplicated.

…nd move MFA check to quotes.ts - Create account.ts with consolidated accountSupports7702 and accountRequiresMfa - Move KEYRING_TYPES_REQUIRING_MFA and KEYRING_TYPES_SUPPORTING_7702 to constants.ts - Move MFA check from relay-quotes normalizeRequest to quotes.ts buildQuoteRequests so it applies to all strategies (Relay, Server, Across, etc.), not just Relay - Delete utils/7702.ts (functions moved to account.ts) - Update imports across 6 files to use account.js instead of 7702.js - Keep defensive MFA check in relay-quotes normalizeRequest for direct calls - Keep isMpcMoneyAccountPayer in keyring.ts for specific Money Account use cases - All 1806 tests pass with no type errors Addresses review feedback: - #11389 (comment) - #11389 (comment) - #11389 (comment)

…nd move MFA check to quotes.ts - Create account.ts with consolidated accountSupports7702 and accountRequiresMfa - Move KEYRING_TYPES_REQUIRING_MFA and KEYRING_TYPES_SUPPORTING_7702 to constants.ts - Move MFA check from relay-quotes normalizeRequest to quotes.ts buildQuoteRequests so it applies to all strategies (Relay, Server, Across, etc.), not just Relay - Delete utils/7702.ts (functions moved to account.ts) - Update imports across 6 files to use account.js instead of 7702.js - Keep defensive MFA check in relay-quotes normalizeRequest for direct calls - All 1802 tests pass with no type errors Addresses review feedback: - #11389 (comment) - #11389 (comment) - #11389 (comment)

…r all strategies Address review feedback on #11389: - Move the MFA guard from `normalizeRequest` in `relay-quotes.ts` to `updateQuotes` in `quotes.ts`, so it is applied when building quote requests and therefore covers every strategy rather than only Relay. - Move `KEYRING_TYPES_REQUIRING_MFA` and `KEYRING_TYPES_SUPPORTING_7702` from `types.ts` to `constants.ts`. - Consolidate `accountRequiresMfa` and `accountSupports7702` into a new `utils/account.ts`, replacing `utils/keyring.ts` and `utils/7702.ts`, and share their keyring lookup. The defensive check in `getRelayMaxQuote` stays, since atomic promotion sets `atomic: true` regardless of the incoming hint.

jpuri self-assigned this Nov 9, 2026

jpuri added the team-confirmations label Nov 9, 2026

jpuri marked this pull request as ready for review November 9, 2026 12:33

jpuri requested review from a team as code owners November 9, 2026 12:33

jpuri temporarily deployed to default-branch November 9, 2026 12:33 — with GitHub Actions Inactive

jpuri enabled auto-merge November 9, 2026 12:34

mm-confirmations-pull-request-bot Bot requested a review from matthewwalsh0 November 9, 2026 12:39

matthewwalsh0 requested changes Nov 9, 2026

View reviewed changes

jpuri changed the title ~~feat(transaction-pay-controller): add atomicForbidden to force non-atomic execution~~ fix(transaction-pay-controller): keep max quotes non-atomic for MFA keyrings Nov 9, 2026

jpuri mentioned this pull request Nov 9, 2026

feat(confirmations): route MFA money account deposits to the non-atomic path MetaMask/metamask-mobile#39222

Draft

4 of 10 tasks

jpuri added 4 commits November 9, 2026 19:51

jpuri force-pushed the feat/conf-2157-atomic-forbidden branch from 9bb6aca to 725fdf8 Compare November 9, 2026 14:23

cursor Bot reviewed Nov 9, 2026

View reviewed changes

Comment thread packages/transaction-pay-controller/src/strategy/relay/relay-max.ts Resolved

Merge branch 'main' into feat/conf-2157-atomic-forbidden

5471351

jpuri requested a review from matthewwalsh0 November 9, 2026 14:31

jpuri added 4 commits November 9, 2026 20:02

docs(transaction-pay-controller): link the changelog entry to the PR

e2a76d7

The changelog check requires Unreleased entries to link to the PR that introduced them. Mine was the only entry in the file without one.

Merge branch 'main' into feat/conf-2157-atomic-forbidden

8e73ed3

matthewwalsh0 requested changes Nov 10, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

jpuri force-pushed the feat/conf-2157-atomic-forbidden branch from 679302e to b8e5cd1 Compare November 10, 2026 15:42

jpuri requested a review from matthewwalsh0 November 10, 2026 15:44

Merge branch 'main' into feat/conf-2157-atomic-forbidden

98ef653

Navigation Menu

Repository navigation

fix(transaction-pay-controller): keep max quotes non-atomic for MFA keyrings - #11389

fix(transaction-pay-controller): keep max quotes non-atomic for MFA keyrings#11389
jpuri wants to merge 11 commits into
mainfrom
feat/conf-2157-atomic-forbidden

jpuri commented Nov 9, 2026 •

edited by cursor Bot

Loading

Uh oh!

matthewwalsh0 Nov 9, 2026

Uh oh!

jpuri Nov 9, 2026 •

edited

Loading

Uh oh!

cursor Bot left a comment

Uh oh!

Uh oh!

matthewwalsh0 Nov 10, 2026

Uh oh!

jpuri Nov 10, 2026

Uh oh!

matthewwalsh0 Nov 10, 2026

Uh oh!

jpuri Nov 10, 2026

Uh oh!

matthewwalsh0 Nov 10, 2026

Uh oh!

jpuri Nov 10, 2026

Uh oh!

This comment was marked as resolved.

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jpuri commented Nov 9, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Solution

The account checked is transaction.txParams.from, not request.from

Why not a client flag

Compatibility

Testing

References

Checklist

Uh oh!

matthewwalsh0 Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

jpuri Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthewwalsh0 Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

jpuri Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

matthewwalsh0 Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

jpuri Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

matthewwalsh0 Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

jpuri Nov 10, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jpuri commented Nov 9, 2026 •

edited by cursor Bot

Loading

The account checked is `transaction.txParams.from`, not `request.from`

jpuri Nov 9, 2026 •

edited

Loading