-
Notifications
You must be signed in to change notification settings - Fork 366
upgrade script etrog -> sovereign #533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/v12
Are you sure you want to change the base?
Conversation
laisolizq
commented
Sep 18, 2025
- Add script upgrade etrog -> sovereign
| address _emergencyBridgePauser, | ||
| address _emergencyBridgeUnpauser, | ||
| address _proxiedTokensManager | ||
| ) public virtual reinitializer(3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea to add the getInitializedVersion as it is done in the AgglayerBridge initialize; check initializer version is 1 (or not zero) and also check that current values to update are zero.
d7080a6 to
c44ff1c
Compare
6277218 to
1207da1
Compare
|
Add |
| * @dev The key is generated as keccak256(abi.encodePacked(originNetwork, originTokenAddress)) | ||
| * @param amount The amount to set for the local balance tree leaf | ||
| */ | ||
| function _setLocalBalanceTreeInitialize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of replicating the function setLocalBalanceTree I would move setLocalBalanceTree code to an internal function _setLocalBalanceTree and call it from initialize, this way you will be saving a lot of bytecode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the idea was not to change the AgglayerBridgeL2 contract, so I implemented this function here.
Is it okay to change it? Should I modify it, or leave it as it is?
@krlosMata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, since it’s an internal change that doesn’t affect functionality, I’ll go ahead and make the change. If anyone thinks otherwise, please let me know ^^
7d700a2 to
3c1c8e2
Compare
| * @param _emergencyBridgeUnpauser emergency bridge unpauser address, allowed to be zero if the chain wants to disable the feature to unpause the bridge | ||
| * @param _proxiedTokensManager address of the proxied tokens manager | ||
| */ | ||
| function initializeBridgeZkEVM( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
funciton name seems uncoherent with contract name
maybe InitializeFromEtrog?
|
JUst to double check, are we sure that the |
7326371 to
2862a36
Compare
I compared the two bytecodes with a script (comparing only strings), and it says everything is the same except for the last few bytes (which, according to what I’ve found, is a metadata hash that shouldn’t matter). Is that right? @invocamanman |
2862a36 to
15dafbb
Compare
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a script for upgrading L2 bridge and global exit root contracts from Etrog to Sovereign versions, facilitating the migration from PolygonZkEVMBridgeV2 to AgglayerBridgeL2 and PolygonZkEVMGlobalExitRootL2 to AgglayerGERL2.
Key changes:
- Upgrade script with timelock operation generation for both bridge and GER contracts
- Test suite for validation via shallow fork testing
- Contract implementation supporting upgrade from Etrog with initialization of new parameters
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| upgrade/upgradeEtrogSovereign/upgradeEtrogToSovereign.ts | Main upgrade script deploying implementations and generating timelock operations |
| upgrade/upgradeEtrogSovereign/test/shallowForkUpgrade.test.ts | Shallow fork test validating contract upgrades and parameter initialization |
| upgrade/upgradeEtrogSovereign/getLBT.ts | Utility script for retrieving Local Balance Tree data from NewWrappedToken events |
| upgrade/upgradeEtrogSovereign/README.md | Documentation for upgrade process and configuration |
| upgrade/upgradeEtrogSovereign/upgrade_parameters.json.example | Example configuration file with required parameters |
| contracts/sovereignChains/AgglayerBridgeL2FromEtrog.sol | Bridge contract implementation supporting upgrade from Etrog version |
| contracts/sovereignChains/AgglayerBridgeL2.sol | Internal method extraction for Local Balance Tree initialization |
| test/contractsv2/AgglayerBridgeL2FromEtrogUpgrade.test.ts | Unit test for upgrade functionality |
| hardhat.config.ts | Compilation configuration for new contract |
| upgrade/upgradeEtrogSovereign/.gitignore | Ignore file for generated LBT data |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| dotenv.config({ path: path.resolve(__dirname, '../../.env') }); | ||
|
|
||
| dotenv.config({ path: path.resolve(__dirname, '../../.env') }); |
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate dotenv.config() call - this line is identical to line 22 and should be removed.
| dotenv.config({ path: path.resolve(__dirname, '../../.env') }); |
| dotenv.config({ path: path.resolve(__dirname, '../../.env') }); | ||
|
|
||
| async function main() { | ||
| // Asser upgrade version |
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'Asser' to 'Assert'.
| // Asser upgrade version | |
| // Assert upgrade version |
| // load timelock | ||
| const timelockContractFactory = await ethers.getContractFactory('PolygonZkEVMTimelock', deployer); | ||
| const timelockContract = (await timelockContractFactory.attach(timelockAddress)) as TimelockController; | ||
| // take params delay, or minimum timelock dela |
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'dela' to 'delay'.
| // take params delay, or minimum timelock dela | |
| // take params delay, or minimum timelock delay |
| import upgradeParams from './upgrade_parameters.json'; | ||
| import { logger } from '../../src/logger'; | ||
|
|
||
| export async function getLBT(contractAddress) { |
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing type annotation for contractAddress parameter. Should be 'contractAddress: string'.
| export async function getLBT(contractAddress) { | |
| export async function getLBT(contractAddress: string) { |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting |
| @@ -0,0 +1,14 @@ | |||
| { | |||
| "bridgeL2": "0x..", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we create a separate object with: initiaizationParameters ?
Just to be more explicit
| import { addInfoOutput } from '../../tools/utils'; | ||
|
|
||
| // You can replace this import with the actual values obtained from getLBT | ||
| import { originNetwork, originTokenAddress, totalSupply } from './initializeLBT.json'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no example of this file.
Reminder that this file could be generated from getLBT.ts tool or from the agglayer-node or any other tool
| logger.info('Create schedule and execute operation'); | ||
| logger.info('Operation Bridge'); | ||
| // Get LBT initialization parameters (if not imported, import getLBT and use it) | ||
| // const { originNetwork, originTokenAddress, totalSupply } = await getLBT(bridgeL2, creationBridgeL2Block); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several comments in the files. It should be cleaned
| @@ -0,0 +1,79 @@ | |||
| import { ethers } from 'hardhat'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another tool. The upgrade can take the LBT from any source.
I'd create a separate tool to synch the LBT.
|
|
||
| dotenv.config({ path: path.resolve(__dirname, '../../.env') }); | ||
|
|
||
| const pathOutputJson = path.join(__dirname, './upgrade_output.json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add timestamp here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d rather not add a timestamp, since from the test I retrieve this upgrade_output in order to use that data within the test.
I think I’ve seen all the upgrade tests done this way.
| @@ -0,0 +1 @@ | |||
| initializeLBT.json No newline at end of file | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing upgrade_output_*.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is added to the .gitignore in the root of the project. Even so, if I add a timestamp to it, I’ll also add it to the .gitignore of this tool.
a0a3ab9 to
e49d164
Compare
| uint32[] memory originNetwork, | ||
| address[] memory originTokenAddress, | ||
| uint256[] memory amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename this and also the internal function, to array i think might be better
e.g originNetworkArray
| * @param _proxiedTokensManager address of the proxied tokens manager | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
natspec
| /* eslint-disable no-await-in-loop, no-use-before-define, no-lonely-if */ | ||
| /* eslint-disable no-console, no-inner-declarations, no-undef, import/no-unresolved */ | ||
| import { expect } from 'chai'; | ||
| import path = require('path'); | ||
|
|
||
| import * as dotenv from 'dotenv'; | ||
| import { ethers, upgrades } from 'hardhat'; | ||
| import { time, reset, setBalance, mine } from '@nomicfoundation/hardhat-network-helpers'; | ||
| import { | ||
| PolygonZkEVMTimelock, | ||
| AgglayerGERL2, | ||
| AgglayerBridgeL2, | ||
| PolygonZkEVMBridgeV2Pessimistic, | ||
| LegacyAgglayerGERL2, | ||
| } from '../../../typechain-types'; | ||
| import upgradeParams from '../upgrade_parameters.json'; | ||
| import upgradeOutput from '../upgrade_output.json'; | ||
| import { logger } from '../../../src/logger'; | ||
| import { checkParams } from '../../../src/utils'; | ||
| import { ProxyAdmin } from '../../../typechain-types/@openzeppelin/contracts4/proxy/transparent'; | ||
|
|
||
| dotenv.config({ path: path.resolve(__dirname, '../../.env') }); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shadow fork ahhaha
919cc16 to
26d5f01
Compare
2d285ae to
ff874a1
Compare