[AUTOMATIONS] Vesting Scheduler - add claimable schedule feature #1944

0xPilou · 2024-05-24T13:00:29Z

The contract modification works includes :

added functions to create claimable feature (including overloaded method for diff configurations)
updated VestingSchedule data structure : added bool isClaimable in order to reflect if a given schedule requires claiming to start flowing
updated VestingSchedule data structure : added claimValidityDate used for claimable vesting schedule
updated VestingScheduleCreated event : added bool isClaimable
modified _executeCliffAndFlow method : behavior changes depending if the schedule is claimable or not. (i.e. if claimable, only the sender or receiver can execute)
increased test coverage

New contract version is available and deployed at :
https://sepolia-optimism.etherscan.io/address/0xB47FbC1B7f8372825D742709087b909D36137FD2

…lock

…otalDuration

* need to improve testing coverage

* work in progress, needs proper testing

* prefer reverting the early end until stream can be closed without needing the transfer (i.e. it will slightly overflow in that case)

* needs proper test cover * consider the log events

* add to log as well

* use greater or equal handling for case when only remainder needs to be transferred * assert transferFrom success result * add todo-s, improve tests

- add base-mainnet option

packages/automation-contracts/scheduler/contracts/VestingSchedulerV2.sol

kasparkallas · 2024-05-27T12:19:40Z

First glance additional thinking points:

Prefer to converge on a single internal _createVestingSchedule logic and avoid duplication
_executeEndVesting should have some check for the vesting schedule to have been claimed
Are there any updateVestingSchedule considerations with the claimable?

0xPilou · 2024-05-27T18:17:52Z

First glance additional thinking points:

Prefer to converge on a single internal _createVestingSchedule logic and avoid duplication

_executeEndVesting should have some check for the vesting schedule to have been claimed

Are there any updateVestingSchedule considerations with the claimable?

Updated the contract to only use a single internal function.

For executeEndVesting and updateVestingSchedule, I do not see further considerations required specifically for claimable schedules.

What extra check did you foresee in executeEndVesting ?
My rational was that if the schedule has been claimed, then _isFlowOngoing evaluates to true, and the vesting can then be ended. Else, (receiver didn't claim), only emit a "fail to execute vesting end" event.
It shouldn't be a problem since this function can only be called after the early end (which is the latest date possible of claim validity)

- keep it as one of the last for backwards compatibility

CannotClaimFlowOnBehalf to CannotClaimScheduleOnBehalf

github-actions · 2024-06-06T06:27:21Z

📦 PR Packages

Install this PR (you need to setup Github packages):

yarn add @superfluid-finance/ethereum-contracts@PR1944
yarn add @superfluid-finance/sdk-core@PR1944
yarn add @superfluid-finance/sdk-redux@PR1944

Click to learn how to use Github packages

To use the Github package registry, create a token with "read:packages" permission. See Creating a personal access token for help.

Next add these lines to your .npmrc file, replacing TOKEN with your personal access token. See Installing a package from Github if you get stuck.

@superfluid-finance:registry=https://npm.pkg.github.com
//npm.pkg.github.com/:_authToken=TOKEN

kasparkallas · 2024-06-10T11:36:34Z

GH closed this PR automatically a bit unexpectedly. Fixing.

packages/automation-contracts/scheduler/contracts/interface/IVestingSchedulerV2.sol

kasparkallas

Good work! @0xPilou

Happy to merge this into the feature set (epic) branch and continue with E2E testing. Future changes and improvements can be in the form of a new PR.

hellwolf · 2024-06-11T09:16:22Z

packages/automation-contracts/scheduler/test/VestingSchedulerV2.t.sol

@@ -25,6 +25,7 @@ contract VestingSchedulerV2Tests is FoundrySuperfluidTester {
        int96 flowRate,
        uint32 endDate,
        uint256 cliffAmount,
+        uint32 claimValidityDate,


some of the repetitive blocks of code could be made reusable internal functions, with some thoughtfulness.

many parameters could have been fuzzed from the start.

* added claimable vesting feature * add check on `_executeCliffAndFlow` for claimable schedules * updated time window condition on schedule claim * fix typo * add some unit tests for claiming schedules * increased test coverage * added claimValidityDate feature * updated tests * add claimValidityDate param to createSchedules function * updated unit tests * refactor internal function params (stack too deep) + add claimValidityDate to schedule creation event * removed `isClaimable` boolean from VestingSchedule data structure * remove internal function creating dupplication * updated claim validity date check logic * refactor: re-order the claimValidityDate in the event - keep it as one of the last for backwards compatibility * refactor: rename error CannotClaimFlowOnBehalf to CannotClaimScheduleOnBehalf * fix: remove merge issues from hardhat configs * fix: remove duplication from hardhat config * fix: moved & rename params struct into VestingSchedulerV2 contract --------- Co-authored-by: Kaspar Kallas <[email protected]> Co-authored-by: Kaspar Kallas <[email protected]> Signed-off-by: Miao, ZhiCheng <[email protected]>

* chore: no-op for pull request diff Signed-off-by: Miao, ZhiCheng <[email protected]> * [automations] Vesting Scheduler V2 ergonomic improvements (#1904) * allow creation and execution of the vesting schedule in the current block * add createVestingSchedule function which works with totalAmount and totalDuration * add overloads without ctx * need to improve testing coverage * add more overloads with fewer parameters * reorganize the functions * add create and execute schedule mvp * work in progress, needs proper testing * remove try-catch from early end * prefer reverting the early end until stream can be closed without needing the transfer (i.e. it will slightly overflow in that case) * add dust amount fix (wip) * needs proper test cover * consider the log events * rename from dustFixAmount to remainderAmount * add to log as well * fix test issues * tiny comment rename * remove functions create and execute functions with cliff period * add a comprehensive fuzzed test for createScheduleFromAmountAndDuration * slightly change end compensation & remainder handling * use greater or equal handling for case when only remainder needs to be transferred * assert transferFrom success result * add todo-s, improve tests * keep V1 contract, separate V2 explicitly * update deploy script for v2 * unify deploy scripts * use newer host.registerApp & unify deploy scripts - add base-mainnet option * clean-up * add diff generation script & completely revert VestingScheduler.sol Signed-off-by: Miao, ZhiCheng <[email protected]> * [AUTOMATIONS] Vesting Scheduler - add claimable schedule feature (#1944) * added claimable vesting feature * add check on `_executeCliffAndFlow` for claimable schedules * updated time window condition on schedule claim * fix typo * add some unit tests for claiming schedules * increased test coverage * added claimValidityDate feature * updated tests * add claimValidityDate param to createSchedules function * updated unit tests * refactor internal function params (stack too deep) + add claimValidityDate to schedule creation event * removed `isClaimable` boolean from VestingSchedule data structure * remove internal function creating dupplication * updated claim validity date check logic * refactor: re-order the claimValidityDate in the event - keep it as one of the last for backwards compatibility * refactor: rename error CannotClaimFlowOnBehalf to CannotClaimScheduleOnBehalf * fix: remove merge issues from hardhat configs * fix: remove duplication from hardhat config * fix: moved & rename params struct into VestingSchedulerV2 contract --------- Co-authored-by: Kaspar Kallas <[email protected]> Co-authored-by: Kaspar Kallas <[email protected]> Signed-off-by: Miao, ZhiCheng <[email protected]> * [AUTOMATIONS] VestingSchdulerV2 improvements (#1963) * update: change claimValidityDate to claimPeriod in function that takes amount and duration as params * fix: clear claimValidityDate on claim + add checks to execute* functions * update: add VestingClaimed event to `_executeCliffAndFlow` function Signed-off-by: Miao, ZhiCheng <[email protected]> * [automations] VestingSchedulerV2 add helpful view functions (#1965) * refactor: use uint96 for remainderAmount & change packing order * feat: add `getMaximumNeededTokenAllowance` helper function with a test * refactor: converge on view function usage * chore: add comments * chore: clean-up * chore: reset whitespace Signed-off-by: Miao, ZhiCheng <[email protected]> * [AUTOMATIONS] VestingSchedulerV2 claim after end date (#1964) * update: change claimValidityDate to claimPeriod in function that takes amount and duration as params * fix: clear claimValidityDate on claim + add checks to execute* functions * update: add VestingClaimed event to `_executeCliffAndFlow` function * feature: add capabilities to have claimValidityDate after endDate * fix: rearrange `_executeCliffAndFlow` logic * test: increased coverage for executeCliffAndFlow * test: added revert check on `executeEndVesting` test * refactor: clean-up - add additional asserts - change log event order (to match other situation) --------- Co-authored-by: Kaspar Kallas <[email protected]> Signed-off-by: Miao, ZhiCheng <[email protected]> * fix: check if schedule is claimed on executeEndVesting Signed-off-by: Miao, ZhiCheng <[email protected]> * test: increased `getMaximumNeededTokenAllowance` coverage Signed-off-by: Miao, ZhiCheng <[email protected]> * feat: added remainderAmount in `VestingScheduleUpdated` Signed-off-by: Miao, ZhiCheng <[email protected]> * add tests & fixes Signed-off-by: Miao, ZhiCheng <[email protected]> * [automations] Vesting Scheduler V2 refactoring after single-transfer feature (#1969) * refactor: explicit functions - _claim - _exececuteAtSingleTransfer - _getTotalVestedAmount * chore: test that schedule is deleted in more places * refactor: use more foundry bound in tests * chore: test better the scenario where the schedule is not ended on time * refactor: refactor to using aggregate object - make executeCliffAndFlow public * chore: improve the test further * refactor: use aggregate in all places * refactor: re-order some functions based on visibility * chore: add small comment * refactor: small whitespace fix * refactor: use named parameters when using structs * refactor: remove unnecessary comments * fix: change type in log event * chore: test claim event * chore: add version Signed-off-by: Miao, ZhiCheng <[email protected]> * chore: add optimism hardhat config Signed-off-by: Miao, ZhiCheng <[email protected]> * refactor: unify `createClaimableVestingSchedule` and `createVestingSchedule` functions Signed-off-by: Miao, ZhiCheng <[email protected]> * refactor: reoder `createVestingScheduleFromAmountAndDuration` function params Signed-off-by: Miao, ZhiCheng <[email protected]> * refactor: unify `createVestingScheduleFormAmountAndDuration` and `createClaimableVestingScheduleFormAmountAndDuration` functions Signed-off-by: Miao, ZhiCheng <[email protected]> * [automations] Vesting Scheduler V2 final clean-up (#1973) * refactor: unify `createClaimableVestingSchedule` and `createVestingSchedule` functions * refactor: reoder `createVestingScheduleFromAmountAndDuration` function params * refactor: unify `createVestingScheduleFormAmountAndDuration` and `createClaimableVestingScheduleFormAmountAndDuration` functions * refactor: remove confusing overloads * feat: add cliffPeriod to createAndExecute functions * unify modifiers & remove a helper function * refactor: use normalizeStartDate function to get a function to be pure * refactor: remove unnecessary passing of ctx * refactor: rename * add more claim fuzz tests * change log event semantics slightly for single transfer * remove version --------- Co-authored-by: Pilou <[email protected]> Signed-off-by: Miao, ZhiCheng <[email protected]> * [automations] Vesting scheduler v2 - fix & v1 compatibility (#1977) * refactor: unify `createClaimableVestingSchedule` and `createVestingSchedule` functions * refactor: reoder `createVestingScheduleFromAmountAndDuration` function params * refactor: unify `createVestingScheduleFormAmountAndDuration` and `createClaimableVestingScheduleFormAmountAndDuration` functions * chore: add `createVestingSchedule` v1 overload for backward compatibility * fix: remove `cliffPeriod` parameter in `createAndExecuteVestingScheduleFromAmountAndDuration` function * refactor: replace `_getSender(bytes(""))` by `msg.sender` Signed-off-by: Miao, ZhiCheng <[email protected]> * refactor: re-order functions for better readability Signed-off-by: Miao, ZhiCheng <[email protected]> --------- Signed-off-by: Miao, ZhiCheng <[email protected]> Co-authored-by: Pilou <[email protected]>

kasparkallas and others added 30 commits March 19, 2024 14:22

allow creation and execution of the vesting schedule in the current b…

e3a1013

…lock

add createVestingSchedule function which works with totalAmount and t…

2a4f363

…otalDuration

add overloads without ctx

552a3df

* need to improve testing coverage

add more overloads with fewer parameters

ad7ce58

reorganize the functions

b9ee878

add create and execute schedule mvp

89a173a

* work in progress, needs proper testing

remove try-catch from early end

b9464fb

* prefer reverting the early end until stream can be closed without needing the transfer (i.e. it will slightly overflow in that case)

add dust amount fix (wip)

648c5bf

* needs proper test cover * consider the log events

Merge remote-tracking branch 'origin/dev' into vesting-scheduler-1_2

adb07a5

rename from dustFixAmount to remainderAmount

59832dc

* add to log as well

fix test issues

96d06fa

tiny comment rename

9cce7cf

Merge remote-tracking branch 'origin/dev' into vesting-scheduler-1_2

d5cdaca

remove functions create and execute functions with cliff period

3c3c09c

add a comprehensive fuzzed test for createScheduleFromAmountAndDuration

78dc300

slightly change end compensation & remainder handling

cd264e5

* use greater or equal handling for case when only remainder needs to be transferred * assert transferFrom success result * add todo-s, improve tests

Merge branch 'dev' into vesting-scheduler-1_2

4af5726

Merge branch 'dev' into vesting-scheduler-1_2

5997eed

keep V1 contract, separate V2 explicitly

bd008a2

update deploy script for v2

5c75550

Merge remote-tracking branch 'origin/dev' into vesting-scheduler-1_2

a5239ce

unify deploy scripts

bfbefa1

use newer host.registerApp & unify deploy scripts

2bdd112

- add base-mainnet option

clean-up

cb84d33

added claimable vesting feature

b08d7d0

add check on _executeCliffAndFlow for claimable schedules

e353ccc

updated time window condition on schedule claim

cd6c5f4

fix typo

3750f31

add some unit tests for claiming schedules

1d2b08d

increased test coverage

23e99f7

kasparkallas requested review from ngmachado and kobuta23 May 27, 2024 08:04

kasparkallas reviewed May 27, 2024

View reviewed changes

packages/automation-contracts/scheduler/contracts/VestingSchedulerV2.sol Outdated Show resolved Hide resolved

kasparkallas reviewed May 27, 2024

View reviewed changes

packages/automation-contracts/scheduler/contracts/VestingSchedulerV2.sol Outdated Show resolved Hide resolved

0xPilou added 2 commits May 27, 2024 18:34

removed isClaimable boolean from VestingSchedule data structure

3da96a3

remove internal function creating dupplication

0485804

0xPilou requested a review from kasparkallas May 27, 2024 18:19

0xPilou and others added 3 commits May 30, 2024 19:58

updated claim validity date check logic

dac59ef

refactor: re-order the claimValidityDate in the event

e757797

- keep it as one of the last for backwards compatibility

refactor: rename error

1c4524e

CannotClaimFlowOnBehalf to CannotClaimScheduleOnBehalf

This was referenced Jun 10, 2024

Vesting Scheduler v2 superfluid-finance/automation-subgraphs#6

Merged

[automations][epic] Vesting Scheduler V2 #1952

Merged

kasparkallas deleted the branch superfluid-finance:feature-set-vesting-scheduler-v2 June 10, 2024 11:32

kasparkallas closed this Jun 10, 2024

kasparkallas reopened this Jun 10, 2024

kasparkallas changed the base branch from vesting-scheduler-1_2 to feature-set-vesting-scheduler-v2 June 10, 2024 11:36

kasparkallas added 3 commits June 10, 2024 14:45

chore: merge

de5d262

fix: remove merge issues from hardhat configs

d7f2a5a

fix: remove duplication from hardhat config

9934341

kasparkallas reviewed Jun 10, 2024

View reviewed changes

packages/automation-contracts/scheduler/contracts/interface/IVestingSchedulerV2.sol Outdated Show resolved Hide resolved

fix: moved & rename params struct into VestingSchedulerV2 contract

de2d7c3

kasparkallas approved these changes Jun 10, 2024

View reviewed changes

kasparkallas merged commit 8012606 into superfluid-finance:feature-set-vesting-scheduler-v2 Jun 10, 2024

hellwolf reviewed Jun 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AUTOMATIONS] Vesting Scheduler - add claimable schedule feature #1944

[AUTOMATIONS] Vesting Scheduler - add claimable schedule feature #1944

0xPilou commented May 24, 2024

kasparkallas commented May 27, 2024

0xPilou commented May 27, 2024

github-actions bot commented Jun 6, 2024

kasparkallas commented Jun 10, 2024

kasparkallas left a comment •

edited

Loading

hellwolf Jun 11, 2024

[AUTOMATIONS] Vesting Scheduler - add claimable schedule feature #1944

[AUTOMATIONS] Vesting Scheduler - add claimable schedule feature #1944

Conversation

0xPilou commented May 24, 2024

kasparkallas commented May 27, 2024

0xPilou commented May 27, 2024

github-actions bot commented Jun 6, 2024

📦 PR Packages

kasparkallas commented Jun 10, 2024

kasparkallas left a comment • edited Loading

Choose a reason for hiding this comment

hellwolf Jun 11, 2024

Choose a reason for hiding this comment

kasparkallas left a comment •

edited

Loading