Add token vesting #476
Add token vesting #476
Conversation
contracts/token/TokenVesting.sol
Outdated
| using SafeMath for uint256; | ||
|
|
||
| event Release(uint256 amount); | ||
| event Revoke(); |
There was a problem hiding this comment.
The naming convention for events is either verbs in the past tense (Revoked) or nouns (Revocation?). I prefer the first one in this case.
There was a problem hiding this comment.
I'm also changing Release to Released then!
contracts/token/TokenVesting.sol
Outdated
| */ | ||
| function TokenVesting(address _beneficiary, uint256 _start, uint256 _cliff, uint256 _duration, bool _revocable) { | ||
| require(_beneficiary != 0x0); | ||
| require(_cliff < _duration); |
There was a problem hiding this comment.
I think I'd allow _cliff == _duration too. Do you see any downsides to that?
There was a problem hiding this comment.
No, I think it's good.
contracts/token/TokenVesting.sol
Outdated
| function TokenVesting(address _beneficiary, uint256 _start, uint256 _cliff, uint256 _duration, bool _revocable) { | ||
| require(_beneficiary != 0x0); | ||
| require(_cliff < _duration); | ||
| require(_start >= now); |
There was a problem hiding this comment.
I'm not sure about this. On one hand it's aligned with our usual recommendation for preconditions, as it would prevent some errors. On the other hand, I can imagine a situation where you want to create a contract for a vesting period that has been arranged off-chain, and already begun. What do you think about doing require(_start + _duration > now) instead?
There was a problem hiding this comment.
@frangio agree! But taking that into account I think we should also allow _start + _duration < now in order to support more use cases. I'll remove this check if you're ok with with it.
contracts/token/TokenVesting.sol
Outdated
| } | ||
|
|
||
| /** | ||
| * @dev Calculates the amount that has already vested but not released. |
There was a problem hiding this comment.
"[...] but hasn't been released yet." sounds better I think.
contracts/token/TokenVesting.sol
Outdated
| uint256 vested = totalBalance.mul(now - start).div(duration); | ||
| uint256 unreleased = vested.sub(released[token]); | ||
|
|
||
| // currentBalance can be 0 in case of a revoke |
There was a problem hiding this comment.
"[...] in the case that vesting was revoked early."
test/TokenVesting.js
Outdated
| }); | ||
|
|
||
| it('should be revoked by owner if revocable is set', async function () { | ||
| const vesting = await TokenVesting.new(beneficiary, this.start, this.cliff, this.duration, true, { from: owner } ); |
There was a problem hiding this comment.
Any reason why you created a new identical instance here?
There was a problem hiding this comment.
I wanted to make it explicit and to avoid breaking this test by changing the beforeEach instantiation, but it can be removed.
|
@frangio thanks for the feedback! Updated with corrections. |
|
LGTM! Please remove the |
…eneficiary balance over vestedAmount
…ts. Improve comments' wording.
martriay
force-pushed
the
add-token-vesting
branch
from
f8c3eda
to
22b9263
Compare
|
@frangio rebased due to merge conflicts. Ready! |
|
Awesome, thanks! |
Add token vesting
Continues #412, fixes #274.