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

Overriding gas estimate in the send page even if enough ETH is not available #18554

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

NiranjanaBinoy
Copy link
Contributor

Explanation

Fixes #17966
Removing the gas warning and enabling the next button on the send page even when enough ETH is not available for gas.

Manual Testing Steps

  1. Start a send transaction
  2. Enter the maximum amount
  3. Make sure the gas warning message is not displayed and Next button is active

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@NiranjanaBinoy NiranjanaBinoy added the team-confirmations-secure-ux-PR PRs from the confirmations team label Apr 12, 2023
@NiranjanaBinoy NiranjanaBinoy self-assigned this Apr 12, 2023
@github-actions
Copy link
Contributor

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.

@NiranjanaBinoy NiranjanaBinoy marked this pull request as ready for review April 13, 2023 16:44
@NiranjanaBinoy NiranjanaBinoy requested a review from a team as a code owner April 13, 2023 16:44
@NiranjanaBinoy NiranjanaBinoy requested review from PeterYinusa, jpuri and digiwand and removed request for PeterYinusa April 13, 2023 16:44
@metamaskbot
Copy link
Collaborator

Builds ready [92b30fc]
Page Load Metrics (1552 ± 35 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint91135106105
domContentLoaded1417166715336933
load1444171915527435
domInteractive1417166615336933
Bundle size diffs
  • background: 0 bytes
  • ui: 44 bytes
  • common: 0 bytes

@NiranjanaBinoy NiranjanaBinoy marked this pull request as draft April 14, 2023 14:10
@NiranjanaBinoy NiranjanaBinoy marked this pull request as ready for review April 14, 2023 14:10
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

I think it would be good to add a test for the new logic. Can we add a follow-up PR for this or handle it in this PR?

@NiranjanaBinoy
Copy link
Contributor Author

@seaona Can we do a round of QA review on this PR to make sure these changes have not affected the other existing scenarios?

@NiranjanaBinoy NiranjanaBinoy added DO-NOT-MERGE Pull requests that should not be merged needs-qa Label will automate into QA workspace labels Apr 17, 2023
@seaona
Copy link
Contributor

seaona commented Apr 18, 2023

The behavior seems to work as expected @NiranjanaBinoy 👍 and flows seem to work normally.

  • We are not blocking the button to proceed to the Confirmation screen and the warning info does not appear on the Edit screen.
  • The Confirmation screen works as before, blocking the Confirm button if there are not enough funds with the appropriate warnings.

So from QA looks good.


I've reviewed the current Warnings/input validations on Edit and Confirm screens and it seems that they are not super consolidated across different network types. See:

On EIP1559 networks (i.e. Mainnet)

  • When I am on the Edit screen with an EIP1559 network (i.e. Mainnet), if I set an Amount bigger than my balance I see this input validation error Insufficient funds for gas

Screenshot from 2023-04-18 10-49-20

  • And if I proceed I see this warning You do not have enough ETH in your account to pay for transaction fees on Ethereum Mainnet network. Buy ETH or deposit from another account.

Screenshot from 2023-04-18 10-47-47

  • On ERC20 Approve screen, we see Insufficient funds for gas warning

Screenshot from 2023-04-18 11-05-02

  • On ERC721 Approve/SetApprovalForAll, we see You do not have enough ETH in your account to pay for transaction fees on Ethereum Mainnet network. Buy ETH or deposit from another account.

On non EIP1559 networks (i.e. Polygon zkEVM)

When I am on Edit screen, I see the following input validation errors:

Screenshot from 2023-04-18 10-47-09

And if I proceed I see this warning Insufficient funds..

Screenshot from 2023-04-18 10-47-22

@bschorchit do we need to consolidate them somehow? This should be a separate issue in case we want to change it.

@seaona seaona removed DO-NOT-MERGE Pull requests that should not be merged needs-qa Label will automate into QA workspace labels Apr 18, 2023
@NiranjanaBinoy NiranjanaBinoy merged commit 9d3cdd1 into develop Apr 18, 2023
@NiranjanaBinoy NiranjanaBinoy deleted the override-gas-send branch April 18, 2023 15:10
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
team-confirmations-secure-ux-PR PRs from the confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Override of gas estimate not possible if not enough ETH at this moment
5 participants