From d76a82a30a4dd137cd89fde8bd7af813e4f69780 Mon Sep 17 00:00:00 2001 From: Pascal Precht Date: Fri, 28 Jun 2019 14:39:51 +0200 Subject: [PATCH] fix(@embark/deployment): don't over estimate gas when running tests against non-simulator nodes When running tests against non-simulated blockchain nodes, even for simplex Smart Contracts, deployment transactions would exceed the block gas limit. E.g. running `embark blockchain` in one process and `embark test --node embark` in another, inside our demo application, will throw an error when Embark attempts to deploy its `SimpleStorage`: ``` Compiling contracts Compilation done [SimpleStorage]: error deploying =SimpleStorage= due to error: Returned error: exceeds block gas limit Error deploying contracts. Please fix errors to continue. Error deploying contracts. Please fix errors to continue. terminating due to error Error deploying contracts. Please fix errors to continue. ``` The reason for that is because in https://github.com/embark-framework/embark/pull/1650, we've introduced a static gas estimation for Smart Contract deployment that is just right below Ganache's maximum gas limit of `6721975`, since Ganache tends to underestimate gas for complex Smart Contracts due to its [low base fee](https://github.com/trufflesuite/ganache-core/blob/8ad1ab29deccbbb4018f6961d0eb7ec984ad8fcb/lib/utils/gasEstimation.js#L33-L39). The static gas estimation would apply any time we're in a test context, but we didn't take into account the case where tests are executed against nodes other than the simulated environment. As mentioned in the comments in the linked PR: > If this is not spec'ed at all, I wonder what complications it could cause when > at some point we maybe switch to not using Ganache anymore for tests, or even > the user itself (which I think is a reasonable thing to do). This causes the error described above because we easily reach the block gas limit with just two Smart Contracts and Embark already deploys a few Smart Contracts for ENS. So basically what we want is to use the static gas estimation when we know the node we're connecting to is Ganache. In all other cases we can rely on the standardized gas estimation offered by the node. --- .../embark-blockchain-connector/src/index.js | 4 ++ .../src/contract_deployer.js | 39 ++++++++++--------- packages/embark-deployment/src/index.js | 2 - 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/packages/embark-blockchain-connector/src/index.js b/packages/embark-blockchain-connector/src/index.js index 5fa08077d..36653924f 100644 --- a/packages/embark-blockchain-connector/src/index.js +++ b/packages/embark-blockchain-connector/src/index.js @@ -715,6 +715,10 @@ class BlockchainConnector { }); } + getClientVersion(cb) { + this.web3._requestManager.send({method: 'web3_clientVersion', params: []}, cb); + } + getNetworkId() { return this.web3.eth.net.getId(); } diff --git a/packages/embark-deployment/src/contract_deployer.js b/packages/embark-deployment/src/contract_deployer.js index dd1e01cb0..a7e0dc801 100644 --- a/packages/embark-deployment/src/contract_deployer.js +++ b/packages/embark-deployment/src/contract_deployer.js @@ -5,6 +5,7 @@ const {ZERO_ADDRESS} = AddressUtils; // Check out definition 97 of the yellow paper: https://ethereum.github.io/yellowpaper/paper.pdf const MAX_CONTRACT_BYTECODE_LENGTH = 24576; +const GANACHE_CLIENT_VERSION_NAME = "EthereumJS TestRPC"; class ContractDeployer { constructor(options) { @@ -320,24 +321,26 @@ class ContractDeployer { next(); }, function estimateCorrectGas(next) { - if (contract._skipGasEstimations) { - // This is Ganache's gas limit. We subtract 1 so we don't reach the limit. - // - // We do this because Ganache's gas estimates are wrong (contract creation - // has a base cost of 53k, not 21k, so it sometimes results in out of gas - // errors.) - contract.gas = 6721975 - 1; - } else if (contract.gas === 'auto' || !contract.gas) { - return self.blockchain.estimateDeployContractGas(deployObject, (err, gasValue) => { - if (err) { - return next(err); - } - let increase_per = 1 + (Math.random() / 10.0); - contract.gas = Math.floor(gasValue * increase_per); - next(); - }); - } - next(); + self.blockchain.getClientVersion((err, version) => { + if (version.split('/')[0] === GANACHE_CLIENT_VERSION_NAME) { + // This is Ganache's gas limit. We subtract 1 so we don't reach the limit. + // + // We do this because Ganache's gas estimates are wrong (contract creation + // has a base cost of 53k, not 21k, so it sometimes results in out of gas + // errors.) + contract.gas = 6721975 - 1; + } else if (contract.gas === 'auto' || !contract.gas) { + return self.blockchain.estimateDeployContractGas(deployObject, (err, gasValue) => { + if (err) { + return next(err); + } + let increase_per = 1 + (Math.random() / 10.0); + contract.gas = Math.floor(gasValue * increase_per); + next(); + }); + } + next(); + }); }, function deployTheContract(next) { let estimatedCost = contract.gas * contract.gasPrice; diff --git a/packages/embark-deployment/src/index.js b/packages/embark-deployment/src/index.js index e40812f8b..de58e15c4 100644 --- a/packages/embark-deployment/src/index.js +++ b/packages/embark-deployment/src/index.js @@ -36,7 +36,6 @@ class DeployManager { this.events.setCommandHandler('deploy:contracts:test', (cb) => { self.fatalErrors = true; self.deployOnlyOnConfig = true; - self.skipGasEstimations = true; self.deployContracts(cb); }); } @@ -71,7 +70,6 @@ class DeployManager { callback = result; } contract._gasLimit = self.gasLimit; - contract._skipGasEstimations = self.skipGasEstimations; self.events.request('deploy:contract', contract, (err) => { if (err) { contract.error = err.message || err;