Conversation
9da8fdd to
0d43e4a
Compare
| const rampsState = messenger.call('RampsController:getState'); | ||
| const selectedProviderId = rampsState.providers.selected?.id; |
There was a problem hiding this comment.
There will always be a selected provider as Ramps team mentioned. Even though this is undefined we will pick first quote.
There was a problem hiding this comment.
Although there's going to be a selected provider already this function restricts the current abilities of RampsController:getQuotes, so in case there is a need to set a specific provider this function would not allow it. I'm ok approving as is if this can be refactored later.
| const quotes = await messenger.call('RampsController:getQuotes', { | ||
| amount: adjustedAmount, | ||
| assetId: fiatAsset.caipAssetId, | ||
| fiat: 'USD', |
There was a problem hiding this comment.
'USD' will be hard lock for MMPay confirmations.
There was a problem hiding this comment.
Not sure if fiat has any effect here as the currency is determined by the region. @saustrie-consensys @pkowalski can check and confirm
| messenger.call('TransactionPayController:updateFiatPayment', { | ||
| callback: (fiatPayment) => { | ||
| fiatPayment.rampsQuote = fiatQuote; | ||
| }, |
There was a problem hiding this comment.
Exact fiatPayment.rampsQuote will be required to start headless ramps operation in client, hence we will hold this in the fiatPayment state.
RampsController:getQuotes and persist rampsQuote on fiatPayment
| const quotes = await messenger.call('RampsController:getQuotes', { | ||
| amount: adjustedAmount, | ||
| assetId: fiatAsset.caipAssetId, | ||
| fiat: 'USD', |
There was a problem hiding this comment.
To clarify, this insists the provider deal in USD? Do all the providers respect that? Or are the values / quotes converted in the RampsController?
| assetId: fiatAsset.caipAssetId, | ||
| fiat: 'USD', | ||
| paymentMethods: [fiatPaymentMethod], | ||
| providers: selectedProviderId ? [selectedProviderId] : undefined, |
There was a problem hiding this comment.
Why do we need to retrieve state from the RampsController only to give it back?
Can this not be the default providers value if not set?
Or we use an optional property such as useSelectedProvider: true?
Not a blocker, but would help encapsulation and decoupling.
There was a problem hiding this comment.
I agree, there are multiple ways to adjust the request in better shape, we should even consider having an explicit API to return single quote for MMPay use-case as we care just the best quote.
These are all mentioned and alignments may follow in future.
| selectedProviderId, | ||
| }); | ||
|
|
||
| const quote = quotes.success?.[0]; |
There was a problem hiding this comment.
Just clarifying the obvious for audit sake that success is ordered by preference so first entry is best?
There was a problem hiding this comment.
Yes that's right, but I believe we most probably never going to get more than one quote as limited to one provider while calling getQuotes.
984a392
984a392 to
73e119e
Compare
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…ler:getState Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…tes and persist rampsQuote Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…fications Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
73e119e to
f0a595c
Compare
## Explanation Release `955.0.0` with a major version bump for: - **`@metamask/transaction-pay-controller`** `20.2.0` → `21.0.0` ### `@metamask/transaction-pay-controller@21.0.0` **Breaking:** Narrow `AllowedActions` type to use individual action types instead of compound controller action unions (`BridgeControllerActions`, `BridgeStatusControllerActions`, `CurrencyRateControllerActions`, `GasFeeControllerActions`) Other changes: - Add Gas Station support for Across source transactions when native balance is insufficient - Pass explicit `assetId`, `providers`, and `fiat` to `RampsController:getQuotes` and persist the selected ramps quote on `TransactionFiatPayment` ### Dependency updates No packages depend on `@metamask/transaction-pay-controller`, so no dependency range updates were needed. ## References - [MetaMask#8670](MetaMask#8670) — Narrow `AllowedActions` to individual action types (BREAKING) - [MetaMask#8588](MetaMask#8588) — Add Gas Station support for Across source transactions - [MetaMask#8628](MetaMask#8628) — Pass explicit params to `RampsController:getQuotes` and persist `rampsQuote` ## 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 - [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) - [ ] 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** > This is primarily a release/versioning PR, but it publishes `@metamask/transaction-pay-controller@21.0.0` which includes a documented **breaking** `AllowedActions` type change that may require consumer updates. > > **Overview** > Bumps the monorepo version to `955.0.0` and releases `@metamask/transaction-pay-controller` from `20.2.0` to `21.0.0`. > > Updates the `transaction-pay-controller` changelog with the `21.0.0` release notes, including a **breaking** narrowing of the `AllowedActions` type, plus Gas Station support for Across source transactions and a ramps quoting fix. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 3a003ea. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Explanation
The fiat quote flow in
transaction-pay-controllerwas callingRampsController:getQuoteswithout specifying the asset, provider, or currency - relying on defaults or a hardcodedpickBestFiatQuotehelper that filtered for a specific staging provider.This PR:
RampsController:getQuotes:assetId(fromfiatAsset.caipAssetId),providers(fromRampsControllerState.providers.selected.id), andfiat: 'USD'.quotes.success[0]instead of searching by hardcoded provider name viapickBestFiatQuote.TransactionFiatPayment.rampsQuoteviaTransactionPayController:updateFiatPaymentso downstream consumers can access it.getRampsQuotehelper to isolate ramps state reading and quote fetching for readability.pickBestFiatQuoteand the unusedFiatQuotesResponsetype alias since they are no longer needed.RampsControllerGetStateActionto allowed actions so the messenger can read ramps controller state.References
Checklist
Note
Medium Risk
Moderate risk: changes the fiat quoting path by selecting providers via
RampsController:getStateand altering how a ramps quote is chosen/persisted, which could affect quote availability and downstream consumers offiatPaymentstate.Overview
Improves the fiat quote flow to call
RampsController:getQuoteswith explicitassetId,fiat(defaulting toUSD), and an optionalprovidersfilter derived fromRampsController:getState(instead of relying on implicit defaults and a hardcoded provider filter).Persists the selected ramps quote onto per-transaction state (
TransactionFiatPayment.rampsQuote) viaTransactionPayController:updateFiatPayment, removes the now-unusedpickBestFiatQuotehelper/types, and updates tests to cover provider-selection behavior and persistence.Reviewed by Cursor Bugbot for commit 9e4c29b. Bugbot is set up for automated code reviews on this repo. Configure here.