Track @next contract changes (Aragon 0.7-part.2)

apps/finance/app/src/App.js

@@ -49,13 +49,10 @@ class App extends React.Component {
   }
   handleWithdraw = (tokenAddress, recipient, amount, reference) => {
     // Immediate, one-time payment
-    this.props.app.newPayment(
+    this.props.app.newImmediatePayment(
       tokenAddress,
       recipient,
       amount,
-      0, // initial payment time
-      0, // interval
-      1, // max repeats
       reference
     )
     this.handleNewTransferClose()

apps/finance/arapp.json

@@ -49,7 +49,8 @@
         "Receiver address",
         "Token amount",
         "Payment interval",
-        "Max repeats"
+        "Max repeats",
+        "Initial payment time"
       ]
     },
     {

apps/finance/contracts/test/mocks/FinanceMock.sol

@@ -4,13 +4,14 @@ import "../../Finance.sol";
 
 
 contract FinanceMock is Finance {
-    uint64 mockTime = uint64(now);
+    uint64 mockTime;
     uint64 mockMaxPeriodTransitions = MAX_UINT64;
 
     function mock_setMaxPeriodTransitions(uint64 i) public { mockMaxPeriodTransitions = i; }
     function mock_setTimestamp(uint64 i) public { mockTime = i; }
-    function getMaxUint64() public pure returns (uint64) { return MAX_UINT64; }
 
     function getMaxPeriodTransitions() internal view returns (uint64) { return mockMaxPeriodTransitions; }
     function getTimestamp64() internal view returns (uint64) { return mockTime; }
+
+    function getMaxUint64() public pure returns (uint64) { return MAX_UINT64; }
 }

apps/survey/contracts/Survey.sol

@@ -133,6 +133,7 @@ contract Survey is AragonApp {
         require(votingPower > 0, ERROR_NO_VOTING_POWER);
 
         surveyId = surveysLength++;
+
         SurveyStruct storage survey = surveys[surveyId];
         survey.startDate = getTimestamp64();
         survey.snapshotBlock = snapshotBlock; // avoid double voting in this very block
@@ -167,6 +168,7 @@ contract Survey is AragonApp {
         external
         surveyExists(_surveyId)
     {
+        require(_optionIds.length == _stakes.length && _optionIds.length > 0, ERROR_VOTE_WRONG_INPUT);
         require(canVote(_surveyId, msg.sender), ERROR_CAN_NOT_VOTE);
 
         _voteOptions(_surveyId, _optionIds, _stakes);
@@ -181,7 +183,6 @@ contract Survey is AragonApp {
     * @param _optionId Index of supported option
     */
     function voteOption(uint256 _surveyId, uint256 _optionId) external surveyExists(_surveyId) {
-        require(_optionId != ABSTAIN_VOTE, ERROR_VOTE_WHOLE_WRONG_OPTION);
         require(canVote(_surveyId, msg.sender), ERROR_CAN_NOT_VOTE);
 
         SurveyStruct storage survey = surveys[_surveyId];
@@ -300,16 +301,15 @@ contract Survey is AragonApp {
     * @dev Assumes the survey exists and that msg.sender can vote
     */
     function _voteOptions(uint256 _surveyId, uint256[] _optionIds, uint256[] _stakes) internal {
-        require(_optionIds.length == _stakes.length && _optionIds.length > 0, ERROR_VOTE_WRONG_INPUT);
-
         SurveyStruct storage survey = surveys[_surveyId];
+        MultiOptionVote storage senderVotes = survey.votes[msg.sender];
 
         // Revert previous votes, if any
         _resetVote(_surveyId);
 
         uint256 totalVoted = 0;
         // Reserve first index for ABSTAIN_VOTE
-        survey.votes[msg.sender].castedVotes[0] = OptionCast({ optionId: ABSTAIN_VOTE, stake: 0 });
+        senderVotes.castedVotes[0] = OptionCast({ optionId: ABSTAIN_VOTE, stake: 0 });
         for (uint256 optionIndex = 1; optionIndex <= _optionIds.length; optionIndex++) {
             // Voters don't specify that they're abstaining,
             // but we still keep track of this by reserving the first index of a survey's votes.
@@ -322,10 +322,10 @@ contract Survey is AragonApp {
             // Let's avoid repeating an option by making sure that ascending order is preserved in
             // the options array by checking that the current optionId is larger than the last one
             // we added
-            require(survey.votes[msg.sender].castedVotes[optionIndex - 1].optionId < optionId, ERROR_OPTIONS_NOT_ORDERED);
+            require(senderVotes.castedVotes[optionIndex - 1].optionId < optionId, ERROR_OPTIONS_NOT_ORDERED);
 
             // Register voter amount
-            survey.votes[msg.sender].castedVotes[optionIndex] = OptionCast({ optionId: optionId, stake: stake });
+            senderVotes.castedVotes[optionIndex] = OptionCast({ optionId: optionId, stake: stake });
 
             // Add to total option support
             survey.optionPower[optionId] = survey.optionPower[optionId].add(stake);
@@ -340,10 +340,10 @@ contract Survey is AragonApp {
         // Implictly we are doing require(totalVoted <= voterStake) too
         // (as stated before, index 0 is for ABSTAIN_VOTE option)
         uint256 voterStake = token.balanceOfAt(msg.sender, survey.snapshotBlock);
-        survey.votes[msg.sender].castedVotes[0].stake = voterStake.sub(totalVoted);
+        senderVotes.castedVotes[0].stake = voterStake.sub(totalVoted);
 
         // Register number of options voted
-        survey.votes[msg.sender].optionsCastedLength = _optionIds.length;
+        senderVotes.optionsCastedLength = _optionIds.length;
 
         // Add voter tokens to participation
         survey.participation = survey.participation.add(totalVoted);

apps/token-manager/contracts/TokenManager.sol

@@ -19,7 +19,6 @@ import "@aragon/apps-shared-minime/contracts/MiniMeToken.sol";
 
 contract TokenManager is ITokenController, IForwarder, AragonApp {
     using SafeMath for uint256;
-    using Uint256Helpers for uint256;
 
     bytes32 public constant MINT_ROLE = keccak256("MINT_ROLE");
     bytes32 public constant ISSUE_ROLE = keccak256("ISSUE_ROLE");
@@ -29,18 +28,18 @@ contract TokenManager is ITokenController, IForwarder, AragonApp {
 
     uint256 public constant MAX_VESTINGS_PER_ADDRESS = 50;
 
+    string private constant ERROR_CALLER_NOT_TOKEN = "TM_CALLER_NOT_TOKEN";
     string private constant ERROR_NO_VESTING = "TM_NO_VESTING";
     string private constant ERROR_TOKEN_CONTROLLER = "TM_TOKEN_CONTROLLER";
-    string private constant ERROR_MINT_BALANCE_INCREASE_NOT_ALLOWED = "TM_MINT_BAL_INC_NOT_ALLOWED";
-    string private constant ERROR_ASSIGN_BALANCE_INCREASE_NOT_ALLOWED = "TM_ASSIGN_BAL_INC_NOT_ALLOWED";
+    string private constant ERROR_MINT_RECEIVER_IS_TM = "TM_MINT_RECEIVER_IS_TM";
+    string private constant ERROR_VESTING_TO_TM = "TM_VESTING_TO_TM";
     string private constant ERROR_TOO_MANY_VESTINGS = "TM_TOO_MANY_VESTINGS";
     string private constant ERROR_WRONG_CLIFF_DATE = "TM_WRONG_CLIFF_DATE";
     string private constant ERROR_VESTING_NOT_REVOKABLE = "TM_VESTING_NOT_REVOKABLE";
     string private constant ERROR_REVOKE_TRANSFER_FROM_REVERTED = "TM_REVOKE_TRANSFER_FROM_REVERTED";
-    string private constant ERROR_ASSIGN_TRANSFER_FROM_REVERTED = "TM_ASSIGN_TRANSFER_FROM_REVERTED";
     string private constant ERROR_CAN_NOT_FORWARD = "TM_CAN_NOT_FORWARD";
-    string private constant ERROR_ON_TRANSFER_WRONG_SENDER = "TM_TRANSFER_WRONG_SENDER";
-    string private constant ERROR_PROXY_PAYMENT_WRONG_SENDER = "TM_PROXY_PAYMENT_WRONG_SENDER";
+    string private constant ERROR_BALANCE_INCREASE_NOT_ALLOWED = "TM_BALANCE_INC_NOT_ALLOWED";
+    string private constant ERROR_ASSIGN_TRANSFER_FROM_REVERTED = "TM_ASSIGN_TRANSFER_FROM_REVERTED";
 
     struct TokenVesting {
         uint256 amount;
@@ -56,12 +55,16 @@ contract TokenManager is ITokenController, IForwarder, AragonApp {
     // We are mimicing an array in the inner mapping, we use a mapping instead to make app upgrade more graceful
     mapping (address => mapping (uint256 => TokenVesting)) internal vestings;
     mapping (address => uint256) public vestingsLengths;
-    mapping (address => bool) public everHeld;
 
     // Other token specific events can be watched on the token address directly (avoids duplication)
     event NewVesting(address indexed receiver, uint256 vestingId, uint256 amount);
     event RevokeVesting(address indexed receiver, uint256 vestingId, uint256 nonVestedAmount);
 
+    modifier onlyToken() {
+        require(msg.sender == address(token), ERROR_CALLER_NOT_TOKEN);
+        _;
+    }
+
     modifier vestingExists(address _holder, uint256 _vestingId) {
         // TODO: it's not checking for gaps that may appear because of deletes in revokeVesting function
         require(_vestingId < vestingsLengths[_holder], ERROR_NO_VESTING);
@@ -96,11 +99,11 @@ contract TokenManager is ITokenController, IForwarder, AragonApp {
 
     /**
     * @notice Mint `@tokenAmount(self.token(): address, _amount, false)` tokens for `_receiver`
-    * @param _receiver The address receiving the tokens
+    * @param _receiver The address receiving the tokens, cannot be the Token Manager itself (use `issue()` instead)
     * @param _amount Number of tokens minted
     */
     function mint(address _receiver, uint256 _amount) external authP(MINT_ROLE, arr(_receiver, _amount)) {
-        require(_isBalanceIncreaseAllowed(_receiver, _amount), ERROR_MINT_BALANCE_INCREASE_NOT_ALLOWED);
+        require(_receiver != address(this), ERROR_MINT_RECEIVER_IS_TM);
         _mint(_receiver, _amount);
     }
 
@@ -133,7 +136,7 @@ contract TokenManager is ITokenController, IForwarder, AragonApp {
 
     /**
     * @notice Assign `@tokenAmount(self.token(): address, _amount, false)` tokens to `_receiver` from the Token Manager's holdings with a `_revokable : 'revokable' : ''` vesting starting at `@formatDate(_start)`, cliff at `@formatDate(_cliff)` (first portion of tokens transferable), and completed vesting at `@formatDate(_vested)` (all tokens transferable)
-    * @param _receiver The address receiving the tokens
+    * @param _receiver The address receiving the tokens, cannot be Token Manager itself
     * @param _amount Number of tokens vested
     * @param _start Date the vesting calculations start
     * @param _cliff Date when the initial portion of tokens are transferable
@@ -152,8 +155,8 @@ contract TokenManager is ITokenController, IForwarder, AragonApp {
         authP(ASSIGN_ROLE, arr(_receiver, _amount))
         returns (uint256)
     {
+        require(_receiver != address(this), ERROR_VESTING_TO_TM);
         require(vestingsLengths[_receiver] < MAX_VESTINGS_PER_ADDRESS, ERROR_TOO_MANY_VESTINGS);
-
         require(_start <= _cliff && _cliff <= _vested, ERROR_WRONG_CLIFF_DATE);
 
         uint256 vestingId = vestingsLengths[_receiver]++;
@@ -187,13 +190,14 @@ contract TokenManager is ITokenController, IForwarder, AragonApp {
 
         uint256 nonVested = _calculateNonVestedTokens(
             v.amount,
-            getTimestamp64(),
+            getTimestamp(),
             v.start,
             v.cliff,
             v.vesting
         );
 
         // To make vestingIds immutable over time, we just zero out the revoked vesting
+        // Clearing this out also allows the token transfer back to the Token Manager to succeed
         delete vestings[_holder][_vestingId];
 
         // transferFrom always works as controller
@@ -231,46 +235,38 @@ contract TokenManager is ITokenController, IForwarder, AragonApp {
     }
 
     // ITokenController fns
+    // `onTransfer()`, `onApprove()`, and `proxyPayment()` are callbacks from the MiniMe token
+    // contract and are only meant to be called through the managed MiniMe token that gets assigned
+    // during initialization.
 
     /*
-    * @dev Notifies the controller about a token transfer allowing the controller to decide whether to allow it or react if desired (only callable from the token)
+    * @dev Notifies the controller about a token transfer allowing the controller to decide whether
+    *      to allow it or react if desired (only callable from the token).
+    *      Initialization check is implicitly provided by `onlyToken()`.
     * @param _from The origin of the transfer
     * @param _to The destination of the transfer
     * @param _amount The amount of the transfer
     * @return False if the controller does not authorize the transfer
     */
-    function onTransfer(address _from, address _to, uint _amount) public isInitialized returns (bool) {
-        require(msg.sender == address(token), ERROR_ON_TRANSFER_WRONG_SENDER);
-
-        bool includesTokenManager = _from == address(this) || _to == address(this);
-
-        if (!includesTokenManager) {
-            bool toCanReceive = _isBalanceIncreaseAllowed(_to, _amount);
-            if (!toCanReceive || transferableBalance(_from, now) < _amount) {
-                return false;
-            }
-        }
-
-        return true;
+    function onTransfer(address _from, address _to, uint256 _amount) public onlyToken returns (bool) {
+        return _isBalanceIncreaseAllowed(_to, _amount) && _transferableBalance(_from, getTimestamp()) >= _amount;
     }
 
     /**
     * @dev Notifies the controller about an approval allowing the controller to react if desired
+    *      Initialization check is implicitly provided by `onlyToken()`.
     * @return False if the controller does not authorize the approval
     */
-    function onApprove(address, address, uint) public returns (bool) {
+    function onApprove(address, address, uint) public onlyToken returns (bool) {
         return true;
     }
 
     /**
-    * @notice Called when ether is sent to the MiniMe Token contract
+    * @dev Called when ether is sent to the MiniMe Token contract
+    *      Initialization check is implicitly provided by `onlyToken()`.
     * @return True if the ether is accepted, false for it to throw
     */
-    function proxyPayment(address) public payable returns (bool) {
-        // Sender check is required to avoid anyone sending ETH to the Token Manager through this method
-        // 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), ERROR_PROXY_PAYMENT_WRONG_SENDER);
+    function proxyPayment(address) public payable onlyToken returns (bool) {
         return false;
     }
 
@@ -299,27 +295,12 @@ contract TokenManager is ITokenController, IForwarder, AragonApp {
         revokable = tokenVesting.revokable;
     }
 
-    function spendableBalanceOf(address _holder) public view returns (uint256) {
-        return transferableBalance(_holder, now);
+    function spendableBalanceOf(address _holder) public view isInitialized returns (uint256) {
+        return _transferableBalance(_holder, getTimestamp());
     }
 
-    function transferableBalance(address _holder, uint256 _time) public view returns (uint256) {
-        uint256 vestingsCount = vestingsLengths[_holder];
-        uint256 totalNonTransferable = 0;
-
-        for (uint256 i = 0; i < vestingsCount; i++) {
-            TokenVesting storage v = vestings[_holder][i];
-            uint nonTransferable = _calculateNonVestedTokens(
-                v.amount,
-                _time.toUint64(),
-                v.start,
-                v.cliff,
-                v.vesting
-            );
-            totalNonTransferable = totalNonTransferable.add(nonTransferable);
-        }
-
-        return token.balanceOf(_holder).sub(totalNonTransferable);
+    function transferableBalance(address _holder, uint256 _time) public view isInitialized returns (uint256) {
+        return _transferableBalance(_holder, _time);
     }
 
     /**
@@ -333,16 +314,21 @@ contract TokenManager is ITokenController, IForwarder, AragonApp {
     // Internal fns
 
     function _assign(address _receiver, uint256 _amount) internal {
-        require(_isBalanceIncreaseAllowed(_receiver, _amount), ERROR_ASSIGN_BALANCE_INCREASE_NOT_ALLOWED);
+        require(_isBalanceIncreaseAllowed(_receiver, _amount), ERROR_BALANCE_INCREASE_NOT_ALLOWED);
         // Must use transferFrom() as transfer() does not give the token controller full control
-        require(token.transferFrom(this, _receiver, _amount), ERROR_ASSIGN_TRANSFER_FROM_REVERTED);
+        require(token.transferFrom(address(this), _receiver, _amount), ERROR_ASSIGN_TRANSFER_FROM_REVERTED);
     }
 
     function _mint(address _receiver, uint256 _amount) internal {
+        require(_isBalanceIncreaseAllowed(_receiver, _amount), ERROR_BALANCE_INCREASE_NOT_ALLOWED);
         token.generateTokens(_receiver, _amount); // minime.generateTokens() never returns false
     }
 
-    function _isBalanceIncreaseAllowed(address _receiver, uint _inc) internal view returns (bool) {
+    function _isBalanceIncreaseAllowed(address _receiver, uint256 _inc) internal view returns (bool) {
+        // Max balance doesn't apply to the token manager itself
+        if (_receiver == address(this)) {
+            return true;
+        }
         return token.balanceOf(_receiver).add(_inc) <= maxAccountTokens;
     }
 
@@ -402,4 +388,30 @@ contract TokenManager is ITokenController, IForwarder, AragonApp {
         // tokens - vestedTokens
         return tokens.sub(vestedTokens);
     }
+
+    function _transferableBalance(address _holder, uint256 _time) internal view returns (uint256) {
+        uint256 transferable = token.balanceOf(_holder);
+
+        // This check is not strictly necessary for the current version of this contract, as
+        // Token Managers now cannot assign vestings to themselves.
+        // However, this was a possibility in the past, so in case there were vestings assigned to
+        // themselves, this will still return the correct value (entire balance, as the Token
+        // Manager does not have a spending limit on its own balance).
+        if (_holder != address(this)) {
+            uint256 vestingsCount = vestingsLengths[_holder];
+            for (uint256 i = 0; i < vestingsCount; i++) {
+                TokenVesting storage v = vestings[_holder][i];
+                uint256 nonTransferable = _calculateNonVestedTokens(
+                    v.amount,
+                    _time,
+                    v.start,
+                    v.cliff,
+                    v.vesting
+                );
+                transferable = transferable.sub(nonTransferable);
+            }
+        }
+
+        return transferable;
+    }
 }

apps/token-manager/contracts/test/mocks/TokenManagerMock.sol

@@ -0,0 +1,11 @@
+pragma solidity 0.4.24;
+
+import "../../TokenManager.sol";
+
+
+contract TokenManagerMock is TokenManager {
+    uint256 mockTime;
+
+    function mock_setTimestamp(uint256 i) public { mockTime = i; }
+    function getTimestamp() internal view returns (uint256) { return mockTime; }
+}