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: Polish payroll app #744

Merged
merged 38 commits into from Apr 5, 2019

Conversation

Projects
None yet
3 participants
@facuspagnuolo
Copy link
Contributor

facuspagnuolo commented Mar 26, 2019

MERGE AFTER #736

Given that I've polishing and tweaking many parts of the payroll app, I tried to work submitting atomic commits to help reviewing my progress. The highlights of this PR are:

  • Fix last payroll timestamp calculation for partial paydays (see 8a86879)
  • Polish contract interface and cosmetic enhancements
  • Refactor and tune up test suite
  • Add missing inline documentation and review wording
  • Clean up unused mock contracts
  • Organize test helper functions

Interface changes

  • The previous addEmployee function without a startDate was renamed to addEmployeeNow.
  • Removed string name argument from both addEmployee and addEmployeeNow functions.
  • Function determineAllocation now receives an uint256 array instead of an uint8 one.
  • Function getAllocation now returns an uint256 instead of uint8
  • Added new partialPayday(uint256 requestedAmount), reimburse() and partialReimburse(uint256 requestedAmount) functions.
  • Added string reference argument to SendPayment event.
  • Removed string name argument from AddEmployee event.

@facuspagnuolo facuspagnuolo force-pushed the polish_payroll_app branch from 94b5da9 to a1900ef Mar 27, 2019

@aragon aragon deleted a comment from coveralls Mar 27, 2019

@aragon aragon deleted a comment from coveralls Mar 27, 2019

@facuspagnuolo facuspagnuolo force-pushed the polish_payroll_app branch from a1900ef to 2bceb0d Mar 27, 2019

@facuspagnuolo facuspagnuolo force-pushed the polish_payroll_app branch from f0bb35c to 0360745 Mar 27, 2019

@facuspagnuolo

This comment has been minimized.

Copy link
Contributor Author

facuspagnuolo commented Mar 29, 2019

Thanks for reviewing @bingen! Addressed all your comments if you'd like to look at my last commit. Regarding the test files unification, my rational was just to follow the rest of the apps where there is one single file per contract. However, it make sense splitting them into separate files, it actually helps a lot when running tests in parallel. Please let me know guys if you prefer having them split, I can roll it back easily.

@bingen

bingen approved these changes Mar 29, 2019

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

@facuspagnuolo facuspagnuolo force-pushed the polish_payroll_app branch from 231c742 to aafbdcd Mar 29, 2019

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 2, 2019

@facuspagnuolo On the subject of tests, I think it makes sense to split them up. As we get better about testing the other apps, we'll likely want to split their test files too :).

@sohkai
Copy link
Member

sohkai left a comment

@facuspagnuolo There are quite a few comments, but 70-80% should be comment suggestions (I can do them myself and push a PR to this branch to clean up the noise if you'd like 😄).

⚠️ However, there are some important things we should think about and discuss (in this priority):

  1. #744 (comment): We've been able to avoid adding re-entrancy guards up to now, but this is a vulnerability that seems most easily fixed by stopping re-entrancy itself. A generic reentrancy guard is probably something we can add as part of aragonOS's next release.
  2. #744 (comment): I like the "reimbursements" terminology and how we're handling the partial payments, but the current behaviour of changing salary is really misleading.
  3. #744 (comment): This may be moot if 2. is changed, but as it is, there's a double spend vulnerability due to the naive division.

But holy smokes, 😍 at those tests! Really, really like the structure and we should definitely move the other apps to start using this structure.

Show resolved Hide resolved future-apps/payroll/test/helpers/numbers.js Outdated
Show resolved Hide resolved future-apps/payroll/test/helpers/setup.js Outdated
Show resolved Hide resolved future-apps/payroll/test/helpers/setup.js Outdated
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol Outdated
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol Outdated
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol Outdated
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol Outdated
Show resolved Hide resolved future-apps/payroll/contracts/Payroll.sol

@sohkai sohkai referenced this pull request Apr 3, 2019

Merged

feat: add ReentrancyGuard #503

@facuspagnuolo facuspagnuolo force-pushed the polish_payroll_app branch from 02a5d30 to c1b8831 Apr 3, 2019

@facuspagnuolo

This comment has been minimized.

Copy link
Contributor Author

facuspagnuolo commented Apr 4, 2019

@sohkai I addressed all the minor comments in this PR and left the more important for further PRs, I'll merge this one in the meantime, thanks for the thorough review. I also added a summary of the interface changes in the PR description :)

@facuspagnuolo facuspagnuolo force-pushed the polish_payroll_app branch from d123287 to 729f5c7 Apr 4, 2019

@facuspagnuolo facuspagnuolo force-pushed the polish_payroll_app branch from 729f5c7 to 3b7cb30 Apr 4, 2019

@facuspagnuolo facuspagnuolo merged commit c1256f0 into master Apr 5, 2019

5 checks passed

License Compliance Analysis timed out. Check FOSSA for updates.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.734%
Details
license/cla Contributor License Agreement is signed.
Details

@facuspagnuolo facuspagnuolo deleted the polish_payroll_app branch Apr 5, 2019

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 5, 2019

@facuspagnuolo

One note on

Added string reference argument to SendPayment event.

Let's use a different variable name (e.g. paymentReference); I just learned that reference is a reserved keyword in pragma 0.5+.

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.