Skip to content
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

Always use estimateGas when computing forgeOrg gas #349

Merged
merged 1 commit into from
Sep 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,3 @@ yarn.lock
test-build/
site/
docs/api/
arcConstants.js
1 change: 1 addition & 0 deletions config/default.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"cacheContractWrappers": true,
"logLevel": 9,
"estimateGas": false,
"defaultGasLimit": 4543760,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quote-there-is-nothing-without-reason-gottfried-leibniz-126-13-19-2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"gasPriceAdjustor": null,
"txDepthRequiredForConfirmation": {
"default": 7,
Expand Down
3 changes: 3 additions & 0 deletions docs/Configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ Automatically approve token transfers for operations that require the sender pay
**cacheContractWrappers**
`true` to cache contract wrappers obtained using the contract wrapper factory methods `.at` and `.new`. The cache is local, it does not persist across application instances. The default is `false`.

**defaultGasLimit**
The default gas limit used for most operations when "estimateGas" is false.

**defaultVotingMachine**
The voting machine used by default by `Dao.new` when creating new DAOs. Default is "AbsoluteVote".

Expand Down
3 changes: 0 additions & 3 deletions docs/Migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@ Arc.js ships with contracts already migrated to Kovan and MainNet. But you may
!!! warning
The mnemonic won't work unless it confirms to [BIP39](https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki). You can generate examples of conformant mnemonics [here](https://iancoleman.io/bip39/).

!!! info
The migration script will use the gas settings defined in the Arc.js file `arc.js/gasLimits.js`. The gas limit when migrating/creating Daos is computed dynamically as a function of the number of founders.

3. Provide a list of Genesis DAO founders as described in [configuring founders](#configuring-founders).

4. If deploying to ganache, then run `npm start ganache`, or from your application: `npm explore @daostack/arc.js -- npm start ganache`.
Expand Down
57 changes: 0 additions & 57 deletions gasLimits.js

This file was deleted.

19 changes: 10 additions & 9 deletions lib/contractWrapperBase.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { BigNumber } from "bignumber.js";
import { promisify } from "es6-promisify";
import { computeMaxGasLimit } from "../gasLimits.js";
import { Address, Hash, SchemePermissions } from "./commonTypes";
import { ConfigService } from "./configService";
import { ControllerService } from "./controllerService";
Expand All @@ -20,6 +19,7 @@ import {
TxGeneratingFunctionOptions
} from "./transactionService";
import { Utils } from "./utils";
import { UtilsInternal } from "./utilsInternal";
import { EventFetcherFactory, Web3EventService } from "./web3EventService";

/**
Expand Down Expand Up @@ -176,9 +176,7 @@ export abstract class ContractWrapperBase implements IContractWrapper {

const currentNetwork = await Utils.getNetworkName();

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dkent600 dkent600 Sep 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down Expand Up @@ -313,14 +311,14 @@ export abstract class ContractWrapperBase implements IContractWrapper {
try {
let error;

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

if (gasPriceComputer && !web3Params.gasPrice) {
const web3 = await Utils.getWeb3();
if (gasPriceComputer) {
if (gasPriceComputed && !web3Params.gasPrice) {
if (gasPriceComputed) {
const defaultGasPrice =
await promisify((callback: any): void => { web3.eth.getGasPrice(callback); })() as BigNumber;
web3Params.gasPrice = await gasPriceComputer(defaultGasPrice);
web3Params.gasPrice = await gasPriceComputed(defaultGasPrice);
}
LoggingService.debug(
`invoking function with configured gasPrice: ${web3.fromWei(web3Params.gasPrice, "gwei")}`);
Expand All @@ -337,6 +335,9 @@ export abstract class ContractWrapperBase implements IContractWrapper {
LoggingService.error(`estimateGas failed: ${ex}`);
error = ex;
});
} else if (web3Params.gas) {
// cap any already-given gas limit
web3Params.gas = Math.min(web3Params.gas, await UtilsInternal.computeMaxGasLimit());
}

if (error) {
Expand Down
36 changes: 36 additions & 0 deletions lib/contractWrapperFactory.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { promisify } from "es6-promisify";
import { Address } from "./commonTypes";
import { ConfigService } from "./configService";
import { IConfigService } from "./iConfigService";
import { IContractWrapper, IContractWrapperFactory } from "./iContractWrapperBase";
import { LoggingService } from "./loggingService";
import { Utils } from "./utils";
import { UtilsInternal } from "./utilsInternal";
import { Web3EventService } from "./web3EventService";

/**
Expand Down Expand Up @@ -44,6 +47,17 @@ export class ContractWrapperFactory<TWrapper extends IContractWrapper>
public async new(...rest: Array<any>): Promise<TWrapper> {
await this.ensureSolidityContract();

let gas;

if (ConfigService.get("estimateGas") && (!rest || !rest.length || (!rest[rest.length - 1].gas))) {
gas = await this.estimateConstructorGas(...rest);
LoggingService.debug(`Instantiating ${this.solidityContractName} with gas: ${gas}`);
}

if (gas) {
rest = [...rest, { gas }];
}

const hydratedWrapper =
await new this.wrapper(this.solidityContract, this.web3EventService).hydrateFromNew(...rest);

Expand Down Expand Up @@ -84,6 +98,28 @@ export class ContractWrapperFactory<TWrapper extends IContractWrapper>
return this.getHydratedWrapper(getWrapper);
}

protected async estimateConstructorGas(...params: Array<any>): Promise<number> {
const web3 = await Utils.getWeb3();
await this.ensureSolidityContract();
const callData = (web3.eth.contract(this.solidityContract.abi).new as any).getData(
...params,
{
data: this.solidityContract.bytecode,
});

const currentNetwork = await Utils.getNetworkName();

const maxGasLimit = await UtilsInternal.computeMaxGasLimit();

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

const gas = await promisify((callback: any) => web3.eth.estimateGas({ data: callData }, callback))() as number;

return Math.max(Math.min(gas, maxGasLimit), 21000);
}

private async getHydratedWrapper(
getWrapper: () => Promise<TWrapper>,
address?: Address): Promise<TWrapper> {
Expand Down
3 changes: 1 addition & 2 deletions lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ export * from "./proposalGeneratorBase";
export * from "./loggingService";
export * from "./transactionService";
export * from "./utils";
export const computeForgeOrgGasLimit: any = require("../gasLimits.js").computeForgeOrgGasLimit;

import { Web3 } from "web3";
import { AccountService } from "./accountService";
Expand Down Expand Up @@ -85,7 +84,7 @@ export async function InitializeArcJs(options?: InitializeArcOptions): Promise<W
*/
ContractWrapperFactory.setConfigService(ConfigService);
/**
* Initialize LoggingService here to avoid cirular dependency involving ConfigService and PubSubService
* Initialize LoggingService here to avoid circular dependency involving ConfigService and PubSubService
*/
LoggingService.logLevel = parseInt(ConfigService.get("logLevel"), 10) as LogLevel;
/**
Expand Down
4 changes: 2 additions & 2 deletions lib/migrations/2_deploy_schemes.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Web3 } from "web3";
import { Utils } from "../utils";
import { UtilsInternal } from "../utilsInternal";
/* tslint:disable-next-line:no-var-requires */
const computeMaxGasLimit: any = require("../../gasLimits.js").computeMaxGasLimit;
/* tslint:disable-next-line:no-var-requires */
const env = require("env-variable")();

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dkent600 dkent600 Sep 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@orenyodfat orenyodfat Sep 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please open an issue for that.

let genTokenAddress;

Expand Down
Loading