-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix: prevent flickering of insufficient balance alert until gas station checks complete #38306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
✨ Files requiring CODEOWNER review ✨✅ @MetaMask/confirmations (14 files, +296 -94)
🔐 @MetaMask/web3auth (1 files, +2 -0)
|
Builds ready [9cacf14]
UI Startup Metrics (1366 ± 129 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [6803f1d]
UI Startup Metrics (1312 ± 124 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ui/pages/confirmations/hooks/alerts/transactions/useInsufficientBalanceAlerts.ts
Outdated
Show resolved
Hide resolved
...nfirmations/components/confirm/info/shared/selected-gas-fee-token/selected-gas-fee-token.tsx
Outdated
Show resolved
Hide resolved
Builds ready [4bc9c12]
UI Startup Metrics (1304 ± 118 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
4bc9c12 to
b06d058
Compare
ui/pages/confirmations/hooks/alerts/transactions/useInsufficientBalanceAlerts.ts
Outdated
Show resolved
Hide resolved
Builds ready [88dd0a6]
UI Startup Metrics (1296 ± 124 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…sk/metamask-extension into vs/flickering-gas-station
Builds ready [cd63bd9]
UI Startup Metrics (1320 ± 119 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ui/pages/confirmations/hooks/alerts/transactions/useInsufficientBalanceAlerts.ts
Show resolved
Hide resolved
Builds ready [1aa1730]
UI Startup Metrics (1326 ± 123 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
1aa1730 to
cd63bd9
Compare
Builds ready [cd63bd9]
UI Startup Metrics (1271 ± 114 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [174348d]
UI Startup Metrics (1240 ± 114 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [ba4bac1]
UI Startup Metrics (1253 ± 99 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
|
||
| const batchTransactionValues = | ||
| currentConfirmation?.nestedTransactions?.map( | ||
| (trxn) => (trxn.value as Hex) ?? 0x0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Numeric literal used instead of hex string in fallback
The fallback value 0x0 is a numeric literal (the number zero) rather than the hex string '0x0'. When trxn.value is undefined, this will insert the number 0 into batchTransactionValues instead of a hex string. The sumHexes function expects string arguments with base 16, so passing a number could cause incorrect calculations or type errors depending on how the Numeric class handles the mismatch. The fallback should be quoted as '0x0'.
Builds ready [2c63ed4]
UI Startup Metrics (1244 ± 111 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| // - Gasless is NOT supported (user needs native currency for gas) | ||
| // - Gasless IS supported but gasFeeTokens is empty (no alternative tokens available) | ||
| const shouldCheckGaslessConditions = | ||
| isGaslessCheckComplete && (!isGaslessSupported || isGasFeeTokensEmpty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Alert suppressed when simulation disabled but gasless supported
When simulation is disabled and gasless IS supported, the insufficient balance alert will never show. The old code had canSkipSimulationChecks = ignoreGasFeeToken || !isSimulationEnabled which allowed the alert to show when simulation was disabled. The new shouldCheckGaslessConditions requires !isGaslessSupported || isGasFeeTokensEmpty, but when gasFeeTokens is undefined (never loaded because simulation is disabled), isGasFeeTokensEmpty is false (since undefined?.length === 0 evaluates to false). This means users with insufficient balance who have simulation disabled but gasless supported will never see the alert, even though they cannot actually use gas fee tokens.
Additional Locations (1)
Builds ready [244bac1]
UI Startup Metrics (1253 ± 95 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** This PR fixes a [issue raised by cursor](#38306 (comment)) to 0x0 is a numeric literal (the number zero) rather than the hex string '0x0' <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [](https://codespaces.new/MetaMask/metamask-extension/pull/38649?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Fixes: MetaMask/MetaMask-planning#6408 ## **Manual testing steps** 1. Send flow with insufficient balance 2. should display the insufficient alert ## **Screenshots/Recordings** [alert.webm](https://github.com/user-attachments/assets/5cf66e88-8256-4995-9b2d-4cb30f2f5198) <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Standardizes zero-hex fallback via a shared `ZERO_HEX_FALLBACK` in `useHasInsufficientBalance` for tx value, batch values, and balance lookups. > > - **Hooks** > - `ui/pages/confirmations/hooks/useHasInsufficientBalance.ts`: > - Introduces `ZERO_HEX_FALLBACK = '0x0'`. > - Replaces inline zero-hex defaults with `ZERO_HEX_FALLBACK` for `txParams.value`, nested transaction values, and balance fallback. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 3d24dbb. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Description
This PR fixes an issue where the insufficient gas critical alert briefly appears and then disappears once gas-station checks determine that the transaction can be paid with a token.
Previously, the UI would render the alert immediately and replace it a moment later, causing a flicker.
With this update, the insufficient balance alert is held back until gas-station checks fully complete.
Changelog
CHANGELOG entry: Fixed a flicker where the insufficient balance alert appeared before gas-station checks completed
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/6210
Manual testing steps
Screenshots/Recordings
true-alert.webm
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Defers the insufficient balance alert until gasless support checks finish by introducing a pending state and a new useHasInsufficientBalance hook, updating related hooks and tests.
useInsufficientBalanceAlertsto useuseHasInsufficientBalanceand gate alerts on gasless check completion (pending) and simulation results to prevent flicker.useIsGaslessSupportednow returnspending;useIsGaslessLoadinganduseAutomaticGasFeeTokenSelectrespectpendingand useuseHasInsufficientBalance.useHasInsufficientBalanceto centralize balance/fee sufficiency andnativeCurrencylookup.pendinghandling and new hook usage.Written by Cursor Bugbot for commit 244bac1. This will update automatically on new commits. Configure here.