Estimate gas price #299

3ach · 2018-10-18T17:22:18Z

So far, I have the gas price being queried from the API if the default is being used. I still need to fix the tests and maybe add a --gasPrice flag to the CLI. This is for issue #56 .

3ach · 2018-10-19T03:02:38Z

@spalladino Could you take a look at this and let me know if it seems reasonable so far?

spalladino · 2018-10-22T08:38:00Z

@zachzundel looks good so far! A few comments:

Try to cache the gas price obtained from the API in the state object, to avoid having to wait one query per each tx.
Double-check the units in which the API returns the gas price, you may need to add a few zeroes to the value you obtain.
Make sure to add a few tests later down the road (I know this is still WIP).
And a minor comment: these lines are repeated several times, see if it's possible to refactor them:

   let fixedGasPrice = checkGasPrice(txParams)
   if (fixedGasPrice) {
      txParams.gasPrice = fixedGasPrice
  }

3ach · 2018-11-05T18:01:15Z

Hi @spalladino -- I've updated my PR and I think I've fixed everything you pointed out. There was already a test for estimateGas that seems to cover my use case, please let me know if that's not correct and I will write another one!

spalladino

Hey @zachzundel, sorry for the delay on the review! We were offline during the past two weeks (devcon + team offsite), but we are now back on track.

The code looks good, thanks for addressing the concerns! There are only two things I think we need to review:

ethgasstation provides gas price info for mainnet only, and its figures may not make sense for testnets. We should only execute the gas price query if we are on mainnet (ie network_id=1), and use the default gas price value if on testnet.
The tests for estimateGas only check gas usage, not gas price; we should add a test that checks that the gas price from ethgasstation is used if no gas price is provided. I'd suggest stubbing the axios response using sinon (check out other tests in Transactions.test.js for inspiration). And since we're at it, another test to check that ethgasstation is not queried if a gasPrice is provided.

Last but not least, I'd add a safeguard on the gas price in case ethgasstation returns a ridiculously high value. For instance, if the suggested gas price is over 100 gwei, let's throw an exception similar to what we're doing now, and ask the user to set a fixed gas price themselves.

packages/lib/src/utils/Transactions.js

3ach · 2018-11-13T22:30:15Z

How can we tell testnet vs. mainnet here? Is it something in txParams? Do I need to use Truffle?

spalladino · 2018-11-14T14:19:09Z

How can we tell testnet vs. mainnet here? Is it something in txParams? Do I need to use Truffle?

The following line returns the network ID (I think that as a string, not a number), by querying the node to retrieve it. If it's equal to '1', then it's mainnet.

await promisify(web3.version.getNetwork.bind(global.web3.version))()

Make sure we don't perform that query to node on every transaction. It should be enough to make it once, and cache the result.

3ach · 2018-11-18T04:54:03Z

@spalladino I've added in tests, this is ready for review!

spalladino

Great work @zachzundel! I left some minor comments, mostly regarding style. There are two changes I'd like to ask of you:
1- Can you revert changes to package-lock.json files that should not have been changed, except for zos-lib?
2- Can you add a test for the API returning a gas price value over 100 gwei?

Thanks a lot!!

package.json

packages/lib/src/utils/Transactions.js

packages/lib/test/src/utils/Transactions.test.js

- don't return txParams, just modify argument - use BigNumber functions instead of math - modify an error mssage

- also fix indentation - refactor tests to use beforeEach, afterEach

3ach · 2018-11-20T23:51:02Z

I've pushed commits to address all your changes!

spalladino

Awesome work @zachzundel!! Thanks a lot for the contribution, and for accommodating to all requests :-)

* Add axios and update package{,-lock}.json * Query for gas price * Persist gasPrice in state * Refactor and test * let -> const; safeLow -> average * Ensure gasPrice api doesn't pick a huge value * Check network * Cache network * Add test * Test update * Rollback all package-lock.json files * Package locks * Finalize tests * Remove unnecessary dependency * Minor stylistic changes - don't return txParams, just modify argument - use BigNumber functions instead of math - modify an error mssage * Add additional test for extreme API response - also fix indentation - refactor tests to use beforeEach, afterEach * Revert package lock changes

* Fix truffle dependencies versions and add truffle-provider * Fix runWithTruffle promise resolution * v2.0.2 * Uncomment git tag checkout (#402) * Update CLI package lock * Changelog for v2.0.1 (#403) [ci skip] * Rename versioned md links to html (#407) * Update testing guide (#406) * Add token mechanics section to the docs (#408) * Add token mechanics section to the docs * Update packages/docs/docs/docs/token_mechanics.md Co-Authored-By: facuspagnuolo <facuspagnuolo@users.noreply.github.com> * Add more tests for create command in CLI * Do not delete deployment info from other networks in truffle artifacts (#415) Upon creating a proxy for a contract, the current implementation clears all deployment information before adding the new one. It should only replace the info for the current network. * Example with truffle migrations (#416) * Estimate gas price (#299) * Add axios and update package{,-lock}.json * Query for gas price * Persist gasPrice in state * Refactor and test * let -> const; safeLow -> average * Ensure gasPrice api doesn't pick a huge value * Check network * Cache network * Add test * Test update * Rollback all package-lock.json files * Package locks * Finalize tests * Remove unnecessary dependency * Minor stylistic changes - don't return txParams, just modify argument - use BigNumber functions instead of math - modify an error mssage * Add additional test for extreme API response - also fix indentation - refactor tests to use beforeEach, afterEach * Revert package lock changes * Add Etherscan as remote verification application for 'verify' command (#413) * add etherscan as remote verification application for verify command * Add test for etherchain network parameter, improve output in Verifier, move etherscan verification status function out of main etherscan verify function * add how to get etherscan api key in description of verify's --api-key option * add tests for etherscan verification and return tests for etherchain verification * fix typo in etherscan tests * Minor code style changes * Use contract method `estimateGas` function (#310) Instead of using web3 estimateGas (only for contract method calls) * Fixed typo (#442) Line 131 typo on Adnd fixed * [CI skip] Fixed several typos and made minor improvements to the docs. * Remove legacy code from App and Lib distinction (#449) * Remove all `lib` references from scripts * Remove deprecated init-lib script * Remove `lib` references from scripts tests * Remove network and local `lib` controllers * Merge base and app controllers Merged base and app for both local and network controllers * Remove/rename deprecated mock contracts * Remove remaining `lib` references in cli models * Remove `lib` remaining references from cli tests * Rename all `lib` variables and references. Renamed all `lib`, `libToLink`, and their derivatives Renamed scripts and commands `lib` referencies, e.g., `deployLibs` variable to `deployDependencies` Renamed Network and Local controllers functions (e.g., `linkLibs` to `linkDependencies`) * Rename LibProject to PackageProject In both cli ProjectDeployer and lib Project classes * Update vouching deploy script * Preserve parameters compatibility in cli scripts * Rename dependenciesNames to dependencies * Add gas and gasPrice to mainnet docs instructions (#446) Attempting to deploy (and EVM package) using zos following the mainnet recomendations as listed here fails with the error: Cowardly refusing to execute transaction with excessively high default gas price of 100 gwei. Consider explicitly setting a different gasPrice value in your truffle.js config file. You can check reasonable values for gas price in https://ethgasstation.info/. We need to set the gas value and gas price. I added an example above, however this is not always enough either and can fail with either: ProviderError: insufficient funds for gas * price + value or Error: Contract transaction couldn't be found after 50 blocks or Error: ProviderError: gas required exceeds allowance or always failing transaction They contract may still be deployed however in the case of the final error example. * docs: add chai to the testing guide (#404) * Added encodeCall import (#422) * Added encodeCall import * Improve encodeCall import * Fix BigNumber promise call in doc site example (#444) * myContract.x().toString() does not return a number myContract.x().toString() does not return a number, it returns a promise object. myContract.x().toString() returns a pending promise: '[object Promise]' myContract.x() returns a bigNumber object: BigNumber { s: 1, e: 1, c: [ 42 ] } To return a string format of the returned promise object we need: myContract.x().then((num)=> num.toString()) will return '42' * Update v2.0.0 doc example * Delete mock contracts when publishing package (#293) Fixes zeppelinos/zos-lib#208 * Update v2.0.0 docs * Fix HTTP request to ethgasstation response handler * Change capital letters from test's descriptions * Add truffle-hdwallet-provider to integration tests * Remove sinon as main dependency from CLI * Remove truffle-workflow-compile in favour of shell compilation * Extend CLI Truffle dependency range to accept versions 4 & 5 * Add truffle bin check before compiling * Extend Initializable pragma version to support solc 0.5 * v2.1.0-rc.0 * Fix encodeCall address handling (#569) * Fix encodeCall address handling * Remove .only from encodeCall tests * Update truffle-migrate example to v2.1.0-rc.0 (#574) * v2.1.0 * Fix verify bad merge * Remove old NetworkController.js file

facuspagnuolo assigned spalladino Oct 22, 2018

spalladino added the status:in-progress Under development, do not merge this PR label Oct 25, 2018

3ach force-pushed the gas-price branch from e0a4e86 to b13f370 Compare November 5, 2018 16:16

spalladino reviewed Nov 13, 2018

View reviewed changes

packages/lib/src/utils/Transactions.js Outdated Show resolved Hide resolved

packages/lib/src/utils/Transactions.js Outdated Show resolved Hide resolved

3ach force-pushed the gas-price branch 2 times, most recently from bfa6c00 to 89f40c2 Compare November 14, 2018 17:00

3ach added 9 commits November 17, 2018 15:37

Add axios and update package{,-lock}.json

e349de7

Query for gas price

8bba92a

Persist gasPrice in state

0aebe9f

Refactor and test

2f4362b

let -> const; safeLow -> average

dd922be

Ensure gasPrice api doesn't pick a huge value

35d6c5f

Check network

d535680

Cache network

876ef5e

Add test

5815f60

3ach force-pushed the gas-price branch from 89f40c2 to e2b23f3 Compare November 18, 2018 02:13

3ach added 4 commits November 17, 2018 19:14

Test update

e2b23f3

Rollback all package-lock.json files

3b72f89

Package locks

b3ff21b

Finalize tests

a1a422c

3ach changed the title ~~WIP: Estimate gas price~~ Estimate gas price Nov 18, 2018

spalladino reviewed Nov 20, 2018

View reviewed changes

3ach added 2 commits November 20, 2018 15:33

Remove unnecessary dependency

c8fa126

Minor stylistic changes

7477d98

- don't return txParams, just modify argument - use BigNumber functions instead of math - modify an error mssage

Add additional test for extreme API response

ce5027c

- also fix indentation - refactor tests to use beforeEach, afterEach

Revert package lock changes

0665aba

spalladino approved these changes Nov 21, 2018

View reviewed changes

spalladino merged commit 79ce941 into OpenZeppelin:master Nov 21, 2018

gitcoinbot mentioned this pull request Nov 21, 2018

Estimate gas price when sending transactions #56

Closed

frangio removed the status:in-progress Under development, do not merge this PR label Jan 31, 2020

Estimate gas price #299

Estimate gas price #299

Uh oh!

Conversation

3ach commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

3ach commented Oct 19, 2018

Uh oh!

spalladino commented Oct 22, 2018

Uh oh!

3ach commented Nov 5, 2018

Uh oh!

spalladino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

3ach commented Nov 13, 2018

Uh oh!

spalladino commented Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

3ach commented Nov 18, 2018

Uh oh!

spalladino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

3ach commented Nov 20, 2018

Uh oh!

spalladino left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

3ach commented Oct 18, 2018 •

edited

Loading

spalladino commented Nov 14, 2018 •

edited

Loading