Skip to content

Conversation

@JoE11-y
Copy link

@JoE11-y JoE11-y commented Dec 19, 2024

This Pr Resolves #53

Work Done

  1. Added Vesting Contract that supports both Cliff and Linear vesting types
  2. Added Tests

@JoE11-y JoE11-y marked this pull request as draft December 19, 2024 10:29
@JoE11-y JoE11-y marked this pull request as ready for review December 21, 2024 15:36
@JoE11-y
Copy link
Author

JoE11-y commented Dec 23, 2024

@raduciobanu22 this is ready for review

@raduciobanu22
Copy link

@JoE11-y A few notes after checking the PR:

  • I would have preferred to rely on a more straightforward approach to calculate claimable amount using only start, duration, current block and amount claimed so far. Using predefined milestones makes the contract less flexible. A good blueprint is the OpenZeppelin VestingWallet contract and also the VestingWalletCliff contract that extends it.
  • In general, smart contract code has to be very efficient, which means relying on math tricks vs loops. Something to keep in mind
  • Support for fungible tokens is required besides ALPH
  • Cliff support is required

Thanks for the effort!

@JoE11-y
Copy link
Author

JoE11-y commented Jan 2, 2025

Hi @raduciobanu22 thanks for taking the time to review the PR and share your thoughts. I appreciate the feedback! I just wanted to clarify a few things and share my perspective:

  1. Cliff Support: Cliff is already supported, as shown in the tests. It’s also designed to work seamlessly with linear vesting when setting milestones, giving flexibility to combine both approaches if needed.

  2. Predefined Milestones: The idea behind using predefined milestones was to give creators more control to design vesting schedules exactly how they want. I thought this would make the contract more adaptable to different needs.

  3. Multiple User Vesting: One of the key benefits of this implementation is that it supports multiple users in a single contract, which simplifies things and avoids the need for deploying separate contracts for each user.

  4. Loops: I get the concern around loops, but in this case, the loop’s impact is minimal once the nextMilestone variable is kept up-to-date.

@JoE11-y
Copy link
Author

JoE11-y commented Jan 3, 2025

@raduciobanu22 made the changes as requested.

@raduciobanu22
Copy link

@raduciobanu22 made the changes as requested.

I think the only thing missing is support for fungible tokens..
Btw, there's a built-in function for the ALPH amount that needs to be deposited when creating a sub-contract: https://docs.alephium.org/ralph/built-in-functions/#minimalcontractdeposit

Thanks for the refactoring, looking good!

@JoE11-y
Copy link
Author

JoE11-y commented Jan 3, 2025

Done with it @raduciobanu22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Token Vesting Contracts

2 participants