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: Support employees bonus #775

Merged
merged 2 commits into from Apr 16, 2019

Conversation

Projects
None yet
3 participants
@facuspagnuolo
Copy link
Contributor

facuspagnuolo commented Apr 6, 2019

Fixes #760

Caveat
I needed to refactor the external interface since we were reaching the bytecode max size limit. I removed payday, partialPayday, reimbursement and , partialReimbursement in favor of a single payday method allowing the user to specify which payment type they want to cash out.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 6, 2019

Coverage Status

Coverage remained the same at 96.734% when pulling 6b52556 on support_payroll_bonuses into 53808f3 on implement_accrued_salary.

@facuspagnuolo

This comment has been minimized.

Copy link
Contributor Author

facuspagnuolo commented Apr 10, 2019

I accidentally closed it due to a bad PR link

@facuspagnuolo facuspagnuolo reopened this Apr 10, 2019

@sohkai

sohkai approved these changes Apr 15, 2019

Copy link
Member

sohkai left a comment

😍 we're getting so feature-full with this change.

How close are we getting to the max?

Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol
Show resolved Hide resolved future-apps/payroll/test/contracts/Payroll_bonuses.test.js
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol
*/
function _addBonus(uint256 _employeeId, uint256 _amount) internal {
Employee storage employee = employees[_employeeId];
employee.bonus = employee.bonus.add(_amount);

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 15, 2019

Member

Wondering if we should max the bonus and accrued value at MAX_UINT256 as well, so everything just uses that as a hard stop?

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Apr 15, 2019

Author Contributor

In this case it will be hold by SafeMath, the employer won't be able to overflow the employee's bonus amount basically. The same holds now for reimbursements (accrued value) since we split it from accrued salary. The only thing that could overflow is the owed salary, which is a concern for accrued salary and payroll payments :/

Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol

facuspagnuolo added a commit that referenced this pull request Apr 15, 2019

@facuspagnuolo

This comment has been minimized.

Copy link
Contributor Author

facuspagnuolo commented Apr 15, 2019

Addressed feedback from @sohkai here c3591b7

@facuspagnuolo facuspagnuolo merged commit f46f259 into implement_accrued_salary Apr 16, 2019

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
License Compliance All checks passed.
Details
license/cla Contributor License Agreement is signed.
Details

facuspagnuolo added a commit that referenced this pull request Apr 16, 2019

Payroll: Ceil last payroll date calculation (#776)
* Payroll: Rename payment `reference` to `paymentReference`

* Payroll: Remove return values from `_payday` and `_reimburse` functions

* Payroll: Implement accrued salary

* Payroll: Support employees bonus

* Payroll: Ceil last payroll date calculation

* Payroll: Address feedback from @sohkai in #774

* Payroll: Address feedback from @sohkai in #775

* Payroll: Address feedback from @sohkai in #776

* Payroll: Fix compiler warnings
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.