last minute collateral swap changes to the Vault #1653
Conversation
naddison36
requested review from
DanielVF,
shahthepro and
sparrowDom
as code owners
| // Strategies approved for use by the Vault | ||
| struct Strategy { | ||
| bool isSupported; | ||
| uint256 _deprecated; // Deprecated storage slot |
There was a problem hiding this comment.
This will affect any future changes to the struct (since any new prop added might already have values set for existing strategies). Uniswap strategy already does that. It's probably low impact but I'd prefer not to do it
There was a problem hiding this comment.
That's why we need to do this now. The Uniswap V3 Strategy will add another bool after this which means we have two bools using three storage slots.
Each read from a storage slot costs 2,100 gas
There was a problem hiding this comment.
I get your point. But how do we make sure that we don't accidentally use the old value in slot1 (set before deprecation) when we add more vars to this? Right now, having this named _depreacted acts as a reminder for us
There was a problem hiding this comment.
All the old slot1 values are set to zero which is the default value.
Here's the OETH Vault
- https://evm.storage/eth/17540186/0x39254033945aa2e4809cc2977e7087bee48bd7ab/strategies.0x1827f9ea98e0bf96550b2fc20f7233277fcd7e63#map
- https://evm.storage/eth/17540186/0x39254033945aa2e4809cc2977e7087bee48bd7ab/strategies.0x3ff8654d633d4ea0fae24c52aec73b4a20d0d0e5#map
- https://evm.storage/eth/17540186/0x39254033945aa2e4809cc2977e7087bee48bd7ab/strategies.0xc1fc9e5ec3058921ea5025d703cbe31764756319#map
Here's the OUSD Vault strategies mapping which you can drill into each mapping to check the slot1 values
https://evm.storage/eth/17540105/0xe75d77b1865ae93c7eaa3040b038d7aa7bc02f70/strategies#map
There was a problem hiding this comment.
Oh, it makes more sense to remove it then
sparrowDom
left a comment
sparrowDom
left a comment
There was a problem hiding this comment.
LGTM.
Pretty cool demonstration of how the evm storage tool helps answer the question of whether we have any dirty data in the deprecated slot.
…pported strategies storage slot layout
Codecov Report
@@ Coverage Diff @@
## nicka/collateral-swaps #1653 +/- ##
=======================================================
Coverage 69.36% 69.36%
=======================================================
Files 44 44
Lines 2350 2350
Branches 617 617
=======================================================
Hits 1630 1630
Misses 717 717
Partials 3 3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Depreciating this variable was done on December 3, 2020. The deploy script file for the relaunch of OUSD was made on Dec 20th, 2020, so it seems likely from the commit history that this depreciated variable has never been live on the current vault. The actual deploy was something like the last day of the year 2020. |
When reviewing the Uniswap V3 Strategy PR, it was clear we need to remove the
_deprecatedproperty from theStrategystruct that is used by the Vault'sstrategiesmapping.Looking at both the OETH and OUSD vault storage, there is no impact in removing this
OUSD
strategiesstorage https://evm.storage/eth/17540105/0xe75d77b1865ae93c7eaa3040b038d7aa7bc02f70/strategies#mapOETH
strategiesstoragehttps://evm.storage/eth/17540186/0x39254033945aa2e4809cc2977e7087bee48bd7ab/strategies#map