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: handle both cases of extra salary being sent to accrued balance #912

Merged
merged 1 commit into from
Jul 8, 2019
Merged

Payroll: handle both cases of extra salary being sent to accrued balance #912

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions future-apps/payroll/contracts/Payroll.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,11 +660,14 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
// 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
uint256 extraSalary = currentSalaryPaid % 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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

salary - extraSalary should go to acrued salary in case when currentSalaryPaid > salary

I think the following code should work fine, or am I missing something?

uint256 extraSalary = currentSalaryPaid % salary;
if (extraSalary > 0) {
            timeDiff = timeDiff.add(1);
            employee.accruedSalary = salary - extraSalary;
        }

Examples:

  1. Salary == 10 and you want to get currentSalaryPaid == 21.
    Time difference would be 3. Contract will transfer currentSalaryPaid == 21 and 10 - 21%10 = 9 will go to the accrued salary.
  2. Salary == 10 and you want to get currentSalaryPaid == 1.
    Time difference would be 1. Contract will transfer currentSalaryPaid == 1 and 10 - 1%10 = 9 will go to the accrued salary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... this is a good catch. I think the current state doesn't represent the

currentSalaryPaid == 21 and 10 - 21%10 = 9

case correctly; it'd put 1 into the accrued salary instead of 9.

cc @facuspagnuolo may also want to double check this, but your judgement on this simplification seems correct @Corsaire :).

Copy link
Contributor

@facuspagnuolo facuspagnuolo Jul 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, we were close in our first attempt here haha 😅

if (extraSalary > 0) {
timeDiff = timeDiff.add(1);
employee.accruedSalary = salary - currentSalaryPaid;
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;
}

Expand Down