Conversation
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
FiatStrategy and getQuotes functionalityFiatStrategy and getQuotes functionality
FiatStrategy and getQuotes functionalityFiatStrategy and getQuotes functionality
|
@SocketSecurity ignore npm/@metamask/ramps-controller@12.0.0 |
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.
| amount: adjustedAmount, | ||
| paymentMethods: [selectedPaymentMethodId], | ||
| walletAddress, | ||
| }); |
There was a problem hiding this comment.
Missing assetId in ramps getQuotes call
High Severity
The RampsController:getQuotes call is missing the assetId parameter. The fiatAsset object (derived from deriveFiatAssetForFiatPayment) contains a caipAssetId field that was clearly intended to be passed as assetId, but it's never included in the call. The RampsController.getQuotes method validates that assetId is present and throws "assetId is required." if neither the parameter nor this.state.tokens.selected?.assetId is available. This causes the fiat quote flow to either silently fail (caught by try-catch, returning []) or use an unrelated asset from ramps controller state.
Additional Locations (1)
| ### Added | ||
|
|
||
| - **BREAKING:** Add `AssetsControllerGetStateForTransactionPayAction` to the `AllowedActions` messenger type ([#8163](https://github.com/MetaMask/core/pull/8163)) | ||
| - Add MMPay `FiatStrategy` quotes flow ([#8121](https://github.com/MetaMask/core/pull/8121)) |
There was a problem hiding this comment.
| - Add MMPay `FiatStrategy` quotes flow ([#8121](https://github.com/MetaMask/core/pull/8121)) | |
| - Add `FiatStrategy` to retrieve quotes via `RampsController` ([#8121](https://github.com/MetaMask/core/pull/8121)) |
| export const POLYGON_USDCE_ADDRESS = | ||
| '0x2791Bca1f2de4661ED88A30C99A7a9449Aa84174' as Hex; | ||
|
|
||
| export type TransactionPayFiatAsset = { |
There was a problem hiding this comment.
Should all of the be in strategy/fiat/[types|consants].ts?
|
|
||
| import type { RelayQuote } from '../relay/types'; | ||
|
|
||
| export type FiatOriginalQuote = { |
There was a problem hiding this comment.
Minor, in the context of this controller, could this just be FiatQuote?
| import type { RelayQuote } from '../relay/types'; | ||
|
|
||
| export type FiatOriginalQuote = { | ||
| fiatQuote: Quote; |
There was a problem hiding this comment.
Would it add clarity to call this rampsQuote as "Fiat" is our abstraction over Ramps and Relay together?
| provider: FiatValue; | ||
|
|
||
| /** Fee charged by fiat on-ramp provider. */ | ||
| fiatProvider?: FiatValue; |
There was a problem hiding this comment.
providerFiat?
Or should we be more generic and call this providerSecondary so we can have a mechanism to have a more granular fee and the client can choose what to call it?
|
|
||
| function getRampsProviderFee(fiatQuote: RampsQuote): BigNumber { | ||
| return new BigNumber(fiatQuote.quote.providerFee ?? 0).plus( | ||
| fiatQuote.quote.networkFee ?? 0, |
There was a problem hiding this comment.
Nice, so they obviously handle any network fees, but include it separately?
| fiat: metaMaskFee, | ||
| usd: metaMaskFee, | ||
| }, | ||
| provider: relayQuote.fees.provider, |
There was a problem hiding this comment.
For consistency, this needs to be the total provider fee for the quote, so ramps + relay.
Meaning we can calculate the Relay portion if needed in the client by subtracting the fiatProvider value.
| } | ||
| } | ||
|
|
||
| return MMPAY_FIAT_ASSET_ID_BY_TX_TYPE[transactionType as TransactionType]; |
| tx.metamaskPay = { | ||
| bridgeFeeFiat: totals.fees.provider.usd, | ||
| bridgeFeeFiat: totals.fees.fiatProvider | ||
| ? new BigNumber(totals.fees.provider.usd) |
There was a problem hiding this comment.
As mentioned in other comment, this shouldn't be needed as provider always needs to be the total.
| .toString(10); | ||
|
|
||
| const totalUsd = new BigNumber(providerFee.usd) | ||
| .plus(fiatProviderFee.usd) |
There was a problem hiding this comment.
Also here, this is an additional optional breakdown, but provider remains the total provider fee.


Explanation
This PR introduces
FiatStrategy:TransactionPayStrategy.Fiatand registersFiatStrategyin strategy resolution.MMPAY_FIAT_ASSET_ID_BY_TX_TYPE).fiat-quotes.tswith relay-first estimation:fiatPayment.amountFiat+ selected payment methodstrategy: fiatquotefees.provider= relay provider/swap feefees.fiatProvider= ramps provider/network feefees.metaMask= MM fee (100 bps overamountFiat + adjustedAmountFiat)metamaskPay.bridgeFeeFiatwhen fiat provider fee exists.References
Checklist
Note
Medium Risk
Adds a new
Fiatpayment strategy that combines Relay fee estimation with Ramps quote retrieval and introduces a new fee bucket (fees.fiatProvider), which can affect totals and transaction metadata calculations.Overview
Adds a new MM Pay
FiatStrategyand wiring (TransactionPayStrategy.Fiat) to generate quotes by first estimating fees via Relay, then requesting Ramps quotes using an adjusted fiat amount (user amount + estimated relay fees), returning a single combined quote.Introduces
TransactionPayFiatAsset+ tx-type-to-fiat-asset mapping, addsRampsController:getQuotesto messengerAllowedActions, and updates fee/totals plumbing to support a separatefees.fiatProviderbucket (including aggregating it into totals andmetamaskPay.bridgeFeeFiat).Includes new unit tests for the fiat quote flow/utilities and adds
@metamask/ramps-controllerdependency/build references; fiatexecuteis currently a placeholder returning no tx hash.Written by Cursor Bugbot for commit ee58873. This will update automatically on new commits. Configure here.