-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewards/store: Fix claimed rewards' disappearance #1678
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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` | ||
|
@@ -97,7 +97,7 @@ contract Rewards is AragonApp { | |
|
||
_transferReward(reward, rewardAmount); | ||
|
||
emit RewardClaimed(_rewardID); | ||
emit RewardClaimed(_rewardID, tx.origin); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not really sure if this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ottodevs thank you for pointing this out. I decided to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. After reading about it, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, neither one of these will behave as we want. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Quazia after testing |
||
return rewardAmount; | ||
} | ||
|
||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here: I am not really sure if this should be |
||
if (_occurrences > 1) { | ||
newReward( | ||
_description, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think keeping the comments would hurt, but on the other hand the events names seem quite intuitive, so I am "ok" with the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, a bit redundant and unnecessary. But if they're part of some convention or standard practice, it might make sense to leave them.