This repository was archived by the owner on Jun 15, 2021. It is now read-only.
Scalable dividends - #38
Merged
Merged
Conversation
added 3 commits
August 28, 2019 18:03
Collaborator
|
Looks great. I had one question on the events: I was thinking it might be nice to keep the 'from' value in the DistributionMade event, to track where all the payments come from. Any thoughts on this? |
Contributor
Author
|
@tspoff I've added back the |
orishim
approved these changes
Sep 2, 2019
dOrgJelli
pushed a commit
that referenced
this pull request
Sep 7, 2019
* payment tests * buy tests * bc factory test fix * sell tests * test cleanup * bancor deploy cost reduction * Merkle payments import (#30) * add daostack test / imports * add babel * add merkle payment pool * return config to use normal ganache port * Zos -> OpenZeppelinSDK update (#33) * remove DAOStack integration tests * zos -> oz update * test update * Deploy script (#34) * add deploy script * update scripts * Coverage (#35) * update openzep dependencies * convert tests to use web3 contracts * test refactor add splitOnPay variant tests * update gitignore * add solcover settings * Add setup settings for coverage network * Change default tx parameters to normal networks * update project.json with new oz package name * update oz dependencies * Factory param change (#39) * separate deploy parameters * change oz manager * add more admin functions, test updates * test updates * Scalable dividends (#38) * add RewardsDistributor * wrapper, first unit tests * fix tests * more tests * better tests * bring back sender address for distribute events * comments * (spelling) reminder -> remainder
Closed
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Implements scalable pull-based dividends as an internal "library", based on:
[1] http://batog.info/papers/scalable-reward-distribution.pdf
[2] https://solmaz.io/2019/02/24/scalable-reward-changing
Scope
Add a private "accounting" interface that is currently not linked to any ERC20 or eth transfers. Note that both the staked token and the awarded (dividend) token can be any of an ERC20 or eth. This integration will be added in a separate PR.
Eligible Unit
If every staked
weiwere to count towards accruing dividends, then no distribution would be possible with amounts smaller than the total stake (if there are two stakers, one with1 ethand one with1 weithen any amount <1 eth + 1 weican not be correctly allocated). Given that in most applications there will be a need to distribute smaller amounts than the total stake then we're introducing an "eligible unit" as is the smallest amount of stake that "matters" for accruing dividends.This value defaults now to
10**9which is half the orders of magnitude a typical ERC20 contract would use as decimals. I find this a good compromise between smaller "wasted" reminders of individual deposits (say ELIGIBLE_UNIT=1 eth, anyone depositing smaller amounts would not receive any dividends) and ability to distribute smaller amounts (like in above example).This default should be revisited in case the staked and the dividend token have largely different circulating supplies. Actually, it depends more on the expected volumes of stake and dividends tokens in the contract, but probably the total circulating supply is a good enough indicator.
Distribution
Accepts any amount as an argument to
_distributeeven if not all amounts could possibly be distributed (if called with1 weithere's no way it could be allocated if total stake, in eligible units, is greater than 1). Smaller amounts that can not immediately be distributed are accumulated until distribution is possible, at a future_distributecall.Currently calls to
_distributeare reverted if there is no stake. Alternatively it could accumulate those amounts until at least one user stakes in.Withdrawal
Stake and dividend can be withdrawn independently.
Gas usage
Currently this implementation stores 3
int256s per address. One of them simply stores stake modulo ELIGIBLE_UNIT and could easily be eliminated at the expense of extra computation per call. Not sure which one is more gas efficient. Also, if this current version will be embedded into an ERC20, the total storage would be of 4int256per address. It could be reduced to only 2, ifbalanceOf,_stakeand_stakeReminderare unified. I leave that as a future improvement.Tests
Contract: RewardsDistributorWrapper
✓ deploys and initializes
✓ withdraws ZERO reward (108ms)
✓ reads ZERO stake (68ms)
✓ reverts if trying to withdraw amount > stake (74ms)
✓ updates total stake after deposit > ELIGIBLE_UNIT (163ms)
✓ doesn't update total stake after deposit < ELIGIBLE_UNIT (92ms)
✓ deposits A, gets stake, withdraws all stake, gets stake again (199ms)
✓ does no distribution if no stake >= ELIGIBLE_UNIT (106ms)
✓ does no distribution if stake becomes ineligible after withdrawl (352ms)
✓ allocates all reward to a single staker and allow its withdrawl (242ms)
✓ allocates no reward to stake <= ELIGIBLE_UNIT (250ms)
✓ allocates 1st reward proportionally to 2 stakers and 2nd reward to remaining staker after the other withdrew (362ms)
✓ withdraws reward after proportional reward distribution (251ms)
✓ withdraws reward after two consecutive reward distributions (462ms)
✓ distributes after partial stake withdrawal and reads reward (481ms)
✓ withdraws reward after stake has been withdrawn (308ms)
✓ handles magnitude: A deposits 9999, B deposits 1, distribute, withdraw stake, distribute, withdraw reward (333ms)
✓ handles magnitude: deposit 106 109 1012 1015, distribute, withdraw reward (421ms)
✓ carries reminder to second distribution and withdraws reward (491ms)
19 passing (8s)