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

Feature/crowdsale refactor #744

Merged
merged 66 commits into from
Feb 20, 2018
Merged
Show file tree
Hide file tree
Changes from 56 commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
49b16cf
Basic idea
theethernaut Feb 4, 2018
fc7af3c
Fine tuning idea
theethernaut Feb 5, 2018
d54f799
Add comments / tidy up Crowdsale base class
theethernaut 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
theethernaut Feb 15, 2018
c4a2f9c
Fix minor param naming issues in Crowdsale functions and added docume…
theethernaut Feb 15, 2018
3ef55bc
Added documentation to Crowdsale extensions
theethernaut 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 …
theethernaut 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 0 additions & 35 deletions contracts/crowdsale/CappedCrowdsale.sol

This file was deleted.

147 changes: 99 additions & 48 deletions contracts/crowdsale/Crowdsale.sol
Original file line number Diff line number Diff line change
@@ -1,107 +1,158 @@
pragma solidity ^0.4.18;

import "../token/ERC20/MintableToken.sol";
import "../token/ERC20/ERC20.sol";
import "../math/SafeMath.sol";


/**
* @title Crowdsale
* @dev Crowdsale is a base contract for managing a token crowdsale.
* Crowdsales have a start and end timestamps, where investors can make
* token purchases and the crowdsale will assign them tokens based
* on a token per ETH rate. Funds collected are forwarded to a wallet
* as they arrive. The contract requires a MintableToken that will be
* minted as contributions arrive, note that the crowdsale contract
* must be owner of the token in order to be able to mint it.
* @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 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;

// The token being sold
MintableToken public token;

// start and end timestamps where investments are allowed (both inclusive)
uint256 public startTime;
uint256 public endTime;
ERC20 public token;

// address where funds are collected
// Address where funds are collected
address public wallet;

// how many token units a buyer gets per wei
// How many token units a buyer gets per wei
uint256 public rate;

// amount of raised money in wei
// Amount of wei raised
uint256 public weiRaised;

/**
* event for token purchase logging
* Event for token purchase logging
* @param purchaser who paid for the tokens
* @param beneficiary who got the tokens
* @param value weis paid for purchase
* @param amount amount of tokens purchased
*/
event TokenPurchase(address indexed purchaser, address indexed beneficiary, uint256 value, uint256 amount);


function Crowdsale(uint256 _startTime, uint256 _endTime, uint256 _rate, address _wallet, MintableToken _token) public {
require(_startTime >= now);
require(_endTime >= _startTime);
/**
* @param _rate Number of token units a buyer gets per wei
* @param _wallet Address where collected funds will be forwarded to
* @param _token Address of the token being sold
*/
function Crowdsale(uint256 _rate, address _wallet, ERC20 _token) public {
require(_rate > 0);
require(_wallet != address(0));
require(_token != address(0));

startTime = _startTime;
endTime = _endTime;
rate = _rate;
wallet = _wallet;
token = _token;
}

// fallback function can be used to buy tokens
// -----------------------------------------
// Crowdsale external interface
// -----------------------------------------

/**
* @dev fallback function ***DO NOT OVERRIDE***
*/
function () external payable {
buyTokens(msg.sender);
}

// low level token purchase function
function buyTokens(address beneficiary) public payable {
require(beneficiary != address(0));
require(validPurchase());
/**
* @dev low level token purchase ***DO NOT OVERRIDE***
* @param _beneficiary Address performing the token purchase
*/
function buyTokens(address _beneficiary) public payable {

uint256 weiAmount = msg.value;
_preValidatePurchase(_beneficiary, weiAmount);

// calculate token amount to be created
uint256 tokens = getTokenAmount(weiAmount);
uint256 tokens = _getTokenAmount(weiAmount);

// update state
weiRaised = weiRaised.add(weiAmount);

token.mint(beneficiary, tokens);
TokenPurchase(msg.sender, beneficiary, weiAmount, tokens);
_processPurchase(_beneficiary, tokens);
TokenPurchase(msg.sender, _beneficiary, weiAmount, tokens);

_updatePurchasingState(_beneficiary, weiAmount);

_forwardFunds();
_postValidatePurchase(_beneficiary, weiAmount);
}

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

/**
* @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
* @param _weiAmount Value in wei involved in the purchase
*/
function _preValidatePurchase(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 about using just _value? I know it is less descriptive than _weiAmount, but it has already been adopted by the community.

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 down to do that, now that we're making sweeping changes. Obviously updated across the file(s) where appropriate

Copy link
Contributor

Choose a reason for hiding this comment

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

NACK

require(_beneficiary != address(0));
require(_weiAmount != 0);
}

forwardFunds();
/**
* @dev Validation of an executed purchase. Observe state and use revert statements to undo rollback when valid conditions are not met.
* @param _beneficiary Address performing the token purchase
* @param _weiAmount Value in wei involved in the purchase
*/
function _postValidatePurchase(address _beneficiary, uint256 _weiAmount) internal {
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.

what are some example use-cases for this, especially since we don't use it in these contracts? If the answer is just "why not allow it" that's fine too

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, if you are performing a purchase that involves a call to another contract, you can check that the call worked the way you were expecting here

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha, for invariants and such. Nice.

// optional override
}

// @return true if crowdsale event has ended
function hasEnded() public view returns (bool) {
return now > endTime;
/**
* @dev Source of tokens. Override this method to modify the way in which the crowdsale ultimately gets and sends its tokens.
* @param _beneficiary Address performing the token purchase
* @param _tokenAmount Number of tokens to be emitted
*/
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.

Given that the function is called _emitTokens, I think it is a little bit redundant using the name _tokenAmount then, what do you think about renaming it _amount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

..but it might be confusing in _processPurchase, where it might not be clear if it's tokens or wei.. I'll keep tokenAmount and weiAmount for now, waiting for further input.

token.transfer(_beneficiary, _tokenAmount);
}

// Override this method to have a way to add business logic to your crowdsale when buying
function getTokenAmount(uint256 weiAmount) internal view returns(uint256) {
return weiAmount.mul(rate);
/**
* @dev Executed when a purchase has been validated and is ready to be executed. Not necessarily emits/sends tokens.
* @param _beneficiary Address receiving the tokens
* @param _tokenAmount Number of tokens to be purchased
*/
function _processPurchase(address _beneficiary, uint256 _tokenAmount) internal {
_emitTokens(_beneficiary, _tokenAmount);
}

// send ether to the fund collection wallet
// override to create custom fund forwarding mechanisms
function forwardFunds() internal {
wallet.transfer(msg.value);
/**
* @dev Override for extensions that require an internal state to check for validity (current user contributions, etc.)
* @param _beneficiary Address receiving the tokens
* @param _weiAmount Value in wei involved in the purchase
*/
function _updatePurchasingState(address _beneficiary, uint256 _weiAmount) internal {
// optional override
}

// @return true if the transaction can buy tokens
function validPurchase() internal view returns (bool) {
bool withinPeriod = now >= startTime && now <= endTime;
bool nonZeroPurchase = msg.value != 0;
return withinPeriod && nonZeroPurchase;
/**
* @dev Override to extend the way in which ether is converted to tokens.
* @param _weiAmount Value in wei to be converted into tokens
* @return Number of tokens that can be purchased with the specified _weiAmount
*/
function _getTokenAmount(uint256 _weiAmount) internal view returns (uint256) {
return _weiAmount.mul(rate);
}

/**
* @dev Determines how ETH is stored/forwarded on purchases.
*/
function _forwardFunds() internal {
wallet.transfer(msg.value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I really loved how this look like, great job guys πŸ˜„

}
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
pragma solidity ^0.4.18;

import "../math/SafeMath.sol";
import "../ownership/Ownable.sol";
import "./Crowdsale.sol";

import "../../math/SafeMath.sol";
import "../../ownership/Ownable.sol";
import "../validation/TimedCrowdsale.sol";

/**
* @title FinalizableCrowdsale
* @dev Extension of Crowdsale where an owner can do extra work
* after finishing.
*/
contract FinalizableCrowdsale is Crowdsale, Ownable {
contract FinalizableCrowdsale is TimedCrowdsale, Ownable {
using SafeMath for uint256;

bool public isFinalized = false;
Expand All @@ -23,7 +22,7 @@ contract FinalizableCrowdsale is Crowdsale, Ownable {
*/
function finalize() onlyOwner public {
require(!isFinalized);
require(hasEnded());
require(hasExpired());

finalization();
Finalized();
Expand Down
35 changes: 35 additions & 0 deletions contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
pragma solidity ^0.4.18;

import "../validation/TimedCrowdsale.sol";
import "../../token/ERC20/ERC20.sol";
import "../../math/SafeMath.sol";

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

mapping(address => uint256) public balances;

/**
* @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

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

/**
* @dev Withdraw tokens only after crowdsale ends.
*/
function withdrawTokens() public {
require(hasExpired());
uint256 amount = balances[msg.sender];
require(amount > 0);
balances[msg.sender] = 0;
_emitTokens(msg.sender, amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

From the description:

locks funds from withdrawal until it ends

but I guess it is locking tokens not funds right?

}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
pragma solidity ^0.4.18;


import "../math/SafeMath.sol";
import "../../math/SafeMath.sol";
import "./FinalizableCrowdsale.sol";
import "./RefundVault.sol";
import "./utils/RefundVault.sol";


/**
Expand All @@ -21,25 +21,37 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
// refund vault used to hold funds while crowdsale is running
RefundVault public vault;

/**
* @dev Constructor, creates RefundVault.
* @param _goal Funding goal
*/
function RefundableCrowdsale(uint256 _goal) public {
require(_goal > 0);
vault = new RefundVault(wallet);
goal = _goal;
}

// if crowdsale is unsuccessful, investors can claim refunds here
/**
* @dev Investors can claim refunds here if crowdsale is unsuccessful
*/
function claimRefund() public {
require(isFinalized);
require(!goalReached());

vault.refund(msg.sender);
}

/**
* @dev Checks whether funding goal was reached.
* @return Whether funding goal was reached
*/
function goalReached() public view returns (bool) {
return weiRaised >= goal;
}

// vault finalization task, called when owner calls finalize()
/**
* @dev vault finalization task, called when owner calls finalize()
*/
function finalization() internal {
if (goalReached()) {
vault.close();
Expand All @@ -50,10 +62,10 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
super.finalization();
}

// We're overriding the fund forwarding from Crowdsale.
// In addition to sending the funds, we want to call
// the RefundVault deposit function
function forwardFunds() internal {
/**
* @dev Overrides Crowdsale fund forwarding, sending funds to vault.
*/
function _forwardFunds() internal {
vault.deposit.value(msg.value)(msg.sender);
}

Expand Down