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: suggestions from review #834

Merged
merged 30 commits into from May 10, 2019

Conversation

Projects
None yet
3 participants
@sohkai
Copy link
Member

commented May 4, 2019

Easiest to review commit by commit, mostly starting from less controversial to more controversial changes.

Also see inline annotations.

sohkai added some commits May 4, 2019

@sohkai sohkai requested a review from facuspagnuolo May 4, 2019

@sohkai sohkai force-pushed the payroll-review branch from 1528396 to a318ab1 May 4, 2019

sohkai added some commits May 4, 2019

feat: modify event parameters for consistency
Prefer employeeIds over account addresses, and move the new variable
to be before the old variable (similar to roles)
feat: refactor _removeEmployeeIfTerminatedAndPaidOut()
Uses the employee's lastPayroll value instead of current owed salary to
check if the employee's has no more accrued salary left.

Their lastPayroll value should never be greater than the current time,
so it should only ever equal their endDate if their endDate was
reached.
feat: refactor calculation of lastPayroll date
- Ensure an employee's lastPayroll date cannot be greater than the current time or their endDate
- Simplify when their lastPayroll value gets updated

@sohkai sohkai force-pushed the payroll-review branch from a318ab1 to 543f505 May 4, 2019

// Check employee exists (and matches)
return (employees[employeeIds[_sender]].accountAddress == _sender);
// Check sender is active employee
return _isEmployeeActive(employeeIds[_sender]);

This comment has been minimized.

Copy link
@sohkai

sohkai May 4, 2019

Author Member

Even though terminated employees would eventually be removed, and therefore fail the previous check, we should also disallow employees who have been terminated but may be lingering as they are technically not part of payroll anymore.

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo May 7, 2019

Contributor

I'm thinking of the other actions employees are allowed to perform even when they are terminated, like claiming their salaries. Could this still be useful in those cases when the calling employee is terminated?

This comment has been minimized.

Copy link
@sohkai

sohkai May 8, 2019

Author Member

These actions shouldn't be forwarded though.

This canForward() should only apply to cases where the Payroll app is used as a forwarder (e.g. setting up an organization so that only employees in Payroll can create votes in Voting).

}
employee.lastPayroll = _getLastPayrollDate(_employeeId, currentSalaryPaid);

This comment has been minimized.

Copy link
@sohkai

sohkai May 4, 2019

Author Member

⚠️ I've opted to remove the _paymentAmount == _currentOwedSalary check and just use _getLastPayrollDate() directly. It's a little bit more expensive in the case where you're drawing out all of your currently owed salary, but reduces the complexity.

In particular, I think it's much easier to reason about how an employee's lastPayroll value changes if it's only affected by how much currently owed salary is paid (through _getLastPayrollDate()).


Note that this check was also previously setting lastPayroll to be now rather than an employee's endDate if we were drawing out all of their accrued salary.

@@ -600,11 +600,13 @@ contract Payroll is EtherTokenConstant, IForwarder, IsContract, AragonApp {
timeDiff = timeDiff.add(1);
}

uint64 lastPayrollDate = employee.lastPayroll.add(uint64(timeDiff));

This comment has been minimized.

Copy link
@sohkai

sohkai May 4, 2019

Author Member

I was a little bit worried about this uint64(timeDiff) cast; although it's unlikely, at some point we may accidentally provide a _paidAmount that would cause timeDiff to be larger than MAX_UINT64.

@coveralls

This comment has been minimized.

Copy link

commented May 5, 2019

Coverage Status

Coverage remained the same at 97.015% when pulling d147441 on payroll-review into 4b5094a on master.

@facuspagnuolo
Copy link
Contributor

left a comment

LGTM, nice job!

Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol
// - Run the tests with `truffle compile --all` on
// - Or trick Truffle by claiming we use it in a Solidity test
//
// You know which one I went for.

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo May 7, 2019

Contributor

😂

// Check employee exists (and matches)
return (employees[employeeIds[_sender]].accountAddress == _sender);
// Check sender is active employee
return _isEmployeeActive(employeeIds[_sender]);

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo May 7, 2019

Contributor

I'm thinking of the other actions employees are allowed to perform even when they are terminated, like claiming their salaries. Could this still be useful in those cases when the calling employee is terminated?

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

facuspagnuolo and others added some commits May 8, 2019

Update future-apps/payroll/contracts/Payroll.sol
Co-Authored-By: sohkai <qisheng.brett.sun@gmail.com>

@sohkai sohkai force-pushed the payroll-review branch from 769bd8a to d147441 May 9, 2019

@sohkai

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@facuspagnuolo I ended up reverting 7b43299 in ce63597 because we were using the term "price feed" elsewhere, and PriceFeed is not the name of an actual contract (in Payroll's eyes).

Added a last few refactors / changes:

  • 1b0e5f4: I think inlining this bit is more clear as to how we use the capped value safely
  • 7bc705f: Pulls out some duplicated logic for setting the employee's address into a consolidated internal function

And added a few last touches for clarity / consistency:

  • bf5ce8b: re-orders role to be the final parameter, as we generally leave the string metadata argument last in the other apps
  • 094116b: re-order accruedSalary to be closer to denominationSalary to group them together
  • d8dd5b1: A little more explicit string for the Finance payment reference (note that these lengths are all still below 32 chars so there's no gas change 😂)
  • Rest: shuffling around some of the functions to logically group them by function (no hard rule, but generally: external -> public -> internal -> private, then state-changing -> view -> pure)

@sohkai sohkai merged commit bf29b82 into master May 10, 2019

5 checks passed

License Compliance All checks passed.
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
coverage/coveralls Coverage remained the same at 97.015%
Details
license/cla Contributor License Agreement is signed.
Details

@sohkai sohkai deleted the payroll-review branch May 10, 2019

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.