Skip to content
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

Sovereign bridge implementation #330

Merged
merged 8 commits into from
Nov 5, 2024

Conversation

ignasirv
Copy link
Contributor

@ignasirv ignasirv commented Sep 16, 2024

Create Bridge and GlobalExitRootManager contracts for Sovereign chains + tests.
BridgeL2SovereignChain.sol : extends from PolygonZkEVMBridgeV2 with the features of:

  • Set a bridgeManager address
  • Functions to remap and undo remap of token addresses by sovereign token addresses
  • Overrided mint/burn functions from BridgeV2 to handle that new remapping

GlobalExitRootManagerL2SovereignChain.sol : extends from PolygonZkEVMGlobalExitRootL2 with the features of:

  • New function insertGlobalExitRoot only callable by coinbase to update the GER of a Sovereign Chain

@invocamanman
Copy link
Collaborator

jsut run the coverage:
function setMultipleSovereignTokenAddress remains untested, even tho will be veery easy to do it so

migrateLegacyToken remains untested as wel which i think it's a critical funciton and we should test it!!!

@invocamanman
Copy link
Collaborator

also we shoudl update the 1_create genesis deployment script to be able to put a flag "sovereign" and deploy these contracts

@invocamanman
Copy link
Collaborator

Ger implementation shoould write blockHashes right?¿ probably from AggLayer or L1, to mirror the behaviour of Current Global exit root

Even tho in the zkResidency we though to change that to an index to help us retrieve all the improted GERs, but that depends a lot on what impementaiton we will follow to do that.
For now i wioudl say that you can set block hashes instead of timestamp

@invocamanman
Copy link
Collaborator

overall LGTM ^^ but we should do a proper review ^^

@invocamanman
Copy link
Collaborator

As an added comment i think the docker does not work since the bytecode of hte new bridge contract is too big.

Warning: Contract code size is 31141 bytes and exceeds 24576 bytes (a limit introduced in Spurious Dragon). This contract may not be deployable on Mainnet. Consider enabling the optimizer (with a low "runs" value!), turning off revert strings, or using libraries.
  --> contracts/v2/sovereignChains/BridgeL2SovereignChain.sol:15:1:

We should reduce the bytecode size, the eaisest way is to put it on exceptions on harhdat and lower the optimizer runs by default

Copy link
Contributor

@krlosMata krlosMata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😸

Copy link
Contributor

@krlosMata krlosMata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😸

ignasirv and others added 3 commits October 21, 2024 10:58
Add more testing

Refactor function overrite

Refactor GlobalExitRoot contracts for dovereign chains


First testing iteration sovereign chains

Review fixes

Review fixes II

Review fixes III

Full testing coverage for sovereign contracts

Added sovereign bridge features

- Batch call function implementation to remap multiple tokens
- Allow migrating legacy to updated tokens natively
- Added weth mapped address are initializer

Refactor migrateLegacyToken function

Coverage tests + fix 1_genesis script

+ typos

Fix sovereign chain deployment at docker and removed struct


Add remove ger function at sovereign GER contracts

Review comment and add override at initialize function

Refactor gerManagerSovereigns

Create batch for pessimistic networks
Update bridge at create rollup for vanilla clients

Review update
@@ -608,7 +619,7 @@ contract PolygonZkEVMBridgeV2 is
address destinationAddress,
uint256 amount,
bytes calldata metadata
) external ifNotEmergencyState {
) external virtual ifNotEmergencyState {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added virtural for no reason?¿

* @notice Updated bridge manager address
* @param _bridgeManager Bridge manager address
*/
function setBridgeManager(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

settes/getters usually go downside of the code

*/
event RemoveLastGlobalExitRoot(bytes32 indexed removedGlobalExitRoot);

modifier onlyGlobalExitRootUpdater() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the rest of contrats convention, modifiers go after consturctor and initialization

// Set sovereign weth token or create new if not provided
if (_sovereignWETHAddress == address(0)) {
// Create a wrapped token for WETH, with salt == 0
WETHToken = _deployWrappedToken(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could check tha WETHisNotMIntable is false?¿

amount
);
} else {
// Claim wETH
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment

) internal override {
// If is not mintable transfer instead of mint
if (wrappedAddressIsNotMintable[address(tokenWrapped)]) {
// Transfer wETH
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment


/**
* @notice Mints tokens from wrapped token to proceed with the claim, if the token is not mintable it will be transferred
* note This function has been extracted to be able to override it by other contracts like Bridge2SovereignChain
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commenr

Package-lock

Small fix upgrade rollupManager pessimistic script
@@ -34,7 +34,7 @@
"@nomicfoundation/hardhat-toolbox": "^3.0.0",
"@openzeppelin/contracts": "4.8.2",
"@openzeppelin/contracts-upgradeable": "4.8.2",
"@openzeppelin/contracts5": "npm:@openzeppelin/contracts@^5.0.0",
"@openzeppelin/contracts5": "npm:@openzeppelin/[email protected]",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! ^^

@ignasirv ignasirv closed this Nov 5, 2024
@ignasirv ignasirv reopened this Nov 5, 2024
Update deploy scripts with more checks

Improve upgrade presimistic script
Copy link

sonarcloud bot commented Nov 5, 2024

@ignasirv ignasirv merged commit 63a89fe into feature/ongoingPP Nov 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants