From 5a6570a5f7b5cccecd7cbc3be64dbf346592dc26 Mon Sep 17 00:00:00 2001 From: Brett Sun Date: Fri, 26 Apr 2019 23:20:18 +0200 Subject: [PATCH] Payroll: revert to old way of obtaining exchange rate and add more clarity (#821) --- future-apps/payroll/contracts/Payroll.sol | 26 ++++++++++++++----- .../test/contracts/Payroll_bonuses.test.js | 6 ++--- .../test/contracts/Payroll_payday.test.js | 14 +++++----- .../contracts/Payroll_reimbursements.test.js | 6 ++--- .../Payroll_terminate_employee.test.js | 9 ++++--- future-apps/payroll/test/helpers/tokens.js | 9 +++++++ 6 files changed, 46 insertions(+), 24 deletions(-) diff --git a/future-apps/payroll/contracts/Payroll.sol b/future-apps/payroll/contracts/Payroll.sol index b53bbf05a4..20f202e7ee 100644 --- a/future-apps/payroll/contracts/Payroll.sol +++ b/future-apps/payroll/contracts/Payroll.sol @@ -655,17 +655,23 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp { } /** - * @dev Get token exchange rate for a token based on the denomination token - * @param _token Token + * @dev Get token exchange rate for a token based on the denomination token. + * If the denomination token was USD and ETH's price was 100USD, + * this would return 0.01 for ETH. + * @param _token Token to get price of in denomination tokens * @return ONE if _token is denominationToken or 0 if the exchange rate isn't recent enough */ - function _getExchangeRate(address _token) internal view returns (uint128) { + function _getExchangeRateInDenominationToken(address _token) internal view returns (uint128) { // Denomination token has always exchange rate of 1 if (_token == denominationToken) { return ONE; } - (uint128 xrt, uint64 when) = feed.get(_token, denominationToken); + // xrt is the number of `_token` that can be exchanged for one `denominationToken` + (uint128 xrt, uint64 when) = feed.get( + denominationToken, // Base (e.g. USD) + _token // Quote (e.g. ETH) + ); // Check the price feed is recent enough if (getTimestamp64().sub(when) >= rateExpiryTime) { @@ -692,10 +698,16 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp { address token = allowedTokensArray[i]; uint256 tokenAllocation = employee.allocation[token]; if (tokenAllocation != uint256(0)) { - uint256 exchangeRate = uint256(_getExchangeRate(token)); + // Get the exchange rate for the token in denomination token, + // as we do accounting in denomination tokens + uint128 exchangeRate = _getExchangeRateInDenominationToken(token); require(exchangeRate > 0, ERROR_EXCHANGE_RATE_ZERO); - uint256 tokenAmount = _totalAmount.mul(tokenAllocation).div(exchangeRate).mul(ONE / 100); - // Salary converted to token and applied allocation percentage + + // Salary (in denomination tokens) converted to payout token + // and applied allocation percentage + uint256 tokenAmount = _totalAmount.mul(exchangeRate).mul(tokenAllocation); + // Divide by 100 for the allocation and by ONE for the exchange rate precision + tokenAmount = tokenAmount / (100 * ONE); finance.newImmediatePayment(token, employeeAddress, tokenAmount, paymentReference); emit SendPayment(employeeAddress, token, tokenAmount, paymentReference); diff --git a/future-apps/payroll/test/contracts/Payroll_bonuses.test.js b/future-apps/payroll/test/contracts/Payroll_bonuses.test.js index 74f23bcc04..fc40dc1ee5 100644 --- a/future-apps/payroll/test/contracts/Payroll_bonuses.test.js +++ b/future-apps/payroll/test/contracts/Payroll_bonuses.test.js @@ -4,7 +4,7 @@ const { getEvents, getEventArgument } = require('../helpers/events') const { NOW, ONE_MONTH, RATE_EXPIRATION_TIME } = require('../helpers/time') const { bn, bigExp, annualSalaryPerSecond, ONE, MAX_UINT256 } = require('../helpers/numbers')(web3) const { deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy')(artifacts, web3) -const { USD, deployDAI, deployANT, DAI_RATE, ANT_RATE, setTokenRates } = require('../helpers/tokens')(artifacts, web3) +const { USD, DAI_RATE, ANT_RATE, exchangedAmount, deployDAI, deployANT, setTokenRates } = require('../helpers/tokens')(artifacts, web3) contract('Payroll bonuses', ([owner, employee, anyone]) => { let dao, payroll, payrollBase, finance, vault, priceFeed, DAI, ANT @@ -177,8 +177,8 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => { }) const assertTransferredAmounts = (requestedAmount, expectedRequestedAmount = requestedAmount) => { - const requestedDAI = expectedRequestedAmount.div(DAI_RATE).mul(allocationDAI).mul(ONE.div(100)).trunc() - const requestedANT = expectedRequestedAmount.div(ANT_RATE).mul(allocationANT).mul(ONE.div(100)).trunc() + const requestedDAI = exchangedAmount(expectedRequestedAmount, DAI_RATE, allocationDAI) + const requestedANT = exchangedAmount(expectedRequestedAmount, ANT_RATE, allocationANT) it('transfers all the pending bonus', async () => { const previousDAI = await DAI.balanceOf(employee) diff --git a/future-apps/payroll/test/contracts/Payroll_payday.test.js b/future-apps/payroll/test/contracts/Payroll_payday.test.js index 485d9e7af9..ab289d0f48 100644 --- a/future-apps/payroll/test/contracts/Payroll_payday.test.js +++ b/future-apps/payroll/test/contracts/Payroll_payday.test.js @@ -4,7 +4,7 @@ const { getEventArgument } = require('../helpers/events') const { bn, ONE, MAX_UINT256, bigExp } = require('../helpers/numbers')(web3) const { NOW, ONE_MONTH, TWO_MONTHS, RATE_EXPIRATION_TIME } = require('../helpers/time') const { deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy')(artifacts, web3) -const { USD, deployDAI, deployANT, DAI_RATE, ANT_RATE, setTokenRates } = require('../helpers/tokens')(artifacts, web3) +const { USD, DAI_RATE, ANT_RATE, exchangedAmount, deployDAI, deployANT, setTokenRates } = require('../helpers/tokens')(artifacts, web3) contract('Payroll payday', ([owner, employee, anyone]) => { let dao, payroll, payrollBase, finance, vault, priceFeed, DAI, ANT @@ -57,8 +57,8 @@ contract('Payroll payday', ([owner, employee, anyone]) => { }) const assertTransferredAmounts = (requestedAmount, expectedRequestedAmount = requestedAmount) => { - const requestedDAI = expectedRequestedAmount.div(DAI_RATE).mul(allocationDAI).trunc().mul(ONE.div(100)) - const requestedANT = expectedRequestedAmount.div(ANT_RATE).mul(allocationANT).trunc().mul(ONE.div(100)) + const requestedDAI = exchangedAmount(expectedRequestedAmount, DAI_RATE, allocationDAI) + const requestedANT = exchangedAmount(expectedRequestedAmount, ANT_RATE, allocationANT) it('transfers the requested salary amount', async () => { const previousDAI = await DAI.balanceOf(employee) @@ -104,8 +104,8 @@ contract('Payroll payday', ([owner, employee, anyone]) => { await payroll.payday(PAYMENT_TYPES.PAYROLL, requestedAmount, { from }) const newOwedAmount = salary.mul(ONE_MONTH) - const newDAIAmount = newOwedAmount.div(DAI_RATE).mul(allocationDAI).trunc().mul(ONE.div(100)) - const newANTAmount = newOwedAmount.div(ANT_RATE).mul(allocationANT).trunc().mul(ONE.div(100)) + const newDAIAmount = exchangedAmount(newOwedAmount, DAI_RATE, allocationDAI) + const newANTAmount = exchangedAmount(newOwedAmount, ANT_RATE, allocationANT) await increaseTime(ONE_MONTH) await setTokenRates(priceFeed, USD, [DAI, ANT], [DAI_RATE, ANT_RATE]) @@ -615,8 +615,8 @@ contract('Payroll payday', ([owner, employee, anyone]) => { const requestedAmount = bigExp(1000, 18) const assertTransferredAmounts = requestedAmount => { - const requestedDAI = requestedAmount.div(DAI_RATE).mul(allocationDAI).mul(ONE.div(100)).trunc() - const requestedANT = requestedAmount.div(ANT_RATE).mul(allocationANT).mul(ONE.div(100)).trunc() + const requestedDAI = exchangedAmount(requestedAmount, DAI_RATE, allocationDAI) + const requestedANT = exchangedAmount(requestedAmount, ANT_RATE, allocationANT) it('transfers the requested salary amount', async () => { const previousDAI = await DAI.balanceOf(employee) diff --git a/future-apps/payroll/test/contracts/Payroll_reimbursements.test.js b/future-apps/payroll/test/contracts/Payroll_reimbursements.test.js index eb361be65a..4f6321b0b0 100644 --- a/future-apps/payroll/test/contracts/Payroll_reimbursements.test.js +++ b/future-apps/payroll/test/contracts/Payroll_reimbursements.test.js @@ -4,7 +4,7 @@ const { getEvents, getEventArgument } = require('../helpers/events') const { NOW, ONE_MONTH, RATE_EXPIRATION_TIME } = require('../helpers/time') const { deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy')(artifacts, web3) const { ONE, bn, bigExp, annualSalaryPerSecond, MAX_UINT256 } = require('../helpers/numbers')(web3) -const { USD, deployDAI, deployANT, DAI_RATE, ANT_RATE, setTokenRates } = require('../helpers/tokens')(artifacts, web3) +const { USD, DAI_RATE, ANT_RATE, exchangedAmount, deployDAI, deployANT, setTokenRates } = require('../helpers/tokens')(artifacts, web3) contract('Payroll reimbursements', ([owner, employee, anyone]) => { let dao, payroll, payrollBase, finance, vault, priceFeed, DAI, ANT @@ -166,8 +166,8 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => { }) const assertTransferredAmounts = (requestedAmount, expectedRequestedAmount = requestedAmount) => { - const requestedDAI = expectedRequestedAmount.div(DAI_RATE).mul(allocationDAI).mul(ONE.div(100)).trunc() - const requestedANT = expectedRequestedAmount.div(ANT_RATE).mul(allocationANT).mul(ONE.div(100)).trunc() + const requestedDAI = exchangedAmount(expectedRequestedAmount, DAI_RATE, allocationDAI) + const requestedANT = exchangedAmount(expectedRequestedAmount, ANT_RATE, allocationANT) it('transfers all the pending reimbursements', async () => { const previousDAI = await DAI.balanceOf(employee) diff --git a/future-apps/payroll/test/contracts/Payroll_terminate_employee.test.js b/future-apps/payroll/test/contracts/Payroll_terminate_employee.test.js index 381c24cfc0..9eb687f4de 100644 --- a/future-apps/payroll/test/contracts/Payroll_terminate_employee.test.js +++ b/future-apps/payroll/test/contracts/Payroll_terminate_employee.test.js @@ -2,7 +2,7 @@ const PAYMENT_TYPES = require('../helpers/payment_types') const { assertRevert } = require('@aragon/test-helpers/assertThrow') const { getEvents, getEventArgument } = require('../helpers/events') const { NOW, ONE_MONTH, RATE_EXPIRATION_TIME } = require('../helpers/time') -const { USD, DAI_RATE, deployDAI, setTokenRate } = require('../helpers/tokens')(artifacts, web3) +const { USD, DAI_RATE, exchangedAmount, deployDAI, setTokenRate } = require('../helpers/tokens')(artifacts, web3) const { ONE, bn, bigExp, MAX_UINT64, annualSalaryPerSecond } = require('../helpers/numbers')(web3) const { deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy')(artifacts, web3) @@ -81,8 +81,6 @@ contract('Payroll employees termination', ([owner, employee, anyone]) => { // Accrue some salary and extras await increaseTime(ONE_MONTH) - // Mimic EVM truncation when calculating token amount to transfer - const owedSalary = salary.times(ONE_MONTH).div(ONE.div(100)).trunc().mul(ONE.div(100)) const reimbursement = bigExp(100000, 18) await payroll.addReimbursement(employeeId, reimbursement, { from: owner }) @@ -95,8 +93,11 @@ contract('Payroll employees termination', ([owner, employee, anyone]) => { await payroll.payday(PAYMENT_TYPES.REIMBURSEMENT, 0, { from: employee }) await assertRevert(payroll.getEmployee(employeeId), 'PAYROLL_EMPLOYEE_DOESNT_EXIST') + const owedSalaryInDai = exchangedAmount(salary.times(ONE_MONTH), DAI_RATE, 100) + const reimbursementInDai = exchangedAmount(reimbursement, DAI_RATE, 100) + const currentDAI = await DAI.balanceOf(employee) - const expectedDAI = previousDAI.plus(owedSalary).plus(reimbursement) + const expectedDAI = previousDAI.plus(owedSalaryInDai).plus(reimbursementInDai) assert.equal(currentDAI.toString(), expectedDAI.toString(), 'current balance does not match') }) diff --git a/future-apps/payroll/test/helpers/tokens.js b/future-apps/payroll/test/helpers/tokens.js index 2eee4416ce..e8684cc426 100644 --- a/future-apps/payroll/test/helpers/tokens.js +++ b/future-apps/payroll/test/helpers/tokens.js @@ -12,6 +12,14 @@ module.exports = (artifacts, web3) => { const DAI_RATE = formatRate(1) // 1 DAI = 1 USD const ANT_RATE = formatRate(0.5) // 1 ANT = 0.5 USD + function exchangedAmount(amount, rate, tokenAllocation) { + // Mimic PPF inversion truncation, as we set the denomination token always + // as the price feed's quote token + const inverseRate = ONE.pow(2).div(rate).trunc() + // Mimic EVM calculation and truncation for token conversion + return amount.mul(inverseRate).mul(tokenAllocation).div(ONE.mul(100)).trunc() + } + const deployANT = async (sender, finance) => deployTokenAndDeposit(sender, finance, 'ANT') const deployDAI = async (sender, finance) => deployTokenAndDeposit(sender, finance, 'DAI') @@ -52,6 +60,7 @@ module.exports = (artifacts, web3) => { DAI_RATE, ANT_RATE, formatRate, + exchangedAmount, deployANT, deployDAI, deployTokenAndDeposit,