Abstract IncentivesContract logic, adapt Staked Incentives Controller and Pull Reward Incentives Controller #6
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6 +/- ##
=========================================
- Coverage 1.95% 1.80% -0.15%
=========================================
Files 9 11 +2
Lines 205 221 +16
Branches 28 29 +1
=========================================
Hits 4 4
- Misses 201 217 +16
Continue to review full report at Codecov.
|
|
Should we update the |
I think is better to not encapsulate it more deeper. |
…ogic. Adapt Stake and PullRewards contracts to use the abstracted incentives controller.
…e json and readme.md
kartojal
changed the title
…o allow users to claim rewards directly to msg.sender
| * @dev update the rewards vault address, only allowed by the Rewards admin | ||
| * @param rewardsVault The address of the rewards vault | ||
| **/ | ||
| function setRewardsVault(address rewardsVault) external onlyEmissionManager { |
There was a problem hiding this comment.
Is there a risk of updating the rewardsVault, but leaving existing token emissions? For token contract updates, would be fine, but if switching tokens could be an issue. Probably can just be handled with clear documentation but can consider include smart contract options.
Potential options
Clearly document that existing emissions will remain for the new token
Clear all emmissions on setRewardsVault
Add a parameters to setRewardsVault and include a call to update emissions
There was a problem hiding this comment.
If the EmissionManager changes the rewards vault, it will not change the current token as is inmmutable, just the source where "pull" and transfer the reward token. In my opinion emissions could still be the same and if the "reward vault" has sufficient tokens the incentives will not stop.
The rewards vault can be an EOA or smart contract with the sole purpose of pulling the reward tokens from this contract to the incentivized accounts.
Perform similar overflow check at BaseIncentivesController:58 as DistributionManager::_updateAssetStateInternal()
Changes
Reasoning
As @miguelmtzinf pointed out, the last iteration of the BaseIncentivesController had the same logic than the Staked Incentives contracts so makes sense to extract the logic into an abstract contract and them and in this way we can create new customized incentives controllers without touching the core logic or storage of the rewards distribution.