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.

Sign up for GitHub

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

Jump to bottom

Or 1206 upgrade contacts for native token #28

Merged
merged 12 commits into from
Jan 11, 2024
Merged

Or 1206 upgrade contacts for native token #28

merged 12 commits into from
Jan 11, 2024

Conversation

nguyenzung
Copy link
Member

@nguyenzung nguyenzung commented Jan 9, 2024 •
edited
Loading

  • Add upgrade functions for L1StandardBridge and L1CrossDomainMessenger . The method of upgrading depends on the new contract business so the we can refer the upgrade functions as example.

Test:
Prerequisite: After deploy smart contracts, we need to save the address of SystemOwnerSafe. Our contract system uses this account as admin of contract ProxyAdmin.

  • Upgrade L1StandardBridge:
export IMPL_SALT=$(openssl rand -hex 32)
forge script scripts/Deploy.s.sol:Deploy --private-key $GS_ADMIN_PRIVATE_KEY --sig 'upgradeL1StandardBridge(address)' --rpc-url $L1_RPC_URL <SystemOwnerSafe address> --broadcast
  • Upgrade L1CrossDomainMessenger:
export IMPL_SALT=$(openssl rand -hex 32)
forge script scripts/Deploy.s.sol:Deploy --private-key $GS_ADMIN_PRIVATE_KEY -vvv --sig 'upgradeL1CrossDomainMessenger(address)' --rpc-url $L1_RPC_URL <SystemOwnerSafe address> --broadcast

I used https://l1.dev.tokamak.network as L1 chain for testing.
Thank you!

@nguyenzung nguyenzung self-assigned this Jan 9, 2024
@nguyenzung nguyenzung force-pushed the OR-1206-upgrade-contacts-for-native-token branch from 9d11c19 to 8dcce70 Compare January 9, 2024 08:50
boohyung
boohyung reviewed Jan 11, 2024
View reviewed changes
.gitignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need it for upgrading contracts only in tokamak/contracts-bedrock.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I updated!

rlgns98kr
rlgns98kr reviewed Jan 11, 2024
View reviewed changes
packages/contracts-bedrock/src/L1/L1CrossDomainMessenger.sol Outdated
@@ -54,4 +54,5 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, ISemver {
function _isUnsafeTarget(address _target) internal view override returns (bool) {
return _target == address(this) || _target == address(PORTAL);
}

Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need this empty line! 🙇🏼‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I updated!

@nguyenzung nguyenzung force-pushed the OR-1206-upgrade-contacts-for-native-token branch from 1181f8e to 58e9bc9 Compare January 11, 2024 02:59
rlgns98kr
rlgns98kr reviewed Jan 11, 2024
View reviewed changes
packages/tokamak/contracts-bedrock/scripts/Deployer.sol
@@ -157,10 +157,11 @@ abstract contract Deployer is Script {
vm.writeJson({ json: json, path: artifactPath });
}

console.log("Synced temp deploy files, deleting %s", tempDeploymentsPath);
vm.removeFile(tempDeploymentsPath);
// console.log("Synced temp deploy files, deleting %s", tempDeploymentsPath);
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry.😢 I didn't see this before. Can you explain about it? Will it be used in next? 🙇🏼‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!
This file contains data for contract addresses and I think it is helpful for us to check the contract addresses conveniently. For eg, we can check address of SystemOwnerSafe to upgrade contracts. The file looks like that:

{
  "AddressManager": "0xB93AdE78a4398Dfb7d043C5D3e512cbd9970198e",
  "L1CrossDomainMessenger": "0xFc9EE331a0431A53bB821E4952f4c8Ac3cA7EcC8",
  "L1CrossDomainMessengerProxy": "0x3370Ce3337d4203b660Ce0Ef6248F78FE42f80D2",
  "L1ERC721Bridge": "0xde38eb856821fFeA08376b76Be1B56846284a0EC",
  "L1ERC721BridgeProxy": "0xf3C49117def7b9D32Ee41b0FEF7E528d5354082a",
  "L1StandardBridge": "0x1d16b9BAC7f5b53Ee46Dd16564Ae6478D7dA0Fca",
  "L1StandardBridgeProxy": "0xDFC784031ceA1540cC41c152b2e8a470f33E5D67",
  "L2OutputOracle": "0x76A9a2e19cfF03307636548b3622790dfcaC14BF",
  "L2OutputOracleProxy": "0xBeA3ad32DF42EB9130260Aa125e04eC610fC324f",
  "OptimismMintableERC20Factory": "0x36899AaBbcD58e35970f347F5828Ef3d4AccdB8e",
  "OptimismMintableERC20FactoryProxy": "0xf0066C9DAb89f07B01f23DC46811031a55bd6C54",
  "OptimismPortal": "0xf01074bdB175009a27ABaFbcbC3E45548e6cFA92",
  "OptimismPortalProxy": "0x33AfBc99faC2F9eEee74ade9577C77655a6574Bd",
  "ProtocolVersions": "0xE40D852f9fb62b4713eebDd9DfA7A3E55e1Fc35f",
  "ProtocolVersionsProxy": "0x6a2e963A62106B432c7a5CA89b9f1a3b6916fba0",
  "ProxyAdmin": "0x32EF05A5C56954B48E7a1038a6FD35043422b743",
  "SafeProxyFactory": "0xC665d41D43CfFFf9204c08f706CEa5FFD2A64c57",
  "SafeSingleton": "0x1F6c9D3C62975c310eC566d4e52c724493C3c8CA",
  "SystemConfig": "0x3B0e586EA3FcCa13BD661a06280ed9250fC5bCc4",
  "SystemConfigProxy": "0xd8A7AC9a49A682528cCe8Ae9d90f0E86FfCca744",
  "SystemOwnerSafe": "0x89584c61160DC6dFE09232b8E9452eF8892e0C18"
}

rlgns98kr
rlgns98kr approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@rlgns98kr rlgns98kr left a comment

Choose a reason for hiding this comment

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

Will you delete something you added in .gitignore?
Thank you very much! 🙇🏼‍♂️

boohyung
boohyung reviewed Jan 11, 2024
View reviewed changes
.gitignore Outdated
@@ -51,3 +51,7 @@ __pycache__

# Ignore echidna artifacts
crytic-export
packages/contracts-bedrock/upgrade*
packages/contracts-bedrock/deploy.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @rlgns98kr's comment. BTW where is deploy.sh? If you don't use this file, never mind and merge it. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

@rlgns98kr @boohyung Let me delete:
packages/contracts-bedrock/upgrade*
packages/contracts-bedrock/deploy.sh
Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

@boohyung deply.sh is my local file and it is used for deploy smart contracts

Copy link
Member

Choose a reason for hiding this comment

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

@nguyenzung
I think we can delete in .gitignore although you are using in your local. You can select specific files to push to repo. 🙇🏼‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @rlgns98kr ,
I was not sure about that. I though you mentioned about file in packages/contract-bedrock. Let me remove it on next PR!

@nguyenzung nguyenzung merged commit 655dbb2 into main Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boohyung boohyung boohyung left review comments

@rlgns98kr rlgns98kr rlgns98kr approved these changes

@ohbyeongmin ohbyeongmin Awaiting requested review from ohbyeongmin

@AaronLee22 AaronLee22 Awaiting requested review from AaronLee22

@0x6e616d 0x6e616d Awaiting requested review from 0x6e616d

Assignees

@nguyenzung nguyenzung

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nguyenzung @boohyung @rlgns98kr