feat: Gas modals refactor I #38624
Open
feat: Gas modals refactor I #38624
+2,243
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
✨ Files requiring CODEOWNER review ✨✅ @MetaMask/confirmations (28 files, +2179 -11)
|
Collaborator
Builds ready [dbbcef8]
UI Startup Metrics (1260 ± 98 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
OGPoyraz
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ui/pages/confirmations/components/confirm/info/hooks/useFeeCalculations.ts
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/hooks/useFeeCalculations.ts
Outdated
Show resolved
Hide resolved
OGPoyraz
added
the
no-changelog
| // Note: feePerGas and priorityFeePerGas are hex strings from txParams/gasFeeEstimates | ||
| let minimumFeePerGas = addHexes( | ||
| decGWEIToHexWEI(estimatedBaseFee) || HEX_ZERO, | ||
| feePerGas ? (priorityFeePerGas as Hex) : HEX_ZERO, |
There was a problem hiding this comment.
Bug: Wrong variable checked in gas fee conditional logic
The calculateGasEstimateCallback function checks feePerGas to decide whether to add priorityFeePerGas to the calculation, but this is the wrong variable to check. The condition feePerGas ? (priorityFeePerGas as Hex) : HEX_ZERO should check priorityFeePerGas directly since that's the value being conditionally added. While current callers happen to set both values together (either both valid or both HEX_ZERO), this logic is confusing and could cause incorrect gas estimates if a caller ever passes a valid priorityFeePerGas with an empty/undefined feePerGas.
| priorityFeePerGas = dappSuggestedGasFees?.maxPriorityFeePerGas; | ||
| } else if (dappSuggestedGasFees?.gasPrice) { | ||
| gasPrice = dappSuggestedGasFees?.gasPrice; | ||
| gas = dappSuggestedGasFees?.gas || HEX_ZERO; |
There was a problem hiding this comment.
Bug: Legacy gas option loses gas limit fallback
When a dapp suggests legacy gas pricing, the code sets gas = dappSuggestedGasFees?.gas || HEX_ZERO, falling back to HEX_ZERO if the dapp doesn't provide a gas value. This loses the transactionMeta.gasLimitNoBuffer fallback that was set on line 57. For EIP-1559 suggestions, gasLimitNoBuffer is used, but for legacy suggestions without a gas value, the estimate would incorrectly use zero gas, resulting in a zero fee estimate rather than using the transaction's actual gas limit.
Collaborator
Builds ready [e3526c8]
UI Startup Metrics (1210 ± 89 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
jpuri
reviewed
Dec 9, 2025
| import { useSelector } from 'react-redux'; | ||
| import { TransactionMeta } from '@metamask/transaction-controller'; | ||
| import { useConfirmContext } from '../../context/confirm'; | ||
| import { getNetworkConfigurationsByChainId } from '../../../../../shared/modules/selectors/networks'; |
jpuri
reviewed
Dec 9, 2025
| } else { | ||
| gasPropertiesToUpdate = { | ||
| maxFeePerGas: transactionGasFeeEstimates?.gasPrice, | ||
| maxPriorityFeePerGas: transactionGasFeeEstimates?.gasPrice, |
Contributor
There was a problem hiding this comment.
gasPrice only is assigned above to maxFeePerGas and maxPriorityFeePerGas
Member
Author
There was a problem hiding this comment.
Yes because estimation is done via gasPrice meaning only maxFeePerGas and maxPriorityFeePerGas should be defined if EIP1559
jpuri
reviewed
Dec 9, 2025
| isSelected: isGasPriceEstimateSelected, | ||
| key: 'gasPrice', | ||
| name: t('networkSuggested'), | ||
| onSelect: () => onGasPriceEstimateLevelClick(), |
Contributor
There was a problem hiding this comment.
Creating new fn here does not look like is needed () => onGasPriceEstimateLevelClick(), instead we can just use onGasPriceEstimateLevelClick.
jpuri
reviewed
Dec 9, 2025
| </ModalContent> | ||
| </Modal> | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
This modal does not seems to have any content.
Member
Author
There was a problem hiding this comment.
Yes, I will implement details in step II as mentioned here
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR implements the first step of https://github.com/MetaMask/MetaMask-planning/issues/6067
Note that new gas modal UI is not enabled yet until very last step. Hence no changelog included.
The recording is manually enabled on local.
Changelog
CHANGELOG entry:
Related issues
Fixes: Step I of https://github.com/MetaMask/MetaMask-planning/issues/6067
Manual testing steps
N/A
Screenshots/Recordings
Before
After
Step.I.gas.modals.mov
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Adds a new gas fee modal stack (Estimates, Advanced EIP-1559/Legacy) with option hooks, UI list items, time utils, and a
calculateGasEstimateAPI, plus i18n and tests.GasFeeModalorchestrator withEstimatesModal,AdvancedEIP1559Modal, andAdvancedGasPriceModal.NetworkStatisticsupdated to support a redesigned header viauseRedesigned.useGasOptionsto compose options from:useGasFeeEstimateLevelOptions(low/medium/high; EIP-1559/legacy aware)useGasPriceEstimateOption(network suggested gas price)useDappSuggestedGasFeeOption(site-suggested)useAdvancedGasFeeOption(opens advanced modals)useFeeCalculationsnow exposescalculateGasEstimatefor computing totals (incl. optional L1 fees) and precise native amounts.GasEstimateListHeaderandGasEstimateListItemwith optional tooltip details.toHumanEstimatedTimeRange/toHumanSeconds.useTransactionNativeTickerto resolve native ticker by chain.networkSuggestedtoenanden_GBlocales.useFeeCalculationsupdates.Written by Cursor Bugbot for commit e3526c8. This will update automatically on new commits. Configure here.