(Fix) Invest gas price zero #434
Conversation
ghost
assigned 
Black Duck Security ReportMerging #434 into master will decrease security risk! Added ComponentsMedium Risk: 1 |
src/components/invest/index.js
Outdated
| sendTXToContract(web3, crowdsaleContract.methods.buy().send(opts)) | ||
| crowdsaleContract.methods.buy().estimateGas(opts) | ||
| .then(estimatedGas => { | ||
| const estimatedGasMax = 362897 |
There was a problem hiding this comment.
@fernandomg is estimatedGasMax for 1-tier crowdsale without whitelist? In this case, estimatedGasMax will be greater for multiple tiers contract with whitelist. Because investing is more expensive, if whitelist is enabled due to https://github.com/poanetwork/ico/blob/master/contracts/CrowdsaleExt.sol#L239 and even more expensive, if whitelist + multiple tiers are switched on due to https://github.com/poanetwork/ico/blob/master/contracts/CrowdsaleExt.sol#L286
There was a problem hiding this comment.
oh, you're completely right! I'll review that. Thanks!
There was a problem hiding this comment.
@vbaranov
I've been testing this in kovan.
Created a 3-tier crowdsale (https://wizard.poa.network/invest?addr=0xcCf27643fa6C4FC844a8945b7c2F8bd562153649&networkID=42) (BTW: there's a bug with the Balance, that I'm about to fix)
And bought tokens with 0x90f8bf6a479f320ead074411a4b0e7944ea8c9c1 (first ganache account on deterministic mode) on every tier.
This are the results (max used gas was for 1.i. with 240139):
- First Tier:
- Second Tier:
- Third Tier:
Please, give me a hint if there's something I'm missing out.
There was a problem hiding this comment.
Yes, it seems that max gas indeed should be for investing at the 1st tier due to https://github.com/poanetwork/ico/blob/master/contracts/CrowdsaleExt.sol#L294.
But the gas used for tx should depend on the number of tiers. I suggest taking gas estimation for estimatedGasMax as investing in the 1st tier of 10-tiers crowdsale with whitelist. 10 - is the max amount of tiers I have ever seen in the real use cases.
There was a problem hiding this comment.
@vbaranov I tested locally with a 12-tiers crowdsale and the max gas used was 376086 for the first tier. If it's Ok, what I'll do is to set estimatedGasMax to the same value than https://github.com/poanetwork/ico-wizard/blob/40978914427610c53f3393abc78979ad243a01ee/src/utils/blockchainHelpers.js#L116
BTW: While I was creating the crowdsale, found 2 tx that went out of gas:
https://github.com/poanetwork/ico-wizard/blob/40978914427610c53f3393abc78979ad243a01ee/src/components/stepFour/utils.js#L132
Let me know if I fix it in a different PR, or should I consider it as part of this research/fix.
There was a problem hiding this comment.
If it's Ok, what I'll do is to set estimatedGasMax to the same value
Sounds good
Let me know if I fix it in a different PR, or should I consider it as part of this research/fix.
Let's separate to different PRs
Closes #433