payroll: support min acceptable exchange rates

future-apps/payroll/app/src/store/employees.js

@@ -23,7 +23,7 @@ export async function getSalaryAllocation(employeeId, tokens) {
       app
         .call('getAllocation', employeeId, token.address)
         .first()
-        .map(allocation => tokenAllocation({ ...token, allocation }))
+        .map(([allocation, minRate]) => tokenAllocation({ ...token, allocation, minRate }))
         .toPromise()
     )
   )

future-apps/payroll/arapp.json

@@ -67,14 +67,9 @@
       "params": ["Token", "Allowed"]
     },
     {
-      "name": "Modify Price Feed",
-      "id": "MODIFY_PRICE_FEED_ROLE",
-      "params": ["New price feed", "Old price feed"]
-    },
-    {
-      "name": "Modify Rate Expiry Time",
-      "id": "MODIFY_RATE_EXPIRY_ROLE",
-      "params": ["New rate expiry time", "Old rate expiry time"]
+      "name": "Modify Price Feed settings",
+      "id": "MODIFY_PRICE_FEED_SETTINGS_ROLE",
+      "params": ["New price feed", "Old price feed", "New rate expiry time", "Old rate expiry time"]
     }
   ],
   "path": "contracts/Payroll.sol"

future-apps/payroll/contracts/Payroll.sol

@@ -26,8 +26,7 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
     * bytes32 constant public ADD_BONUS_ROLE = keccak256("ADD_BONUS_ROLE");
     * bytes32 constant public ADD_REIMBURSEMENT_ROLE = keccak256("ADD_REIMBURSEMENT_ROLE");
     * bytes32 constant public MANAGE_ALLOWED_TOKENS_ROLE = keccak256("MANAGE_ALLOWED_TOKENS_ROLE");
-    * bytes32 constant public MODIFY_PRICE_FEED_ROLE = keccak256("MODIFY_PRICE_FEED_ROLE");
-    * bytes32 constant public MODIFY_RATE_EXPIRY_ROLE = keccak256("MODIFY_RATE_EXPIRY_ROLE");
+    * bytes32 constant public MODIFY_PRICE_FEED_SETTINGS_ROLE = keccak256("MODIFY_PRICE_FEED_SETTINGS_ROLE");
     */
 
     bytes32 constant public ADD_EMPLOYEE_ROLE = 0x9ecdc3c63716b45d0756eece5fe1614cae1889ec5a1ce62b3127c1f1f1615d6e;
@@ -36,8 +35,7 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
     bytes32 constant public ADD_BONUS_ROLE = 0xceca7e2f5eb749a87aaf68f3f76d6b9251aa2f4600f13f93c5a4adf7a72df4ae;
     bytes32 constant public ADD_REIMBURSEMENT_ROLE = 0x90698b9d54427f1e41636025017309bdb1b55320da960c8845bab0a504b01a16;
     bytes32 constant public MANAGE_ALLOWED_TOKENS_ROLE = 0x0be34987c45700ee3fae8c55e270418ba903337decc6bacb1879504be9331c06;
-    bytes32 constant public MODIFY_PRICE_FEED_ROLE = 0x74350efbcba8b85341c5bbf70cc34e2a585fc1463524773a12fa0a71d4eb9302;
-    bytes32 constant public MODIFY_RATE_EXPIRY_ROLE = 0x79fe989a8899060dfbdabb174ebb96616fa9f1d9dadd739f8d814cbab452404e;
+    bytes32 constant public MODIFY_PRICE_FEED_SETTINGS_ROLE = 0xd5da0a7d4d69a338f4ff576cbdd0b341bc45341d64c650c4f63b3bde43509faf;
 
     uint256 internal constant MAX_ALLOWED_TOKENS = 20; // prevent OOG issues with `payday()`
     uint64 internal constant MIN_RATE_EXPIRY = uint64(1 minutes); // 1 min == ~4 block window to mine both a price feed update and a payout
@@ -63,15 +61,20 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
     string private constant ERROR_FEED_NOT_CONTRACT = "PAYROLL_FEED_NOT_CONTRACT";
     string private constant ERROR_EXPIRY_TIME_TOO_SHORT = "PAYROLL_EXPIRY_TIME_TOO_SHORT";
     string private constant ERROR_PAST_TERMINATION_DATE = "PAYROLL_PAST_TERMINATION_DATE";
-    string private constant ERROR_EXCHANGE_RATE_ZERO = "PAYROLL_EXCHANGE_RATE_ZERO";
+    string private constant ERROR_EXCHANGE_RATE_TOO_LOW = "PAYROLL_EXCHANGE_RATE_TOO_LOW";
     string private constant ERROR_LAST_PAYROLL_DATE_TOO_BIG = "PAYROLL_LAST_DATE_TOO_BIG";
     string private constant ERROR_INVALID_REQUESTED_AMOUNT = "PAYROLL_INVALID_REQUESTED_AMT";
 
     enum PaymentType { Payroll, Reimbursement, Bonus }
 
+    struct TokenAllocation {
+        uint256 percentage;
+        uint256 minAcceptableRate;
+    }
+
     struct Employee {
         address accountAddress; // unique, but can be changed over time
-        mapping(address => uint256) allocation;
+        mapping(address => TokenAllocation) allocation;
         uint256 denominationTokenSalary; // salary per second in denomination Token
         uint256 accruedSalary; // keep track of any leftover accrued salary when changing salaries
         uint256 bonus;
@@ -141,13 +144,14 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
     /**
      * @notice Initialize Payroll app for Finance at `_finance` and price feed at `_priceFeed`, setting denomination token to `_token` and exchange rate expiry time to `@transformTime(_rateExpiryTime)`
      * @dev Note that we do not require _denominationToken to be a contract, as it may be a "fake"
-     *      address used by the price feed to denominate fiat currencies
+     *      address used by the price feed to denominate fiat currencies.
+     *      Note that the initialization check is already guaranteed by `initialized`.
      * @param _finance Address of the Finance app this Payroll app will rely on for payments (non-changeable)
      * @param _denominationToken Address of the denomination token used for salary accounting
      * @param _priceFeed Address of the price feed
      * @param _rateExpiryTime Acceptable expiry time in seconds for the price feed's exchange rates
      */
-    function initialize(Finance _finance, address _denominationToken, IFeed _priceFeed, uint64 _rateExpiryTime) external onlyInit {
+    function initialize(Finance _finance, address _denominationToken, IFeed _priceFeed, uint64 _rateExpiryTime) external {
         initialized();
 
         require(isContract(_finance), ERROR_FINANCE_NOT_CONTRACT);
@@ -172,19 +176,15 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
     }
 
     /**
-     * @notice Set the price feed for exchange rates to `_feed`
+     * @notice Set the price feed for exchange rates to `_feed` and the acceptable rates expiry time to `@transformTime(_time)`
      * @param _feed Address of the new price feed instance
-     */
-    function setPriceFeed(IFeed _feed) external authP(MODIFY_PRICE_FEED_ROLE, arr(_feed, feed)) {
-        _setPriceFeed(_feed);
-    }
-
-    /**
-     * @notice Set the acceptable expiry time for the price feed's exchange rates to `@transformTime(_time)`
-     * @dev Exchange rates older than the given value won't be accepted for payments and will cause payouts to revert
      * @param _time The expiration time in seconds for exchange rates
      */
-    function setRateExpiryTime(uint64 _time) external authP(MODIFY_RATE_EXPIRY_ROLE, arr(uint256(_time), uint256(rateExpiryTime))) {
+    function setPriceFeedSettings(IFeed _feed, uint64 _time)
+        external
+        authP(MODIFY_PRICE_FEED_SETTINGS_ROLE, _arr(_feed, feed, uint256(_time), uint256(rateExpiryTime)))
+    {
+        _setPriceFeed(_feed);
         _setRateExpiryTime(_time);
     }
 
@@ -292,8 +292,9 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
      * @param _tokens Array of token addresses; they must belong to the list of allowed tokens
      * @param _distribution Array with each token's corresponding proportions (must be integers summing to 100)
      */
-    function determineAllocation(address[] _tokens, uint256[] _distribution) external employeeMatches nonReentrant {
-        // Check arrays match
+    function determineAllocation(address[] _tokens, uint256[] _distribution, uint256[] _minRates) external employeeMatches nonReentrant {
+        // Check arrays length match
+        require(_tokens.length == _minRates.length, ERROR_TOKEN_ALLOCATION_MISMATCH);
         require(_tokens.length == _distribution.length, ERROR_TOKEN_ALLOCATION_MISMATCH);
 
         uint256 employeeId = employeeIds[msg.sender];
@@ -307,7 +308,7 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
         // Set distributions only if given tokens are allowed
         for (uint256 i = 0; i < _distribution.length; i++) {
             require(allowedTokens[_tokens[i]], ERROR_NOT_ALLOWED_TOKEN);
-            employee.allocation[_tokens[i]] = _distribution[i];
+            employee.allocation[_tokens[i]] = TokenAllocation({ percentage: _distribution[i], minAcceptableRate: _minRates[i] });
         }
 
         _ensureEmployeeTokenAllocationsIsValid(employee);
@@ -452,9 +453,14 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
      * @param _employeeId Employee's identifier
      * @param _token Token to query the payment allocation for
      * @return Employee's payment allocation for the token being queried
+     * @return Employee's min acceptable rate for the token being queried
      */
-    function getAllocation(uint256 _employeeId, address _token) public view employeeIdExists(_employeeId) returns (uint256) {
-        return employees[_employeeId].allocation[_token];
+    function getAllocation(uint256 _employeeId, address _token) public view employeeIdExists(_employeeId)
+        returns (uint256 percentage, uint256 minAcceptableRate)
+    {
+        TokenAllocation storage allocation = employees[_employeeId].allocation[_token];
+        percentage = allocation.percentage;
+        minAcceptableRate = allocation.minAcceptableRate;
     }
 
     /**
@@ -591,15 +597,16 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
 
         for (uint256 i = 0; i < allowedTokensArray.length; i++) {
             address token = allowedTokensArray[i];
-            uint256 tokenAllocation = employee.allocation[token];
-            if (tokenAllocation != uint256(0)) {
+            TokenAllocation storage tokenAllocation = employee.allocation[token];
+            uint256 percentage = tokenAllocation.percentage;
+            if (percentage != uint256(0)) {
                 // Get the exchange rate for the payout token in denomination token,
                 // as we do accounting in denomination tokens
                 uint256 exchangeRate = _getExchangeRateInDenominationToken(token);
-                require(exchangeRate > 0, ERROR_EXCHANGE_RATE_ZERO);
+                require(exchangeRate >= tokenAllocation.minAcceptableRate, ERROR_EXCHANGE_RATE_TOO_LOW);
 
                 // Convert amount (in denomination tokens) to payout token and apply allocation
-                uint256 tokenAmount = _totalAmount.mul(exchangeRate).mul(tokenAllocation);
+                uint256 tokenAmount = _totalAmount.mul(exchangeRate).mul(percentage);
                 // Divide by 100 for the allocation percentage and by the exchange rate precision
                 tokenAmount = tokenAmount.div(100).div(feed.ratePrecision());
 
@@ -843,7 +850,7 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
     function _ensureEmployeeTokenAllocationsIsValid(Employee storage employee_) internal view {
         uint256 sum = 0;
         for (uint256 i = 0; i < allowedTokensArray.length; i++) {
-            sum = sum.add(employee_.allocation[allowedTokensArray[i]]);
+            sum = sum.add(employee_.allocation[allowedTokensArray[i]].percentage);
         }
         require(sum == 100, ERROR_DISTRIBUTION_NOT_FULL);
     }
@@ -853,4 +860,14 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
         require(_owedAmount >= _requestedAmount, ERROR_INVALID_REQUESTED_AMOUNT);
         return _requestedAmount > 0 ? _requestedAmount : _owedAmount;
     }
+
+    // Syntax sugar
+
+    function _arr(address _a, address _b, uint256 _c, uint256 _d) private pure returns (uint256[] r) {
+        r = new uint256[](4);
+        r[0] = uint256(_a);
+        r[1] = uint256(_b);
+        r[2] = _c;
+        r[3] = _d;
+    }
 }

future-apps/payroll/contracts/examples/PayrollKit.sol

@@ -212,13 +212,11 @@ contract PayrollKit is KitBase {
         address account2 = 0x8401Eb5ff34cc943f096A32EF3d5113FEbE8D4Eb;
         address account3 = 0x306469457266CBBe7c0505e8Aad358622235e768;
         address account4 = 0xd873F6DC68e3057e4B7da74c6b304d0eF0B484C7;
-        address account5 = 0xDcC5dD922fb1D0fd0c450a0636a8cE827521f0eD;
 
         uint256 salary1 = 2535047025122316; // 80000
         uint256 salary2 = 2851927903262605; // 90000
         uint256 salary3 = 3168808781402895; // 100000
         uint256 salary4 = 2218166146982026; // 70000
-        uint256 salary5 = 1901285268841737; // 60000
 
         // Set up first user; use this contract as the account so we can set up the initial distribution
         payroll.addEmployee(this, salary1, uint64(now - 86400), "CEO");
@@ -231,13 +229,16 @@ contract PayrollKit is KitBase {
         distribution[0] = 45;
         distribution[1] = 55;
 
-        payroll.determineAllocation(allowedTokens, distribution);
+        uint256[] memory minRates = new uint256[](2);
+        minRates[0] = 0;
+        minRates[1] = 0;
+
+        payroll.determineAllocation(allowedTokens, distribution, minRates);
         payroll.changeAddressByEmployee(root); // Set account to root
 
         // Create more users
         payroll.addEmployee(account2, salary2, uint64(now - 86400), "Project Manager");
         payroll.addEmployee(account3, salary3, uint64(now - 172800), "Developer");
         payroll.addEmployee(account4, salary4, uint64(now - 172800), "Developer");
-        payroll.addEmployee(account5, salary5, uint64(now - 172800), "Developer");
     }
 }

future-apps/payroll/contracts/test/mocks/MaliciousEmployee.sol

@@ -24,8 +24,8 @@ contract MaliciousEmployee {
         payroll.payday(Payroll.PaymentType.Payroll, 0);
     }
 
-    function determineAllocation(address[] _tokens, uint256[] _distribution) public {
-        payroll.determineAllocation(_tokens, _distribution);
+    function determineAllocation(address[] _tokens, uint256[] _distribution, uint256[] _minRates) public {
+        payroll.determineAllocation(_tokens, _distribution, _minRates);
     }
 
     function reenter() public {
@@ -42,9 +42,11 @@ contract MaliciousEmployee {
         } else if (action == Action.SetAllocation) {
             address[] memory tokens = new address[](1);
             tokens[0] = address(0);
-            uint256[] memory allocations = new uint256[](1);
-            allocations[0] = 100;
-            payroll.determineAllocation(tokens, allocations);
+            uint256[] memory distribution = new uint256[](1);
+            distribution[0] = 100;
+            uint256[] memory minRates = new uint256[](1);
+            minRates[0] = 1e18;
+            payroll.determineAllocation(tokens, distribution, minRates);
         }
     }
 }

future-apps/payroll/test/contracts/Payroll_allowed_tokens.test.js

@@ -90,9 +90,10 @@ contract('Payroll allowed tokens,', ([owner, employee, anyone]) => {
           })
 
           it('does not run out of gas to payout salary', async () => {
-            const allocations = tokenAddresses.map(() => 100 / MAX_ALLOWED_TOKENS)
+            const distribution = tokenAddresses.map(() => 100 / MAX_ALLOWED_TOKENS)
+            const minRates = tokenAddresses.map(() => 0)
 
-            const allocationTx = await payroll.determineAllocation(tokenAddresses, allocations, { from: employee })
+            const allocationTx = await payroll.determineAllocation(tokenAddresses, distribution, minRates, { from: employee })
             assert.isBelow(allocationTx.receipt.cumulativeGasUsed, MAX_GAS_USED, 'too much gas consumed for allocation')
 
             const paydayTx = await payroll.payday(PAYMENT_TYPES.PAYROLL, 0, { from: employee })

future-apps/payroll/test/contracts/Payroll_bonuses.test.js

@@ -165,7 +165,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
           beforeEach('set tokens allocation', async () => {
             await payroll.setAllowedToken(ANT.address, true, { from: owner })
             await payroll.setAllowedToken(DAI.address, true, { from: owner })
-            await payroll.determineAllocation([DAI.address, ANT.address], [allocationDAI, allocationANT], { from })
+            await payroll.determineAllocation([DAI.address, ANT.address], [allocationDAI, allocationANT], [DAI_RATE, ANT_RATE], { from })
           })
 
           context('when the employee has a pending bonus', () => {
@@ -246,7 +246,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
                   })
 
                   it('reverts', async () => {
-                    await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from }), 'PAYROLL_EXCHANGE_RATE_ZERO')
+                    await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from }), 'PAYROLL_EXCHANGE_RATE_TOO_LOW')
                   })
                 })
               })
@@ -310,7 +310,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
                     })
 
                     it('reverts', async () => {
-                      await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from }), 'PAYROLL_EXCHANGE_RATE_ZERO')
+                      await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from }), 'PAYROLL_EXCHANGE_RATE_TOO_LOW')
                     })
                   })
                 })
@@ -401,7 +401,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
                     })
 
                     it('reverts', async () => {
-                      await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from }), 'PAYROLL_EXCHANGE_RATE_ZERO')
+                      await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from }), 'PAYROLL_EXCHANGE_RATE_TOO_LOW')
                     })
                   })
                 })

future-apps/payroll/test/contracts/Payroll_constants.test.js

@@ -15,8 +15,7 @@ contract('Payroll roles', () => {
       assert.equal(await payroll.ADD_BONUS_ROLE(), web3.sha3('ADD_BONUS_ROLE'), 'add bonus role does not match')
       assert.equal(await payroll.ADD_REIMBURSEMENT_ROLE(), web3.sha3('ADD_REIMBURSEMENT_ROLE'), 'add reimbursement role does not match')
       assert.equal(await payroll.MANAGE_ALLOWED_TOKENS_ROLE(), web3.sha3('MANAGE_ALLOWED_TOKENS_ROLE'), 'modify allowed tokens role does not match')
-      assert.equal(await payroll.MODIFY_PRICE_FEED_ROLE(), web3.sha3('MODIFY_PRICE_FEED_ROLE'), 'modify price feed does not match')
-      assert.equal(await payroll.MODIFY_RATE_EXPIRY_ROLE(), web3.sha3('MODIFY_RATE_EXPIRY_ROLE'), 'modify rate expiry role does not match')
+      assert.equal(await payroll.MODIFY_PRICE_FEED_SETTINGS_ROLE(), web3.sha3('MODIFY_PRICE_FEED_SETTINGS_ROLE'), 'modify price feed settings role does not match')
     })
   })
 })

future-apps/payroll/test/contracts/Payroll_gas_costs.test.js

@@ -36,7 +36,7 @@ contract('Payroll gas costs', ([owner, employee, anotherEmployee]) => {
 
     context('when there is only one allowed token', function () {
       it('expends ~339k gas for a single allowed token', async () => {
-        await payroll.determineAllocation([DAI.address], [100], { from: employee })
+        await payroll.determineAllocation([DAI.address], [100], [0], { from: employee })
 
         const { receipt: { cumulativeGasUsed } } = await payroll.payday(PAYMENT_TYPES.PAYROLL, 0, { from: employee })
 
@@ -46,10 +46,10 @@ contract('Payroll gas costs', ([owner, employee, anotherEmployee]) => {
 
     context('when there are two allowed token', function () {
       it('expends ~295k gas per allowed token', async () => {
-        await payroll.determineAllocation([DAI.address], [100], { from: employee })
+        await payroll.determineAllocation([DAI.address], [100], [0], { from: employee })
         const { receipt: { cumulativeGasUsed: employeePayoutGasUsed } } = await payroll.payday(PAYMENT_TYPES.PAYROLL, 0, { from: employee })
 
-        await payroll.determineAllocation([DAI.address, ANT.address], [60, 40], { from: anotherEmployee })
+        await payroll.determineAllocation([DAI.address, ANT.address], [60, 40], [0, 0], { from: anotherEmployee })
         const { receipt: { cumulativeGasUsed: anotherEmployeePayoutGasUsed } } = await payroll.payday(PAYMENT_TYPES.PAYROLL, 0, { from: anotherEmployee })
 
         const gasPerAllowedToken = anotherEmployeePayoutGasUsed - employeePayoutGasUsed