feat: add a Governance Rewards Splitter contract

[zugdev]

Resolves #831

I think GovernanceRewardsSplitter or TreasurySplitter works for the contract's name.

[ubiquity-os-deployer]

addf9eb

684ab94

807b3e4

[zugdev]

@rndquu please evaluate the configuration mechanism.

Currently, since the number of shares becomes imbalanced as a new payee is added, I have payees and shares tied to a mapping which is a configuration ID. Owner can always set a new configuration which increments the current global configuration ID. Depending on array size, it is probably cheaper to create a new array than clearing the entire configuration array, we can delete old arrays if total storage is important.

In this implementation, owner can either set a whole new configuration or add a single payee. I can add functions to edit and delete a payee as well. There should be a pointer approach to it but I imagine you want to stay away from assembly.

[rndquu]

@zugdev

I think GovernanceRewardsSplitter or TreasurySplitter works for the contract's name.

Current naming (GovernanceRewardsSplitter) looks good.

I can add functions to edit and delete a payee as well.

Yes, we definitely need methods for adding, editing and removing payess.

stay away from assembly

Yes.

mapping which is a configuration ID

Ok, the concept of "configurations" appeared because we somehow need to modify arrays of payees and shares. The issue with the current approach is that payee may not be able to claim Governance tokens if currentConfig has been incremented and payee removed:

1. Payee is added by the owner (currentConfig=0)
2. Payee is eligible for 100 Governance tokens
3. Owner calls setNewConfig and removes payee from the payees array
4. Now payee calls release and gets a revert. Besides releasable returns 0 for the payee's address since here _configToShares[currentConfig][account]=0 because currentConfig has been incremented + payee's address has been removed.

To sum up:

1. You sure there's no audited solution for distributing ERC20 tokens?
2. I think it makes sense to keep a single list of payees and shares because it's more explicit. Usually EnumerableSet or EnumerableMap is used for such kind of tasks.
3. Code style: there're no _ prefixes or suffixes in the whole code base. So shares_ => shares, _configToPayees => configToPayees, etc...
4. As far as I understand the PR is not ready yet so there are no unit tests hence this CI is failing

[rndquu]

PaymentSplitter already has almost all we need for distributing ERC20 rewards. We need to:

1. Remove ETH related methods from that contract
2. Add logic for adding/editing/removing payees and shares via EnumerableSet or EnumerableMap

Update: one more https://github.com/immutable/contracts/blob/main/contracts/payment-splitter/PaymentSplitter.sol (not sure if it's audited, need to dig into https://github.com/immutable/contracts/tree/main/audits)

[zugdev]

@rndquu

Ok, the concept of "configurations" appeared because we somehow need to modify arrays of payees and shares. The issue with the current approach is that payee may not be able to claim Governance tokens if currentConfig has been incremented and payee removed:

Yes this is actually intented in this initial approach, the reason is, if config is set poorly you can change config first and then appropriately release with new config. This only assumes contract owner behaves correctly.

4. As far as I understand the PR is not ready yet so there are no unit tests hence this CI is failing

Yes I wanted you to take a look at the contract before testing it.

PaymentSplitter already has almost all we need for distributing ERC20 rewards.

Notice that this is a direct fork of PaymentSplitter where I've removed ETH handling capabilities and made it only able to handle governance tokens. The only thing that has changed is arrays became mappings from config to array. The reason I took the mapping approach is that handling long arrays (deleting , moving) would incur in extra gas costs specially if the array was very big. Therefore just adding new arrays would actually be gas efficient. I will take a look at Enumerable approaches though, I've used it a couple times as well.

[zugdev]

@rndquu, I've considered EnumerableMap but I still think the native mapping approach is more efficient. The biggest gas cost in this contract happens when you insert/remove a payee, since all shares array becomes imbalanced. For example, if there are two payees, one with 70% (7 shares) and the other with 30% (3 shares), adding a third payee with 1 share will change the distribution for all payees ~(63%, 27%, 9%). Therefore just creating new arrays and forgetting the previous one when you setup a new config is definitely better than iterating through the list of payees and delete every key manually.

EnumerableSet doesn't make sense to use since it's biggest offering is O(1) existence check which is not that useful in this case. We need O(1) insertion/deletion and O(n) iteration, this is achieved by both mapping and EnumerableMap, the difference is that in my mappings approach we just forget the old payees and shares and create new arrays when we set new ones instead of modifying the old ones. It's your final call and I'll add an EnumerableMap version here for you to take a look.

[zugdev]

Please let me know what is your preferred approach, if you want more strict arguments in favor of using native mappings please let me know, I can further elaborate why it should be more efficient.

[zugdev]

Underflow is not possible without assembly or unchecked blocks in Solidity ^0.8.0.

[ubiquity-os]

@zugdev, this task has been idle for a while. Please provide an update.

[zugdev]

waiting on PR review

[ubiquity-os]

@zugdev, this task has been idle for a while. Please provide an update.