Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make TokenVesting more aligned with usual requirements #1214

Closed
frangio opened this issue Aug 16, 2018 · 21 comments · Fixed by #2748
Closed

Make TokenVesting more aligned with usual requirements #1214

frangio opened this issue Aug 16, 2018 · 21 comments · Fixed by #2748
Labels
contracts Smart contract code.

Comments

@frangio
Copy link
Contributor

frangio commented Aug 16, 2018

@alcuadrado mentioned to me that our TokenVesting contract is not aligned with the usual requirements for vesting, so some potential users have had to write their own implementations.

For example, our contract does a "continuous" release of tokens, whereas the usual requirement is for tokens to be released in batches e.g. every month.

We should review the contract and make it conform to what users need.

I'm a bit concerned that this will be a breaking change but we won't be able to get it done for v2.0. So perhaps we should look into adding any parameters that we need now, and implement support for them later. For example, for the issue I described above we could add a "granularity" parameter in the constructor (problaby a bad name) and for now require that it is always zero. Just a thought.

@frangio frangio added kind:improvement contracts Smart contract code. breaking change Changes that break backwards compatibility of the public API. labels Aug 16, 2018
@nventuro
Copy link
Contributor

nventuro commented Aug 16, 2018

What if we kept it out of the 2.0 release, since we know we're going to revamp it? It could then be later added in a minor release.

@rstormsf
Copy link
Contributor

rstormsf commented Aug 22, 2018

https://github.com/JoinColony/colonyToken/blob/master/contracts/Vesting.sol - I like this one
@nventuro
I'd change Colony's vesting to use SafeMath and update some of the requirements.

Let me know if you want me to submit a PR. But before we do it, please come up with clear specs.

@frangio

what I like about Colony's contracts:

  • it can support multiple vesting schedules
  • continuous release of tokens

@frangio
Copy link
Contributor Author

frangio commented Aug 23, 2018

@rstormsf We actually want to keep the current model where there is one vesting contract deployed per schedule per beneficiary, and Colony's Vesting is quite the opposite since it is a central contract that is used for all beneficiaries. Do you see other problems with our current TokenVesting that Colony's contract solves?

The continuous release is the biggest issue with our current offering. We need the possibility to make it non-continuous because that's the way actual paper-based vesting contracts are signed. Those may, for example, vest monthly, i.e. release only at the end of each month.

The frequency is one of the parameters that we have to add, but we need to do some more research on the typical vesting schedules used and try to support them. There may be other parameters necessary.

@rstormsf
Copy link
Contributor

rstormsf commented Aug 25, 2018

@frangio
Got it.

 one vesting contract deployed per schedule per beneficiary

any reason to keep it that way vs having a mapping of schedules?

@frangio
Copy link
Contributor Author

frangio commented Aug 27, 2018

@rstormsf Because it better sandboxes each investor's funds. It's also a simpler implementation, resulting in less attack surface.

@nventuro
Copy link
Contributor

#1040 by @ninjaahhh proposed graded vesting, where the amount of tokens released during each period doesn't remain the same (in their project, it increased as time went by). We may want to consider adding this to our implementation.

@thedon-chris
Copy link

As a token contract owner, I would like to vest tokens to recipients on a “monthly” & non-linear basis, so that tokens are released on a controlled monthly interval (as opposed to token holders withdrawing at their leisure)

@nventuro
Copy link
Contributor

Thanks for your input @thedon-chris! Would you (the owner) be in charge of doing the release of tokens (i.e. manually sending out transactions for each holder), or would the holders withdraw their tokens once they're able to do so (e.g. monthly)? Also, does this restriction come from a legal requirement? Are there any other aspects of the vesting scheme that you find don't fulfill your needs?

@thedon-chris
Copy link

@nventuro, I'm speaking as a representative of others by the way.

I wouldn't want to manually send out the transactions for each holder, instead they would withdraw once they're able to do so (or monthly). Restriction does not come from a legal requirement AFAIK.

@djenning90
Copy link

djenning90 commented Mar 18, 2019

I have something I'd like to share that might be of interest to this group.

I have just completed implementation of a pretty sophisticated ERC20 contract for a full-featured vesting token. I rolled my own because what I've seen on the internet so far hasn't met our needs, which include many of the wishes expressed above. Our implementation is based on openzeppen-solidity as much as possible, and is built along the same guidelines of style and works harmoniously with it. I will be publishing this work on GitHub within the next day or so, and will post the link here for all to see when it's up.

Here is a brief summary of its features and capabilities:

  • Simultaneously supports both revocable and irrevocable token grants. Use cases for this are token purchases (irrevocable) as well employee compensation plans (revocable).
  • At the time of grant, tokens are deposited into the beneficiary's account, but the non-vested portion is prevented from being spent. We like this model because it ensures the not-vested tokens are stored safely with the beneficiary and remain locked, with no possibility of unreleased tokens being accidentally spent or used for another purpose.
  • If a revocable token grant is revoked by the grantor, the beneficiary keeps the vested tokens, and the not-vested tokens are returned to the grantor.
  • Sophisticated support for a grantor role, which can be assigned to multiple accounts, creating the ability to form multiple grant pools of different types for different purposes, each with its own grantor.
  • Each grant pool may have its own uniform vesting schedule, which is applied to grants made to all beneficiaries from that pool. Restrictions can be set to parameterize the grantor's ability to set start dates, and a grantor expiration date can be set to automatically close the pool.
  • There's also an ability to create one-off grants, where each beneficiary can have a unique vesting schedule for its grant.
  • The vesting schedule supports start date, cliff date, end date and interval. If interval is 1, the grant vests linearly. If interval is a number like say 30, vesting bumps up every 30 days. There is a restriction that both the interval from start to the cliff and the overall duration must be an even multiple of the interval. This flexibility opens up many possible vesting patterns.
  • All grant-related dates are measured in whole days, with each day starting at midnight UTC time. This locks all vesting to the same clock, which will help with orderly bookkeeping. It also disallows absurd cases like grants that are 15 minutes long, etc.
  • There is a limit of one vesting grant at a time per account. A beneficiary can have multiple grants in effect on different terms simply by using a new account for each new grant.
  • Support for an address self-registration mechanism, and safe methods which will only transact with a verified address, to help prevent token loss through accidental bad data.
  • Enterprise-style support for roles and permissions, with mechanisms in place to prevent loss of ownership control or accidental transfer of ownership to an invalid address.
  • Full automated test coverage of the contract code written in nodejs.

I am wrapping up loose ends with this work before I publish the source, but it will be soon. I am eager to receive any feedback and hear ideas for improvement that this group may have. Also, I hope many of you will find it useful, and can help me improve on it.

Thanks!

  • DJ

@djenning90
Copy link

djenning90 commented Mar 18, 2019

We've shared the code to the world, for all to look at, reuse and improve upon. Another wish is to get some experienced fresh eyes on the Solidity code, to have a look for any obvious problems. Any comments, feedback or ideas will all be appreciated and used constructively.

Please see here: https://github.com/mediarichio/ProxyToken

A good starting might be: https://github.com/mediarichio/ProxyToken/blob/master/contracts/ERC20Vestable.sol

Thank you :-)

Note: We are currently experiencing issues with our GitHub repository, which at the moment is not reachable (you may see a 404). We are working with GitHub to resolve this as soon as possible. If you are unable to see ProxyToken, please check back in a day or two (I will remove this message once it has been resolved). Thank you for your patience.

@djenning90
Copy link

djenning90 commented Mar 19, 2019

Wishes stated in the above conversation that are supported by my implementation:

  • Continuous vesting vs interval vesting are both supported (the shortest supported vesting interval is daily, which might be close enough to “continuous” for most applications that need it).
  • Uses SafeMath, and/or only works with range-restricted, checked values.
  • Supports both vesting schedule models: a unique vesting schedule per beneficiary or a shared vesting schedule for many beneficiaries (administered by a grant pool).
  • Tokens are automatically released at at regular interval, depending on what is specified in the vesting schedule. Can be daily, weekly, monthly, quarterly, etc.) The grantor never has to take any action to release vested tokens.

What’s not supported, which I saw mentioned above:

  • Doesn’t support múltiple simultaneous vesting schedules for one beneficiary. Implementing that looked to be messy and would have cost a lot of gas. Workaround is simply to use more ethereum accounts if needed, so one person can serve as multiple beneficiaries.
  • Implementation is rather large, so it doesn’t necessarily have a small attack surface. Best practices are used to minimize risk, however.
  • Doesn’t try to be a general purpose token, but instead assumes that the ICO/IEO project may not have implemented its ecosystem token or even fully know its requirements up front, and therefore tries only to be the token for managing grants. The expectation is that when the ecosystem is built later and it’s smart contract is ready, the vested/granted tokens would later be exchangeable/redeemable 1:1 for the project’s final ecosystem token. This also adheres to a software best practice called “the single responsibility principle”.

Eager to hear your feedback.

@rstormsf
Copy link
Contributor

If someone is looking for a vesting contract, feel free to use this one: https://etherscan.io/address/0xd6d79f85d8cb962b15181aac0c1545d61b6c5672#code

It's battle tested already with cliffs, monthly releases.

https://gist.github.com/rstormsf/7cfb0c6b7a835c0c67b4a394b4fd9383

@tholder
Copy link

tholder commented Nov 16, 2020

@rstormsf i made some amends to your contract here https://github.com/tapmydata/tap-protocol/blob/main/contracts/VestingVault.sol and added tests for it. Thanks.

@NoahMarconi
Copy link

Here's a modified version as well that vests ether instead of tokens: https://github.com/dan13ram/grants-platform-contracts/blob/master/contracts/EtherVesting.sol

It's the one used at opengrants.com

@frangio frangio removed the breaking change Changes that break backwards compatibility of the public API. label Feb 12, 2021
@ashishk74
Copy link

@rstormsf i made some amends to your contract here https://github.com/tapmydata/tap-protocol/blob/main/contracts/VestingVault.sol and added tests for it. Thanks.

Hello , I used the vestingvault contract but at the time of using addTokenGrant() , it throws an error of 'subtraction overflow' . I believe that is to say that owner does not have any token at the time of transfer to contract .
require(token.transferFrom(owner(), address(this), _amount));

but I provided all 1 billion token to owner in ERC20 contract at time of deployment. My contract vestingvault.sol is at
https://github.com/ashishk74/Vesting

Can somebody kindly debug the issue ?

@NoahMarconi
Copy link

I'm on mobile but from your message only my first guess is: did owner approve the contract to be permitted to transferFrom?

@ashishk74
Copy link

I'm on mobile but from your message only my first guess is: did owner approve the contract to be permitted to transferFrom?

You mean I should run approve() in vesting contract ?

@NoahMarconi
Copy link

NoahMarconi commented Apr 29, 2021 via email

@frangio
Copy link
Contributor Author

frangio commented Apr 29, 2021

@ashishk74 @NoahMarconi Please open a different issue or a topic on the forum, as many people are probably subscribed to this issue and get notifications.

@mirzausman371
Copy link

We've shared the code to the world, for all to look at, reuse and improve upon. Another wish is to get some experienced fresh eyes on the Solidity code, to have a look for any obvious problems. Any comments, feedback or ideas will all be appreciated and used constructively.

Please see here: https://github.com/mediarichio/ProxyToken

A good starting might be: https://github.com/mediarichio/ProxyToken/blob/master/contracts/ERC20Vestable.sol

Thank you :-)

Note: We are currently experiencing issues with our GitHub repository, which at the moment is not reachable (you may see a 404). We are working with GitHub to resolve this as soon as possible. If you are unable to see ProxyToken, please check back in a day or two (I will remove this message once it has been resolved). Thank you for your patience.

Hi @djenning90 I've a question, Can we able to Vest Tokens for Multiple Users in 1 Trx, Let's say that we want to Vest the Tokens for 1000 Users, Is that possible?

@frangio frangio mentioned this issue Jul 5, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants