Reward calculation issue when the same player update in a row

[sydneyitguy]

When the same user updates the pixels twice, the reward doesn't get accumulated.

[assafom]

It seems that this happens also when a user update twice, not just in a row.
For example: Alice updates 2 pixels, Bob updates 2 pixels, Alice updates 2 pixels - reward will be lost.
See here for a simple test scenario that shows this by printing Dixel's balance.
But if Alice updates, then Bob updates, then Carol updates - then the calculation is ok.

So it doesn't need to be twice in a row.

I'll try looking at it further if you won't fix it by the time I'm able to.

[assafom]

OK, I was too curious and continued to look at it 🙂 I believe the problem is that the code doesn't take into account that for example in this scenario:

• Alice updated
• Bob updated
• Alice updated

Of the reward that was added in the last step, part of it belongs to Alice because of her first deposit. We are neglecting that part when we set the rewardDebt at the end.

To fix, we need to calculate how much reward Alice earned from her new reward - playerEarned :

            uint256 playerEarned;
            // Update acc values before updating contributions so players don't get rewards for their own penalties
            if (totalContribution != 0) { // The first reward will be permanently locked on the contract
                _increaseRewardPerContribution(reward);
                playerEarned = player.contribution * reward / totalContribution;
            }

(Note that at this point player.contribution still doesn't contain the new contribution)

And then substract it from rewardDebt so the user can claim it.

player.rewardDebt = _totalPlayerRewardSoFar(player.contribution) - pendingReward - playerEarned;

Perhaps more testing is needed, but this fixes the issue based on a quick test that I did.

[assafom]

And if you don't want that Alice's second update will reward Alice's contribution from first update, I believe this is the change that is needed:

Update _increaseRewardPerContribution to also take the totalContribution/shares as parameter:

function _increaseRewardPerContribution(uint256 rewardAdded, uint256 totalShares) private {
        unchecked {
            accRewardPerContribution += (1e18 * rewardAdded) / totalShares;
        }
    }

After updating the increase of shares reward and calculating how much of it belongs to the updating user, we use _increaseRewardPerContribution to add that reward to the rest of the users:

            uint256 userEarned;
            // Update acc values before updating contributions so players don't get rewards for their own penalties
            if (totalContribution != 0) { // The first reward will be permanently locked on the contract
                _increaseRewardPerContribution(reward, totalContribution);
                userEarned = player.contribution * reward / totalContribution;
                _increaseRewardPerContribution(userEarned, totalContribution-player.contribution);
            }

And we keep rewardDebt the same - so it already takes userEarned into account and the user can't claim it.

player.rewardDebt = _totalPlayerRewardSoFar(player.contribution) - pendingReward; 

[sydneyitguy]

Oh Alice's second update should reward Alice's first update. I just tried to prevent rewarding from the current update.

For example,

1. Alice creates 10 reward at the beginning -> No one is prior to Alice, so reward is just locked on the contract forever
2. Alice creates another 20 reward -> Alice gets the 20 reward from her first update
3. Bob creates another 40 reward -> Alice gets all 40 because Bob shouldn't be rewarded by his own contribution
4. Bob creates another 30 reward -> So far, Alice contribution: 2 / Bob contribution: 1 -> Alice gets 20 / Bob gets 10
5. Carol creates 40 reward -> Alice contribution: 2 / Bob contribution: 2 -> Alice gets 20 / Bob gets 20 / Carol gets nothing because she doesn't have any previous updates

Assumption: All event increases the player's contribution by 1 (1 pixel update)

[assafom]

Alright, cool. So this first fix that I suggested will fix it.

I have created test script for the scenario you just described.
See here. Seems to be working with the fix. You can add the test to Dixel.test.js.

[Note: there's some repetition in the test, maybe you'd like to extract some of it to a different function to remove the repetition.
I tried to do it but honestly I haven't used JS in a while, so so far I didn't manage to get the right scoping to be able to access all the variables.
Anyway this way is not a disaster and you can see what's happening.]

[sydneyitguy]

Thank you so much for your help on this issue @assafom 👍🏻

It looks fine now, but please let me know if there's more edge cases you can think of.