From 128c4c0dda8d50bfa5ade266f29a0ed00300b280 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 05:41:34 +0000 Subject: [PATCH 01/30] Improve encapsulation on ERC165 --- contracts/introspection/SupportsInterfaceWithLookup.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/introspection/SupportsInterfaceWithLookup.sol b/contracts/introspection/SupportsInterfaceWithLookup.sol index c2b009aea0f..6fca22ac8ca 100644 --- a/contracts/introspection/SupportsInterfaceWithLookup.sol +++ b/contracts/introspection/SupportsInterfaceWithLookup.sol @@ -10,7 +10,7 @@ import "./IERC165.sol"; */ contract SupportsInterfaceWithLookup is IERC165 { - bytes4 public constant InterfaceId_ERC165 = 0x01ffc9a7; + bytes4 private constant InterfaceId_ERC165 = 0x01ffc9a7; /** * 0x01ffc9a7 === * bytes4(keccak256('supportsInterface(bytes4)')) From 0d8a5e4ab76bc1e7b16e0e026caacb2a6bc43b0c Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 05:44:20 +0000 Subject: [PATCH 02/30] Improve encapsulation on ERC20 --- contracts/token/ERC20/ERC20Capped.sol | 13 ++- contracts/token/ERC20/ERC20Detailed.sol | 33 ++++++-- contracts/token/ERC20/ERC20Mintable.sol | 13 ++- contracts/token/ERC20/TokenTimelock.sol | 39 ++++++--- contracts/token/ERC20/TokenVesting.sol | 93 +++++++++++++++++----- test/token/ERC20/DetailedERC20.test.js | 6 +- test/token/ERC20/ERC20Capped.behavior.js | 2 +- test/token/ERC20/ERC20Mintable.behavior.js | 6 +- 8 files changed, 155 insertions(+), 50 deletions(-) diff --git a/contracts/token/ERC20/ERC20Capped.sol b/contracts/token/ERC20/ERC20Capped.sol index 3af03f167cd..13f5dabdcbf 100644 --- a/contracts/token/ERC20/ERC20Capped.sol +++ b/contracts/token/ERC20/ERC20Capped.sol @@ -9,11 +9,18 @@ import "./ERC20Mintable.sol"; */ contract ERC20Capped is ERC20Mintable { - uint256 public cap; + uint256 private cap_; constructor(uint256 _cap) public { require(_cap > 0); - cap = _cap; + cap_ = _cap; + } + + /** + * @return the cap for the token minting. + */ + function getCap() public view returns(uint256) { + return cap_; } /** @@ -29,7 +36,7 @@ contract ERC20Capped is ERC20Mintable { public returns (bool) { - require(totalSupply().add(_amount) <= cap); + require(totalSupply().add(_amount) <= cap_); return super.mint(_to, _amount); } diff --git a/contracts/token/ERC20/ERC20Detailed.sol b/contracts/token/ERC20/ERC20Detailed.sol index ad6f1dd059a..aefa3947184 100644 --- a/contracts/token/ERC20/ERC20Detailed.sol +++ b/contracts/token/ERC20/ERC20Detailed.sol @@ -10,13 +10,34 @@ import "./IERC20.sol"; * just as on Ethereum all the operations are done in wei. */ contract ERC20Detailed is IERC20 { - string public name; - string public symbol; - uint8 public decimals; + string private name_; + string private symbol_; + uint8 private decimals_; constructor(string _name, string _symbol, uint8 _decimals) public { - name = _name; - symbol = _symbol; - decimals = _decimals; + name_ = _name; + symbol_ = _symbol; + decimals_ = _decimals; + } + + /** + * @return the name of the token. + */ + function getName() public view returns(string) { + return name_; + } + + /** + * @return the symbol of the token. + */ + function getSymbol() public view returns(string) { + return symbol_; + } + + /** + * @return the number of decimals of the token. + */ + function getDecimals() public view returns(uint8) { + return decimals_; } } diff --git a/contracts/token/ERC20/ERC20Mintable.sol b/contracts/token/ERC20/ERC20Mintable.sol index 0ba200cdfac..6c329317a58 100644 --- a/contracts/token/ERC20/ERC20Mintable.sol +++ b/contracts/token/ERC20/ERC20Mintable.sol @@ -13,11 +13,11 @@ contract ERC20Mintable is ERC20, Ownable { event Mint(address indexed to, uint256 amount); event MintFinished(); - bool public mintingFinished = false; + bool private mintingFinished_ = false; modifier canMint() { - require(!mintingFinished); + require(!mintingFinished_); _; } @@ -26,6 +26,13 @@ contract ERC20Mintable is ERC20, Ownable { _; } + /** + * @return true if the minting is finished. + */ + function isMintingFinished() public view returns(bool) { + return mintingFinished_; + } + /** * @dev Function to mint tokens * @param _to The address that will receive the minted tokens. @@ -51,7 +58,7 @@ contract ERC20Mintable is ERC20, Ownable { * @return True if the operation was successful. */ function finishMinting() public onlyOwner canMint returns (bool) { - mintingFinished = true; + mintingFinished_ = true; emit MintFinished(); return true; } diff --git a/contracts/token/ERC20/TokenTimelock.sol b/contracts/token/ERC20/TokenTimelock.sol index a24642f6de5..0ed6341a71c 100644 --- a/contracts/token/ERC20/TokenTimelock.sol +++ b/contracts/token/ERC20/TokenTimelock.sol @@ -12,13 +12,13 @@ contract TokenTimelock { using SafeERC20 for IERC20; // ERC20 basic token contract being held - IERC20 public token; + IERC20 private token_; // beneficiary of tokens after they are released - address public beneficiary; + address private beneficiary_; // timestamp when token release is enabled - uint256 public releaseTime; + uint256 private releaseTime_; constructor( IERC20 _token, @@ -29,9 +29,30 @@ contract TokenTimelock { { // solium-disable-next-line security/no-block-members require(_releaseTime > block.timestamp); - token = _token; - beneficiary = _beneficiary; - releaseTime = _releaseTime; + token_ = _token; + beneficiary_ = _beneficiary; + releaseTime_ = _releaseTime; + } + + /** + * @return the token being held. + */ + function getToken() public view returns(IERC20) { + return token_; + } + + /** + * @return the beneficiary of the tokens. + */ + function getBeneficiary() public view returns(address) { + return beneficiary_; + } + + /** + * @return the time when the tokens are released. + */ + function getReleaseTime() public view returns(uint256) { + return releaseTime_; } /** @@ -39,11 +60,11 @@ contract TokenTimelock { */ function release() public { // solium-disable-next-line security/no-block-members - require(block.timestamp >= releaseTime); + require(block.timestamp >= releaseTime_); - uint256 amount = token.balanceOf(address(this)); + uint256 amount = token_.balanceOf(address(this)); require(amount > 0); - token.safeTransfer(beneficiary, amount); + token_.safeTransfer(beneficiary_, amount); } } diff --git a/contracts/token/ERC20/TokenVesting.sol b/contracts/token/ERC20/TokenVesting.sol index 31fac8882ce..cf3d97a783c 100644 --- a/contracts/token/ERC20/TokenVesting.sol +++ b/contracts/token/ERC20/TokenVesting.sol @@ -21,16 +21,16 @@ contract TokenVesting is Ownable { event Revoked(); // beneficiary of tokens after they are released - address public beneficiary; + address private beneficiary_; - uint256 public cliff; - uint256 public start; - uint256 public duration; + uint256 private cliff_; + uint256 private start_; + uint256 private duration_; - bool public revocable; + bool private revocable_; - mapping (address => uint256) public released; - mapping (address => bool) public revoked; + mapping (address => uint256) private released_; + mapping (address => bool) private revoked_; /** * @dev Creates a vesting contract that vests its balance of any ERC20 token to the @@ -54,11 +54,60 @@ contract TokenVesting is Ownable { require(_beneficiary != address(0)); require(_cliff <= _duration); - beneficiary = _beneficiary; - revocable = _revocable; - duration = _duration; - cliff = _start.add(_cliff); - start = _start; + beneficiary_ = _beneficiary; + revocable_ = _revocable; + duration_ = _duration; + cliff_ = _start.add(_cliff); + start_ = _start; + } + + /** + * @return the beneficiary of the tokens. + */ + function getBeneficiary() public view returns(address) { + return beneficiary_; + } + + /** + * @return the cliff time of the token vesting. + */ + function getCliff() public view returns(uint256) { + return cliff_; + } + + /** + * @return the start time of the token vesting. + */ + function getStart() public view returns(uint256) { + return start_; + } + + /** + * @return the duration of the token vesting. + */ + function getDuration() public view returns(uint256) { + return duration_; + } + + /** + * @return true if the vesting is revocable. + */ + function isRevocable() public view returns(bool) { + return revocable_; + } + + /** + * @return the amount released to an account. + */ + function getReleased(address _account) public view returns(uint256) { + return released_[_account]; + } + + /** + * @return true if the account is revoked. + */ + function isRevoked(address _account) public view returns(bool) { + return revoked_[_account]; } /** @@ -70,9 +119,9 @@ contract TokenVesting is Ownable { require(unreleased > 0); - released[_token] = released[_token].add(unreleased); + released_[_token] = released_[_token].add(unreleased); - _token.safeTransfer(beneficiary, unreleased); + _token.safeTransfer(beneficiary_, unreleased); emit Released(unreleased); } @@ -83,15 +132,15 @@ contract TokenVesting is Ownable { * @param _token ERC20 token which is being vested */ function revoke(IERC20 _token) public onlyOwner { - require(revocable); - require(!revoked[_token]); + require(revocable_); + require(!revoked_[_token]); uint256 balance = _token.balanceOf(address(this)); uint256 unreleased = releasableAmount(_token); uint256 refund = balance.sub(unreleased); - revoked[_token] = true; + revoked_[_token] = true; _token.safeTransfer(owner, refund); @@ -103,7 +152,7 @@ contract TokenVesting is Ownable { * @param _token ERC20 token which is being vested */ function releasableAmount(IERC20 _token) public view returns (uint256) { - return vestedAmount(_token).sub(released[_token]); + return vestedAmount(_token).sub(released_[_token]); } /** @@ -112,14 +161,14 @@ contract TokenVesting is Ownable { */ function vestedAmount(IERC20 _token) public view returns (uint256) { uint256 currentBalance = _token.balanceOf(this); - uint256 totalBalance = currentBalance.add(released[_token]); + uint256 totalBalance = currentBalance.add(released_[_token]); - if (block.timestamp < cliff) { + if (block.timestamp < cliff_) { return 0; - } else if (block.timestamp >= start.add(duration) || revoked[_token]) { + } else if (block.timestamp >= start_.add(duration_) || revoked_[_token]) { return totalBalance; } else { - return totalBalance.mul(block.timestamp.sub(start)).div(duration); + return totalBalance.mul(block.timestamp.sub(start_)).div(duration_); } } } diff --git a/test/token/ERC20/DetailedERC20.test.js b/test/token/ERC20/DetailedERC20.test.js index 71b22a7b9a8..07dce7f642e 100644 --- a/test/token/ERC20/DetailedERC20.test.js +++ b/test/token/ERC20/DetailedERC20.test.js @@ -18,14 +18,14 @@ contract('ERC20Detailed', function () { }); it('has a name', async function () { - (await detailedERC20.name()).should.be.equal(_name); + (await detailedERC20.getName()).should.be.equal(_name); }); it('has a symbol', async function () { - (await detailedERC20.symbol()).should.be.equal(_symbol); + (await detailedERC20.getSymbol()).should.be.equal(_symbol); }); it('has an amount of decimals', async function () { - (await detailedERC20.decimals()).should.be.bignumber.equal(_decimals); + (await detailedERC20.getDecimals()).should.be.bignumber.equal(_decimals); }); }); diff --git a/test/token/ERC20/ERC20Capped.behavior.js b/test/token/ERC20/ERC20Capped.behavior.js index 004d512c042..8aeb58d3279 100644 --- a/test/token/ERC20/ERC20Capped.behavior.js +++ b/test/token/ERC20/ERC20Capped.behavior.js @@ -12,7 +12,7 @@ function shouldBehaveLikeERC20Capped (minter, [anyone], cap) { const from = minter; it('should start with the correct cap', async function () { - (await this.token.cap()).should.be.bignumber.equal(cap); + (await this.token.getCap()).should.be.bignumber.equal(cap); }); it('should mint when amount is less than cap', async function () { diff --git a/test/token/ERC20/ERC20Mintable.behavior.js b/test/token/ERC20/ERC20Mintable.behavior.js index 63fb861b551..12a8d376378 100644 --- a/test/token/ERC20/ERC20Mintable.behavior.js +++ b/test/token/ERC20/ERC20Mintable.behavior.js @@ -20,7 +20,7 @@ function shouldBehaveLikeERC20Mintable (owner, minter, [anyone]) { describe('minting finished', function () { describe('when the token minting is not finished', function () { it('returns false', async function () { - (await this.token.mintingFinished()).should.equal(false); + (await this.token.isMintingFinished()).should.equal(false); }); }); @@ -30,7 +30,7 @@ function shouldBehaveLikeERC20Mintable (owner, minter, [anyone]) { }); it('returns true', async function () { - (await this.token.mintingFinished()).should.equal(true); + (await this.token.isMintingFinished()).should.equal(true); }); }); }); @@ -43,7 +43,7 @@ function shouldBehaveLikeERC20Mintable (owner, minter, [anyone]) { it('finishes token minting', async function () { await this.token.finishMinting({ from }); - (await this.token.mintingFinished()).should.equal(true); + (await this.token.isMintingFinished()).should.equal(true); }); it('emits a mint finished event', async function () { From 1c365fe9287800f4ae2c6a965195bbfa9fac4ff8 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 07:02:46 +0000 Subject: [PATCH 03/30] Improve encapsulation on ERC721 --- contracts/token/ERC721/ERC721.sol | 64 +++++++++++++------------- contracts/token/ERC721/ERC721Basic.sol | 36 +++++++-------- 2 files changed, 50 insertions(+), 50 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index ef6a98453c4..b52b2b5ae63 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -20,19 +20,19 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { string internal symbol_; // Mapping from owner to list of owned token IDs - mapping(address => uint256[]) internal ownedTokens; + mapping(address => uint256[]) private ownedTokens_; // Mapping from token ID to index of the owner tokens list - mapping(uint256 => uint256) internal ownedTokensIndex; + mapping(uint256 => uint256) private ownedTokensIndex_; // Array with all token ids, used for enumeration - uint256[] internal allTokens; + uint256[] private allTokens_; // Mapping from token id to position in the allTokens array - mapping(uint256 => uint256) internal allTokensIndex; + mapping(uint256 => uint256) private allTokensIndex_; // Optional mapping for token URIs - mapping(uint256 => string) internal tokenURIs; + mapping(uint256 => string) private tokenURIs_; /** * @dev Constructor function @@ -69,7 +69,7 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { */ function tokenURI(uint256 _tokenId) public view returns (string) { require(_exists(_tokenId)); - return tokenURIs[_tokenId]; + return tokenURIs_[_tokenId]; } /** @@ -87,7 +87,7 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { returns (uint256) { require(_index < balanceOf(_owner)); - return ownedTokens[_owner][_index]; + return ownedTokens_[_owner][_index]; } /** @@ -95,7 +95,7 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { * @return uint256 representing the total amount of tokens */ function totalSupply() public view returns (uint256) { - return allTokens.length; + return allTokens_.length; } /** @@ -106,7 +106,7 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { */ function tokenByIndex(uint256 _index) public view returns (uint256) { require(_index < totalSupply()); - return allTokens[_index]; + return allTokens_[_index]; } /** @@ -117,7 +117,7 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { */ function _setTokenURI(uint256 _tokenId, string _uri) internal { require(_exists(_tokenId)); - tokenURIs[_tokenId] = _uri; + tokenURIs_[_tokenId] = _uri; } /** @@ -127,9 +127,9 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { */ function addTokenTo(address _to, uint256 _tokenId) internal { super.addTokenTo(_to, _tokenId); - uint256 length = ownedTokens[_to].length; - ownedTokens[_to].push(_tokenId); - ownedTokensIndex[_tokenId] = length; + uint256 length = ownedTokens_[_to].length; + ownedTokens_[_to].push(_tokenId); + ownedTokensIndex_[_tokenId] = length; } /** @@ -142,20 +142,20 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { // To prevent a gap in the array, we store the last token in the index of the token to delete, and // then delete the last slot. - uint256 tokenIndex = ownedTokensIndex[_tokenId]; - uint256 lastTokenIndex = ownedTokens[_from].length.sub(1); - uint256 lastToken = ownedTokens[_from][lastTokenIndex]; + uint256 tokenIndex = ownedTokensIndex_[_tokenId]; + uint256 lastTokenIndex = ownedTokens_[_from].length.sub(1); + uint256 lastToken = ownedTokens_[_from][lastTokenIndex]; - ownedTokens[_from][tokenIndex] = lastToken; + ownedTokens_[_from][tokenIndex] = lastToken; // This also deletes the contents at the last position of the array - ownedTokens[_from].length--; + ownedTokens_[_from].length--; // Note that this will handle single-element arrays. In that case, both tokenIndex and lastTokenIndex are going to // be zero. Then we can make sure that we will remove _tokenId from the ownedTokens list since we are first swapping // the lastToken to the first position, and then dropping the element placed in the last position of the list - ownedTokensIndex[_tokenId] = 0; - ownedTokensIndex[lastToken] = tokenIndex; + ownedTokensIndex_[_tokenId] = 0; + ownedTokensIndex_[lastToken] = tokenIndex; } /** @@ -167,8 +167,8 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { function _mint(address _to, uint256 _tokenId) internal { super._mint(_to, _tokenId); - allTokensIndex[_tokenId] = allTokens.length; - allTokens.push(_tokenId); + allTokensIndex_[_tokenId] = allTokens_.length; + allTokens_.push(_tokenId); } /** @@ -181,21 +181,21 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { super._burn(_owner, _tokenId); // Clear metadata (if any) - if (bytes(tokenURIs[_tokenId]).length != 0) { - delete tokenURIs[_tokenId]; + if (bytes(tokenURIs_[_tokenId]).length != 0) { + delete tokenURIs_[_tokenId]; } // Reorg all tokens array - uint256 tokenIndex = allTokensIndex[_tokenId]; - uint256 lastTokenIndex = allTokens.length.sub(1); - uint256 lastToken = allTokens[lastTokenIndex]; + uint256 tokenIndex = allTokensIndex_[_tokenId]; + uint256 lastTokenIndex = allTokens_.length.sub(1); + uint256 lastToken = allTokens_[lastTokenIndex]; - allTokens[tokenIndex] = lastToken; - allTokens[lastTokenIndex] = 0; + allTokens_[tokenIndex] = lastToken; + allTokens_[lastTokenIndex] = 0; - allTokens.length--; - allTokensIndex[_tokenId] = 0; - allTokensIndex[lastToken] = tokenIndex; + allTokens_.length--; + allTokensIndex_[_tokenId] = 0; + allTokensIndex_[lastToken] = tokenIndex; } } diff --git a/contracts/token/ERC721/ERC721Basic.sol b/contracts/token/ERC721/ERC721Basic.sol index 58a40cae893..0c4700bf46a 100644 --- a/contracts/token/ERC721/ERC721Basic.sol +++ b/contracts/token/ERC721/ERC721Basic.sol @@ -21,16 +21,16 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { bytes4 private constant ERC721_RECEIVED = 0x150b7a02; // Mapping from token ID to owner - mapping (uint256 => address) internal tokenOwner; + mapping (uint256 => address) private tokenOwner_; // Mapping from token ID to approved address - mapping (uint256 => address) internal tokenApprovals; + mapping (uint256 => address) private tokenApprovals_; // Mapping from owner to number of owned token - mapping (address => uint256) internal ownedTokensCount; + mapping (address => uint256) private ownedTokensCount_; // Mapping from owner to operator approvals - mapping (address => mapping (address => bool)) internal operatorApprovals; + mapping (address => mapping (address => bool)) private operatorApprovals_; constructor() public @@ -46,7 +46,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { */ function balanceOf(address _owner) public view returns (uint256) { require(_owner != address(0)); - return ownedTokensCount[_owner]; + return ownedTokensCount_[_owner]; } /** @@ -55,7 +55,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { * @return owner address currently marked as the owner of the given token ID */ function ownerOf(uint256 _tokenId) public view returns (address) { - address owner = tokenOwner[_tokenId]; + address owner = tokenOwner_[_tokenId]; require(owner != address(0)); return owner; } @@ -73,7 +73,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { require(_to != owner); require(msg.sender == owner || isApprovedForAll(owner, msg.sender)); - tokenApprovals[_tokenId] = _to; + tokenApprovals_[_tokenId] = _to; emit Approval(owner, _to, _tokenId); } @@ -83,7 +83,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { * @return address currently approved for the given token ID */ function getApproved(uint256 _tokenId) public view returns (address) { - return tokenApprovals[_tokenId]; + return tokenApprovals_[_tokenId]; } /** @@ -94,7 +94,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { */ function setApprovalForAll(address _to, bool _approved) public { require(_to != msg.sender); - operatorApprovals[msg.sender][_to] = _approved; + operatorApprovals_[msg.sender][_to] = _approved; emit ApprovalForAll(msg.sender, _to, _approved); } @@ -112,7 +112,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { view returns (bool) { - return operatorApprovals[_owner][_operator]; + return operatorApprovals_[_owner][_operator]; } /** @@ -194,7 +194,7 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { * @return whether the token exists */ function _exists(uint256 _tokenId) internal view returns (bool) { - address owner = tokenOwner[_tokenId]; + address owner = tokenOwner_[_tokenId]; return owner != address(0); } @@ -255,8 +255,8 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { */ function clearApproval(address _owner, uint256 _tokenId) internal { require(ownerOf(_tokenId) == _owner); - if (tokenApprovals[_tokenId] != address(0)) { - tokenApprovals[_tokenId] = address(0); + if (tokenApprovals_[_tokenId] != address(0)) { + tokenApprovals_[_tokenId] = address(0); } } @@ -266,9 +266,9 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { * @param _tokenId uint256 ID of the token to be added to the tokens list of the given address */ function addTokenTo(address _to, uint256 _tokenId) internal { - require(tokenOwner[_tokenId] == address(0)); - tokenOwner[_tokenId] = _to; - ownedTokensCount[_to] = ownedTokensCount[_to].add(1); + require(tokenOwner_[_tokenId] == address(0)); + tokenOwner_[_tokenId] = _to; + ownedTokensCount_[_to] = ownedTokensCount_[_to].add(1); } /** @@ -278,8 +278,8 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { */ function removeTokenFrom(address _from, uint256 _tokenId) internal { require(ownerOf(_tokenId) == _from); - ownedTokensCount[_from] = ownedTokensCount[_from].sub(1); - tokenOwner[_tokenId] = address(0); + ownedTokensCount_[_from] = ownedTokensCount_[_from].sub(1); + tokenOwner_[_tokenId] = address(0); } /** From 9ebdae64860039eaf9881b86466f2dc0e15f7f47 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 16:27:05 +0000 Subject: [PATCH 04/30] Add tests, use standard getters --- contracts/token/ERC20/ERC20Detailed.sol | 6 +++--- contracts/token/ERC20/TokenVesting.sol | 6 +++--- ...ailedERC20.test.js => ERC20Detailed.test.js} | 6 +++--- test/token/ERC20/TokenTimelock.test.js | 6 ++++++ test/token/ERC20/TokenVesting.test.js | 17 ++++++++++++++--- 5 files changed, 29 insertions(+), 12 deletions(-) rename test/token/ERC20/{DetailedERC20.test.js => ERC20Detailed.test.js} (75%) diff --git a/contracts/token/ERC20/ERC20Detailed.sol b/contracts/token/ERC20/ERC20Detailed.sol index aefa3947184..6becdee1086 100644 --- a/contracts/token/ERC20/ERC20Detailed.sol +++ b/contracts/token/ERC20/ERC20Detailed.sol @@ -23,21 +23,21 @@ contract ERC20Detailed is IERC20 { /** * @return the name of the token. */ - function getName() public view returns(string) { + function name() public view returns(string) { return name_; } /** * @return the symbol of the token. */ - function getSymbol() public view returns(string) { + function symbol() public view returns(string) { return symbol_; } /** * @return the number of decimals of the token. */ - function getDecimals() public view returns(uint8) { + function decimals() public view returns(uint8) { return decimals_; } } diff --git a/contracts/token/ERC20/TokenVesting.sol b/contracts/token/ERC20/TokenVesting.sol index cf3d97a783c..422431e7200 100644 --- a/contracts/token/ERC20/TokenVesting.sol +++ b/contracts/token/ERC20/TokenVesting.sol @@ -104,10 +104,10 @@ contract TokenVesting is Ownable { } /** - * @return true if the account is revoked. + * @return true if the token is revoked. */ - function isRevoked(address _account) public view returns(bool) { - return revoked_[_account]; + function isRevoked(address _token) public view returns(bool) { + return revoked_[_token]; } /** diff --git a/test/token/ERC20/DetailedERC20.test.js b/test/token/ERC20/ERC20Detailed.test.js similarity index 75% rename from test/token/ERC20/DetailedERC20.test.js rename to test/token/ERC20/ERC20Detailed.test.js index 07dce7f642e..71b22a7b9a8 100644 --- a/test/token/ERC20/DetailedERC20.test.js +++ b/test/token/ERC20/ERC20Detailed.test.js @@ -18,14 +18,14 @@ contract('ERC20Detailed', function () { }); it('has a name', async function () { - (await detailedERC20.getName()).should.be.equal(_name); + (await detailedERC20.name()).should.be.equal(_name); }); it('has a symbol', async function () { - (await detailedERC20.getSymbol()).should.be.equal(_symbol); + (await detailedERC20.symbol()).should.be.equal(_symbol); }); it('has an amount of decimals', async function () { - (await detailedERC20.getDecimals()).should.be.bignumber.equal(_decimals); + (await detailedERC20.decimals()).should.be.bignumber.equal(_decimals); }); }); diff --git a/test/token/ERC20/TokenTimelock.test.js b/test/token/ERC20/TokenTimelock.test.js index 168992707af..c121247e3e7 100644 --- a/test/token/ERC20/TokenTimelock.test.js +++ b/test/token/ERC20/TokenTimelock.test.js @@ -33,6 +33,12 @@ contract('TokenTimelock', function ([_, owner, beneficiary]) { await this.token.mint(this.timelock.address, amount, { from: owner }); }); + it('can get state', async function () { + (await this.timelock.getToken()).should.be.equal(this.token.address); + (await this.timelock.getBeneficiary()).should.be.equal(beneficiary); + (await this.timelock.getReleaseTime()).should.be.bignumber.equal(this.releaseTime); + }); + it('cannot be released before time limit', async function () { await expectThrow(this.timelock.release()); }); diff --git a/test/token/ERC20/TokenVesting.test.js b/test/token/ERC20/TokenVesting.test.js index 60d773511ec..7bb6d72c69a 100644 --- a/test/token/ERC20/TokenVesting.test.js +++ b/test/token/ERC20/TokenVesting.test.js @@ -48,6 +48,14 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { await this.token.mint(this.vesting.address, amount, { from: owner }); }); + it('can get state', async function () { + (await this.vesting.getBeneficiary()).should.be.equal(beneficiary); + (await this.vesting.getCliff()).should.be.equal(this.cliff); + (await this.vesting.getStart()).should.be.equal(this.start); + (await this.vesting.getDuration()).should.be.equal(this.duration); + (await this.vesting.isRevocable()).should.be.equal(true); + }); + it('cannot be released before cliff', async function () { await expectThrow( this.vesting.release(this.token.address), @@ -67,9 +75,9 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { const block = await ethGetBlock(receipt.blockNumber); const releaseTime = block.timestamp; - (await this.token.balanceOf(beneficiary)).should.bignumber.equal( - amount.mul(releaseTime - this.start).div(this.duration).floor() - ); + const releasedAmount = amount.mul(releaseTime - this.start).div(this.duration).floor(); + (await this.token.balanceOf(beneficiary)).should.bignumber.equal(releasedAmount); + (await this.vesting.getReleased(beneficiary)).should.bignumber.equal(releasedAmount); }); it('should linearly release tokens during vesting period', async function () { @@ -83,6 +91,7 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { await this.vesting.release(this.token.address); const expectedVesting = amount.mul(now - this.start).div(this.duration).floor(); (await this.token.balanceOf(beneficiary)).should.bignumber.equal(expectedVesting); + (await this.vesting.getReleased(beneficiary)).should.bignumber.equal(expectedVesting); } }); @@ -90,10 +99,12 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { await increaseTimeTo(this.start + this.duration); await this.vesting.release(this.token.address); (await this.token.balanceOf(beneficiary)).should.bignumber.equal(amount); + (await this.vesting.getReleased(beneficiary)).should.bignumber.equal(amount); }); it('should be revoked by owner if revocable is set', async function () { await this.vesting.revoke(this.token.address, { from: owner }); + (await this.vesting.isRevoked(this.token.address)).should.equal(true); }); it('should fail to be revoked by owner if revocable not set', async function () { From 963a00bfa6b587e9be728750bea5689c5b1e8ebf Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 17:06:19 +0000 Subject: [PATCH 05/30] fix tests --- contracts/token/ERC20/TokenVesting.sol | 14 ++++---- test/token/ERC20/TokenVesting.test.js | 44 +++++++++++++------------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/contracts/token/ERC20/TokenVesting.sol b/contracts/token/ERC20/TokenVesting.sol index 422431e7200..5c48e18882e 100644 --- a/contracts/token/ERC20/TokenVesting.sol +++ b/contracts/token/ERC20/TokenVesting.sol @@ -37,7 +37,7 @@ contract TokenVesting is Ownable { * _beneficiary, gradually in a linear fashion until _start + _duration. By then all * of the balance will have vested. * @param _beneficiary address of the beneficiary to whom vested tokens are transferred - * @param _cliff duration in seconds of the cliff in which tokens will begin to vest + * @param _cliffDuration duration in seconds of the cliff in which tokens will begin to vest * @param _start the time (as Unix time) at which point vesting starts * @param _duration duration in seconds of the period in which the tokens will vest * @param _revocable whether the vesting is revocable or not @@ -45,19 +45,19 @@ contract TokenVesting is Ownable { constructor( address _beneficiary, uint256 _start, - uint256 _cliff, + uint256 _cliffDuration, uint256 _duration, bool _revocable ) public { require(_beneficiary != address(0)); - require(_cliff <= _duration); + require(_cliffDuration <= _duration); beneficiary_ = _beneficiary; revocable_ = _revocable; duration_ = _duration; - cliff_ = _start.add(_cliff); + cliff_ = _start.add(_cliffDuration); start_ = _start; } @@ -97,10 +97,10 @@ contract TokenVesting is Ownable { } /** - * @return the amount released to an account. + * @return the amount of the token released. */ - function getReleased(address _account) public view returns(uint256) { - return released_[_account]; + function getReleased(address _token) public view returns(uint256) { + return released_[_token]; } /** diff --git a/test/token/ERC20/TokenVesting.test.js b/test/token/ERC20/TokenVesting.test.js index 7bb6d72c69a..9fc7c347599 100644 --- a/test/token/ERC20/TokenVesting.test.js +++ b/test/token/ERC20/TokenVesting.test.js @@ -13,36 +13,36 @@ require('chai') const ERC20Mintable = artifacts.require('ERC20Mintable'); const TokenVesting = artifacts.require('TokenVesting'); -contract('TokenVesting', function ([_, owner, beneficiary]) { +contract.only('TokenVesting', function ([_, owner, beneficiary]) { const amount = new BigNumber(1000); const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'; beforeEach(async function () { this.start = (await latestTime()) + duration.minutes(1); // +1 minute so it starts after contract instantiation - this.cliff = duration.years(1); + this.cliffDuration = duration.years(1); this.duration = duration.years(2); }); it('rejects a duration shorter than the cliff', async function () { - const cliff = this.duration; - const duration = this.cliff; + const cliffDuration = this.duration; + const duration = this.cliffDuration; - cliff.should.be.gt(duration); + cliffDuration.should.be.gt(duration); await expectThrow( - TokenVesting.new(beneficiary, this.start, cliff, duration, true, { from: owner }) + TokenVesting.new(beneficiary, this.start, cliffDuration, duration, true, { from: owner }) ); }); it('requires a valid beneficiary', async function () { await expectThrow( - TokenVesting.new(ZERO_ADDRESS, this.start, this.cliff, this.duration, true, { from: owner }) + TokenVesting.new(ZERO_ADDRESS, this.start, this.cliffDuration, this.duration, true, { from: owner }) ); }); context('once deployed', function () { beforeEach(async function () { - this.vesting = await TokenVesting.new(beneficiary, this.start, this.cliff, this.duration, true, { from: owner }); + this.vesting = await TokenVesting.new(beneficiary, this.start, this.cliffDuration, this.duration, true, { from: owner }); this.token = await ERC20Mintable.new({ from: owner }); await this.token.mint(this.vesting.address, amount, { from: owner }); @@ -50,9 +50,9 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { it('can get state', async function () { (await this.vesting.getBeneficiary()).should.be.equal(beneficiary); - (await this.vesting.getCliff()).should.be.equal(this.cliff); - (await this.vesting.getStart()).should.be.equal(this.start); - (await this.vesting.getDuration()).should.be.equal(this.duration); + (await this.vesting.getCliff()).should.be.bignumber.equal(this.start + this.cliffDuration); + (await this.vesting.getStart()).should.be.bignumber.equal(this.start); + (await this.vesting.getDuration()).should.be.bignumber.equal(this.duration); (await this.vesting.isRevocable()).should.be.equal(true); }); @@ -64,12 +64,12 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { }); it('can be released after cliff', async function () { - await increaseTimeTo(this.start + this.cliff + duration.weeks(1)); + await increaseTimeTo(this.start + this.cliffDuration + duration.weeks(1)); await this.vesting.release(this.token.address); }); it('should release proper amount after cliff', async function () { - await increaseTimeTo(this.start + this.cliff); + await increaseTimeTo(this.start + this.cliffDuration); const { receipt } = await this.vesting.release(this.token.address); const block = await ethGetBlock(receipt.blockNumber); @@ -77,21 +77,21 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { const releasedAmount = amount.mul(releaseTime - this.start).div(this.duration).floor(); (await this.token.balanceOf(beneficiary)).should.bignumber.equal(releasedAmount); - (await this.vesting.getReleased(beneficiary)).should.bignumber.equal(releasedAmount); + (await this.vesting.getReleased(this.token.address)).should.bignumber.equal(releasedAmount); }); it('should linearly release tokens during vesting period', async function () { - const vestingPeriod = this.duration - this.cliff; + const vestingPeriod = this.duration - this.cliffDuration; const checkpoints = 4; for (let i = 1; i <= checkpoints; i++) { - const now = this.start + this.cliff + i * (vestingPeriod / checkpoints); + const now = this.start + this.cliffDuration + i * (vestingPeriod / checkpoints); await increaseTimeTo(now); await this.vesting.release(this.token.address); const expectedVesting = amount.mul(now - this.start).div(this.duration).floor(); (await this.token.balanceOf(beneficiary)).should.bignumber.equal(expectedVesting); - (await this.vesting.getReleased(beneficiary)).should.bignumber.equal(expectedVesting); + (await this.vesting.getReleased(this.token.address)).should.bignumber.equal(expectedVesting); } }); @@ -99,7 +99,7 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { await increaseTimeTo(this.start + this.duration); await this.vesting.release(this.token.address); (await this.token.balanceOf(beneficiary)).should.bignumber.equal(amount); - (await this.vesting.getReleased(beneficiary)).should.bignumber.equal(amount); + (await this.vesting.getReleased(this.token.address)).should.bignumber.equal(amount); }); it('should be revoked by owner if revocable is set', async function () { @@ -109,7 +109,7 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { it('should fail to be revoked by owner if revocable not set', async function () { const vesting = await TokenVesting.new( - beneficiary, this.start, this.cliff, this.duration, false, { from: owner } + beneficiary, this.start, this.cliffDuration, this.duration, false, { from: owner } ); await expectThrow( @@ -119,7 +119,7 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { }); it('should return the non-vested tokens when revoked by owner', async function () { - await increaseTimeTo(this.start + this.cliff + duration.weeks(12)); + await increaseTimeTo(this.start + this.cliffDuration + duration.weeks(12)); const vested = await this.vesting.vestedAmount(this.token.address); @@ -129,7 +129,7 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { }); it('should keep the vested tokens when revoked by owner', async function () { - await increaseTimeTo(this.start + this.cliff + duration.weeks(12)); + await increaseTimeTo(this.start + this.cliffDuration + duration.weeks(12)); const vestedPre = await this.vesting.vestedAmount(this.token.address); @@ -141,7 +141,7 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { }); it('should fail to be revoked a second time', async function () { - await increaseTimeTo(this.start + this.cliff + duration.weeks(12)); + await increaseTimeTo(this.start + this.cliffDuration + duration.weeks(12)); await this.vesting.vestedAmount(this.token.address); From 4d8db8d8e5e8a1b3b639ed4ab5d70726ee3c9fe1 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 17:48:16 +0000 Subject: [PATCH 06/30] Fix lint --- test/token/ERC20/TokenVesting.test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/token/ERC20/TokenVesting.test.js b/test/token/ERC20/TokenVesting.test.js index 9fc7c347599..dcf080cfe50 100644 --- a/test/token/ERC20/TokenVesting.test.js +++ b/test/token/ERC20/TokenVesting.test.js @@ -13,7 +13,7 @@ require('chai') const ERC20Mintable = artifacts.require('ERC20Mintable'); const TokenVesting = artifacts.require('TokenVesting'); -contract.only('TokenVesting', function ([_, owner, beneficiary]) { +contract('TokenVesting', function ([_, owner, beneficiary]) { const amount = new BigNumber(1000); const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'; @@ -42,7 +42,8 @@ contract.only('TokenVesting', function ([_, owner, beneficiary]) { context('once deployed', function () { beforeEach(async function () { - this.vesting = await TokenVesting.new(beneficiary, this.start, this.cliffDuration, this.duration, true, { from: owner }); + this.vesting = await TokenVesting.new( + beneficiary, this.start, this.cliffDuration, this.duration, true, { from: owner }); this.token = await ERC20Mintable.new({ from: owner }); await this.token.mint(this.vesting.address, amount, { from: owner }); From 0b0d6c6ebb8f515919b1fee53b67c75c08460a4f Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 4 Sep 2018 17:35:53 -0300 Subject: [PATCH 07/30] move interface ids to implementation contracts --- contracts/token/ERC721/ERC721.sol | 16 +++++++++++++ contracts/token/ERC721/ERC721Basic.sol | 14 ++++++++++++ contracts/token/ERC721/IERC721Basic.sol | 30 ------------------------- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index 35575bd6a0d..a2c63ef5527 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -34,6 +34,22 @@ contract ERC721 is SupportsInterfaceWithLookup, ERC721Basic, IERC721 { // Optional mapping for token URIs mapping(uint256 => string) private tokenURIs_; + bytes4 private constant InterfaceId_ERC721Enumerable = 0x780e9d63; + /** + * 0x780e9d63 === + * bytes4(keccak256('totalSupply()')) ^ + * bytes4(keccak256('tokenOfOwnerByIndex(address,uint256)')) ^ + * bytes4(keccak256('tokenByIndex(uint256)')) + */ + + bytes4 private constant InterfaceId_ERC721Metadata = 0x5b5e139f; + /** + * 0x5b5e139f === + * bytes4(keccak256('name()')) ^ + * bytes4(keccak256('symbol()')) ^ + * bytes4(keccak256('tokenURI(uint256)')) + */ + /** * @dev Constructor function */ diff --git a/contracts/token/ERC721/ERC721Basic.sol b/contracts/token/ERC721/ERC721Basic.sol index 93b1f2321d6..fcc8397ea87 100644 --- a/contracts/token/ERC721/ERC721Basic.sol +++ b/contracts/token/ERC721/ERC721Basic.sol @@ -32,6 +32,20 @@ contract ERC721Basic is SupportsInterfaceWithLookup, IERC721Basic { // Mapping from owner to operator approvals mapping (address => mapping (address => bool)) private operatorApprovals_; + bytes4 private constant InterfaceId_ERC721 = 0x80ac58cd; + /* + * 0x80ac58cd === + * bytes4(keccak256('balanceOf(address)')) ^ + * bytes4(keccak256('ownerOf(uint256)')) ^ + * bytes4(keccak256('approve(address,uint256)')) ^ + * bytes4(keccak256('getApproved(uint256)')) ^ + * bytes4(keccak256('setApprovalForAll(address,bool)')) ^ + * bytes4(keccak256('isApprovedForAll(address,address)')) ^ + * bytes4(keccak256('transferFrom(address,address,uint256)')) ^ + * bytes4(keccak256('safeTransferFrom(address,address,uint256)')) ^ + * bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)')) + */ + constructor() public { diff --git a/contracts/token/ERC721/IERC721Basic.sol b/contracts/token/ERC721/IERC721Basic.sol index 50a5c7f5359..6e71d1cf589 100644 --- a/contracts/token/ERC721/IERC721Basic.sol +++ b/contracts/token/ERC721/IERC721Basic.sol @@ -9,36 +9,6 @@ import "../../introspection/IERC165.sol"; */ contract IERC721Basic is IERC165 { - bytes4 internal constant InterfaceId_ERC721 = 0x80ac58cd; - /* - * 0x80ac58cd === - * bytes4(keccak256('balanceOf(address)')) ^ - * bytes4(keccak256('ownerOf(uint256)')) ^ - * bytes4(keccak256('approve(address,uint256)')) ^ - * bytes4(keccak256('getApproved(uint256)')) ^ - * bytes4(keccak256('setApprovalForAll(address,bool)')) ^ - * bytes4(keccak256('isApprovedForAll(address,address)')) ^ - * bytes4(keccak256('transferFrom(address,address,uint256)')) ^ - * bytes4(keccak256('safeTransferFrom(address,address,uint256)')) ^ - * bytes4(keccak256('safeTransferFrom(address,address,uint256,bytes)')) - */ - - bytes4 internal constant InterfaceId_ERC721Enumerable = 0x780e9d63; - /** - * 0x780e9d63 === - * bytes4(keccak256('totalSupply()')) ^ - * bytes4(keccak256('tokenOfOwnerByIndex(address,uint256)')) ^ - * bytes4(keccak256('tokenByIndex(uint256)')) - */ - - bytes4 internal constant InterfaceId_ERC721Metadata = 0x5b5e139f; - /** - * 0x5b5e139f === - * bytes4(keccak256('name()')) ^ - * bytes4(keccak256('symbol()')) ^ - * bytes4(keccak256('tokenURI(uint256)')) - */ - event Transfer( address indexed from, address indexed to, From 68094d13c8105908ce5301d32e3c538eb3788c2f Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Wed, 5 Sep 2018 02:30:19 +0000 Subject: [PATCH 08/30] Do not prefix getters --- contracts/token/ERC20/ERC20Capped.sol | 2 +- contracts/token/ERC20/ERC20Mintable.sol | 2 +- contracts/token/ERC20/TokenTimelock.sol | 6 +++--- contracts/token/ERC20/TokenVesting.sol | 14 +++++++------- test/token/ERC20/ERC20Capped.behavior.js | 2 +- test/token/ERC20/ERC20Mintable.behavior.js | 6 +++--- test/token/ERC20/TokenTimelock.test.js | 6 +++--- test/token/ERC20/TokenVesting.test.js | 18 +++++++++--------- 8 files changed, 28 insertions(+), 28 deletions(-) diff --git a/contracts/token/ERC20/ERC20Capped.sol b/contracts/token/ERC20/ERC20Capped.sol index 13f5dabdcbf..23631f9049c 100644 --- a/contracts/token/ERC20/ERC20Capped.sol +++ b/contracts/token/ERC20/ERC20Capped.sol @@ -19,7 +19,7 @@ contract ERC20Capped is ERC20Mintable { /** * @return the cap for the token minting. */ - function getCap() public view returns(uint256) { + function cap() public view returns(uint256) { return cap_; } diff --git a/contracts/token/ERC20/ERC20Mintable.sol b/contracts/token/ERC20/ERC20Mintable.sol index 6c329317a58..e680e6b6055 100644 --- a/contracts/token/ERC20/ERC20Mintable.sol +++ b/contracts/token/ERC20/ERC20Mintable.sol @@ -29,7 +29,7 @@ contract ERC20Mintable is ERC20, Ownable { /** * @return true if the minting is finished. */ - function isMintingFinished() public view returns(bool) { + function mintingFinished() public view returns(bool) { return mintingFinished_; } diff --git a/contracts/token/ERC20/TokenTimelock.sol b/contracts/token/ERC20/TokenTimelock.sol index 0ed6341a71c..d8f96fa6668 100644 --- a/contracts/token/ERC20/TokenTimelock.sol +++ b/contracts/token/ERC20/TokenTimelock.sol @@ -37,21 +37,21 @@ contract TokenTimelock { /** * @return the token being held. */ - function getToken() public view returns(IERC20) { + function token() public view returns(IERC20) { return token_; } /** * @return the beneficiary of the tokens. */ - function getBeneficiary() public view returns(address) { + function beneficiary() public view returns(address) { return beneficiary_; } /** * @return the time when the tokens are released. */ - function getReleaseTime() public view returns(uint256) { + function releaseTime() public view returns(uint256) { return releaseTime_; } diff --git a/contracts/token/ERC20/TokenVesting.sol b/contracts/token/ERC20/TokenVesting.sol index 5c48e18882e..5cadf03c465 100644 --- a/contracts/token/ERC20/TokenVesting.sol +++ b/contracts/token/ERC20/TokenVesting.sol @@ -64,49 +64,49 @@ contract TokenVesting is Ownable { /** * @return the beneficiary of the tokens. */ - function getBeneficiary() public view returns(address) { + function beneficiary() public view returns(address) { return beneficiary_; } /** * @return the cliff time of the token vesting. */ - function getCliff() public view returns(uint256) { + function cliff() public view returns(uint256) { return cliff_; } /** * @return the start time of the token vesting. */ - function getStart() public view returns(uint256) { + function start() public view returns(uint256) { return start_; } /** * @return the duration of the token vesting. */ - function getDuration() public view returns(uint256) { + function duration() public view returns(uint256) { return duration_; } /** * @return true if the vesting is revocable. */ - function isRevocable() public view returns(bool) { + function revocable() public view returns(bool) { return revocable_; } /** * @return the amount of the token released. */ - function getReleased(address _token) public view returns(uint256) { + function released(address _token) public view returns(uint256) { return released_[_token]; } /** * @return true if the token is revoked. */ - function isRevoked(address _token) public view returns(bool) { + function revoked(address _token) public view returns(bool) { return revoked_[_token]; } diff --git a/test/token/ERC20/ERC20Capped.behavior.js b/test/token/ERC20/ERC20Capped.behavior.js index 8aeb58d3279..004d512c042 100644 --- a/test/token/ERC20/ERC20Capped.behavior.js +++ b/test/token/ERC20/ERC20Capped.behavior.js @@ -12,7 +12,7 @@ function shouldBehaveLikeERC20Capped (minter, [anyone], cap) { const from = minter; it('should start with the correct cap', async function () { - (await this.token.getCap()).should.be.bignumber.equal(cap); + (await this.token.cap()).should.be.bignumber.equal(cap); }); it('should mint when amount is less than cap', async function () { diff --git a/test/token/ERC20/ERC20Mintable.behavior.js b/test/token/ERC20/ERC20Mintable.behavior.js index 12a8d376378..63fb861b551 100644 --- a/test/token/ERC20/ERC20Mintable.behavior.js +++ b/test/token/ERC20/ERC20Mintable.behavior.js @@ -20,7 +20,7 @@ function shouldBehaveLikeERC20Mintable (owner, minter, [anyone]) { describe('minting finished', function () { describe('when the token minting is not finished', function () { it('returns false', async function () { - (await this.token.isMintingFinished()).should.equal(false); + (await this.token.mintingFinished()).should.equal(false); }); }); @@ -30,7 +30,7 @@ function shouldBehaveLikeERC20Mintable (owner, minter, [anyone]) { }); it('returns true', async function () { - (await this.token.isMintingFinished()).should.equal(true); + (await this.token.mintingFinished()).should.equal(true); }); }); }); @@ -43,7 +43,7 @@ function shouldBehaveLikeERC20Mintable (owner, minter, [anyone]) { it('finishes token minting', async function () { await this.token.finishMinting({ from }); - (await this.token.isMintingFinished()).should.equal(true); + (await this.token.mintingFinished()).should.equal(true); }); it('emits a mint finished event', async function () { diff --git a/test/token/ERC20/TokenTimelock.test.js b/test/token/ERC20/TokenTimelock.test.js index c121247e3e7..70eaae8f54d 100644 --- a/test/token/ERC20/TokenTimelock.test.js +++ b/test/token/ERC20/TokenTimelock.test.js @@ -34,9 +34,9 @@ contract('TokenTimelock', function ([_, owner, beneficiary]) { }); it('can get state', async function () { - (await this.timelock.getToken()).should.be.equal(this.token.address); - (await this.timelock.getBeneficiary()).should.be.equal(beneficiary); - (await this.timelock.getReleaseTime()).should.be.bignumber.equal(this.releaseTime); + (await this.timelock.token()).should.be.equal(this.token.address); + (await this.timelock.beneficiary()).should.be.equal(beneficiary); + (await this.timelock.releaseTime()).should.be.bignumber.equal(this.releaseTime); }); it('cannot be released before time limit', async function () { diff --git a/test/token/ERC20/TokenVesting.test.js b/test/token/ERC20/TokenVesting.test.js index dcf080cfe50..1fe9326f5fa 100644 --- a/test/token/ERC20/TokenVesting.test.js +++ b/test/token/ERC20/TokenVesting.test.js @@ -50,11 +50,11 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { }); it('can get state', async function () { - (await this.vesting.getBeneficiary()).should.be.equal(beneficiary); - (await this.vesting.getCliff()).should.be.bignumber.equal(this.start + this.cliffDuration); - (await this.vesting.getStart()).should.be.bignumber.equal(this.start); - (await this.vesting.getDuration()).should.be.bignumber.equal(this.duration); - (await this.vesting.isRevocable()).should.be.equal(true); + (await this.vesting.beneficiary()).should.be.equal(beneficiary); + (await this.vesting.cliff()).should.be.bignumber.equal(this.start + this.cliffDuration); + (await this.vesting.start()).should.be.bignumber.equal(this.start); + (await this.vesting.duration()).should.be.bignumber.equal(this.duration); + (await this.vesting.revocable()).should.be.equal(true); }); it('cannot be released before cliff', async function () { @@ -78,7 +78,7 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { const releasedAmount = amount.mul(releaseTime - this.start).div(this.duration).floor(); (await this.token.balanceOf(beneficiary)).should.bignumber.equal(releasedAmount); - (await this.vesting.getReleased(this.token.address)).should.bignumber.equal(releasedAmount); + (await this.vesting.released(this.token.address)).should.bignumber.equal(releasedAmount); }); it('should linearly release tokens during vesting period', async function () { @@ -92,7 +92,7 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { await this.vesting.release(this.token.address); const expectedVesting = amount.mul(now - this.start).div(this.duration).floor(); (await this.token.balanceOf(beneficiary)).should.bignumber.equal(expectedVesting); - (await this.vesting.getReleased(this.token.address)).should.bignumber.equal(expectedVesting); + (await this.vesting.released(this.token.address)).should.bignumber.equal(expectedVesting); } }); @@ -100,12 +100,12 @@ contract('TokenVesting', function ([_, owner, beneficiary]) { await increaseTimeTo(this.start + this.duration); await this.vesting.release(this.token.address); (await this.token.balanceOf(beneficiary)).should.bignumber.equal(amount); - (await this.vesting.getReleased(this.token.address)).should.bignumber.equal(amount); + (await this.vesting.released(this.token.address)).should.bignumber.equal(amount); }); it('should be revoked by owner if revocable is set', async function () { await this.vesting.revoke(this.token.address, { from: owner }); - (await this.vesting.isRevoked(this.token.address)).should.equal(true); + (await this.vesting.revoked(this.token.address)).should.equal(true); }); it('should fail to be revoked by owner if revocable not set', async function () { From 5fc3742423913c8e460cac5fc2a947a029daf420 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 03:11:24 +0000 Subject: [PATCH 09/30] Improve encapsulation on Pausable --- contracts/lifecycle/Pausable.sol | 9 ++++++++- test/token/ERC20/ERC20Pausable.test.js | 10 +++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/contracts/lifecycle/Pausable.sol b/contracts/lifecycle/Pausable.sol index 0a2767aa61f..03dff6692ee 100644 --- a/contracts/lifecycle/Pausable.sol +++ b/contracts/lifecycle/Pausable.sol @@ -12,9 +12,16 @@ contract Pausable is Ownable { event Paused(); event Unpaused(); - bool public paused = false; + bool private paused = false; + /** + * @return true if the contract is paused, false otherwise. + */ + function isPaused() public view returns(bool) { + return paused; + } + /** * @dev Modifier to make a function callable only when the contract is not paused. */ diff --git a/test/token/ERC20/ERC20Pausable.test.js b/test/token/ERC20/ERC20Pausable.test.js index 8fbe4f70876..b11ecb3e7b6 100644 --- a/test/token/ERC20/ERC20Pausable.test.js +++ b/test/token/ERC20/ERC20Pausable.test.js @@ -13,7 +13,7 @@ contract('ERC20Pausable', function ([_, owner, recipient, anotherAccount]) { describe('when the token is unpaused', function () { it('pauses the token', async function () { await this.token.pause({ from }); - (await this.token.paused()).should.equal(true); + (await this.token.isPaused()).should.equal(true); }); it('emits a Pause event', async function () { @@ -55,7 +55,7 @@ contract('ERC20Pausable', function ([_, owner, recipient, anotherAccount]) { it('unpauses the token', async function () { await this.token.unpause({ from }); - (await this.token.paused()).should.equal(false); + (await this.token.isPaused()).should.equal(false); }); it('emits an Unpause event', async function () { @@ -87,18 +87,18 @@ contract('ERC20Pausable', function ([_, owner, recipient, anotherAccount]) { describe('paused', function () { it('is not paused by default', async function () { - (await this.token.paused({ from })).should.equal(false); + (await this.token.isPaused({ from })).should.equal(false); }); it('is paused after being paused', async function () { await this.token.pause({ from }); - (await this.token.paused({ from })).should.equal(true); + (await this.token.isPaused({ from })).should.equal(true); }); it('is not paused after being paused and then unpaused', async function () { await this.token.pause({ from }); await this.token.unpause({ from }); - (await this.token.paused()).should.equal(false); + (await this.token.isPaused()).should.equal(false); }); }); From 172b72066699690b24c446acc1ecd34bddb4acba Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 03:13:48 +0000 Subject: [PATCH 10/30] add the underscore --- contracts/lifecycle/Pausable.sol | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/lifecycle/Pausable.sol b/contracts/lifecycle/Pausable.sol index 03dff6692ee..3e5676f7f87 100644 --- a/contracts/lifecycle/Pausable.sol +++ b/contracts/lifecycle/Pausable.sol @@ -12,21 +12,21 @@ contract Pausable is Ownable { event Paused(); event Unpaused(); - bool private paused = false; + bool private paused_ = false; /** * @return true if the contract is paused, false otherwise. */ function isPaused() public view returns(bool) { - return paused; + return paused_; } /** * @dev Modifier to make a function callable only when the contract is not paused. */ modifier whenNotPaused() { - require(!paused); + require(!paused_); _; } @@ -34,7 +34,7 @@ contract Pausable is Ownable { * @dev Modifier to make a function callable only when the contract is paused. */ modifier whenPaused() { - require(paused); + require(paused_); _; } @@ -42,7 +42,7 @@ contract Pausable is Ownable { * @dev called by the owner to pause, triggers stopped state */ function pause() public onlyOwner whenNotPaused { - paused = true; + paused_ = true; emit Paused(); } @@ -50,7 +50,7 @@ contract Pausable is Ownable { * @dev called by the owner to unpause, returns to normal state */ function unpause() public onlyOwner whenPaused { - paused = false; + paused_ = false; emit Unpaused(); } } From 86bbab3bb26ed30baf6dbb1bdb2c69da98290d52 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 03:44:22 +0000 Subject: [PATCH 11/30] Improve encapsulation on ownership --- contracts/drafts/TokenVesting.sol | 2 +- contracts/ownership/Claimable.sol | 19 +++++++++---- contracts/ownership/DelayedClaimable.sol | 32 +++++++++++++++------- contracts/ownership/Heritable.sol | 13 ++++----- contracts/ownership/Ownable.sol | 19 +++++++++---- contracts/ownership/Superuser.sol | 2 +- contracts/token/ERC20/ERC20Mintable.sol | 2 +- test/access/SignatureBouncer.test.js | 2 +- test/crowdsale/MintedCrowdsale.test.js | 2 +- test/ownership/Claimable.test.js | 6 ++-- test/ownership/DelayedClaimable.test.js | 20 +++++++------- test/ownership/Ownable.behavior.js | 6 ++-- test/ownership/Superuser.test.js | 4 +-- test/token/ERC20/ERC20Mintable.behavior.js | 2 +- 14 files changed, 78 insertions(+), 53 deletions(-) diff --git a/contracts/drafts/TokenVesting.sol b/contracts/drafts/TokenVesting.sol index cc90e24940b..08f2c64d988 100644 --- a/contracts/drafts/TokenVesting.sol +++ b/contracts/drafts/TokenVesting.sol @@ -142,7 +142,7 @@ contract TokenVesting is Ownable { revoked_[_token] = true; - _token.safeTransfer(owner, refund); + _token.safeTransfer(getOwner(), refund); emit Revoked(); } diff --git a/contracts/ownership/Claimable.sol b/contracts/ownership/Claimable.sol index 9027c11f52d..6576632af12 100644 --- a/contracts/ownership/Claimable.sol +++ b/contracts/ownership/Claimable.sol @@ -10,30 +10,37 @@ import "./Ownable.sol"; * This allows the new owner to accept the transfer. */ contract Claimable is Ownable { - address public pendingOwner; + address private pendingOwner_; /** * @dev Modifier throws if called by any account other than the pendingOwner. */ modifier onlyPendingOwner() { - require(msg.sender == pendingOwner); + require(msg.sender == pendingOwner_); _; } + /** + * @return the address of the pending owner. + */ + function getPendingOwner() public view returns(address) { + return pendingOwner_; + } + /** * @dev Allows the current owner to set the pendingOwner address. * @param newOwner The address to transfer ownership to. */ function transferOwnership(address newOwner) public onlyOwner { - pendingOwner = newOwner; + pendingOwner_ = newOwner; } /** * @dev Allows the pendingOwner address to finalize the transfer. */ function claimOwnership() public onlyPendingOwner { - emit OwnershipTransferred(owner, pendingOwner); - owner = pendingOwner; - pendingOwner = address(0); + emit OwnershipTransferred(getOwner(), pendingOwner_); + owner = pendingOwner_; + pendingOwner_ = address(0); } } diff --git a/contracts/ownership/DelayedClaimable.sol b/contracts/ownership/DelayedClaimable.sol index a9c0857f8e9..b965856490a 100644 --- a/contracts/ownership/DelayedClaimable.sol +++ b/contracts/ownership/DelayedClaimable.sol @@ -10,8 +10,22 @@ import "./Claimable.sol"; */ contract DelayedClaimable is Claimable { - uint256 public end; - uint256 public start; + uint256 private start_; + uint256 private end_; + + /** + * @return the start of the claimable period. + */ + function getStart() public view returns(uint256) { + return start_; + } + + /** + * @return the end of the claimable period. + */ + function getEnd() public view returns(uint256) { + return end_; + } /** * @dev Used to specify the time period during which a pending @@ -21,20 +35,18 @@ contract DelayedClaimable is Claimable { */ function setLimits(uint256 _start, uint256 _end) public onlyOwner { require(_start <= _end); - end = _end; - start = _start; + end_ = _end; + start_ = _start; } /** - * @dev Allows the pendingOwner address to finalize the transfer, as long as it is called within + * @dev Allows the pending owner address to finalize the transfer, as long as it is called within * the specified start and end time. */ function claimOwnership() public onlyPendingOwner { - require((block.number <= end) && (block.number >= start)); - emit OwnershipTransferred(owner, pendingOwner); - owner = pendingOwner; - pendingOwner = address(0); - end = 0; + require((block.number <= end_) && (block.number >= start_)); + super.claimOwnership(); + end_ = 0; } } diff --git a/contracts/ownership/Heritable.sol b/contracts/ownership/Heritable.sol index 88e6fee62f4..9e3214c071f 100644 --- a/contracts/ownership/Heritable.sol +++ b/contracts/ownership/Heritable.sol @@ -51,9 +51,9 @@ contract Heritable is Ownable { } function setHeir(address _newHeir) public onlyOwner { - require(_newHeir != owner); + require(_newHeir != getOwner()); heartbeat(); - emit HeirChanged(owner, _newHeir); + emit HeirChanged(getOwner(), _newHeir); heir_ = _newHeir; } @@ -87,7 +87,7 @@ contract Heritable is Ownable { */ function proclaimDeath() public onlyHeir { require(_ownerLives()); - emit OwnerProclaimedDead(owner, heir_, timeOfDeath_); + emit OwnerProclaimedDead(getOwner(), heir_, timeOfDeath_); // solium-disable-next-line security/no-block-members timeOfDeath_ = block.timestamp; } @@ -96,7 +96,7 @@ contract Heritable is Ownable { * @dev Owner can send a heartbeat if they were mistakenly pronounced dead. */ function heartbeat() public onlyOwner { - emit OwnerHeartbeated(owner); + emit OwnerHeartbeated(getOwner()); timeOfDeath_ = 0; } @@ -107,9 +107,8 @@ contract Heritable is Ownable { require(!_ownerLives()); // solium-disable-next-line security/no-block-members require(block.timestamp >= timeOfDeath_ + heartbeatTimeout_); - emit OwnershipTransferred(owner, heir_); - emit HeirOwnershipClaimed(owner, heir_); - owner = heir_; + emit HeirOwnershipClaimed(getOwner(), heir_); + transferOwnership(heir_); timeOfDeath_ = 0; } diff --git a/contracts/ownership/Ownable.sol b/contracts/ownership/Ownable.sol index 1fbf571bf7c..a4d42ab0501 100644 --- a/contracts/ownership/Ownable.sol +++ b/contracts/ownership/Ownable.sol @@ -7,7 +7,7 @@ pragma solidity ^0.4.24; * functions, this simplifies the implementation of "user permissions". */ contract Ownable { - address public owner; + address private owner_; event OwnershipRenounced(address indexed previousOwner); @@ -22,14 +22,21 @@ contract Ownable { * account. */ constructor() public { - owner = msg.sender; + owner_ = msg.sender; + } + + /** + * @return the address of the owner. + */ + function getOwner() public view returns(address) { + return owner_; } /** * @dev Throws if called by any account other than the owner. */ modifier onlyOwner() { - require(msg.sender == owner); + require(msg.sender == owner_); _; } @@ -40,7 +47,7 @@ contract Ownable { * modifier anymore. */ function renounceOwnership() public onlyOwner { - emit OwnershipRenounced(owner); + emit OwnershipRenounced(owner_); owner = address(0); } @@ -58,7 +65,7 @@ contract Ownable { */ function _transferOwnership(address _newOwner) internal { require(_newOwner != address(0)); - emit OwnershipTransferred(owner, _newOwner); - owner = _newOwner; + emit OwnershipTransferred(owner_, _newOwner); + owner_ = _newOwner; } } diff --git a/contracts/ownership/Superuser.sol b/contracts/ownership/Superuser.sol index d8d081d03c3..c0fddf3ab5c 100644 --- a/contracts/ownership/Superuser.sol +++ b/contracts/ownership/Superuser.sol @@ -27,7 +27,7 @@ contract Superuser is Ownable, RBAC { } modifier onlyOwnerOrSuperuser() { - require(msg.sender == owner || isSuperuser(msg.sender)); + require(msg.sender == getOwner() || isSuperuser(msg.sender)); _; } diff --git a/contracts/token/ERC20/ERC20Mintable.sol b/contracts/token/ERC20/ERC20Mintable.sol index e680e6b6055..93f0d28e977 100644 --- a/contracts/token/ERC20/ERC20Mintable.sol +++ b/contracts/token/ERC20/ERC20Mintable.sol @@ -22,7 +22,7 @@ contract ERC20Mintable is ERC20, Ownable { } modifier hasMintPermission() { - require(msg.sender == owner); + require(msg.sender == getOwner()); _; } diff --git a/test/access/SignatureBouncer.test.js b/test/access/SignatureBouncer.test.js index 798ab0ac9fa..19143c850b0 100644 --- a/test/access/SignatureBouncer.test.js +++ b/test/access/SignatureBouncer.test.js @@ -20,7 +20,7 @@ contract('Bouncer', function ([_, owner, anyone, bouncerAddress, authorizedUser] context('management', function () { it('has a default owner of self', async function () { - (await this.bouncer.owner()).should.equal(owner); + (await this.bouncer.getOwner()).should.equal(owner); }); it('allows the owner to add a bouncer', async function () { diff --git a/test/crowdsale/MintedCrowdsale.test.js b/test/crowdsale/MintedCrowdsale.test.js index 50a37eaef72..9f3c8204229 100644 --- a/test/crowdsale/MintedCrowdsale.test.js +++ b/test/crowdsale/MintedCrowdsale.test.js @@ -21,7 +21,7 @@ contract('MintedCrowdsale', function ([_, investor, wallet, purchaser]) { }); it('should be token owner', async function () { - (await this.token.owner()).should.equal(this.crowdsale.address); + (await this.token.getOwner()).should.equal(this.crowdsale.address); }); shouldBehaveLikeMintedCrowdsale([_, investor, wallet, purchaser], rate, value); diff --git a/test/ownership/Claimable.test.js b/test/ownership/Claimable.test.js index bfc1355d5f7..40659532676 100644 --- a/test/ownership/Claimable.test.js +++ b/test/ownership/Claimable.test.js @@ -16,12 +16,12 @@ contract('Claimable', function ([_, owner, newOwner, anyone]) { }); it('should have an owner', async function () { - (await claimable.owner()).should.not.equal(0); + (await claimable.getOwner()).should.not.equal(0); }); it('changes pendingOwner after transfer', async function () { await claimable.transferOwnership(newOwner, { from: owner }); - (await claimable.pendingOwner()).should.equal(newOwner); + (await claimable.getPendingOwner()).should.equal(newOwner); }); it('should prevent to claimOwnership from anyone', async function () { @@ -40,7 +40,7 @@ contract('Claimable', function ([_, owner, newOwner, anyone]) { it('changes allow pending owner to claim ownership', async function () { await claimable.claimOwnership({ from: newOwner }); - (await claimable.owner()).should.equal(newOwner); + (await claimable.getOwner()).should.equal(newOwner); }); }); }); diff --git a/test/ownership/DelayedClaimable.test.js b/test/ownership/DelayedClaimable.test.js index e65ac86c41d..039f1ae672c 100644 --- a/test/ownership/DelayedClaimable.test.js +++ b/test/ownership/DelayedClaimable.test.js @@ -17,35 +17,35 @@ contract('DelayedClaimable', function ([_, owner, newOwner]) { await this.delayedClaimable.transferOwnership(newOwner, { from: owner }); await this.delayedClaimable.setLimits(0, 1000, { from: owner }); - (await this.delayedClaimable.end()).should.be.bignumber.equal(1000); + (await this.delayedClaimable.getEnd()).should.be.bignumber.equal(1000); - (await this.delayedClaimable.start()).should.be.bignumber.equal(0); + (await this.delayedClaimable.getStart()).should.be.bignumber.equal(0); }); it('changes pendingOwner after transfer successful', async function () { await this.delayedClaimable.transferOwnership(newOwner, { from: owner }); await this.delayedClaimable.setLimits(0, 1000, { from: owner }); - (await this.delayedClaimable.end()).should.be.bignumber.equal(1000); + (await this.delayedClaimable.getEnd()).should.be.bignumber.equal(1000); - (await this.delayedClaimable.start()).should.be.bignumber.equal(0); + (await this.delayedClaimable.getStart()).should.be.bignumber.equal(0); - (await this.delayedClaimable.pendingOwner()).should.equal(newOwner); + (await this.delayedClaimable.getPendingOwner()).should.equal(newOwner); await this.delayedClaimable.claimOwnership({ from: newOwner }); - (await this.delayedClaimable.owner()).should.equal(newOwner); + (await this.delayedClaimable.getOwner()).should.equal(newOwner); }); it('changes pendingOwner after transfer fails', async function () { await this.delayedClaimable.transferOwnership(newOwner, { from: owner }); await this.delayedClaimable.setLimits(100, 110, { from: owner }); - (await this.delayedClaimable.end()).should.be.bignumber.equal(110); + (await this.delayedClaimable.getEnd()).should.be.bignumber.equal(110); - (await this.delayedClaimable.start()).should.be.bignumber.equal(100); + (await this.delayedClaimable.getStart()).should.be.bignumber.equal(100); - (await this.delayedClaimable.pendingOwner()).should.equal(newOwner); + (await this.delayedClaimable.getPendingOwner()).should.equal(newOwner); await assertRevert(this.delayedClaimable.claimOwnership({ from: newOwner })); - (await this.delayedClaimable.owner()).should.not.equal(newOwner); + (await this.delayedClaimable.getOwner()).should.not.equal(newOwner); }); it('set end and start invalid values fail', async function () { diff --git a/test/ownership/Ownable.behavior.js b/test/ownership/Ownable.behavior.js index 6257c01df5a..ebb550b0f25 100644 --- a/test/ownership/Ownable.behavior.js +++ b/test/ownership/Ownable.behavior.js @@ -9,12 +9,12 @@ require('chai') function shouldBehaveLikeOwnable (owner, [anyone]) { describe('as an ownable', function () { it('should have an owner', async function () { - (await this.ownable.owner()).should.equal(owner); + (await this.ownable.getOwner()).should.equal(owner); }); it('changes owner after transfer', async function () { await this.ownable.transferOwnership(anyone, { from: owner }); - (await this.ownable.owner()).should.equal(anyone); + (await this.ownable.getOwner()).should.equal(anyone); }); it('should prevent non-owners from transfering', async function () { @@ -27,7 +27,7 @@ function shouldBehaveLikeOwnable (owner, [anyone]) { it('loses owner after renouncement', async function () { await this.ownable.renounceOwnership({ from: owner }); - (await this.ownable.owner()).should.equal(ZERO_ADDRESS); + (await this.ownable.getOwner()).should.equal(ZERO_ADDRESS); }); it('should prevent non-owners from renouncement', async function () { diff --git a/test/ownership/Superuser.test.js b/test/ownership/Superuser.test.js index 5e4d2c32456..3b4e6ede62d 100644 --- a/test/ownership/Superuser.test.js +++ b/test/ownership/Superuser.test.js @@ -40,7 +40,7 @@ contract('Superuser', function ([_, firstOwner, newSuperuser, newOwner, anyone]) 'OwnershipTransferred' ); - (await this.superuser.owner()).should.equal(newOwner); + (await this.superuser.getOwner()).should.equal(newOwner); }); it('should change owner after the owner transfers the ownership', async function () { @@ -49,7 +49,7 @@ contract('Superuser', function ([_, firstOwner, newSuperuser, newOwner, anyone]) 'OwnershipTransferred' ); - (await this.superuser.owner()).should.equal(newOwner); + (await this.superuser.getOwner()).should.equal(newOwner); }); }); diff --git a/test/token/ERC20/ERC20Mintable.behavior.js b/test/token/ERC20/ERC20Mintable.behavior.js index 63fb861b551..cb982ed26f1 100644 --- a/test/token/ERC20/ERC20Mintable.behavior.js +++ b/test/token/ERC20/ERC20Mintable.behavior.js @@ -13,7 +13,7 @@ function shouldBehaveLikeERC20Mintable (owner, minter, [anyone]) { describe('as a basic mintable token', function () { describe('after token creation', function () { it('sender should be token owner', async function () { - (await this.token.owner({ from: owner })).should.equal(owner); + (await this.token.getOwner({ from: owner })).should.equal(owner); }); }); From faed52de1fc8c3e152b0b9089495f5ae1674e459 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 03:50:07 +0000 Subject: [PATCH 12/30] fix rebase --- contracts/bounties/BreakInvariantBounty.sol | 2 +- contracts/ownership/Ownable.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/bounties/BreakInvariantBounty.sol b/contracts/bounties/BreakInvariantBounty.sol index c889fbe606f..1f330ffd378 100644 --- a/contracts/bounties/BreakInvariantBounty.sol +++ b/contracts/bounties/BreakInvariantBounty.sol @@ -50,7 +50,7 @@ contract BreakInvariantBounty is PullPayment, Ownable { * @dev Transfers the current balance to the owner and terminates the contract. */ function destroy() public onlyOwner { - selfdestruct(owner); + selfdestruct(getOwner()); } /** diff --git a/contracts/ownership/Ownable.sol b/contracts/ownership/Ownable.sol index a4d42ab0501..9a7d60d8978 100644 --- a/contracts/ownership/Ownable.sol +++ b/contracts/ownership/Ownable.sol @@ -48,7 +48,7 @@ contract Ownable { */ function renounceOwnership() public onlyOwner { emit OwnershipRenounced(owner_); - owner = address(0); + owner_ = address(0); } /** From 21ae1773101d8df91cc7124a0e748624bef5004c Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 03:55:20 +0000 Subject: [PATCH 13/30] fix ownership --- contracts/ownership/CanReclaimToken.sol | 2 +- contracts/ownership/Claimable.sol | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/contracts/ownership/CanReclaimToken.sol b/contracts/ownership/CanReclaimToken.sol index 858a1db84e8..11452cdeb9b 100644 --- a/contracts/ownership/CanReclaimToken.sol +++ b/contracts/ownership/CanReclaimToken.sol @@ -20,7 +20,7 @@ contract CanReclaimToken is Ownable { */ function reclaimToken(IERC20 _token) external onlyOwner { uint256 balance = _token.balanceOf(this); - _token.safeTransfer(owner, balance); + _token.safeTransfer(getOwner(), balance); } } diff --git a/contracts/ownership/Claimable.sol b/contracts/ownership/Claimable.sol index 6576632af12..fd7c2ffbb39 100644 --- a/contracts/ownership/Claimable.sol +++ b/contracts/ownership/Claimable.sol @@ -39,8 +39,6 @@ contract Claimable is Ownable { * @dev Allows the pendingOwner address to finalize the transfer. */ function claimOwnership() public onlyPendingOwner { - emit OwnershipTransferred(getOwner(), pendingOwner_); - owner = pendingOwner_; - pendingOwner_ = address(0); + _transferOwnership(pendingOwner_); } } From 917a0197586e9bb6d3c0b628b22659a1a261ca20 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 04:23:33 +0000 Subject: [PATCH 14/30] Improve encapsulation on payments --- contracts/ownership/Heritable.sol | 2 +- contracts/payment/RefundEscrow.sol | 38 +++++++++++------ contracts/payment/SplitPayment.sol | 67 +++++++++++++++++++++++------- test/payment/RefundEscrow.test.js | 4 ++ test/payment/SplitPayment.test.js | 6 +-- 5 files changed, 85 insertions(+), 32 deletions(-) diff --git a/contracts/ownership/Heritable.sol b/contracts/ownership/Heritable.sol index 9e3214c071f..fc3d561fcad 100644 --- a/contracts/ownership/Heritable.sol +++ b/contracts/ownership/Heritable.sol @@ -108,7 +108,7 @@ contract Heritable is Ownable { // solium-disable-next-line security/no-block-members require(block.timestamp >= timeOfDeath_ + heartbeatTimeout_); emit HeirOwnershipClaimed(getOwner(), heir_); - transferOwnership(heir_); + _transferOwnership(heir_); timeOfDeath_ = 0; } diff --git a/contracts/payment/RefundEscrow.sol b/contracts/payment/RefundEscrow.sol index 9ba5fc60032..d9c11db5395 100644 --- a/contracts/payment/RefundEscrow.sol +++ b/contracts/payment/RefundEscrow.sol @@ -16,8 +16,8 @@ contract RefundEscrow is Ownable, ConditionalEscrow { event Closed(); event RefundsEnabled(); - State public state; - address public beneficiary; + State private state_; + address private beneficiary_; /** * @dev Constructor. @@ -25,8 +25,22 @@ contract RefundEscrow is Ownable, ConditionalEscrow { */ constructor(address _beneficiary) public { require(_beneficiary != address(0)); - beneficiary = _beneficiary; - state = State.Active; + beneficiary_ = _beneficiary; + state_ = State.Active; + } + + /** + * @return the current state of the escrow. + */ + function getState() public view returns(State) { + return state_; + } + + /** + * @return the beneficiary of the escrow. + */ + function getBeneficiary() public view returns(address) { + return beneficiary_; } /** @@ -34,7 +48,7 @@ contract RefundEscrow is Ownable, ConditionalEscrow { * @param _refundee The address funds will be sent to if a refund occurs. */ function deposit(address _refundee) public payable { - require(state == State.Active); + require(state_ == State.Active); super.deposit(_refundee); } @@ -43,8 +57,8 @@ contract RefundEscrow is Ownable, ConditionalEscrow { * further deposits. */ function close() public onlyOwner { - require(state == State.Active); - state = State.Closed; + require(state_ == State.Active); + state_ = State.Closed; emit Closed(); } @@ -52,8 +66,8 @@ contract RefundEscrow is Ownable, ConditionalEscrow { * @dev Allows for refunds to take place, rejecting further deposits. */ function enableRefunds() public onlyOwner { - require(state == State.Active); - state = State.Refunding; + require(state_ == State.Active); + state_ = State.Refunding; emit RefundsEnabled(); } @@ -61,14 +75,14 @@ contract RefundEscrow is Ownable, ConditionalEscrow { * @dev Withdraws the beneficiary's funds. */ function beneficiaryWithdraw() public { - require(state == State.Closed); - beneficiary.transfer(address(this).balance); + require(state_ == State.Closed); + beneficiary_.transfer(address(this).balance); } /** * @dev Returns whether refundees can withdraw their deposits (be refunded). */ function withdrawalAllowed(address _payee) public view returns (bool) { - return state == State.Refunding; + return state_ == State.Refunding; } } diff --git a/contracts/payment/SplitPayment.sol b/contracts/payment/SplitPayment.sol index 04b01425683..8b1c53017de 100644 --- a/contracts/payment/SplitPayment.sol +++ b/contracts/payment/SplitPayment.sol @@ -11,12 +11,12 @@ import "../math/SafeMath.sol"; contract SplitPayment { using SafeMath for uint256; - uint256 public totalShares = 0; - uint256 public totalReleased = 0; + uint256 private totalShares_ = 0; + uint256 private totalReleased_ = 0; - mapping(address => uint256) public shares; - mapping(address => uint256) public released; - address[] public payees; + mapping(address => uint256) private shares_; + mapping(address => uint256) private released_; + address[] private payees_; /** * @dev Constructor @@ -35,26 +35,61 @@ contract SplitPayment { */ function () external payable {} + /** + * @return the total shares of the contract. + */ + function getTotalShares() public view returns(uint256) { + return totalShares_; + } + + /** + * @return the total amount already released. + */ + function getTotalReleased() public view returns(uint256) { + return totalReleased_; + } + + /** + * @return the shares of an account. + */ + function getShares(address _account) public view returns(uint256) { + return shares_[_account]; + } + + /** + * @return the amount already released to an account. + */ + function getReleased(address _account) public view returns(uint256) { + return released_[_account]; + } + + /** + * @return the address of a payee. + */ + function getPayee(uint256 index) public view returns(address) { + return payees_[index]; + } + /** * @dev Claim your share of the balance. */ function claim() public { address payee = msg.sender; - require(shares[payee] > 0); + require(shares_[payee] > 0); - uint256 totalReceived = address(this).balance.add(totalReleased); + uint256 totalReceived = address(this).balance.add(totalReleased_); uint256 payment = totalReceived.mul( - shares[payee]).div( - totalShares).sub( - released[payee] + shares_[payee]).div( + totalShares_).sub( + released_[payee] ); require(payment != 0); assert(address(this).balance >= payment); - released[payee] = released[payee].add(payment); - totalReleased = totalReleased.add(payment); + released_[payee] = released_[payee].add(payment); + totalReleased_ = totalReleased_.add(payment); payee.transfer(payment); } @@ -67,10 +102,10 @@ contract SplitPayment { function _addPayee(address _payee, uint256 _shares) internal { require(_payee != address(0)); require(_shares > 0); - require(shares[_payee] == 0); + require(shares_[_payee] == 0); - payees.push(_payee); - shares[_payee] = _shares; - totalShares = totalShares.add(_shares); + payees_.push(_payee); + shares_[_payee] = _shares; + totalShares_ = totalShares_.add(_shares); } } diff --git a/test/payment/RefundEscrow.test.js b/test/payment/RefundEscrow.test.js index e74d68f674b..d96413e4148 100644 --- a/test/payment/RefundEscrow.test.js +++ b/test/payment/RefundEscrow.test.js @@ -28,6 +28,10 @@ contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2] }); context('active state', function () { + it('has beneficiary', async function () { + (await this.escrow.getBeneficiary()).should.be.equal(beneficiary); + }); + it('accepts deposits', async function () { await this.escrow.deposit(refundee1, { from: owner, value: amount }); diff --git a/test/payment/SplitPayment.test.js b/test/payment/SplitPayment.test.js index bb09ea73cac..c713ff48cc6 100644 --- a/test/payment/SplitPayment.test.js +++ b/test/payment/SplitPayment.test.js @@ -53,11 +53,11 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1, }); it('should store shares if address is payee', async function () { - (await this.contract.shares.call(payee1)).should.be.bignumber.not.equal(0); + (await this.contract.getShares(payee1)).should.be.bignumber.not.equal(0); }); it('should not store shares if address is not payee', async function () { - (await this.contract.shares.call(nonpayee1)).should.be.bignumber.equal(0); + (await this.contract.getShares(nonpayee1)).should.be.bignumber.equal(0); }); it('should throw if no funds to claim', async function () { @@ -96,7 +96,7 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1, (await ethGetBalance(this.contract.address)).should.be.bignumber.equal(0); // check correct funds released accounting - (await this.contract.totalReleased.call()).should.be.bignumber.equal(initBalance); + (await this.contract.getTotalReleased()).should.be.bignumber.equal(initBalance); }); }); }); From 478d9741f0be068e102e169e09a52ad2ffb07adc Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Tue, 4 Sep 2018 04:55:07 +0000 Subject: [PATCH 15/30] Add missing tests --- test/payment/SplitPayment.test.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/payment/SplitPayment.test.js b/test/payment/SplitPayment.test.js index c713ff48cc6..fcbf6c58af1 100644 --- a/test/payment/SplitPayment.test.js +++ b/test/payment/SplitPayment.test.js @@ -46,6 +46,17 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1, this.contract = await SplitPayment.new(this.payees, this.shares); }); + it('should have total shares', async function () { + (await this.contract.getTotalShares()).should.be.bignumber.equal(20 + 10 + 70); + }); + + it('should have payees', async function () { + this.payees.forEach(async (payee, index) => { + (await this.getPayee(index)).should.be.equal(payee); + (await this.contract.getReleased(payee)).should.be.bignumber.equal(0); + }); + }); + it('should accept payments', async function () { await ethSendTransaction({ from: owner, to: this.contract.address, value: amount }); From 0fecbacc2fb68513d40884cb7dec96d6941c8934 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Tue, 4 Sep 2018 18:39:30 +0000 Subject: [PATCH 16/30] add missing test --- test/payment/RefundEscrow.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/payment/RefundEscrow.test.js b/test/payment/RefundEscrow.test.js index d96413e4148..392d07603cb 100644 --- a/test/payment/RefundEscrow.test.js +++ b/test/payment/RefundEscrow.test.js @@ -28,8 +28,9 @@ contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2] }); context('active state', function () { - it('has beneficiary', async function () { + it('has beneficiary and state', async function () { (await this.escrow.getBeneficiary()).should.be.equal(beneficiary); + (await this.escrow.getState()).should.be.bignumber.equal(0); }); it('accepts deposits', async function () { From 9449572c3ec14ec7cbd679b54cf4e1ff3dd2030c Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Wed, 5 Sep 2018 02:23:44 +0000 Subject: [PATCH 17/30] Do not prefix getters --- contracts/bounties/BreakInvariantBounty.sol | 2 +- contracts/drafts/TokenVesting.sol | 2 +- contracts/lifecycle/Pausable.sol | 2 +- contracts/ownership/CanReclaimToken.sol | 2 +- contracts/ownership/Claimable.sol | 2 +- contracts/ownership/DelayedClaimable.sol | 4 ++-- contracts/ownership/Heritable.sol | 10 +++++----- contracts/ownership/Ownable.sol | 2 +- contracts/ownership/Superuser.sol | 2 +- contracts/payment/RefundEscrow.sol | 4 ++-- contracts/payment/SplitPayment.sol | 10 +++++----- contracts/token/ERC20/ERC20Mintable.sol | 2 +- test/access/SignatureBouncer.test.js | 2 +- test/crowdsale/MintedCrowdsale.test.js | 2 +- test/ownership/Claimable.test.js | 6 +++--- test/ownership/DelayedClaimable.test.js | 20 ++++++++++---------- test/ownership/Ownable.behavior.js | 6 +++--- test/ownership/Superuser.test.js | 4 ++-- test/payment/RefundEscrow.test.js | 4 ++-- test/payment/SplitPayment.test.js | 10 +++++----- test/token/ERC20/ERC20Pausable.test.js | 10 +++++----- 21 files changed, 54 insertions(+), 54 deletions(-) diff --git a/contracts/bounties/BreakInvariantBounty.sol b/contracts/bounties/BreakInvariantBounty.sol index 1f330ffd378..5301a2d51a4 100644 --- a/contracts/bounties/BreakInvariantBounty.sol +++ b/contracts/bounties/BreakInvariantBounty.sol @@ -50,7 +50,7 @@ contract BreakInvariantBounty is PullPayment, Ownable { * @dev Transfers the current balance to the owner and terminates the contract. */ function destroy() public onlyOwner { - selfdestruct(getOwner()); + selfdestruct(owner()); } /** diff --git a/contracts/drafts/TokenVesting.sol b/contracts/drafts/TokenVesting.sol index 08f2c64d988..cfe7697d093 100644 --- a/contracts/drafts/TokenVesting.sol +++ b/contracts/drafts/TokenVesting.sol @@ -142,7 +142,7 @@ contract TokenVesting is Ownable { revoked_[_token] = true; - _token.safeTransfer(getOwner(), refund); + _token.safeTransfer(owner(), refund); emit Revoked(); } diff --git a/contracts/lifecycle/Pausable.sol b/contracts/lifecycle/Pausable.sol index 3e5676f7f87..37caf1e0a52 100644 --- a/contracts/lifecycle/Pausable.sol +++ b/contracts/lifecycle/Pausable.sol @@ -18,7 +18,7 @@ contract Pausable is Ownable { /** * @return true if the contract is paused, false otherwise. */ - function isPaused() public view returns(bool) { + function paused() public view returns(bool) { return paused_; } diff --git a/contracts/ownership/CanReclaimToken.sol b/contracts/ownership/CanReclaimToken.sol index 11452cdeb9b..e4f7e88a638 100644 --- a/contracts/ownership/CanReclaimToken.sol +++ b/contracts/ownership/CanReclaimToken.sol @@ -20,7 +20,7 @@ contract CanReclaimToken is Ownable { */ function reclaimToken(IERC20 _token) external onlyOwner { uint256 balance = _token.balanceOf(this); - _token.safeTransfer(getOwner(), balance); + _token.safeTransfer(owner(), balance); } } diff --git a/contracts/ownership/Claimable.sol b/contracts/ownership/Claimable.sol index fd7c2ffbb39..7c19d288076 100644 --- a/contracts/ownership/Claimable.sol +++ b/contracts/ownership/Claimable.sol @@ -23,7 +23,7 @@ contract Claimable is Ownable { /** * @return the address of the pending owner. */ - function getPendingOwner() public view returns(address) { + function pendingOwner() public view returns(address) { return pendingOwner_; } diff --git a/contracts/ownership/DelayedClaimable.sol b/contracts/ownership/DelayedClaimable.sol index b965856490a..b373e0067ea 100644 --- a/contracts/ownership/DelayedClaimable.sol +++ b/contracts/ownership/DelayedClaimable.sol @@ -16,14 +16,14 @@ contract DelayedClaimable is Claimable { /** * @return the start of the claimable period. */ - function getStart() public view returns(uint256) { + function start() public view returns(uint256) { return start_; } /** * @return the end of the claimable period. */ - function getEnd() public view returns(uint256) { + function end() public view returns(uint256) { return end_; } diff --git a/contracts/ownership/Heritable.sol b/contracts/ownership/Heritable.sol index fc3d561fcad..d015468f5ca 100644 --- a/contracts/ownership/Heritable.sol +++ b/contracts/ownership/Heritable.sol @@ -51,9 +51,9 @@ contract Heritable is Ownable { } function setHeir(address _newHeir) public onlyOwner { - require(_newHeir != getOwner()); + require(_newHeir != owner()); heartbeat(); - emit HeirChanged(getOwner(), _newHeir); + emit HeirChanged(owner(), _newHeir); heir_ = _newHeir; } @@ -87,7 +87,7 @@ contract Heritable is Ownable { */ function proclaimDeath() public onlyHeir { require(_ownerLives()); - emit OwnerProclaimedDead(getOwner(), heir_, timeOfDeath_); + emit OwnerProclaimedDead(owner(), heir_, timeOfDeath_); // solium-disable-next-line security/no-block-members timeOfDeath_ = block.timestamp; } @@ -96,7 +96,7 @@ contract Heritable is Ownable { * @dev Owner can send a heartbeat if they were mistakenly pronounced dead. */ function heartbeat() public onlyOwner { - emit OwnerHeartbeated(getOwner()); + emit OwnerHeartbeated(owner()); timeOfDeath_ = 0; } @@ -107,7 +107,7 @@ contract Heritable is Ownable { require(!_ownerLives()); // solium-disable-next-line security/no-block-members require(block.timestamp >= timeOfDeath_ + heartbeatTimeout_); - emit HeirOwnershipClaimed(getOwner(), heir_); + emit HeirOwnershipClaimed(owner(), heir_); _transferOwnership(heir_); timeOfDeath_ = 0; } diff --git a/contracts/ownership/Ownable.sol b/contracts/ownership/Ownable.sol index 9a7d60d8978..f89013a1a53 100644 --- a/contracts/ownership/Ownable.sol +++ b/contracts/ownership/Ownable.sol @@ -28,7 +28,7 @@ contract Ownable { /** * @return the address of the owner. */ - function getOwner() public view returns(address) { + function owner() public view returns(address) { return owner_; } diff --git a/contracts/ownership/Superuser.sol b/contracts/ownership/Superuser.sol index c0fddf3ab5c..b1f1fa4b52d 100644 --- a/contracts/ownership/Superuser.sol +++ b/contracts/ownership/Superuser.sol @@ -27,7 +27,7 @@ contract Superuser is Ownable, RBAC { } modifier onlyOwnerOrSuperuser() { - require(msg.sender == getOwner() || isSuperuser(msg.sender)); + require(msg.sender == owner() || isSuperuser(msg.sender)); _; } diff --git a/contracts/payment/RefundEscrow.sol b/contracts/payment/RefundEscrow.sol index d9c11db5395..802dd471128 100644 --- a/contracts/payment/RefundEscrow.sol +++ b/contracts/payment/RefundEscrow.sol @@ -32,14 +32,14 @@ contract RefundEscrow is Ownable, ConditionalEscrow { /** * @return the current state of the escrow. */ - function getState() public view returns(State) { + function state() public view returns(State) { return state_; } /** * @return the beneficiary of the escrow. */ - function getBeneficiary() public view returns(address) { + function beneficiary() public view returns(address) { return beneficiary_; } diff --git a/contracts/payment/SplitPayment.sol b/contracts/payment/SplitPayment.sol index 8b1c53017de..1655b738383 100644 --- a/contracts/payment/SplitPayment.sol +++ b/contracts/payment/SplitPayment.sol @@ -38,35 +38,35 @@ contract SplitPayment { /** * @return the total shares of the contract. */ - function getTotalShares() public view returns(uint256) { + function totalShares() public view returns(uint256) { return totalShares_; } /** * @return the total amount already released. */ - function getTotalReleased() public view returns(uint256) { + function totalReleased() public view returns(uint256) { return totalReleased_; } /** * @return the shares of an account. */ - function getShares(address _account) public view returns(uint256) { + function shares(address _account) public view returns(uint256) { return shares_[_account]; } /** * @return the amount already released to an account. */ - function getReleased(address _account) public view returns(uint256) { + function released(address _account) public view returns(uint256) { return released_[_account]; } /** * @return the address of a payee. */ - function getPayee(uint256 index) public view returns(address) { + function payee(uint256 index) public view returns(address) { return payees_[index]; } diff --git a/contracts/token/ERC20/ERC20Mintable.sol b/contracts/token/ERC20/ERC20Mintable.sol index 93f0d28e977..518936699e4 100644 --- a/contracts/token/ERC20/ERC20Mintable.sol +++ b/contracts/token/ERC20/ERC20Mintable.sol @@ -22,7 +22,7 @@ contract ERC20Mintable is ERC20, Ownable { } modifier hasMintPermission() { - require(msg.sender == getOwner()); + require(msg.sender == owner()); _; } diff --git a/test/access/SignatureBouncer.test.js b/test/access/SignatureBouncer.test.js index 19143c850b0..798ab0ac9fa 100644 --- a/test/access/SignatureBouncer.test.js +++ b/test/access/SignatureBouncer.test.js @@ -20,7 +20,7 @@ contract('Bouncer', function ([_, owner, anyone, bouncerAddress, authorizedUser] context('management', function () { it('has a default owner of self', async function () { - (await this.bouncer.getOwner()).should.equal(owner); + (await this.bouncer.owner()).should.equal(owner); }); it('allows the owner to add a bouncer', async function () { diff --git a/test/crowdsale/MintedCrowdsale.test.js b/test/crowdsale/MintedCrowdsale.test.js index 9f3c8204229..50a37eaef72 100644 --- a/test/crowdsale/MintedCrowdsale.test.js +++ b/test/crowdsale/MintedCrowdsale.test.js @@ -21,7 +21,7 @@ contract('MintedCrowdsale', function ([_, investor, wallet, purchaser]) { }); it('should be token owner', async function () { - (await this.token.getOwner()).should.equal(this.crowdsale.address); + (await this.token.owner()).should.equal(this.crowdsale.address); }); shouldBehaveLikeMintedCrowdsale([_, investor, wallet, purchaser], rate, value); diff --git a/test/ownership/Claimable.test.js b/test/ownership/Claimable.test.js index 40659532676..bfc1355d5f7 100644 --- a/test/ownership/Claimable.test.js +++ b/test/ownership/Claimable.test.js @@ -16,12 +16,12 @@ contract('Claimable', function ([_, owner, newOwner, anyone]) { }); it('should have an owner', async function () { - (await claimable.getOwner()).should.not.equal(0); + (await claimable.owner()).should.not.equal(0); }); it('changes pendingOwner after transfer', async function () { await claimable.transferOwnership(newOwner, { from: owner }); - (await claimable.getPendingOwner()).should.equal(newOwner); + (await claimable.pendingOwner()).should.equal(newOwner); }); it('should prevent to claimOwnership from anyone', async function () { @@ -40,7 +40,7 @@ contract('Claimable', function ([_, owner, newOwner, anyone]) { it('changes allow pending owner to claim ownership', async function () { await claimable.claimOwnership({ from: newOwner }); - (await claimable.getOwner()).should.equal(newOwner); + (await claimable.owner()).should.equal(newOwner); }); }); }); diff --git a/test/ownership/DelayedClaimable.test.js b/test/ownership/DelayedClaimable.test.js index 039f1ae672c..e65ac86c41d 100644 --- a/test/ownership/DelayedClaimable.test.js +++ b/test/ownership/DelayedClaimable.test.js @@ -17,35 +17,35 @@ contract('DelayedClaimable', function ([_, owner, newOwner]) { await this.delayedClaimable.transferOwnership(newOwner, { from: owner }); await this.delayedClaimable.setLimits(0, 1000, { from: owner }); - (await this.delayedClaimable.getEnd()).should.be.bignumber.equal(1000); + (await this.delayedClaimable.end()).should.be.bignumber.equal(1000); - (await this.delayedClaimable.getStart()).should.be.bignumber.equal(0); + (await this.delayedClaimable.start()).should.be.bignumber.equal(0); }); it('changes pendingOwner after transfer successful', async function () { await this.delayedClaimable.transferOwnership(newOwner, { from: owner }); await this.delayedClaimable.setLimits(0, 1000, { from: owner }); - (await this.delayedClaimable.getEnd()).should.be.bignumber.equal(1000); + (await this.delayedClaimable.end()).should.be.bignumber.equal(1000); - (await this.delayedClaimable.getStart()).should.be.bignumber.equal(0); + (await this.delayedClaimable.start()).should.be.bignumber.equal(0); - (await this.delayedClaimable.getPendingOwner()).should.equal(newOwner); + (await this.delayedClaimable.pendingOwner()).should.equal(newOwner); await this.delayedClaimable.claimOwnership({ from: newOwner }); - (await this.delayedClaimable.getOwner()).should.equal(newOwner); + (await this.delayedClaimable.owner()).should.equal(newOwner); }); it('changes pendingOwner after transfer fails', async function () { await this.delayedClaimable.transferOwnership(newOwner, { from: owner }); await this.delayedClaimable.setLimits(100, 110, { from: owner }); - (await this.delayedClaimable.getEnd()).should.be.bignumber.equal(110); + (await this.delayedClaimable.end()).should.be.bignumber.equal(110); - (await this.delayedClaimable.getStart()).should.be.bignumber.equal(100); + (await this.delayedClaimable.start()).should.be.bignumber.equal(100); - (await this.delayedClaimable.getPendingOwner()).should.equal(newOwner); + (await this.delayedClaimable.pendingOwner()).should.equal(newOwner); await assertRevert(this.delayedClaimable.claimOwnership({ from: newOwner })); - (await this.delayedClaimable.getOwner()).should.not.equal(newOwner); + (await this.delayedClaimable.owner()).should.not.equal(newOwner); }); it('set end and start invalid values fail', async function () { diff --git a/test/ownership/Ownable.behavior.js b/test/ownership/Ownable.behavior.js index ebb550b0f25..6257c01df5a 100644 --- a/test/ownership/Ownable.behavior.js +++ b/test/ownership/Ownable.behavior.js @@ -9,12 +9,12 @@ require('chai') function shouldBehaveLikeOwnable (owner, [anyone]) { describe('as an ownable', function () { it('should have an owner', async function () { - (await this.ownable.getOwner()).should.equal(owner); + (await this.ownable.owner()).should.equal(owner); }); it('changes owner after transfer', async function () { await this.ownable.transferOwnership(anyone, { from: owner }); - (await this.ownable.getOwner()).should.equal(anyone); + (await this.ownable.owner()).should.equal(anyone); }); it('should prevent non-owners from transfering', async function () { @@ -27,7 +27,7 @@ function shouldBehaveLikeOwnable (owner, [anyone]) { it('loses owner after renouncement', async function () { await this.ownable.renounceOwnership({ from: owner }); - (await this.ownable.getOwner()).should.equal(ZERO_ADDRESS); + (await this.ownable.owner()).should.equal(ZERO_ADDRESS); }); it('should prevent non-owners from renouncement', async function () { diff --git a/test/ownership/Superuser.test.js b/test/ownership/Superuser.test.js index 3b4e6ede62d..5e4d2c32456 100644 --- a/test/ownership/Superuser.test.js +++ b/test/ownership/Superuser.test.js @@ -40,7 +40,7 @@ contract('Superuser', function ([_, firstOwner, newSuperuser, newOwner, anyone]) 'OwnershipTransferred' ); - (await this.superuser.getOwner()).should.equal(newOwner); + (await this.superuser.owner()).should.equal(newOwner); }); it('should change owner after the owner transfers the ownership', async function () { @@ -49,7 +49,7 @@ contract('Superuser', function ([_, firstOwner, newSuperuser, newOwner, anyone]) 'OwnershipTransferred' ); - (await this.superuser.getOwner()).should.equal(newOwner); + (await this.superuser.owner()).should.equal(newOwner); }); }); diff --git a/test/payment/RefundEscrow.test.js b/test/payment/RefundEscrow.test.js index 392d07603cb..7730d8242a0 100644 --- a/test/payment/RefundEscrow.test.js +++ b/test/payment/RefundEscrow.test.js @@ -29,8 +29,8 @@ contract('RefundEscrow', function ([_, owner, beneficiary, refundee1, refundee2] context('active state', function () { it('has beneficiary and state', async function () { - (await this.escrow.getBeneficiary()).should.be.equal(beneficiary); - (await this.escrow.getState()).should.be.bignumber.equal(0); + (await this.escrow.beneficiary()).should.be.equal(beneficiary); + (await this.escrow.state()).should.be.bignumber.equal(0); }); it('accepts deposits', async function () { diff --git a/test/payment/SplitPayment.test.js b/test/payment/SplitPayment.test.js index fcbf6c58af1..f1f08601e3d 100644 --- a/test/payment/SplitPayment.test.js +++ b/test/payment/SplitPayment.test.js @@ -47,13 +47,13 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1, }); it('should have total shares', async function () { - (await this.contract.getTotalShares()).should.be.bignumber.equal(20 + 10 + 70); + (await this.contract.totalShares()).should.be.bignumber.equal(20 + 10 + 70); }); it('should have payees', async function () { this.payees.forEach(async (payee, index) => { - (await this.getPayee(index)).should.be.equal(payee); - (await this.contract.getReleased(payee)).should.be.bignumber.equal(0); + (await this.payee(index)).should.be.equal(payee); + (await this.contract.released(payee)).should.be.bignumber.equal(0); }); }); @@ -64,11 +64,11 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1, }); it('should store shares if address is payee', async function () { - (await this.contract.getShares(payee1)).should.be.bignumber.not.equal(0); + (await this.contract.shares(payee1)).should.be.bignumber.not.equal(0); }); it('should not store shares if address is not payee', async function () { - (await this.contract.getShares(nonpayee1)).should.be.bignumber.equal(0); + (await this.contract.shares(nonpayee1)).should.be.bignumber.equal(0); }); it('should throw if no funds to claim', async function () { diff --git a/test/token/ERC20/ERC20Pausable.test.js b/test/token/ERC20/ERC20Pausable.test.js index b11ecb3e7b6..8fbe4f70876 100644 --- a/test/token/ERC20/ERC20Pausable.test.js +++ b/test/token/ERC20/ERC20Pausable.test.js @@ -13,7 +13,7 @@ contract('ERC20Pausable', function ([_, owner, recipient, anotherAccount]) { describe('when the token is unpaused', function () { it('pauses the token', async function () { await this.token.pause({ from }); - (await this.token.isPaused()).should.equal(true); + (await this.token.paused()).should.equal(true); }); it('emits a Pause event', async function () { @@ -55,7 +55,7 @@ contract('ERC20Pausable', function ([_, owner, recipient, anotherAccount]) { it('unpauses the token', async function () { await this.token.unpause({ from }); - (await this.token.isPaused()).should.equal(false); + (await this.token.paused()).should.equal(false); }); it('emits an Unpause event', async function () { @@ -87,18 +87,18 @@ contract('ERC20Pausable', function ([_, owner, recipient, anotherAccount]) { describe('paused', function () { it('is not paused by default', async function () { - (await this.token.isPaused({ from })).should.equal(false); + (await this.token.paused({ from })).should.equal(false); }); it('is paused after being paused', async function () { await this.token.pause({ from }); - (await this.token.isPaused({ from })).should.equal(true); + (await this.token.paused({ from })).should.equal(true); }); it('is not paused after being paused and then unpaused', async function () { await this.token.pause({ from }); await this.token.unpause({ from }); - (await this.token.isPaused()).should.equal(false); + (await this.token.paused()).should.equal(false); }); }); From 1856f07ff33e7918271d0b48bd0bca683075c25e Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Wed, 5 Sep 2018 03:02:17 +0000 Subject: [PATCH 18/30] Fix tests. --- test/payment/SplitPayment.test.js | 2 +- test/token/ERC20/ERC20Mintable.behavior.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/payment/SplitPayment.test.js b/test/payment/SplitPayment.test.js index f1f08601e3d..a284530996a 100644 --- a/test/payment/SplitPayment.test.js +++ b/test/payment/SplitPayment.test.js @@ -107,7 +107,7 @@ contract('SplitPayment', function ([_, owner, payee1, payee2, payee3, nonpayee1, (await ethGetBalance(this.contract.address)).should.be.bignumber.equal(0); // check correct funds released accounting - (await this.contract.getTotalReleased()).should.be.bignumber.equal(initBalance); + (await this.contract.totalReleased()).should.be.bignumber.equal(initBalance); }); }); }); diff --git a/test/token/ERC20/ERC20Mintable.behavior.js b/test/token/ERC20/ERC20Mintable.behavior.js index cb982ed26f1..63fb861b551 100644 --- a/test/token/ERC20/ERC20Mintable.behavior.js +++ b/test/token/ERC20/ERC20Mintable.behavior.js @@ -13,7 +13,7 @@ function shouldBehaveLikeERC20Mintable (owner, minter, [anyone]) { describe('as a basic mintable token', function () { describe('after token creation', function () { it('sender should be token owner', async function () { - (await this.token.getOwner({ from: owner })).should.equal(owner); + (await this.token.owner({ from: owner })).should.equal(owner); }); }); From f61acdcd51c554ba067c2455090419ea75720503 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 01:24:02 +0000 Subject: [PATCH 19/30] Improve encapsulation on Crowdsales --- contracts/crowdsale/Crowdsale.sol | 58 ++++++++++++++----- .../distribution/FinalizableCrowdsale.sol | 13 ++++- .../distribution/PostDeliveryCrowdsale.sol | 15 +++-- .../distribution/RefundableCrowdsale.sol | 17 ++++-- .../crowdsale/emission/AllowanceCrowdsale.sol | 15 +++-- .../crowdsale/emission/MintedCrowdsale.sol | 3 +- .../price/IncreasingPriceCrowdsale.sol | 26 +++++++-- .../crowdsale/validation/CappedCrowdsale.sol | 15 +++-- .../IndividuallyCappedCrowdsale.sol | 18 +++--- test/examples/SampleCrowdsale.test.js | 8 +-- 10 files changed, 134 insertions(+), 54 deletions(-) diff --git a/contracts/crowdsale/Crowdsale.sol b/contracts/crowdsale/Crowdsale.sol index 8bf055ada87..adfb59e95ea 100644 --- a/contracts/crowdsale/Crowdsale.sol +++ b/contracts/crowdsale/Crowdsale.sol @@ -22,19 +22,16 @@ contract Crowdsale { using SafeERC20 for IERC20; // The token being sold - IERC20 public token; + IERC20 private token_; // Address where funds are collected - address public wallet; + address private wallet_; // How many token units a buyer gets per wei. - // The rate is the conversion between wei and the smallest and indivisible token unit. - // So, if you are using a rate of 1 with a ERC20Detailed token with 3 decimals called TOK - // 1 wei will give you 1 unit, or 0.001 TOK. - uint256 public rate; + uint256 private rate_; // Amount of wei raised - uint256 public weiRaised; + uint256 private weiRaised_; /** * Event for token purchase logging @@ -52,6 +49,9 @@ contract Crowdsale { /** * @param _rate Number of token units a buyer gets per wei + * @dev The rate is the conversion between wei and the smallest and indivisible + * token unit. So, if you are using a rate of 1 with a ERC20Detailed token + * with 3 decimals called TOK, 1 wei will give you 1 unit, or 0.001 TOK. * @param _wallet Address where collected funds will be forwarded to * @param _token Address of the token being sold */ @@ -60,9 +60,9 @@ contract Crowdsale { require(_wallet != address(0)); require(_token != address(0)); - rate = _rate; - wallet = _wallet; - token = _token; + rate_ = _rate; + wallet_ = _wallet; + token_ = _token; } // ----------------------------------------- @@ -76,6 +76,34 @@ contract Crowdsale { buyTokens(msg.sender); } + /** + * @return the token being sold. + */ + function getToken() public view returns(IERC20) { + return token_; + } + + /** + * @return the address where funds are collected. + */ + function getWallet() public view returns(address) { + return wallet_; + } + + /** + * @return the number of token units a buyer gets per wei. + */ + function getRate() public view returns(uint256) { + return rate_; + } + + /** + * @return the mount of wei raised. + */ + function getWeiRaised() public view returns (uint256) { + return weiRaised_; + } + /** * @dev low level token purchase ***DO NOT OVERRIDE*** * @param _beneficiary Address performing the token purchase @@ -89,7 +117,7 @@ contract Crowdsale { uint256 tokens = _getTokenAmount(weiAmount); // update state - weiRaised = weiRaised.add(weiAmount); + weiRaised_ = weiRaised_.add(weiAmount); _processPurchase(_beneficiary, tokens); emit TokensPurchased( @@ -113,7 +141,7 @@ contract Crowdsale { * @dev Validation of an incoming purchase. Use require statements to revert state when conditions are not met. Use `super` in contracts that inherit from Crowdsale to extend their validations. * Example from CappedCrowdsale.sol's _preValidatePurchase method: * super._preValidatePurchase(_beneficiary, _weiAmount); - * require(weiRaised.add(_weiAmount) <= cap); + * require(getWeiRaised().add(_weiAmount) <= cap); * @param _beneficiary Address performing the token purchase * @param _weiAmount Value in wei involved in the purchase */ @@ -152,7 +180,7 @@ contract Crowdsale { ) internal { - token.safeTransfer(_beneficiary, _tokenAmount); + token_.safeTransfer(_beneficiary, _tokenAmount); } /** @@ -191,13 +219,13 @@ contract Crowdsale { function _getTokenAmount(uint256 _weiAmount) internal view returns (uint256) { - return _weiAmount.mul(rate); + return _weiAmount.mul(rate_); } /** * @dev Determines how ETH is stored/forwarded on purchases. */ function _forwardFunds() internal { - wallet.transfer(msg.value); + wallet_.transfer(msg.value); } } diff --git a/contracts/crowdsale/distribution/FinalizableCrowdsale.sol b/contracts/crowdsale/distribution/FinalizableCrowdsale.sol index 651069d41af..42d37b8ee15 100644 --- a/contracts/crowdsale/distribution/FinalizableCrowdsale.sol +++ b/contracts/crowdsale/distribution/FinalizableCrowdsale.sol @@ -13,22 +13,29 @@ import "../validation/TimedCrowdsale.sol"; contract FinalizableCrowdsale is Ownable, TimedCrowdsale { using SafeMath for uint256; - bool public isFinalized = false; + bool private finalized_ = false; event CrowdsaleFinalized(); + /** + * @return true if the crowdsale is finalized, false otherwise. + */ + function isFinalized() public view returns(bool) { + return finalized_; + } + /** * @dev Must be called after crowdsale ends, to do some extra finalization * work. Calls the contract's finalization function. */ function finalize() public onlyOwner { - require(!isFinalized); + require(!finalized_); require(hasClosed()); _finalization(); emit CrowdsaleFinalized(); - isFinalized = true; + finalized_ = true; } /** diff --git a/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol b/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol index b09d09709a5..6971669aedb 100644 --- a/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol +++ b/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol @@ -12,19 +12,26 @@ import "../../math/SafeMath.sol"; contract PostDeliveryCrowdsale is TimedCrowdsale { using SafeMath for uint256; - mapping(address => uint256) public balances; + mapping(address => uint256) private balances_; /** * @dev Withdraw tokens only after crowdsale ends. */ function withdrawTokens() public { require(hasClosed()); - uint256 amount = balances[msg.sender]; + uint256 amount = balances_[msg.sender]; require(amount > 0); - balances[msg.sender] = 0; + balances_[msg.sender] = 0; _deliverTokens(msg.sender, amount); } + /** + * @return the balance of an account. + */ + function getBalance(address _account) public view returns(uint256) { + return balances_[_account]; + } + /** * @dev Overrides parent by storing balances instead of issuing tokens right away. * @param _beneficiary Token purchaser @@ -36,7 +43,7 @@ contract PostDeliveryCrowdsale is TimedCrowdsale { ) internal { - balances[_beneficiary] = balances[_beneficiary].add(_tokenAmount); + balances_[_beneficiary] = balances_[_beneficiary].add(_tokenAmount); } } diff --git a/contracts/crowdsale/distribution/RefundableCrowdsale.sol b/contracts/crowdsale/distribution/RefundableCrowdsale.sol index 05f95ca8584..690afd2910d 100644 --- a/contracts/crowdsale/distribution/RefundableCrowdsale.sol +++ b/contracts/crowdsale/distribution/RefundableCrowdsale.sol @@ -15,7 +15,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale { using SafeMath for uint256; // minimum amount of funds to be raised in weis - uint256 public goal; + uint256 private goal_; // refund escrow used to hold funds while crowdsale is running RefundEscrow private escrow_; @@ -26,15 +26,22 @@ contract RefundableCrowdsale is FinalizableCrowdsale { */ constructor(uint256 _goal) public { require(_goal > 0); - escrow_ = new RefundEscrow(wallet); - goal = _goal; + escrow_ = new RefundEscrow(getWallet()); + goal_ = _goal; + } + + /** + * @return minimum amount of funds to be raised in wei. + */ + function getGoal() public view returns(uint256) { + return goal_; } /** * @dev Investors can claim refunds here if crowdsale is unsuccessful */ function claimRefund() public { - require(isFinalized); + require(isFinalized()); require(!goalReached()); escrow_.withdraw(msg.sender); @@ -45,7 +52,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale { * @return Whether funding goal was reached */ function goalReached() public view returns (bool) { - return weiRaised >= goal; + return getWeiRaised() >= goal_; } /** diff --git a/contracts/crowdsale/emission/AllowanceCrowdsale.sol b/contracts/crowdsale/emission/AllowanceCrowdsale.sol index 4e48638ab74..776933948ac 100644 --- a/contracts/crowdsale/emission/AllowanceCrowdsale.sol +++ b/contracts/crowdsale/emission/AllowanceCrowdsale.sol @@ -14,7 +14,7 @@ contract AllowanceCrowdsale is Crowdsale { using SafeMath for uint256; using SafeERC20 for IERC20; - address public tokenWallet; + address private tokenWallet_; /** * @dev Constructor, takes token wallet address. @@ -22,7 +22,14 @@ contract AllowanceCrowdsale is Crowdsale { */ constructor(address _tokenWallet) public { require(_tokenWallet != address(0)); - tokenWallet = _tokenWallet; + tokenWallet_ = _tokenWallet; + } + + /** + * @return the address of the wallet that will hold the tokens. + */ + function getTokenWallet() public view returns(address) { + return tokenWallet_; } /** @@ -30,7 +37,7 @@ contract AllowanceCrowdsale is Crowdsale { * @return Amount of tokens left in the allowance */ function remainingTokens() public view returns (uint256) { - return token.allowance(tokenWallet, this); + return getToken().allowance(tokenWallet_, this); } /** @@ -44,6 +51,6 @@ contract AllowanceCrowdsale is Crowdsale { ) internal { - token.safeTransferFrom(tokenWallet, _beneficiary, _tokenAmount); + getToken().safeTransferFrom(tokenWallet_, _beneficiary, _tokenAmount); } } diff --git a/contracts/crowdsale/emission/MintedCrowdsale.sol b/contracts/crowdsale/emission/MintedCrowdsale.sol index 9f3a8b4b115..5718fca1ede 100644 --- a/contracts/crowdsale/emission/MintedCrowdsale.sol +++ b/contracts/crowdsale/emission/MintedCrowdsale.sol @@ -23,6 +23,7 @@ contract MintedCrowdsale is Crowdsale { internal { // Potentially dangerous assumption about the type of the token. - require(ERC20Mintable(address(token)).mint(_beneficiary, _tokenAmount)); + require( + ERC20Mintable(address(getToken())).mint(_beneficiary, _tokenAmount)); } } diff --git a/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol b/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol index 841f09bc36b..dd75917873f 100644 --- a/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol +++ b/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol @@ -13,8 +13,8 @@ import "../../math/SafeMath.sol"; contract IncreasingPriceCrowdsale is TimedCrowdsale { using SafeMath for uint256; - uint256 public initialRate; - uint256 public finalRate; + uint256 private initialRate_; + uint256 private finalRate_; /** * @dev Constructor, takes initial and final rates of tokens received per wei contributed. @@ -24,8 +24,22 @@ contract IncreasingPriceCrowdsale is TimedCrowdsale { constructor(uint256 _initialRate, uint256 _finalRate) public { require(_finalRate > 0); require(_initialRate >= _finalRate); - initialRate = _initialRate; - finalRate = _finalRate; + initialRate_ = _initialRate; + finalRate_ = _finalRate; + } + + /** + * @return the initial rate of the crowdsale. + */ + function getInitialRate() public view returns(uint256) { + return initialRate_; + } + + /** + * @return the final rate of the crowdsale. + */ + function getFinalRate() public view returns (uint256) { + return finalRate_; } /** @@ -37,8 +51,8 @@ contract IncreasingPriceCrowdsale is TimedCrowdsale { // solium-disable-next-line security/no-block-members uint256 elapsedTime = block.timestamp.sub(openingTime); uint256 timeRange = closingTime.sub(openingTime); - uint256 rateRange = initialRate.sub(finalRate); - return initialRate.sub(elapsedTime.mul(rateRange).div(timeRange)); + uint256 rateRange = initialRate_.sub(finalRate_); + return initialRate_.sub(elapsedTime.mul(rateRange).div(timeRange)); } /** diff --git a/contracts/crowdsale/validation/CappedCrowdsale.sol b/contracts/crowdsale/validation/CappedCrowdsale.sol index 1df0e3eb311..9e158143e04 100644 --- a/contracts/crowdsale/validation/CappedCrowdsale.sol +++ b/contracts/crowdsale/validation/CappedCrowdsale.sol @@ -11,7 +11,7 @@ import "../Crowdsale.sol"; contract CappedCrowdsale is Crowdsale { using SafeMath for uint256; - uint256 public cap; + uint256 private cap_; /** * @dev Constructor, takes maximum amount of wei accepted in the crowdsale. @@ -19,7 +19,14 @@ contract CappedCrowdsale is Crowdsale { */ constructor(uint256 _cap) public { require(_cap > 0); - cap = _cap; + cap_ = _cap; + } + + /** + * @return the cap of the crowdsale. + */ + function getCap() public view returns(uint256) { + return cap_; } /** @@ -27,7 +34,7 @@ contract CappedCrowdsale is Crowdsale { * @return Whether the cap was reached */ function capReached() public view returns (bool) { - return weiRaised >= cap; + return getWeiRaised() >= cap_; } /** @@ -42,7 +49,7 @@ contract CappedCrowdsale is Crowdsale { internal { super._preValidatePurchase(_beneficiary, _weiAmount); - require(weiRaised.add(_weiAmount) <= cap); + require(getWeiRaised().add(_weiAmount) <= cap_); } } diff --git a/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol b/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol index 3c7884c658a..f0c3fdd4c43 100644 --- a/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol +++ b/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol @@ -12,8 +12,8 @@ import "../../ownership/Ownable.sol"; contract IndividuallyCappedCrowdsale is Ownable, Crowdsale { using SafeMath for uint256; - mapping(address => uint256) public contributions; - mapping(address => uint256) public caps; + mapping(address => uint256) private contributions_; + mapping(address => uint256) private caps_; /** * @dev Sets a specific user's maximum contribution. @@ -21,7 +21,7 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale { * @param _cap Wei limit for individual contribution */ function setUserCap(address _beneficiary, uint256 _cap) external onlyOwner { - caps[_beneficiary] = _cap; + caps_[_beneficiary] = _cap; } /** @@ -37,7 +37,7 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale { onlyOwner { for (uint256 i = 0; i < _beneficiaries.length; i++) { - caps[_beneficiaries[i]] = _cap; + caps_[_beneficiaries[i]] = _cap; } } @@ -47,7 +47,7 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale { * @return Current cap for individual user */ function getUserCap(address _beneficiary) public view returns (uint256) { - return caps[_beneficiary]; + return caps_[_beneficiary]; } /** @@ -58,7 +58,7 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale { function getUserContribution(address _beneficiary) public view returns (uint256) { - return contributions[_beneficiary]; + return contributions_[_beneficiary]; } /** @@ -73,7 +73,8 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale { internal { super._preValidatePurchase(_beneficiary, _weiAmount); - require(contributions[_beneficiary].add(_weiAmount) <= caps[_beneficiary]); + require( + contributions_[_beneficiary].add(_weiAmount) <= caps_[_beneficiary]); } /** @@ -88,7 +89,8 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale { internal { super._updatePurchasingState(_beneficiary, _weiAmount); - contributions[_beneficiary] = contributions[_beneficiary].add(_weiAmount); + contributions_[_beneficiary] = contributions_[_beneficiary].add( + _weiAmount); } } diff --git a/test/examples/SampleCrowdsale.test.js b/test/examples/SampleCrowdsale.test.js index f7b88080fb3..bfca735a992 100644 --- a/test/examples/SampleCrowdsale.test.js +++ b/test/examples/SampleCrowdsale.test.js @@ -45,10 +45,10 @@ contract('SampleCrowdsale', function ([_, owner, wallet, investor]) { (await this.crowdsale.openingTime()).should.be.bignumber.equal(this.openingTime); (await this.crowdsale.closingTime()).should.be.bignumber.equal(this.closingTime); - (await this.crowdsale.rate()).should.be.bignumber.equal(RATE); - (await this.crowdsale.wallet()).should.be.equal(wallet); - (await this.crowdsale.goal()).should.be.bignumber.equal(GOAL); - (await this.crowdsale.cap()).should.be.bignumber.equal(CAP); + (await this.crowdsale.getRate()).should.be.bignumber.equal(RATE); + (await this.crowdsale.getWallet()).should.be.equal(wallet); + (await this.crowdsale.getGoal()).should.be.bignumber.equal(GOAL); + (await this.crowdsale.getCap()).should.be.bignumber.equal(CAP); }); it('should not accept payments before start', async function () { From 44d113ab2e3642c6a9015abc11be385a10f4b9f2 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 02:31:01 +0000 Subject: [PATCH 20/30] add missing tests --- test/crowdsale/AllowanceCrowdsale.test.js | 4 ++++ test/crowdsale/IncreasingPriceCrowdsale.test.js | 5 +++++ test/crowdsale/PostDeliveryCrowdsale.test.js | 2 ++ 3 files changed, 11 insertions(+) diff --git a/test/crowdsale/AllowanceCrowdsale.test.js b/test/crowdsale/AllowanceCrowdsale.test.js index b0d68a9b15c..7a49f7e827c 100644 --- a/test/crowdsale/AllowanceCrowdsale.test.js +++ b/test/crowdsale/AllowanceCrowdsale.test.js @@ -25,6 +25,10 @@ contract('AllowanceCrowdsale', function ([_, investor, wallet, purchaser, tokenW }); describe('accepting payments', function () { + it.only('should have token wallet', async function () { + (await this.crowdsale.getTokenWallet()).should.be.equal(tokenWallet); + }); + it('should accept sends', async function () { await this.crowdsale.send(value); }); diff --git a/test/crowdsale/IncreasingPriceCrowdsale.test.js b/test/crowdsale/IncreasingPriceCrowdsale.test.js index e68d451bd46..ef647c18a28 100644 --- a/test/crowdsale/IncreasingPriceCrowdsale.test.js +++ b/test/crowdsale/IncreasingPriceCrowdsale.test.js @@ -55,6 +55,11 @@ contract('IncreasingPriceCrowdsale', function ([_, investor, wallet, purchaser]) await this.token.transfer(this.crowdsale.address, tokenSupply); }); + it('should have initial and final rate', async function () { + (await this.crowdsale.getInitialRate()).should.be.bignumber.equal(initialRate); + (await this.crowdsale.getFinalRate()).should.be.bignumber.equal(finalRate); + }); + it('at start', async function () { await increaseTimeTo(this.startTime); await this.crowdsale.buyTokens(investor, { value, from: purchaser }); diff --git a/test/crowdsale/PostDeliveryCrowdsale.test.js b/test/crowdsale/PostDeliveryCrowdsale.test.js index ebc41111c3d..843ea0b73c4 100644 --- a/test/crowdsale/PostDeliveryCrowdsale.test.js +++ b/test/crowdsale/PostDeliveryCrowdsale.test.js @@ -47,6 +47,7 @@ contract('PostDeliveryCrowdsale', function ([_, investor, wallet, purchaser]) { }); it('does not immediately assign tokens to beneficiaries', async function () { + (await this.crowdsale.getBalance(investor)).should.be.bignumber.equal(value); (await this.token.balanceOf(investor)).should.be.bignumber.equal(0); }); @@ -61,6 +62,7 @@ contract('PostDeliveryCrowdsale', function ([_, investor, wallet, purchaser]) { it('allows beneficiaries to withdraw tokens', async function () { await this.crowdsale.withdrawTokens({ from: investor }); + (await this.crowdsale.getBalance(investor)).should.be.bignumber.equal(0); (await this.token.balanceOf(investor)).should.be.bignumber.equal(value); }); From 02576701372e1c4fe38c1b587f5063a1d31bef1d Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Mon, 3 Sep 2018 02:49:14 +0000 Subject: [PATCH 21/30] remove only --- test/crowdsale/AllowanceCrowdsale.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/crowdsale/AllowanceCrowdsale.test.js b/test/crowdsale/AllowanceCrowdsale.test.js index 7a49f7e827c..64988d0f112 100644 --- a/test/crowdsale/AllowanceCrowdsale.test.js +++ b/test/crowdsale/AllowanceCrowdsale.test.js @@ -25,7 +25,7 @@ contract('AllowanceCrowdsale', function ([_, investor, wallet, purchaser, tokenW }); describe('accepting payments', function () { - it.only('should have token wallet', async function () { + it('should have token wallet', async function () { (await this.crowdsale.getTokenWallet()).should.be.equal(tokenWallet); }); From 5a476d14e548595217d9c5a9ddf0e82b51618281 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Wed, 5 Sep 2018 02:09:36 +0000 Subject: [PATCH 22/30] Do not prefix getters --- contracts/crowdsale/Crowdsale.sol | 10 +++++----- .../crowdsale/distribution/FinalizableCrowdsale.sol | 2 +- .../crowdsale/distribution/PostDeliveryCrowdsale.sol | 2 +- .../crowdsale/distribution/RefundableCrowdsale.sol | 8 ++++---- contracts/crowdsale/emission/AllowanceCrowdsale.sol | 6 +++--- contracts/crowdsale/emission/MintedCrowdsale.sol | 2 +- contracts/crowdsale/price/IncreasingPriceCrowdsale.sol | 4 ++-- contracts/crowdsale/validation/CappedCrowdsale.sol | 6 +++--- test/crowdsale/AllowanceCrowdsale.test.js | 2 +- test/crowdsale/IncreasingPriceCrowdsale.test.js | 4 ++-- test/crowdsale/PostDeliveryCrowdsale.test.js | 4 ++-- test/examples/SampleCrowdsale.test.js | 8 ++++---- 12 files changed, 29 insertions(+), 29 deletions(-) diff --git a/contracts/crowdsale/Crowdsale.sol b/contracts/crowdsale/Crowdsale.sol index adfb59e95ea..0b133c0aa28 100644 --- a/contracts/crowdsale/Crowdsale.sol +++ b/contracts/crowdsale/Crowdsale.sol @@ -79,28 +79,28 @@ contract Crowdsale { /** * @return the token being sold. */ - function getToken() public view returns(IERC20) { + function token() public view returns(IERC20) { return token_; } /** * @return the address where funds are collected. */ - function getWallet() public view returns(address) { + function wallet() public view returns(address) { return wallet_; } /** * @return the number of token units a buyer gets per wei. */ - function getRate() public view returns(uint256) { + function rate() public view returns(uint256) { return rate_; } /** * @return the mount of wei raised. */ - function getWeiRaised() public view returns (uint256) { + function weiRaised() public view returns (uint256) { return weiRaised_; } @@ -141,7 +141,7 @@ contract Crowdsale { * @dev Validation of an incoming purchase. Use require statements to revert state when conditions are not met. Use `super` in contracts that inherit from Crowdsale to extend their validations. * Example from CappedCrowdsale.sol's _preValidatePurchase method: * super._preValidatePurchase(_beneficiary, _weiAmount); - * require(getWeiRaised().add(_weiAmount) <= cap); + * require(weiRaised().add(_weiAmount) <= cap); * @param _beneficiary Address performing the token purchase * @param _weiAmount Value in wei involved in the purchase */ diff --git a/contracts/crowdsale/distribution/FinalizableCrowdsale.sol b/contracts/crowdsale/distribution/FinalizableCrowdsale.sol index 42d37b8ee15..693e0b498e1 100644 --- a/contracts/crowdsale/distribution/FinalizableCrowdsale.sol +++ b/contracts/crowdsale/distribution/FinalizableCrowdsale.sol @@ -20,7 +20,7 @@ contract FinalizableCrowdsale is Ownable, TimedCrowdsale { /** * @return true if the crowdsale is finalized, false otherwise. */ - function isFinalized() public view returns(bool) { + function finalized() public view returns(bool) { return finalized_; } diff --git a/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol b/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol index 6971669aedb..382c6210c89 100644 --- a/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol +++ b/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol @@ -28,7 +28,7 @@ contract PostDeliveryCrowdsale is TimedCrowdsale { /** * @return the balance of an account. */ - function getBalance(address _account) public view returns(uint256) { + function balanceOf(address _account) public view returns(uint256) { return balances_[_account]; } diff --git a/contracts/crowdsale/distribution/RefundableCrowdsale.sol b/contracts/crowdsale/distribution/RefundableCrowdsale.sol index 690afd2910d..8ffcfb592c6 100644 --- a/contracts/crowdsale/distribution/RefundableCrowdsale.sol +++ b/contracts/crowdsale/distribution/RefundableCrowdsale.sol @@ -26,14 +26,14 @@ contract RefundableCrowdsale is FinalizableCrowdsale { */ constructor(uint256 _goal) public { require(_goal > 0); - escrow_ = new RefundEscrow(getWallet()); + escrow_ = new RefundEscrow(wallet()); goal_ = _goal; } /** * @return minimum amount of funds to be raised in wei. */ - function getGoal() public view returns(uint256) { + function goal() public view returns(uint256) { return goal_; } @@ -41,7 +41,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale { * @dev Investors can claim refunds here if crowdsale is unsuccessful */ function claimRefund() public { - require(isFinalized()); + require(finalized()); require(!goalReached()); escrow_.withdraw(msg.sender); @@ -52,7 +52,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale { * @return Whether funding goal was reached */ function goalReached() public view returns (bool) { - return getWeiRaised() >= goal_; + return weiRaised() >= goal_; } /** diff --git a/contracts/crowdsale/emission/AllowanceCrowdsale.sol b/contracts/crowdsale/emission/AllowanceCrowdsale.sol index 776933948ac..54db50af1c9 100644 --- a/contracts/crowdsale/emission/AllowanceCrowdsale.sol +++ b/contracts/crowdsale/emission/AllowanceCrowdsale.sol @@ -28,7 +28,7 @@ contract AllowanceCrowdsale is Crowdsale { /** * @return the address of the wallet that will hold the tokens. */ - function getTokenWallet() public view returns(address) { + function tokenWallet() public view returns(address) { return tokenWallet_; } @@ -37,7 +37,7 @@ contract AllowanceCrowdsale is Crowdsale { * @return Amount of tokens left in the allowance */ function remainingTokens() public view returns (uint256) { - return getToken().allowance(tokenWallet_, this); + return token().allowance(tokenWallet_, this); } /** @@ -51,6 +51,6 @@ contract AllowanceCrowdsale is Crowdsale { ) internal { - getToken().safeTransferFrom(tokenWallet_, _beneficiary, _tokenAmount); + token().safeTransferFrom(tokenWallet_, _beneficiary, _tokenAmount); } } diff --git a/contracts/crowdsale/emission/MintedCrowdsale.sol b/contracts/crowdsale/emission/MintedCrowdsale.sol index 5718fca1ede..1b676e07767 100644 --- a/contracts/crowdsale/emission/MintedCrowdsale.sol +++ b/contracts/crowdsale/emission/MintedCrowdsale.sol @@ -24,6 +24,6 @@ contract MintedCrowdsale is Crowdsale { { // Potentially dangerous assumption about the type of the token. require( - ERC20Mintable(address(getToken())).mint(_beneficiary, _tokenAmount)); + ERC20Mintable(address(token())).mint(_beneficiary, _tokenAmount)); } } diff --git a/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol b/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol index dd75917873f..7cc07f083d5 100644 --- a/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol +++ b/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol @@ -31,14 +31,14 @@ contract IncreasingPriceCrowdsale is TimedCrowdsale { /** * @return the initial rate of the crowdsale. */ - function getInitialRate() public view returns(uint256) { + function initialRate() public view returns(uint256) { return initialRate_; } /** * @return the final rate of the crowdsale. */ - function getFinalRate() public view returns (uint256) { + function finalRate() public view returns (uint256) { return finalRate_; } diff --git a/contracts/crowdsale/validation/CappedCrowdsale.sol b/contracts/crowdsale/validation/CappedCrowdsale.sol index 9e158143e04..99b188a9883 100644 --- a/contracts/crowdsale/validation/CappedCrowdsale.sol +++ b/contracts/crowdsale/validation/CappedCrowdsale.sol @@ -25,7 +25,7 @@ contract CappedCrowdsale is Crowdsale { /** * @return the cap of the crowdsale. */ - function getCap() public view returns(uint256) { + function cap() public view returns(uint256) { return cap_; } @@ -34,7 +34,7 @@ contract CappedCrowdsale is Crowdsale { * @return Whether the cap was reached */ function capReached() public view returns (bool) { - return getWeiRaised() >= cap_; + return weiRaised() >= cap_; } /** @@ -49,7 +49,7 @@ contract CappedCrowdsale is Crowdsale { internal { super._preValidatePurchase(_beneficiary, _weiAmount); - require(getWeiRaised().add(_weiAmount) <= cap_); + require(weiRaised().add(_weiAmount) <= cap_); } } diff --git a/test/crowdsale/AllowanceCrowdsale.test.js b/test/crowdsale/AllowanceCrowdsale.test.js index 64988d0f112..ee6a063f2bd 100644 --- a/test/crowdsale/AllowanceCrowdsale.test.js +++ b/test/crowdsale/AllowanceCrowdsale.test.js @@ -26,7 +26,7 @@ contract('AllowanceCrowdsale', function ([_, investor, wallet, purchaser, tokenW describe('accepting payments', function () { it('should have token wallet', async function () { - (await this.crowdsale.getTokenWallet()).should.be.equal(tokenWallet); + (await this.crowdsale.tokenWallet()).should.be.equal(tokenWallet); }); it('should accept sends', async function () { diff --git a/test/crowdsale/IncreasingPriceCrowdsale.test.js b/test/crowdsale/IncreasingPriceCrowdsale.test.js index ef647c18a28..c62c17d89bc 100644 --- a/test/crowdsale/IncreasingPriceCrowdsale.test.js +++ b/test/crowdsale/IncreasingPriceCrowdsale.test.js @@ -56,8 +56,8 @@ contract('IncreasingPriceCrowdsale', function ([_, investor, wallet, purchaser]) }); it('should have initial and final rate', async function () { - (await this.crowdsale.getInitialRate()).should.be.bignumber.equal(initialRate); - (await this.crowdsale.getFinalRate()).should.be.bignumber.equal(finalRate); + (await this.crowdsale.initialRate()).should.be.bignumber.equal(initialRate); + (await this.crowdsale.finalRate()).should.be.bignumber.equal(finalRate); }); it('at start', async function () { diff --git a/test/crowdsale/PostDeliveryCrowdsale.test.js b/test/crowdsale/PostDeliveryCrowdsale.test.js index 843ea0b73c4..11058928ade 100644 --- a/test/crowdsale/PostDeliveryCrowdsale.test.js +++ b/test/crowdsale/PostDeliveryCrowdsale.test.js @@ -47,7 +47,7 @@ contract('PostDeliveryCrowdsale', function ([_, investor, wallet, purchaser]) { }); it('does not immediately assign tokens to beneficiaries', async function () { - (await this.crowdsale.getBalance(investor)).should.be.bignumber.equal(value); + (await this.crowdsale.balanceOf(investor)).should.be.bignumber.equal(value); (await this.token.balanceOf(investor)).should.be.bignumber.equal(0); }); @@ -62,7 +62,7 @@ contract('PostDeliveryCrowdsale', function ([_, investor, wallet, purchaser]) { it('allows beneficiaries to withdraw tokens', async function () { await this.crowdsale.withdrawTokens({ from: investor }); - (await this.crowdsale.getBalance(investor)).should.be.bignumber.equal(0); + (await this.crowdsale.balanceOf(investor)).should.be.bignumber.equal(0); (await this.token.balanceOf(investor)).should.be.bignumber.equal(value); }); diff --git a/test/examples/SampleCrowdsale.test.js b/test/examples/SampleCrowdsale.test.js index bfca735a992..f7b88080fb3 100644 --- a/test/examples/SampleCrowdsale.test.js +++ b/test/examples/SampleCrowdsale.test.js @@ -45,10 +45,10 @@ contract('SampleCrowdsale', function ([_, owner, wallet, investor]) { (await this.crowdsale.openingTime()).should.be.bignumber.equal(this.openingTime); (await this.crowdsale.closingTime()).should.be.bignumber.equal(this.closingTime); - (await this.crowdsale.getRate()).should.be.bignumber.equal(RATE); - (await this.crowdsale.getWallet()).should.be.equal(wallet); - (await this.crowdsale.getGoal()).should.be.bignumber.equal(GOAL); - (await this.crowdsale.getCap()).should.be.bignumber.equal(CAP); + (await this.crowdsale.rate()).should.be.bignumber.equal(RATE); + (await this.crowdsale.wallet()).should.be.equal(wallet); + (await this.crowdsale.goal()).should.be.bignumber.equal(GOAL); + (await this.crowdsale.cap()).should.be.bignumber.equal(CAP); }); it('should not accept payments before start', async function () { From 85a0fc3fdea37d7f9cdd266ce050ddb7d6d9ebb0 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Wed, 5 Sep 2018 03:45:14 +0000 Subject: [PATCH 23/30] Update modifiers to call public view functions. Fixes #1179. --- .../crowdsale/validation/TimedCrowdsale.sol | 10 +++++++- contracts/ownership/Ownable.sol | 9 ++++++- contracts/token/ERC20/ERC20Mintable.sol | 24 ++++++++++++++----- contracts/token/ERC20/RBACMintableToken.sol | 2 +- test/crowdsale/TimedCrowdsale.test.js | 2 ++ test/ownership/Ownable.behavior.js | 3 +++ 6 files changed, 41 insertions(+), 9 deletions(-) diff --git a/contracts/crowdsale/validation/TimedCrowdsale.sol b/contracts/crowdsale/validation/TimedCrowdsale.sol index 52b7f4ef3e0..be2dd7e81db 100644 --- a/contracts/crowdsale/validation/TimedCrowdsale.sol +++ b/contracts/crowdsale/validation/TimedCrowdsale.sol @@ -19,7 +19,7 @@ contract TimedCrowdsale is Crowdsale { */ modifier onlyWhileOpen { // solium-disable-next-line security/no-block-members - require(block.timestamp >= openingTime && block.timestamp <= closingTime); + require(isOpen()); _; } @@ -37,6 +37,14 @@ contract TimedCrowdsale is Crowdsale { closingTime = _closingTime; } + /** + * @return true if the crowdsale is open, false otherwise. + */ + function isOpen() public view returns (bool) { + // solium-disable-next-line security/no-block-members + return block.timestamp >= openingTime && block.timestamp <= closingTime; + } + /** * @dev Checks whether the period in which the crowdsale is open has already elapsed. * @return Whether crowdsale period has elapsed diff --git a/contracts/ownership/Ownable.sol b/contracts/ownership/Ownable.sol index f89013a1a53..c6ea3e16fa0 100644 --- a/contracts/ownership/Ownable.sol +++ b/contracts/ownership/Ownable.sol @@ -36,10 +36,17 @@ contract Ownable { * @dev Throws if called by any account other than the owner. */ modifier onlyOwner() { - require(msg.sender == owner_); + require(isOwner()); _; } + /** + * @return true if `msg.sender` is the owner of the contract. + */ + function isOwner() public view returns(bool) { + return msg.sender == owner_; + } + /** * @dev Allows the current owner to relinquish control of the contract. * @notice Renouncing to ownership will leave the contract without an owner. diff --git a/contracts/token/ERC20/ERC20Mintable.sol b/contracts/token/ERC20/ERC20Mintable.sol index 518936699e4..0ecabe3f4d1 100644 --- a/contracts/token/ERC20/ERC20Mintable.sol +++ b/contracts/token/ERC20/ERC20Mintable.sol @@ -16,13 +16,13 @@ contract ERC20Mintable is ERC20, Ownable { bool private mintingFinished_ = false; - modifier canMint() { + modifier onlyBeforeMintingFinished() { require(!mintingFinished_); _; } - modifier hasMintPermission() { - require(msg.sender == owner()); + modifier onlyMinter() { + require(isMinter()); _; } @@ -33,6 +33,13 @@ contract ERC20Mintable is ERC20, Ownable { return mintingFinished_; } + /** + * @return true if `msg.sender` is the minter. + */ + function isMinter() public view returns(bool) { + return msg.sender == owner(); + } + /** * @dev Function to mint tokens * @param _to The address that will receive the minted tokens. @@ -44,8 +51,8 @@ contract ERC20Mintable is ERC20, Ownable { uint256 _amount ) public - hasMintPermission - canMint + onlyMinter + onlyBeforeMintingFinished returns (bool) { _mint(_to, _amount); @@ -57,7 +64,12 @@ contract ERC20Mintable is ERC20, Ownable { * @dev Function to stop minting new tokens. * @return True if the operation was successful. */ - function finishMinting() public onlyOwner canMint returns (bool) { + function finishMinting() + public + onlyOwner + onlyBeforeMintingFinished + returns (bool) + { mintingFinished_ = true; emit MintFinished(); return true; diff --git a/contracts/token/ERC20/RBACMintableToken.sol b/contracts/token/ERC20/RBACMintableToken.sol index ad6ab19ca58..65e16f564f8 100644 --- a/contracts/token/ERC20/RBACMintableToken.sol +++ b/contracts/token/ERC20/RBACMintableToken.sol @@ -18,7 +18,7 @@ contract RBACMintableToken is ERC20Mintable, RBAC { /** * @dev override the Mintable token modifier to add role based logic */ - modifier hasMintPermission() { + modifier onlyMinter() { checkRole(msg.sender, ROLE_MINTER); _; } diff --git a/test/crowdsale/TimedCrowdsale.test.js b/test/crowdsale/TimedCrowdsale.test.js index ad9ca29f556..5c246482f4a 100644 --- a/test/crowdsale/TimedCrowdsale.test.js +++ b/test/crowdsale/TimedCrowdsale.test.js @@ -50,8 +50,10 @@ contract('TimedCrowdsale', function ([_, investor, wallet, purchaser]) { }); it('should be ended only after end', async function () { + (await this.crowdsale.isOpen()).should.equal(true); (await this.crowdsale.hasClosed()).should.equal(false); await increaseTimeTo(this.afterClosingTime); + (await this.crowdsale.isOpen()).should.equal(false); (await this.crowdsale.hasClosed()).should.equal(true); }); diff --git a/test/ownership/Ownable.behavior.js b/test/ownership/Ownable.behavior.js index 6257c01df5a..eac3c10ce81 100644 --- a/test/ownership/Ownable.behavior.js +++ b/test/ownership/Ownable.behavior.js @@ -13,8 +13,11 @@ function shouldBehaveLikeOwnable (owner, [anyone]) { }); it('changes owner after transfer', async function () { + (await this.ownable.isOwner({ from: anyone })).should.be.equal(false); await this.ownable.transferOwnership(anyone, { from: owner }); + (await this.ownable.owner()).should.equal(anyone); + (await this.ownable.isOwner({ from: anyone })).should.be.equal(true); }); it('should prevent non-owners from transfering', async function () { From 24761a56790295e304e566beb50859c31486c25f Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Wed, 5 Sep 2018 06:00:45 +0000 Subject: [PATCH 24/30] remove isMinter --- contracts/token/ERC20/ERC20Mintable.sol | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/contracts/token/ERC20/ERC20Mintable.sol b/contracts/token/ERC20/ERC20Mintable.sol index 0ecabe3f4d1..569bea522f5 100644 --- a/contracts/token/ERC20/ERC20Mintable.sol +++ b/contracts/token/ERC20/ERC20Mintable.sol @@ -22,7 +22,7 @@ contract ERC20Mintable is ERC20, Ownable { } modifier onlyMinter() { - require(isMinter()); + require(isOwner(msg.sender)); _; } @@ -33,13 +33,6 @@ contract ERC20Mintable is ERC20, Ownable { return mintingFinished_; } - /** - * @return true if `msg.sender` is the minter. - */ - function isMinter() public view returns(bool) { - return msg.sender == owner(); - } - /** * @dev Function to mint tokens * @param _to The address that will receive the minted tokens. From c14d59784079cb759f6e85868eba9cb8d09f0838 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Wed, 5 Sep 2018 13:19:57 +0000 Subject: [PATCH 25/30] fix is owner call --- contracts/token/ERC20/ERC20Mintable.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/token/ERC20/ERC20Mintable.sol b/contracts/token/ERC20/ERC20Mintable.sol index 569bea522f5..8b80dd80ae5 100644 --- a/contracts/token/ERC20/ERC20Mintable.sol +++ b/contracts/token/ERC20/ERC20Mintable.sol @@ -22,7 +22,7 @@ contract ERC20Mintable is ERC20, Ownable { } modifier onlyMinter() { - require(isOwner(msg.sender)); + require(isOwner()); _; } From ba7fa16029355cf21b7666fbde61df064ef540b5 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Wed, 5 Sep 2018 13:43:30 +0000 Subject: [PATCH 26/30] fix isOpen --- test/crowdsale/TimedCrowdsale.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/crowdsale/TimedCrowdsale.test.js b/test/crowdsale/TimedCrowdsale.test.js index 5c246482f4a..4c7eff303c2 100644 --- a/test/crowdsale/TimedCrowdsale.test.js +++ b/test/crowdsale/TimedCrowdsale.test.js @@ -50,7 +50,6 @@ contract('TimedCrowdsale', function ([_, investor, wallet, purchaser]) { }); it('should be ended only after end', async function () { - (await this.crowdsale.isOpen()).should.equal(true); (await this.crowdsale.hasClosed()).should.equal(false); await increaseTimeTo(this.afterClosingTime); (await this.crowdsale.isOpen()).should.equal(false); @@ -59,12 +58,14 @@ contract('TimedCrowdsale', function ([_, investor, wallet, purchaser]) { describe('accepting payments', function () { it('should reject payments before start', async function () { + (await this.crowdsale.isOpen()).should.equal(false); await expectThrow(this.crowdsale.send(value), EVMRevert); await expectThrow(this.crowdsale.buyTokens(investor, { from: purchaser, value: value }), EVMRevert); }); it('should accept payments after start', async function () { await increaseTimeTo(this.openingTime); + (await this.crowdsale.isOpen()).should.equal(true); await this.crowdsale.send(value); await this.crowdsale.buyTokens(investor, { value: value, from: purchaser }); }); From 7a37725e54f092272b323640d49eb03bb2eebc2e Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Thu, 6 Sep 2018 00:29:05 +0000 Subject: [PATCH 27/30] Fix merge --- contracts/payment/SplitPayment.sol | 35 ------------------------------ 1 file changed, 35 deletions(-) diff --git a/contracts/payment/SplitPayment.sol b/contracts/payment/SplitPayment.sol index b01fe7fcce3..0d4a2e0956d 100644 --- a/contracts/payment/SplitPayment.sol +++ b/contracts/payment/SplitPayment.sol @@ -70,41 +70,6 @@ contract SplitPayment { return payees_[index]; } - /** - * @dev Claim your share of the balance. - */ - function totalShares() public view returns(uint256) { - return totalShares_; - } - - /** - * @return the total amount already released. - */ - function totalReleased() public view returns(uint256) { - return totalReleased_; - } - - /** - * @return the shares of an account. - */ - function shares(address _account) public view returns(uint256) { - return shares_[_account]; - } - - /** - * @return the amount already released to an account. - */ - function released(address _account) public view returns(uint256) { - return released_[_account]; - } - - /** - * @return the address of a payee. - */ - function payee(uint256 index) public view returns(address) { - return payees_[index]; - } - /** * @dev Release one of the payee's proportional payment. * @param _payee Whose payments will be released. From 52352ce5ce8cd7aedd559bbc5bb3b5595ad19380 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Thu, 6 Sep 2018 01:06:52 +0000 Subject: [PATCH 28/30] Improve encapsulation on TimedCrowdsale --- .../price/IncreasingPriceCrowdsale.sol | 4 +-- .../crowdsale/validation/TimedCrowdsale.sol | 26 ++++++++++++++----- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol b/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol index 7cc07f083d5..ac910d1c855 100644 --- a/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol +++ b/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol @@ -49,8 +49,8 @@ contract IncreasingPriceCrowdsale is TimedCrowdsale { */ function getCurrentRate() public view returns (uint256) { // solium-disable-next-line security/no-block-members - uint256 elapsedTime = block.timestamp.sub(openingTime); - uint256 timeRange = closingTime.sub(openingTime); + uint256 elapsedTime = block.timestamp.sub(openingTime()); + uint256 timeRange = closingTime.sub(openingTime()); uint256 rateRange = initialRate_.sub(finalRate_); return initialRate_.sub(elapsedTime.mul(rateRange).div(timeRange)); } diff --git a/contracts/crowdsale/validation/TimedCrowdsale.sol b/contracts/crowdsale/validation/TimedCrowdsale.sol index be2dd7e81db..fd049fc6cb7 100644 --- a/contracts/crowdsale/validation/TimedCrowdsale.sol +++ b/contracts/crowdsale/validation/TimedCrowdsale.sol @@ -11,8 +11,8 @@ import "../Crowdsale.sol"; contract TimedCrowdsale is Crowdsale { using SafeMath for uint256; - uint256 public openingTime; - uint256 public closingTime; + uint256 private openingTime_; + uint256 private closingTime_; /** * @dev Reverts if not in crowdsale time range. @@ -33,8 +33,22 @@ contract TimedCrowdsale is Crowdsale { require(_openingTime >= block.timestamp); require(_closingTime >= _openingTime); - openingTime = _openingTime; - closingTime = _closingTime; + openingTime_ = _openingTime; + closingTime_ = _closingTime; + } + + /** + * @return the crowdsale opening time. + */ + function openingTime() public view returns(uint256) { + return openingTime_; + } + + /** + * @return the crowdsale closing time. + */ + function closingTime() public view returns(uint256) { + return closingTime_; } /** @@ -42,7 +56,7 @@ contract TimedCrowdsale is Crowdsale { */ function isOpen() public view returns (bool) { // solium-disable-next-line security/no-block-members - return block.timestamp >= openingTime && block.timestamp <= closingTime; + return block.timestamp >= openingTime_ && block.timestamp <= closingTime_; } /** @@ -51,7 +65,7 @@ contract TimedCrowdsale is Crowdsale { */ function hasClosed() public view returns (bool) { // solium-disable-next-line security/no-block-members - return block.timestamp > closingTime; + return block.timestamp > closingTime_; } /** From 63b93ccc1d55ec7abcaafcdd268869d82319f201 Mon Sep 17 00:00:00 2001 From: Leo Arias Date: Thu, 6 Sep 2018 01:13:08 +0000 Subject: [PATCH 29/30] Add missing parentheses --- contracts/crowdsale/price/IncreasingPriceCrowdsale.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol b/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol index ac910d1c855..ad448b319a4 100644 --- a/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol +++ b/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol @@ -50,7 +50,7 @@ contract IncreasingPriceCrowdsale is TimedCrowdsale { function getCurrentRate() public view returns (uint256) { // solium-disable-next-line security/no-block-members uint256 elapsedTime = block.timestamp.sub(openingTime()); - uint256 timeRange = closingTime.sub(openingTime()); + uint256 timeRange = closingTime().sub(openingTime()); uint256 rateRange = initialRate_.sub(finalRate_); return initialRate_.sub(elapsedTime.mul(rateRange).div(timeRange)); } From 369b8d6293e5dac5754cb03c85dc3deab1f5a0f7 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Thu, 6 Sep 2018 12:15:34 -0300 Subject: [PATCH 30/30] remove duplicate function definition --- contracts/crowdsale/validation/TimedCrowdsale.sol | 8 -------- 1 file changed, 8 deletions(-) diff --git a/contracts/crowdsale/validation/TimedCrowdsale.sol b/contracts/crowdsale/validation/TimedCrowdsale.sol index 4804a51eed9..c9726d9402b 100644 --- a/contracts/crowdsale/validation/TimedCrowdsale.sol +++ b/contracts/crowdsale/validation/TimedCrowdsale.sol @@ -58,14 +58,6 @@ contract TimedCrowdsale is Crowdsale { return block.timestamp >= openingTime_ && block.timestamp <= closingTime_; } - /** - * @return true if the crowdsale is open, false otherwise. - */ - function isOpen() public view returns (bool) { - // solium-disable-next-line security/no-block-members - return block.timestamp >= openingTime && block.timestamp <= closingTime; - } - /** * @dev Checks whether the period in which the crowdsale is open has already elapsed. * @return Whether crowdsale period has elapsed