Estimate gas for all contract deployments and transactions #211

spalladino · 2018-07-17T14:40:47Z

Add new deploy function to deploy contracts running an estimateGas
call before to send the precise amount of gas needed. All classes
now use this function to deploy both user and library contracts.

Add new sendTransaction function to estimate gas on a transaction
to a contract as well.

Fixes zeppelinos/zos-cli#312
Fixes #212

spalladino · 2018-07-17T14:41:53Z

src/release/ReleaseDeployer.js

@@ -42,15 +43,15 @@ export default class ReleaseDeployer {
  async _deployLocalContract(contractName) {
    const contractClass = Contracts.getFromLib(contractName)
    log.info(`Deploying new ${contractName}...`)
-    const implementation = await contractClass.new()
+    const implementation = await deploy(contractClass, [], this.txParams)


Note that txParams was missing here

Add new Deploy function to deploy contracts running an estimateGas call before to send the precise amount of gas needed. All classes now use this function to deploy both user and library contracts.

frangio

Looks good!

We should be using all lowercase for deploy, since it's a function. So the new file should be utils/deploy.js, and the export in index.js should be deploy.

Should we leave a comment somewhere saying that this is a temporary patch for something that should be handled by the underlying Contract class?

frangio · 2018-07-17T18:32:40Z

src/utils/Deploy.js

+
+  // Get raw binary transaction for creating the contract
+  const txOpts = { data: contract.binary, ... txParams };
+  const txData = web3.eth.contract(contract.abi).new.getData(...args, txOpts);


I think you can get the underlying web3.js Contract instance through contract.contract. (See here.)

For some reason it's failing, so I'll keep using this way of setting up the web3 contract

frangio · 2018-07-17T19:02:58Z

test/src/utils/Deploy.test.js

+      });
+    });
+
+    describe('with a constructor', function () {


What is the behavior when the constructor reverts? We should probably add tests for this scenario too.

facuspagnuolo

Looking good to me! I'd use the new deploy function in our tests too

facuspagnuolo · 2018-07-17T19:10:03Z

src/utils/Deploy.js

+  return contract.new(...args, { gas: estimatedGas, ... txParams });
+}
+
+export default deploy;


nitpick: you can export the function from the first line with export default async function ...

facuspagnuolo · 2018-07-17T19:10:58Z

src/utils/Deploy.js

+        else resolve(gas);
+      }
+    );
+  });


should we add an estimation of the gas price in case no one was given?

I'd rather have the function fail actually

spalladino · 2018-07-19T20:09:59Z

@facuspagnuolo @frangio I've added a new commit addressing your comments, and also with a new sendTransaction method to tackle #212. Please re-review.

frangio

Do we have any idea as to the performance cost of estimating all transactions before sending them?

I'm not sure why the file with the helpers is called Transactions.js! Maybe it would be better to call it gasEstimation.js.

frangio · 2018-07-19T20:11:11Z

src/release/Release.js

@@ -26,29 +27,29 @@ export default class Release {
  }

  async owner() {
-    return await this._release.owner(this.txParams)


Does removing this.txParams here have anything to do with gas estimation? Otherwise, why remove it?

I preferred to remove the txParams, which have information for transactions (such as sender), from all the static calls. They should not make any difference at all.

frangio · 2018-07-19T20:19:58Z

test/src/utils/Transactions.test.js

+      });
+
+      it('handles failing constructors', async function () {
+        await assertRevert(deploy(this.WithConstructorImplementation, [0, "foo"]));


My previous question rather was about how estimateGas behaves if the underlying transaction reverts. I suppose it estimates how much gas is needed to hit the revert?

I have a feeling that I've seen some tools (MyCrypto?) warn me before sending a transaction that is guaranteed to revert, and gas estimation sounds like the moment when they can figure that out. Maybe we should just open another issue to consider implementing such warnings. WDYT?

Actually estimateGas returns an error if the transaction fails. On geth I'm getting a Error: gas required exceeds allowance or always failing transaction. This means that we will never send the tx, but actually fail beforehand. So this optimization is already in place :-)

spalladino · 2018-07-19T21:18:32Z

@frangio:

Do we have any idea as to the performance cost of estimating all transactions before sending them?

That will depend almost exclusively on the available resources of the node executing the estimateGas call. However, that cost is negligible compared with the time that takes to actually mine that transaction.

I'm not sure why the file with the helpers is called Transactions.js! Maybe it would be better to call it gasEstimation.js.

I wanted to use a name generic enough so that any further improvements we make to the sendTx and deploy methods can be placed here. Eventually, if we swap out truffle, I'd expect to handle the calls to the new fwk in this class directly. But I agree that it is not representative of what the functions in this file do. I can change it if you think it's better for now.

facuspagnuolo

I really like we found a solution for this issue, but I'd prefer to have these functions (deploy and sendTx) already in our wrapped contracts so any users don't have to learn a new way of deploying or calling a contract. This came up to my mind after all the modifications we needed to do to our code to use it. Which means, everyone using zos-lib that wants to have this improvement will need to do the same. What do you think guys?

facuspagnuolo · 2018-07-20T02:55:45Z

changelog.md

@@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

 ## [Unreleased]

+### Changed
+- Contract deployments are executed with an estimate of the gas needed, instead of using the network default


Add the sendTransaction function if we decide to keep it as is, actually we can add both functions in the "Added" section since they are being exported

spalladino · 2018-07-20T13:36:07Z

I'd prefer to have these functions (deploy and sendTx) already in our wrapped contracts so any users don't have to learn a new way of deploying or calling a contract. This came up to my mind after all the modifications we needed to do to our code to use it. Which means, everyone using zos-lib that wants to have this improvement will need to do the same.

I disagree there. As much as I love the code I wrote myself and would like to see everyone else using it, I don't like forcing a feature down a user's throat. The helpers are available if a user wants to use them, but are not in the way or mandatory.

Just think how much we have complained when we stumble upon something we cannot change on the toolchain (whether it is truffle or web3). This is the typical framework vs library dichotomy: whether you want something to already provide you with all sensible defaults for you to work, or you have to manually opt-in into every feature you want to pick. Point is, when the framework has a bug or something you actually don't want, it's a real PITA.

Anyway, I'll cut the rambling here. I'd prefer merging this as-is, and then revisit this once we start seriously reviewing the entire toolchain.

facuspagnuolo · 2018-07-20T16:53:17Z

I see your point, I'm actually kind of in favor of what you are saying, maybe I didn't make myself clear. With "in our wrapped contracts" I meant having those functions integrated with the Contracts object we are providing from zos-lib. Which is actually aligned with your opinion, we are not forcing the users, if they decide to use our contracts wrappers they are actually choosing our code. Note that we could also provide different alternatives from the Contracts object indeed, and it could be opt-in.

My idea was just to see how this new functionality would fit being integrated with our Contracts object, considering that, IMO, it will improve the DX if we decide to use it since you don't need to change the way you send a transaction or the way you deploy a contract, it would be built-in.

I do agree with merging it as is for now, we can keep improving it later

spalladino · 2018-07-20T18:03:23Z

Got it. Although I'm still not 100% convinced, I understand that it makes sense that if you request a contact via zos-lib, that contract has all the goodies that zos-lib has to offer. I'll open a new issue for that.

…s/zos-lib#211) * Deployer function for estimating gas upon deploy * Estimate gas in all transactions via sendTransaction method

spalladino commented Jul 17, 2018

View reviewed changes

Deployer function for estimating gas upon deploy

2ac1fa7

Add new Deploy function to deploy contracts running an estimateGas call before to send the precise amount of gas needed. All classes now use this function to deploy both user and library contracts.

spalladino force-pushed the feature/estimate-gas-on-deployments branch from d09b0f7 to 2ac1fa7 Compare July 17, 2018 18:20

frangio reviewed Jul 17, 2018

View reviewed changes

facuspagnuolo approved these changes Jul 17, 2018

View reviewed changes

spalladino changed the title ~~Deployer function for estimating gas upon deploy~~ Estimate gas for all contract deployments and transactions Jul 19, 2018

frangio approved these changes Jul 19, 2018

View reviewed changes

facuspagnuolo reviewed Jul 20, 2018

View reviewed changes

Estimate gas in all transactions via sendTransaction method

50fe666

spalladino force-pushed the feature/estimate-gas-on-deployments branch from a15130c to 50fe666 Compare July 20, 2018 13:37

spalladino mentioned this pull request Jul 20, 2018

Support creating non-upgradeable contract instances #209

Merged

spalladino merged commit c8008bc into master Jul 20, 2018

spalladino deleted the feature/estimate-gas-on-deployments branch July 20, 2018 14:17

spalladino mentioned this pull request Jul 20, 2018

Discuss overloading transaction function calls with gas estimation in contract wrapper #214

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Estimate gas for all contract deployments and transactions #211

Estimate gas for all contract deployments and transactions #211

spalladino commented Jul 17, 2018 •

edited

Loading

spalladino Jul 17, 2018

frangio left a comment

frangio Jul 17, 2018

spalladino Jul 19, 2018

frangio Jul 17, 2018

facuspagnuolo left a comment

facuspagnuolo Jul 17, 2018

facuspagnuolo Jul 17, 2018

spalladino Jul 19, 2018

spalladino commented Jul 19, 2018 •

edited

Loading

frangio left a comment •

edited

Loading

frangio Jul 19, 2018

spalladino Jul 19, 2018

frangio Jul 19, 2018

spalladino Jul 19, 2018

spalladino commented Jul 19, 2018

facuspagnuolo left a comment

facuspagnuolo Jul 20, 2018

spalladino commented Jul 20, 2018

facuspagnuolo commented Jul 20, 2018 •

edited

Loading

spalladino commented Jul 20, 2018

Estimate gas for all contract deployments and transactions #211

Estimate gas for all contract deployments and transactions #211

Conversation

spalladino commented Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

frangio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino commented Jul 19, 2018 • edited Loading

frangio left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino commented Jul 19, 2018

facuspagnuolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino commented Jul 20, 2018

facuspagnuolo commented Jul 20, 2018 • edited Loading

spalladino commented Jul 20, 2018

spalladino commented Jul 17, 2018 •

edited

Loading

spalladino commented Jul 19, 2018 •

edited

Loading

frangio left a comment •

edited

Loading

facuspagnuolo commented Jul 20, 2018 •

edited

Loading