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

feat(contracts): add Kroma-specific contracts #1

Open
wants to merge 16 commits into
base: feat/apply-kroma-v2.1.0
Choose a base branch
from

Conversation

sm-stack
Copy link

Description

Adding kroma-specific contracts, including L1, governance, universal, and libraries.

I moved kroma-specific constants and types into separate Solidity files(KromaConstants.sol, KromaTypes.sol), for future upstream convenience. Regarding the predeploy (Predeploys.sol), I should put kroma-specific ones inside the existing OP ones, since the library file will be used in the deployment process.

Also added OZ upgradeable library with version of 4.9.3 as dependency, since there were some compile errors that some kroma-specific files that imports OZ upgradeable contract induced. The current OP Stack uses version of 4.7.3.

Expected L1 Contract Upgrades

This change may induce upgrade of the following L1 contracts. The upgrade will make those contracts to be same with OP.

  • L1CrossDomainMessenger
  • L1StandardBridge
  • L1ERC721Bridge
  • SystemConfig

In terms of KromaPortal, it seems that we can upgrade it to OptimismPortal, but needs more investigation about the effects of that change, so I decided to move KromaPortal temporarily this time.

Expected L1 Contract Upgrades (Kroma-specific)

The following contracts should be upgraded, due to the removal of Validator System V1.

  • KromaPortal
  • Colosseum
  • L2OutputOracle
  • ValidatorManager
  • ZKProofVerifier

Notes on Formatting & Compile

To format the Solidity files, I used forge fmt <filename>, which seems to be used in OP Stack. Note that just executing forge fmt will change the format of some unrelated Solidity files, since the formatting is not consistent within this repository by default (be cautious).

Also did some changes on formats at the comments inside the files.

To build / compile the whole contract, please use just forge-build-dev instead of just forge-build. If the LITE mode of Foundry is not turned on, it will take about 6 minutes to compile the whole Solidity files (Note that vanilla OP Stack takes about 4-5 minutes for the compile of Solidity files).

@sm-stack sm-stack requested a review from a team January 13, 2025 06:49
@sm-stack sm-stack self-assigned this Jan 13, 2025
@sm-stack sm-stack changed the base branch from kroma to feat/apply-kroma-v2.1.0 January 13, 2025 06:50
@sm-stack sm-stack force-pushed the feat/add-l1-contracts branch from 0ccb2ad to 74b387a Compare January 14, 2025 06:59
@sm-stack sm-stack force-pushed the feat/apply-kroma-v2.1.0 branch from 28c4ad2 to 8bf7ff6 Compare January 14, 2025 08:07
@sm-stack sm-stack force-pushed the feat/add-l1-contracts branch from 74b387a to 6d43f13 Compare January 14, 2025 08:11
@sm-stack sm-stack force-pushed the feat/add-l1-contracts branch from 6d43f13 to b3bf017 Compare January 14, 2025 08:14
@seolaoh
Copy link

seolaoh commented Jan 21, 2025

  • L1CrossDomainMessenger.sol: IOptimismPortal should be replaced with KromaPortal.
  • L1ERC721Bridge.sol: since we use another address for L2_ERC721_BRIDGE, we should handle it in Predeploys.sol.
  • We need to add SuperchainConfig contract in our L1 deploy process, since it is used in L1CrossDomainMessenger, L1ERC721Bridge, ...
  • SystemConfig.sol
    • I wonder if it's ok to override validatorRewardScalar slot with eip1559Denominator.
    • We need to replace IOptimismPortal with KromaPortal and handle _setGasPayingToken function which uses IOptimismPortal.setGasPayingToken().
    • Probably we need to deploy GasPayingToken also.

@seolaoh
Copy link

seolaoh commented Jan 21, 2025

  • CrossDomainMessenger.sol: now it inherits different contracts so we need to check the backward compatibility for the storage variables. I think we need to modify CrossDomainMessengerLegacySpacer0, CrossDomainMessengerLegacySpacer1.
  • ProxyAdmin.sol: We cannot upgrade our ProxyAdmin contract on L1, and there are some legacy stuffs of OP's ProxyAdmin so I think we should replace it with Kroma's ProxyAdmin contract.
  • StandardBridge.sol
    • it now adds 2 slots in front of deposits. I think we need to handle it?
    • To be backward compatible, we need to add the supports for KromaMintableERC20. Esp the governance token is not upgradeable anymore, we should support the interface also.

@sm-stack
Copy link
Author

  • Probably we need to deploy GasPayingToken also.

I think if we don't use custom gas token feature, we don't have to deploy it.

@seolaoh
Copy link

seolaoh commented Jan 22, 2025

  • Probably we need to deploy GasPayingToken also.

I think if we don't use custom gas token feature, we don't have to deploy it.

But there are some usages of GasPayingToken in SystemConfig contract, so if we won't deploy it, then we should comment the related parts I think?

OP uses version 2.2.0 for their mainnet which does not include GasPayingToken stuffs.

@sm-stack
Copy link
Author

  • ProxyAdmin.sol: We cannot upgrade our ProxyAdmin contract on L1, and there are some legacy stuffs of OP's ProxyAdmin so I think we should replace it with Kroma's ProxyAdmin contract.

I didn't handle the ProxyAdmin part yet in the first PR review, since it seems that we have an option to use a new ProxyAdmin at the same address as OP.

@seolaoh
Copy link

seolaoh commented Jan 22, 2025

Please apply forge fmt right before merging this branch since there are some changes when running forge fmt.

packages/contracts-bedrock/src/L1/Colosseum.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/L1/Colosseum.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/L1/L2OutputOracle.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/L1/L2OutputOracle.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/L1/Colosseum.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/L1/L2OutputOracle.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/L1/ValidatorManager.sol Outdated Show resolved Hide resolved
@sm-stack sm-stack requested a review from seolaoh January 23, 2025 06:06
@sm-stack
Copy link
Author

sm-stack commented Jan 23, 2025

Also note that Optimism just removed the L2OutputOracle / OptimismPortal and all related deploy/test codes from its monorepo recently in this and this PR. So maybe at the following PR, I think removing the whole OptimismPortal.t.sol file would make us more convenient, since we have to revise various compile errors regarding fixed L2OutputOracle there.

@sm-stack sm-stack requested a review from seolaoh January 24, 2025 04:17
@sm-stack sm-stack requested a review from seolaoh February 3, 2025 01:53
@sm-stack
Copy link
Author

sm-stack commented Feb 3, 2025

I added a work related to removing the restriction for MPT first output index, since it would be useless after the MPT migration.

Copy link

@seolaoh seolaoh left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants