Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wise-bobcats-speak.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`VestinWalletWithCliff`: Add a extension of the `VestingWallet` contract with an added cliff.
46 changes: 46 additions & 0 deletions contracts/finance/VestingWalletWithCliff.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

import {SafeCast} from "../utils/math/SafeCast.sol";
import {VestingWallet} from "./VestingWallet.sol";

/**
* @dev Extension of {VestingWallet} that adds a cliff to the vesting schedule.
*/
abstract contract VestingWalletWithCliff is VestingWallet {
using SafeCast for *;

uint64 private immutable _cliff;

error InvalidCliffDuration(uint64 cliffSeconds, uint64 durationSeconds);

/**
* @dev Sets the sender as the initial owner, the beneficiary as the pending owner, the start timestamp, the
* vesting duration and the duration of the cliff of the vesting wallet.
*/
constructor(uint64 cliffSeconds) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see durationSeconds is used in VestingWallet, but there it's straightforward because the parameter is the actual duration. In the case of cliffSeconds, it's added over the start, which may seem confusing.

We can:

  • Rename cliffSeconds to cliffDurationSeconds
  • Rename the cliff() function to cliffTimestamp

Leaning towards number 1:

Suggested change
constructor(uint64 cliffSeconds) {
constructor(uint64 cliffDurationSeconds) {

if (cliffSeconds > duration()) {
revert InvalidCliffDuration(cliffSeconds, duration().toUint64());
}
_cliff = start().toUint64() + cliffSeconds;
}

/**
* @dev Getter for the cliff timestamp.
*/
function cliff() public view virtual returns (uint256) {
return _cliff;
}

/**
* @dev Virtual implementation of the vesting formula. This returns the amount vested, as a function of time, for
* an asset given its total historical allocation.
*/
function _vestingSchedule(
uint256 totalAllocation,
uint64 timestamp
) internal view virtual override returns (uint256) {
return timestamp < cliff() ? 0 : super._vestingSchedule(totalAllocation, timestamp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not always calling super, and it's part of the discussion we've had before in regards to always call super.

In this case it harmless, so it doesn't make sense to call super every time since the ternary operator is extending the original branching in _vestingWallet, making it equivalent to:

function _vestingSchedule(uint256 totalAllocation, uint64 timestamp) internal view virtual returns (uint256) {
    if (timestamp < cliff() || timestamp < start()) {
        return 0;
    } else if (timestamp >= end()) {
        return totalAllocation;
    } else {
        return (totalAllocation * (timestamp - start())) / duration();
    }
}

I think it doesn't make sense to call super every time here, but if _vestingWallet is overridden by an user and it includes a check or an state update, it might be an issue.

I don't have a strong opinion. But I'm leaning towards not calling super. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not caller super at all is dangerous. Lets discuss that next week.

}
}
112 changes: 112 additions & 0 deletions test/finance/VestingWalletWithCliff.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
const { ethers } = require('hardhat');
const { expect } = require('chai');
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');

const { min } = require('../helpers/math');
const time = require('../helpers/time');

const { shouldBehaveLikeVesting } = require('./VestingWallet.behavior');

async function fixture() {
const amount = ethers.parseEther('100');
const duration = time.duration.years(4);
const start = (await time.clock.timestamp()) + time.duration.hours(1);
const cliffDuration = time.duration.years(1);
const cliff = start + cliffDuration;

const [sender, beneficiary] = await ethers.getSigners();
const mock = await ethers.deployContract('$VestingWalletWithCliff', [beneficiary, start, duration, cliffDuration]);

const token = await ethers.deployContract('$ERC20', ['Name', 'Symbol']);
await token.$_mint(mock, amount);
await sender.sendTransaction({ to: mock, value: amount });

const pausableToken = await ethers.deployContract('$ERC20Pausable', ['Name', 'Symbol']);
const beneficiaryMock = await ethers.deployContract('EtherReceiverMock');

const env = {
eth: {
checkRelease: async (tx, amount) => {
await expect(tx).to.emit(mock, 'EtherReleased').withArgs(amount);
await expect(tx).to.changeEtherBalances([mock, beneficiary], [-amount, amount]);
},
setupFailure: async () => {
await beneficiaryMock.setAcceptEther(false);
await mock.connect(beneficiary).transferOwnership(beneficiaryMock);
return { args: [], error: [mock, 'FailedInnerCall'] };
},
releasedEvent: 'EtherReleased',
argsVerify: [],
args: [],
},
token: {
checkRelease: async (tx, amount) => {
await expect(tx).to.emit(token, 'Transfer').withArgs(mock, beneficiary, amount);
await expect(tx).to.changeTokenBalances(token, [mock, beneficiary], [-amount, amount]);
},
setupFailure: async () => {
await pausableToken.$_pause();
return {
args: [ethers.Typed.address(pausableToken)],
error: [pausableToken, 'EnforcedPause'],
};
},
releasedEvent: 'ERC20Released',
argsVerify: [token],
args: [ethers.Typed.address(token)],
},
};

const schedule = Array(64)
.fill()
.map((_, i) => (BigInt(i) * duration) / 60n + start);

const vestingFn = timestamp => min(amount, timestamp < cliff ? 0n : (amount * (timestamp - start)) / duration);

return { mock, duration, start, beneficiary, cliff, schedule, vestingFn, env };
}

describe('VestingWallet', function () {
beforeEach(async function () {
Object.assign(this, await loadFixture(fixture));
});

it('rejects zero address for beneficiary', async function () {
await expect(
ethers.deployContract('$VestingWalletWithCliff', [
this.beneficiary,
this.start,
this.duration,
this.duration + 1n,
]),
)
.revertedWithCustomError(this.mock, 'InvalidCliffDuration')
.withArgs(this.duration + 1n, this.duration);
});

it('check vesting contract', async function () {
expect(await this.mock.owner()).to.equal(this.beneficiary);
expect(await this.mock.start()).to.equal(this.start);
expect(await this.mock.duration()).to.equal(this.duration);
expect(await this.mock.end()).to.equal(this.start + this.duration);
expect(await this.mock.cliff()).to.equal(this.cliff);
});

describe('vesting schedule', function () {
describe('Eth vesting', function () {
beforeEach(async function () {
Object.assign(this, this.env.eth);
});

shouldBehaveLikeVesting();
});

describe('ERC20 vesting', function () {
beforeEach(async function () {
Object.assign(this, this.env.token);
});

shouldBehaveLikeVesting();
});
});
});