Feat/upgrade on forking #978
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
don't read addresses from locator in Sanity Checker ctor
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hardhat Unit Tests Coverage SummaryDiff against masterResults for commit: 47fb019 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
TheDZhon
reviewed
Mar 12, 2025
| 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
| } | ||
|
|
||
| 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
Comment on lines
+436
to
+441
| 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
TheDZhon
reviewed
Mar 19, 2025
| _setContractVersion(3); | ||
|
|
There was a problem hiding this comment.
let's check oldBurner != newBurner
contracts/0.4.24/Lido.sol
Outdated
|
|
||
| _initialize_v3(); | ||
| _initialize_v3(_oldBurner); |
There was a problem hiding this comment.
| address predepositGuarantee; | ||
|
|
||
| // New non-proxy contracts | ||
| address burner; |
There was a problem hiding this comment.
let's make it a proxy, 7201 storage (a separate PR)
| 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; |
| // | ||
| event UpgradeFinished(); | ||
|
|
||
| struct UpgradeTemplateV3Params { |
There was a problem hiding this comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.