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
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 5ce7386. Configure here.
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
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.References
ogp/fiat-money-account-deposit-fix)Checklist
Note
High Risk
Changes post-settlement fiat relay submission, on-ramp wallet resolution, and calldata patching for money-account deposits—errors could block payments or submit wrong amounts without a matching client
getAmountDataimplementation.Overview
Fixes MetaMask Pay fiat flows after on-ramp settlement, especially
moneyAccountDepositwith nested approve/deposit calldata.TransactionPayControllergains an optionalgetAmountDatacallback andTransactionPayController:getAmountDatamessenger action so the client can re-encode nested transactiondatafor a settled token amount (defaults to empty updates).fiat-submit.tsis reworked post-order completion:accountOverride ?? txParams.from; on-chain source resolution usesbaseRequest.from.isMaxAmount: false,isPostQuote: true(avoids max-amount + included-tx errors).getAmountData→ patch nesteddataandrequiredAssets[0].amount→ delegation quote withtargetAmountMinimum.validateRelaySlippageis replaced byvalidateRelayRateDrift, comparing USD in/out ratios (10% max) instead of raw output amounts.Totals:
calculateTotalstakesfiatPaymentAmount; when a fiat strategy quote is present, totals use the user-entered fiat amount (viaquotes.tspassingfiatPayment.amountFiat), not relaytargetAmount.Exports and tests cover the new API and both relay paths.
Reviewed by Cursor Bugbot for commit d9d35fd. Bugbot is set up for automated code reviews on this repo. Configure here.