Skip to content

Commit

Permalink
Remove TokenTimelock, PaymentSplitter, ERC20Snapshot, ERC20VotesComp,…
Browse files Browse the repository at this point in the history 
… GovernorVotesComp (#4276)
  • Loading branch information
@Amxx
Amxx authored May 26, 2023
1 parent 4448c13 commit 15c5c71
Show file tree
Hide file tree
Showing 22 changed files with 37 additions and 1,828 deletions.
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
# Changelog

### Removals

The following contracts were removed:

- `ERC20Snapshot`

This comment has been minimized.

Sign in to view

Copy link
@jat9292

jat9292 Jul 10, 2023

why??

This comment has been minimized.

Sign in to view

Copy link
@frangio

frangio Jul 10, 2023 •
edited
Loading

Contributor

Our efforts are focused on ERC20Votes which provides similar functionality. However, we are open to feedback on removals. If this contract is important to you please let us know.

This comment has been minimized.

Sign in to view

Copy link
@jat9292

jat9292 Jul 10, 2023

ERC20Votes does a snapshot at each transfer or delegation. This is surely convenient for voting but not for coupon distribution. ERC20Snapshot was particularly good for coupon distribution of a security token

This comment has been minimized.

Sign in to view

Copy link
@jbrodier

jbrodier Jul 24, 2023 •
edited
Loading

Our efforts are focused on ERC20Votes which provides similar functionality. However, we are open to feedback on removals. If this contract is important to you please let us know.

PaymentSplitter removal is very important / penalizing to us, as our token post-issuance infrastructure relies on it.
ERC20Votes is very generic and has many dependencies.

Investors need simplistic Vesting contracts to verify their holdings (i.e. VestingWallet and PaymentSplitter) and easily readable code. ERC20Votes is more generic and requires a lot of non-used dependencies in the context of Vesting.

Also, shares should not be redistributed in the context of vesting, otherwise investors have no proof that they don't get diluted. Of course the owner could revoke himself but the Ownable interface is not useful.

Investment operations like ICOs or STOs contain multiple vesting rules (one per investment round + one for founder + advisors, etc). Deploying one or more couples [ VestingWallet | PaymentSplitter ], sometimes with several levels of PaymentSplitters (when too many beneficiaries) was already cumbersome. But replacing a simple PaymentSplitter by ERC20Votes would imply deploying tens and tens of smart contracts and make the process unclear for investors and error prone.

Also, the wording "ERC20Votes" is not adapted if the shares only imply financial rights (in the Vesting process) and no voting rights.

We really hope that the PaymentSplitter could be maintained or that a simpler version of ERC20Votes could be provided.

Thanks for your help and amazing job

This comment has been minimized.

Sign in to view

Copy link
@Amxx

Amxx Jul 25, 2023

Author Collaborator

@jbrodier We feel like ERC20Snapshot is replaced with ERC20Votes. PaymentSplitter is a very different thing.

  • You can still use the PaymentSplitter from the 4.9 releases. We will continue to provide security warnings for a while (@frangio maybe we want to make sure that is covered by the bug bounty for some time even after it gets out of the master branch).
  • Are you using a PaymentSplitter as the recipient of a VestingWallet. Why do that instead of having multiple VestingWallet (one per investor) ? If that would solve your usecase, know that this app allows you to deploy vesting wallets easily (Note that the beneficiary is transferable)

This comment has been minimized.

Sign in to view

Copy link
@jbrodier

jbrodier Jul 25, 2023

@Amxx

  • Thanks for the reply and continuing the security warnings on the 4.9 release, that would be great.
  • We use VestingWallet and PaymentSplitter in the context of a tokenisation platform.
    We have tens of STOs/ICOs, each of them have between 5 and 10 vesting rules, and each rule has 1 to 5000 beneficiaries.
    We deploy at least one couple [VestingWallet+PaymentSplitter] per vesting rule ( often more than one, because we don't gather all the beneficiaries address at the same time, and also for technical reasons, when there are many beneficiaries addresses to pass to the constructor of the PaymentSplitter)

This comment has been minimized.

Sign in to view

Copy link
@Amxx

Amxx Jul 25, 2023 •
edited
Loading

Author Collaborator

You should maybe have a look at this contract
- `ERC20VotesComp`
- `GovernorVotesComp`
- `PaymentSplitter`

This comment has been minimized.

Sign in to view

Copy link
@rdevaul

rdevaul Jul 18, 2023

I'm confused as to how PaymentSplitter.sol, which appears to be a very general mechanism for pull payment distribution for eth or ERC20, is redundant with ERC20Votes or VestingWallet? Was this was a mistake, or was there some other problem with PaymentSplitter?

This comment has been minimized.

Sign in to view

Copy link
@Amxx

Amxx Jul 18, 2023

Author Collaborator

The issue we have with PaymentSplitter is that the list of beneficiary is set in stone during construction. Its impossible for a beneficiary to change its address if there is any issue with the key. It's also impossible to add, remove or redistribute shares.

We would like to reintroduce it with a more powerfull design that allows that kind of things

This comment has been minimized.

Sign in to view

Copy link
@jbrodier

jbrodier Jul 21, 2023

We precisely do not want shares to be redistributed in the context of vesting.
Otherwise beneficiaries cannot be sure they will receive the right amount escrowed in a VestingWallet
- `TokenTimelock` (removed in favor of `VestingWallet`)

### How to upgrade from 4.x

#### ERC20, ERC721, and ERC1155
Expand Down
214 changes: 0 additions & 214 deletions contracts/finance/PaymentSplitter.sol
View file

This file was deleted.

6 changes: 0 additions & 6 deletions contracts/finance/README.adoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,10 @@ NOTE: This document is better viewed at https://docs.openzeppelin.com/contracts/

This directory includes primitives for financial systems:

- {PaymentSplitter} allows to split Ether and ERC20 payments among a group of accounts. The sender does not need to be
aware that the assets will be split in this way, since it is handled transparently by the contract. The split can be
in equal parts or in any other arbitrary proportion.
- {VestingWallet} handles the vesting of Ether and ERC20 tokens for a given beneficiary. Custody of multiple tokens can
be given to this contract, which will release the token to the beneficiary following a given, customizable, vesting
schedule.
== Contracts

{{PaymentSplitter}}

{{VestingWallet}}
12 changes: 11 additions & 1 deletion contracts/finance/VestingWallet.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ import "../utils/Context.sol";
* Any token transferred to this contract will follow the vesting schedule as if they were locked from the beginning.
* Consequently, if the vesting has already started, any amount of tokens sent to this contract will (at least partly)
* be immediately releasable.
*
* By setting the duration to 0, one can configure this contract to behave like an asset timelock that hold tokens for
* a beneficiary until a specified time.
*/
contract VestingWallet is Context {
event EtherReleased(uint256 amount);
Expand Down Expand Up @@ -62,6 +65,13 @@ contract VestingWallet is Context {
return _duration;
}

/**
* @dev Getter for the end timestamp.
*/
function end() public view virtual returns (uint256) {
return start() + duration();
}

/**
* @dev Amount of eth already released
*/
Expand Down Expand Up @@ -136,7 +146,7 @@ contract VestingWallet is Context {
function _vestingSchedule(uint256 totalAllocation, uint64 timestamp) internal view virtual returns (uint256) {
if (timestamp < start()) {
return 0;
} else if (timestamp > start() + duration()) {
} else if (timestamp > end()) {
return totalAllocation;
} else {
return (totalAllocation * (timestamp - start())) / duration();
Expand Down
4 changes: 0 additions & 4 deletions contracts/governance/README.adoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ Votes modules determine the source of voting power, and sometimes quorum number.

* {GovernorVotes}: Extracts voting weight from an {ERC20Votes}, or since v4.5 an {ERC721Votes} token.

* {GovernorVotesComp}: Extracts voting weight from a COMP-like or {ERC20VotesComp} token.

* {GovernorVotesQuorumFraction}: Combines with `GovernorVotes` to set the quorum as a fraction of the total token supply.

Counting modules determine valid voting options.
Expand Down Expand Up @@ -66,8 +64,6 @@ NOTE: Functions of the `Governor` contract do not include access control. If you

{{GovernorVotesQuorumFraction}}

{{GovernorVotesComp}}

=== Extensions

{{GovernorTimelockControl}}
Expand Down
55 changes: 0 additions & 55 deletions contracts/governance/extensions/GovernorVotesComp.sol
View file

This file was deleted.

20 changes: 0 additions & 20 deletions contracts/mocks/governance/GovernorCompMock.sol
View file

This file was deleted.

4 changes: 2 additions & 2 deletions contracts/mocks/governance/GovernorCompatibilityBravoMock.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ pragma solidity ^0.8.0;
import "../../governance/compatibility/GovernorCompatibilityBravo.sol";
import "../../governance/extensions/GovernorTimelockCompound.sol";
import "../../governance/extensions/GovernorSettings.sol";
import "../../governance/extensions/GovernorVotesComp.sol";
import "../../governance/extensions/GovernorVotes.sol";

abstract contract GovernorCompatibilityBravoMock is
GovernorCompatibilityBravo,
GovernorSettings,
GovernorTimelockCompound,
GovernorVotesComp
GovernorVotes
{
function quorum(uint256) public pure override returns (uint256) {
return 0;
Expand Down
Loading

0 comments on commit 15c5c71

Please sign in to comment.