Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
49b16cf
Basic idea
eternauta1337 Feb 4, 2018
fc7af3c
Fine tuning idea
eternauta1337 Feb 5, 2018
d54f799
Add comments / tidy up Crowdsale base class
eternauta1337 Feb 5, 2018
f48c150
fixed TimedCrowdsale constructor
fiiiu Feb 7, 2018
3f5680b
added simple crowdsale test
fiiiu Feb 7, 2018
a8a14df
added HODL directory under home to store unused contracts. ugly hack …
fiiiu Feb 7, 2018
469a999
Capped no longer inherits from Timed, added capReached() method (repl…
fiiiu Feb 8, 2018
90f0973
added SafeMath in TimedCrowdsale for safety, CHECK whether it is inhe…
fiiiu Feb 8, 2018
3ffe518
several fixes related to separating Capped from Timed. functions rena…
fiiiu Feb 8, 2018
a5aaf94
added TimedCrowdsaleImpl.sol, TimedCrowdsale tests, passed
fiiiu Feb 8, 2018
8a3cfb9
added Whitelisted implementation and test, passed.
fiiiu Feb 8, 2018
6343246
removed unnecessary super constructor call in WhitelistedCrowdsale, r…
fiiiu Feb 8, 2018
2c22337
renamed UserCappedCrowdsale to IndividuallyCappedCrowdsale, implement…
fiiiu Feb 8, 2018
8c8fed1
homogeneized use of using SafeMath for uint256 across validation crow…
fiiiu Feb 8, 2018
962b5bc
adding questions.md where I track questions, bugs and progress
fiiiu Feb 8, 2018
bbb2dfa
modified VariablePriceCrowdsale, added Impl.
fiiiu Feb 9, 2018
3fdb6da
finished VariablePrice, fixed sign, added test, passing.
fiiiu Feb 9, 2018
53ec3cc
changed VariablePrice to IncreasingPrice, added corresponding require()
fiiiu Feb 14, 2018
2be5806
MintedCrowdsale done, mock implemented, test passing
fiiiu Feb 14, 2018
b814a05
PremintedCrowdsale done, mocks, tests passing
fiiiu Feb 14, 2018
dd05925
checked FinalizableCrowdsale
fiiiu Feb 14, 2018
120d277
PostDeliveryCrowdsale done, mock, tests passing.
fiiiu Feb 14, 2018
e677e33
RefundableCrowdsale done. Detached Vault. modified mock and test, pas…
fiiiu Feb 14, 2018
5873f8e
renamed crowdsale-refactor to crowdsale in contracts and test
fiiiu Feb 14, 2018
2fe239c
deleted HODL old contracts
fiiiu Feb 15, 2018
bf5e9dd
polished variable names in tests
fiiiu Feb 15, 2018
cd0ba80
fixed typos and removed comments in tests
fiiiu Feb 15, 2018
0487d25
Renamed 'crowdsale-refactor' to 'crowdsale' in all imports
eternauta1337 Feb 15, 2018
c4a2f9c
Fix minor param naming issues in Crowdsale functions and added docume…
eternauta1337 Feb 15, 2018
3ef55bc
Added documentation to Crowdsale extensions
eternauta1337 Feb 15, 2018
c1a41ae
removed residual comments and progress tracking files
fiiiu Feb 15, 2018
b455000
added docs for validation crowdsales
fiiiu Feb 15, 2018
a841691
Made user promises in PostDeliveryCrowdsale public so that users can …
eternauta1337 Feb 15, 2018
3d4d41a
added docs for distribution crowdsales
fiiiu Feb 15, 2018
8b6b342
renamed PremintedCrowdsale to AllowanceCrowdsale
fiiiu Feb 16, 2018
1a2a5ec
added allowance check function and corresponding test. fixed filename…
fiiiu Feb 16, 2018
1680a18
spilt Crowdsale _postValidatePurchase in _postValidatePurchase and _u…
fiiiu Feb 16, 2018
344bec6
polished tests for linter, salve Travis
fiiiu Feb 16, 2018
7d4035a
polished IncreasingPriceCrowdsale.sol for linter.
fiiiu Feb 16, 2018
bcd0464
renamed and polished for linter WhitelistedCrowdsale test.
fiiiu Feb 16, 2018
51c0954
fixed indentation in IncreasingPriceCrowdsaleImpl.sol for linter
fiiiu Feb 16, 2018
cd15b41
fixed ignoring token.mint return value in MintedCrowdsale.sol
fiiiu Feb 17, 2018
e716a22
expanded docs throughout, fixed minor issues
fiiiu Feb 19, 2018
7134f0d
extended test coverage for IndividuallyCappedCrowdsale
fiiiu Feb 19, 2018
f56116e
Extended WhitelistedCrwodsale test coverage
fiiiu Feb 19, 2018
2a6dd9f
roll back decoupling of RefundVault in RefundableCrowdsale
fiiiu Feb 19, 2018
8e8e8cc
moved cap exceedance checks in Capped and IndividuallyCapped crowdsal…
fiiiu Feb 19, 2018
fa055a6
revert name change, IndividuallyCapped to UserCapped
fiiiu Feb 19, 2018
56e83b9
extended docs.
fiiiu Feb 19, 2018
1bf30f9
added crowd whitelisting with tests
fiiiu Feb 19, 2018
a70e3ed
added group capping, plus tests
fiiiu Feb 19, 2018
5f4fc83
added modifiers in TimedCrowdsale and WhitelistedCrowdsale
fiiiu Feb 19, 2018
47ce79f
polished tests for linter
fiiiu Feb 19, 2018
5dab495
moved check of whitelisted to modifier, mainly for testing coverage
fiiiu Feb 19, 2018
dd338f1
fixed minor ordering/polishingafter review
fiiiu Feb 19, 2018
8376baa
modified TimedCrowdsale modifier/constructor ordering
fiiiu Feb 19, 2018
8ad6a10
unchanged truffle-config.js
fiiiu Feb 20, 2018
107fdc4
changed indentation of visibility modifier in mocks
fiiiu Feb 20, 2018
530d85d
changed naming of modifier and function to use Open/Closed for TimedC…
fiiiu Feb 20, 2018
9fe61b8
changed ordering of constructor calls in SampleCrowdsale
fiiiu Feb 20, 2018
de8e88f
changed startTime and endTime to openingTime and closingTime throughout
fiiiu Feb 20, 2018
6c38f46
fixed exceeding line lenght for linter
fiiiu Feb 20, 2018
33839c1
Merge branch 'master' into feature/crowdsale-refactor
fiiiu Feb 20, 2018
df03099
renamed _emitTokens to _deliverTokens
fiiiu Feb 20, 2018
c6def0e
renamed addCrowdToWhitelist to addManyToWhitelist
fiiiu Feb 20, 2018
c177f01
renamed UserCappedCrowdsale to IndividuallyCappedCrowdsale
fiiiu Feb 20, 2018
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
Prev Previous commit
Next Next commit
expanded docs throughout, fixed minor issues
  • Loading branch information
fiiiu committed Feb 19, 2018
commit e716a22f5f535728146bdd3229cecf0c97a3b37e
25 changes: 8 additions & 17 deletions contracts/crowdsale/Crowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,15 @@ import "../math/SafeMath.sol";
* @title Crowdsale
* @dev Crowdsale is a base contract for managing a token crowdsale,
* allowing investors to purchase tokens with ether. This contract implements
Copy link
Contributor

@shrugs shrugs Feb 20, 2018

Choose a reason for hiding this comment

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

I've traditionally seen multiline @dev comments either prefixed with @dev all the way down

/**
 * @dev like
 * @dev this
 */

The docs don't specify what happens for multiline, though, so I'm not sure what the standard is. Off the top of my head, though, I think OZ primarily uses the @dev prefix per-line, but I could be wrong and I'm not particularly partial

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @shrugs. Throughout the OZ codebase, a single @dev at the beginning of a multi-sentence description seems to be the norm (examples: Bounty.sol, DayLimit.sol). Also, the automatically-generated docs parse NatSpec following this convention.

The NatSpec specification is not at all clear about how to handle this case, but there are two examples within the document which use a muti-line @notice tag (link).

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the heads up!

* such functionality in it's most fundamental form and can be extended to provide additional
* such functionality in its most fundamental form and can be extended to provide additional
* functionality and/or custom behavior.
* The external interface represents the basic interface for purchasing tokens, and conform
* the base architecture for crowdsales. They are *not* intended to be modified / overriden.
* The internal interface conforms the extensible and modifiable surface of crowdsales. Override
* the methods to add functionality. Consider using 'super' where appropiate to concatenate
* behavior.
*/

contract Crowdsale {
using SafeMath for uint256;

Expand Down Expand Up @@ -53,13 +59,6 @@ contract Crowdsale {
// Crowdsale external interface
// -----------------------------------------

/**
* These functions represent the basic
* interface for purchasing tokens, conform
* the base architecture for crowdsales,
* and are not intended to be modified / overriden.
*/

/**
* @dev fallback function ***DO NOT OVERRIDE***
*/
Expand Down Expand Up @@ -92,17 +91,9 @@ contract Crowdsale {
}

// -----------------------------------------
// Internal (extensible) implementations
// Internal interface (extensible)
// -----------------------------------------

/**
* These functions conform the extensible
* and modifiable surface of crowdsales.
* Override the methods to add additional functionality.
* Consider using 'super' where appropiate to concatenate
* behavior.
*/

/**
* @dev Validation of an incoming purchase. Use require statemens to revert state when conditions are not met. Use super to concatenate validations.
* @param _beneficiary Address performing the token purchase
Expand Down
9 changes: 7 additions & 2 deletions contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,24 @@ import "../../math/SafeMath.sol";

/**
* @title PostDeliveryCrowdsale
* @dev Crowdsale that locks funds from withdrawal until it ends
* @dev Crowdsale that locks tokens from withdrawal until it ends.
*/
contract PostDeliveryCrowdsale is TimedCrowdsale {
using SafeMath for uint256;

mapping(address => uint256) public promises;
Copy link
Contributor

Choose a reason for hiding this comment

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

2 things here:

  1. IMO promises does not fit properly with the name of this crowdsale, what about tokenPromises or balances directly?
  2. shouldn't this be internal or private? I think we are trying to promote a refactor where visibility and overriding matters

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to balances, but I think public is okay to allow users to check their balance.


/**
* @dev Overrides parent by storing balances instead of issuing tokens right away.
* @param _beneficiary Token purchaser
* @param _tokenAmount Amount of tokens purchased
*/
function _processPurchase(address _beneficiary, uint256 _tokenAmount) internal {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing doc

promises[_beneficiary] = promises[_beneficiary].add(_tokenAmount);
}

/**
* @dev Withdraw tokens only after crowdsale ends
* @dev Withdraw tokens only after crowdsale ends.
*/
function withdrawTokens() public {
require(hasExpired());
Expand Down
2 changes: 1 addition & 1 deletion contracts/crowdsale/distribution/RefundableCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
}

/**
* @dev Overrides Crowdsale fund forwarding. In addition to sending the funds, we call the RefundVault deposit function
* @dev Overrides Crowdsale fund forwarding, sending funds to vault.
*/
function _forwardFunds() internal {
vault.deposit.value(msg.value)(msg.sender);
Expand Down
10 changes: 10 additions & 0 deletions contracts/crowdsale/emission/AllowanceCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ import "../Crowdsale.sol";
import "../../token/ERC20/ERC20.sol";
import "../../math/SafeMath.sol";


/**
* @title AllowanceCrowdsale
* @dev Extension of Crowdsale where tokens are held by a wallet, which approves an allowance to the crowdsale.
*/
contract AllowanceCrowdsale is Crowdsale {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try if we can find a better name for this one

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought it was very clear, even more considering the alternatives @fiiiu and @ajsantander discussed.

using SafeMath for uint256;

Expand All @@ -24,6 +29,11 @@ contract AllowanceCrowdsale is Crowdsale {
return token.allowance(tokenWallet, this);
}

/**
* @dev Overrides parent behavior by transferring tokens from wallet.
* @param _beneficiary Token purchaser
* @param _tokenAmount Amount of tokens purchased
*/
function _emitTokens(address _beneficiary, uint256 _tokenAmount) internal {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super happy with this name

Copy link
Contributor

Choose a reason for hiding this comment

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

discussed offline with @fiiiu and @facuspagnuolo and decided to go with _deliverTokens πŸ•

token.transferFrom(tokenWallet, _beneficiary, _tokenAmount);
}
Expand Down
11 changes: 11 additions & 0 deletions contracts/crowdsale/emission/MintedCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,19 @@ pragma solidity ^0.4.18;
import "../Crowdsale.sol";
import "../../token/ERC20/MintableToken.sol";


/**
* @title MintedCrowdsale
* @dev Extension of Crowdsale contract whose tokens are minted in each purchase.
* Token ownership should be transferred to MintedCrowdsale for minting.
*/
contract MintedCrowdsale is Crowdsale {

/**
* @dev Overrides emission by minting tokens upon purchase.
* @param _beneficiary Token purchaser
* @param _tokenAmount Number of tokens to be minted
*/
function _emitTokens(address _beneficiary, uint256 _tokenAmount) internal {
require(MintableToken(token).mint(_beneficiary, _tokenAmount));
}
Expand Down
20 changes: 16 additions & 4 deletions contracts/crowdsale/price/IncreasingPriceCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ pragma solidity ^0.4.18;
import "../validation/TimedCrowdsale.sol";
import "../../math/SafeMath.sol";


/**
* @title IncreasingPriceCrowdsale
* @dev Extension of Crowdsale contract that increases the price of tokens linearly in time.
* Note that what should be provided to the constructor is the initial and final _rates_, that is,
* the amount of tokens per ETH contributed. Thus, the initial rate must be greater than the final rate.
*/
contract IncreasingPriceCrowdsale is TimedCrowdsale {
using SafeMath for uint256;

Expand All @@ -22,15 +27,22 @@ contract IncreasingPriceCrowdsale is TimedCrowdsale {
}

/**
* @dev Returns the rate of tokens per ETH at the present time.
* Note that, as price _increases_ with time, the rate _decreases_.
* @return The number of tokens a buyer gets per wei at a given time
*/
function getCurrentRate() public view returns (uint256) {
uint256 elapsedTime = now - startTime;
uint256 timeRange = endTime - startTime;
uint256 rateRange = initialRate - finalRate;
uint256 elapsedTime = now.sub(startTime);
uint256 timeRange = endTime.sub(startTime);
uint256 rateRange = initialRate.sub(finalRate);
return initialRate.sub(elapsedTime.mul(rateRange).div(timeRange));
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using
rateRange = finalRate - initialRate for consistency and easier readability. (.sub is replaced by .add)

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 would make rateRangeΒ negative, we require(_initialRate >= _finalRate); in order to work with unsigned ints (hence IncreasingPrice). We could add a DecreasingPriceCrowdsale, but I'm skeptical it would be of much use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, slope of the rate curve is negative. I was thinking about the slope of the price curve.
My bad again then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm with @ajsantander in here, this is confusing. We should leave a clarification to avoid silly millionaire mistakes.

}

/**
* @dev Overrides parent method taking into account variable rate.
* @param _weiAmount The value in wei to be converted into tokens
* @return The number of tokens _weiAmount wei will buy at present time
*/
function _getTokenAmount(uint256 _weiAmount) internal view returns (uint256) {
uint256 currentRate = getCurrentRate();
return currentRate.mul(_weiAmount);
Expand Down
9 changes: 8 additions & 1 deletion contracts/crowdsale/validation/CappedCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ pragma solidity ^0.4.18;
import "../../math/SafeMath.sol";
import "../Crowdsale.sol";


/**
* @dev Crowdsale with a limit for total contributions
* @title CappedCrowdsale
* @dev Crowdsale with a limit for total contributions.
*/
contract CappedCrowdsale is Crowdsale {
using SafeMath for uint256;
Expand All @@ -26,6 +28,11 @@ contract CappedCrowdsale is Crowdsale {
return weiRaised >= cap;
}

/**
* @dev Extend parent behavior requiring to not exceed the cap.
* @param _beneficiary Token purchaser
* @param _weiAmount Amount of wei contributed
*/
function _postValidatePurchase(address _beneficiary, uint256 _weiAmount) internal {
super._postValidatePurchase(_beneficiary, _weiAmount);
require(weiRaised <= cap);
Expand Down
14 changes: 13 additions & 1 deletion contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import "../../math/SafeMath.sol";
import "../Crowdsale.sol";
import "../../ownership/Ownable.sol";


/**
* @dev Crowdsale with per-user caps
* @title IndividuallyCappedCrowdsale
* @dev Crowdsale with per-user caps.
*/
contract IndividuallyCappedCrowdsale is Crowdsale, Ownable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't be more appropriate to say UserCappedCrowdsale? see the names of the functions below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, this was @ajsantander's original proposal, I changed it because this sounded to me like "capped by the user", but we can go back if you all favor this :)

using SafeMath for uint256;
Expand Down Expand Up @@ -37,11 +39,21 @@ contract IndividuallyCappedCrowdsale is Crowdsale, Ownable {
return contributions[_beneficiary];
}

/**
* @dev Extend parent behavior requiring to not exceed the individual cap.
* @param _beneficiary Token purchaser
* @param _weiAmount Amount of wei contributed
*/
function _postValidatePurchase(address _beneficiary, uint256 _weiAmount) internal {
super._postValidatePurchase(_beneficiary, _weiAmount);
require(contributions[_beneficiary] <= caps[_beneficiary]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I would try to move this to _preValidatePurchase to save gas.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why moving this precondition above is going to save some gas? I mean, we will evaluate it anyway.. can you explain a little bit more?

Copy link
Contributor

@martriay martriay Feb 19, 2018

Choose a reason for hiding this comment

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

Because you're performing the check at the start of buyTokens, if you do it at the end you would have called _getTokenAmount, _processPurchase, _updatePurchasingState and _fowardFunds, and any of those can make subsequent calls to another functions or even contracts. That's gassy 😲.

}

/**
* @dev Extend parent behavior to update user contributions
* @param _beneficiary Token purchaser
* @param _weiAmount Amount of wei contributed
*/
function _updatePurchasingState(address _beneficiary, uint256 _weiAmount) internal {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this? πŸ˜‹

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 is how the new crowdsale model flow works, we should add more documentation comments though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed adding this with @ajsantander, please see above (link might work as intended as the comment is folded). The idea is to decouple validation from state tracking, taking into account future extensions.

Usage is now described in parent Crowdsale, should we repeat it here?

super._updatePurchasingState(_beneficiary, _weiAmount);
contributions[_beneficiary] = contributions[_beneficiary].add(_weiAmount);
Expand Down
11 changes: 9 additions & 2 deletions contracts/crowdsale/validation/TimedCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ pragma solidity ^0.4.18;
import "../../math/SafeMath.sol";
import "../Crowdsale.sol";


/**
* @dev Crowdsale accepting contributions only within a time frame
* @title TimedCrowdsale
* @dev Crowdsale accepting contributions only within a time frame.
*/
contract TimedCrowdsale is Crowdsale {
using SafeMath for uint256;
Expand All @@ -30,7 +32,12 @@ contract TimedCrowdsale is Crowdsale {
function hasExpired() public view returns (bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Expired instead of Ended (_endTime) for any particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hasEnded was the prior method that encompassed ending either by reaching endTime or by reaching the cap in a CappedCrowdsale. Now the purchase validation is done in separate places so we need to split this (hasExpired and capReached). While we could call this one hasEnded, a crowdsale could also end "prematurely" by reaching its cap, thus, be ended but not expired..

Copy link
Contributor

Choose a reason for hiding this comment

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

any reason we're not using the existing naming of hasEnded? I don't mind either way, but it's good to know why we changed the api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hasEnded was the prior method that encompassed ending either by reaching endTime or by reaching the cap in a CappedCrowdsale. Now the purchase validation is done in separate places so we need to split this (hasExpired and capReached). While we could call this one hasEnded, a crowdsale could also end "prematurely" by reaching its cap, thus, be ended but not expired..

return now > endTime;
}


/**
* @dev Extend parent behavior requiring to be within contributing period
* @param _beneficiary Token purchaser
* @param _weiAmount Amount of wei contributed
*/
function _preValidatePurchase(address _beneficiary, uint256 _weiAmount) internal {
super._preValidatePurchase(_beneficiary, _weiAmount);
require(now >= startTime && now <= endTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using
require(!hasExpired());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean substitute
require(now >= startTime && now <= endTime);
for:
require(now >= startTime);
require(!hasExpired());
?
We still need to check we are after startTime, so I'm not sure what the advantage of this would be...

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. My bad =P

Copy link
Contributor

Choose a reason for hiding this comment

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

I would abstract that require into another inTimeRange modifier, to make the code easier to read and because it may come handy when working with a TimedCrowdsale. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe withinBuyingPeriod

Expand Down
9 changes: 8 additions & 1 deletion contracts/crowdsale/validation/WhitelistedCrowdsale.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ pragma solidity ^ 0.4.18;
import "../Crowdsale.sol";
import "../../ownership/Ownable.sol";


/**
* @dev Crowdsale in which only whitelisted users can contribute
* @title WhitelistedCrowdsale
* @dev Crowdsale in which only whitelisted users can contribute.
*/
contract WhitelistedCrowdsale is Crowdsale, Ownable {

Expand Down Expand Up @@ -32,6 +34,11 @@ contract WhitelistedCrowdsale is Crowdsale, Ownable {
return whitelist[_beneficiary];
}

/**
* @dev Extend parent behavior requiring beneficiary to be in whitelist.
* @param _beneficiary Token beneficiary
* @param _weiAmount Amount of wei contributed
*/
function _preValidatePurchase(address _beneficiary, uint256 _weiAmount) internal {
super._preValidatePurchase(_beneficiary, _weiAmount);
require(isWhitelisted(_beneficiary));
Expand Down