Add VestingWalletWithCliff #4870
Conversation
…ed cliff in the release schedule
🦋 Changeset detectedLatest commit: 188d86e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Following discussion, moving to a model where the extension is an abstract contract that needs to be inherited/compiled. |
| uint256 totalAllocation, | ||
| uint64 timestamp | ||
| ) internal view virtual override returns (uint256) { | ||
| return timestamp < cliff() ? 0 : super._vestingSchedule(totalAllocation, timestamp); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think not caller super at all is dangerous. Lets discuss that next week.
| * @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) { |
There was a problem hiding this comment.
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
cliffSecondstocliffDurationSeconds - Rename the
cliff()function tocliffTimestamp
Leaning towards number 1:
| constructor(uint64 cliffSeconds) { | |
| constructor(uint64 cliffDurationSeconds) { |
Co-authored-by: Ernesto García <[email protected]>
… "$"" This reverts commit d4367bb.
There was a problem hiding this comment.
I've been thinking a lot about the _vestingSchedule not calling super, and even tried to think about implementing the cliff in terms of the _vestingSchedule, but also realized that allows an attacker to extend the cliff undefinitely if they have enough resources.
So far I think this is the best approach, and also, if the "cliff" also means ignoring the scheduling function at all, then it makes sense to return 0 directly.
I added a note about this to _vestingWallet, since in the end, this is a threat if they ever have to dissambiguate the inheritance graph, so ideally they should take a look at this and decide whether or not they're okay with the cliff implementation.
|
Why vesting wallet cliff is not available in the v5.0.2 version? Can't import it in my contracts. |
|
@urataps This PR was not released yet. It will be in 5.1 |
... an extension of VestingWallet with an added cliff in the release schedule
Fixes #4701
Fixes LIB-1190
PR Checklist
npx changeset add)