Skip to content

Commit

Permalink
Rewards/store: Fix claimed rewards' disappearance
Browse files Browse the repository at this point in the history
*This PR is the product of some awesome team work. @chadoh helped me
try to reproduce the issue locally; @stellarmagnet taught me how to
inspect the state of a DAO on Rinkeby; @OttoG generously spent many
of his sleeping hours going with me through the steps to publish my
solution to Rinkeby and make it work there; and @topocount debugged
an issue with claiming rewards. Thank you all so much!*

On Rinkeby, when a reward is claimed it suddenly disappears from My
Rewards, and reappears after switching tabs. Since I was unable to
reproduce this on my local environment, I investigated the root cause
of this problem and found the following facts:

F1. The recently-claimed reward disappears because it gets filtered out
from the `myRewards` object by the app state reducer.

F2. The app state reducer filters out the reward because its
`userRewardAmount` property suddenly becomes zero.

F3. The reward with `userRewardAmount` set to zero comes from the
store's `onRewardClaimed` function, which gets the reward's properties
from `getRewardById`.

F4. The `getRewardById` function gets the `userRewardAmount` property
from the `rewardAmount` return value of the contract's `getReward` function.

F5. The `rewardAmount` return value of the contract's `getReward`
function is calculated dynamically by the contract's
`_calculateRewardAmount` function.

F6. `rewardAmount` is the reward's amount multiplied by the user's
balance at disbursement date, divided by the reference token's supply
at disbursement date.

F7. In order for `rewardAmount` to be zero, either the reward's amount,
the user's balance or the token's supply have to be zero.

After determining this, I formulated the following hypothesis:

H1. `rewardAmount` is zero because the user's balance is zero, because
`_calculateRewardAmount` is receiving the wrong user; a user who
doesn't possess any of that token. This happens when the store's
`getRewardById` function doesn't receive a `userAddress` param.

My hypothesis H1 is supported by the following facts:

F8. On the UI, the reward reappears whenever tabs are switched. What
happens underneath is that the store's `onRefreshRewards` function
gets called. This function calls `getRewardById` just like
`onRewardClaimed` (see F3), but unlike it, it specifies a user
address.

F9. Another store function, `onRewardAdded`, also calls
`getRewardById` without specifying a user address, and the same
behaviour on the UI can be observed as in `onRewardClaimed`: the
reward's `rewardAmount` field is set to zero. This is less noticeable,
but can be observed by creating a reward while on the My Rewards tab.

F10. The `userRewardAmount` property of the recently-claimed reward is
not the only property that becomes zero. The same happens to
`timeClaimed`. On the contract, `timeClaimed` is a map from addresses
to integers. The contract's `getReward` function uses the calling user's
address to get the value. If the user's address didn't exist on the
map, that could explain why it ends up being zero.

F11. @topocount [solved a similar
bug](11c9b3f?diff=split#diff-44e6c6b68470709dfa38518ab1f9c198R53)
a while ago. The description he gave me of the bug matched perfectly H1.

In order to test my hypothesis, @OttoG and I published my locally
modified version of the Rewards app to Rinkeby, created a Rinkeby DAO
and installed it there. The bug went away. You can test it
[here](https://rinkeby.aragon.org/#/rewards45/0x632b710170835d55946cc717a50586267c2b6832/).

So I can say with confidence that this PR fixes #1518 :)

The only remaining mistery is why can't this be reproduced locally. I
don't know enough about the differences between Ganache and
Rinkeby (or between the local environment and other blockchain
environments in general) in order to answer that question. But I do
believe it's a relevant question that needs further inquiry in order
to determine if the best path forward with similar issues is to
somehow make the local environment more closely resemble the
blockchain, or to acknowledge the need to sometimes deploy custom
Rinkeby DAOs, just like in this case.
  • Loading branch information
e18r committed Nov 20, 2019
1 parent 7a06765 commit e1cff47
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 8 deletions.
15 changes: 11 additions & 4 deletions apps/rewards/app/store/reward.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,13 @@ const CONVERT_API_BASE = 'https://min-api.cryptocompare.com/data'
const convertApiUrl = symbols =>
`${CONVERT_API_BASE}/price?fsym=USD&tsyms=${symbols.join(',')}`

export async function onRewardAdded({ rewards = [], refTokens = [], balances = [] }, { rewardId }, settings) {
export async function onRewardAdded(
{ rewards = [], refTokens = [], balances = [] },
{ rewardId, adder },
settings
) {
if (!rewards[rewardId]) {
rewards[rewardId] = await getRewardById(rewardId)
rewards[rewardId] = await getRewardById(rewardId, adder)
const { referenceToken } = rewards[rewardId]
const response = await updateBalancesAndRefTokens({ balances, refTokens }, referenceToken, settings)
return { rewards, refTokens: response.refTokens }
Expand All @@ -21,8 +25,11 @@ export async function onRewardAdded({ rewards = [], refTokens = [], balances = [
return { rewards, refTokens }
}

export async function onRewardClaimed({ rewards = [], claims = {} }, { rewardId }) {
rewards[rewardId] = await getRewardById(rewardId)
export async function onRewardClaimed(
{ rewards = [], claims = {} },
{ rewardId, claimant }
) {
rewards[rewardId] = await getRewardById(rewardId, claimant)

let { claimsByToken = [], totalClaimsMade = 0 } = claims

Expand Down
8 changes: 4 additions & 4 deletions apps/rewards/contracts/Rewards.sol
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ contract Rewards is AragonApp {
Vault public vault;

/// Events
event RewardAdded(uint256 rewardId); /// Emitted when a new reward is created
event RewardClaimed(uint256 rewardId); /// Emitted when a reward is claimed
event RewardAdded(uint256 rewardId, address adder);
event RewardClaimed(uint256 rewardId, address claimant);

/**
* @notice Initialize Rewards app for Vault at `_vault`
Expand Down Expand Up @@ -97,7 +97,7 @@ contract Rewards is AragonApp {

_transferReward(reward, rewardAmount);

emit RewardClaimed(_rewardID);
emit RewardClaimed(_rewardID, tx.origin);
return rewardAmount;
}

Expand Down Expand Up @@ -216,7 +216,7 @@ contract Rewards is AragonApp {
reward.delay = _delay;
reward.blockStart = _startBlock;
reward.creator = msg.sender;
emit RewardAdded(rewardId);
emit RewardAdded(rewardId, tx.origin);
if (_occurrences > 1) {
newReward(
_description,
Expand Down

0 comments on commit e1cff47

Please sign in to comment.