feat: upgrade on forking #978
Conversation
don't read addresses from locator in Sanity Checker ctor
Hardhat Unit Tests Coverage SummaryDiff against masterResults for commit: fd8584d Minimum allowed coverage is ♻️ This comment has been updated with latest results |
| run: cp deployed-mainnet.json deployed-mainnet-upgrade.json | ||
|
|
||
| - name: Run upgrade | ||
| run: ./scripts/dao-upgrade.sh |
There was a problem hiding this comment.
🧠 let's think about the granular approach:
- pre-deployment
- deployed but pre-vote
- vote started
- vote finished not enacted
- vote finished and enacted
There was a problem hiding this comment.
shall have entry point from dao-ops to run arbitrary evm scripts (later-later)
scripts/upgrade/steps.json
Outdated
| @@ -1,3 +1,9 @@ | |||
| { | |||
| "steps": [] | |||
| "steps": [ | |||
| "upgrade/steps/0100-deploy-contracts", | |||
There was a problem hiding this comment.
can we re-use scratch?
seems not but would need to sync both
There was a problem hiding this comment.
🧠 Testnet and Mainnet both together?
| bytes32 internal constant DEFAULT_ADMIN_ROLE = 0x00; | ||
| // Burner | ||
| bytes32 internal constant REQUEST_BURN_SHARES_ROLE = keccak256("REQUEST_BURN_SHARES_ROLE"); | ||
| bytes32 internal constant REQUEST_BURN_MY_STETH_ROLE = keccak256("REQUEST_BURN_MY_STETH_ROLE"); |
There was a problem hiding this comment.
@TheDZhon need to redo ACL for Burner
There was a problem hiding this comment.
double check that Burner isn't cached somewhere
There was a problem hiding this comment.
⭐ let's read roles in constructor to immutables
| } | ||
|
|
||
| function _finishUpgrade() internal { | ||
| if (msg.sender != _voting) revert OnlyVotingCanUpgrade(); |
There was a problem hiding this comment.
🧠 after DG here should be Agent (not for this particular line)
| _assertSingleOZRoleHolder(_withdrawalVault, DEFAULT_ADMIN_ROLE, _agent); | ||
| _assertSingleOZRoleHolder(_withdrawalVault, ADD_FULL_WITHDRAWAL_REQUEST_ROLE, _validatorsExitBusOracle); | ||
|
|
||
| // VaultHub |
There was a problem hiding this comment.
here will be PDG too
- probably two new gate seals
contracts/0.8.9/Burner.sol
Outdated
| * The initialization is moved from the constructor to be able to set the shares burnt counters | ||
| * to the up-to-date values from the old Burner upon the upgrade. | ||
| */ | ||
| function initialize( |
There was a problem hiding this comment.
- we have to migrate the whole state, the most important is about
*BurnRequested - transfer stETH back here
⚠️
There was a problem hiding this comment.
let's pass the prev burner address
There was a problem hiding this comment.
🧠 should be a part of the finalizeUpgrade_Vx for Lido
There was a problem hiding this comment.
probably should be a proxy?
unit tests are not updated yet
contracts/0.4.24/Lido.sol
Outdated
|
|
||
| _initialize_v3(); | ||
| _initialize_v3(_oldBurner); |
There was a problem hiding this comment.
| address voting; | ||
| address nodeOperatorsRegistry; | ||
| address simpleDvt; | ||
| address wstETH; |
There was a problem hiding this comment.
probably can remove it bc new locator has it, for old one the workaround is retrieve through WQ
| // New non-aragon implementations | ||
| address accountingOracleImplementation; | ||
| address newLocatorImplementation; | ||
| address withdrawalVaultImplementation; |
| function getEVMScript() public view returns (bytes memory) { | ||
| CallsScriptBuilder.Context memory scriptBuilder = CallsScriptBuilder.create(); | ||
| VoteItem[] memory voteItems = this.getVoteItems(); | ||
|
|
||
| uint256 voteItemsCount = voteItems.length; | ||
| for (uint256 i = 0; i < voteItemsCount; i++) { | ||
| scriptBuilder.addCall(voteItems[i].call.to, voteItems[i].call.data); | ||
| } | ||
|
|
||
| return scriptBuilder.getResult(); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| function getEVMScript() public view returns (bytes memory) { | ||
| CallsScriptBuilder.Context memory scriptBuilder = CallsScriptBuilder.create(); | ||
| VoteItem[] memory voteItems = this.getVoteItems(); | ||
|
|
||
| uint256 voteItemsCount = voteItems.length; | ||
| for (uint256 i = 0; i < voteItemsCount; i++) { | ||
| scriptBuilder.addCall(voteItems[i].call.to, voteItems[i].call.data); | ||
| } | ||
|
|
||
| return scriptBuilder.getResult(); | ||
| } |
Check warning
Code scanning / Slither
Public variable read in external context Warning
| function _assertAragonAppImplementation(IAragonAppRepo repo, address implementation) internal view { | ||
| (, address actualImplementation, ) = repo.getLatest(); | ||
| if (actualImplementation != implementation) { | ||
| revert IncorrectAragonAppImplementation(address(repo), implementation); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| // | ||
| // -------- Pre-upgrade old contracts -------- | ||
| // | ||
| address public immutable OLD_LOCATOR_IMPLEMENTATION; |
There was a problem hiding this comment.
Visual distinction between parameters and derived addresses may be helpful
There was a problem hiding this comment.
Part of immutables are directly passed as a constructor parameters, but part are fetched from other places. I'd appreciate if they're being treat differently.
| function implementation() external view returns (address); | ||
| } | ||
|
|
||
| interface IVaultFactory { |
There was a problem hiding this comment.
Use contracts directly instead of interface where possible
| // | ||
| bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00; | ||
| // Burner | ||
| bytes32 public immutable REQUEST_BURN_SHARES_ROLE; |
There was a problem hiding this comment.
Disclaimer, why roles are immutable
|
|
||
| /// @param _addresses V3Addresses contract address | ||
| constructor(address _addresses) { | ||
| ADDRESSES = V3Addresses(_addresses); |
There was a problem hiding this comment.
Maybe, inherit Template from V3Addresses instead of deploying it as a separate address
There was a problem hiding this comment.
Yeah, would rather inherit
contracts/0.4.24/Lido.sol
Outdated
| uint256 oldBurnerShares = _sharesOf(_oldBurner); | ||
| if (oldBurnerShares > 0) { | ||
| _transferShares(_oldBurner, burner, oldBurnerShares); | ||
| _emitTransferEvents(_oldBurner, burner, oldBurnerShares, oldBurnerShares); |
There was a problem hiding this comment.
| _emitTransferEvents(_oldBurner, burner, oldBurnerShares, oldBurnerShares); | |
| _emitTransferEvents(_oldBurner, burner, oldBurnerBalance, oldBurnerShares); |
contracts/0.4.24/Lido.sol
Outdated
| */ | ||
| function finalizeUpgrade_v3() external { | ||
| function finalizeUpgrade_v3(address _oldBurner, address[4] _contractsWithBurnerAllowances) external { |
There was a problem hiding this comment.
not sure if we need to hardcode exactly four addresses as the array length, tho
| return scriptBuilder.getResult(); | ||
| } | ||
|
|
||
| function getNewVoteCallBytecode(string memory description) external view returns (bytes memory newVoteBytecode) { |
There was a problem hiding this comment.
probably let's document this func 🤔
| // New non-proxy contracts | ||
| address vaultFactory; | ||
|
|
||
| // New fancy proxy contracts |
There was a problem hiding this comment.
| // New fancy proxy contracts | |
| // New fancy proxy and blueprint contracts |
| // | ||
| // -------- Pre-upgrade old contracts -------- | ||
| // | ||
| address public immutable OLD_LOCATOR_IMPLEMENTATION; |
|
|
||
| /// @notice Must be called after LidoLocator is upgraded | ||
| function startUpgrade() external { | ||
| if (msg.sender != ADDRESSES.VOTING()) revert OnlyVotingCanUpgrade(); |
There was a problem hiding this comment.
probably will be an agent here
There was a problem hiding this comment.
set it to agent for now
| if (ILidoWithFinalizeUpgrade(ADDRESSES.LIDO()).getTotalShares() != initialTotalShares || ILidoWithFinalizeUpgrade(ADDRESSES.LIDO()).getTotalPooledEther() != initialTotalPooledEther) { | ||
| revert TotalSharesOrPooledEtherChanged(); | ||
| } |
There was a problem hiding this comment.
why not inside _assertPostUpgradeState?
There was a problem hiding this comment.
I mean it's too early here, bc later we call at least one finalizer 🤔
|
|
||
| /// @title V3VoteScript | ||
| /// @notice Script for upgrading Lido protocol components | ||
| contract V3VoteScript is OmnibusBase { |
There was a problem hiding this comment.
Need to discuss and decide what we will do with:
- new ET plugs
- initial parameters, caps, and limits
| description: "11. Call UpgradeTemplateV3.finishUpgrade", | ||
| call: _votingCall(address(params.upgradeTemplate), abi.encodeCall(ITemplateV3.finishUpgrade, ())) | ||
| }); | ||
| } |
There was a problem hiding this comment.
| } | |
| assert(index == VOTE_ITEMS_COUNT); | |
| } |
| // SPDX-License-Identifier: MIT | ||
| pragma solidity 0.8.25; | ||
|
|
||
| library CallsScriptBuilder { |
There was a problem hiding this comment.
would be great to add some docs and references
scripts/upgrade/upgrade-utils.ts
Outdated
There was a problem hiding this comment.
Maybe move to scripts/utils?
scripts/upgrade/upgrade-utils.ts
Outdated
| import path from "path"; | ||
|
|
||
| export function readUpgradeParameters() { | ||
| const filePath = path.join(__dirname, "upgrade-parameters-mainnet.json"); |
There was a problem hiding this comment.
We'll first need to upgrade the hoodi testnet. Instead of hardcoding, we can add an optional parameter that will override any hardcode and use whatever is specified in it.
| } | ||
|
|
||
| const { contracts, signers } = await discover(); | ||
| // TODO: revisit this when Pectra is live |
There was a problem hiding this comment.
Looks like with the latest HardHat 2.24.0 we can remove this logic, since it's already Pectra
| getSigner: async (signer: Signer, balance?: bigint) => getSigner(signer, balance, signers), | ||
| getEvents: (receipt: ContractTransactionReceipt, eventName: string, extraInterfaces: Interface[] = []) => | ||
| findEventsWithInterfaces(receipt, eventName, [...interfaces, ...extraInterfaces]), | ||
| } as ProtocolContext; | ||
|
|
||
| await provision(context); | ||
| if (isScratch) { |
lib/protocol/discover.ts
Outdated
| } as ProtocolContracts; | ||
|
|
||
| log.debug("Contracts discovered", { | ||
| "Locator": locator.address, | ||
| "Lido": foundationContracts.lido.address, | ||
| "Accounting": foundationContracts.accounting.address, | ||
| "Accounting": foundationContracts.accounting && foundationContracts.accounting.address, |
There was a problem hiding this comment.
| "Accounting": foundationContracts.accounting && foundationContracts.accounting.address, | |
| "Accounting": foundationContracts.accounting?.address, |
There was a problem hiding this comment.
And so on, just more readable. Just a polishing, not a blocker.
| agentAddress: state[Sk.appAgent].proxy.address, | ||
| votingAddress: state[Sk.appVoting].proxy.address, | ||
| easyTrackAddress: state["easyTrackEVMScriptExecutor"].address, | ||
| stakingVaultFactory: state[Sk.stakingVaultFactory] && state[Sk.stakingVaultFactory].address, |
There was a problem hiding this comment.
| stakingVaultFactory: state[Sk.stakingVaultFactory] && state[Sk.stakingVaultFactory].address, | |
| stakingVaultFactory: state[Sk.stakingVaultFactory]?.address, |
and so on...
There was a problem hiding this comment.
Maybe move to contracts/common/interfaces?
| pragma solidity 0.8.25; | ||
|
|
||
| library CallsScriptBuilder { | ||
| bytes4 internal constant SPEC_ID = 0x00000001; |
|
|
||
| /// @notice Must be called after LidoLocator is upgraded | ||
| function startUpgrade() external { | ||
| if (msg.sender != AGENT) revert OnlyVotingCanUpgrade(); |
There was a problem hiding this comment.
set agent for now, easy to change in case
| /** | ||
| * initializer for the Lido version "3" | ||
| */ | ||
| function _initialize_v3() internal { | ||
| _setContractVersion(3); |
There was a problem hiding this comment.
Because one setContractVersion(3) is for upgrade and one for scratch deployment
There was a problem hiding this comment.
there was common _initialize_v3 before but after refactoring not much left to it
No description provided.