Payroll: Fix accrued salary maths (aragon#926)

* payroll: fix accrued salary maths * Payroll: add explicit tests for remaining accrued salary
ConsiderItDone · Jul 17, 2019 · 9244544 · 9244544
1 parent a4ae7fb
commit 9244544
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 23 deletions.
diff --git a/future-apps/payroll/contracts/Payroll.sol b/future-apps/payroll/contracts/Payroll.sol
@@ -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);

diff --git a/future-apps/payroll/test/contracts/Payroll_payday.test.js b/future-apps/payroll/test/contracts/Payroll_payday.test.js
@@ -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
@@ -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)
@@ -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', () => {