Fix test on TokenVesting draft

[hems]

I just went through the tests on TokenVesting and figured out that this test isn't finished:

openzeppelin-contracts/test/drafts/TokenVesting.test.js

Lines 146 to 156 in cdf655f

	it('should keep the vested tokens when revoked by owner', async function () {
	await time.increaseTo(this.start.add(this.cliffDuration).add(time.duration.weeks(12)));
	const vestedPre = vestedAmount(amount, await time.latest(), this.start, this.cliffDuration, this.duration);
	await this.vesting.revoke(this.token.address, { from: owner });
	const vestedPost = vestedAmount(amount, await time.latest(), this.start, this.cliffDuration, this.duration);
	expect(vestedPre).to.be.bignumber.equal(vestedPost);
	});

What actually needs to be done in order for this test to be valuable is:

1. Add the "vested()" method on TokenVesting.sol

    /**
     * @return the vested amount of the token vesting.
     */
    function vested(IERC20 token) public view returns (uint256) {
        return _vestedAmount(token);
    }

2. Call the vested() method on the test instead of using the vestedAmount function from the test file

    expect(await this.vesting.vested()).to.equal(vestedPre);

Other than all the tests seems to be perfect, thanks a lot for all your work

[abcoathup]

Hi @hems,

Thanks for the feedback. The drafts directory was removed from OpenZeppelin Contracts 3.0.
See the documentation for details: https://docs.openzeppelin.com/contracts/3.x/drafts

If you look at #1214 TokenVesting may not be aligned with traditional vesting.

Depending on your use case, you could use TokenTimelock

When creating token time locks or vesting you should consider prevention strategies for: Bypassing Smart Contract Timelocks

I will close this issue as TokenVesting has been removed but really appreciate the feedback on the test.

[hems]

Hi @hems,

Hello

Thanks for the feedback. The drafts directory was removed from OpenZeppelin Contracts 3.0.
See the documentation for details: https://docs.openzeppelin.com/contracts/3.x/drafts

Yes I noticed!

If you look at #1214 TokenVesting may not be aligned with traditional vesting.

Actually the linear vesting is the perfect implementation for me and to be frank, I believe to a lot more people, making it more "feature-rich" ( aka complicated ) seems to create a lot of room for discussion and bugs, so in a way I really think the linear lock it the perfect solution as it's very simple to understand and to test, by tweaking that test i posted here i was able to get 100% test coverage.

Depending on your use case, you could use TokenTimelock

Thanks for the link, I'll use the TokenVesting contract tough as it works just as we would like it to work.

When creating token time locks or vesting you should consider prevention strategies for: Bypassing Smart Contract Timelocks

very interesting concept, thank you for pointing me out!