Payroll Milestone 1 - Addressing review comments #568

lmcorbalan · 2018-11-09T16:51:59Z

Resolves protofire#48

This PR is meant for addressing @sohkai, @bingen and @Quazia comments on PR #479

Following there is the list of comments that we are going to address in this PR:

sohkai - Remove contracts/Migrations.sol
sohkai - Remove migrations/2_deploy_contracts.js
sohkai - payroll/package.json - Pin dependecies version (@aragon/apps-finance, @aragon/os, @aragon/os, @aragon/ppf-contracts)

Payroll.sol

PayrollKit.sol

Quazia/sohkai - initialize vault

Unit Tests

Check and fix unit tests broken by code review changes

…lly a contract

…as other functions

- Make getters as public - Delete Migrations.sol since it's been imported in TestImports.sol - Remove unused migration file - Prune some public constants - Make employeeId uint256 - Fix some role params - Add _ prefix for function arguments - Move MAX_ACCRUED_VALUE check to _addAccruedValue function - Remove escapeHatch function - Some minor changes from code review - Transform getAllocation into a more generic getter - Improve gas efficiency when adding an employee - Increase minimun rate expiry time - Some minor changes from code review - Add dates check in _getOwnedSalary - Ammend _getExchangeRate docs - Add generic reference when calling finance newPayment - Bump some dependencies versions - Fix contract unit test

coveralls · 2018-11-09T18:22:31Z

Coverage remained the same at 96.124% when pulling d2818ac on protofire:payroll-milestone-1-review into e25aa4b on aragon:master.

… arguments

sohkai

@lmcorbalan Thanks for making the changes! It's looking really good!

I added a number of commits, mostly just to smooth some things out that were easier to do myself, like little comment tweaks or rewording :).

I think there's still three changes we could make (cc @bingen @izqui):

Add a recoverToVault() or recoverToFinance() function, since it's nicer to do it this way to generate an event from the Vault rather than using the generic recoverability functionality. Debating if we should just deposit straight into the Vault that the Finance app's attached to (leaning towards it, since it's leaner as a recovery mechanism).
Decide if we should remove the deletion of terminated employees, or remove the terminated storage. Given the latter being leaner, and realistically there being very little chance you'd want to know if an employee was terminated on-chain rather than an employee simply existing, I'm also leaning on the latter.
⚠️ Note that the latter would mean you can do things like change someone's salary, add accrued value, or even change their termination date up to their actual termination date, rather when the call to terminate() happened (as it is now). I find the current behaviour a bug.
Add the ability for an employee to withdraw their accrued value, without factoring in the time from their last pay date. I think this is a much more robust way to solve the denial of service issue that's somewhat mitigated by MAX_ACCRUED_VALUE.

I couldn't get coverage to run in a reasonable amount of time, due to how dependencies completely blow up the instrumentation, so it might be time to give up on it for the moment...

sohkai · 2018-11-15T12:42:31Z

future-apps/payroll/test/payroll_add_remove_employees.js

@@ -57,47 +58,42 @@ contract('Payroll, adding and removing employees,', function(accounts) {
  })

  it("adds employee", async () => {
-    let name = ''
+    let name = 'Kakaroto'


Oh, I had to google it, I didn't know it has this name too, haha.

sohkai · 2018-11-15T17:48:41Z

future-apps/payroll/contracts/Payroll.sol

+    string private constant ERROR_EMPLOYEE_ALREADY_EXIST = "PAYROLL_EMPLOYEE_ALREADY_EXIST";
+    string private constant ERROR_NULL_ADDRESS = "PAYROLL_NULL_ADDRESS";
+    string private constant ERROR_NO_FORWARD = "PAYROLL_NO_FORWARD";
+    string private constant ERROR_RATE_EXPIRY_TIME_TOO_SHORT = "PAYROLL_RATE_EXPIRE_TIME_TOO_SHORT";


@bingen can confirm, but I think we need to limit the error strings' length to be <= 32 chars to avoid eating up another 32 bytes of memory?

Yes, @izqui knows better, but that was the assumption when we added them to aragon OS.

sohkai · 2018-11-15T18:04:11Z

future-apps/payroll/contracts/Payroll.sol

-        uint256 initialDenominationSalary,
-        string name,
-        uint64 startDate
+    function addEmployeeWithStartDate(


Should we just make this an overload of addEmployee()?

Function overload still not working with Truffle... 😞
trufflesuite/truffle#737
trufflesuite/truffle#569

Ahh yeah, we generally fix that with a mock, e.g. VotingMock

sohkai · 2018-11-16T09:54:44Z

future-apps/payroll/contracts/Payroll.sol

+    string private constant ERROR_RATE_EXPIRY_TIME_TOO_SHORT = "PAYROLL_RATE_EXPIRE_TIME_TOO_SHORT";
+    string private constant ERROR_EXCHANGE_RATE_ZERO = "PAYROLL_EXCHANGE_RATE_ZERO";
+    string private constant ERROR_PAST_TERMINATION_DATE = "PAYROLL_PAST_TERMINATION_DATE";
+

    struct Employee {


I noticed in #576 and in our designs we have a "role" field...

If this is going to work we'll need to add another param to addEmployee() and capture the role in an event.

I've been thinking about this, and I think we should either remove the "role" field entirely from the mock ups, or include a way to change an employee's title. Leaning towards the latter, as a function protected behind a MANAGE_EMPLOYEE_TITLE_ROLE role, that could just emit an event with the employee's id and their new title.

We'd rather to do this change in #576, it will affect the app state management and we made a big change on it there

We finally made this change here, this is the most "updated" contract version, once it gets merged we can make the changes in the front end.

bingen

Really good job indeed, @lmcorbalan !!
Replying to @sohkai:

But this is what the old functions escapeHatch and depositToFinance were doing, isn't it? I thought you wanted to remove them. If we do a custom recovery here, I would go through Finance, as this way it gets properly registered there.
I would just remove terminated employees to avoid having to deal with re-enabling. I don't think I fully understand your Note there.
It's quite a hard problem. I like this idea of being able to withdraw accrued value independently from Payroll. But we would still have the problem of the employer setting a value larger than existing funds. See discussion here

bingen · 2018-11-16T20:14:07Z

future-apps/payroll/contracts/Payroll.sol

@@ -43,53 +61,56 @@ contract Payroll is AragonApp { //, IForwarder { // makes coverage crash (remove
        uint64 lastPayroll;
        uint64 endDate;
        bool terminated;


Do we really need terminated? Isn't it enough with endDate? See discussion here

Oh, not really, see more discussion here

bingen · 2018-11-16T20:25:46Z

future-apps/payroll/contracts/Payroll.sol

        }
    }

-    function _terminateEmployee(uint128 employeeId, uint64 endDate) internal {
-        // prevent past termination dates
-        require(endDate >= getTimestamp64());


See this comment. Pinging @izqui

sohkai · 2018-11-19T13:56:14Z

@bingen

But this is what the old functions escapeHatch and depositToFinance were doing, isn't it?

I think a depositToFinance() or depositToVault() would still be useful, since it'll properly register the deposit with an event rather than being a silent transfer. The previous implementation didn't support ETH though.

I would just remove terminated employees to avoid having to deal with re-enabling. I don't think I fully understand your Note there.

Hmm, interesting note in #394 (comment).

My warning was that right now, it would be impossible to change an employee's details once they've been terminated due to the employeeExists modifier. Even if you terminate an employee, you may still want to change their salary, or even change their end date before their termination date (e.g., you might sign a 1-year contract and automatically set the end date to a year in the future).

Perhaps changing this modifier's behaviour may be better.

It's quite a hard problem. I like this idea of being able to withdraw accrued value independently from Payroll. But we would still have the problem of the employer setting a value larger than existing funds.

This seems like a general problem with this app and how it does accounting. Unless the employee can verify the employer's funds, they have no way to know if even payday() will work regardless of accumulated value. Having a cap of 2**128 is pretty artificial too; it's unlikely any employer would have enough if you add that much.

Maybe instead of choosing whether payday() should also pay your regular salary, we can instead add an optional value to let you choose how much you'd like to take out?

lmcorbalan · 2018-11-20T14:54:20Z

I think a depositToFinance() or depositToVault() would still be useful, since it'll properly register the deposit with an event rather than being a silent transfer. The previous implementation didn't support ETH though.

Something like?

function depositToFinance(address _token) public isInitialized {
    ERC20 tokenContract = ERC20(_token);
    uint256 amount = _token == ETH ? address(this).balance : tokenContract.balanceOf(this);
    require(amount > 0, ERROR_RECOVER_AMOUNT_ZERO);
    
    tokenContract.approve(address(finance), value);
    finance.deposit(tokenContract,  amount,  "Adding Funds");
}

sohkai · 2018-12-14T10:51:28Z

@lmcorbalan I've added a few commits that clean up some formatting and a bit of the testing.

Going on the depositToVault() discussion, I realized that this contract disallows receiving ETH and so only tokens can be mistakenly sent to it. Given that this is not a contract that is meant to receive tokens (unlike the Finance app), I think we can treat it as any other app and just use the default recoverability functionality included in all apps.

The remaining things left to think about, then, are:

terminated status (see PR: Payroll: remove terminated storage and change exist modifier for activeness check #610)
Accrued value denial-of-service mitigation (is there a strategy that better protects the receiver from the employer acting in bad faith; e.g. out of fund situations or overflowing an employee's accrued value?)

And will be addressed later.

lmcorbalan added 15 commits November 7, 2018 17:15

Payroll: Add messages for requires

f0bcccf

Payroll: Some tight Variable Packing

426cb1b

Payroll: Add missing @param in initialize header

f4764f8

Payroll: Use isContract from aragonOS to check _finance that is actua…

73c468a

…lly a contract

Payroll: Move emit at the end of function to follow the same pattern …

964f8d0

…as other functions

Payroll: Remove unnecessary isInitialized check

e2a8f5b

Payroll: Remove unused variable in payday

d7711c6

Payroll: Remove isInitialized when checking for employeeMatches

1001a63

Remove name from Employee struct and leave it in event

1ab4fcd

Remove depositToFinance

9c8261d

Keep only two functions for adding employee

d7c0a21

Pin some devDependencies

fee8739

Add index to addresses in events

5491e78

Rollback MAX_ACCRUED_VALUE check place

c1addec

sohkai added 11 commits November 16, 2018 12:12

Update package.json scripts and dependencies

bf39e5c

Update solidity-coverage configuration

4d28f51

tests: fix warning

f7dda76

Move nextEmployee storage slot to be next to employees

abe862c

Move rateExpiryTime storage slot to be in same order as constructor's…

84a1181

… arguments

Index price feed events

25f09a9

Adjust comments and radspec

2f11783

Fix arapp.json

5c03e3e

Update arapp.json to use new environments format

aa6cb36

Use a different error message for feed not being a contract

95639ba

Cache employee accesses

a5691c1

sohkai reviewed Nov 16, 2018

View reviewed changes

sohkai added 2 commits November 16, 2018 13:28

Use already created timestamp value

fba1746

Make isTokenAllowed() public

db604ce

sohkai and others added 3 commits November 16, 2018 13:46

Move visibility modifier to be first

61437fe

Reduce error string length

bc812b9

Add ganache-cli to devDependencies

d2818ac

bingen reviewed Nov 16, 2018

View reviewed changes

Use function overload for addEmployee

7c98370

lmcorbalan added 3 commits November 27, 2018 08:50

Add role param to addEmployee and capture it in AddEmployee event

ac81b59

Allow to change employee's role

a9d42a5

Send role to addEmployee from the app

66aa930

This was referenced Dec 6, 2018

Add Employee, role is not being stored in the contract protofire/aragon-apps#98

Closed

Add/Edit employee Role protofire/aragon-apps#100

Open

lmcorbalan and others added 3 commits December 12, 2018 16:20

Rollback setEmployeeRole logic

367bc84

cosmetic: small indenting change

5a97678

test: improve test scoping for add and removing employee tests

d1ae903

sohkai merged commit 487e0f8 into aragon:master Dec 14, 2018

sohkai deleted the payroll-milestone-1-review branch December 14, 2018 10:52

ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021

Payroll Milestone 1 - Addressing review comments (aragon#568)

7e581a5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Payroll Milestone 1 - Addressing review comments #568

Payroll Milestone 1 - Addressing review comments #568

lmcorbalan commented Nov 9, 2018 •

edited

Loading

coveralls commented Nov 9, 2018 •

edited

Loading

sohkai left a comment •

edited

Loading

sohkai Nov 15, 2018

bingen Nov 16, 2018

sohkai Nov 15, 2018

bingen Nov 16, 2018

sohkai Nov 15, 2018

lmcorbalan Nov 16, 2018

sohkai Nov 16, 2018

sohkai Nov 16, 2018

sohkai Nov 19, 2018

lmcorbalan Nov 20, 2018

lmcorbalan Nov 27, 2018

bingen left a comment

bingen Nov 16, 2018

bingen Nov 16, 2018

bingen Nov 16, 2018

sohkai commented Nov 19, 2018 •

edited

Loading

lmcorbalan commented Nov 20, 2018 •

edited

Loading

sohkai commented Dec 14, 2018 •

edited

Loading

Payroll Milestone 1 - Addressing review comments #568

Payroll Milestone 1 - Addressing review comments #568

Conversation

lmcorbalan commented Nov 9, 2018 • edited Loading

Payroll.sol

PayrollKit.sol

Unit Tests

coveralls commented Nov 9, 2018 • edited Loading

sohkai left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai commented Nov 19, 2018 • edited Loading

lmcorbalan commented Nov 20, 2018 • edited Loading

sohkai commented Dec 14, 2018 • edited Loading

lmcorbalan commented Nov 9, 2018 •

edited

Loading

coveralls commented Nov 9, 2018 •

edited

Loading

sohkai left a comment •

edited

Loading

sohkai commented Nov 19, 2018 •

edited

Loading

lmcorbalan commented Nov 20, 2018 •

edited

Loading

sohkai commented Dec 14, 2018 •

edited

Loading