From 4115686b4f8c1abf29f1f855eb15308076159959 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= <nicolas.venturo@gmail.com>
Date: Thu, 18 Oct 2018 11:27:46 -0300
Subject: [PATCH] TokenVesting improvements (#1431)

* Improved TokenVesting events.

* Added extra checks to TokenVesting.

* Renamed the events.

* Fixed linter error.

* Fixed a test that failed to cover a require.

* Renamed TokensRevoked to TokenVestingRevoked.

(cherry picked from commit 67dac7ae9960fd1790671a315cde56c901db5271)
---
 contracts/drafts/TokenVesting.sol | 10 ++++++----
 test/drafts/TokenVesting.test.js  | 30 ++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/contracts/drafts/TokenVesting.sol b/contracts/drafts/TokenVesting.sol
index 965ace102..2f5fd2fdc 100644
--- a/contracts/drafts/TokenVesting.sol
+++ b/contracts/drafts/TokenVesting.sol
@@ -16,8 +16,8 @@ contract TokenVesting is Ownable {
   using SafeMath for uint256;
   using SafeERC20 for IERC20;
 
-  event Released(uint256 amount);
-  event Revoked();
+  event TokensReleased(address token, uint256 amount);
+  event TokenVestingRevoked(address token);
 
   // beneficiary of tokens after they are released
   address private _beneficiary;
@@ -52,6 +52,8 @@ contract TokenVesting is Ownable {
   {
     require(beneficiary != address(0));
     require(cliffDuration <= duration);
+    require(duration > 0);
+    require(start.add(duration) > block.timestamp);
 
     _beneficiary = beneficiary;
     _revocable = revocable;
@@ -122,7 +124,7 @@ contract TokenVesting is Ownable {
 
     token.safeTransfer(_beneficiary, unreleased);
 
-    emit Released(unreleased);
+    emit TokensReleased(token, unreleased);
   }
 
   /**
@@ -143,7 +145,7 @@ contract TokenVesting is Ownable {
 
     token.safeTransfer(owner(), refund);
 
-    emit Revoked();
+    emit TokenVestingRevoked(token);
   }
 
   /**
diff --git a/test/drafts/TokenVesting.test.js b/test/drafts/TokenVesting.test.js
index c43a0d8b8..be872ae22 100644
--- a/test/drafts/TokenVesting.test.js
+++ b/test/drafts/TokenVesting.test.js
@@ -1,4 +1,5 @@
 const shouldFail = require('../helpers/shouldFail');
+const expectEvent = require('../helpers/expectEvent');
 const time = require('../helpers/time');
 const { ethGetBlock } = require('../helpers/web3');
 const { ZERO_ADDRESS } = require('../helpers/constants');
@@ -22,7 +23,7 @@ contract('TokenVesting', function ([_, owner, beneficiary]) {
     this.duration = time.duration.years(2);
   });
 
-  it('rejects a duration shorter than the cliff', async function () {
+  it('reverts with a duration shorter than the cliff', async function () {
     const cliffDuration = this.duration;
     const duration = this.cliffDuration;
 
@@ -33,12 +34,28 @@ contract('TokenVesting', function ([_, owner, beneficiary]) {
     );
   });
 
-  it('requires a valid beneficiary', async function () {
+  it('reverts with a null beneficiary', async function () {
     await shouldFail.reverting(
       TokenVesting.new(ZERO_ADDRESS, this.start, this.cliffDuration, this.duration, true, { from: owner })
     );
   });
 
+  it('reverts with a null duration', async function () {
+    // cliffDuration should also be 0, since the duration must be larger than the cliff
+    await shouldFail.reverting(
+      TokenVesting.new(beneficiary, this.start, 0, 0, true, { from: owner })
+    );
+  });
+
+  it('reverts if the end time is in the past', async function () {
+    const now = await time.latest();
+
+    this.start = now - this.duration - time.duration.minutes(1);
+    await shouldFail.reverting(
+      TokenVesting.new(beneficiary, this.start, this.cliffDuration, this.duration, true, { from: owner })
+    );
+  });
+
   context('once deployed', function () {
     beforeEach(async function () {
       this.vesting = await TokenVesting.new(
@@ -62,7 +79,11 @@ contract('TokenVesting', function ([_, owner, beneficiary]) {
 
     it('can be released after cliff', async function () {
       await time.increaseTo(this.start + this.cliffDuration + time.duration.weeks(1));
-      await this.vesting.release(this.token.address);
+      const { logs } = await this.vesting.release(this.token.address);
+      expectEvent.inLogs(logs, 'TokensReleased', {
+        token: this.token.address,
+        amount: await this.token.balanceOf(beneficiary),
+      });
     });
 
     it('should release proper amount after cliff', async function () {
@@ -100,7 +121,8 @@ contract('TokenVesting', function ([_, owner, beneficiary]) {
     });
 
     it('should be revoked by owner if revocable is set', async function () {
-      await this.vesting.revoke(this.token.address, { from: owner });
+      const { logs } = await this.vesting.revoke(this.token.address, { from: owner });
+      expectEvent.inLogs(logs, 'TokenVestingRevoked', { token: this.token.address });
       (await this.vesting.revoked(this.token.address)).should.equal(true);
     });