Skip to content
Permalink
Browse files

Payroll: suggestions from review (#834)

  • Loading branch information...
sohkai committed May 10, 2019
1 parent 4b5094a commit bf29b82ef516e2e82633de181205efc504c5f3f1
@@ -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.


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;

@@ -39,7 +39,7 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon
const address = employee

beforeEach('add employee', async () => {
receipt = await payroll.addEmployee(address, salary, role, startDate, { from })
receipt = await payroll.addEmployee(address, salary, startDate, role, { from })
employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId').toString()
})

@@ -56,35 +56,35 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon

const event = events[0].args
assert.equal(event.employeeId, employeeId, 'employee id does not match')
assert.equal(event.role, role, 'employee role does not match')
assert.equal(event.accountAddress, employee, 'employee address does not match')
assert.equal(event.startDate.toString(), startDate, 'employee start date does not match')
assert.equal(event.initialDenominationSalary.toString(), salary.toString(), 'employee salary does not match')
assert.equal(event.startDate.toString(), startDate, 'employee start date does not match')
assert.equal(event.role, role, 'employee role does not match')
})

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

const receipt = await payroll.addEmployee(anotherEmployee, anotherSalary, anotherRole, startDate)
const receipt = await payroll.addEmployee(anotherEmployee, anotherSalary, startDate, anotherRole)
const anotherEmployeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId')

const events = getEvents(receipt, 'AddEmployee');
assert.equal(events.length, 1, 'number of AddEmployee events does not match')

const event = events[0].args
assert.equal(event.employeeId, anotherEmployeeId, 'employee id does not match')
assert.equal(event.role, anotherRole, 'employee role does not match')
assert.equal(event.accountAddress, anotherEmployee, 'employee address does not match')
assert.equal(event.startDate.toString(), startDate, 'employee start date does not match')
assert.equal(event.initialDenominationSalary.toString(), anotherSalary.toString(), 'employee salary does not match')
assert.equal(event.startDate.toString(), startDate, 'employee start date does not match')
assert.equal(event.role, anotherRole, 'employee role does not match')

const [address, employeeSalary, bonus, reimbursements, accruedSalary, lastPayroll, endDate] = await payroll.getEmployee(anotherEmployeeId)
const [address, employeeSalary, accruedSalary, bonus, reimbursements, lastPayroll, endDate] = await payroll.getEmployee(anotherEmployeeId)
assert.equal(address, anotherEmployee, 'employee address does not match')
assert.equal(bonus.toString(), 0, 'employee bonus does not match')
assert.equal(reimbursements, 0, 'employee reimbursements does not match')
assert.equal(accruedSalary, 0, 'employee accrued salary does not match')
assert.equal(employeeSalary.toString(), anotherSalary.toString(), 'employee salary does not match')
assert.equal(accruedSalary.toString(), 0, 'employee accrued salary does not match')
assert.equal(bonus.toString(), 0, 'employee bonus does not match')
assert.equal(reimbursements.toString(), 0, 'employee reimbursements does not match')
assert.equal(lastPayroll.toString(), startDate.toString(), 'employee last payroll does not match')
assert.equal(endDate.toString(), MAX_UINT64, 'employee end date does not match')
})
@@ -94,7 +94,7 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon
const address = ZERO_ADDRESS

it('reverts', async () => {
await assertRevert(payroll.addEmployee(address, salary, role, startDate, { from }), 'PAYROLL_EMPLOYEE_NULL_ADDRESS')
await assertRevert(payroll.addEmployee(address, salary, startDate, role, { from }), 'PAYROLL_EMPLOYEE_NULL_ADDRESS')
})
})
}
@@ -114,22 +114,22 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon

context('when the employee has already been added', () => {
beforeEach('add employee', async () => {
await payroll.addEmployee(employee, salary, role, NOW, { from })
await payroll.addEmployee(employee, salary, NOW, role, { from })
})

context('when the given end date is in the past ', () => {
const startDate = NOW - TWO_MONTHS

it('reverts', async () => {
await assertRevert(payroll.addEmployee(employee, salary, role, startDate, { from }), 'PAYROLL_EMPLOYEE_ALREADY_EXIST')
await assertRevert(payroll.addEmployee(employee, salary, startDate, role, { from }), 'PAYROLL_EMPLOYEE_ALREADY_EXIST')
})
})

context('when the given end date is in the future', () => {
const startDate = NOW + TWO_MONTHS

it('reverts', async () => {
await assertRevert(payroll.addEmployee(employee, salary, role, startDate, { from }), 'PAYROLL_EMPLOYEE_ALREADY_EXIST')
await assertRevert(payroll.addEmployee(employee, salary, startDate, role, { from }), 'PAYROLL_EMPLOYEE_ALREADY_EXIST')
})
})
})
@@ -139,14 +139,14 @@ contract('Payroll employees addition', ([owner, employee, anotherEmployee, anyon
const from = anyone

it('reverts', async () => {
await assertRevert(payroll.addEmployee(employee, salary, role, NOW, { from }), 'APP_AUTH_FAILED')
await assertRevert(payroll.addEmployee(employee, salary, NOW, role, { from }), 'APP_AUTH_FAILED')
})
})
})

context('when it has not been initialized yet', function () {
it('reverts', async () => {
await assertRevert(payroll.addEmployee(employee, salary, role, NOW, { from: owner }), 'APP_AUTH_FAILED')
await assertRevert(payroll.addEmployee(employee, salary, NOW, role, { from: owner }), 'APP_AUTH_FAILED')
})
})
})
@@ -84,7 +84,7 @@ contract('Payroll allowed tokens,', ([owner, employee, anyone]) => {
const rates = tokenAddresses.map(() => formatRate(5))
await setTokenRates(priceFeed, USD, tokenAddresses, rates)

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

it('can not add one more token', async () => {
@@ -37,18 +37,18 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
let employeeId

beforeEach('add employee', async () => {
const receipt = await payroll.addEmployee(employee, annualSalaryPerSecond(100000), 'Boss', await payroll.getTimestampPublic())
const receipt = await payroll.addEmployee(employee, annualSalaryPerSecond(100000), await payroll.getTimestampPublic(), 'Boss')
employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId')
})

context('when the given employee is active', () => {

const itAddsBonusesSuccessfully = amount => {
it('adds given bonus amount', async () => {
const previousBonus = (await payroll.getEmployee(employeeId))[2]
const previousBonus = (await payroll.getEmployee(employeeId))[3]
await payroll.addBonus(employeeId, amount, { from })

const currentBonus = (await payroll.getEmployee(employeeId))[2]
const currentBonus = (await payroll.getEmployee(employeeId))[3]
assert.equal(previousBonus.plus(amount).toString(), currentBonus.toString(), 'bonus amount does not match')
})

@@ -152,7 +152,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
let employeeId, salary = annualSalaryPerSecond(100000)

beforeEach('add employee and accumulate some salary', async () => {
const receipt = await payroll.addEmployee(employee, salary, 'Boss', await payroll.getTimestampPublic())
const receipt = await payroll.addEmployee(employee, salary, await payroll.getTimestampPublic(), 'Boss')
employeeId = getEventArgument(receipt, 'AddEmployee', 'employeeId')

await increaseTime(ONE_MONTH)
@@ -202,27 +202,29 @@ 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.accountAddress, 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')
assert.equal(eventDAI.paymentReference, 'Employee 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.accountAddress, 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')
assert.equal(eventANT.paymentReference, 'Bonus', 'payment reference does not match')
assert.equal(eventANT.paymentReference, 'Employee bonus', 'payment reference does not match')
})
}

const assertEmployeeIsNotRemoved = (requestedAmount, expectedRequestedAmount = requestedAmount) => {
it('does not remove the employee and resets the bonus amount', async () => {
const previousBonus = (await payroll.getEmployee(employeeId))[2]
const previousBonus = (await payroll.getEmployee(employeeId))[3]
await payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from })

const [address, employeeSalary, bonus] = await payroll.getEmployee(employeeId)
const [address, employeeSalary, , bonus] = await payroll.getEmployee(employeeId)

assert.equal(address, employee, 'employee address does not match')
assert.equal(employeeSalary.toString(), salary.toString(), 'employee salary does not match')
@@ -491,15 +493,15 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
const requestedAmount = bigExp(100, 18)

it('reverts', async () => {
await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from }), 'PAYROLL_EMPLOYEE_DOES_NOT_MATCH')
await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from }), 'PAYROLL_SENDER_DOES_NOT_MATCH')
})
})

context('when the requested amount is zero', () => {
const requestedAmount = bn(0)

it('reverts', async () => {
await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from }), 'PAYROLL_EMPLOYEE_DOES_NOT_MATCH')
await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from }), 'PAYROLL_SENDER_DOES_NOT_MATCH')
})
})
})
@@ -509,7 +511,7 @@ contract('Payroll bonuses', ([owner, employee, anyone]) => {
const requestedAmount = bn(0)

it('reverts', async () => {
await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from: employee }), 'PAYROLL_EMPLOYEE_DOES_NOT_MATCH')
await assertRevert(payroll.payday(PAYMENT_TYPES.BONUS, requestedAmount, { from: employee }), 'PAYROLL_SENDER_DOES_NOT_MATCH')
})
})
})

0 comments on commit bf29b82

Please sign in to comment.
You can’t perform that action at this time.