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.

Sign up for GitHub

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

Jump to bottom

Fix/gas estimations #2408

Merged
merged 18 commits into from
Apr 7, 2021
Merged

Fix/gas estimations #2408

merged 18 commits into from
Apr 7, 2021

Conversation

andrepimenta
Copy link
Member

@andrepimenta andrepimenta commented Mar 17, 2021 •
edited
Loading

Description

This fixes gas estimations for non-mainnet networks.

Checklist

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

Issue

Resolves #2316

@andrepimenta andrepimenta marked this pull request as ready for review March 17, 2021 16:13
@andrepimenta andrepimenta requested a review from a team as a code owner March 17, 2021 16:13
@andrepimenta andrepimenta added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 17, 2021
@andrepimenta andrepimenta removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Mar 23, 2021
@andrepimenta andrepimenta added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Mar 25, 2021
@omnat
Copy link
Contributor

omnat commented Apr 6, 2021

@andrepimenta notifying here to ensure we handle this case:

For all gas estimations (e.g., on mainnet), if the gas API fails (gives an error, or API is down or API is irresponsive), then fall back on gasPrice

@ibrahimtaveras00 ibrahimtaveras00 added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 6, 2021
estebanmino
estebanmino reviewed Apr 7, 2021
View reviewed changes
app/components/UI/CustomGas/index.js
// Check if either no basicGasEstimates were provided or less than 3 options were provided (for example, only the average gas price)
const noBasicGasEstimates = !basicGasEstimates || Object.keys(basicGasEstimates).length < 3;
return isNotMainnet || noBasicGasEstimates;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

also we're calling this method like 3 times, maybe it could be a good idea to set state or a const on didmount ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there might be some situations where basicGasEstimates is not available at mount?
Since this is a very simple check I think it's fine to keep it as a method, wdyt?

@ibrahimtaveras00
Copy link
Contributor

We're still showing speed up/cancel option, just wanted to make sure if this was a requirement or not (taken from #2316 (comment))
cc: @andrepimenta/ @omnat

image

@ibrahimtaveras00
Copy link
Contributor

If a user attempts to edit the gas price while on a custom network, there is an exception that pops up

seen here = https://recordit.co/wjjSR1Ws9z

Screen Shot 2021-04-07 at 1 35 31 PM

@ibrahimtaveras00
Copy link
Contributor

@andrepimenta notifying here to ensure we handle this case:

For all gas estimations (e.g., on mainnet), if the gas API fails (gives an error, or API is down or API is irresponsive), then fall back on gasPrice

@omnat I just tested this use case, and we hide the basic gas options in this case and only show advanced (as we do on other non-mainnet networks in this PR)

Screen Shot 2021-04-07 at 4 09 53 PM

@andrepimenta andrepimenta removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 7, 2021
ibrahimtaveras00
ibrahimtaveras00 approved these changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

QA Passed 👍🏽

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Apr 7, 2021
@andrepimenta andrepimenta merged commit 821934c into develop Apr 7, 2021
@andrepimenta andrepimenta deleted the fix/gas-estimations branch April 7, 2021 21:43
@omnat omnat linked an issue Apr 8, 2021 that may be closed by this pull request
Wrong gasPrice on bsc net #2472
Closed
rickycodes added a commit that referenced this pull request Apr 8, 2021
@rickycodes
Merge branch 'develop' into feature/custom-nonce
40b99f6 
* develop:
  v2.1.0 (#2481)
  Analytics v2 (priority 1) (#2456)
  Fix/gas estimations (#2408)
rickycodes added a commit that referenced this pull request Apr 8, 2021
@rickycodes
Merge branch 'develop' into feature/confusables
6f99419 
* develop:
  v2.1.0 (#2481)
  Analytics v2 (priority 1) (#2456)
  Fix/gas estimations (#2408)
  remove controllers tgz (#2479)
  Improvement/assets by chainid (#2441)
  Improvement/chain ticker (#2442)
  Remove instapay (#2372)
  Fix iOS build (#2467)
  Migrate from AsyncStorage to FileStorage (#2084)
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
@andrepimenta
Fix/gas estimations (#2408)
ab3b75e 
* initial implementation

* Only display advanced

* Approve transaction gas estimation

* Fix custom gas input

* Hide transaction actions for non-mainnet

* Fix typo

* Fix gas estimations for dapp transactions

* Typo: Chain id as prop

* Add test

* Improve test

* Fix Transactions UI test

* Move to chainId

* Added fallback to network gas price for mainnet basic estimates error

* Update tests

* Move code to util & fix custom gas component & timeout

* Remove unused getBasicGasEstimatesWithHardcodedFallback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebanmino estebanmino estebanmino approved these changes

@ibrahimtaveras00 ibrahimtaveras00 ibrahimtaveras00 approved these changes

Assignees
No one assigned
Labels
next release QA Passed A successful QA run through has been done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong gasPrice on bsc net Show correct gas on custom networks
4 participants
@andrepimenta @omnat @ibrahimtaveras00 @estebanmino