[automations] Vesting Scheduler V2 refactoring after single-transfer feature #1969
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- _claim - _exececuteAtSingleTransfer - _getTotalVestedAmount
- make executeCliffAndFlow public
hellwolf
approved these changes
Jun 28, 2024
kasparkallas
merged commit 67539fb
into
feature-set-vesting-scheduler-v2
14 checks passed
kasparkallas
deleted the
vesting-scheduler-v2-refactoring-after-single-transfer-claim
branch
XKCD Comic RelifLink: https://xkcd.com/1969 |
hellwolf
pushed a commit
that referenced
this pull request
Jul 23, 2024
…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]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 23, 2024
* 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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why?
The single-transfer feature for claiming after the end date created code duplication and made it hard to follow from bird's eye view what was happening, making bugs easy to miss and contract harder to audit.
What?
Refactored to additional private functions to alleviate code duplication and to make it clearer at a glance what's happening.
Notes
_getTotalVestedAmount,_maxDateToExecuteStartInclusive,_minDateToExecuteEndInclusiveprivate functions to reduce duplication and increase readabiliy_getVestingScheduleAggregatefunction and struct to move (vesting schedule, vesting schedule id, super token, sender, receiver) together in the private functions, and to reduce duplicationTest version with smaller timeframes deployed here: https://sepolia-optimism.etherscan.io/address/0xF0789B3B229b00D81473935f84960E4435e430b4#code