fix: MMPay fiat payment fixes #8987
Conversation
OGPoyraz
changed the title
MoneyAccountDeposit
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
OGPoyraz
force-pushed
the
ogp/fiat-money-account-deposit-fix
branch
from
3f77b67 to
0560a35
Compare
OGPoyraz
force-pushed
the
ogp/fiat-money-account-deposit-fix
branch
from
b3b05dd to
8ca96e8
Compare
OGPoyraz
changed the title
MoneyAccountDeposit 
| ### Added | ||
|
|
||
| - Add optional `getAmountData` callback to `TransactionPayControllerOptions` for client-side nested calldata re-encoding ([#8987](https://github.com/MetaMask/core/pull/8987)) | ||
| - Add `TransactionPayController:getAmountData` messenger action ([#8987](https://github.com/MetaMask/core/pull/8987)) |
There was a problem hiding this comment.
Can we combine / nest some or all of these?
|
|
||
| export type GetAmountDataRequest = { | ||
| /** Raw token amount (atomic units) to encode into calldata. */ | ||
| amount: string; |
There was a problem hiding this comment.
Minor, I think getPaymentOverrideData provides the human / formatted amount. Not sure which is easier for the client to handle.
There was a problem hiding this comment.
buildMoneyAccountDepositBatch in client was expecting raw amount hence I passed as is.
For reference: https://github.com/MetaMask/metamask-mobile/pull/31009/files#diff-c2390f54bc74c73163ad5949d6accb3524100bf2dd425db4dd82c1ed73d7076f
| refundTo, | ||
| sourceAmounts, | ||
| tokens, | ||
| fiatPayment, |
| ? targetAmount.fiat | ||
| : amountFiat, | ||
| ) | ||
| .plus(paymentAmountFiat) |
There was a problem hiding this comment.
For clarity and consistency, could we call this sourceAmountX and rename the function to getSourceAmount?
| const amountFiat = transactionData?.fiatPayment?.amountFiat; | ||
| const walletAddress = transaction.txParams.from as Hex; | ||
| const walletAddress = | ||
| transactionData?.accountOverride ?? (transaction.txParams.from as Hex); |
There was a problem hiding this comment.
As discussed, this can always use request.from as it's the same calculation?
| const amountFiat = transactionData?.fiatPayment?.amountFiat; | ||
| const walletAddress = transaction.txParams.from as Hex; | ||
| const walletAddress = | ||
| transactionData?.accountOverride ?? (transaction.txParams.from as Hex); |
There was a problem hiding this comment.
On line 95, below, it looks like we were already added the relay fee to the fiat amount?
So was it just the UI that wasn't reflecting that?
There was a problem hiding this comment.
Yes but this is just a quote, real order flow (headless buy) starts when we click "Add funds" in UI where we need the amount again (in useFiatConfirm).
You may ask "why we are not passing the fiat quote amount while starting headless buy", but somehow I never see what amount we requested originally in the quote, either less or more amount saved in fiat quote entity, not sure why. Hence I subtract the fiat provider fee from totals.total in client
https://github.com/MetaMask/metamask-mobile/pull/31009/files#diff-aafbaf61a37d99028f87dea7fbe81be4508bc20412d534db932c1a1cd83e59b6
| (tx) => { | ||
| for (const { nestedTransactionIndex, data } of updates) { | ||
| if (tx.nestedTransactions?.[nestedTransactionIndex]) { | ||
| tx.nestedTransactions[nestedTransactionIndex].data = data; |
There was a problem hiding this comment.
Do we not to update data also? In which case should we look at updateAtomicData in the transaction controller? Or have the new callback return the full data also?
Or are we just updating enough so the Relay quote can generate the delegation and include in the quote?
There was a problem hiding this comment.
Just updating enough parts so only nestedTransactions as other parts will be unnecessary for final quote happening in the Phase3 below.
| const relayRequest: QuoteRequest = { | ||
| ...baseRequest, | ||
| isMaxAmount: false, | ||
| isPostQuote: false, |
There was a problem hiding this comment.
Does this not need to be conditional?
If we're doing a Perps or Predict deposit for example, we won't need transaction data, so we will want to use EXACT_INPUT instead of EXACT_OUTPUT so we can ensure nothing left over and cheaper Relay fees.
We won't even need the first discovery quote in that case so can avoid an entire additional request?
There was a problem hiding this comment.
Very valid point, I will make sure perps predict cases to use EXACT_OUTPUT, thanks.
|
|
||
| // Phase 3: Real relay quote with delegation (standard crypto-like flow). | ||
| const relayRequest: QuoteRequest = { | ||
| ...baseRequest, |
There was a problem hiding this comment.
If we're doing an EXACT_OUTPUT for money account deposit, what do we do with any remaining funds on the selected account after the fiat quote?
There was a problem hiding this comment.
There is no clean way to avoid it, unfortunately it will leave some dust.
If we do the EXACT_INPUT for everything then the vault deposit calldata amount won't match what Relay delivers, causing reverts
| targetAmountMinimum: settledTargetRaw, | ||
| }; | ||
|
|
||
| const relayQuotes = await getRelayQuotes({ |
There was a problem hiding this comment.
Suppose we do a Ramps quote that gets $100 into the EOA.
And we get an EXACT_INPUT quote that tell us we can get $95.
And then we get our final EXACT_OUPUT quote targeting $95... Except the fees are higher in general on EXACT_OUTPUT, plus we now have our additional transaction data included. So what if it says the required source amount is $101?
Do we need to leave a buffer for additional cost so intentionally ask for less in the second quote? But that means we always have funds left over.
Or do we retry with repeating lower amounts until it's less than our balance? Meaning we have to repeatedly re-encode the amount.
Or do we just throw if we can't afford it? But that's not the user's fault so no way for them to remedy.
Essentially we have to consider the slippage twice, from original to discovery, and discovery to final.
There was a problem hiding this comment.
Understood, so depends on the spread between EXACT_INPUT and EXACT_OUTPUT fees + the delegation gas overhead. I didn't foresee this point as I always did my tests on already delegated account.
There is no easy solution for that. But we can try adding potential buffer here for the non-delegated cases, which we can assume discovery phase determine partially (for fees) that. But not sure how can we determine the cost of delegation, maybe we can apply some dedicated amount for that. Would it make sense?
- Consolidate duplicate changelog entries for #8987 - Alphabetize destructured properties in updateQuotes - Rename getPaymentAmount to getSourceAmount for clarity - Simplify walletAddress in fiat-quotes to use transaction.txParams.from
Add optional getAmountData callback that re-encodes nested transaction calldata for a given token amount. Used by transaction types with non-standard nested data (e.g. vault approve + deposit) that require client-side context (vault config, RPC providers) to encode.
After fiat order settlement, use a three-phase relay flow: 1. Discovery quote (EXACT_INPUT) to find settled target token output 2. Re-encode nested calldata via getAmountData callback 3. Real relay quote with delegation (EXACT_OUTPUT) for execution Also removes validateRelaySlippage which incorrectly compared outputs from relay quotes made with different source amounts, and removes isMaxAmount:true which caused delegation errors with nested transactions.
- Fix unnecessary type assertion on requiredAssets hex amount - Add JSDoc @param tags to validateRelayRateDrift - Update fiat-submit tests for three-phase relay flow - Add getAmountData controller tests - Add rate drift, stale calldata, and discovery quote error tests - 100% test coverage maintained
A better post-settlement rate benefits the user and should not block fiat completion. Remove .abs() so only positive drift (rate worsened) is rejected.
- Remove unused args parameter - Replace non-null assertions with optional chaining
The execute flow uses Relay's relayer to handle the source-side transaction, so the user's EOA does not need to hold the source tokens at submit time. This was causing fiat moneyAccountDeposit to fail with 'Insufficient source token balance' after Transak settlement.
The fiat quoting and submission flows used transaction.txParams.from as the wallet address. For moneyAccountDeposit, txParams.from is the money account address on the target chain, not the user's EOA. This caused: - Ramps/Transak to receive the wrong deposit address - resolveSourceAmountRaw to look for on-chain ETH at the wrong address - Relay quotes to use the wrong from/user address - Balance validation to check the wrong account Use accountOverride (the user's selected EVM account) when available, matching the pattern already used in quotes.ts. Also revert the isExecute balance skip (no longer needed with correct address) and remove hasFiatStrategy from totals calculation.
Reverts the test changes from the isExecute balance skip commit since the source code was also reverted. Restores the original test that validates source balance for execute flows.
The test validated the removed hasFiatStrategy path in calculateTotals. With fiat strategy now using amountFiat consistently, this test case is no longer applicable.
Pass fiatPayment.amountFiat from quote context into calculateTotals so the fiat flow uses the user-entered fiat amount for the total instead of deriving it from token amounts or targetAmount.
- Consolidate duplicate changelog entries for #8987 - Alphabetize destructured properties in updateQuotes - Rename getPaymentAmount to getSourceAmount for clarity - Simplify walletAddress in fiat-quotes to use transaction.txParams.from
Transactions without nested calldata (e.g. Perps, Predict deposits) now use a single EXACT_INPUT relay quote after fiat settlement instead of the three-phase discovery + re-encoding + delegation flow. This reduces relay calls from 3 to 1, uses cheaper EXACT_INPUT fees, and avoids leftover dust on the source chain. The three-phase flow is preserved for moneyAccountDeposit and other transactions with nested calldata that requires re-encoding.
OGPoyraz
force-pushed
the
ogp/fiat-money-account-deposit-fix
branch
from
5ce7386 to
d9d35fd
Compare
…iat submit - Add fee-as-buffer strategy to prevent EXACT_OUTPUT cost overruns after fiat settlement by reserving the original relay fee from the discovery source amount and adding the discovery fee back to the final target. - Simple deposits (Perps, Predict) skip the three-phase discovery flow and use a single EXACT_INPUT relay quote for cheaper fees and no dust. - Split fiat-submit.ts into focused modules: - fiat-submit.ts: orchestration (polling, validation, routing) - fiat-submit-simple.ts: single EXACT_INPUT relay path - fiat-submit-with-calldata.ts: three-phase flow with fee buffer - utils.ts: shared validateRelayRateDrift, extractProviderCode - Add configurable feeReserveMultiplier and maxRateDriftPercent via confirmations_pay_fiat feature flag with safe defaults (1 and 10%).
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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 eb1521d. Configure here.
| messenger, | ||
| requests: [discoveryRequest], | ||
| transaction, | ||
| }); |
There was a problem hiding this comment.
Discovery double-subtracts native source
High Severity
The calldata discovery relay request sets isPostQuote: true only to force EXACT_INPUT, but that flag also runs post-quote gas subtraction when the source is the native gas token. That stacks on top of the fiat fee reserve already removed from sourceTokenAmount, so discovery uses an artificially small input. Downstream calculateAdjustedTarget, getAmountData, and validateRelayRateDrift can then be wrong or fail for native-funded moneyAccountDeposit flows.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit eb1521d. Configure here.
2 participants
Explanation
Fiat payments for
moneyAccountDeposittransactions fail with multiple sequential errors after the on-ramp order settles:isMaxAmount: truein the fiat re-quote — after Transak settles,submitRelayAfterFiatCompletionre-quotes withisMaxAmount: true, isPostQuote: false. This callsprocessTransactions, which throws"Max amount quotes do not support included transactions"because moneyAccountDeposit has nested txs (approve + deposit) that require delegation.validateRelaySlippagecompares wrong amounts — it comparescurrencyOut.amountfrom two relay quotes made with different source amounts (theoretical $5 quoting phase vs actual ~$4.95 post-Transak settlement). This produces ~25% apparent "slippage" that is not real slippage — it is just a smaller input producing a smaller output.Nested calldata has zero amounts — the fiat path in
handleDoneonly setsamountFiatand never callsupdateTokenAmount(), sorequiredAssets.amountstays0x0and the nested approve + deposit calldata encodes zero amounts.Wrong wallet address in fiat flow —
fiat-quotes.tsandfiat-submit.tsusedtransaction.txParams.fromas the wallet address. FormoneyAccountDeposit,txParams.fromis the money account address on the target chain (Monad), not the user's EOA. This caused Ramps/Transak to receive the wrong deposit address,resolveSourceAmountRawto look for on-chain ETH at the wrong address, and all relay quotes to use the wrongfrom/useraddress — resulting inTRANSFER_FROM_FAILEDreverts.Fiat total calculation using wrong amount —
calculateTotalsderived the payment amount from token amounts ortargetAmount, which is incorrect for fiat flows where the user enters a specific fiat amount.Changes
fiat-quotes.ts— usesaccountOverridewhen available forwalletAddress, matching the existing pattern inquotes.ts. This ensures Ramps/Transak receives the user's actual EOA address, not the money account address.fiat-submit.ts— two wallet address fixes plus three-phase relay flow:submitFiatQuotes: usesaccountOverride ?? txParams.fromfor order polling wallet addresssubmitRelayAfterFiatCompletion: usesbaseRequest.from(already accountOverride-aware from the quote) for on-chain amount lookupisPostQuote: true, EXACT_INPUT) with the settled source amount to learncurrencyOut.minimumAmountgetAmountDatavia messenger to delegate calldata re-encoding to the client, then patches nested tx data +requiredAssets[0].amountisPostQuote: false, EXACT_OUTPUT) with delegationrelay-submit.ts— reverts theisExecutebalance skip (no longer needed with correct wallet address). The balance check now correctly validates the user's EOA balance.validateRelaySlippage→validateRelayRateDrift— compares USD exchange rate ratios (output_usd / input_usd) between original and discovery quotes instead of absolute amounts, normalising for different source amounts.TransactionPayController— adds optionalgetAmountDatacallback on the constructor, exposed via messenger asTransactionPayController:getAmountData. Keeps ABI knowledge on the client side.totals.ts— addsfiatPaymentAmountparameter tocalculateTotals. When a fiat strategy quote is present, uses the user-entered fiat payment amount directly instead of deriving it from token amounts or relaytargetAmount. Falls back to'0'if fiat amount is unavailable.quotes.ts— passesfiatPayment.amountFiatfrom the transaction pay state intocalculateTotalsasfiatPaymentAmount.relay-quotes.ts(recipient routing fix) — addsskipProcessTransactionstoQuoteRequesttype. Defaults toisPostQuotewhen not set. The simple fiat path (fiat-submit-simple.ts) setsskipProcessTransactions: falseto forceprocessTransactionsto run and extract thetransfer(to, amount)recipient from calldata — fixing Predict/Perps deposits where Relay sent swapped tokens to the user's EOA instead of the target contract.relay-quotes.ts(7702 batch gas estimation) — for post-quote flows, includes the original transaction in batch gas estimation alongside relay params. Previously, a single relay step was estimated alone, gotis7702: false, and the batch fell back to separate type-0x2 transactions — breaking zero-balance fiat-funded accounts that need all native tokens for the swap.relay-quotes.ts(native gas subtraction) — extends the phase-2 gas subtraction to trigger when the source token is the native gas token (e.g. POL on Polygon), not only whenisSourceGasFeeTokenis true. For zero-balance accounts where the source IS the native token, gas must be reserved from the source amount to avoidinsufficient funds for gas * price + value.relay-submit.ts(gas price alignment) — the prepended original transaction in post-quote batch submissions now carries the relay step'smaxFeePerGasandmaxPriorityFeePerGas. This ensures the top-level 7702 batch uses the same gas price as the relay quote, matching the gas cost computed during the phase-2 gas subtraction.eip7702.ts(transaction-controller) —generateEIP7702BatchTransactionnow sums native values from all nested calls and sets the result as the top-level transactionvalue. Previously the top-level value was always0x0, causing 7702 batches with native value transfers (e.g. POL swap) to revert because the delegation contract had no native tokens to forward.Feature Flags (LaunchDarkly)
This PR reads optional properties from two feature flags. No new LaunchDarkly flags need to be created — these are properties on existing flag objects.
confirmations_pay_fiatfeeReserveMultipliernumber11.5) if EXACT_OUTPUT quotes consistently exceed the settled balance after settlement.maxRateDriftPercentnumber10confirmations_pay_post_quotegasBuffernumber1.1insufficient funds.No action required for launch — all properties default to safe values when absent. They exist as remote safety valves to tune post-settlement behavior without a code release.
References
ogp/fiat-money-account-deposit-fix)Checklist
Note
High Risk
Touches fiat on-ramp settlement, relay quoting/submit, EIP-7702 batch value and gas paths, and money-account deposit calldata—errors can block deposits or mis-route funds.
Overview
Fixes fiat MM Pay flows (especially
moneyAccountDeposit) after on-ramp settlement by splitting post-settlement relay into simple vs nested-calldata paths and tightening wallet, quoting, gas, and batch behavior.Fiat submit & client hook: After the ramp order completes, deposits with two or more nested calls use a new three-phase flow (discovery EXACT_INPUT with fee reserve →
TransactionPayController:getAmountDatato re-encode approve/deposit calldata and updaterequiredAssets→ final EXACT_OUTPUT relay). Simpler deposits use a single EXACT_INPUT post-quote relay (skipProcessTransactions: false) for lower cost. Wallet resolution now prefersaccountOverrideovertxParams.from; on-chain source amount usesbaseRequest.from. Slippage checks move tovalidateRelayRateDrift(USD rate ratio) with tunablefeeReserveMultiplier/maxRateDriftPercentflags. Totals usefiatPayment.amountFiatwhen a fiat strategy quote is present.Relay quoting & submit: Post-quote gas estimation includes the original transaction in the batch (7702-friendly for zero-balance fiat users); native-source post-quote flows can reserve gas from the swap amount via
getPostQuoteGasBuffer.skipProcessTransactionscan override the default tied toisPostQuote. Prepended original txs in post-quote batches inherit relay gas price fields.Transaction controller: EIP-7702 batch construction now sets top-level
valueto the sum of nested call values when non-zero.API: Optional
getAmountDatacallback onTransactionPayController(exported types + messenger action).Reviewed by Cursor Bugbot for commit eb1521d. Bugbot is set up for automated code reviews on this repo. Configure here.