Employee vesting #10
Employee vesting #10
Conversation
| * @param amount the total outstanding amount to be claimed for this vesting schedule. | ||
| * @param currentTime the current timestamp. | ||
| * @param startTime the timestamp this vesting schedule started. | ||
| * @param endTime the timestamp this vesting schedule ends. |
There was a problem hiding this comment.
Missing @return field in this NatSpec comment.
There was a problem hiding this comment.
This would arguably be what @notice is describing, but I agree that it should be @return instead.
| @@ -2,3 +2,4 @@ node_modules/ | |||
| package-lock.json | |||
| yarn.lock | |||
There was a problem hiding this comment.
Once again, with modern package managers, lockfiles should be committed to source control. Here are some resources elaborating on this topic:
test/EmployeeVesting.js
Outdated
| assert.equal(claimed[1].toString(), await web3.utils.toWei('50').toString()) | ||
| }) | ||
|
|
||
| it('With no cliff', async () => { |
There was a problem hiding this comment.
Either the message is wrong or this should be a context?
| }) | ||
| }) | ||
| }) | ||
| describe('cancel', () => { |
There was a problem hiding this comment.
Check for onlyOwner is warranted here.
There was a problem hiding this comment.
@sporejack what do you mean by this comment? A check for onlyOwner and onlyDAO exists for this test?
test/EmployeeVesting.js
Outdated
| context('When not enough tokens are held', () => { | ||
| it('allows vesting to be set', async () => { | ||
| await tcr.transfer(vesting.address, web3.utils.toWei('51')) | ||
| await vesting.setVestingSchedule(accounts[1], web3.utils.toWei('50'), true, 26, 156, now); | ||
| let userVesting = await vesting.getVesting(accounts[1], 0) | ||
| assert.equal(userVesting[0].toString(), await web3.utils.toWei('50')) | ||
| assert.equal(userVesting[1].toString(), await web3.utils.toWei('0')) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
This could be a nitpick, but this test claims to test When not enough tokens are held, but there are 51 tokens held (greater than the amount vested, which is 50).
There was a problem hiding this comment.
Ah I meant to change this to be the other way around (I added 1 extra token instead of taking away 1).
Motivation
Added features for optimising the way in which employee vesting is handled. This includes only the DAO being able to cancel employee vests, adding functionality for multiple employee vesting schedules within a single transaction.
Changes
-Added capability for adding multiple employee vesting schedules in one transaction
-Added onlyDAO modifier
-Added setDAOAddress for changing the dao address
-Changed cancel vesting function to only be callable by DAO address