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: Fix gas estimates #24253

Merged
merged 4 commits into from
Apr 25, 2024
Merged

fix: Fix gas estimates #24253

merged 4 commits into from
Apr 25, 2024

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Apr 25, 2024

Description

This is a patch of the bug fix included in MetaMask/core#4214. This ensures that the gas estimates we show for the globally selected chain always reflect the estimates for that chain.

Open in GitHub Codespaces

Related issues

Fixes #24254

Manual testing steps

(from #24254)

  1. Be on network A. Toggle off "estimate balance changes" in settings.
  2. Switch to another network that does not have a transaction in the activity list
  3. Go through the send flow until you get to the confirm screen. Let the gas update once
  4. click "Edit" in the top left corner, to go back to the send screen
  5. Reject from the send screen
  6. Switch to a network where you already have a confirmed transaction in the activity list
  7. Observe in the background console that suggestGasFees network requests are being sent to endpoints for two different chainIds
  8. Go through the send flow to the confirm screen
  9. Submit when the most recent suggestGasFee network request is to the network that is not the currently selected network
  10. You should be able to verify that the transaction was signed and published with incorrect gas values for the current network

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). 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.

This is a patch of the bug fix included in MetaMask/core#4214
@metamaskbot
Copy link
Collaborator

Builds ready [69b1d6a]
Page Load Metrics (604 ± 445 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint75157962311
domContentLoaded10292242
load622520604926445
domInteractive10292242

@Gudahtt Gudahtt marked this pull request as ready for review April 25, 2024 22:52
@Gudahtt Gudahtt requested a review from a team as a code owner April 25, 2024 22:52
@danjm
Copy link
Contributor

danjm commented Apr 25, 2024

Here is a video of how this branch successfully prevents simultaneous polling from causing incorrect gas estimates

gas-working.mp4

@danjm danjm merged commit 9d2d6cf into Version-v11.14.2 Apr 25, 2024
8 of 10 checks passed
@danjm danjm deleted the patch-core-4214 branch April 25, 2024 23:23
@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants