Simple Harvester V2 for SSV strategies. #2363
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2363 +/- ##
==========================================
+ Coverage 50.80% 51.53% +0.72%
==========================================
Files 92 92
Lines 4513 4527 +14
Branches 1195 1200 +5
==========================================
+ Hits 2293 2333 +40
+ Misses 2217 2191 -26
Partials 3 3 ☔ View full report in Codecov by Sentry. |
clement-ux
requested review from
sparrowDom,
DanielVF and
shahthepro
as code owners
Updated OETH process diagram with new Harvester
| //////////////////////////////////////////////////// | ||
| /// --- STORAGE | ||
| //////////////////////////////////////////////////// | ||
| /// @notice Gap for upgrade safety | ||
| uint256[50] private ___gap; |
There was a problem hiding this comment.
Usually we place the __gap after the local properties (after supportedStrategies mapping) and make it so that local variables + the gap make up a round number. In this case since the dripper takes 1 slot and supportedStrategies another one, uint256[48] private ___gap; would be better.
There was a problem hiding this comment.
I thought we were using gaps to prevent reusing slots that have already been allocated in the previous implementation. That’s why I placed it at the front.
In the PR description, I included a picture showing that 7 slots are already in use. Therefore, I added a gap before introducing new storage variables to avoid any conflicts.
I’m not an expert on proxies and may not be as familiar with this as some of you. Please let me know if this doesn’t make sense!
If it does, should I consider using 41 or 43 for the gap to reach a round number?
There was a problem hiding this comment.
Yes, its important to line up the slots so they are backward compatible with the exiting OETHHarvester
There was a problem hiding this comment.
This is what the slots in the current OETHHarvesterProxy contract look like
And here's the slots for the new OETHHarvesterSimple contract
There was a problem hiding this comment.
ok, after talking through this one. Let's not upgrade the exiting OETHHarvesterProxy. Instead we'll do what the current scripts do which is deploy and new OETHHarvesterSimpleProxy and OETHHarvesterSimple implementation. All the strategies can then be changed to use the new harvester
There was a problem hiding this comment.
Sorry for the confusion.
I’ve changed the gap position and value accordingly to @sparrowDom comment: 5e7f623.
| .connect(strategist)["harvestAndTransfer(address)"](nativeStakingSSVStrategy.address); | ||
|
|
||
| const balanceAfterWETH = await weth.balanceOf(oethFixedRateDripper.address); | ||
| expect(balanceAfterWETH).to.be.gt(balanceBeforeWETH); |
There was a problem hiding this comment.
If 1 ETH is sent to the fee accumulator the balanceAfterWETH should be at least:
to.be.gte(balanceBeforeWETH.add(oethUnits("1")))
There was a problem hiding this comment.
Indeed! Changed in this commit: bededf7
| .connect(timelock)["harvestAndTransfer(address)"](nativeStakingSSVStrategy.address); | ||
|
|
||
| const balanceAfterWETH = await weth.balanceOf(oethFixedRateDripper.address); | ||
| expect(balanceAfterWETH).to.be.gt(balanceBeforeWETH); |
There was a problem hiding this comment.
similar comment here as a little bit above
There was a problem hiding this comment.
Changed in this commit: bededf7
Contracts
Governance
Code Change Checklist
To be completed before internal review begins:
Internal review: