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: Add payroll remainders as accrued salary #907

Merged
merged 5 commits into from Jul 7, 2019

Conversation

Projects
None yet
2 participants
@facuspagnuolo
Copy link
Contributor

commented Jul 7, 2019

Merging #903 against master

@facuspagnuolo facuspagnuolo changed the title Payroll/floor payday paid amount Payroll: Add payroll remainders as accrued salary Jul 7, 2019

@facuspagnuolo facuspagnuolo merged commit e52aae4 into master Jul 7, 2019

3 of 6 checks passed

Travis CI - Pull Request Build Errored
Details
coverage/coveralls Coverage decreased (-0.7%) to 97.015%
Details
License Compliance Analysis done, scanning for issues...
Details
Travis CI - Branch Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@facuspagnuolo facuspagnuolo deleted the payroll/floor_payday_paid_amount branch Jul 7, 2019

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

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 8, 2019

Member

@facuspagnuolo Not sure I understand this change in ae3ee3d#diff-9d7adaffbcce19867f027a983a30bcc7R666 (Good catch on the timeDiff though!).

We've already calculated the remaining salary with extraSalary, and salary - currentSalaryPaid looks like it would underflow (salary is salary per second, currentSalaryPaid is some multiple of that).

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Jul 8, 2019

Author Contributor

True.. I was thinking the case where you request less than a second of your salary, but ofc the scenario you're pointing out is also true. I think the tests are passing because we're using 1 USD per second which will always fit perfectly

@@ -597,7 +597,7 @@ contract('Payroll payday', ([owner, employee, anyone]) => {
})
})

context('when the employee has a huge salary', () => {
context.only('when the employee has a huge salary', () => {

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 8, 2019

Member

Oops, let's also revert the .only 😄 (#908)

@@ -39,7 +39,6 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
bytes32 constant public MODIFY_PRICE_FEED_ROLE = 0x74350efbcba8b85341c5bbf70cc34e2a585fc1463524773a12fa0a71d4eb9302;
bytes32 constant public MODIFY_RATE_EXPIRY_ROLE = 0x79fe989a8899060dfbdabb174ebb96616fa9f1d9dadd739f8d814cbab452404e;

uint128 internal constant ONE = 10 ** 18; // 10^18 is considered 1 in the price feed to allow for decimal calculations

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 8, 2019

Member

It's quite weird the changes in #902 are showing up in this diff :/

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 8, 2019

Member

It doesn't show in the commit history though :shrug:

e52aae4

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Jul 8, 2019

Author Contributor

weird :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.