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

Eliminate throw() per issue #44. #61

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 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
2 changes: 1 addition & 1 deletion contracts/AllocatedCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,6 @@ contract AllocatedCrowdsale is Crowdsale {
* Use approve() given to this crowdsale to distribute the tokens.
*/
function assignTokens(address receiver, uint tokenAmount) private {
if(!token.transferFrom(beneficiary, receiver, tokenAmount)) throw;
require(token.transferFrom(beneficiary, receiver, tokenAmount));
}
}
13 changes: 2 additions & 11 deletions contracts/BonusFinalizeAgent.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,17 +35,10 @@ contract BonusFinalizeAgent is FinalizeAgent {
uint public allocatedBonus;

function BonusFinalizeAgent(CrowdsaleToken _token, Crowdsale _crowdsale, uint _bonusBasePoints, address _teamMultisig) {
require(address(_crowdsale) != 0 && address(_teamMultisig) != 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be two separate require()s?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the compliments, changed to two separate require()s as requested.

token = _token;
crowdsale = _crowdsale;
if(address(crowdsale) == 0) {
throw;
}

teamMultisig = _teamMultisig;
if(address(teamMultisig) == 0) {
throw;
}

bonusBasePoints = _bonusBasePoints;
}

Expand All @@ -56,9 +49,7 @@ contract BonusFinalizeAgent is FinalizeAgent {

/** Called once by crowdsale finalize() if the sale was success. */
function finalizeCrowdsale() {
if(msg.sender != address(crowdsale)) {
throw;
}
require(msg.sender == address(crowdsale));

// How many % of tokens the founders and others get
uint tokensSold = crowdsale.tokensSold();
Expand Down
86 changes: 29 additions & 57 deletions contracts/Crowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -127,34 +127,23 @@ contract Crowdsale is Haltable {

function Crowdsale(address _token, PricingStrategy _pricingStrategy, address _multisigWallet, uint _start, uint _end, uint _minimumFundingGoal) {

require(_multisigWallet != 0);
require(_start != 0 && _end != 0);

// Don't mess the dates
require(_start < _end);

owner = msg.sender;

token = FractionalERC20(_token);

setPricingStrategy(_pricingStrategy);

multisigWallet = _multisigWallet;
if(multisigWallet == 0) {
throw;
}

if(_start == 0) {
throw;
}

startsAt = _start;

if(_end == 0) {
throw;
}

endsAt = _end;

// Don't mess the dates
if(startsAt >= endsAt) {
throw;
}

// Minimum funding goal can be zero
minimumFundingGoal = _minimumFundingGoal;
}
Expand All @@ -163,7 +152,7 @@ contract Crowdsale is Haltable {
* Don't expect to just send in money and get tokens.
*/
function() payable {
throw;
require(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could remove the "payable" modifier and the require() to make an empty function (In my opinion the function should be stay there anyway to signal that this is intentional)

Copy link
Author

Choose a reason for hiding this comment

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

Removed "payable" modifier and require() to make empty function as requested.

}

/**
Expand All @@ -181,26 +170,21 @@ contract Crowdsale is Haltable {
// Determine if it's a good time to accept investment from this participant
if(getState() == State.PreFunding) {
// Are we whitelisted for early deposit
if(!earlyParticipantWhitelist[receiver]) {
throw;
}
require(earlyParticipantWhitelist[receiver]);
} else if(getState() == State.Funding) {
// Retail participants can only come in when the crowdsale is running
// pass
} else {
// Unwanted state
throw;
assert(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be probably revert()?

Copy link
Author

Choose a reason for hiding this comment

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

Changed to revert() as requested.

}

uint weiAmount = msg.value;

// Account presale sales separately, so that they do not count against pricing tranches
uint tokenAmount = pricingStrategy.calculatePrice(weiAmount, weiRaised - presaleWeiRaised, tokensSold, msg.sender, token.decimals());

if(tokenAmount == 0) {
// Dust transaction
throw;
}
require(tokenAmount != 0); // Dust transaction

if(investedAmountOf[receiver] == 0) {
// A new investor
Expand All @@ -220,14 +204,12 @@ contract Crowdsale is Haltable {
}

// Check that we did not bust the cap
if(isBreakingCap(weiAmount, tokenAmount, weiRaised, tokensSold)) {
throw;
}
require(!isBreakingCap(weiAmount, tokenAmount, weiRaised, tokensSold));

assignTokens(receiver, tokenAmount);

// Pocket the money
if(!multisigWallet.send(weiAmount)) throw;
require(multisigWallet.send(weiAmount));
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use .transfer() instead of .send(), then require() would not be needed.

Copy link
Author

Choose a reason for hiding this comment

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

Changed to use .transfer() as requested.


// Tell us invest was success
Invested(receiver, weiAmount, tokenAmount, customerId);
Expand Down Expand Up @@ -270,26 +252,26 @@ contract Crowdsale is Haltable {
*/
function investWithSignedAddress(address addr, uint128 customerId, uint8 v, bytes32 r, bytes32 s) public payable {
bytes32 hash = sha256(addr);
if (ecrecover(hash, v, r, s) != signerAddress) throw;
if(customerId == 0) throw; // UUIDv4 sanity check
require(ecrecover(hash, v, r, s) == signerAddress);
require(customerId != 0); // UUIDv4 sanity check
investInternal(addr, customerId);
}

/**
* Track who is the customer making the payment so we can send thank you email.
*/
function investWithCustomerId(address addr, uint128 customerId) public payable {
if(requiredSignedAddress) throw; // Crowdsale allows only server-side signed participants
if(customerId == 0) throw; // UUIDv4 sanity check
require(!requiredSignedAddress); // Crowdsale allows only server-side signed participants
require(customerId != 0); // UUIDv4 sanity check
investInternal(addr, customerId);
}

/**
* Allow anonymous contributions to this crowdsale.
*/
function invest(address addr) public payable {
if(requireCustomerId) throw; // Crowdsale needs to track participants for thank you email
if(requiredSignedAddress) throw; // Crowdsale allows only server-side signed participants
require(!requireCustomerId); // Crowdsale needs to track participants for thank you email
require(!requiredSignedAddress); // Crowdsale allows only server-side signed participants
investInternal(addr, 0);
}

Expand Down Expand Up @@ -326,9 +308,7 @@ contract Crowdsale is Haltable {
function finalize() public inState(State.Success) onlyOwner stopInEmergency {

// Already finalized
if(finalized) {
throw;
}
require(!finalized);

// Finalizing is optional. We only call it if we are given a finalizing agent.
if(address(finalizeAgent) != 0) {
Expand All @@ -344,12 +324,10 @@ contract Crowdsale is Haltable {
* Design choice: no state restrictions on setting this, so that we can fix fat finger mistakes.
*/
function setFinalizeAgent(FinalizeAgent addr) onlyOwner {
finalizeAgent = addr;

// Don't allow setting bad agent
if(!finalizeAgent.isFinalizeAgent()) {
throw;
}
require(addr.isFinalizeAgent());
finalizeAgent = addr;
}

/**
Expand Down Expand Up @@ -395,9 +373,7 @@ contract Crowdsale is Haltable {
*/
function setEndsAt(uint time) onlyOwner {

if(now > time) {
throw; // Don't change past
}
require(now <= time); // Don't change past

endsAt = time;
EndsAtChanged(endsAt);
Expand All @@ -409,12 +385,10 @@ contract Crowdsale is Haltable {
* Design choice: no state restrictions on the set, so that we can fix fat finger mistakes.
*/
function setPricingStrategy(PricingStrategy _pricingStrategy) onlyOwner {
pricingStrategy = _pricingStrategy;

// Don't allow setting bad agent
if(!pricingStrategy.isPricingStrategy()) {
throw;
}
require(_pricingStrategy.isPricingStrategy());
pricingStrategy = _pricingStrategy;
}

/**
Expand All @@ -427,9 +401,7 @@ contract Crowdsale is Haltable {
function setMultisig(address addr) public onlyOwner {

// Change
if(investorCount > MAX_INVESTMENTS_BEFORE_MULTISIG_CHANGE) {
throw;
}
require(investorCount <= MAX_INVESTMENTS_BEFORE_MULTISIG_CHANGE);

multisigWallet = addr;
}
Expand All @@ -440,7 +412,7 @@ contract Crowdsale is Haltable {
* The team can transfer the funds back on the smart contract in the case the minimum goal was not reached..
*/
function loadRefund() public payable inState(State.Failure) {
if(msg.value == 0) throw;
require(msg.value != 0);
loadedRefund = loadedRefund.plus(msg.value);
}

Expand All @@ -452,11 +424,11 @@ contract Crowdsale is Haltable {
*/
function refund() public inState(State.Refunding) {
uint256 weiValue = investedAmountOf[msg.sender];
if (weiValue == 0) throw;
require(weiValue != 0);
investedAmountOf[msg.sender] = 0;
weiRefunded = weiRefunded.plus(weiValue);
Refund(msg.sender, weiValue);
if (!msg.sender.send(weiValue)) throw;
require(msg.sender.send(weiValue));
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use .transfer() instead of .send(), eliminating the need for require()

Copy link
Author

Choose a reason for hiding this comment

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

Changed to use .transfer() as requested.

}

/**
Expand Down Expand Up @@ -513,7 +485,7 @@ contract Crowdsale is Haltable {

/** Modified allowing execution only if the crowdsale is currently running. */
modifier inState(State state) {
if(getState() != state) throw;
require(getState() == state);
_;
}

Expand Down
6 changes: 3 additions & 3 deletions contracts/CrowdsaleToken.sol
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ contract CrowdsaleToken is ReleasableToken, MintableToken, UpgradeableToken {
function CrowdsaleToken(string _name, string _symbol, uint _initialSupply, uint _decimals, bool _mintable)
UpgradeableToken(msg.sender) {

// Cannot create a token without supply and no minting
require(_mintable || _initialSupply != 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should probably be moved to the same place the "throw" used to be? Minting should be optional, right?

Copy link
Author

Choose a reason for hiding this comment

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

The require() ensures that token is either mintable or is pre-allocated an initial supply so minting is optional as long as initial supply of tokens has been pre-allocated. Hence, I think start of function is an appropriate place for the require().


// Create any address, can be transferred
// to team multisig via changeOwner(),
// also remember to call setUpgradeMaster()
Expand All @@ -70,9 +73,6 @@ contract CrowdsaleToken is ReleasableToken, MintableToken, UpgradeableToken {
// No more new supply allowed after the token creation
if(!_mintable) {
mintingFinished = true;
if(totalSupply == 0) {
throw; // Cannot create a token without supply and no minting
}
}
}

Expand Down
4 changes: 1 addition & 3 deletions contracts/DefaultFinalizeAgent.sol
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ contract DefaultFinalizeAgent is FinalizeAgent {

/** Called once by crowdsale finalize() if the sale was success. */
function finalizeCrowdsale() public {
if(msg.sender != address(crowdsale)) {
throw;
}
require(msg.sender == address(crowdsale));
token.releaseTokenTransfer();
}

Expand Down
18 changes: 5 additions & 13 deletions contracts/EthTranchePricing.sol
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,7 @@ contract EthTranchePricing is PricingStrategy, Ownable {
/// @param _tranches uint[] tranches Pairs of (start amount, price)
function EthTranchePricing(uint[] _tranches) {
// Need to have tuples, length check
if(_tranches.length % 2 == 1 || _tranches.length >= MAX_TRANCHES*2) {
throw;
}
require((_tranches.length % 2 == 0) && (_tranches.length < MAX_TRANCHES*2));
Copy link
Contributor

Choose a reason for hiding this comment

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

Mikko, this is well translated from "if" to "require()", however, should be really do "_tranches.length < MAX_TRANCHES2" instead of "_tranches.length <= MAX_TRANCHES2"?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, I've changed condition to _tranches.length <= MAX_TRANCHES * 2.


trancheCount = _tranches.length / 2;

Expand All @@ -61,22 +59,16 @@ contract EthTranchePricing is PricingStrategy, Ownable {
tranches[i].price = _tranches[i*2+1];

// No invalid steps
if((highestAmount != 0) && (tranches[i].amount <= highestAmount)) {
throw;
}
require((highestAmount == 0) || (tranches[i].amount > highestAmount));

highestAmount = tranches[i].amount;
}

// We need to start from zero, otherwise we blow up our deployment
if(tranches[0].amount != 0) {
throw;
}
require(tranches[0].amount == 0);

// Last tranche price must be zero, terminating the crowdale
if(tranches[trancheCount-1].price != 0) {
throw;
}
require(tranches[trancheCount-1].price == 0);
}

/// @dev This is invoked once for every pre-ICO address, set pricePerToken
Expand Down Expand Up @@ -162,7 +154,7 @@ contract EthTranchePricing is PricingStrategy, Ownable {
}

function() payable {
throw; // No money on this contract
require(false); // No money on this contract
Copy link
Contributor

Choose a reason for hiding this comment

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

The "payable" modifier could be removed with require(), resulting an empty function for clarity.

Copy link
Author

Choose a reason for hiding this comment

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

Removed "payable" modifier and require() resulting in empty function as requested.

}

}
13 changes: 3 additions & 10 deletions contracts/ExtraFinalizeAgent.sol
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,12 @@ contract ExtraFinalizeAgent is FinalizeAgent {
uint public accountedTokenSales;

function ExtraFinalizeAgent(CrowdsaleToken _token, Crowdsale _crowdsale, uint _bonusBasePoints, address _teamMultisig, uint _accountedTokenSales) {
require(address(_crowdsale) != 0 && address(_teamMultisig) != 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe two separate require()s?

Copy link
Author

Choose a reason for hiding this comment

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

Changed to two separate require()s as requested.


token = _token;
crowdsale = _crowdsale;

if(address(crowdsale) == 0) {
throw;
}

teamMultisig = _teamMultisig;
if(address(teamMultisig) == 0) {
throw;
}

accountedTokenSales = _accountedTokenSales;
}
Expand All @@ -60,9 +55,7 @@ contract ExtraFinalizeAgent is FinalizeAgent {

/** Called once by crowdsale finalize() if the sale was success. */
function finalizeCrowdsale() {
if(msg.sender != address(crowdsale)) {
throw;
}
require(msg.sender == address(crowdsale));

// How many % of tokens the founders and others get
uint tokensSold = crowdsale.tokensSold().minus(accountedTokenSales);
Expand Down
Loading