diff --git a/apps/finance/app/src/App.js b/apps/finance/app/src/App.js index 7f14338f2f..ff9b865f80 100644 --- a/apps/finance/app/src/App.js +++ b/apps/finance/app/src/App.js @@ -34,13 +34,10 @@ class App extends React.Component { } handleWithdraw = (tokenAddress, recipient, amount, reference) => { // Immediate, one-time payment - this.props.api.newPayment( + this.props.api.newImmediatePayment( tokenAddress, recipient, amount, - 0, // initial payment time - 0, // interval - 1, // max repeats reference ) this.handleNewTransferClose() diff --git a/apps/finance/arapp.json b/apps/finance/arapp.json index 8a71fd921f..953973493c 100644 --- a/apps/finance/arapp.json +++ b/apps/finance/arapp.json @@ -49,7 +49,8 @@ "Receiver address", "Token amount", "Payment interval", - "Max repeats" + "Max repeats", + "Initial payment time" ] }, { diff --git a/apps/finance/contracts/Finance.sol b/apps/finance/contracts/Finance.sol index 96a6722d5f..ac73dd6a72 100644 --- a/apps/finance/contracts/Finance.sol +++ b/apps/finance/contracts/Finance.sol @@ -27,34 +27,37 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { bytes32 public constant EXECUTE_PAYMENTS_ROLE = keccak256("EXECUTE_PAYMENTS_ROLE"); bytes32 public constant MANAGE_PAYMENTS_ROLE = keccak256("MANAGE_PAYMENTS_ROLE"); - uint256 internal constant NO_RECURRING_PAYMENT = 0; + uint256 internal constant NO_SCHEDULED_PAYMENT = 0; uint256 internal constant NO_TRANSACTION = 0; - uint256 internal constant MAX_RECURRING_PAYMENTS_PER_TX = 20; - uint256 internal constant MAX_UINT = uint256(-1); + uint256 internal constant MAX_SCHEDULED_PAYMENTS_PER_TX = 20; + uint256 internal constant MAX_UINT256 = uint256(-1); uint64 internal constant MAX_UINT64 = uint64(-1); + uint64 internal constant MINIMUM_PERIOD = uint64(1 days); string private constant ERROR_COMPLETE_TRANSITION = "FINANCE_COMPLETE_TRANSITION"; - string private constant ERROR_NO_RECURRING_PAYMENT = "FINANCE_NO_RECURRING_PAYMENT"; + string private constant ERROR_NO_SCHEDULED_PAYMENT = "FINANCE_NO_SCHEDULED_PAYMENT"; string private constant ERROR_NO_TRANSACTION = "FINANCE_NO_TRANSACTION"; string private constant ERROR_NO_PERIOD = "FINANCE_NO_PERIOD"; string private constant ERROR_VAULT_NOT_CONTRACT = "FINANCE_VAULT_NOT_CONTRACT"; - string private constant ERROR_INIT_PERIOD_TOO_SHORT = "FINANCE_INIT_PERIOD_TOO_SHORT"; string private constant ERROR_SET_PERIOD_TOO_SHORT = "FINANCE_SET_PERIOD_TOO_SHORT"; string private constant ERROR_NEW_PAYMENT_AMOUNT_ZERO = "FINANCE_NEW_PAYMENT_AMOUNT_ZERO"; + string private constant ERROR_NEW_PAYMENT_INTERVAL_ZERO = "FINANCE_NEW_PAYMENT_INTRVL_ZERO"; + string private constant ERROR_NEW_PAYMENT_EXECS_ZERO = "FINANCE_NEW_PAYMENT_EXECS_ZERO"; + string private constant ERROR_NEW_PAYMENT_IMMEDIATE = "FINANCE_NEW_PAYMENT_IMMEDIATE"; string private constant ERROR_RECOVER_AMOUNT_ZERO = "FINANCE_RECOVER_AMOUNT_ZERO"; string private constant ERROR_DEPOSIT_AMOUNT_ZERO = "FINANCE_DEPOSIT_AMOUNT_ZERO"; + string private constant ERROR_ETH_VALUE_MISMATCH = "FINANCE_ETH_VALUE_MISMATCH"; string private constant ERROR_BUDGET = "FINANCE_BUDGET"; + string private constant ERROR_EXECUTE_PAYMENT_NUM = "FINANCE_EXECUTE_PAYMENT_NUM"; string private constant ERROR_EXECUTE_PAYMENT_TIME = "FINANCE_EXECUTE_PAYMENT_TIME"; - string private constant ERROR_RECEIVER_EXECUTE_PAYMENT_TIME = "FINANCE_RCVR_EXEC_PAYMENT_TIME"; string private constant ERROR_PAYMENT_RECEIVER = "FINANCE_PAYMENT_RECEIVER"; string private constant ERROR_TOKEN_TRANSFER_FROM_REVERTED = "FINANCE_TKN_TRANSFER_FROM_REVERT"; - string private constant ERROR_VALUE_MISMATCH = "FINANCE_VALUE_MISMATCH"; string private constant ERROR_TOKEN_APPROVE_FAILED = "FINANCE_TKN_APPROVE_FAILED"; - string private constant ERROR_RECURRING_PAYMENT_INACTIVE = "FINANCE_RECURRING_PAYMENT_INACTIVE"; + string private constant ERROR_PAYMENT_INACTIVE = "FINANCE_PAYMENT_INACTIVE"; string private constant ERROR_REMAINING_BUDGET = "FINANCE_REMAINING_BUDGET"; // Order optimized for storage - struct RecurringPayment { + struct ScheduledPayment { address token; address receiver; address createdBy; @@ -62,8 +65,8 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { uint256 amount; uint64 initialPaymentTime; uint64 interval; - uint64 maxRepeats; - uint64 repeats; + uint64 maxExecutions; + uint64 executions; } // Order optimized for storage @@ -73,7 +76,7 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { bool isIncoming; uint256 amount; uint256 paymentId; - uint64 paymentRepeatNumber; + uint64 paymentExecutionNumber; uint64 date; uint64 periodId; } @@ -101,9 +104,9 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { Settings internal settings; // We are mimicing arrays, we use mappings instead to make app upgrade more graceful - mapping (uint256 => RecurringPayment) internal recurringPayments; - // Payments start at index 1, to allow us to use recurringPayments[0] for transactions that are not - // linked to a recurring payment + mapping (uint256 => ScheduledPayment) internal scheduledPayments; + // Payments start at index 1, to allow us to use scheduledPayments[0] for transactions that are not + // linked to a scheduled payment uint256 public paymentsNextIndex; mapping (uint256 => Transaction) internal transactions; @@ -114,9 +117,9 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { event NewPeriod(uint64 indexed periodId, uint64 periodStarts, uint64 periodEnds); event SetBudget(address indexed token, uint256 amount, bool hasBudget); - event NewPayment(uint256 indexed paymentId, address indexed recipient, uint64 maxRepeats, string reference); + event NewPayment(uint256 indexed paymentId, address indexed recipient, uint64 maxExecutions, string reference); event NewTransaction(uint256 indexed transactionId, bool incoming, address indexed entity, uint256 amount, string reference); - event ChangePaymentState(uint256 indexed paymentId, bool inactive); + event ChangePaymentState(uint256 indexed paymentId, bool active); event ChangePeriodDuration(uint64 newDuration); event PaymentFailure(uint256 paymentId); @@ -129,8 +132,8 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { _; } - modifier recurringPaymentExists(uint256 _paymentId) { - require(_paymentId > 0 && _paymentId < paymentsNextIndex, ERROR_NO_RECURRING_PAYMENT); + modifier scheduledPaymentExists(uint256 _paymentId) { + require(_paymentId > 0 && _paymentId < paymentsNextIndex, ERROR_NO_SCHEDULED_PAYMENT); _; } @@ -145,10 +148,11 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { } /** - * @dev Sends ETH to Vault. Sends all the available balance. * @notice Deposit ETH to the Vault, to avoid locking them in this Finance app forever + * @dev Send ETH to Vault. Send all the available balance. */ function () external payable isInitialized transitionsPeriod { + require(msg.value > 0, ERROR_DEPOSIT_AMOUNT_ZERO); _deposit( ETH, msg.value, @@ -169,12 +173,12 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { require(isContract(_vault), ERROR_VAULT_NOT_CONTRACT); vault = _vault; - require(_periodDuration >= 1 days, ERROR_INIT_PERIOD_TOO_SHORT); + require(_periodDuration >= MINIMUM_PERIOD, ERROR_SET_PERIOD_TOO_SHORT); settings.periodDuration = _periodDuration; - // Reserve the first recurring payment index as an unused index for transactions not linked - // to a payment - recurringPayments[0].inactive = true; + // Reserve the first scheduled payment index as an unused index for transactions not linked + // to a scheduled payment + scheduledPayments[0].inactive = true; paymentsNextIndex = 1; // Reserve the first transaction index as an unused index for periods with no transactions @@ -185,13 +189,19 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { } /** - * @dev Deposit for approved ERC20 tokens or ETH * @notice Deposit `@tokenAmount(_token, _amount)` + * @dev Deposit for approved ERC20 tokens or ETH * @param _token Address of deposited token * @param _amount Amount of tokens sent * @param _reference Reason for payment */ function deposit(address _token, uint256 _amount, string _reference) external payable isInitialized transitionsPeriod { + require(_amount > 0, ERROR_DEPOSIT_AMOUNT_ZERO); + if (_token == ETH) { + // Ensure that the ETH sent with the transaction equals the amount in the deposit + require(msg.value == _amount, ERROR_ETH_VALUE_MISMATCH); + } + _deposit( _token, _amount, @@ -202,62 +212,89 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { } /** - * @notice Create a new payment of `@tokenAmount(_token, _amount)` to `_receiver``_maxRepeats > 0 ? ', executing ' + _maxRepeats + ' times at intervals of ' + @transformTime(_interval) : ''`, for '`_reference`' + * @notice Create a new payment of `@tokenAmount(_token, _amount)` to `_receiver` for '`_reference`' + * @dev Note that this function is protected by the `CREATE_PAYMENTS_ROLE` but uses `MAX_UINT256` + * as its interval auth parameter (as a sentinel value for "never repeating"). + * While this protects against most cases (you typically want to set a baseline requirement + * for interval time), it does mean users will have to explicitly check for this case when + * granting a permission that includes a upperbound requirement on the interval time. * @param _token Address of token for payment * @param _receiver Address that will receive payment - * @param _amount Tokens that are payed every time the payment is due + * @param _amount Tokens that are paid every time the payment is due + * @param _reference String detailing payment reason + */ + function newImmediatePayment(address _token, address _receiver, uint256 _amount, string _reference) + external + // Use MAX_UINT256 as the interval parameter, as this payment will never repeat + // Payment time parameter is left as the last param as it was added later + authP(CREATE_PAYMENTS_ROLE, _arr(_token, _receiver, _amount, MAX_UINT256, uint256(1), getTimestamp())) + transitionsPeriod + { + require(_amount > 0, ERROR_NEW_PAYMENT_AMOUNT_ZERO); + + _makePaymentTransaction( + _token, + _receiver, + _amount, + NO_SCHEDULED_PAYMENT, // unrelated to any payment id; it isn't created + 0, // also unrelated to any payment executions + _reference + ); + } + + /** + * @notice Create a new payment of `@tokenAmount(_token, _amount)` to `_receiver` for `_reference`, executing `_maxExecutions` times at intervals of `@transformTime(_interval)` + * @dev See `newImmediatePayment()` for limitations on how the interval auth parameter can be used + * @param _token Address of token for payment + * @param _receiver Address that will receive payment + * @param _amount Tokens that are paid 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 _maxExecutions Maximum instances a payment can be executed * @param _reference String detailing payment reason */ - function newPayment( + function newScheduledPayment( address _token, address _receiver, uint256 _amount, uint64 _initialPaymentTime, uint64 _interval, - uint64 _maxRepeats, + uint64 _maxExecutions, string _reference ) external - authP(CREATE_PAYMENTS_ROLE, arr(_token, _receiver, _amount, _interval, _maxRepeats)) + // Payment time parameter is left as the last param as it was added later + authP(CREATE_PAYMENTS_ROLE, _arr(_token, _receiver, _amount, uint256(_interval), uint256(_maxExecutions), uint256(_initialPaymentTime))) transitionsPeriod returns (uint256 paymentId) { require(_amount > 0, ERROR_NEW_PAYMENT_AMOUNT_ZERO); + require(_interval > 0, ERROR_NEW_PAYMENT_INTERVAL_ZERO); + require(_maxExecutions > 0, ERROR_NEW_PAYMENT_EXECS_ZERO); - // Avoid saving payment data for 1 time immediate payments - if (_initialPaymentTime <= getTimestamp64() && _maxRepeats == 1) { - _makePaymentTransaction( - _token, - _receiver, - _amount, - NO_RECURRING_PAYMENT, // unrelated to any payment id; it isn't created - 0, // also unrelated to any payment repeats - _reference - ); - return; - } + // Token budget must not be set at all or allow at least one instance of this payment each period + require(!settings.hasBudget[_token] || settings.budgets[_token] >= _amount, ERROR_BUDGET); - // Budget must allow at least one instance of this payment each period, or not be set at all - require(settings.budgets[_token] >= _amount || !settings.hasBudget[_token], ERROR_BUDGET); + // Don't allow creating single payments that are immediately executable, use `newImmediatePayment()` instead + if (_maxExecutions == 1) { + require(_initialPaymentTime > getTimestamp64(), ERROR_NEW_PAYMENT_IMMEDIATE); + } paymentId = paymentsNextIndex++; - emit NewPayment(paymentId, _receiver, _maxRepeats, _reference); + emit NewPayment(paymentId, _receiver, _maxExecutions, _reference); - RecurringPayment storage payment = recurringPayments[paymentId]; + ScheduledPayment storage payment = scheduledPayments[paymentId]; payment.token = _token; payment.receiver = _receiver; payment.amount = _amount; payment.initialPaymentTime = _initialPaymentTime; payment.interval = _interval; - payment.maxRepeats = _maxRepeats; + payment.maxExecutions = _maxExecutions; payment.createdBy = msg.sender; - if (nextPaymentTime(paymentId) <= getTimestamp64()) { - _executePayment(paymentId); - } + // We skip checking how many times the new payment was executed to allow creating new + // scheduled payments before having enough vault balance + _executePayment(paymentId); } /** @@ -269,7 +306,7 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { authP(CHANGE_PERIOD_ROLE, arr(uint256(_periodDuration), uint256(settings.periodDuration))) transitionsPeriod { - require(_periodDuration >= 1 days, ERROR_SET_PERIOD_TOO_SHORT); + require(_periodDuration >= MINIMUM_PERIOD, ERROR_SET_PERIOD_TOO_SHORT); settings.periodDuration = _periodDuration; emit ChangePeriodDuration(_periodDuration); } @@ -284,7 +321,7 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { uint256 _amount ) external - authP(CHANGE_BUDGETS_ROLE, arr(_token, _amount, settings.budgets[_token], settings.hasBudget[_token] ? 1 : 0)) + authP(CHANGE_BUDGETS_ROLE, arr(_token, _amount, settings.budgets[_token], uint256(settings.hasBudget[_token] ? 1 : 0))) transitionsPeriod { settings.budgets[_token] = _amount; @@ -300,7 +337,7 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { */ function removeBudget(address _token) external - authP(CHANGE_BUDGETS_ROLE, arr(_token, uint256(0), settings.budgets[_token], settings.hasBudget[_token] ? 1 : 0)) + authP(CHANGE_BUDGETS_ROLE, arr(_token, uint256(0), settings.budgets[_token], uint256(settings.hasBudget[_token] ? 1 : 0))) transitionsPeriod { settings.budgets[_token] = 0; @@ -309,31 +346,29 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { } /** - * @dev Executes any payment (requires role) * @notice Execute pending payment #`_paymentId` + * @dev Executes any payment (requires role) * @param _paymentId Identifier for payment */ function executePayment(uint256 _paymentId) external - authP(EXECUTE_PAYMENTS_ROLE, arr(_paymentId, recurringPayments[_paymentId].amount)) - recurringPaymentExists(_paymentId) + authP(EXECUTE_PAYMENTS_ROLE, arr(_paymentId, scheduledPayments[_paymentId].amount)) + scheduledPaymentExists(_paymentId) transitionsPeriod { - require(nextPaymentTime(_paymentId) <= getTimestamp64(), ERROR_EXECUTE_PAYMENT_TIME); - - _executePayment(_paymentId); + _executePaymentAtLeastOnce(_paymentId); } /** - * @dev Always allows receiver of a payment to trigger execution * @notice Execute pending payment #`_paymentId` + * @dev Always allow receiver of a payment to trigger execution + * Initialization check is implicitly provided by `scheduledPaymentExists()` as new + * scheduled payments can only be created via `newScheduledPayment(),` which requires initialization * @param _paymentId Identifier for payment */ - function receiverExecutePayment(uint256 _paymentId) external isInitialized recurringPaymentExists(_paymentId) transitionsPeriod { - require(nextPaymentTime(_paymentId) <= getTimestamp64(), ERROR_RECEIVER_EXECUTE_PAYMENT_TIME); - require(recurringPayments[_paymentId].receiver == msg.sender, ERROR_PAYMENT_RECEIVER); - - _executePayment(_paymentId); + function receiverExecutePayment(uint256 _paymentId) external scheduledPaymentExists(_paymentId) transitionsPeriod { + require(scheduledPayments[_paymentId].receiver == msg.sender, ERROR_PAYMENT_RECEIVER); + _executePaymentAtLeastOnce(_paymentId); } /** @@ -348,37 +383,37 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { function setPaymentStatus(uint256 _paymentId, bool _active) external authP(MANAGE_PAYMENTS_ROLE, arr(_paymentId, uint256(_active ? 1 : 0))) - recurringPaymentExists(_paymentId) + scheduledPaymentExists(_paymentId) { - recurringPayments[_paymentId].inactive = !_active; + scheduledPayments[_paymentId].inactive = !_active; emit ChangePaymentState(_paymentId, _active); } /** + * @notice Send tokens held in this contract to the 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 recoverToVault(address _token) external isInitialized transitionsPeriod { - uint256 amount = _token == ETH ? address(this).balance : ERC20(_token).staticBalanceOf(this); + uint256 amount = _token == ETH ? address(this).balance : ERC20(_token).staticBalanceOf(address(this)); require(amount > 0, ERROR_RECOVER_AMOUNT_ZERO); _deposit( _token, amount, "Recover to Vault", - this, + address(this), false ); } /** + * @notice Transition accounting period if needed * @dev Transitions accounting periods if needed. For preventing OOG attacks, a maxTransitions * param is provided. If more than the specified number of periods need to be transitioned, * it will return false. - * @notice Transition accounting period if needed * @param _maxTransitions Maximum periods that can be transitioned * @return success Boolean indicating whether the accounting period is the correct one (if false, * maxTransitions was surpased and another call is needed) @@ -401,28 +436,28 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { function getPayment(uint256 _paymentId) public view - recurringPaymentExists(_paymentId) + scheduledPaymentExists(_paymentId) returns ( address token, address receiver, uint256 amount, uint64 initialPaymentTime, uint64 interval, - uint64 maxRepeats, + uint64 maxExecutions, bool inactive, - uint64 repeats, + uint64 executions, address createdBy ) { - RecurringPayment storage payment = recurringPayments[_paymentId]; + ScheduledPayment storage payment = scheduledPayments[_paymentId]; token = payment.token; receiver = payment.receiver; amount = payment.amount; initialPaymentTime = payment.initialPaymentTime; interval = payment.interval; - maxRepeats = payment.maxRepeats; - repeats = payment.repeats; + maxExecutions = payment.maxExecutions; + executions = payment.executions; inactive = payment.inactive; createdBy = payment.createdBy; } @@ -435,7 +470,7 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { uint64 periodId, uint256 amount, uint256 paymentId, - uint64 paymentRepeatNumber, + uint64 paymentExecutionNumber, address token, address entity, bool isIncoming, @@ -451,7 +486,7 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { periodId = transaction.periodId; amount = transaction.amount; paymentId = transaction.paymentId; - paymentRepeatNumber = transaction.paymentRepeatNumber; + paymentExecutionNumber = transaction.paymentExecutionNumber; } function getPeriod(uint64 _periodId) @@ -487,11 +522,24 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { income = tokenStatement.income; } - function getPeriodDuration() public view returns (uint64) { + /** + * @dev We have to check for initialization as periods are only valid after initializing + */ + function currentPeriodId() public view isInitialized returns (uint64) { + return _currentPeriodId(); + } + + /** + * @dev We have to check for initialization as periods are only valid after initializing + */ + function getPeriodDuration() public view isInitialized returns (uint64) { return settings.periodDuration; } - function getBudget(address _token) public view returns (uint256 budget, bool hasBudget) { + /** + * @dev We have to check for initialization as budgets are only valid after initializing + */ + function getBudget(address _token) public view isInitialized returns (uint256 budget, bool hasBudget) { budget = settings.budgets[_token]; hasBudget = settings.hasBudget[_token]; } @@ -504,33 +552,23 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { } /** - * @dev We have to check for initialization as periods are only valid after initializing + * @dev We have to check for initialization as budgets are only valid after initializing */ - function currentPeriodId() public view isInitialized returns (uint64) { - return _currentPeriodId(); + function canMakePayment(address _token, uint256 _amount) public view isInitialized returns (bool) { + return _canMakePayment(_token, _amount); } /** - * @dev Initialization check is implicitly provided by `recurringPaymentExists()` as new - * recurring payments can only be created via `newPayment(),` which requires initialization + * @dev Initialization check is implicitly provided by `scheduledPaymentExists()` as new + * scheduled payments can only be created via `newScheduledPayment(),` which requires initialization */ - function nextPaymentTime(uint256 _paymentId) public view recurringPaymentExists(_paymentId) returns (uint64) { - RecurringPayment storage payment = recurringPayments[_paymentId]; - - if (payment.repeats >= payment.maxRepeats) { - return MAX_UINT64; // re-executes in some billions of years time... should not need to worry - } - - // Split in multiple lines to circunvent linter warning - uint64 increase = payment.repeats.mul(payment.interval); - uint64 nextPayment = payment.initialPaymentTime.add(increase); - return nextPayment; + function nextPaymentTime(uint256 _paymentId) public view scheduledPaymentExists(_paymentId) returns (uint64) { + return _nextPaymentTime(_paymentId); } // Internal fns function _deposit(address _token, uint256 _amount, string _reference, address _sender, bool _isExternalDeposit) internal { - require(_amount > 0, ERROR_DEPOSIT_AMOUNT_ZERO); _recordIncomingTransaction( _token, _sender, @@ -538,72 +576,64 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { _reference ); - // If it is an external deposit, check that the assets are actually transferred - // External deposit will be false when the assets were already in the Finance app - // and just need to be transferred to the vault - if (_isExternalDeposit) { - if (_token != ETH) { - // Get the tokens to Finance - require(ERC20(_token).safeTransferFrom(msg.sender, this, _amount), ERROR_TOKEN_TRANSFER_FROM_REVERTED); - } else { - // Ensure that the ETH sent with the transaction equals the amount in the deposit - require(msg.value == _amount, ERROR_VALUE_MISMATCH); - } - } - if (_token == ETH) { vault.deposit.value(_amount)(ETH, _amount); } else { + // First, transfer the tokens to Finance if necessary + // External deposit will be false when the assets were already in the Finance app + // and just need to be transferred to the Vault + if (_isExternalDeposit) { + // This assumes the sender has approved the tokens for Finance + require( + ERC20(_token).safeTransferFrom(msg.sender, address(this), _amount), + ERROR_TOKEN_TRANSFER_FROM_REVERTED + ); + } + // Approve the tokens for the Vault (it does the actual transferring) require(ERC20(_token).safeApprove(vault, _amount), ERROR_TOKEN_APPROVE_FAILED); - // finally we can deposit them + // Finally, initiate the deposit vault.deposit(_token, _amount); } } - function _newPeriod(uint64 _startTime) internal returns (Period storage) { - // There should be no way for this to overflow since each period is at least one day - uint64 newPeriodId = periodsLength++; - - Period storage period = periods[newPeriodId]; - period.startTime = _startTime; - - // Be careful here to not overflow; if startTime + periodDuration overflows, we set endTime - // to MAX_UINT64 (let's assume that's the end of time for now). - uint64 endTime = _startTime + settings.periodDuration - 1; - if (endTime < _startTime) { // overflowed - endTime = MAX_UINT64; - } - period.endTime = endTime; - - emit NewPeriod(newPeriodId, period.startTime, period.endTime); - - return period; - } - - function _executePayment(uint256 _paymentId) internal { - RecurringPayment storage payment = recurringPayments[_paymentId]; - require(!payment.inactive, ERROR_RECURRING_PAYMENT_INACTIVE); + function _executePayment(uint256 _paymentId) internal returns (uint256) { + ScheduledPayment storage payment = scheduledPayments[_paymentId]; + require(!payment.inactive, ERROR_PAYMENT_INACTIVE); - uint64 payed = 0; - while (nextPaymentTime(_paymentId) <= getTimestamp64() && payed < MAX_RECURRING_PAYMENTS_PER_TX) { + uint64 paid = 0; + while (_nextPaymentTime(_paymentId) <= getTimestamp64() && paid < MAX_SCHEDULED_PAYMENTS_PER_TX) { if (!_canMakePayment(payment.token, payment.amount)) { emit PaymentFailure(_paymentId); - return; + break; } // The while() predicate prevents these two from ever overflowing - payment.repeats += 1; - payed += 1; + payment.executions += 1; + paid += 1; - _makePaymentTransaction( + // We've already checked the remaining budget with `_canMakePayment()` + _unsafeMakePaymentTransaction( payment.token, payment.receiver, payment.amount, _paymentId, - payment.repeats, + payment.executions, "" ); } + + return paid; + } + + function _executePaymentAtLeastOnce(uint256 _paymentId) internal { + uint256 paid = _executePayment(_paymentId); + if (paid == 0) { + if (_nextPaymentTime(_paymentId) <= getTimestamp64()) { + revert(ERROR_EXECUTE_PAYMENT_NUM); + } else { + revert(ERROR_EXECUTE_PAYMENT_TIME); + } + } } function _makePaymentTransaction( @@ -611,25 +641,62 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { address _receiver, uint256 _amount, uint256 _paymentId, - uint64 _paymentRepeatNumber, + uint64 _paymentExecutionNumber, string _reference ) internal { require(_getRemainingBudget(_token) >= _amount, ERROR_REMAINING_BUDGET); + _unsafeMakePaymentTransaction(_token, _receiver, _amount, _paymentId, _paymentExecutionNumber, _reference); + } + + /** + * @dev Unsafe version of _makePaymentTransaction that assumes you have already checked the + * remaining budget + */ + function _unsafeMakePaymentTransaction( + address _token, + address _receiver, + uint256 _amount, + uint256 _paymentId, + uint64 _paymentExecutionNumber, + string _reference + ) + internal + { _recordTransaction( false, _token, _receiver, _amount, _paymentId, - _paymentRepeatNumber, + _paymentExecutionNumber, _reference ); vault.transfer(_token, _receiver, _amount); } + function _newPeriod(uint64 _startTime) internal returns (Period storage) { + // There should be no way for this to overflow since each period is at least one day + uint64 newPeriodId = periodsLength++; + + Period storage period = periods[newPeriodId]; + period.startTime = _startTime; + + // Be careful here to not overflow; if startTime + periodDuration overflows, we set endTime + // to MAX_UINT64 (let's assume that's the end of time for now). + uint64 endTime = _startTime + settings.periodDuration - 1; + if (endTime < _startTime) { // overflowed + endTime = MAX_UINT64; + } + period.endTime = endTime; + + emit NewPeriod(newPeriodId, period.startTime, period.endTime); + + return period; + } + function _recordIncomingTransaction( address _token, address _sender, @@ -643,8 +710,8 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { _token, _sender, _amount, - NO_RECURRING_PAYMENT, // unrelated to any existing payment - 0, // and no payment repeats + NO_SCHEDULED_PAYMENT, // unrelated to any existing payment + 0, // and no payment executions _reference ); } @@ -655,7 +722,7 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { address _entity, uint256 _amount, uint256 _paymentId, - uint64 _paymentRepeatNumber, + uint64 _paymentExecutionNumber, string _reference ) internal @@ -669,13 +736,14 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { } uint256 transactionId = transactionsNextIndex++; + Transaction storage transaction = transactions[transactionId]; transaction.token = _token; transaction.entity = _entity; transaction.isIncoming = _incoming; transaction.amount = _amount; transaction.paymentId = _paymentId; - transaction.paymentRepeatNumber = _paymentRepeatNumber; + transaction.paymentExecutionNumber = _paymentExecutionNumber; transaction.date = getTimestamp64(); transaction.periodId = periodId; @@ -687,7 +755,7 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { emit NewTransaction(transactionId, _incoming, _entity, _amount, _reference); } - function _tryTransitionAccountingPeriod(uint256 _maxTransitions) internal returns (bool success) { + function _tryTransitionAccountingPeriod(uint64 _maxTransitions) internal returns (bool success) { Period storage currentPeriod = periods[_currentPeriodId()]; uint64 timestamp = getTimestamp64(); @@ -698,7 +766,8 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { // it didn't fully transition return false; } - _maxTransitions = _maxTransitions.sub(1); + // We're already protected from underflowing above + _maxTransitions -= 1; // If there were any transactions in period, record which was the last // In case 0 transactions occured, first and last tx id will be 0 @@ -717,25 +786,52 @@ contract Finance is EtherTokenConstant, IsContract, AragonApp { return _getRemainingBudget(_token) >= _amount && vault.balance(_token) >= _amount; } + function _currentPeriodId() internal view returns (uint64) { + // There is no way for this to overflow if protected by an initialization check + return periodsLength - 1; + } + function _getRemainingBudget(address _token) internal view returns (uint256) { if (!settings.hasBudget[_token]) { - return MAX_UINT; + return MAX_UINT256; } + uint256 budget = settings.budgets[_token]; uint256 spent = periods[_currentPeriodId()].tokenStatement[_token].expenses; // A budget decrease can cause the spent amount to be greater than period budget // If so, return 0 to not allow more spending during period - if (spent >= settings.budgets[_token]) { + if (spent >= budget) { return 0; } - return settings.budgets[_token].sub(spent); + // We're already protected from the overflow above + return budget - spent; } - function _currentPeriodId() internal view returns (uint64) { - // There is no way for this to overflow if protected by an initialization check - return periodsLength - 1; + function _nextPaymentTime(uint256 _paymentId) internal view returns (uint64) { + ScheduledPayment storage payment = scheduledPayments[_paymentId]; + + if (payment.executions >= payment.maxExecutions) { + return MAX_UINT64; // re-executes in some billions of years time... should not need to worry + } + + // Split in multiple lines to circumvent linter warning + uint64 increase = payment.executions.mul(payment.interval); + uint64 nextPayment = payment.initialPaymentTime.add(increase); + return nextPayment; + } + + // Syntax sugar + + function _arr(address _a, address _b, uint256 _c, uint256 _d, uint256 _e, uint256 _f) internal pure returns (uint256[] r) { + r = new uint256[](6); + r[0] = uint256(_a); + r[1] = uint256(_b); + r[2] = _c; + r[3] = _d; + r[4] = _e; + r[5] = _f; } // Mocked fns (overrided during testing) diff --git a/apps/finance/contracts/test/mocks/FinanceMock.sol b/apps/finance/contracts/test/mocks/FinanceMock.sol index c5d44c4184..9a6b029fc9 100644 --- a/apps/finance/contracts/test/mocks/FinanceMock.sol +++ b/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; } } diff --git a/apps/finance/test/finance.js b/apps/finance/test/finance.js index d23e093dfe..88acd1ace4 100644 --- a/apps/finance/test/finance.js +++ b/apps/finance/test/finance.js @@ -1,3 +1,4 @@ +const assertEvent = require('@aragon/test-helpers/assertEvent') const { assertRevert, assertInvalidOpcode } = require('@aragon/test-helpers/assertThrow') const getBalance = require('@aragon/test-helpers/balance')(web3) @@ -14,6 +15,7 @@ const getContract = name => artifacts.require(name) const getEventData = (receipt, event, arg) => receipt.logs.filter(log => log.event == event)[0].args[arg] + // Tests for different token interfaces const tokenTestGroups = [ { @@ -91,6 +93,7 @@ contract('Finance App', accounts => { // finance const receipt2 = await dao.newAppInstance('0x5678', financeBase.address, '0x', false, { from: root }) const financeApp = Finance.at(receipt2.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + await financeApp.mock_setTimestamp(START_TIME) await financeApp.mock_setMaxPeriodTransitions(MAX_UINT64) await acl.createPermission(ANY_ENTITY, financeApp.address, CREATE_PAYMENTS_ROLE, root, { from: root }) @@ -128,7 +131,6 @@ contract('Finance App', accounts => { await token2.transfer(vault.address, VAULT_INITIAL_TOKEN2_BALANCE) await vault.deposit(ETH, VAULT_INITIAL_ETH_BALANCE, { value: VAULT_INITIAL_ETH_BALANCE, from: accounts[0] }); - await finance.mock_setTimestamp(START_TIME) await finance.initialize(vault.address, PERIOD_DURATION) }) @@ -170,9 +172,7 @@ contract('Finance App', accounts => { it('fails on initializing with less than one day period', async () => { const badPeriod = 60 * 60 * 24 - 1 - const { financeApp } = await newProxyFinance() - await financeApp.mock_setTimestamp(START_TIME) return assertRevert(() => financeApp.initialize(vault.address, badPeriod)) }) @@ -194,7 +194,7 @@ contract('Finance App', accounts => { await finance.mock_setTimestamp(time) - await finance.newPayment(token2.address, recipient, amount, time, 0, 1, '') + await finance.newImmediatePayment(token2.address, recipient, amount, '') assert.equal((await token2.balanceOf(recipient)).valueOf(), amount, 'recipient should have received tokens') }) @@ -225,7 +225,7 @@ contract('Finance App', accounts => { assert.equal(await finance.currentPeriodId(), 1, 'should have transitioned 1 periods') }) - for ({ title, tokenContract} of tokenTestGroups) { + for (const { title, tokenContract} of tokenTestGroups) { context(`ERC20 (${title}) deposits`, () => { const transferAmount = 5 let tokenInstance @@ -243,11 +243,11 @@ contract('Finance App', accounts => { // vault has 100 tokens initially assert.equal((await tokenInstance.balanceOf(vault.address)).valueOf(), VAULT_INITIAL_TOKEN1_BALANCE + transferAmount, 'deposited tokens must be in vault') - const [periodId, amount, paymentId, paymentRepeatNumber, token, entity, incoming, date] = await finance.getTransaction(1) + const [periodId, amount, paymentId, paymentExecutionNumber, token, entity, incoming, date] = await finance.getTransaction(1) assert.equal(periodId, 0, 'period id should be correct') assert.equal(amount, transferAmount, 'amount should be correct') assert.equal(paymentId, 0, 'payment id should be 0') - assert.equal(paymentRepeatNumber, 0, 'payment repeat number should be 0') + assert.equal(paymentExecutionNumber, 0, 'payment execution number should be 0') assert.equal(token, tokenInstance.address, 'token should be correct') assert.equal(entity, accounts[0], 'entity should be correct') assert.isTrue(incoming, 'tx should be incoming') @@ -272,13 +272,13 @@ contract('Finance App', accounts => { const transactionId = receipt.logs.filter(log => log.event == 'NewTransaction')[0].args.transactionId - const [periodId, amount, paymentId, paymentRepeatNumber, token, entity, incoming, date] = await finance.getTransaction(transactionId) + const [periodId, amount, paymentId, paymentExecutionNumber, token, entity, incoming, date] = await finance.getTransaction(transactionId) assert.equal(await vault.balance(ETH), VAULT_INITIAL_ETH_BALANCE + sentWei, 'deposited ETH must be in vault') assert.equal(periodId, 0, 'period id should be correct') assert.equal(amount, sentWei, 'amount should be correct') assert.equal(paymentId, 0, 'payment id should be 0') - assert.equal(paymentRepeatNumber, 0, 'payment repeat number should be 0') + assert.equal(paymentExecutionNumber, 0, 'payment execution number should be 0') assert.equal(token, ETH, 'token should be ETH token') assert.equal(entity, accounts[0], 'entity should be correct') assert.isTrue(incoming, 'tx should be incoming') @@ -290,13 +290,13 @@ contract('Finance App', accounts => { const receipt = await finance.send(sentWei) const transactionId = receipt.logs.filter(log => log.event == 'NewTransaction')[0].args.transactionId - const [periodId, amount, paymentId, paymentRepeatNumber, token, entity, incoming, date] = await finance.getTransaction(transactionId) + const [periodId, amount, paymentId, paymentExecutionNumber, token, entity, incoming, date] = await finance.getTransaction(transactionId) assert.equal(await vault.balance(ETH), VAULT_INITIAL_ETH_BALANCE + sentWei, 'deposited ETH must be in vault') assert.equal(periodId, 0, 'period id should be correct') assert.equal(amount, sentWei, 'amount should be correct') assert.equal(paymentId, 0, 'payment id should be 0') - assert.equal(paymentRepeatNumber, 0, 'payment repeat number should be 0') + assert.equal(paymentExecutionNumber, 0, 'payment execution number should be 0') assert.equal(token, ETH, 'token should be ETH token') assert.equal(entity, accounts[0], 'entity should be correct') assert.isTrue(incoming, 'tx should be incoming') @@ -305,7 +305,7 @@ contract('Finance App', accounts => { }) }) - for ({ title, tokenContract} of tokenTestGroups) { + for (const { title, tokenContract} of tokenTestGroups) { context(`locked ERC20 (${title})`, () => { const lockedTokenAmount = 5 let tokenInstance @@ -329,11 +329,11 @@ contract('Finance App', accounts => { assert.equal((await tokenInstance.balanceOf(vault.address)).valueOf(), VAULT_INITIAL_TOKEN1_BALANCE + lockedTokenAmount, 'deposited tokens must be in vault') assert.equal(await tokenInstance.balanceOf(finance.address), 0, 'finance shouldn\'t have tokens') - const [periodId, amount, paymentId, paymentRepeatNumber, token, entity, incoming, date] = await finance.getTransaction(1) + const [periodId, amount, paymentId, paymentExecutionNumber, token, entity, incoming, date] = await finance.getTransaction(1) assert.equal(periodId, 0, 'period id should be correct') assert.equal(amount, lockedTokenAmount, 'amount should be correct') assert.equal(paymentId, 0, 'payment id should be 0') - assert.equal(paymentRepeatNumber, 0, 'payment repeat number should be 0') + assert.equal(paymentExecutionNumber, 0, 'payment execution number should be 0') assert.equal(token, tokenInstance.address, 'token should be correct') assert.equal(entity, finance.address, 'entity should be correct') assert.isTrue(incoming, 'tx should be incoming') @@ -371,14 +371,14 @@ contract('Finance App', accounts => { it('is recovered using Finance#recoverToVault', async () => { const receipt = await finance.recoverToVault(ETH) - const [periodId, amount, paymentId, paymentRepeatNumber, token, entity, incoming, date] = await finance.getTransaction(1) + const [periodId, amount, paymentId, paymentExecutionNumber, token, entity, incoming, date] = await finance.getTransaction(1) assert.equal(await vault.balance(ETH), VAULT_INITIAL_ETH_BALANCE + lockedETH, 'recovered ETH must be in vault') assert.equal((await getBalance(finance.address)).valueOf(), 0, 'finance shouldn\'t have ETH') assert.equal(periodId, 0, 'period id should be correct') assert.equal(amount, lockedETH, 'amount should be correct') assert.equal(paymentId, 0, 'payment id should be 0') - assert.equal(paymentRepeatNumber, 0, 'payment repeat number should be 0') + assert.equal(paymentExecutionNumber, 0, 'payment execution number should be 0') assert.equal(token, ETH, 'token should be correct') assert.equal(entity, finance.address, 'entity should be correct') assert.isTrue(incoming, 'tx should be incoming') @@ -416,27 +416,27 @@ contract('Finance App', accounts => { it('records payment', async () => { const amount = 10 - // repeats up to 10 times every 2 seconds - const receipt = await finance.newPayment(token1.address, recipient, amount, time, 2, 10, 'ref') + // executes up to 10 times every 2 seconds + const receipt = await finance.newScheduledPayment(token1.address, recipient, amount, time, 2, 10, 'ref') - const [token, receiver, txAmount, initialTime, interval, maxRepeats, disabled, repeats, createdBy] = await finance.getPayment(1) + const [token, receiver, txAmount, initialTime, interval, maxExecutions, disabled, executions, createdBy] = await finance.getPayment(1) assert.equal(token, token1.address, 'token address should match') assert.equal(receiver, recipient, 'receiver should match') assert.equal(amount, txAmount, 'amount should match') assert.equal(initialTime, time, 'time should match') assert.equal(interval, 2, 'interval should match') - assert.equal(maxRepeats, 10, 'max repeats should match') + assert.equal(maxExecutions, 10, 'max executionss should match') assert.equal(getEventData(receipt, 'NewPayment', 'reference'), 'ref', 'ref should match') assert.isFalse(disabled, 'should be enabled') - assert.equal(repeats, 1, 'should be on repeat 1') + assert.equal(executions, 1, 'should be on first execution') assert.equal(createdBy, accounts[0], 'should have correct creator') }) it('fails trying to get payment out of bounds', async () => { const amount = 10 - // repeats up to 10 times every 2 seconds - await finance.newPayment(token1.address, recipient, amount, time, 2, 10, 'ref') + // executes up to 10 times every 2 seconds + await finance.newScheduledPayment(token1.address, recipient, amount, time, 2, 10, 'ref') await assertRevert(async () => { await finance.getPayment(0) @@ -448,27 +448,26 @@ contract('Finance App', accounts => { it('fails trying to get transaction out of bounds', async () => { const amount = 10 - // repeats up to 10 times every 2 seconds - await finance.newPayment(token1.address, recipient, amount, time, 2, 10, 'ref') + // executes up to 10 times every 2 seconds + await finance.newScheduledPayment(token1.address, recipient, amount, time, 2, 10, 'ref') await assertRevert(async () => { await finance.getTransaction(2) }) }) - it('can create single payment', async () => { + it('can create single payment transaction', async () => { const amount = 10 - // interval 0, repeat 1 (single payment) - const receipt = await finance.newPayment(token1.address, recipient, amount, time, 0, 1, 'ref') + const receipt = await finance.newImmediatePayment(token1.address, recipient, amount, 'ref') assert.equal((await token1.balanceOf(recipient)).valueOf(), amount, 'recipient should have received tokens') - const [periodId, txAmount, paymentId, paymentRepeatNumber, token, entity, isIncoming, date] = await finance.getTransaction(1) + const [periodId, txAmount, paymentId, paymentExecutionNumber, token, entity, isIncoming, date] = await finance.getTransaction(1) assert.equal(periodId, 0, 'period id should be correct') assert.equal(txAmount, amount, 'amount should match') assert.equal(paymentId, 0, 'payment id should be 0 for single payment') - assert.equal(paymentRepeatNumber, 0, 'payment repeat number should be 0') + assert.equal(paymentExecutionNumber, 0, 'payment execution number should be 0') assert.equal(token, token1.address, 'token address should match') assert.equal(entity, recipient, 'receiver should match') assert.isFalse(isIncoming, 'single payment should be outgoing') @@ -479,8 +478,7 @@ contract('Finance App', accounts => { it('can decrease budget after spending', async () => { const amount = 10 - // interval 0, repeat 1 (single payment) - await finance.newPayment(token1.address, recipient, amount, time, 0, 1, '') + await finance.newImmediatePayment(token1.address, recipient, amount, '') const newBudgetAmount = 5 await finance.setBudget(token1.address, newBudgetAmount) @@ -501,47 +499,48 @@ contract('Finance App', accounts => { assert.isFalse(hasBudget, 'should not have budget') // budget was 100 - await finance.newPayment(token2.address, recipient, 190, time, 0, 1, '') + await finance.newImmediatePayment(token2.address, recipient, 190, '') assert.equal((await token2.balanceOf(recipient)).valueOf(), 190, 'recipient should have received tokens') }) - it('can create recurring payment', async () => { + it('can create scheduled payment', async () => { const amount = 10 - // repeats up to 10 times every 2 seconds - const firstReceipt = await finance.newPayment(token1.address, recipient, amount, time, 2, 10, '') + // executes up to 10 times every 2 seconds + const firstReceipt = await finance.newScheduledPayment(token1.address, recipient, amount, time, 2, 10, '') await finance.mock_setTimestamp(time + 4) const secondReceipt = await finance.executePayment(1) assert.equal((await token1.balanceOf(recipient)).valueOf(), amount * 3, 'recipient should have received tokens') - assert.equal(await finance.nextPaymentTime(1), time + 4 + 2, 'payment should be repeated again in 2') + assert.equal(await finance.nextPaymentTime(1), time + 4 + 2, 'payment should be executed again in 2') return Promise.all([firstReceipt, secondReceipt].map(async (receipt, index) => { - const repeatNum = index + 1 + const executionNum = index + 1 const transactionId = receipt.logs.filter(log => log.event == 'NewTransaction')[0].args.transactionId - const [periodId, txAmount, paymentId, paymentRepeatNumber, token, entity, incoming, date] = await finance.getTransaction(transactionId) + const [periodId, txAmount, paymentId, paymentExecutionNumber, token, entity, incoming, date] = await finance.getTransaction(transactionId) assert.equal(txAmount, amount, 'amount should be correct') assert.equal(paymentId, 1, 'payment id should be 1') - assert.equal(paymentRepeatNumber.valueOf(), repeatNum, `payment repeat number should be ${repeatNum}`) + assert.equal(paymentExecutionNumber.valueOf(), executionNum, `payment execution number should be ${executionNum}`) })) }) - it('can create recurring ether payment', async () => { + it('can create scheduled ether payment', async () => { const amount = 10 - // repeats up to 10 times every 2 seconds - await finance.newPayment(ETH, withdrawAddr, amount, time, 2, 10, '') + // executes up to 10 times every 2 seconds + await finance.newScheduledPayment(ETH, withdrawAddr, amount, time, 2, 10, '') await finance.mock_setTimestamp(time + 4) await finance.executePayment(1) assert.equal((await getBalance(withdrawAddr)).valueOf(), amount * 3, 'recipient should have received ether') }) - it('doesnt record payment for one time past transaction', async () => { - await finance.newPayment(token1.address, recipient, 1, time, 1, 1, '') - return assertRevert(async () => { + it('doesnt record payment for single payment transaction', async () => { + const receipt = await finance.newImmediatePayment(token1.address, recipient, 1, '') + assertEvent(receipt, 'NewPayment', 0) + await assertRevert(async () => { await finance.getPayment(1) }) }) @@ -549,9 +548,9 @@ contract('Finance App', accounts => { context('multitransaction period', async () => { beforeEach(async () => { // single payment - await finance.newPayment(token1.address, recipient, 10, time, 0, 1, '') // will spend 10 - // repeats up to 2 times every 1 seconds - await finance.newPayment(token2.address, recipient, 5, time + 1, 1, 2, '') // will spend 10 + await finance.newImmediatePayment(token1.address, recipient, 10, '') // will spend 10 + // executes up to 2 times every 1 seconds + await finance.newScheduledPayment(token2.address, recipient, 5, time + 1, 1, 2, '') // will spend 10 await finance.mock_setTimestamp(time + 4) await finance.executePayment(1) // first create payment doesn't get an id because it is simple immediate tx @@ -607,7 +606,7 @@ contract('Finance App', accounts => { it('fails when too many period transitions are needed', async () => { // Normal payments await assertRevert(async () => { - await finance.newPayment(token1.address, recipient, 10, time, 1, 1, '') + await finance.newImmediatePayment(token1.address, recipient, 10, '') }) // Direct ETH transfers @@ -618,7 +617,7 @@ contract('Finance App', accounts => { it('can transition periods externally to remove deadlock for payments', async () => { await finance.tryTransitionAccountingPeriod(maxTransitions) - await finance.newPayment(token1.address, recipient, 10, time, 1, 1, '') + await finance.newImmediatePayment(token1.address, recipient, 10, '') assert.equal((await token1.balanceOf(recipient)).valueOf(), 10, 'recipient should have received tokens') }) @@ -631,7 +630,7 @@ contract('Finance App', accounts => { const receipt = await finance.send(sentWei, { gas: 3e5 }) const transactionId = receipt.logs.filter(log => log.event == 'NewTransaction')[0].args.transactionId - const [periodId, amount, paymentId, paymentRepeatNumber, token, entity, incoming, date] = await finance.getTransaction(transactionId) + const [periodId, amount, paymentId, paymentExecutionNumber, token, entity, incoming, date] = await finance.getTransaction(transactionId) assert.equal(amount, sentWei, 'app should have received ETH and sent it to vault') assert.equal((await getBalance(vault.address)).valueOf(), prevVaultBalance + sentWei, 'app should have received ETH and sent it to vault') @@ -650,118 +649,234 @@ contract('Finance App', accounts => { }) }) - context('creating payment', async () => { + context('single payment', async () => { const amount = 10 - beforeEach(async () => { - await finance.newPayment(token1.address, recipient, amount, time + 1, 1, 4, '') + it('can create a single payment', async () => { + const receipt = await finance.newImmediatePayment(token1.address, recipient, amount,'') + assertEvent(receipt, 'NewTransaction') + assertEvent(receipt, 'NewPeriod', 0) + }) + + it('fails to create a zero-amount single payment', async () => { + await assertRevert(async () => { + await finance.newImmediatePayment(token1.address, recipient, 0, '') + }) + }) + + it('fails to create a single payment too high for the current budget', async () => { + const budget = 10 + await finance.setBudget(token1.address, budget) + + return assertRevert(() => { + return finance.newImmediatePayment(token1.address, recipient, budget + 1, '') + }) + }) + + it('fails to execute a single payment without enough funds', async () => { + const vaultBalance = await vault.balance(token1.address) + await finance.removeBudget(token1.address) // clear any budget restrictions + + return assertRevert(async () => { + await finance.newImmediatePayment(token1.address, recipient, vaultBalance + 1, '') + }) + }) + }) + + context('scheduled payment', async () => { + const amount = 10 + + it('can create a scheduled payment', async () => { + const receipt = await finance.newScheduledPayment(token1.address, recipient, amount, time + 1, 1, 4, '') + assertEvent(receipt, 'NewPayment') }) - it('only repeats payment until max repeats', async () => { - await finance.mock_setTimestamp(time + 10) - await finance.executePayment(1) + it('can create a future scheduled payment too large for current funds', async () => { + const vaultBalance = await vault.balance(token1.address) + await finance.removeBudget(token1.address) // clear any budget restrictions - assert.equal((await token1.balanceOf(recipient)).valueOf(), amount * 4, 'recipient should have received tokens') - assert.deepEqual(await finance.nextPaymentTime(1), MAX_UINT64, 'payment should be repeated again in 2') + const receipt = await finance.newScheduledPayment(token1.address, recipient, vaultBalance * 2, time + 1, 1, 4, '') + assertEvent(receipt, 'NewPayment') }) - it('receiver can always execute a payment', async () => { - await finance.mock_setTimestamp(time + 1) - await finance.receiverExecutePayment(1, { from: recipient }) + it('can create a single future payment', async () => { + const receipt = await finance.newScheduledPayment(token1.address, recipient, amount, time + 1, 1, 1, '') + assertEvent(receipt, 'NewPayment') + }) - assert.equal((await token1.balanceOf(recipient)).valueOf(), amount, 'should have received payment') + it('can create a single future payment too large for current funds', async () => { + const vaultBalance = await vault.balance(token1.address) + await finance.removeBudget(token1.address) // clear any budget restrictions + + const receipt = await finance.newScheduledPayment(token1.address, recipient, vaultBalance * 2, time + 1, 1, 1, '') + assertEvent(receipt, 'NewPayment') }) - it('fails creating a zero-amount payment', async () => { + it('fails to create a zero-amount payment', async () => { await assertRevert(async () => { - // one-time - await finance.newPayment(token1.address, recipient, 0, time + 1, 1, 1, '') + await finance.newScheduledPayment(token1.address, recipient, 0, time + 1, 1, 2, '') }) + }) + it('fails to create a no-interval payment', async () => { await assertRevert(async () => { - // recurring - await finance.newPayment(token1.address, recipient, 0, time + 1, 4, 1, '') + await finance.newScheduledPayment(token1.address, recipient, 1, time + 1, 0, 2, '') }) }) - it('fails when non-receiver attempts to execute a payment', async () => { - await finance.mock_setTimestamp(time + 1) + it('fails to create a new one-time payment without enough budget', async () => { + const budget = 10 + await finance.setBudget(token1.address, budget) + + return assertRevert(async () => { + await finance.newScheduledPayment(token1.address, recipient, budget + 1, time, 1, 1, '') + }) + }) + + it('fails to create a new one-time payment without enough funds', async () => { + const vaultBalance = await vault.balance(token1.address) + await finance.removeBudget(token1.address) // clear any budget restrictions return assertRevert(async () => { - await finance.receiverExecutePayment(1) + await finance.newScheduledPayment(token1.address, recipient, vaultBalance + 1, time, 1, 1, '') + }) + }) + + it('fails to create an immediate single payment', async () => { + await assertRevert(async () => { + await finance.newScheduledPayment(token1.address, recipient, 1, time - 1, 1, 1, '') }) }) - it('fails to create a payment too high for the current budget', async () => { + it('fails to create a scheduled payment too high for the current budget', async () => { const budget = 10 await finance.setBudget(token1.address, budget) return assertRevert(() => { - const paymentAmount = budget * 10 - return finance.newPayment(token1.address, recipient, 50, paymentAmount, 1, 2, '') + return finance.newScheduledPayment(token1.address, recipient, budget + 1, time, 1, 2, '') }) }) - it('fails executing a payment before time', async () => { + it('fails to execute a scheduled payment without enough funds', async () => { + const vaultBalance = await vault.balance(token1.address) + await finance.removeBudget(token1.address) // clear any budget restrictions + + const receipt = await finance.newScheduledPayment(token1.address, recipient, vaultBalance + 1, time, 1, 2, '') + const newScheduledPaymentId = getEventData(receipt, 'NewPayment', 'paymentId') + return assertRevert(async () => { - await finance.executePayment(1, { from: recipient }) + await finance.executePayment(newScheduledPaymentId, { from: recipient }) }) }) - it('fails executing a payment by receiver before time', async () => { - return assertRevert(async () => { - await finance.receiverExecutePayment(1, { from: recipient }) + context('created scheduled payment', async () => { + let paymentId + + beforeEach(async () => { + const receipt = await finance.newScheduledPayment(token1.address, recipient, amount, time + 1, 1, 4, '') + paymentId = getEventData(receipt, 'NewPayment', 'paymentId') }) - }) - it('fails executing inactive payment', async () => { - await finance.setPaymentStatus(1, false) - await finance.mock_setTimestamp(time + 1) + it('only executes scheduled payment until max executions', async () => { + await finance.mock_setTimestamp(time + 10) + await finance.executePayment(paymentId) - return assertRevert(async () => { - await finance.executePayment(1, { from: recipient }) + assert.equal((await token1.balanceOf(recipient)).valueOf(), amount * 4, 'recipient should have received tokens') + assert.deepEqual(await finance.nextPaymentTime(paymentId), MAX_UINT64, 'payment should never be repeated') }) - }) - it('succeeds payment after setting payment status to active', async () => { - await finance.setPaymentStatus(1, true) - await finance.mock_setTimestamp(time + 1) + it('receiver can always execute a payment', async () => { + await finance.mock_setTimestamp(time + 1) + await finance.receiverExecutePayment(paymentId, { from: recipient }) + + assert.equal((await token1.balanceOf(recipient)).valueOf(), amount, 'should have received payment') + }) + + + it('fails when non-receiver attempts to execute a payment', async () => { + await finance.mock_setTimestamp(time + 1) + + return assertRevert(async () => { + await finance.receiverExecutePayment(paymentId) + }) + }) + + it('fails to execute a scheduled payment before next available time', async () => { + return assertRevert(async () => { + await finance.executePayment(paymentId, { from: recipient }) + }) + }) + + it('fails to execute a scheduled payment by receiver before next available time', async () => { + return assertRevert(async () => { + await finance.receiverExecutePayment(paymentId, { from: recipient }) + }) + }) + + it('fails to execute inactive scheduled payment', async () => { + await finance.setPaymentStatus(paymentId, false) + await finance.mock_setTimestamp(time + 1) + + return assertRevert(async () => { + await finance.executePayment(paymentId, { from: recipient }) + }) + }) + + it('succeeds payment after setting payment status to active', async () => { + await finance.setPaymentStatus(paymentId, true) + await finance.mock_setTimestamp(time + 1) - await finance.executePayment(1, { from: recipient }) + await finance.executePayment(paymentId, { from: recipient }) + }) }) - }) - const assertPaymentFailure = receipt => { - const filteredLogs = receipt.logs.filter(log => log.event == 'PaymentFailure') - assert.equal(filteredLogs.length, 1, 'should have logged payment failure') - } + context('payment failure', async () => { + it('tries to execute a new scheduled payment if initially possible even without enough funds', async () => { + const vaultBalance = await vault.balance(token1.address) + await finance.removeBudget(token1.address) // clear any budget restrictions + + const receipt = await finance.newScheduledPayment(token1.address, recipient, vaultBalance + 1, time, 1, 2, '') - it('emits payment failure event when out of budget', async () => { - // Enough budget to allow creation of a new payment, but not enough left in the period - // to execute it - const budget = 50 - const amountPerPayment = 50 + assertEvent(receipt, 'PaymentFailure') + // Make sure no transactions were made + assertEvent(receipt, 'NewTransaction', 0) + }) - // Create the budget, and use it up for the period - await finance.setBudget(token1.address, budget) - await finance.newPayment(token1.address, recipient, amountPerPayment, time, 1, 2, '') + it('emits payment failure event when out of budget', async () => { + // Enough budget to allow creation of a new payment, but not enough left in the period + // to execute it + const budget = 50 + const amountPerPayment = 50 + assert.isTrue(await finance.canMakePayment(token1.address, amountPerPayment)) + + // Create the budget, and use it up for the period + await finance.setBudget(token1.address, budget) + await finance.newScheduledPayment(token1.address, recipient, amountPerPayment, time, 1, 2, '') + + // No more budget left + const receipt = await finance.newScheduledPayment(token1.address, recipient, amountPerPayment, time, 1, 2, '') + assertEvent(receipt, 'PaymentFailure') + assert.isFalse(await finance.canMakePayment(token1.address, amountPerPayment)) + }) - // No more budget left - const receipt = await finance.newPayment(token1.address, recipient, amountPerPayment, time, 1, 2, '') - assertPaymentFailure(receipt) - }) + it('emits payment failure event when out of balance', async () => { + const amountPerPayment = 40 + const paidInterval = 100 + const paidTimes = Math.floor((await vault.balance(token1.address)) / amountPerPayment) + await finance.removeBudget(token1.address) - it('emits payment failure event when out of balance', async () => { - // repeats up to 3 times every 100 seconds - await finance.newPayment(token1.address, recipient, 40, time, 100, 3, '') - await finance.mock_setTimestamp(time + PERIOD_DURATION) - await finance.executePayment(1) + assert.isTrue(await finance.canMakePayment(token1.address, amountPerPayment)) - await finance.mock_setTimestamp(time + PERIOD_DURATION * 2) - const receipt = await finance.executePayment(1) + // creates a repeating payment that can be executed one more than the vault's funds will allow + await finance.newScheduledPayment(token1.address, recipient, amountPerPayment, time, paidInterval, paidTimes + 1, '') + await finance.mock_setTimestamp(time + paidInterval * (paidTimes + 1)) + const receipt = await finance.executePayment(1) - assertPaymentFailure(receipt) - assert.equal(await token1.balanceOf(recipient), 80, 'recipient should have received tokens') + assertEvent(receipt, 'PaymentFailure') + assert.equal(await token1.balanceOf(recipient), amountPerPayment * paidTimes, 'recipient should have received tokens') + assert.isFalse(await finance.canMakePayment(token1.address, amountPerPayment)) + }) + }) }) }) @@ -772,17 +887,27 @@ contract('Finance App', accounts => { const { financeApp, recoveryVault } = await newProxyFinance() nonInit = financeApp recVault = recoveryVault - await nonInit.mock_setTimestamp(START_TIME) }) - it('fails to create new payment', async() => { + it('fails to create new scheduled payment', async() => { + const recipient = accounts[1] + const amount = 1 + const time = 22 + await nonInit.mock_setTimestamp(time) + + return assertRevert(async() => { + await nonInit.newScheduledPayment(token1.address, recipient, amount, time, 1, 2, 'ref') + }) + }) + + it('fails to create new single payment transaction', async() => { const recipient = accounts[1] const amount = 1 const time = 22 await nonInit.mock_setTimestamp(time) return assertRevert(async() => { - await nonInit.newPayment(token1.address, recipient, amount, time, 0, 1, 'ref') + await nonInit.newImmediatePayment(token1.address, recipient, amount, 'ref') }) }) diff --git a/apps/survey/contracts/Survey.sol b/apps/survey/contracts/Survey.sol index 3aa1563583..7d00140365 100644 --- a/apps/survey/contracts/Survey.sol +++ b/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); diff --git a/apps/token-manager/contracts/TokenManager.sol b/apps/token-manager/contracts/TokenManager.sol index 4a8c9d4e92..f28d343ab8 100644 --- a/apps/token-manager/contracts/TokenManager.sol +++ b/apps/token-manager/contracts/TokenManager.sol @@ -8,9 +8,7 @@ pragma solidity 0.4.24; import "@aragon/os/contracts/apps/AragonApp.sol"; import "@aragon/os/contracts/common/IForwarder.sol"; -import "@aragon/os/contracts/common/Uint256Helpers.sol"; -import "@aragon/os/contracts/lib/token/ERC20.sol"; import "@aragon/os/contracts/lib/math/SafeMath.sol"; import "@aragon/apps-shared-minime/contracts/ITokenController.sol"; @@ -19,7 +17,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 +26,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; @@ -50,18 +47,23 @@ contract TokenManager is ITokenController, IForwarder, AragonApp { bool revokable; } + // Note that we COMPLETELY trust this MiniMeToken to not be malicious for proper operation of this contract MiniMeToken public token; uint256 public maxAccountTokens; // 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 +98,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 +135,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 +154,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 +189,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 @@ -203,6 +206,42 @@ contract TokenManager is ITokenController, IForwarder, AragonApp { emit RevokeVesting(_holder, _vestingId, nonVested); } + // 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). + * 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, uint256 _amount) external 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) external onlyToken returns (bool) { + return true; + } + + /** + * @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) external payable onlyToken returns (bool) { + return false; + } + // Forwarding fns function isForwarder() external pure returns (bool) { @@ -230,50 +269,6 @@ contract TokenManager is ITokenController, IForwarder, AragonApp { return hasInitialized() && token.balanceOf(_sender) > 0; } - // ITokenController fns - - /* - * @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) - * @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; - } - - /** - * @dev Notifies the controller about an approval allowing the controller to react if desired - * @return False if the controller does not authorize the approval - */ - function onApprove(address, address, uint) public returns (bool) { - return true; - } - - /** - * @notice Called when ether is sent to the MiniMe Token contract - * @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); - return false; - } - // Getter fns function getVesting( @@ -299,27 +294,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 +313,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 +387,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; + } } diff --git a/apps/token-manager/contracts/test/mocks/TokenManagerMock.sol b/apps/token-manager/contracts/test/mocks/TokenManagerMock.sol new file mode 100644 index 0000000000..c2e0c7a642 --- /dev/null +++ b/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; } +} diff --git a/apps/token-manager/test/tokenmanager.js b/apps/token-manager/test/tokenmanager.js index 19f349360a..dcf9889539 100644 --- a/apps/token-manager/test/tokenmanager.js +++ b/apps/token-manager/test/tokenmanager.js @@ -1,12 +1,10 @@ const { assertRevert } = require('@aragon/test-helpers/assertThrow') -const timetravel = require('@aragon/test-helpers/timeTravel')(web3) -const getBlock = require('@aragon/test-helpers/block')(web3) -const getBlockNumber = require('@aragon/test-helpers/blockNumber')(web3) +const getBalance = require('@aragon/test-helpers/balance')(web3) const { encodeCallScript } = require('@aragon/test-helpers/evmScript') const ExecutionTarget = artifacts.require('ExecutionTarget') -const TokenManager = artifacts.require('TokenManager') +const TokenManager = artifacts.require('TokenManagerMock') const MiniMeToken = artifacts.require('MiniMeToken') const DAOFactory = artifacts.require('@aragon/core/contracts/factory/DAOFactory') const EVMScriptRegistryFactory = artifacts.require('@aragon/core/contracts/factory/EVMScriptRegistryFactory') @@ -27,8 +25,11 @@ contract('Token Manager', accounts => { let MINT_ROLE, ISSUE_ROLE, ASSIGN_ROLE, REVOKE_VESTINGS_ROLE, BURN_ROLE let ETH + const NOW_TIME = 1 + const root = accounts[0] const holder = accounts[1] + const holder2 = accounts[2] before(async () => { const kernelBase = await getContract('Kernel').new(true) // petrify immediately @@ -58,6 +59,7 @@ contract('Token Manager', accounts => { const receipt = await dao.newAppInstance('0x1234', tokenManagerBase.address, '0x', false, { from: root }) tokenManager = TokenManager.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + tokenManager.mock_setTimestamp(NOW_TIME) await acl.createPermission(ANY_ADDR, tokenManager.address, MINT_ROLE, root, { from: root }) await acl.createPermission(ANY_ADDR, tokenManager.address, ISSUE_ROLE, root, { from: root }) @@ -112,7 +114,7 @@ contract('Token Manager', accounts => { await tokenManager.mint(holder, 2000) return assertRevert(async () => { - await token.transfer(accounts[2], 10, { from: holder }) + await token.transfer(holder2, 10, { from: holder }) }) }) @@ -168,10 +170,20 @@ contract('Token Manager', accounts => { }) }) - it('can issue unlimited tokens for manager', async () => { + it('can issue unlimited tokens to itself', async () => { await tokenManager.issue(limit + 100000) - assert.equal(await token.balanceOf(tokenManager.address), limit + 100000, 'should have tokens') + assert.equal(await token.balanceOf(tokenManager.address), limit + 100000, 'should have more tokens than limit') + }) + + it('can assign unlimited tokens to itself', async () => { + // First issue some tokens to the Token Manager + await tokenManager.issue(limit + 100000) + + // Then assign these tokens to the Token Manager (should not actually move any tokens) + await tokenManager.assign(tokenManager.address, limit + 100000) + + assert.equal(await token.balanceOf(tokenManager.address), limit + 100000, 'should have more tokens than limit') }) it('can assign up to limit', async () => { @@ -188,6 +200,25 @@ contract('Token Manager', accounts => { await tokenManager.assign(holder, limit + 1) }) }) + + it('can transfer tokens to token manager without regard to token limit', async () => { + await tokenManager.issue(limit + 100000) + await tokenManager.assign(holder, 5) + + await token.transfer(tokenManager.address, 5, { from: holder }) + + assert.equal(await token.balanceOf(tokenManager.address), limit + 100000, 'should have more tokens than limit') + }) + + it('cannot transfer tokens to an address if it would go over the limit', async () => { + await tokenManager.issue(limit * 2) + await tokenManager.assign(holder, limit - 1) + await tokenManager.assign(holder2, limit - 1) + + return assertRevert(async () => { + await token.transfer(holder2, 5, { from: holder }) + }) + }) }) context('for normal native tokens', () => { @@ -230,6 +261,19 @@ contract('Token Manager', accounts => { assert.equal(await token.balanceOf(tokenManager.address), 0, 'token manager should have 0 tokens') }) + it('can assign issued tokens to itself', async () => { + await tokenManager.issue(50) + await tokenManager.assign(tokenManager.address, 50) + + assert.equal(await token.balanceOf(tokenManager.address), 50, 'token manager should not have changed token balance') + }) + + it('cannot mint tokens to itself', async () => { + return assertRevert(async () => { + await tokenManager.mint(tokenManager.address, 100) + }) + }) + it('cannot assign more tokens than owned', async () => { await tokenManager.issue(50) @@ -258,10 +302,32 @@ contract('Token Manager', accounts => { await tokenManager.mint(holder, amount) // Make sure this callback fails when called out-of-context - await assertRevert(() => tokenManager.onTransfer(holder, accounts[2], 10)) + await assertRevert(() => tokenManager.onTransfer(holder, holder2, 10)) // Make sure the same transfer through the token's context doesn't revert - await token.transfer(accounts[2], amount, { from: holder }) + await token.transfer(holder2, amount, { from: holder }) + }) + + it("cannot call onApprove() from outside of the token's context", async () => { + const amount = 10 + await tokenManager.mint(holder, amount) + + // Make sure this callback fails when called out-of-context + await assertRevert(() => tokenManager.onApprove(holder, holder2, 10)) + + // Make sure no allowance was registered + assert.equal(await token.allowance(holder, holder2), 0, 'token approval should be 0') + }) + + it("cannot call proxyPayment() from outside of the token's context", async () => { + const value = 10 + const prevTokenManagerBalance = (await getBalance(tokenManager.address)).toNumber() + + // Make sure this callback fails when called out-of-context + await assertRevert(() => tokenManager.proxyPayment(root, { value })) + + // Make sure no ETH was transferred + assert.equal((await getBalance(tokenManager.address)).toNumber(), prevTokenManagerBalance, 'token manager ETH balance should be the same') }) it('fails when assigning invalid vesting schedule', async () => { @@ -282,24 +348,15 @@ contract('Token Manager', accounts => { }) context('assigning vested tokens', () => { - let now = 0 - let startDate, cliffDate, vestingDate - - const start = 1000 - const cliff = 2000 - const vesting = 5000 + const startDate = NOW_TIME + 1000 + const cliffDate = NOW_TIME + 2000 + const vestingDate = NOW_TIME + 5000 const totalTokens = 40 const revokable = true beforeEach(async () => { await tokenManager.issue(totalTokens) - const block = await getBlock(await getBlockNumber()) - now = block.timestamp - startDate = now + start - cliffDate = now + cliff - vestingDate = now + vesting - await tokenManager.assignVested(holder, totalTokens, startDate, cliffDate, vestingDate, revokable) }) @@ -318,62 +375,71 @@ contract('Token Manager', accounts => { assert.equal(vRevokable, revokable) }) - it('can start transfering on cliff', async () => { - await timetravel(cliff) - await token.transfer(accounts[2], 10, { from: holder }) - assert.equal(await token.balanceOf(accounts[2]), 10, 'should have received tokens') + it('can start transferring on cliff', async () => { + await tokenManager.mock_setTimestamp(cliffDate) + + await token.transfer(holder2, 10, { from: holder }) + assert.equal(await token.balanceOf(holder2), 10, 'should have received tokens') assert.equal(await tokenManager.spendableBalanceOf(holder), 0, 'should not be able to spend more tokens') }) it('can transfer all tokens after vesting', async () => { - await timetravel(vesting) - await token.transfer(accounts[2], totalTokens, { from: holder }) - assert.equal(await token.balanceOf(accounts[2]), totalTokens, 'should have received tokens') + await tokenManager.mock_setTimestamp(vestingDate) + + await token.transfer(holder2, totalTokens, { from: holder }) + assert.equal(await token.balanceOf(holder2), totalTokens, 'should have received tokens') }) it('can transfer half mid vesting', async () => { - await timetravel(start + (vesting - start) / 2) + await tokenManager.mock_setTimestamp(startDate + (vestingDate - startDate) / 2) - await token.transfer(accounts[2], 20, { from: holder }) + await token.transfer(holder2, 20, { from: holder }) - assert.equal(await tokenManager.spendableBalanceOf(holder), 0, 'should not be able to spend more tokens') + assert.equal((await tokenManager.spendableBalanceOf(holder)).toString(), 0, 'should not be able to spend more tokens') }) it('cannot transfer non-vested tokens', async () => { return assertRevert(async () => { - await token.transfer(accounts[2], 10, { from: holder }) + await token.transfer(holder2, 10, { from: holder }) }) }) it('can approve non-vested tokens but transferFrom fails', async () => { - await token.approve(accounts[2], 10, { from: holder }) + await token.approve(holder2, 10, { from: holder }) return assertRevert(async () => { - await token.transferFrom(holder, accounts[2], 10, { from: accounts[2] }) + await token.transferFrom(holder, holder2, 10, { from: holder2 }) }) }) it('cannot transfer all tokens right before vesting', async () => { - await timetravel(vesting - 10) + await tokenManager.mock_setTimestamp(vestingDate - 10) + return assertRevert(async () => { - await token.transfer(accounts[2], totalTokens, { from: holder }) + await token.transfer(holder2, totalTokens, { from: holder }) }) }) it('can be revoked and not vested tokens are transfered to token manager', async () => { - await timetravel(cliff) + await tokenManager.mock_setTimestamp(cliffDate) await tokenManager.revokeVesting(holder, 0) - await token.transfer(accounts[2], 5, { from: holder }) + await token.transfer(holder2, 5, { from: holder }) assert.equal(await token.balanceOf(holder), 5, 'should have kept vested tokens') - assert.equal(await token.balanceOf(accounts[2]), 5, 'should have kept vested tokens') + assert.equal(await token.balanceOf(holder2), 5, 'should have kept vested tokens') assert.equal(await token.balanceOf(tokenManager.address), totalTokens - 10, 'should have received unvested') }) + it('cannot assign a vesting to itself', async () => { + return assertRevert(async () => { + await tokenManager.assignVested(tokenManager.address, 5, startDate, cliffDate, vestingDate, revokable) + }) + }) + it('cannot revoke non-revokable vestings', async () => { await tokenManager.issue(1) - await tokenManager.assignVested(holder, 1, now + start, now + cliff, now + vesting, false) + await tokenManager.assignVested(holder, 1, startDate, cliffDate, vestingDate, false) return assertRevert(async () => { await tokenManager.revokeVesting(holder, 1) @@ -381,17 +447,25 @@ contract('Token Manager', accounts => { }) it('cannot have more than 50 vestings', async () => { - let i = 49 // already have 1 await tokenManager.issue(50) - while (i > 0) { - await tokenManager.assignVested(holder, 1, now + start, now + cliff, now + vesting, false) - i-- + + // Only create 49 new vestings as we've already created one in beforeEach() + for (ii = 0; ii < 49; ++ii) { + await tokenManager.assignVested(holder, 1, startDate, cliffDate, vestingDate, false) } - await timetravel(vesting) - await token.transfer(accounts[3], 1) // can transfer - return assertRevert(async () => { - await tokenManager.assignVested(holder, 1, now + start, now + cliff, now + vesting, false) + + await assertRevert(async () => { + await tokenManager.assignVested(holder, 1, startDate, cliffDate, vestingDate, false) }) + + // Can't create a new vesting even after other vestings have finished + await tokenManager.mock_setTimestamp(vestingDate) + await assertRevert(async () => { + await tokenManager.assignVested(holder, 1, startDate, cliffDate, vestingDate, false) + }) + + // But can now transfer + await token.transfer(holder2, 1, { from: holder }) }) }) }) @@ -399,13 +473,13 @@ contract('Token Manager', accounts => { context('app not initialized', async () => { it('fails to mint tokens', async() => { return assertRevert(async() => { - await tokenManager.mint(accounts[1], 1) + await tokenManager.mint(holder, 1) }) }) it('fails to assign tokens', async() => { return assertRevert(async() => { - await tokenManager.assign(accounts[1], 1) + await tokenManager.assign(holder, 1) }) }) @@ -417,7 +491,7 @@ contract('Token Manager', accounts => { it('fails to burn tokens', async() => { return assertRevert(async() => { - await tokenManager.burn(accounts[0], 1) + await tokenManager.burn(holder, 1) }) }) }) diff --git a/apps/vault/contracts/Vault.sol b/apps/vault/contracts/Vault.sol index 50b6012a9b..dfc755d3e4 100644 --- a/apps/vault/contracts/Vault.sol +++ b/apps/vault/contracts/Vault.sol @@ -77,7 +77,7 @@ contract Vault is EtherTokenConstant, AragonApp, DepositableStorage { if (_token == ETH) { return address(this).balance; } else { - return ERC20(_token).staticBalanceOf(this); + return ERC20(_token).staticBalanceOf(address(this)); } } @@ -97,7 +97,10 @@ contract Vault is EtherTokenConstant, AragonApp, DepositableStorage { // Deposit is implicit in this case require(msg.value == _value, ERROR_VALUE_MISMATCH); } else { - require(ERC20(_token).safeTransferFrom(msg.sender, this, _value), ERROR_TOKEN_TRANSFER_FROM_REVERTED); + require( + ERC20(_token).safeTransferFrom(msg.sender, address(this), _value), + ERROR_TOKEN_TRANSFER_FROM_REVERTED + ); } emit VaultDeposit(_token, msg.sender, _value); diff --git a/apps/vault/test/vault_shared.js b/apps/vault/test/vault_shared.js index f4e3d46350..472f9da12c 100644 --- a/apps/vault/test/vault_shared.js +++ b/apps/vault/test/vault_shared.js @@ -165,7 +165,7 @@ module.exports = ( tokenContract: TokenReturnMissingMock, }, ] - for ({ title, tokenContract} of tokenTestGroups) { + for (const { title, tokenContract} of tokenTestGroups) { context(`> ERC20 (${title})`, () => { let token diff --git a/apps/voting/contracts/Voting.sol b/apps/voting/contracts/Voting.sol index af9ca82f29..d2d880c259 100644 --- a/apps/voting/contracts/Voting.sol +++ b/apps/voting/contracts/Voting.sol @@ -160,7 +160,7 @@ contract Voting is IForwarder, AragonApp { * @param _executesIfDecided Whether the vote should execute its action if it becomes decided */ function vote(uint256 _voteId, bool _supports, bool _executesIfDecided) external voteExists(_voteId) { - require(canVote(_voteId, msg.sender), ERROR_CAN_NOT_VOTE); + require(_canVote(_voteId, msg.sender), ERROR_CAN_NOT_VOTE); _vote(_voteId, _supports, msg.sender, _executesIfDecided); } @@ -171,7 +171,6 @@ contract Voting is IForwarder, AragonApp { * @param _voteId Id for vote */ function executeVote(uint256 _voteId) external voteExists(_voteId) { - require(canExecute(_voteId), ERROR_CAN_NOT_EXECUTE); _executeVote(_voteId); } @@ -202,44 +201,16 @@ contract Voting is IForwarder, AragonApp { * @dev Initialization check is implicitly provided by `voteExists()` as new votes can only be * created via `newVote(),` which requires initialization */ - function canVote(uint256 _voteId, address _voter) public view voteExists(_voteId) returns (bool) { - Vote storage vote_ = votes[_voteId]; - - return _isVoteOpen(vote_) && token.balanceOfAt(_voter, vote_.snapshotBlock) > 0; + function canExecute(uint256 _voteId) public view voteExists(_voteId) returns (bool) { + return _canExecute(_voteId); } /** * @dev Initialization check is implicitly provided by `voteExists()` as new votes can only be * created via `newVote(),` which requires initialization */ - function canExecute(uint256 _voteId) public view voteExists(_voteId) returns (bool) { - Vote storage vote_ = votes[_voteId]; - - if (vote_.executed) { - return false; - } - - // Voting is already decided - if (_isValuePct(vote_.yea, vote_.votingPower, vote_.supportRequiredPct)) { - return true; - } - - uint256 totalVotes = vote_.yea.add(vote_.nay); - - // Vote ended? - if (_isVoteOpen(vote_)) { - return false; - } - // Has enough support? - if (!_isValuePct(vote_.yea, totalVotes, vote_.supportRequiredPct)) { - return false; - } - // Has min quorum? - if (!_isValuePct(vote_.yea, vote_.votingPower, vote_.minAcceptQuorumPct)) { - return false; - } - - return true; + function canVote(uint256 _voteId, address _voter) public view voteExists(_voteId) returns (bool) { + return _canVote(_voteId, _voter); } function getVote(uint256 _voteId) @@ -288,6 +259,7 @@ contract Voting is IForwarder, AragonApp { require(votingPower > 0, ERROR_NO_VOTING_POWER); voteId = votesLength++; + Vote storage vote_ = votes[voteId]; vote_.startDate = getTimestamp64(); vote_.snapshotBlock = snapshotBlock; @@ -298,7 +270,7 @@ contract Voting is IForwarder, AragonApp { emit StartVote(voteId, msg.sender, _metadata); - if (_castVote && canVote(voteId, msg.sender)) { + if (_castVote && _canVote(voteId, msg.sender)) { _vote(voteId, true, msg.sender, _executesIfDecided); } } @@ -333,12 +305,21 @@ contract Voting is IForwarder, AragonApp { emit CastVote(_voteId, _voter, _supports, voterStake); - if (_executesIfDecided && canExecute(_voteId)) { - _executeVote(_voteId); + if (_executesIfDecided && _canExecute(_voteId)) { + // We've already checked if the vote can be executed with `_canExecute()` + _unsafeExecuteVote(_voteId); } } function _executeVote(uint256 _voteId) internal { + require(_canExecute(_voteId), ERROR_CAN_NOT_EXECUTE); + _unsafeExecuteVote(_voteId); + } + + /** + * @dev Unsafe version of _executeVote that assumes you have already checked if the vote can be executed + */ + function _unsafeExecuteVote(uint256 _voteId) internal { Vote storage vote_ = votes[_voteId]; vote_.executed = true; @@ -349,6 +330,41 @@ contract Voting is IForwarder, AragonApp { emit ExecuteVote(_voteId); } + function _canExecute(uint256 _voteId) internal view returns (bool) { + Vote storage vote_ = votes[_voteId]; + + if (vote_.executed) { + return false; + } + + // Voting is already decided + if (_isValuePct(vote_.yea, vote_.votingPower, vote_.supportRequiredPct)) { + return true; + } + + // Vote ended? + if (_isVoteOpen(vote_)) { + return false; + } + // Has enough support? + uint256 totalVotes = vote_.yea.add(vote_.nay); + if (!_isValuePct(vote_.yea, totalVotes, vote_.supportRequiredPct)) { + return false; + } + // Has min quorum? + if (!_isValuePct(vote_.yea, vote_.votingPower, vote_.minAcceptQuorumPct)) { + return false; + } + + return true; + } + + function _canVote(uint256 _voteId, address _voter) internal view returns (bool) { + Vote storage vote_ = votes[_voteId]; + + return _isVoteOpen(vote_) && token.balanceOfAt(_voter, vote_.snapshotBlock) > 0; + } + function _isVoteOpen(Vote storage vote_) internal view returns (bool) { return getTimestamp64() < vote_.startDate.add(voteTime) && !vote_.executed; } diff --git a/apps/voting/contracts/test/mocks/VotingMock.sol b/apps/voting/contracts/test/mocks/VotingMock.sol index 93a73400e3..78bbc4fd7c 100644 --- a/apps/voting/contracts/test/mocks/VotingMock.sol +++ b/apps/voting/contracts/test/mocks/VotingMock.sol @@ -4,6 +4,11 @@ import "../../Voting.sol"; contract VotingMock is Voting { + uint64 mockTime; + + function mock_setTimestamp(uint64 i) public { mockTime = i; } + function getTimestamp64() internal view returns (uint64) { return mockTime; } + /* Ugly hack to work around this issue: * https://github.com/trufflesuite/truffle/issues/569 * https://github.com/trufflesuite/truffle/issues/737 diff --git a/apps/voting/test/voting.js b/apps/voting/test/voting.js index 22c458607a..2cb9481eab 100644 --- a/apps/voting/test/voting.js +++ b/apps/voting/test/voting.js @@ -2,7 +2,6 @@ const sha3 = require('solidity-sha3').default const { assertRevert } = require('@aragon/test-helpers/assertThrow') const getBlockNumber = require('@aragon/test-helpers/blockNumber')(web3) -const timeTravel = require('@aragon/test-helpers/timeTravel')(web3) const { encodeCallScript, EMPTY_SCRIPT } = require('@aragon/test-helpers/evmScript') const ExecutionTarget = artifacts.require('ExecutionTarget') @@ -36,7 +35,9 @@ contract('Voting App', accounts => { let APP_MANAGER_ROLE let CREATE_VOTES_ROLE, MODIFY_SUPPORT_ROLE, MODIFY_QUORUM_ROLE - const votingTime = 1000 + const startTime = 1 + const votingDuration = 1000 + const votingEnd = startTime + votingDuration + 1 const root = accounts[0] before(async () => { @@ -62,6 +63,7 @@ contract('Voting App', accounts => { const receipt = await dao.newAppInstance('0x1234', votingBase.address, '0x', false, { from: root }) voting = Voting.at(receipt.logs.filter(l => l.event == 'NewAppProxy')[0].args.proxy) + await voting.mock_setTimestamp(startTime) await acl.createPermission(ANY_ADDR, voting.address, CREATE_VOTES_ROLE, root, { from: root }) await acl.createPermission(ANY_ADDR, voting.address, MODIFY_SUPPORT_ROLE, root, { from: root }) @@ -80,14 +82,14 @@ contract('Voting App', accounts => { beforeEach(async () => { token = await MiniMeToken.new(NULL_ADDRESS, NULL_ADDRESS, 0, 'n', 0, 'n', true) // empty parameters minime - await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingTime) + await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingDuration) executionTarget = await ExecutionTarget.new() }) it('fails on reinitialization', async () => { return assertRevert(async () => { - await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingTime) + await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingDuration) }) }) @@ -95,7 +97,7 @@ contract('Voting App', accounts => { const newVoting = await Voting.new() assert.isTrue(await newVoting.isPetrified()) return assertRevert(async () => { - await newVoting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingTime) + await newVoting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingDuration) }) }) @@ -159,7 +161,7 @@ contract('Voting App', accounts => { await token.generateTokens(holder29, bigExp(29, decimals)) await token.generateTokens(holder51, bigExp(51, decimals)) - await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingTime) + await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingDuration) executionTarget = await ExecutionTarget.new() }) @@ -250,7 +252,7 @@ contract('Voting App', accounts => { await voting.vote(voteId, true, false, { from: holder51 }) await voting.vote(voteId, true, false, { from: holder20 }) await voting.vote(voteId, false, false, { from: holder29 }) - await timeTravel(votingTime + 1) + await voting.mock_setTimestamp(votingEnd) const state = await voting.getVote(voteId) assert.equal(state[4].toString(), neededSupport.toString(), 'required support in vote should stay equal') @@ -265,7 +267,7 @@ contract('Voting App', accounts => { // it will succeed await voting.vote(voteId, true, true, { from: holder29 }) - await timeTravel(votingTime + 1) + await voting.mock_setTimestamp(votingEnd) const state = await voting.getVote(voteId) assert.equal(state[5].toString(), minimumAcceptanceQuorum.toString(), 'acceptance quorum in vote should stay equal') @@ -308,7 +310,7 @@ contract('Voting App', accounts => { }) it('throws when voting after voting closes', async () => { - await timeTravel(votingTime + 1) + await voting.mock_setTimestamp(votingEnd) return assertRevert(async () => { await voting.vote(voteId, true, true, { from: holder29 }) }) @@ -317,14 +319,14 @@ contract('Voting App', accounts => { it('can execute if vote is approved with support and quorum', async () => { await voting.vote(voteId, true, true, { from: holder29 }) await voting.vote(voteId, false, true, { from: holder20 }) - await timeTravel(votingTime + 1) + await voting.mock_setTimestamp(votingEnd) await voting.executeVote(voteId) assert.equal(await executionTarget.counter(), 2, 'should have executed result') }) it('cannot execute vote if not enough quorum met', async () => { await voting.vote(voteId, true, true, { from: holder20 }) - await timeTravel(votingTime + 1) + await voting.mock_setTimestamp(votingEnd) return assertRevert(async () => { await voting.executeVote(voteId) }) @@ -333,7 +335,7 @@ contract('Voting App', accounts => { it('cannot execute vote if not support met', async () => { await voting.vote(voteId, false, true, { from: holder29 }) await voting.vote(voteId, false, true, { from: holder20 }) - await timeTravel(votingTime + 1) + await voting.mock_setTimestamp(votingEnd) return assertRevert(async () => { await voting.executeVote(voteId) }) @@ -376,17 +378,17 @@ contract('Voting App', accounts => { const neededSupport = pct16(20) const minimumAcceptanceQuorum = pct16(50) return assertRevert(async() => { - await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingTime) + await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingDuration) }) }) it('fails if min support is 100% or more', async() => { const minimumAcceptanceQuorum = pct16(20) await assertRevert(async() => { - await voting.initialize(token.address, pct16(101), minimumAcceptanceQuorum, votingTime) + await voting.initialize(token.address, pct16(101), minimumAcceptanceQuorum, votingDuration) }) return assertRevert(async() => { - await voting.initialize(token.address, pct16(100), minimumAcceptanceQuorum, votingTime) + await voting.initialize(token.address, pct16(100), minimumAcceptanceQuorum, votingDuration) }) }) }) @@ -398,7 +400,7 @@ contract('Voting App', accounts => { beforeEach(async() => { token = await MiniMeToken.new(NULL_ADDRESS, NULL_ADDRESS, 0, 'n', 0, 'n', true) // empty parameters minime - await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingTime) + await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingDuration) }) it('fails creating a survey if token has no holder', async () => { @@ -419,7 +421,7 @@ contract('Voting App', accounts => { await token.generateTokens(holder, 1) - await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingTime) + await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingDuration) }) it('new vote cannot be executed before voting', async () => { @@ -469,7 +471,7 @@ contract('Voting App', accounts => { await token.generateTokens(holder1, 1) await token.generateTokens(holder2, 2) - await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingTime) + await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingDuration) }) it('new vote cannot be executed before holder2 voting', async () => { @@ -508,7 +510,7 @@ contract('Voting App', accounts => { await token.generateTokens(holder1, 1) await token.generateTokens(holder2, 1) - await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingTime) + await voting.initialize(token.address, neededSupport, minimumAcceptanceQuorum, votingDuration) }) it('uses the correct snapshot value if tokens are minted afterwards', async () => {