-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Revisit gas costs of having Crowdsale deploy the token itself #358
Comments
I'm having the same issue when deploy the sample crowdsale code to Ropsten, even it fails, still costing me 0.5 ether for gas. How did you guys solve this issue? |
Setting the truffle gas limits manually. @spalladino maybe remembers the exact parameters we used. |
The problem was deploying a customised version, so that exact same value
would not apply. You may want to check on etherscan what gas limit is being
set by truffle, and try increasing it.
…On Sat, Aug 12, 2017, 18:10 Francisco Giordano ***@***.***> wrote:
Setting the truffle gas limits manually. @spalladino
<https://github.com/spalladino> maybe remembers the exact parameters we
used.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#358 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAaOJIxdmiFLYXsAOaxmYm-MdxYkeUNTks5sXhS-gaJpZM4O1BqD>
.
|
I'm having the same/similar problem. I cannot deploy the SampleCrowdsale. Im attempting on Ropsten, using Remix, and passing in these parameters to "Create" for |
I'm also having the same/similar problem. I posted about it to stackexchange. I'd appreciate it if you could help me. |
I think the error might be caused by the constructor arguments (not be excessive use of gas) - the first and second Crowdsale constructor arguments are timestamps, not block numbers. Probably the first check throws:
|
Thanks! @gundas As you mentioned, I could deploy it after change to timestamp. I'm sorry please forget my issue. I updated my stackexchange post. |
I keep getting this error while trying to deploy BTW, I use timestamps for the first constructor arguments, as @gundas mentioned. |
@tomericco keep in mind that that error is also thrown when there is an error during the constructor execution, and may actually be unrelated to the gas amount. Make sure you are not breaking any precondition of the Crowdsale constructor, such as timestamps needing to be in the future, with start less than end. |
@spalladino Here is my Truffle deployment script:
Do you see anything suspicious? |
Yup, startTime is not enough in the future. You are using 1 second after
the timestamp of the current block, and the contract won't be included
until at least the next block, which will be mined about 20s after the
current one. Try changing start time to +120 instead of +1.
…On Tue, Oct 10, 2017 at 10:23 AM, Tomer Gabbai ***@***.***> wrote:
@spalladino <https://github.com/spalladino> Here is my Truffle deployment
script:
const Crowdsale = artifacts.require('zeppelin-solidity/contracts/crowdsale/Crowdsale.sol');
module.exports = function(deployer, network, accounts) {
const startTime = web3.eth.getBlock(web3.eth.blockNumber).timestamp + 1;
const endTime = startTime + (86400 * 30); // 30 days
const ethRate = new web3.BigNumber(1000);
const wallet = accounts[0];
deployer.deploy(Crowdsale, startTime, endTime, ethRate, wallet);
};
Do you see anything suspicious?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#358 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAaOJM6qz8WCZsXtXVIWEQcByvkDFF5Lks5sq2-_gaJpZM4O1BqD>
.
|
It works now. This is why it worked on the local TestRPC, and not on Ropsten. |
my setting rinkeby: {
host: 'localhost',
port: 8545,
network_id: 4,
gas: 6000000
} and set |
This is driving me crazy, i now get a different error. Using network 'ropsten'. Running migration: 1_initial_migration.js |
Moving to |
I've been playing around a bit with removing the token creation from the crowdsale contract, and requiring it as a parameter. The code change would be the following: - function Crowdsale(uint256 _startTime, uint256 _endTime, uint256 _rate, address _wallet) public {
+ function Crowdsale(uint256 _startTime, uint256 _endTime, uint256 _rate, address _wallet, MintableToken _token) public {
require(_startTime >= now);
require(_endTime >= _startTime);
require(_rate > 0);
require(_wallet != address(0));
- token = createTokenContract();
+ token = _token;
startTime = _startTime;
endTime = _endTime;
rate = _rate;
wallet = _wallet;
}
- // creates the token to be sold.
- // override this method to have crowdsale of a specific mintable token.
- function createTokenContract() internal returns (MintableToken) {
- return new MintableToken();
- } The gas costs for the current implementation with the embedded token contract code, vs this new implementation with the token as a parameter, with the solc optimizer on and off, are the following:
The costs of deploying the mintable token and then the sale are shown separately as the two addends in each cell. The cost of the transfer ownership is missing, though it is negligible (~30K in a not optimized version). The code I used for testing both versions was the following: // Current version
ts = web3.eth.getBlock('latest').timestamp
Crowdsale.new(ts + 100, ts + 1000, 1000, 0x1).then(c => sale = c)
t = web3.eth.getTransaction(sale.transactionHash)
web3.eth.estimateGas({from: web3.eth.accounts[0], to: 0x0, data: t.input})
// New version
MintableToken.new().then(m => token = m)
web3.eth.estimateGas({from: web3.eth.accounts[0], to: 0x0, data: web3.eth.getTransaction(token.transactionHash).input})
ts = web3.eth.getBlock('latest').timestamp
Crowdsale.new(ts + 100, ts + 1000, 1000, 0x1, token.address).then(c => sale = c)
web3.eth.estimateGas({from: web3.eth.accounts[0], to: 0x0, data: web3.eth.getTransaction(sale.transactionHash).input}) I think we have enough arguments to move the token creation outside of the crowdsale contract, right? |
ACK, nice work @spalladino! :) |
Do you have an example of how one would call the transferOwnership() function on the token in the 2_deploy_contracts.js file to be able to use this new functionality? |
@gururise sure, check out the setup step in the tests with this new feature: let token = await MintableToken.new();
let crowdsale = await Crowdsale.new(startTime, endTime, rate, wallet, token.address);
await token.transferOwnership(crowdsale.address); |
Getting the error:
Using @tomericco code with seconds modification:
All the suggestions above don't work including @amazingandyyy
Edit: Managed to import
|
@propercoil maybe try increasing the number of seconds for startTime from |
@spalladino it wasn't that. I was looking at master when my repo was on the release. Turned out to be a missed constructor param. Need to remember that a |
@spalladino your answer to @tomericco solved me a several days lasting frustration! |
@spalladino Nice changes ! |
The bytecode that results from having
Crowdsale
deploy the token itself is too big and causes the deployment to be too costly.Related to #351. Should be fixed the same way.
The text was updated successfully, but these errors were encountered: