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] Gas is not re-calculated when updating a transaction #5876

Merged
merged 10 commits into from
Apr 28, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Mar 2, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description
There is a problem with gas calculation when we switch from one type of Send to another (Send ETH/ Send ERC20 / Send NFT). The transaction object retains the previous gas value before attempting to estimate the new gas value. This bug results in non-desired behaviours for both cases.

Screenshots/Recordings
Before
https://recordit.co/oGbCf2W6iy

After
http://recordit.co/xOnFZ3DyQy

Issue

Progresses #5865

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

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.

@blackdevelopa blackdevelopa self-assigned this Mar 9, 2023
@blackdevelopa blackdevelopa marked this pull request as ready for review March 13, 2023 08:33
@blackdevelopa blackdevelopa requested a review from a team as a code owner March 13, 2023 08:33
@blackdevelopa blackdevelopa added team-confirmations-secure-ux-PR PR from the confirmations team needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 13, 2023
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

Good work 👍

Copy link

@NiranjanaBinoy NiranjanaBinoy left a comment

Choose a reason for hiding this comment

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

lgtm

@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Apr 6, 2023
@seaona seaona removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 28, 2023
@seaona
Copy link
Contributor

seaona commented Apr 28, 2023

PR looks good @blackdevelopa 💯 I've checked switching from ERC20 to ETH and the other way around as well as with ERC721 tokens and they all work fine.

A small thing I've noticed is that the button Send is enabled before the gas has updated, but this can be handled in the future, whenever the loading states are consolidated. cc @bschorchit

The PR can be merged, just requires to fix some conflicts with a test file.

mobile-gas-update-tx.mp4

@seaona seaona added the QA Passed A successful QA run through has been done label Apr 28, 2023
@seaona
Copy link
Contributor

seaona commented Apr 28, 2023

Re-tested again and looks good. I think we can merge it since last changes were due to conflict on a snapshot test

@seaona seaona added the release-6.6.0 Issue or pull request that will be included in release 6.6.0 label Apr 28, 2023
@seaona seaona merged commit 7606ded into main Apr 28, 2023
@seaona seaona deleted the bg/recal_gas_fee branch April 28, 2023 11:44
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-6.6.0 Issue or pull request that will be included in release 6.6.0 team-confirmations-secure-ux-PR PR from the confirmations team unit test coverage confirmed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants