Always use estimateGas when computing forgeOrg gas #349

dkent600 · 2018-09-07T07:29:19Z

Resolves: #342

always estimate gas when computing gas for forgeOrg and GenesisProtocol.new, regardless of the "estimateGas" configuration setting
caps gas limit on any attempt to override the default gas limit on a transaction
rewrites the createDaoScript to use DAO.new
uses AbsoluteVote for schemes (other than ContributionReward) in the createGenesisDao script
removes arcConstants.js and gasLimits.js, no more dependency on gas numbers from Arc
The default gas limit used by truffle is now stored in defaults.json as a configurable setting
when the "estimateGas" config setting is true, we now also estimate gas for all contract .new operations.

orenyodfat · 2018-09-12T05:41:04Z

lib/wrappers/daoCreator.ts

+      controllerAddress,
+      options.tokenCap];
+
+    // always use estimateGas or will fail when using non-universal controller


"or will fail when using non-universal controller ? why ?

Added explanation to the comment in the code.

orenyodfat · 2018-09-12T05:44:11Z

gasLimits.js

@@ -10,34 +10,16 @@ const arcForgeOrgGasLimit = require("./arcConstants.js").ARC_GAS_LIMIT;
 const gasLimitsConfig =
 {
  /**
-   * The gas limit used by Arc to forge a DAO with foundersInGasLimitArc founders
-   * and to `new` GenesisProtocol.
+   * The gas limit used by Arc to `new` GenesisProtocol.


This comment is not clear.please clarify

Clarified in the comment in the code.

orenyodfat · 2018-09-12T05:44:53Z

gasLimits.js

-  /**
-   * How many founders are already accounted-for in the gaslimit from Arc
-   */
-  "foundersInGasLimitArc": 3,
  /**
   * Gas limit sufficient (though not necessarily optimal) for all other transactions
   * besides creating DAOs
   */
  "gasLimit_runtime": 4543760,


How this number is calculated ? from where it is taken ?

orenyodfat · 2018-09-12T20:02:48Z

gasLimits.js

@@ -15,7 +15,8 @@ const gasLimitsConfig =
  "gasLimit_arc": arcForgeOrgGasLimit,
  /**
   * Gas limit sufficient (though not necessarily optimal) for all other transactions
-   * besides creating DAOs.  This is taken from the default that truffle puts in truffle.js.
+   * besides creating DAOs and instantiating a new GenesisProtocol.
+   * This is taken from the default that truffle puts in truffle.js.


why did you choose to use truffle default values ? why do we care about truffle default value ?
how did truffle choose this value ?

I think the code that uses this may not be required. It is very old code that actually predates me, and has never, that I recall, been questioned. I will test with it removed and report back.

BTW I am also looking into removing the other setting, gasLimit_arc, a task which is proving to be tricky, but with both of those settings gone we could completely remove some code including the dependency on the Arc test/constants.js file.

dkent600 · 2018-09-13T13:58:46Z

@orenyodfat I have removed arcConstants.js and gasLimits.js, so there is no longer a dependency on gas numbers from Arc.

The default gas limit used by truffle is now stored in defaults.json as a global configuration setting. It is required for certain operations to succeed with enough gas.

I have updated the description in this PR to include all of the recent changes.

orenyodfat · 2018-09-16T06:53:02Z

config/default.json

@@ -4,6 +4,7 @@
  "cacheContractWrappers": true,
  "logLevel": 9,
  "estimateGas": false,
+  "defaultGasLimit": 4543760,


why this value and not other (e.g 4543759) ?
Is there a good reason for that except that "truffle use that" ?

No particular reason except that it works for both Arc and Arc.js automated tests.

lets find out the reason or set it to max gas limit

It is the same reason Arc is using it. I don't see how it makes sense to raise it to the max, as that is wasteful. It would only make sense to lower it to a minimum value that still works. Since Arc tests cover more contract API use cases than Arc.js, I think it would be best to find that minimum value in Arc.

orenyodfat · 2018-09-16T06:57:42Z

lib/contractWrapperBase.ts

-    const web3 = await Utils.getWeb3();
-
-    const maxGasLimit = await computeMaxGasLimit(web3);
+    const maxGasLimit = await UtilsInternal.computeMaxGasLimit();

    if (currentNetwork === "Ganache") {
      return maxGasLimit; // because who cares with ganache and we can't get good estimates from it


Gas estimation for Ganache should be the same as other networks.
In general, we should strive to execute same arc.js code regardless that networks it is connected to.(as possible as can be done).
This line should be removed.

Ganache returns gas estimates that are not sufficient. At the time I wrote this code I spent a lot of time on this issue. I just now gave it another try, but encountered the same result. I am in the process of moving to truffle 5.0 and web3 1.0 -- maybe something will change then, I can retry when I have things working enough to test.

I think this may be a case requiring network-specific code.

Is this a known ganache issue ? If it is please add a link to this issue.

Here is one: trufflesuite/ganache#151

could you please add a link to that on the code ?

orenyodfat · 2018-09-16T07:03:08Z

lib/contractWrapperBase.ts

@@ -321,9 +319,9 @@ export abstract class ContractWrapperBase implements IContractWrapper {
      let error;

      const gasPriceComputer = ConfigService.get("gasPriceAdjustment") as GasPriceAdjustor;
+      const web3 = await Utils.getWeb3();

      if (gasPriceComputer && !web3Params.gasPrice) {


gasPriceComputer -> gasPriceComputed ?

orenyodfat · 2018-09-16T07:04:12Z

lib/migrations/2_deploy_schemes.ts

@@ -53,7 +53,7 @@ export const arcJsDeployer = (
    console.log(`Deploying schemes to ${network}`);

    const DAOToken = await Utils.requireContract("DAOToken");
-    const gasLimit = await computeMaxGasLimit(web3);
+    const gasLimit = await UtilsInternal.computeMaxGasLimit();
    const gasPrice = 10000000000; // 10 Gwei


const gasPrice = 10000000000; // 10 Gwei ?
from where this come from ?

That was your suggestion when we were doing the last complete migration to mainnet and were having issues with the mining taking too long.

The script doubles that in a few specific cases where it doesn't suffice.

fix 10 Gwei is a workaround.
please set the appropriate gas price needed.
You can read that from eth gas station ...(alchemy done that)
https://github.com/daostack/alchemy/blob/dev/src/index.tsx#L21

Yeah, I was thinking of that too. Would you mind waiting on this until I have Truffle 5 working? As you know, they have lots of migration changes and I'd like to code this in the context of those changes. I'm doing the work right now of upgrading to Truffle 5.

please open an issue for that.

orenyodfat

see comments

comments comments remove gasLimits and arcConstants lint comments .gitignore docs typo supposed to use gas estimates in tests

orenyodfat

Please open an issues for all open issues on this pr

dkent600 · 2018-09-20T13:28:22Z

New tickets:

daostack/arc.js#358
daostack/arc.js#357
daostack/arc.js#359

@orenyodfat

dkent600 self-assigned this Sep 7, 2018

dkent600 requested review from tibetsprague, orenyodfat and tsuberim September 7, 2018 07:29

orenyodfat reviewed Sep 12, 2018

View reviewed changes

orenyodfat reviewed Sep 16, 2018

View reviewed changes

orenyodfat suggested changes Sep 16, 2018

View reviewed changes

dkent600 force-pushed the forgeOrgGasFormula branch from 6c4aff5 to 9a8abe4 Compare September 18, 2018 18:26

done

de8dc44

comments comments remove gasLimits and arcConstants lint comments .gitignore docs typo supposed to use gas estimates in tests

dkent600 force-pushed the forgeOrgGasFormula branch from 9a8abe4 to de8dc44 Compare September 18, 2018 18:26

orenyodfat approved these changes Sep 20, 2018

View reviewed changes

dkent600 merged commit 7c58438 into master Sep 20, 2018

dkent600 deleted the forgeOrgGasFormula branch September 20, 2018 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always use estimateGas when computing forgeOrg gas #349

Always use estimateGas when computing forgeOrg gas #349

dkent600 commented Sep 7, 2018 •

edited

Loading

orenyodfat Sep 12, 2018

dkent600 Sep 12, 2018

orenyodfat Sep 12, 2018

dkent600 Sep 12, 2018

orenyodfat Sep 12, 2018

orenyodfat Sep 12, 2018

dkent600 Sep 12, 2018

dkent600 commented Sep 13, 2018 •

edited

Loading

orenyodfat Sep 16, 2018

dkent600 Sep 16, 2018

orenyodfat Sep 16, 2018

orenyodfat Sep 16, 2018

dkent600 Sep 16, 2018

orenyodfat Sep 16, 2018

dkent600 Sep 16, 2018

orenyodfat Sep 16, 2018

dkent600 Sep 16, 2018 •

edited

Loading

orenyodfat Sep 20, 2018

orenyodfat Sep 16, 2018 •

edited

Loading

dkent600 Sep 16, 2018

orenyodfat Sep 16, 2018

dkent600 Sep 16, 2018

dkent600 Sep 16, 2018 •

edited

Loading

orenyodfat Sep 16, 2018 •

edited

Loading

dkent600 Sep 16, 2018

orenyodfat Sep 20, 2018

orenyodfat left a comment

orenyodfat left a comment

dkent600 commented Sep 20, 2018

Always use estimateGas when computing forgeOrg gas #349

Always use estimateGas when computing forgeOrg gas #349

Conversation

dkent600 commented Sep 7, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkent600 commented Sep 13, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkent600 Sep 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenyodfat Sep 16, 2018 • 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

dkent600 Sep 16, 2018 • edited Loading

Choose a reason for hiding this comment

orenyodfat Sep 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenyodfat left a comment

Choose a reason for hiding this comment

orenyodfat left a comment

Choose a reason for hiding this comment

dkent600 commented Sep 20, 2018

dkent600 commented Sep 7, 2018 •

edited

Loading

dkent600 commented Sep 13, 2018 •

edited

Loading

dkent600 Sep 16, 2018 •

edited

Loading

orenyodfat Sep 16, 2018 •

edited

Loading

dkent600 Sep 16, 2018 •

edited

Loading

orenyodfat Sep 16, 2018 •

edited

Loading