feat: Derive fiat asset from feature flags before hardcoded fallback#8631
feat: Derive fiat asset from feature flags before hardcoded fallback#8631
Conversation
OGPoyraz
had a problem deploying
to
default-branch
GitHub Actions
Failure
OGPoyraz
force-pushed
the
ogp/fiat-ff-implementation
branch
from
5425439 to
b24eac7
Compare
OGPoyraz
changed the title
OGPoyraz
force-pushed
the
ogp/fiat-ff-implementation
branch
from
b24eac7 to
7ef80a6
Compare
OGPoyraz
force-pushed
the
ogp/fiat-ff-implementation
branch
from
7ef80a6 to
1096df7
Compare
| import type { TransactionPayFiatAsset } from './constants'; | ||
| import { ETH_MAINNET_FIAT_ASSET, FIAT_ASSET_ID_BY_TX_TYPE } from './constants'; | ||
|
|
||
| function resolveTransactionType( |
There was a problem hiding this comment.
Minor, exported functions at top of file for readability?
| transaction: TransactionMeta, | ||
| ): TransactionType | undefined { | ||
| if (transaction.type === TransactionType.batch) { | ||
| return transaction.nestedTransactions?.[0]?.type; |
There was a problem hiding this comment.
Is it safe to assume this is always the first call?
Don't we iterate over calls looking for a supported type in Relay strategy?
In Predict for example, if first deposit, the initial call is the deploy for Safe proxy?
There was a problem hiding this comment.
Done, added a similar utility
| const firstMatchingType = transaction.nestedTransactions?.[0]?.type; | ||
| if (firstMatchingType) { | ||
| return FIAT_ASSET_ID_BY_TX_TYPE[firstMatchingType]; | ||
| const hardcodedAsset = FIAT_ASSET_ID_BY_TX_TYPE[txType]; |
There was a problem hiding this comment.
The intent of the feature-flag utils is to abstract these defaults also, so most of the existing functions fallback to defaults.
Can we do that here and encapsulate the constant and mainnet fallbacks in there?
There was a problem hiding this comment.
Good call, carried over the fallbacks info FF utility
| transactionType: TransactionType, | ||
| ): TransactionPayFiatAsset | undefined { | ||
| const state = messenger.call('RemoteFeatureFlagController:getState'); | ||
| const fiatFlags = state.remoteFeatureFlags?.confirmations_pay_fiat as |
There was a problem hiding this comment.
Should we define a local FiatFlags type at the top of the file for re-use and readability?
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
Reviewed by Cursor Bugbot for commit 0c647d8. Configure here.
|
|
||
| return transaction.nestedTransactions?.find( | ||
| (tx) => tx.type && FIAT_ASSET_ID_BY_TX_TYPE[tx.type] !== undefined, | ||
| )?.type; |
There was a problem hiding this comment.
Batch transactions ignore feature-flag-only transaction types
Medium Severity
resolveTransactionType filters nested batch transactions using only FIAT_ASSET_ID_BY_TX_TYPE, so any transaction type defined exclusively via the confirmations_pay_fiat feature flag (and not in the hardcoded map) will never be matched inside a batch. The .find() predicate skips that nested type, returns undefined, and getFiatAssetPerTransactionType falls through to ETH_MAINNET_FIAT_ASSET — silently ignoring the feature flag entry. This defeats the PR's core goal of enabling asset mappings without a code release for batch transactions.
Reviewed by Cursor Bugbot for commit 0c647d8. Configure here.
| 'erc20', | ||
| tokenAddress, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Exported buildCaipAssetType has no production callers
Low Severity
buildCaipAssetType is exported and tested but has zero production callers anywhere in the codebase. The caipAssetId field was removed from TransactionPayFiatAsset in this PR, but nothing calls this new utility to compute it dynamically. This is dead code that adds maintenance burden without providing value.
Reviewed by Cursor Bugbot for commit 0c647d8. Configure here.
2 participants
Explanation
Currently
deriveFiatAssetForFiatPaymentresolves the fiat asset for a payment entirely from a hardcoded map (FIAT_ASSET_ID_BY_TX_TYPE). This makes it impossible to adjust asset mappings without a code release.This PR introduces a 3-tier resolution for fiat assets:
confirmations_pay_fiat.assetPerTransactionType[txType]viaRemoteFeatureFlagControllerFIAT_ASSET_ID_BY_TX_TYPEconstantThe function signature gains a
messengerparameter (already available at the call site infiat-quotes.ts), and the return type tightens fromTransactionPayFiatAsset | undefinedtoTransactionPayFiatAssetsince the ETH mainnet fallback guarantees a value.References
confirmations_pay_fiatChecklist
Note
Medium Risk
Changes fiat-quote asset selection to be feature-flag driven and to always fall back to mainnet ETH, which can alter quote inputs and pricing behavior for unsupported transaction types.
Overview
Enables feature-flag-driven fiat asset resolution for MM Pay fiat quotes:
deriveFiatAssetForFiatPaymentnow consultsconfirmations_pay_fiat.assetPerTransactionType(via newgetFiatAssetPerTransactionType) before falling back to the existing hardcoded map, and finally to an ETH-on-mainnet default.Updates the fiat quote path to pass a
messengerinto fiat-asset derivation and to compute raw source amounts usinggetTokenInfo().decimals(with early return when token info/rates are unavailable). AddsbuildCaipAssetTypeutility for generating CAIP-19 asset identifiers, and expands tests/changelog accordingly.Reviewed by Cursor Bugbot for commit 0c647d8. Bugbot is set up for automated code reviews on this repo. Configure here.