feat: add rev share upgrader#53
Conversation
0xDiscotech
left a comment
There was a problem hiding this comment.
Good job so far! Logic for upgrade seems solid
Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com> Signed-off-by: Chiin <77933451+0xChin@users.noreply.github.com>
Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com> Signed-off-by: Chiin <77933451+0xChin@users.noreply.github.com>
|
@0xChin don't forget to remove everything related to the outdated version we are descoping, e.g. the |
c8793a5 to
c02d294
Compare
Co-authored-by: Disco <131301107+0xDiscotech@users.noreply.github.com> Signed-off-by: Chiin <77933451+0xChin@users.noreply.github.com>
| function _upgradeVaultsWithRevShareConfig(address _portal) private { | ||
| address[4] memory vaultProxies = [ | ||
| RevShareLibrary.OPERATOR_FEE_VAULT, | ||
| RevShareLibrary.SEQUENCER_FEE_WALLET, | ||
| RevShareLibrary.BASE_FEE_VAULT, | ||
| RevShareLibrary.L1_FEE_VAULT | ||
| ]; | ||
| bytes[4] memory creationCodes = [ | ||
| RevShareLibrary.operatorFeeVaultCreationCode, | ||
| RevShareLibrary.sequencerFeeVaultCreationCode, | ||
| RevShareLibrary.defaultFeeVaultCreationCode, | ||
| RevShareLibrary.defaultFeeVaultCreationCode | ||
| ]; | ||
| string[4] memory vaultNames = ["OperatorFeeVault", "SequencerFeeVault", "BaseFeeVault", "L1FeeVault"]; | ||
|
|
||
| address defaultImpl; | ||
| for (uint256 i; i < vaultProxies.length; i++) { | ||
| bytes32 salt = _getSalt(vaultNames[i]); | ||
| address impl; | ||
|
|
||
| // Check if this is the BaseFeeVault or L1FeeVault (both use default implementation) | ||
| bool isBaseFeeVault = keccak256(bytes(vaultNames[i])) == keccak256(bytes("BaseFeeVault")); | ||
| bool isL1FeeVault = keccak256(bytes(vaultNames[i])) == keccak256(bytes("L1FeeVault")); | ||
|
|
||
| if (isBaseFeeVault) { | ||
| // Deploy default implementation for BaseFeeVault | ||
| impl = Utils.getCreate2Address(salt, creationCodes[i], RevShareLibrary.CREATE2_DEPLOYER); | ||
| defaultImpl = impl; | ||
| _depositCreate2(_portal, RevShareLibrary.FEE_VAULTS_DEPLOYMENT_GAS_LIMIT, salt, creationCodes[i]); | ||
| } else if (isL1FeeVault) { | ||
| // Reuse the default implementation for L1FeeVault | ||
| impl = defaultImpl; | ||
| } else { | ||
| // Deploy specific implementations for OperatorFeeVault and SequencerFeeVault | ||
| impl = Utils.getCreate2Address(salt, creationCodes[i], RevShareLibrary.CREATE2_DEPLOYER); | ||
| _depositCreate2(_portal, RevShareLibrary.FEE_VAULTS_DEPLOYMENT_GAS_LIMIT, salt, creationCodes[i]); | ||
| } |
There was a problem hiding this comment.
The readibility here got a bit worse because we decided to only deploy one implementation FeeVault for both BaseFeeVault and L1FeeVault given they use the exact same bytecode.
On the previous way, the loop was way cleaner but it required an extra deployment, something that is not trivial given that the accumulation of deposits on the block are expensive in terms on gas, and we have a limit there (usually 20m gas).
There was a problem hiding this comment.
Curious about wdyt about this @funkornaut001 @0xng
There was a problem hiding this comment.
It makes sense to me as is. Previously, with the four deployments, were you approaching the block gas limit?
There was a problem hiding this comment.
We were deploying four vaults, one per each, even though BaseFeeVault and L1FeeVault share the same one
There was a problem hiding this comment.
It's ok, but it's incohesive. Not the code itself, but the practice. I find two things off about this approach:
- The implementation is being deployed with the salt corresponding to the
BaseFeeVault. This is awkward for the "L1FeeVault" in terms of following the practices. There's no real reason why it uses the salt of that vault over the other. - The only reason this works is because they share the same semver version constant. Otherwise, their bytecodes would differ.
Does any of these two points matter in practice? Not really. Point 2 could be an "issue" if this contract was meant to be reusable as the contracts evolved, because the bytecodes could technically differ at some point. But that's not the case, so fair game.
…ethereum-optimism#1257) * feat: add Revenue Share Upgrade Path (#8) * feat: Deploy FeesDepositor (#15) * feat: late opt-in template (#21) * refactor: contracts update creationcode (#23) * fix: sync (#24) * test: add require tests for fields on Revenue Sharing templates (#26) * fix: L2 target ProxyAdmin (#27) * chore: update cost of upgrades and deployments (#29) * test: revenue sharing upgrade unit tests (#22) * test: add basic validation for fees depositor template (#32) * chore: updates the bytecode for vaults, using initialize (#31) * test: integration supersim revshare (#30) * fix: broken simulations * fix: wrong location of synced regression tests * test: integration revshare late opt in (#33) * fix: ir fixes (#38) * chore: gas costs (#39) * fix: minors (#46) * fix: remove vm usage for getCreate2Address utils function (#48) * feat: descope late opt in (#51) * feat: add rev share upgrader (#53) * fix: bash on comment * fix: forge fmt * refactor: reduce code size by splitting logic between lib and contract (#59) * refactor: reduce code size by splitting logic between lib and contract * chore: remove comment * chore: remove prefix * refactor: split library (#60) * refactor: reduce code size by splitting logic between lib and contract * chore: remove comment * chore: remove prefix * refactor: split library in two to avoid code size error * feat: regression tests (#61) * fix: relay typo * fix: improve template validation checks (#62) * fix: wrong fee splitter bytecode (#63) * fix: skip comment * fix: rename test create two deployer * chore: add commit * feat: add upgrader deployment script (#64) * chore: update gas limits * refactor: use create two to get a deterministic deployment of the upgrader (#68) * chore: add todos (#66) * chore: remove tenderly simulations (#65) * chore: deploy and update new contract (#67) * fix: forge fmt (#69) * chore: fuzz init code on get create2 test (#70) * feat: deploy revshare mainnet (#71) * fix: forge fmt (#72) * chore: add todo comments (#73) * chore: reduce tenderly gas Co-authored-by: Maurelian <john@oplabs.co> * fix: reduce tenderly gas limit on sepolia (#77) --------- Co-authored-by: IamFlux <175354924+0xiamflux@users.noreply.github.com> Co-authored-by: Chiin <77933451+0xChin@users.noreply.github.com> Co-authored-by: 0xchin <alanracciatti1220@gmail.com> Co-authored-by: Maurelian <john@oplabs.co>
This PR contains the
RevShareContractsUpgradercontract with the on-chain logic for the upgrade, which involves:FeeVaults are upgraded,FeeSplitterdeployed and revenue sharing enabledFeeVaults and deployFeeSplitterbut without revenue sharingFor the scope of this PR we'll also add a template that executes the main path looping through chains, with an example task of 2 chains config as input.
Finally, we'll adapt current integration tests to check the state on L2, and we'll add comprehensive unit tests coverage to the on-chain upgrader contract and to the template too.