Payroll: Fix exchange rates request call (#797)

future-apps/payroll/contracts/Payroll.sol

@@ -690,9 +690,7 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
             return ONE;
         }
 
-        uint128 xrt;
-        uint64 when;
-        (xrt, when) = feed.get(denominationToken, _token);
+        (uint128 xrt, uint64 when) = feed.get(_token, denominationToken);
 
         // Check the price feed is recent enough
         if (getTimestamp64().sub(when) >= rateExpiryTime) {
@@ -710,6 +708,7 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
      * @return True if there was at least one token transfer
      */
     function _transferTokensAmount(uint256 _employeeId, PaymentType _type, uint256 _totalAmount) internal returns (bool somethingPaid) {
+        if (_totalAmount == 0) return false;
         Employee storage employee = employees[_employeeId];
         string memory paymentReference = _paymentReferenceFor(_type);
         for (uint256 i = 0; i < allowedTokensArray.length; i++) {

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

@@ -1,29 +1,36 @@
 pragma solidity ^0.4.24;
 
-import "@aragon/ppf-contracts/contracts/IFeed.sol";
+import "@aragon/ppf-contracts/contracts/PPF.sol";
 import "@aragon/test-helpers/contracts/TimeHelpersMock.sol";
 
 
-contract PriceFeedMock is IFeed, TimeHelpersMock {
+contract PriceFeedMock is PPF, TimeHelpersMock {
+    // Set operator to address(0) so invalid signatures can pass
+    constructor () PPF(address(0), msg.sender) public {
+        // solium-disable-previous-line no-empty-blocks
+    }
 
-    event PriceFeedLogSetRate(address sender, address token, uint128 value);
+    // Overriding function for testing purposes, removing check for zero address operator
+    function _setOperator(address _operator) internal {
+        // require(_operator != address(0));
+        operator = _operator;
+        emit SetOperator(_operator);
+    }
 
-    function get(address base, address quote) external view returns (uint128 xrt, uint64 when) {
-        xrt = toInt(quote);
-        when = getTimestamp64();
+    // Overwrite function using TimeHelpers and allowing to set past rates
+    function update(address base, address quote, uint128 xrt, uint64 when, bytes sig) public {
+        bytes32 pair = super.pairId(base, quote);
 
-        emit PriceFeedLogSetRate(msg.sender, quote, xrt);
-    }
+        // Remove check that ensures a given rate is more recent than the current value
+        // require(when > feed[pair].when && when <= getTimestamp());
+        require(xrt > 0); // Make sure xrt is not 0, as the math would break (Dividing by 0 sucks big time)
+        require(base != quote); // Assumption that currency units are fungible and xrt should always be 1
 
-    /// Gets the first byte of an address as an integer
-    function toInt(address x) public pure returns(uint128 i) {
-        uint256 j = uint256(x);
-        j = j >> 152;
-        if (j == 0)
-            j = 10**15;
-        else
-            j = j * 10**18;
-        i = uint128(j);
-    }
+        bytes32 h = super.setHash(base, quote, xrt, when);
+        require(h.personalRecover(sig) == operator); // Make sure the update was signed by the operator
+
+        feed[pair] = Price(super.pairXRT(base, quote, xrt), when);
 
+        emit SetRate(base, quote, xrt, when);
+    }
 }

future-apps/payroll/package.json

@@ -41,7 +41,7 @@
   "dependencies": {
     "@aragon/apps-finance": "3.0.0",
     "@aragon/os": "4.1.0",
-    "@aragon/ppf-contracts": "1.0.2"
+    "@aragon/ppf-contracts": "1.1.0"
   },
   "devDependencies": {
     "@aragon/apps-shared-migrations": "1.0.0",

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

@@ -1,12 +1,12 @@
 const { assertRevert } = require('@aragon/test-helpers/assertThrow')
 const { getEvents, getEventArgument } = require('../helpers/events')
 const { maxUint64, annualSalaryPerSecond } = require('../helpers/numbers')(web3)
-const { deployErc20TokenAndDeposit, deployContracts, createPayrollInstance, mockTimestamps } = require('../helpers/setup.js')(artifacts, web3)
+const { deployErc20TokenAndDeposit, deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy.js')(artifacts, web3)
 
 const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'
 
 contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyone]) => {
-  let dao, payroll, payrollBase, finance, vault, priceFeed, denominationToken, anotherToken
+  let dao, payroll, payrollBase, finance, vault, priceFeed, denominationToken
 
   const NOW = 1553703809 // random fixed timestamp in seconds
   const ONE_MONTH = 60 * 60 * 24 * 31
@@ -17,15 +17,13 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon
 
   const currentTimestamp = async () => payroll.getTimestampPublic()
 
-  before('setup base apps and tokens', async () => {
-    ({ dao, finance, vault, priceFeed, payrollBase } = await deployContracts(owner))
-    anotherToken = await deployErc20TokenAndDeposit(owner, finance, vault, 'Another token', TOKEN_DECIMALS)
-    denominationToken = await deployErc20TokenAndDeposit(owner, finance, vault, 'Denomination Token', TOKEN_DECIMALS)
+  before('deploy base apps and tokens', async () => {
+    ({ dao, finance, vault, payrollBase } = await deployContracts(owner))
+    denominationToken = await deployErc20TokenAndDeposit(owner, finance, 'Denomination Token', TOKEN_DECIMALS)
   })
 
-  beforeEach('setup payroll instance', async () => {
-    payroll = await createPayrollInstance(dao, payrollBase, owner)
-    await mockTimestamps(payroll, priceFeed, NOW)
+  beforeEach('create payroll and price feed instance', async () => {
+    ({ payroll, priceFeed } = await createPayrollAndPriceFeed(dao, payrollBase, owner, NOW))
   })
 
   describe('addEmployeeNow', () => {
@@ -39,7 +37,6 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon
 
       context('when the sender has permissions to add employees', () => {
         const from = owner
-        let receipt, employeeId
 
         context('when the employee has not been added yet', () => {
           let receipt, employeeId

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

@@ -1,13 +1,14 @@
 const PAYMENT_TYPES = require('../helpers/payment_types')
+const setTokenRates = require('../helpers/set_token_rates')(web3)
 const { getEvent } = require('../helpers/events')
 const { assertRevert } = require('@aragon/test-helpers/assertThrow')
-const { deployErc20TokenAndDeposit, deployContracts, createPayrollInstance, mockTimestamps } = require('../helpers/setup.js')(artifacts, web3)
+const { deployErc20TokenAndDeposit, deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy.js')(artifacts, web3)
 
 const MAX_GAS_USED = 6.5e6
 const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'
 
-contract('Payroll allowed tokens,', ([owner, employee, anotherEmployee, anyone]) => {
-  let dao, payroll, payrollBase, finance, vault, priceFeed, denominationToken, anotherToken
+contract('Payroll allowed tokens,', ([owner, employee, anyone]) => {
+  let dao, payroll, payrollBase, finance, vault, priceFeed, denominationToken
 
   const NOW = 1553703809 // random fixed timestamp in seconds
   const ONE_MONTH = 60 * 60 * 24 * 31
@@ -16,15 +17,13 @@ contract('Payroll allowed tokens,', ([owner, employee, anotherEmployee, anyone])
 
   const TOKEN_DECIMALS = 18
 
-  before('setup base apps and tokens', async () => {
-    ({ dao, finance, vault, priceFeed, payrollBase } = await deployContracts(owner))
-    anotherToken = await deployErc20TokenAndDeposit(owner, finance, vault, 'Another token', TOKEN_DECIMALS)
-    denominationToken = await deployErc20TokenAndDeposit(owner, finance, vault, 'Denomination Token', TOKEN_DECIMALS)
+  before('deploy base apps and tokens', async () => {
+    ({ dao, finance, vault, payrollBase } = await deployContracts(owner))
+    denominationToken = await deployErc20TokenAndDeposit(owner, finance, 'Denomination Token', TOKEN_DECIMALS)
   })
 
-  beforeEach('setup payroll instance', async () => {
-    payroll = await createPayrollInstance(dao, payrollBase, owner)
-    await mockTimestamps(payroll, priceFeed, NOW)
+  beforeEach('create payroll and price feed instance', async () => {
+    ({ payroll, priceFeed } = await createPayrollAndPriceFeed(dao, payrollBase, owner, NOW))
   })
 
   describe('addAllowedToken', () => {
@@ -58,8 +57,8 @@ contract('Payroll allowed tokens,', ([owner, employee, anotherEmployee, anyone])
           })
 
           it('can allow multiple tokens', async () => {
-            const erc20Token1 = await deployErc20TokenAndDeposit(owner, finance, vault, 'Token 1', 18)
-            const erc20Token2 = await deployErc20TokenAndDeposit(owner, finance, vault, 'Token 2', 16)
+            const erc20Token1 = await deployErc20TokenAndDeposit(owner, finance, 'Token 1', 18)
+            const erc20Token2 = await deployErc20TokenAndDeposit(owner, finance, 'Token 2', 16)
 
             await payroll.addAllowedToken(denominationToken.address, { from })
             await payroll.addAllowedToken(erc20Token1.address, { from })
@@ -75,23 +74,26 @@ contract('Payroll allowed tokens,', ([owner, employee, anotherEmployee, anyone])
         context('when it reaches the maximum amount allowed', () => {
           let tokenAddresses = [], MAX_ALLOWED_TOKENS
 
-          before('deploy multiple tokens', async () => {
-            MAX_ALLOWED_TOKENS = (await payroll.getMaxAllowedTokens()).valueOf()
+          before('deploy multiple tokens and set rates', async () => {
+            MAX_ALLOWED_TOKENS = (await payrollBase.getMaxAllowedTokens()).valueOf()
             for (let i = 0; i < MAX_ALLOWED_TOKENS; i++) {
-              const token = await deployErc20TokenAndDeposit(owner, finance, vault, `Token ${i}`, 18);
+              const token = await deployErc20TokenAndDeposit(owner, finance, `Token ${i}`, 18);
               tokenAddresses.push(token.address)
             }
           })
 
-          beforeEach('allow tokens and add employee', async () => {
+          beforeEach('allow tokens, set rates, and add employee', async () => {
             await Promise.all(tokenAddresses.map(address => payroll.addAllowedToken(address, { from: owner })))
             assert.equal(await payroll.getAllowedTokensArrayLength(), MAX_ALLOWED_TOKENS, 'amount of allowed tokens does not match')
 
+            const rates = tokenAddresses.map(() => 5)
+            await setTokenRates(priceFeed, denominationToken, tokenAddresses, rates)
+
             await payroll.addEmployee(employee, 100000, 'Boss', NOW - ONE_MONTH, { from: owner })
           })
 
           it('can not add one more token', async () => {
-            const erc20Token = await deployErc20TokenAndDeposit(owner, finance, vault, 'Extra token', 18)
+            const erc20Token = await deployErc20TokenAndDeposit(owner, finance, 'Extra token', 18)
 
             await assertRevert(payroll.addAllowedToken(erc20Token.address), 'PAYROLL_MAX_ALLOWED_TOKENS')
           })
@@ -100,10 +102,10 @@ contract('Payroll allowed tokens,', ([owner, employee, anotherEmployee, anyone])
             const allocations = tokenAddresses.map(() => 100 / MAX_ALLOWED_TOKENS)
 
             const allocationTx = await payroll.determineAllocation(tokenAddresses, allocations, { from: employee })
-            assert.isBelow(allocationTx.receipt.cumulativeGasUsed, MAX_GAS_USED, 'Too much gas consumed for allocation')
+            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 })
-            assert.isBelow(paydayTx.receipt.cumulativeGasUsed, MAX_GAS_USED, 'Too much gas consumed for payday')
+            assert.isBelow(paydayTx.receipt.cumulativeGasUsed, MAX_GAS_USED, 'too much gas consumed for payday')
           })
         })
       })

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

@@ -1,29 +1,33 @@
 const PAYMENT_TYPES = require('../helpers/payment_types')
+const setTokenRates = require('../helpers/set_token_rates')(web3)
 const { assertRevert } = require('@aragon/test-helpers/assertThrow')
+const { bn, maxUint256 } = require('../helpers/numbers')(web3)
 const { getEvents, getEventArgument } = require('../helpers/events')
-const { bigExp, maxUint256 } = require('../helpers/numbers')(web3)
-const { deployErc20TokenAndDeposit, deployContracts, createPayrollInstance, mockTimestamps } = require('../helpers/setup.js')(artifacts, web3)
+const { deployErc20TokenAndDeposit, deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy.js')(artifacts, web3)
 
 contract('Payroll bonuses', ([owner, employee, anyone]) => {
-  let dao, payroll, payrollBase, finance, vault, priceFeed, denominationToken, anotherToken
+  let dao, payroll, payrollBase, finance, vault, priceFeed, denominationToken, anotherToken, anotherTokenRate
 
   const NOW = 1553703809 // random fixed timestamp in seconds
   const ONE_MONTH = 60 * 60 * 24 * 31
   const TWO_MONTHS = ONE_MONTH * 2
   const RATE_EXPIRATION_TIME = TWO_MONTHS
 
-  const PCT_ONE = bigExp(1, 18)
   const TOKEN_DECIMALS = 18
 
-  before('setup base apps and tokens', async () => {
-    ({ dao, finance, vault, priceFeed, payrollBase } = await deployContracts(owner))
-    anotherToken = await deployErc20TokenAndDeposit(owner, finance, vault, 'Another token', TOKEN_DECIMALS)
-    denominationToken = await deployErc20TokenAndDeposit(owner, finance, vault, 'Denomination Token', TOKEN_DECIMALS)
+  const increaseTime = async seconds => {
+    await payroll.mockIncreaseTime(seconds)
+    await priceFeed.mockIncreaseTime(seconds)
+  }
+
+  before('deploy base apps and tokens', async () => {
+    ({ dao, finance, vault, payrollBase } = await deployContracts(owner))
+    anotherToken = await deployErc20TokenAndDeposit(owner, finance, 'Another token', TOKEN_DECIMALS)
+    denominationToken = await deployErc20TokenAndDeposit(owner, finance, 'Denomination Token', TOKEN_DECIMALS)
   })
 
-  beforeEach('setup payroll instance', async () => {
-    payroll = await createPayrollInstance(dao, payrollBase, owner)
-    await mockTimestamps(payroll, priceFeed, NOW)
+  beforeEach('create payroll and price feed instance', async () => {
+    ({ payroll, priceFeed } = await createPayrollAndPriceFeed(dao, payrollBase, owner, NOW))
   })
 
   describe('addBonus', () => {
@@ -100,7 +104,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
           context('when the given employee is not active', () => {
             beforeEach('terminate employee', async () => {
               await payroll.terminateEmployeeNow(employeeId, { from: owner })
-              await payroll.mockIncreaseTime(ONE_MONTH)
+              await increaseTime(ONE_MONTH)
             })
 
             it('reverts', async () => {
@@ -145,6 +149,11 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
         await payroll.initialize(finance.address, denominationToken.address, priceFeed.address, RATE_EXPIRATION_TIME, { from: owner })
       })
 
+      beforeEach('set token rates', async () => {
+        anotherTokenRate = bn(5)
+        await setTokenRates(priceFeed, denominationToken, [anotherToken], [anotherTokenRate])
+      })
+
       context('when the sender is an employee', () => {
         const from = employee
         let employeeId, salary = 1000
@@ -153,7 +162,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
           const receipt = await payroll.addEmployeeNow(employee, salary, 'Boss')
           employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId')
 
-          await payroll.mockIncreaseTime(ONE_MONTH)
+          await increaseTime(ONE_MONTH)
         })
 
         context('when the employee has already set some token allocations', () => {
@@ -189,7 +198,6 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
                 assert.equal(currentDenominationTokenBalance.toString(), expectedDenominationTokenBalance.toString(), 'current denomination token balance does not match')
 
                 const currentAnotherTokenBalance = await anotherToken.balanceOf(employee)
-                const anotherTokenRate = (await priceFeed.get(denominationToken.address, anotherToken.address))[0].div(PCT_ONE)
                 const expectedAnotherTokenBalance = anotherTokenRate.mul(requestedAnotherTokenAmount).plus(previousAnotherTokenBalance).trunc()
                 assert.equal(currentAnotherTokenBalance.toString(), expectedAnotherTokenBalance.toString(), 'current token balance does not match')
               })
@@ -206,7 +214,6 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
                 assert.equal(denominationTokenEvent.amount.toString(), requestedDenominationTokenAmount, 'payment amount does not match')
                 assert.equal(denominationTokenEvent.paymentReference, 'Bonus', 'payment reference does not match')
 
-                const anotherTokenRate = (await priceFeed.get(denominationToken.address, anotherToken.address))[0].div(PCT_ONE)
                 const anotherTokenEvent = events.find(e => e.args.token === anotherToken.address).args
                 assert.equal(anotherTokenEvent.employee, employee, 'employee address does not match')
                 assert.equal(anotherTokenEvent.token, anotherToken.address, 'token address does not match')
@@ -236,7 +243,8 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
 
               context('when exchange rates are expired', () => {
                 beforeEach('expire exchange rates', async () => {
-                  await priceFeed.mockSetTimestamp(NOW - TWO_MONTHS)
+                  const expiredTimestamp = (await payroll.getTimestampPublic()).sub(RATE_EXPIRATION_TIME + 1)
+                  await setTokenRates(priceFeed, denominationToken, [anotherToken], [anotherTokenRate], expiredTimestamp)
                 })
 
                 it('reverts', async () => {
@@ -288,7 +296,8 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
 
                   context('when exchange rates are expired', () => {
                     beforeEach('expire exchange rates', async () => {
-                      await priceFeed.mockSetTimestamp(NOW - TWO_MONTHS)
+                      const expiredTimestamp = (await payroll.getTimestampPublic()).sub(RATE_EXPIRATION_TIME + 1)
+                      await setTokenRates(priceFeed, denominationToken, [anotherToken], [anotherTokenRate], expiredTimestamp)
                     })
 
                     it('reverts', async () => {
@@ -378,7 +387,8 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
 
                   context('when exchange rates are expired', () => {
                     beforeEach('expire exchange rates', async () => {
-                      await priceFeed.mockSetTimestamp(NOW - TWO_MONTHS)
+                      const expiredTimestamp = (await payroll.getTimestampPublic()).sub(RATE_EXPIRATION_TIME + 1)
+                      await setTokenRates(priceFeed, denominationToken, [anotherToken], [anotherTokenRate], expiredTimestamp)
                     })
 
                     it('reverts', async () => {