Fix gas calculation checking wrong account balance #21174

bergeron · 2023-10-03T17:56:54Z

Description

Dapps can connect to / prompt transactions from a different account than the one currently selected within the wallet itself.

When this occurs, useGasFeeErrors was doing a balance check against the wrong account. It was using the current account within the wallet, instead of the account issuing the transaction. This can cause balance errors even when the account has sufficient funds.

This shows in 2 places in the UI, which are the only places checking balanceError:

Token approvals
Customizing gas popup

Solution: Check balance of the account issuing the transaction.

Manual testing steps

Within the wallet, select an empty account.
On the E2E Test Dapp, connect to a funded account.
Send ETH
Click "🌐 Site suggested" to customize gas
A balance error used to appear, but no longer should.

Screenshots/Recordings

Before

gas-check-wrong-account.mov

Related issues

Fixes: #20770

Pre-merge author checklist

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.

github-actions · 2023-10-03T17:57:07Z

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.

bergeron · 2023-10-03T17:57:50Z

ui/hooks/gasFeeInput/test-utils.js

@@ -122,7 +121,7 @@ export const generateUseSelectorRouter =
        },
      };
    }
-    if (selector === getSelectedAccount) {
+    if (selector.toString().includes('getTargetAccount')) {


Hack because the selector is actually a lambda, to allow getTargetAccount to accept a second parameter. Therefore it can't use function equality like the others.

A better approach to this kind of mocking, would keep the selectors from the product code and mock the state instead. Better to mock the lowest level thing, so that the product selectors are tested too if they change.

bergeron · 2023-10-03T17:58:24Z

ui/hooks/gasFeeInput/useGasFeeErrors.js

+  // Balance check is only relevant for outgoing + pending transactions
+  const balanceError =
+    account !== undefined &&
+    transaction?.type !== TransactionType.incoming &&
+    transaction?.status in PENDING_STATUS_HASH


I found that this balance check was being called in cases it should not. Like for incoming transactions, and transactions that have already confirmed. Because we won't have the from account in our state for incoming transactions, this guard is needed.

A better fix may be to further split these gas operations, so that callers can request only the necessary gas information for their contexts.

metamaskbot · 2023-10-03T18:34:37Z

Builds ready [25a9129]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1118 ± 355 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	91	145	112	17	8
		domContentLoaded	78	138	106	18	8
		load	90	1968	1118	740	355
		domInteractive	78	138	106	18	8

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 486 Bytes (0.01%)
common: 0 Bytes (0.00%)

… brian/gas-check-wrong-account

metamaskbot · 2023-10-12T15:11:06Z

Builds ready [506f20a]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (640 ± 356 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	84	150	101	16	8
		domContentLoaded	67	155	90	22	10
		load	78	1869	640	742	356
		domInteractive	67	155	90	22	10

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 486 Bytes (0.01%)
common: 0 Bytes (0.00%)

codecov · 2023-10-12T15:11:37Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d6d324b) 68.61% compared to head (506f20a) 68.61%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #21174   +/-   ##
========================================
  Coverage    68.61%   68.61%           
========================================
  Files         1020     1020           
  Lines        40929    40934    +5     
  Branches     10920    10925    +5     
========================================
+ Hits         28080    28085    +5     
  Misses       12849    12849

Files	Coverage Δ
ui/hooks/gasFeeInput/test-utils.js	`76.60% <100.00%> (ø)`
ui/hooks/gasFeeInput/useGasFeeErrors.js	`95.10% <100.00%> (+0.25%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bergeron added 2 commits October 3, 2023 12:03

Check gas against correct account balance

0a50779

fix edit gas popover test

25a9129

bergeron requested a review from a team as a code owner October 3, 2023 17:56

bergeron commented Oct 3, 2023

View reviewed changes

bergeron added the team-confirmations-secure-ux-PR PRs from the confirmations team label Oct 3, 2023

brad-decker previously approved these changes Oct 4, 2023

View reviewed changes

salimtb previously approved these changes Oct 11, 2023

View reviewed changes

sahar-fehri previously approved these changes Oct 12, 2023

View reviewed changes

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

506f20a

… brian/gas-check-wrong-account

bergeron dismissed stale reviews from sahar-fehri, salimtb, and brad-decker via 506f20a October 12, 2023 14:09

sahar-fehri approved these changes Oct 12, 2023

View reviewed changes

salimtb approved these changes Oct 12, 2023

View reviewed changes

bergeron requested review from brad-decker and a team October 12, 2023 16:01

salimtb added the needs-ux-ds-review label Oct 13, 2023

brad-decker approved these changes Oct 13, 2023

View reviewed changes

bergeron merged commit 0bec1df into develop Oct 13, 2023
10 checks passed

bergeron deleted the brian/gas-check-wrong-account branch October 13, 2023 16:07

github-actions bot locked and limited conversation to collaborators Oct 13, 2023

metamaskbot added the release-11.5.0 Issue or pull request that will be included in release 11.5.0 label Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix gas calculation checking wrong account balance #21174

Fix gas calculation checking wrong account balance #21174

bergeron commented Oct 3, 2023 •

edited

Loading

github-actions bot commented Oct 3, 2023

bergeron Oct 3, 2023

bergeron Oct 3, 2023

metamaskbot commented Oct 3, 2023

metamaskbot commented Oct 12, 2023

codecov bot commented Oct 12, 2023 •

edited

Loading

Fix gas calculation checking wrong account balance #21174

Fix gas calculation checking wrong account balance #21174

Conversation

bergeron commented Oct 3, 2023 • edited Loading

Description

Manual testing steps

Screenshots/Recordings

Before

Related issues

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 3, 2023

bergeron Oct 3, 2023

Choose a reason for hiding this comment

bergeron Oct 3, 2023

Choose a reason for hiding this comment

metamaskbot commented Oct 3, 2023

metamaskbot commented Oct 12, 2023

codecov bot commented Oct 12, 2023 • edited Loading

Codecov Report

bergeron commented Oct 3, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading