Skip to content
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

fix: POL gas fee is reused after switching to a new network #30417

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

micaelae
Copy link
Member

@micaelae micaelae commented Feb 19, 2025

Description

Open in GitHub Codespaces

Related issues

Fixes: #29374

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

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.

@metamaskbot
Copy link
Collaborator

Builds ready [a795786]
Page Load Metrics (1447 ± 44 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1294170014479244
domContentLoaded1287164914298842
load1295170014479144
domInteractive227335188
backgroundConnect85419157
firstReactRender1368352311
getState4478105
initialActions01000
loadScripts901123510247737
setupStore55210105
uiStartup1509185916428942
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 207 Bytes (0.00%)
  • common: 41 Bytes (0.00%)

@cryptotavares cryptotavares added the team-confirmations Push issues to confirmations team label Feb 19, 2025
@sleepytanya
Copy link
Contributor

@micaelae

Screen.Recording.2025-02-20.at.12.36.38.mov

Copy link
Contributor

@cryptotavares cryptotavares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some unit tests?

const isSwapsChain = useSelector(getIsSwapsChain);

useGasFeeEstimates(network?.id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that you need to initialise polling here again.. The send duck already instructs the gasFeeController to start polling. And that send duck initialisation is triggered whenever we reach a edit send page (startDraftTransaction, editExistingTransaction)

// remove this logic once getGasFeeEstimates is typed
const gasFee1559 = maybeGasFee ?? medium?.suggestedMaxFeePerGas;
// Need to use estimates from gas fee controller instead of transaction gas estimates bc transaction gasEstimates don't get cleared after submitting a POL tx
const { medium } = useSelector(getGasFeeControllerEstimates);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only change that you need. If the confirmTransaction state exists, the getGasFeeEstimates takes the maxFeePerGas and maxPriorityFeePerGas from it. Otherwise it gets those values from the gas fee controller state directly.

This should be a temporary fix, because I believe that the main issue is that we are not clearing the confirmation redux state after successfully submitting the confirmation successfully (store.confirmTransaction). FYI @matthewwalsh0

@cryptotavares
Copy link
Contributor

This PR is not necessary anymore. Fix is implemented here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants