From a53f9aced7184a0e4281a6e2789b44cb0b698420 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Wed, 25 Apr 2018 16:36:42 +0800 Subject: [PATCH] Cosmetic fixes (#78) --- apps/finance/contracts/Finance.sol | 37 ++++++----- apps/token-manager/contracts/TokenManager.sol | 61 +++++++++---------- apps/voting/contracts/Voting.sol | 18 +++--- 3 files changed, 56 insertions(+), 60 deletions(-) diff --git a/apps/finance/contracts/Finance.sol b/apps/finance/contracts/Finance.sol index 2961a423e6..2ffde6658d 100644 --- a/apps/finance/contracts/Finance.sol +++ b/apps/finance/contracts/Finance.sol @@ -132,7 +132,7 @@ contract Finance is AragonApp { } /** - * @dev Deposit for ERC20 approved tokens + * @dev Deposit for approved ERC20 tokens * @notice Deposit `_amount / 10^18` `_token.symbol(): string` * @param _token Address of deposited token * @param _amount Amount of tokens sent @@ -178,12 +178,12 @@ contract Finance is AragonApp { /** * @notice Create a new payment of `_amount / 10^18` `_token.symbol(): string`. `_maxRepeats > 0 ? 'It will be executed ' + _maxRepeats + ' times at intervals of ' + (_interval - _interval % 86400) / 86400 + ' days' : ''` * @param _token Address of token for payment - * @param _receiver Address that will receive payment. - * @param _amount units of token that are payed every time the payment is due. - * @param _initialPaymentTime timestamp for when the first payment is done. - * @param _interval number of seconds that need to pass between payment transactions. - * @param _maxRepeats maximum instances a payment can be executed. - * @param _reference string detailing payment reason + * @param _receiver Address that will receive payment + * @param _amount Tokens that are payed every time the payment is due + * @param _initialPaymentTime Timestamp for when the first payment is done + * @param _interval Number of seconds that need to pass between payment transactions + * @param _maxRepeats Maximum instances a payment can be executed + * @param _reference String detailing payment reason */ function newPayment( address _token, @@ -195,7 +195,6 @@ contract Finance is AragonApp { string _reference ) authP(CREATE_PAYMENTS_ROLE, arr(_token, _receiver, _amount, _interval, _maxRepeats)) isInitialized transitionsPeriod external returns (uint256 paymentId) { - require(settings.budgets[_token] > 0 || !settings.hasBudget[_token]); // Token must have been added to budget // Avoid saving payment data for 1 time immediate payments @@ -294,11 +293,10 @@ contract Finance is AragonApp { } /** - * @dev Allows make a simple payment from this contract to Vault, - to avoid locked tokens in contract forever. - This contract should never receive tokens with a simple transfer call, - but in case it happens, this function allows to recover them. - * @notice Send tokens to Vault + * @dev Allows making a simple payment from this contract to the Vault, to avoid locked tokens. + * This contract should never receive tokens with a simple transfer call, but in case it + * happens, this function allows for their recovery. + * @notice Send tokens held in this contract to the Vault * @param _token Token whose balance is going to be transferred. */ function depositToVault(address _token) isInitialized public { @@ -319,11 +317,11 @@ contract Finance is AragonApp { /** * @dev Transitions accounting periods if needed. For preventing OOG attacks, - a TTL param is provided. If more that TTL periods need to be transitioned, - it will return false. - * @notice Transition accounting period + * a TTL param is provided. If more than the TTL periods need to be transitioned, + * it will return false. + * @notice Transition accounting period if needed * @param _ttl Maximum periods that can be transitioned - * @return success boolean indicating whether the accounting period is the correct one (if false, TTL was surpased and another call is needed) + * @return Boolean indicating whether the accounting period was transitioned to the correct one (if false, TTL was surpased and another call is needed) */ function tryTransitionAccountingPeriod(uint256 _ttl) public returns (bool success) { Period storage currentPeriod = periods[currentPeriodId()]; @@ -394,7 +392,8 @@ contract Finance is AragonApp { function getPeriodTokenStatement(uint256 _periodId, address _token) public view returns (uint256 expenses, uint256 income) { TokenStatement storage tokenStatement = periods[_periodId].tokenStatement[_token]; - return (tokenStatement.expenses, tokenStatement.income); + expenses = tokenStatement.expenses; + income = tokenStatement.income; } function nextPaymentTime(uint256 _paymentId) public view returns (uint64) { @@ -409,7 +408,7 @@ contract Finance is AragonApp { return uint64(nextPayment); } - function getPeriodDuration() public view returns (uint64 periodDuration) { + function getPeriodDuration() public view returns (uint64) { return settings.periodDuration; } diff --git a/apps/token-manager/contracts/TokenManager.sol b/apps/token-manager/contracts/TokenManager.sol index ec05cc3bcd..05b842386a 100644 --- a/apps/token-manager/contracts/TokenManager.sol +++ b/apps/token-manager/contracts/TokenManager.sol @@ -49,10 +49,10 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove /** * @notice Initializes Token Manager for `_token.symbol(): string`, `transerable ? 'T' : 'Not t'`ransferable`_maxAccountTokens > 0 ? ', with a maximum of ' _maxAccountTokens ' per account' : ''` and with`_logHolders ? '' : 'out'` storage of token holders. - * @param _token MiniMeToken address for the managed token (token manager must be the token controller) + * @param _token MiniMeToken address for the managed token (Token Manager instance must be already set as the token controller) * @param _transferable whether the token can be transferred by holders - * @param _maxAccountTokens maximum amount of tokens an account can have (0 for infinite tokens) - * @param _logHolders Whether the token manager will store all token holders (makes token transfers more expensive!) + * @param _maxAccountTokens Maximum amount of tokens an account can have (0 for infinite tokens) + * @param _logHolders Whether the Token Manager will store all token holders (makes token transfers more expensive!) */ function initialize( MiniMeToken _token, @@ -84,7 +84,7 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove } /** - * @notice Mint `_amount / 10^18` tokens + * @notice Mint `_amount / 10^18` tokens for the Token Manager * @param _amount Number of tokens minted */ function issue(uint256 _amount) authP(ISSUE_ROLE, arr(_amount)) external { @@ -94,7 +94,7 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove /** * @notice Assign `_amount / 10^18` tokens to `_receiver` from Token Manager's holdings * @param _receiver The address receiving the tokens - * @param _amount Number of tokens transfered + * @param _amount Number of tokens transferred */ function assign(address _receiver, uint256 _amount) authP(ASSIGN_ROLE, arr(_receiver, _amount)) external { _assign(_receiver, _amount); @@ -111,32 +111,32 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove } /** - * @notice Assign `_amount / 10^18` tokens to `_receiver` with a `_revokable : 'revokable' : ''` vesting starting at `_start` and a cliff at `_cliff`, with vesting on `_vesting` + * @notice Assign `_amount / 10^18` tokens to `_receiver` from the Token Manager's holdings with a `_revokable : 'revokable' : ''` vesting starting at `_start`, cliff at `_cliff` (first portion of tokens transferable), and completed vesting at `_vesting` (all tokens transferable) * @param _receiver The address receiving the tokens - * @param _amount Number of tokens transfered + * @param _amount Number of tokens vested * @param _start Date the vesting calculations start - * @param _cliff Date when the initial proportional amount of tokens are transferable - * @param _vesting Date when all tokens are transferable - * @param _revokable Whether the vesting can be revoked by the token manager + * @param _cliff Date when the initial portion of tokens are transferable + * @param _vested Date when all tokens are transferable + * @param _revokable Whether the vesting can be revoked by the Token Manager */ function assignVested( address _receiver, uint256 _amount, uint64 _start, uint64 _cliff, - uint64 _vesting, + uint64 _vested, bool _revokable ) authP(ASSIGN_ROLE, arr(_receiver, _amount)) external returns (uint256) { require(tokenGrantsCount(_receiver) < MAX_VESTINGS_PER_ADDRESS); - require(_start <= _cliff && _cliff <= _vesting); + require(_start <= _cliff && _cliff <= _vested); TokenVesting memory tokenVesting = TokenVesting( _amount, _start, _cliff, - _vesting, + _vested, _revokable ); uint256 vestingId = vestings[_receiver].push(tokenVesting) - 1; @@ -178,7 +178,7 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove /** * @notice Execute desired action as a token holder * @dev IForwarder interface conformance. Forwards any token holder action. - * @param _evmScript script being executed + * @param _evmScript Script being executed */ function forward(bytes _evmScript) public { require(canForward(msg.sender, _evmScript)); @@ -199,8 +199,7 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove function allHolders() public view returns (address[]) { return holders; } /* - * @dev Notifies the controller about a token transfer allowing the - * controller to decide whether to allow it or react if desired + * @dev Notifies the controller about a token transfer allowing the controller to decide whether to allow it or react if desired * @param _from The origin of the transfer * @param _to The destination of the transfer * @param _amount The amount of the transfer @@ -253,13 +252,13 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove } /** - * @dev Calculate amount of non-vested tokens at a specifc time. - * @param tokens uint256 The amount of tokens grantted. - * @param time uint64 The time to be checked - * @param start uint64 A time representing the begining of the grant - * @param cliff uint64 The cliff period. - * @param vesting uint64 The vesting period. - * @return An uint256 representing the amount of non-vested tokensof a specif grant. + * @dev Calculate amount of non-vested tokens at a specifc time + * @param tokens The total amount of tokens vested + * @param time The time at which to check + * @param start The date vesting started + * @param cliff The cliff period + * @param vested The fully vested date + * @return The amount of non-vested tokens of a specific grant * transferableTokens * | _/-------- vestedTokens rect * | _/ @@ -274,17 +273,17 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove * | . | * | . | * +===+===========+---------+----------> time - * Start Clift Vesting + * Start Clift Vested */ function calculateNonVestedTokens( uint256 tokens, uint256 time, uint256 start, uint256 cliff, - uint256 vesting) private pure returns (uint256) + uint256 vested) private pure returns (uint256) { - // Shortcuts for before cliff and after vesting cases. - if (time >= vesting) { + // Shortcuts for before cliff and after vested cases. + if (time >= vested) { return 0; } if (time < cliff) { @@ -295,7 +294,7 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove // As before cliff the shortcut returns 0, we can use just calculate a value // in the vesting rect (as shown in above's figure) - // vestedTokens = tokens * (time - start) / (vesting - start) + // vestedTokens = tokens * (time - start) / (vested - start) uint256 vestedTokens = SafeMath.div( SafeMath.mul( tokens, @@ -305,7 +304,7 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove ) ), SafeMath.sub( - vesting, + vested, start ) ); @@ -343,13 +342,11 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove // Even though it is tested, solidity-coverage doesnt get it because // MiniMeToken is not instrumented and entire tx is reverted require(msg.sender == address(token)); - _owner; return false; } /** - * @dev Notifies the controller about an approval allowing the - * controller to react if desired + * @dev Notifies the controller about an approval allowing the controller to react if desired * @param _owner The address that calls `approve()` * @param _spender The spender in the `approve()` call * @param _amount The amount in the `approve()` call diff --git a/apps/voting/contracts/Voting.sol b/apps/voting/contracts/Voting.sol index c666a28f2e..74b07534d8 100644 --- a/apps/voting/contracts/Voting.sol +++ b/apps/voting/contracts/Voting.sol @@ -19,7 +19,7 @@ contract Voting is IForwarder, AragonApp { uint256 public minAcceptQuorumPct; uint64 public voteTime; - uint256 constant public PCT_BASE = 10 ** 18; // 0% = 0; 1% = 10^16; 100% = 10^18 + uint256 constant public PCT_BASE = 10 ** 18; // 0% = 0; 1% = 10^16; 100% = 10^18 bytes32 constant public CREATE_VOTES_ROLE = keccak256("CREATE_VOTES_ROLE"); bytes32 constant public MODIFY_QUORUM_ROLE = keccak256("MODIFY_QUORUM_ROLE"); @@ -49,10 +49,10 @@ contract Voting is IForwarder, AragonApp { /** * @notice Initializes Voting app with `_token.symbol(): string` for governance, minimum support of `(_supportRequiredPct - _supportRequiredPct % 10^14) / 10^16`, minimum acceptance quorum of `(_minAcceptQuorumPct - _minAcceptQuorumPct % 10^14) / 10^16` and vote duations of `(_voteTime - _voteTime % 86400) / 86400` day `_voteTime >= 172800 ? 's' : ''` - * @param _token MiniMeToken address that will be used as governance token - * @param _supportRequiredPct Percentage of voters that must support a vote for it to succeed (expressed as a 10^18 percentage, (eg 10^16 = 1%, 10^18 = 100%) - * @param _minAcceptQuorumPct Percentage of total voting power that must support a vote for it to succeed (expressed as a 10^18 percentage, (eg 10^16 = 1%, 10^18 = 100%) - * @param _voteTime Seconds that a vote will be open for token holders to vote (unless it is impossible for the fate of the vote to change) + * @param _token MiniMeToken Address that will be used as governance token + * @param _supportRequiredPct Percentage of yeas in casted votes for a vote to succeed (expressed as a percentage of 10^18; eg. 10^16 = 1%, 10^18 = 100%) + * @param _minAcceptQuorumPct Percentage of yeas in total possible votes for a vote to succeed (expressed as a percentage of 10^18; eg. 10^16 = 1%, 10^18 = 100%) + * @param _voteTime Seconds that a vote will be open for token holders to vote (unless enough yeas or nays have been cast to make an early decision) */ function initialize( MiniMeToken _token, @@ -91,7 +91,7 @@ contract Voting is IForwarder, AragonApp { * @notice Create a new vote about "`_metadata`" * @param _executionScript EVM script to be executed on approval * @param _metadata Vote metadata - * @return voteId id for newly created vote + * @return voteId Id for newly created vote */ function newVote(bytes _executionScript, string _metadata) auth(CREATE_VOTES_ROLE) external returns (uint256 voteId) { return _newVote(_executionScript, _metadata, true); @@ -112,7 +112,7 @@ contract Voting is IForwarder, AragonApp { * @notice Vote `_supports ? 'yay' : 'nay'` in vote #`_voteId` * @param _voteId Id for vote * @param _supports Whether voter supports the vote - * @param _executesIfDecided Whether it should execute the vote if it becomes decided + * @param _executesIfDecided Whether the vote should execute its action if it becomes decided */ function vote(uint256 _voteId, bool _supports, bool _executesIfDecided) external { require(canVote(_voteId, msg.sender)); @@ -237,7 +237,7 @@ contract Voting is IForwarder, AragonApp { { Vote storage vote = votes[_voteId]; - // this could re-enter, though we can asume the governance token is not maliciuous + // this could re-enter, though we can assume the governance token is not malicious uint256 voterStake = token.balanceOfAt(_voter, vote.snapshotBlock); VoterState state = vote.voters[_voter]; @@ -281,7 +281,7 @@ contract Voting is IForwarder, AragonApp { } /** - * @dev Calculates whether `_value` is at least a percent `_pct` over `_total` + * @dev Calculates whether `_value` is at least a percentage `_pct` of `_total` */ function _isValuePct(uint256 _value, uint256 _total, uint256 _pct) internal pure returns (bool) { if (_value == 0 && _total > 0)