Claiming Rewards from the Reward Pot #195

lazovicff · 2018-04-13T19:56:57Z

Closes #95

Reward payout

Storage variables:

Global reward payout counter - globalRewardPayoutCount
Active reward payouts - activeRewardPayouts
User reward payout counter mapping - userRewardPayoutCount
Reward payout cycles mapping - rewardPayoutCycles

Implemented functions for:

starting the payout cycle - startNextRewardPayout
claiming reward payout - claimRewardPayout
waiving reward - waiveRewardPayout
moving remaining funds after payout - moveRemainingRewardPayoutFunds
getting the info about payout - getRewardPayoutInfo

Starting the payout cycle

Takes one argument: token address

Saving all the necessary data to rewardPayoutCycles (current root hash, current total tokens, reward pot balance, token, current block number).
Incrementing globalRewardPayoutCount and using it for mapping key.
Setting payout token to active, which will not allow creation of new payouts with the same token until the current one finishes.

Claiming the reward payout

Takes three arguments: payout id, user reputation, colony-wide reputation

Can be called only if reward payout is active i.e if its created less than 60 days ago.
Can be called only if payout is not claimed, i.e if payoutId - userRewardPayoutCount = 1 (To force the user to claim the payout that is next in queue, otherwise there would be a lot of problems).

Calculating amount to be send to user according to formula defined in the whitepaper.

Incrementing userRewardPayoutCount.
Sending the user the calculated amount.

Waiving reward payout

Takes one argument: number of payouts to waive

Can only be called if user has not claimed that reward i.e if payoutId - userRewardPayoutCount = 1.

Incrementing userRewardPayoutCount.

Moving remaining funds after payout

Can be called only if payout is finished i.e if its created more than 60 days ago.
Setting payout token to inactive. This will allow creation of reward payout with that token.

Moves remaining funds to pot 0.

Getting the info about the payout

Takes one argument: payout id

Getting the useful information about the payout for UI.

area

Regarding the custom fork of ganache, it's better to use block.timestamp rather than the number of blocks to measure duration on chain - with hardforks, block times can change and may do so drastically when PoS arrived. I realise the whitepaper says 500k blocks, but frankly that is a holdover from us being idiots and not knowing any better. Replace this with sixty days, which I imagine is what we were aiming for in the first place.

I hope the delving you did in to ganache was educational though! It's been a while since I got grubby in its internals, but I learned a lot when I did so.

area · 2018-04-20T09:07:20Z

contracts/ColonyFunding.sol

+  }
+
+  function waiveRewardPayout(uint8 _payoutId) public {
+    require(_payoutId - userRewardPayoutCount[msg.sender] == 1);


Arguably, it should be possible for a user to waive multiple payouts at once - if they've been away for a long time, maybe there are lots for them to waive.

area · 2018-04-20T09:09:59Z

contracts/ColonyStorage.sol

@@ -45,6 +45,18 @@ contract ColonyStorage is DSAuth {
  // Pot 0 is the 'reward' pot containing funds that can be paid to holders of colony tokens in the future.
  mapping (uint256 => Pot) pots;

+  struct RewardPayoutCycle {


Document the properties of this struct.

area · 2018-04-20T09:18:49Z

contracts/ColonyFunding.sol

+    address tokenAddress = rewardPayoutCycles[_payoutId].token;
+    ERC20Extended payoutToken = ERC20Extended(tokenAddress);
+    uint256 userTokens = payoutToken.balanceOf(msg.sender);
+    uint256 totalTokens = sub(payoutToken.totalSupply(), payoutToken.balanceOf(address(this)));


The tokens used to work out the reward a user is entitled to should be the colony tokens they hold in the colony paying out the reward, not the tokens of the type being paid out that they already hold.

I hadn't considered removing the colony tokens held by the colony from consideration from this calculation. I think that this will give strictly better behaviour than what was spec'd (everyone should get slightly more rewards than they would have done before - i.e. there will be less left over and returned to the pot). Very nice!

area · 2018-04-20T09:58:00Z

The approximations with the payout function is to be expected - even if the calculation could be done precisely on-chain, the payouts could not. I suspect what you've done is accurate enough, but maybe test with very small numbers for both the tokens and the reputation is in order just to see how bad it can be.

Parallel payouts with one token is an interesting one. I like that you've taken that possibility in to account so far; given that an additional mapping would be required, and we want to encourage payouts to be fairly infrequent, I think I'm okay with disallowing it. I will check with some others and confirm.

lazovicff · 2018-04-21T10:56:30Z

Playing with ganache was useful, although the code was rusty.

Thanks for the feedback. I'm just not sure what you meant by:

even if the calculation could be done precisely on-chain, the payouts could not

area · 2018-04-23T08:46:16Z

Thanks for the feedback. I'm just not sure what you meant by:

even if the calculation could be done precisely on-chain, the payouts could not

Just that there is a smallest indivisible token unit on-chain, so even if the calculation could be done precisely and required, for example, .1 tokens to be paid out, we couldn't do that.

area · 2018-04-25T14:15:39Z

contracts/IColony.sol

+
+  /// @notice Claim the reward payout at `_payoutId`. User needs to provide his reputation and colony-wide reputation
+  /// which will be proven via Merkle proof inside this function.
+  /// Can only be called if payout is active, i.e its created less than 500k blocks ago.


This comment should talk about time, not blocks.

area · 2018-04-25T14:16:24Z

contracts/IColony.sol

+  function getRewardPayoutInfo(uint8 _payoutId) public view returns (bytes32, uint256, uint256, address, uint256);
+
+  /// @notice Moves remaining (unclaimed) tokens back to reward pot
+  /// Can only be called when reward payout cycle is finished i.e its created more than 500k blocks ago


Again, 60 days

area · 2018-04-25T14:20:26Z

contracts/ColonyFunding.sol

+  }
+
+  function claimRewardPayout(uint8 _payoutId, int256 _userReputation, int256 _totalReputation) public {
+    require(block.timestamp - rewardPayoutCycles[_payoutId].blockTimestamp < 5184000000); // 60 days


block.timestamp is in seconds, not milliseconds. So this is actually 60000 days! While technically you won't need to change the tests (their very large numbers will still take us past the reward payout window once this is fixed), I'd prefer it if they were changed too.

Sorry, i should have checked the documentation.

area · 2018-04-25T14:26:48Z

contracts/ColonyFunding.sol

+
+    require(userTokens > 0 && totalTokens > 0 && _userReputation > 0 && _totalReputation > 0);
+
+    uint256 diff = 76 - SafeMath.numDigits(totalTokens * uint256(_totalReputation));


uint256 can hold up to a 78 digit number in base 10, so this can underflow to a very large value, which I'm not terribly happy with. I think the worse case scenario there is that reward ends up being zero (i.e. someone gets less than they should, rather than more than they should), but users would be pretty annoyed about that. Did this implementation come from anywhere in particular? Why did you choose 76?

I thought it was 77 (idiot me), and 1 will be added in SafeMath.percent. Which is not good, anyway.
About the underflows and overflows i was not so sure, thats why i didn't added the tests for more extreme values.
We can can continue this on Gitter, i just realized a few more things.

area · 2018-04-25T14:34:49Z

test/colony-funding.js

+        userReputation: toBN(Math.floor(Math.random() * 1e16)),
+        delta: 100
+      }
+    ];


I like these tests! Can we have more of them, for even more extreme values? 😃

Sure, i just deleted them because i was getting 0 reward (As you mentioned above)

area · 2018-04-25T14:46:51Z

contracts/ColonyFunding.sol

+    uint256 diff = 76 - SafeMath.numDigits(totalTokens * uint256(_totalReputation));
+    uint256 factor = (diff % 2 == 0) ? diff : sub(diff, 1);
+    uint256 sharePercentage = SafeMath.sqrt(SafeMath.percent(userTokens * uint256(_userReputation), totalTokens * uint256(_totalReputation), factor));
+    uint256 reward = rewardPayoutCycles[_payoutId].amount * sharePercentage / (10 ** (factor / 2));


It also feels like I would be able to cook up an overflow here.

area · 2018-04-25T14:47:45Z

test/colony-funding.js

+
+        const rewardPayoutInfoAfterPayout = await newColony.getRewardPayoutInfo(payoutId);
+        const remaining = rewardPayoutInfoAfterPayout[2];
+


Can we print out how close we were explicitly for these tests?

lazovicff · 2018-05-01T14:04:46Z

@area Can you take a look at this?

elenadimitrova · 2018-05-01T14:50:49Z

test/colony-funding.js

+      await fundColonyWithTokens(colony, newToken, initialFunding.toString());
+
+      const payoutId1 = await colony.startNextRewardPayout.call(newToken.address);
+      await colony.startNextRewardPayout(newToken.address);


I'd argue to not return anything out of that function and instead get the payoutId via emitting an event out of the transaction itself. See https://github.com/JoinColony/colonyNetwork/blob/develop/test/colony-network-auction.js#L92

area · 2018-05-01T15:01:13Z

I like the choice of implementation 3 from the repository we had on the side; I think that implementation has the properties I'm happiest with. Two things about the tests are giving me pause though:

I like tests to be as deterministic as possible - no randomness! I also don't think we need this many; the important regimes to test are very small numbers (for accuracy) and very large ones (for overflow); I would expect to see tests using numbers like 1, 10, 1e76, 1e77 and not much in between.
The amount the solidity implementation is wrong by appears to be positive, which is definitely bad - if true, it'd mean that the last person to claim wouldn't be able to claim what was owed to them. That wasn't the case in the other repository, though, so something a little odd is going on here.

Finally, gas costs for this implementation are higher than I was hoping for. A way to reduce them is to have the user supply the values of the sqrt, which are then verified by squaring the passed claimed square root and checking that it is less than the original value. This calculation can be done automatically by our client, so it won't be more onerous for the user, and should reduce gas costs drastically.

lazovicff · 2018-05-01T15:31:49Z

Im ok with that.
I think you are wrong, otherwise the tests would fail. I explicitly added sqrt(denomerator) + 1 here in effort to avoid that. It seems like its doing the job, unless you know a case when it wont work.

Thanks for the hint about gas costs

** Update. Sorry, i see now that absolute wrong is in +. Probably calculation is not done right in tests, but there is enough for everybody

elenadimitrova · 2018-05-02T07:03:49Z

contracts/IColony.sol

+  /// @param _token Addess of the token used for reward payout
+  function startNextRewardPayout(address _token) public returns (uint256);
+
+  /// @notice Claim the reward payout at `_payoutId`. User needs to provide his reputation and colony-wide reputation


Just some notes on comments language:

Pls use the non-gender specific their instead of his or hers. There may be women interacting with this contract you know :)

elenadimitrova · 2018-05-02T07:07:54Z

contracts/ColonyStorage.sol

+  mapping (address => bool) activeRewardPayouts;
+  uint8 globalRewardPayoutCount;
+  mapping (address => uint256) userRewardPayoutCount;
+


Comment mappings, arrays, global variables, everything really that is storage. As you see all other items, not just structs in ColonyStorage are commented.
Our current stand on documentation is that IColony and IColonyNetwork are the two interfaces we document plus the two respective storage contracts: ColonyStorage and ColonyNetworkStorage
In addition any other non-EtherRouter contracts (not covered by the above) are documented e.g. ReputationMiningCycle

elenadimitrova · 2018-05-02T07:08:30Z

contracts/IColony.sol

+  function getRewardPayoutInfo(uint8 _payoutId) public view returns (bytes32, uint256, uint256, uint256, address, uint256);
+
+  /// @notice Moves remaining (unclaimed) tokens back to reward pot
+  /// Can only be called when reward payout cycle is finished i.e when 60 days passed from its creation


when 60 days passed -> when 60 days have passed

elenadimitrova · 2018-05-02T07:09:46Z

contracts/IColony.sol

+  /// @param _totalReputation Total reputation at the point of creation of reward payout cycle
+  function claimRewardPayout(uint8 _payoutId, int256 _userReputation, int256 _totalReputation) public;
+
+  /// @notice Waive reward payouts. This will unlock the senders tokens and increment users reward payout counter,


senders tokens -> sender's tokens

elenadimitrova · 2018-05-02T07:15:19Z

contracts/IColony.sol

@@ -275,7 +275,49 @@ contract IColony {
  /// @param _role Id of the role, as defined in `ColonyStorage` `MANAGER`, `EVALUATOR` and `WORKER` constants
  /// @param _token Address of the token, `0x0` value indicates Ether
  function claimPayout(uint256 _id, uint256 _role, address _token) public;
-
+
+  /// @notice Start next reward payout in `_token`. All funds in that are in reward pot at the time will become unavailable.


"in token" implies in some Token contract, pls replace with "for" or just say "Start next _token reward payout"

Also here this reads weird

All funds in that are in reward pot at the time

Did you mean: "All funds in the reward pot will become unavailable"

elenadimitrova · 2018-05-02T09:35:02Z

contracts/ColonyFunding.sol

+    require(_userReputation > 0, "colony-reward-payout-invalid-user-reputation");
+
+    uint256 numerator = mul(SafeMath.sqrt(uint256(_userReputation)), SafeMath.sqrt(userTokens));
+    uint256 denomerator = mul(SafeMath.sqrt(uint256(_totalReputation)), SafeMath.sqrt(payout.totalTokens));


I don't know what denomerator is in English did you mean denominator?

lazovicff changed the title ~~Feature/95 claiming rewards~~ Claiming Rewards from the Reward Pot Apr 13, 2018

elenadimitrova assigned area Apr 14, 2018

elenadimitrova requested a review from area April 14, 2018 19:06

area requested changes Apr 20, 2018

View reviewed changes

area requested changes Apr 25, 2018

View reviewed changes

elenadimitrova reviewed May 1, 2018

View reviewed changes

elenadimitrova reviewed May 2, 2018

View reviewed changes

Filip Lazovic added 2 commits May 8, 2018 16:05

Added set of functions for reward payout

0c29273

Added gas costs for reward payout cycle

4d7ba95

area approved these changes May 8, 2018

View reviewed changes

area merged commit 1dac4bd into JoinColony:develop May 8, 2018

elenadimitrova assigned lazovicff and unassigned area May 10, 2018

kronosapiens mentioned this pull request Dec 24, 2018

Denote durations in time rather than blocks JoinColony/Colony-Whitepaper#13

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Claiming Rewards from the Reward Pot #195

Claiming Rewards from the Reward Pot #195

lazovicff commented Apr 13, 2018 •

edited

Loading

area left a comment

area Apr 20, 2018

area Apr 20, 2018

area Apr 20, 2018

area commented Apr 20, 2018

lazovicff commented Apr 21, 2018

area commented Apr 23, 2018

area Apr 25, 2018

area Apr 25, 2018

area Apr 25, 2018

lazovicff Apr 25, 2018

area Apr 25, 2018

lazovicff Apr 25, 2018

area Apr 25, 2018

lazovicff Apr 25, 2018

area Apr 25, 2018

area Apr 25, 2018

lazovicff commented May 1, 2018

elenadimitrova May 1, 2018

area commented May 1, 2018

lazovicff commented May 1, 2018 •

edited

Loading

elenadimitrova May 2, 2018

elenadimitrova May 2, 2018

elenadimitrova May 2, 2018

elenadimitrova May 2, 2018

elenadimitrova May 2, 2018

elenadimitrova May 2, 2018 •

edited

Loading

elenadimitrova May 2, 2018


		require(userTokens > 0 && totalTokens > 0 && _userReputation > 0 && _totalReputation > 0);

		uint256 diff = 76 - SafeMath.numDigits(totalTokens * uint256(_totalReputation));


		const rewardPayoutInfoAfterPayout = await newColony.getRewardPayoutInfo(payoutId);
		const remaining = rewardPayoutInfoAfterPayout[2];

Claiming Rewards from the Reward Pot #195

Claiming Rewards from the Reward Pot #195

Conversation

lazovicff commented Apr 13, 2018 • edited Loading

Reward payout

Starting the payout cycle

Claiming the reward payout

Waiving reward payout

Moving remaining funds after payout

Getting the info about the payout

area left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

area commented Apr 20, 2018

lazovicff commented Apr 21, 2018

area commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazovicff commented May 1, 2018

Choose a reason for hiding this comment

area commented May 1, 2018

lazovicff commented May 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elenadimitrova May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazovicff commented Apr 13, 2018 •

edited

Loading

lazovicff commented May 1, 2018 •

edited

Loading

elenadimitrova May 2, 2018 •

edited

Loading