Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Payroll: suggestions from review #834

Merged
merged 30 commits into from May 10, 2019
Merged
Changes from 17 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
6f119b3
cosmetic: comments and radspec improvements
sohkai May 4, 2019
60e1217
cosmetic: re-order and rename some revert errors
sohkai May 4, 2019
9b38c54
cosmetic: rename some internal functions for clarity
sohkai May 4, 2019
f91912d
feat: allow 1min as the price feed rate expiry time, to clarify minimum
sohkai May 4, 2019
057ca34
feat: reorganize state variables, small clarity and code optimizations
sohkai May 4, 2019
1c3cc30
feat: optimize employeeActive check
sohkai May 4, 2019
7838a79
feat: only allow forwarding for active employees
sohkai May 4, 2019
4f1fee9
feat: add more parameters to some roles and sync arapp.json's roles
sohkai May 4, 2019
75778ba
feat: modify event parameters for consistency
sohkai May 4, 2019
3b2be77
cosmetic: reorder events to group them by employee-related events the…
sohkai May 4, 2019
7f95f55
feat: cast exchange rate to uint256 when used internally
sohkai May 4, 2019
5a1da92
feat: refactor _removeEmployeeIfTerminatedAndPaidOut()
sohkai May 4, 2019
3eab68a
feat: refactor calculation of lastPayroll date
sohkai May 4, 2019
2cd56f9
cosmetic: fix linter errors
sohkai May 4, 2019
9a2e30d
cosmetic: reorder internal functions so non-view are first
sohkai May 4, 2019
5ffa25f
feat: calculate lastPayrollDate in uint256 instead of uint64 for safety
sohkai May 4, 2019
543f505
feat: refactor capped and uncapped calculations of owed salary since …
sohkai May 4, 2019
7b43299
Update future-apps/payroll/contracts/Payroll.sol
facuspagnuolo May 8, 2019
ce63597
cosmetic: group all internal view functions after internal
sohkai May 8, 2019
90dca09
cosmetic: comments and radspec improvements
sohkai May 9, 2019
1e9f8ee
cosmetic: error message renaming for clarity
sohkai May 9, 2019
bf5ce8b
feat: re-order arguments for AddEmployee to be more consistent with o…
sohkai May 9, 2019
094116b
feat: reorder accruedSalary storage to be beside employee's denominat…
sohkai May 9, 2019
418ed9c
feat: rename employeeAccountAddress to accountAddress in SendPayment …
sohkai May 9, 2019
d8dd5b1
feat: update payment reference strings to be more explicit
sohkai May 9, 2019
7bc705f
feat: consolidate logic for setting employee into internal function
sohkai May 9, 2019
1b0e5f4
fix: inline _getOwedSalaries() to consolidate the uint max capping lo…
sohkai May 9, 2019
bdb5bc2
cosmetic: group external functions to management functions then emplo…
sohkai May 9, 2019
3df781e
cosmetic: reorder internal functions to group management functions be…
sohkai May 9, 2019
d147441
cosmetic: move initialized() to be first statement in initialize() fo…
sohkai May 9, 2019
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -39,17 +39,22 @@
{
"name": "Add employees",
"id": "ADD_EMPLOYEE_ROLE",
"params": ["Account address", "Initial yearly salary", "Start date"]
"params": ["Account address", "Initial yearly salary per second", "Start date"]
},
{
"name": "Terminate employees",
"id": "TERMINATE_EMPLOYEE_ROLE",
"params": ["Employee Id"]
"params": ["Employee Id", "End date"]
},
{
"name": "Set employee salary",
"id": "SET_EMPLOYEE_SALARY_ROLE",
"params": ["Employee Id", "Yearly salary"]
"params": ["Employee Id", "New yearly salary per second", "Old yearly salary per second"]
},
{
"name": "Add bonuses for employee",
"id": "ADD_BONUS_ROLE",
"params": ["Employee Id", "Amount"]
},
{
"name": "Add reimbursements for employee",

Large diffs are not rendered by default.

@@ -8,6 +8,22 @@ import "@aragon/apps-vault/contracts/Vault.sol";
import "@aragon/apps-shared-minime/contracts/MiniMeToken.sol";
import "@aragon/apps-shared-migrations/contracts/Migrations.sol";

contract TestImports {

// You might think this file is a bit odd, but let me explain.
// We only use some contracts in our tests, which means Truffle
// will not compile it for us, because it is from an external
// dependency.
//
// We are now left with three options:
// - Copy/paste these contracts
// - Run the tests with `truffle compile --all` on
// - Or trick Truffle by claiming we use it in a Solidity test
//
// You know which one I went for.

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo May 7, 2019

Contributor

😂



contract TestImports {
constructor() public {
// solium-disable-previous-line no-empty-blocks
}
}
@@ -1,5 +1,6 @@
pragma solidity 0.4.24;


contract ExecutionTarget {
uint public counter;

@@ -29,16 +29,17 @@ contract MaliciousEmployee {
}

function reenter() public {
if (counter > 0) return;
if (counter > 0) {
return;
}

counter++;

if (action == Action.Payday) {
payroll.payday(Payroll.PaymentType.Payroll, 0);
}
else if (action == Action.ChangeAddress) {
} else if (action == Action.ChangeAddress) {
payroll.changeAddressByEmployee(msg.sender);
}
else if (action == Action.SetAllocation) {
} else if (action == Action.SetAllocation) {
address[] memory tokens = new address[](1);
tokens[0] = address(0);
uint256[] memory allocations = new uint256[](1);
@@ -48,6 +49,7 @@ contract MaliciousEmployee {
}
}


contract MaliciousERC20 is ERC20 {
using SafeMath for uint256;

@@ -202,14 +202,16 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
assert.equal(events.length, 2, 'should have emitted two events')

const eventDAI = events.find(e => e.args.token === DAI.address).args
assert.equal(eventDAI.employee, employee, 'employee address does not match')
assert.equal(eventDAI.employeeId.toString(), employeeId.toString(), 'employee id does not match')
assert.equal(eventDAI.employeeAccountAddress, employee, 'employee address does not match')
assert.equal(eventDAI.token, DAI.address, 'DAI address does not match')
assert.equal(eventDAI.amount.toString(), requestedDAI, 'payment amount does not match')
assert.equal(eventDAI.exchangeRate.toString(), inverseRate(DAI_RATE).toString(), 'payment exchange rate does not match')
assert.equal(eventDAI.paymentReference, 'Bonus', 'payment reference does not match')

const eventANT = events.find(e => e.args.token === ANT.address).args
assert.equal(eventANT.employee, employee, 'employee address does not match')
assert.equal(eventANT.employeeId.toString(), employeeId.toString(), 'employee id does not match')
assert.equal(eventANT.employeeAccountAddress, employee, 'employee address does not match')
assert.equal(eventANT.token, ANT.address, 'token address does not match')
assert.equal(eventANT.amount.toString(), requestedANT, 'payment amount does not match')
assert.equal(eventANT.exchangeRate.toString(), inverseRate(ANT_RATE).toString(), 'payment exchange rate does not match')
@@ -1,14 +1,14 @@
const { assertRevert } = require('@aragon/test-helpers/assertThrow')
const { getEventArgument } = require('../helpers/events')
const { encodeCallScript } = require('@aragon/test-helpers/evmScript')
const { annualSalaryPerSecond } = require('../helpers/numbers')(web3)
const { annualSalaryPerSecond, bn } = require('../helpers/numbers')(web3)
const { USD, deployDAI } = require('../helpers/tokens')(artifacts, web3)
const { NOW, ONE_MONTH, RATE_EXPIRATION_TIME } = require('../helpers/time')
const { deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy')(artifacts, web3)

const ExecutionTarget = artifacts.require('ExecutionTarget')

contract('Payroll forwarding,', ([owner, employee, anyone]) => {
contract('Payroll forwarding', ([owner, employee, anyone]) => {
let dao, payroll, payrollBase, finance, vault, priceFeed, DAI

before('deploy base apps and tokens', async () => {
@@ -59,14 +59,32 @@ contract('Payroll forwarding,', ([owner, employee, anyone]) => {
})
})

context('when the employee was already terminated', () => {
context('when the employee has termination date', () => {
const timeUntilTermination = ONE_MONTH + 1

beforeEach('terminate employee', async () => {
await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner })
await payroll.mockIncreaseTime(ONE_MONTH + 1)
const terminationDate = (await payroll.getTimestampPublic()).plus(bn(timeUntilTermination))
await payroll.terminateEmployee(employeeId, terminationDate, { from: owner })
})

it('returns true', async () => {
assert(await payroll.canForward(sender, '0x'), 'sender should be able to forward')
context('when the termination date has not been reached', () => {
beforeEach('increase time to before termination date', async () => {
await payroll.mockIncreaseTime(timeUntilTermination)
})

it('returns true', async () => {
assert(await payroll.canForward(sender, '0x'), 'sender should be able to forward')
})
})

context('when the termination date has been reached', () => {
beforeEach('increase time to after termination date', async () => {
await payroll.mockIncreaseTime(timeUntilTermination + 1)
})

it('returns false', async () => {
assert.isFalse(await payroll.canForward(sender, '0x'), 'sender should not be able to forward')
})
})
})
})
@@ -118,16 +136,36 @@ contract('Payroll forwarding,', ([owner, employee, anyone]) => {
})
})

context('when the employee was already terminated', () => {
context('when the employee has termination date', () => {
const timeUntilTermination = ONE_MONTH + 1

beforeEach('terminate employee', async () => {
await payroll.terminateEmployee(employeeId, await payroll.getTimestampPublic(), { from: owner })
await payroll.mockIncreaseTime(ONE_MONTH + 1)
const terminationDate = (await payroll.getTimestampPublic()).plus(bn(timeUntilTermination))
await payroll.terminateEmployee(employeeId, terminationDate, { from: owner })
})

it('executes the given script', async () => {
await payroll.forward(script, { from })
context('when the termination date has not been reached', () => {
beforeEach('increase time to before termination date', async () => {
await payroll.mockIncreaseTime(timeUntilTermination)
})

assert.equal(await executionTarget.counter(), 1, 'should have received execution calls')
it('executes the given script', async () => {
await payroll.forward(script, { from })

assert.equal(await executionTarget.counter(), 1, 'should have received execution calls')
})
})

context('when the termination date has been reached', () => {
beforeEach('increase time to after termination date', async () => {
await payroll.mockIncreaseTime(timeUntilTermination + 1)
})

it('reverts', async () => {
await assertRevert(payroll.forward(script, { from }), 'PAYROLL_CAN_NOT_FORWARD')

assert.equal(await executionTarget.counter(), 0, 'should not have received execution calls')
})
})
})
})
@@ -136,7 +174,7 @@ contract('Payroll forwarding,', ([owner, employee, anyone]) => {
const from = anyone

it('reverts', async () => {
await assertRevert(payroll.forward(script, { from }), 'PAYROLL_NO_FORWARD')
await assertRevert(payroll.forward(script, { from }), 'PAYROLL_CAN_NOT_FORWARD')

assert.equal(await executionTarget.counter(), 0, 'should not have received execution calls')
})
@@ -145,7 +183,7 @@ contract('Payroll forwarding,', ([owner, employee, anyone]) => {

context('when it has not been initialized yet', function () {
it('reverts', async () => {
await assertRevert(payroll.forward(script, { from: employee }), 'PAYROLL_NO_FORWARD')
await assertRevert(payroll.forward(script, { from: employee }), 'PAYROLL_CAN_NOT_FORWARD')
})
})
})
@@ -1,6 +1,6 @@
const { USD } = require('../helpers/tokens')(artifacts, web3)
const { assertRevert } = require('@aragon/test-helpers/assertThrow')
const { NOW, RATE_EXPIRATION_TIME } = require('../helpers/time')
const { NOW, ONE_MINUTE, RATE_EXPIRATION_TIME } = require('../helpers/time')
const { deployContracts, createPayrollAndPriceFeed } = require('../helpers/deploy')(artifacts, web3)

const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'
@@ -30,9 +30,9 @@ contract('Payroll initialization', ([owner]) => {
assert.equal(await payroll.denominationToken(), ZERO_ADDRESS, 'denomination token does not match')
})

it('reverts when passing an expiration time lower than or equal to a minute', async () => {
const ONE_MINUTE = 60
await assertRevert(payroll.initialize(finance.address, USD, priceFeed.address, ONE_MINUTE, { from }), 'PAYROLL_EXPIRY_TIME_TOO_SHORT')
it('reverts when passing an expiration time lower than one minute', async () => {
const badExpirationTime = ONE_MINUTE - 1
await assertRevert(payroll.initialize(finance.address, USD, priceFeed.address, badExpirationTime, { from }), 'PAYROLL_EXPIRY_TIME_TOO_SHORT')
})

it('reverts when passing an invalid finance instance', async () => {
@@ -186,8 +186,8 @@ contract('Payroll employees modification', ([owner, employee, anotherEmployee, a
const events = getEvents(receipt, 'ChangeAddressByEmployee')
assert.equal(events.length, 1, 'number of ChangeAddressByEmployee emitted events does not match')
assert.equal(events[0].args.employeeId.toString(), employeeId, 'employee id does not match')
assert.equal(events[0].args.oldAddress, employee, 'previous address does not match')
assert.equal(events[0].args.newAddress, newAddress, 'new address does not match')
assert.equal(events[0].args.newAccountAddress, newAddress, 'new address does not match')
assert.equal(events[0].args.oldAccountAddress, employee, 'previous address does not match')
})
})

@@ -82,14 +82,16 @@ contract('Payroll payday', ([owner, employee, anyone]) => {
assert.equal(events.length, 2, 'should have emitted two events')

const eventDAI = events.find(e => e.args.token === DAI.address).args
assert.equal(eventDAI.employee, employee, 'employee address does not match')
assert.equal(eventDAI.employeeId.toString(), employeeId.toString(), 'employee id does not match')
assert.equal(eventDAI.employeeAccountAddress, employee, 'employee address does not match')
assert.equal(eventDAI.token, DAI.address, 'DAI address does not match')
assert.equal(eventDAI.amount.toString(), requestedDAI.toString(), 'payment amount does not match')
assert.equal(eventDAI.exchangeRate.toString(), inverseRate(DAI_RATE).toString(), 'payment exchange rate does not match')
assert.equal(eventDAI.paymentReference, 'Payroll', 'payment reference does not match')

const eventANT = events.find(e => e.args.token === ANT.address).args
assert.equal(eventANT.employee, employee, 'employee address does not match')
assert.equal(eventANT.employeeId.toString(), employeeId.toString(), 'employee id does not match')
assert.equal(eventANT.employeeAccountAddress, employee, 'employee address does not match')
assert.equal(eventANT.token, ANT.address, 'ANT address does not match')
assert.equal(eventANT.amount.toString(), requestedANT.toString(), 'payment amount does not match')
assert.equal(eventANT.exchangeRate.toString(), inverseRate(ANT_RATE).toString(), 'payment exchange rate does not match')
@@ -642,15 +644,19 @@ contract('Payroll payday', ([owner, employee, anyone]) => {
assert.equal(events.length, 2, 'should have emitted two events')

const eventDAI = events.find(e => e.args.token === DAI.address).args
assert.equal(eventDAI.employee, employee, 'employee address does not match')
assert.equal(eventDAI.employeeId.toString(), employeeId.toString(), 'employee id does not match')
assert.equal(eventDAI.employeeAccountAddress, employee, 'employee address does not match')
assert.equal(eventDAI.token, DAI.address, 'DAI address does not match')
assert.equal(eventDAI.amount.toString(), requestedDAI, 'payment amount does not match')
assert.equal(eventDAI.exchangeRate.toString(), inverseRate(DAI_RATE).toString(), 'payment exchange rate does not match')
assert.equal(eventDAI.paymentReference, 'Payroll', 'payment reference does not match')

const eventANT = events.find(e => e.args.token === ANT.address).args
assert.equal(eventANT.employee, employee, 'employee address does not match')
assert.equal(eventANT.employeeId.toString(), employeeId.toString(), 'employee id does not match')
assert.equal(eventANT.employeeAccountAddress, employee, 'employee address does not match')
assert.equal(eventANT.token, ANT.address, 'ANT address does not match')
assert.equal(eventANT.amount.toString(), requestedANT, 'payment amount does not match')
assert.equal(eventANT.exchangeRate.toString(), inverseRate(ANT_RATE).toString(), 'payment exchange rate does not match')
assert.equal(eventANT.paymentReference, 'Payroll', 'payment reference does not match')
})

@@ -191,14 +191,16 @@ contract('Payroll reimbursements', ([owner, employee, anyone]) => {
assert.equal(events.length, 2, 'should have emitted two events')

const eventDAI = events.find(e => e.args.token === DAI.address).args
assert.equal(eventDAI.employee, employee, 'employee address does not match')
assert.equal(eventDAI.employeeId.toString(), employeeId.toString(), 'employee id does not match')
assert.equal(eventDAI.employeeAccountAddress, employee, 'employee address does not match')
assert.equal(eventDAI.token, DAI.address, 'DAI address does not match')
assert.equal(eventDAI.amount.toString(), requestedDAI, 'payment amount does not match')
assert.equal(eventDAI.exchangeRate.toString(), inverseRate(DAI_RATE).toString(), 'payment exchange rate does not match')
assert.equal(eventDAI.paymentReference, 'Reimbursement', 'payment reference does not match')

const eventANT = events.find(e => e.args.token === ANT.address).args
assert.equal(eventANT.employee, employee, 'employee address does not match')
assert.equal(eventANT.employeeId.toString(), employeeId.toString(), 'employee id does not match')
assert.equal(eventANT.employeeAccountAddress, employee, 'employee address does not match')
assert.equal(eventANT.token, ANT.address, 'token address does not match')
assert.equal(eventANT.amount.toString(), requestedANT, 'payment amount does not match')
assert.equal(eventANT.exchangeRate.toString(), inverseRate(ANT_RATE).toString(), 'payment exchange rate does not match')
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.