Skip to content

Commit

Permalink
Payroll: clarify how rates are handled and add unit tests (#823)
Browse files Browse the repository at this point in the history
  • Loading branch information
@facuspagnuolo @sohkai
facuspagnuolo authored and sohkai committed Apr 27, 2019
1 parent cb9b20c commit 327abd6
Show file tree
Hide file tree
Showing 21 changed files with 1,070 additions and 604 deletions.
24 changes: 17 additions & 7 deletions future-apps/payroll/contracts/Payroll.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -692,11 +698,15 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
address token = allowedTokensArray[i];
uint256 tokenAllocation = employee.allocation[token];
if (tokenAllocation != uint256(0)) {
uint128 exchangeRate = _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);
// 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
// Divide by 100 for the allocation and by ONE for the exchange rate precision
tokenAmount = tokenAmount / (100 * ONE);

finance.newImmediatePayment(token, employeeAddress, tokenAmount, paymentReference);
Expand Down
27 changes: 11 additions & 16 deletions future-apps/payroll/test/contracts/Payroll_add_employee.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,23 +1,18 @@
const { assertRevert } = require('@aragon/test-helpers/assertThrow')
const { getEvents, getEventArgument } = require('../helpers/events')
const { maxUint64, annualSalaryPerSecond } = require('../helpers/numbers')(web3)
const { deployErc20TokenAndDeposit, deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy.js')(artifacts, web3)
const { USD, deployDAI } = require('../helpers/tokens')(artifacts, web3)
const { NOW, TWO_MONTHS, RATE_EXPIRATION_TIME } = require('../helpers/time')
const { MAX_UINT64, annualSalaryPerSecond } = require('../helpers/numbers')(web3)
const { deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy')(artifacts, web3)

const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'

contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyone]) => {
let dao, payroll, payrollBase, finance, vault, priceFeed, denominationToken

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 TOKEN_DECIMALS = 18
let dao, payroll, payrollBase, finance, vault, priceFeed, DAI

before('deploy base apps and tokens', async () => {
({ dao, finance, vault, payrollBase } = await deployContracts(owner))
denominationToken = await deployErc20TokenAndDeposit(owner, finance, 'Denomination Token', TOKEN_DECIMALS)
DAI = await deployDAI(owner, finance)
})

beforeEach('create payroll and price feed instance', async () => {
Expand All @@ -26,11 +21,11 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon

describe('addEmployee', () => {
const role = 'Boss'
const salary = annualSalaryPerSecond(100000, TOKEN_DECIMALS)
const salary = annualSalaryPerSecond(100000)

context('when it has already been initialized', function () {
beforeEach('initialize payroll app', async () => {
await payroll.initialize(finance.address, denominationToken.address, priceFeed.address, RATE_EXPIRATION_TIME, { from: owner })
beforeEach('initialize payroll app using USD as denomination token', async () => {
await payroll.initialize(finance.address, USD, priceFeed.address, RATE_EXPIRATION_TIME, { from: owner })
})

context('when the sender has permissions to add employees', () => {
Expand Down Expand Up @@ -69,7 +64,7 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon

it('can add another employee', async () => {
const anotherRole = 'Manager'
const anotherSalary = annualSalaryPerSecond(120000, TOKEN_DECIMALS)
const anotherSalary = annualSalaryPerSecond(120000)

const receipt = await payroll.addEmployee(anotherEmployee, anotherSalary, anotherRole, startDate)
const anotherEmployeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId')
Expand All @@ -91,7 +86,7 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon
assert.equal(accruedSalary, 0, 'employee accrued salary does not match')
assert.equal(employeeSalary.toString(), anotherSalary.toString(), 'employee salary does not match')
assert.equal(lastPayroll.toString(), startDate.toString(), 'employee last payroll does not match')
assert.equal(endDate.toString(), maxUint64(), 'employee end date does not match')
assert.equal(endDate.toString(), MAX_UINT64, 'employee end date does not match')
})
})

Expand Down
61 changes: 28 additions & 33 deletions future-apps/payroll/test/contracts/Payroll_allowed_tokens.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,25 +1,20 @@
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, createPayrollAndPriceFeed } = require('../helpers/deploy.js')(artifacts, web3)
const { annualSalaryPerSecond } = require('../helpers/numbers')(web3)
const { NOW, ONE_MONTH, RATE_EXPIRATION_TIME } = require('../helpers/time')
const { deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy')(artifacts, web3)
const { USD, deployDAI, deployTokenAndDeposit, setTokenRates, formatRate } = require('../helpers/tokens')(artifacts, web3)

const MAX_GAS_USED = 6.5e6
const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'

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
const TWO_MONTHS = ONE_MONTH * 2
const RATE_EXPIRATION_TIME = TWO_MONTHS

const TOKEN_DECIMALS = 18
let dao, payroll, payrollBase, finance, vault, priceFeed, DAI

before('deploy base apps and tokens', async () => {
({ dao, finance, vault, payrollBase } = await deployContracts(owner))
denominationToken = await deployErc20TokenAndDeposit(owner, finance, 'Denomination Token', TOKEN_DECIMALS)
DAI = await deployDAI(owner, finance)
})

beforeEach('create payroll and price feed instance', async () => {
Expand All @@ -28,22 +23,22 @@ contract('Payroll allowed tokens,', ([owner, employee, anyone]) => {

describe('addAllowedToken', () => {
context('when it has already been initialized', function () {
beforeEach('initialize payroll app', async () => {
await payroll.initialize(finance.address, denominationToken.address, priceFeed.address, RATE_EXPIRATION_TIME, { from: owner })
beforeEach('initialize payroll app using USD as denomination token', async () => {
await payroll.initialize(finance.address, USD, priceFeed.address, RATE_EXPIRATION_TIME, { from: owner })
})

context('when the sender has permissions', () => {
const from = owner

context('when it does not reach the maximum amount allowed', () => {
it('can allow a token', async () => {
const receipt = await payroll.addAllowedToken(denominationToken.address, { from })
const receipt = await payroll.addAllowedToken(DAI.address, { from })

const event = getEvent(receipt, 'AddAllowedToken')
assert.equal(event.token, denominationToken.address, 'denomination token address should match')
assert.equal(event.token, DAI.address, 'denomination token address should match')

assert.equal(await payroll.getAllowedTokensArrayLength(), 1, 'allowed tokens length does not match')
assert(await payroll.isTokenAllowed(denominationToken.address), 'denomination token should be allowed')
assert(await payroll.isTokenAllowed(DAI.address), 'denomination token should be allowed')
})

it('can allow a the zero address', async () => {
Expand All @@ -57,15 +52,15 @@ contract('Payroll allowed tokens,', ([owner, employee, anyone]) => {
})

it('can allow multiple tokens', async () => {
const erc20Token1 = await deployErc20TokenAndDeposit(owner, finance, 'Token 1', 18)
const erc20Token2 = await deployErc20TokenAndDeposit(owner, finance, 'Token 2', 16)
const erc20Token1 = await deployTokenAndDeposit(owner, finance, 'Token 1', 18)
const erc20Token2 = await deployTokenAndDeposit(owner, finance, 'Token 2', 16)

await payroll.addAllowedToken(denominationToken.address, { from })
await payroll.addAllowedToken(DAI.address, { from })
await payroll.addAllowedToken(erc20Token1.address, { from })
await payroll.addAllowedToken(erc20Token2.address, { from })

assert.equal(await payroll.getAllowedTokensArrayLength(), 3, 'allowed tokens length does not match')
assert(await payroll.isTokenAllowed(denominationToken.address), 'denomination token should be allowed')
assert(await payroll.isTokenAllowed(DAI.address), 'denomination token should be allowed')
assert(await payroll.isTokenAllowed(erc20Token1.address), 'ERC20 token 1 should be allowed')
assert(await payroll.isTokenAllowed(erc20Token2.address), 'ERC20 token 2 should be allowed')
})
Expand All @@ -77,7 +72,7 @@ contract('Payroll allowed tokens,', ([owner, employee, anyone]) => {
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, `Token ${i}`, 18);
const token = await deployTokenAndDeposit(owner, finance, `Token ${i}`, 18);
tokenAddresses.push(token.address)
}
})
Expand All @@ -86,14 +81,14 @@ contract('Payroll allowed tokens,', ([owner, employee, anyone]) => {
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)
const rates = tokenAddresses.map(() => formatRate(5))
await setTokenRates(priceFeed, USD, tokenAddresses, rates)

await payroll.addEmployee(employee, 100000, 'Boss', NOW - ONE_MONTH, { from: owner })
await payroll.addEmployee(employee, annualSalaryPerSecond(100000), 'Boss', NOW - ONE_MONTH, { from: owner })
})

it('can not add one more token', async () => {
const erc20Token = await deployErc20TokenAndDeposit(owner, finance, 'Extra token', 18)
const erc20Token = await deployTokenAndDeposit(owner, finance, 'Extra token', 18)

await assertRevert(payroll.addAllowedToken(erc20Token.address), 'PAYROLL_MAX_ALLOWED_TOKENS')
})
Expand All @@ -114,38 +109,38 @@ contract('Payroll allowed tokens,', ([owner, employee, anyone]) => {
const from = anyone

it('reverts', async () => {
await assertRevert(payroll.addAllowedToken(denominationToken.address, { from }), 'APP_AUTH_FAILED')
await assertRevert(payroll.addAllowedToken(DAI.address, { from }), 'APP_AUTH_FAILED')
})
})
})

context('when it has not been initialized yet', function () {
it('reverts', async () => {
await assertRevert(payroll.addAllowedToken(denominationToken.address, { from: owner }), 'APP_AUTH_FAILED')
await assertRevert(payroll.addAllowedToken(DAI.address, { from: owner }), 'APP_AUTH_FAILED')
})
})
})

describe('isTokenAllowed', () => {
context('when it has already been initialized', function () {
beforeEach('initialize payroll app', async () => {
await payroll.initialize(finance.address, denominationToken.address, priceFeed.address, RATE_EXPIRATION_TIME, { from: owner })
beforeEach('initialize payroll app using USD as denomination token', async () => {
await payroll.initialize(finance.address, USD, priceFeed.address, RATE_EXPIRATION_TIME, { from: owner })
})

context('when the given token is not the zero address', () => {
context('when the requested token was allowed', () => {
beforeEach('allow denomination token', async () => {
await payroll.addAllowedToken(denominationToken.address, { from: owner })
await payroll.addAllowedToken(DAI.address, { from: owner })
})

it('returns true', async () => {
assert(await payroll.isTokenAllowed(denominationToken.address), 'token should be allowed')
assert(await payroll.isTokenAllowed(DAI.address), 'token should be allowed')
})
})

context('when the requested token was not allowed yet', () => {
it('returns false', async () => {
assert.isFalse(await payroll.isTokenAllowed(denominationToken.address), 'token should not be allowed')
assert.isFalse(await payroll.isTokenAllowed(DAI.address), 'token should not be allowed')
})
})
})
Expand All @@ -159,7 +154,7 @@ contract('Payroll allowed tokens,', ([owner, employee, anyone]) => {

context('when it has not been initialized yet', function () {
it('reverts', async () => {
await assertRevert(payroll.isTokenAllowed(denominationToken.address), 'INIT_NOT_INITIALIZED')
await assertRevert(payroll.isTokenAllowed(DAI.address), 'INIT_NOT_INITIALIZED')
})
})
})
Expand Down
Loading

0 comments on commit 327abd6

Please sign in to comment.