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

feat(wrapper): introduce getRecommendedGasLimit util #159

Merged
merged 3 commits into from Sep 17, 2018

Conversation

Projects
None yet
2 participants
@PascalPrecht
Copy link
Contributor

PascalPrecht commented Sep 13, 2018

In order to implement #150 this commit introduces a new getRecommendedGasLimit() util
which is calculated based on the estimatedGas by web3 and the latest block
available gas limit.

This is similar to how MetaMask handles gas limit proposals.

We now also prefer using the provided gas that is passed to an intent, if it's higher
than the recommended gas limit. In other words, provided gas is treated as
minimum gas required to perform the transaction/intent.

Closes #150


if (directTransaction.gas && directTransaction.gas < recommendedGasLimit) {

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 13, 2018

Contributor

If no gas is given we just fall back to MetaMask's recommended gas limit calculation which happens in the extension. The calculation is identical except for the block gas limit upper hard cap (which is 95% as opposed to 90%)

This comment has been minimized.

@izqui

izqui Sep 14, 2018

Member

I would say we should always calculate the gas limit of the transaction even if it is not provided, this way we can use the the getDefaultGasPrice function from #158.

This will also remove the need for having to manually calculate the gas limit sending the transactions that aragon.js returns if not using Metamask (for example when using the dao exec command in the CLI, which uses aragon.js to generate the transaction and then needs to set the gas limit before sending it).

const latestBlock = await web3.eth.getBlock('latest')
const latestBlockGasLimit = latestBlock.gasLimit;

const upperGasLimit = latestBlockGasLimit*0.95

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 13, 2018

Contributor

@izqui I went with 0.95 now as per #150 (comment)

Let me know if this needs adjustment.

This comment has been minimized.

@izqui

izqui Sep 14, 2018

Member

I would extract these two magic numbers into constants:

const GAS_FUZZ_FACTOR = 1.5
const PREVIOUS_BLOCK_GAS_LIMIT = 0.95
feat(Aragon): introduce defaultGasPriceFn option
This option can be used to provide a function that returns a default gas
price. It can be async as well, which enables calls to third party APIs such as
ethgasstation

Closes #73
const latestBlock = await web3.eth.getBlock('latest')
const latestBlockGasLimit = latestBlock.gasLimit;

const upperGasLimit = latestBlockGasLimit*0.95

This comment has been minimized.

@izqui

izqui Sep 14, 2018

Member

I would extract these two magic numbers into constants:

const GAS_FUZZ_FACTOR = 1.5
const PREVIOUS_BLOCK_GAS_LIMIT = 0.95
return bufferedGasLimit
} else {
return upperGasLimit
}

This comment has been minimized.

@izqui

izqui Sep 14, 2018

Member

All the returned gas values from this function should be integers

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 17, 2018

Contributor

Correct me if I'm missing something, but all those values are integers. Both, estimatedGasLimit and latestBlockGasLimit are integers and all other values are derived from that.

This comment has been minimized.

@izqui

izqui Sep 17, 2018

Member

Yep, but when multiplied by 0.95 or 1.5 they may become decimal.

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 17, 2018

Contributor

🙈 of course.

This comment has been minimized.

@izqui

izqui Sep 17, 2018

Member

Learned this the hard way when attempting to send a transaction with a decimal gas limit and it blew up ;)


if (directTransaction.gas && directTransaction.gas < recommendedGasLimit) {
directTransaction.gas = recommendedGasLimit
}
return [directTransaction]
} catch (_) { }
}

This comment has been minimized.

@izqui

izqui Sep 14, 2018

Member

We should also check the recommendedGasLimit in createForwarderTransaction and also use the gasPrice function from #158 there.


if (directTransaction.gas && directTransaction.gas < recommendedGasLimit) {

This comment has been minimized.

@izqui

izqui Sep 14, 2018

Member

I would say we should always calculate the gas limit of the transaction even if it is not provided, this way we can use the the getDefaultGasPrice function from #158.

This will also remove the need for having to manually calculate the gas limit sending the transactions that aragon.js returns if not using Metamask (for example when using the dao exec command in the CLI, which uses aragon.js to generate the transaction and then needs to set the gas limit before sending it).

feat(wrapper): introduce getRecommendedGasLimit util
In order to implement #150 this commit introduces a new `getRecommendedGasLimit()` util
which is calculated based on the estimatedGas by web3 and the latest block
available gas limit.

This is similar to how MetaMask handles gas limit proposals.

We now also prefer using the provided gas that is passed to an intent, if it's higher
than the recommended gas limit. In other words, provided gas is treated as
minimum gas required to perform the transaction/intent.

Closes #150
@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 17, 2018

@izqui addressed your comments. PTAL.

Also, I've rebased #158 into this PR. Would be cool if those commits could stay separate when merging :)

@izqui

izqui approved these changes Sep 17, 2018

@izqui

This comment has been minimized.

Copy link
Member

izqui commented Sep 17, 2018

Added some minor fixes that came up while testing the changes and updated the documentation.

@izqui izqui merged commit 4416eed into aragon:master Sep 17, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@PascalPrecht PascalPrecht deleted the PascalPrecht:feat/min-gas branch Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment