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.

Sign up for GitHub

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: Fix accrued salary maths #926

Merged
merged 2 commits into from
Jul 17, 2019
Merged

Payroll: Fix accrued salary maths #926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24c4e0f
payroll: fix accrued salary maths
facuspagnuolo Jul 17, 2019
5c52a69
Payroll: add explicit tests for remaining accrued salary
sohkai Jul 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions future-apps/payroll/contracts/Payroll.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,22 +664,20 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
// first use up their accrued salary
// No need to use SafeMath here as we already know _paymentAmount > accruedSalary
currentSalaryPaid = _paymentAmount - accruedSalary;
// We finally need to clear their accrued salary
_employee.accruedSalary = 0;
}

uint256 salary = _employee.denominationTokenSalary;
uint256 timeDiff = currentSalaryPaid.div(salary);

// If they're being paid an amount that doesn't match perfectly with the adjusted time
// (up to a seconds' worth of salary), add the second and put the extra remaining salary
// into their accrued salary
// The extra check is to handle the case where someone requested less than one second of their salary
uint256 extraSalary = currentSalaryPaid < salary ? salary - currentSalaryPaid : currentSalaryPaid % salary;
uint256 extraSalary = currentSalaryPaid % salary;
if (extraSalary > 0) {
timeDiff = timeDiff.add(1);
_employee.accruedSalary = extraSalary;
} else if (accruedSalary > 0) {
// We finally need to clear their accrued salary, but as an optimization, we only do
// this if they had a non-zero value before
_employee.accruedSalary = 0;
_employee.accruedSalary = salary - extraSalary;
}

uint256 lastPayrollDate = uint256(_employee.lastPayroll).add(timeDiff);
Expand Down
48 changes: 32 additions & 16 deletions future-apps/payroll/test/contracts/Payroll_payday.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,22 +127,15 @@ contract('Payroll payday', ([owner, employee, anyone]) => {
}

const assertEmployeeIsUpdatedCorrectly = (requestedAmount, expectedRequestedAmount, minRates = []) => {
it('updates the accrued salary and the last payroll date', async () => {
it('updates the employee accounting', async () => {
let expectedLastPayrollDate, expectedAccruedSalary
const [employeeSalary, previousAccruedSalary, , , previousPayrollDate] = (await payroll.getEmployee(employeeId)).slice(1, 6)
const [, previousAccruedSalary, , , previousPayrollDate] = (await payroll.getEmployee(employeeId)).slice(1, 6)

if (expectedRequestedAmount.gte(previousAccruedSalary)) {
const remainder = expectedRequestedAmount.minus(previousAccruedSalary)
if (remainder.eq(0)) {
expectedAccruedSalary = bn(0)
} else {
// Have remaining salary that needs to be put back into the accrued salary
expectedAccruedSalary =
remainder.lt(employeeSalary)
? salary.minus(remainder)
: expectedAccruedSalary = remainder.mod(employeeSalary)
}
expectedLastPayrollDate = previousPayrollDate.plus(remainder.div(employeeSalary).ceil())
const currentSalaryPaid = expectedRequestedAmount.minus(previousAccruedSalary)
const extraSalary = currentSalaryPaid.mod(salary)
expectedAccruedSalary = extraSalary.gt(0) ? salary.minus(extraSalary) : 0
expectedLastPayrollDate = previousPayrollDate.plus(currentSalaryPaid.div(salary).ceil())
} else {
expectedAccruedSalary = previousAccruedSalary.minus(expectedRequestedAmount).toString()
expectedLastPayrollDate = previousPayrollDate
Expand Down Expand Up @@ -386,6 +379,15 @@ contract('Payroll payday', ([owner, employee, anyone]) => {
await payroll.setEmployeeSalary(employeeId, salary, { from: owner })
})

const itHandlesRemainingAccruedSalaryCorrectly = (requestedAmount, expectedAccruedSalary) => {
it('handles remaining accrued salary correctly', async () => {
await payroll.payday(PAYMENT_TYPES.PAYROLL, requestedAmount, [], { from })

const [accruedSalary] = (await payroll.getEmployee(employeeId)).slice(2)
assert.equal(accruedSalary.toString(), expectedAccruedSalary.toString(), 'accrued salary does not match')
})
}

context('when the employee has some pending salary', () => {
const currentOwedSalary = salary.mul(ONE_MONTH)
const totalOwedSalary = previousOwedSalary.plus(currentOwedSalary)
Expand All @@ -399,24 +401,38 @@ contract('Payroll payday', ([owner, employee, anyone]) => {
const requestedAmount = bn(0)

itHandlesPayrollProperlyNeverthelessExtrasOwedAmounts(requestedAmount, totalOwedSalary)
itHandlesRemainingAccruedSalaryCorrectly(requestedAmount, bn(0))
})

context('when the requested amount is lower than the previous owed salary', () => {
const requestedAmount = previousOwedSalary.minus(10)
const remainingAmount = bn(10)
const requestedAmount = previousOwedSalary.minus(remainingAmount)

itHandlesPayrollProperlyNeverthelessExtrasOwedAmounts(requestedAmount, totalOwedSalary)
itHandlesRemainingAccruedSalaryCorrectly(requestedAmount, remainingAmount)
})

context('when the requested amount is greater than the previous owed salary but less than one second of additional salary', () => {
const requestedAmount = previousOwedSalary.plus(salary).minus(1)
const remainingAmount = bn(1)
const requestedAmount = previousOwedSalary.plus(salary).minus(remainingAmount)

itHandlesPayrollProperlyNeverthelessExtrasOwedAmounts(requestedAmount, totalOwedSalary)

// We're requesting all of the accrued salary and just under one second of salary,
// so we should get the remaining bit left over
itHandlesRemainingAccruedSalaryCorrectly(requestedAmount, remainingAmount)
})

context('when the requested amount is greater than the previous owed salary but greater than one second of additional salary', () => {
const requestedAmount = previousOwedSalary.plus(salary).plus(1)
const extraAmount = bn(1)
const requestedAmount = previousOwedSalary.plus(salary).plus(extraAmount)

itHandlesPayrollProperlyNeverthelessExtrasOwedAmounts(requestedAmount, totalOwedSalary)

// We move the salary up a second, so we should get one full second of salary
// minus the extra amount left over
const remainingAmount = salary.minus(extraAmount)
itHandlesRemainingAccruedSalaryCorrectly(requestedAmount, remainingAmount)
})

context('when the requested amount is equal to the total owed salary', () => {
Expand Down