-
Notifications
You must be signed in to change notification settings - Fork 14
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,4 +12,3 @@ yarn.lock | |
test-build/ | ||
site/ | ||
docs/api/ | ||
arcConstants.js |
This file was deleted.
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"; | ||
|
@@ -20,6 +19,7 @@ import { | |
TxGeneratingFunctionOptions | ||
} from "./transactionService"; | ||
import { Utils } from "./utils"; | ||
import { UtilsInternal } from "./utilsInternal"; | ||
import { EventFetcherFactory, Web3EventService } from "./web3EventService"; | ||
|
||
/** | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gas estimation for Ganache should be the same as other networks. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is one: trufflesuite/ganache#151 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you please add a link to that on the code ? |
||
|
@@ -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")}`); | ||
|
@@ -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) { | ||
|
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")(); | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. const gasPrice = 10000000000; // 10 Gwei ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fix 10 Gwei is a workaround. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please open an issue for that. |
||
let genTokenAddress; | ||
|
||
|
There was a problem hiding this comment.
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" ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.