Add support for Strategist to harvest rewards#2853
Conversation
shahthepro
requested review from
clement-ux,
naddison36 and
sparrowDom
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2853 +/- ##
==========================================
+ Coverage 49.10% 53.26% +4.16%
==========================================
Files 112 112
Lines 4835 4844 +9
Branches 1338 1343 +5
==========================================
+ Hits 2374 2580 +206
+ Misses 2457 2259 -198
- Partials 4 5 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shahthepro
changed the title
| * remain in the strategy until the harvester is unpaused or reset. | ||
| */ | ||
| function collectRewardTokens() external virtual onlyHarvester nonReentrant { | ||
| if (harvesterAddress == address(0) || harvesterPaused) { |
There was a problem hiding this comment.
this if statement is duplicated in _collectRewardTokens
There was a problem hiding this comment.
Yep, that's intentional and have left a comment about it as well. It's because some strategies inherit the internal method, while other inherit the public method. If we do it in one place and forget to do this in the inherited strategies, the rewards could end up going to address(0). Right now, it won't happen because only harvester can call the rewards. It might not be the case going forward.
sparrowDom
left a comment
sparrowDom
left a comment
There was a problem hiding this comment.
Left some comments, otherwise looks good
| * @notice Collect accumulated reward token and send to Vault. | ||
| * No-ops when the harvester address is not set. | ||
| */ | ||
| function collectRewardTokens() external virtual onlyHarvester nonReentrant { |
There was a problem hiding this comment.
The onlyHarvester modifier was renamed to onlyHarvesterOrStrategist (line 165) but this function still references the old name. This breaks compilation:
DeclarationError: Identifier not found or not unique.
--> contracts/utils/InitializableAbstractStrategy.sol:124:53
Should be onlyHarvesterOrStrategist here too.
…gist modifier Align test expectations with master's PR #2853 which allows strategist to call collectRewardTokens.
4 participants
Code Change
Right now, only
harvesterAddrcan call thecollectRewardTokenson the strategies. This changeset will also allow the Strategist/Guardian to call it. It allows Harvester address to be different from the caller. It also enables the 2/8 to change the Harvester address.The harvesterAddress zero-check is done in both internal and external function. It's because some strategies inherit the internal method, while other inherit the public method. If we do it in one place and forget to do this in the inherited strategies, the rewards could end up going to address(0). Right now, it won't happen because only harvester can call the rewards. It might not be the case going forward.