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 deploy configs and implementation deploy logic for kroma L1 contracts #2

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

Conversation

sm-stack
Copy link

@sm-stack sm-stack commented Feb 5, 2025

Description

This PR does 2 things

  • Add deploy configs for kroma L1 contracts
  • Add KromaDeployImplementation.s.sol, which is to deploy implementation contracts of kroma L1.

KromaDeployImplementation

Background

The reason why I made this contract is, to make the future upstream process as easy as possible while not breaking the compatibility with future modifications on OP Stack. The current OP Stack highly depends on 2 script files when deploying the smart contract: Deploy.s.sol and L2Genesis.s.sol. So adding kroma-specific one in that level will break the actions at devnet & testing.
For example, recently OP Stack made a breaking change on its devnet structure, which uses kurtosis instead of custom python file to initiate devnet. The optimism-package at kurtosis relies on op-deployer, and op-deployer calls Deploy.s.sol and L2Genesis.s.sol internally.

So I thought that instead of adding a separate deploy script for Kroma contracts, integrating the deployment process of Kroma specific contracts inside Deploy.s.sol and L2Genesis.s.sol is much better. Please suggest any ideas if you have!

Demonstration

The KromaDeployImplementation.s.sol contract takes the role of deploying the impl contracts, and doing the sanity checks of the deploy configs and deployment itself.

The KromaDeployImplementation contract will be initialized in Deploy.s.sol, and will be used to deploy kroma L1 contracts. There would be a function such as deployKromaImplementations(), which is similar with deployImplementations function. The full process will be like this:

  1. deployKromaImplementations() function (which will be implemented in the following PR) will get the necessary constructor arguments from DeployConfig.s.sol and set it at KromaDeployImplementationsInput contract.
  2. It will execute run() function at KromaDeployImplementation, which will then deploy the whole contracts and return it at the output format of KromaDeployImplementationsOutput.

Considerations

  • Optimism actively uses contract interfaces, so I had to add interfaces for governance contracts & some function interfaces for L1 contracts to read immutable variables. There is a cyclic dependency issue between ValidatorManager and AssetManager, so I couldn't add a check for ValidatorManager address at assertValidAssetManager() function.
  • Optimism used abi.encodeCall and <InterfaceName>.__constructor__ when deploying the impl contract, but I changed it to abi.encodeWithSelector. abi.encodeCall is a better one, since it checks the type of each variables when it is called, while abi.encodeWithSelector does not. The reason why I did like this was because ValidatorManager and AssetManager imports their own interfaces. If I add __constructor__(...) at the interface files, the contracts that imports those interfaces breaks, since there's no __constructor__ function in their contract. Maybe removing interface imports from ValidatorManager and AssetManager will work, but not sure it's a good idea.

Please lmk if you have any idea to solve those problems.

@sm-stack sm-stack self-assigned this Feb 5, 2025
@sm-stack sm-stack requested a review from a team February 5, 2025 08:58
@sm-stack sm-stack force-pushed the feat/add-deploy-scripts branch 2 times, most recently from 781feba to 0309a5f Compare February 6, 2025 05:11
@sm-stack sm-stack force-pushed the feat/add-deploy-scripts branch from d4b9300 to d0c6c59 Compare February 7, 2025 08:11
@sm-stack
Copy link
Author

sm-stack commented Feb 7, 2025

I've rebased this onto the target branch to apply the fix that removes importing interfaces at contracts (ValidatorManager, AssetManager). Also, based on that changes, I've managed to address the latter consideration, by commit d0c6c59.

+Addressed the first consideration by the following commit!

@sm-stack sm-stack force-pushed the feat/add-deploy-scripts branch from add21db to 6ae2273 Compare February 10, 2025 02:14
@sm-stack sm-stack requested review from seolaoh and Pangssu February 10, 2025 03:18
@sm-stack
Copy link
Author

sm-stack commented Feb 12, 2025

Added 5 commits, doing the following things

  • Thanks to @0xbenyun, interface files are generated by cast interface or forge inspect. Updated all added interfaces at this commit.
  • Removed interface imports from KromaMintableERC20 and ProxyAdmin contract, based on the interface policy of Optimism, at this commit.
  • The interface check script which is executed at just build generates error both at the incomplete interface files like IL2OutputOracle, and at newly imported OZ contracts. Thus, I added them to 'excluded contracts' at the interface checking script, at this commit. Note that 3 contracts (IL2OutputOracle / ProxyAdmin / IL1CrossDomainMessenger) should be removed after resolving compile errors related to them at the tests.
  • Added storage and abi files generated by just snapshots-abi-storage at this commit.

@sm-stack sm-stack requested a review from seolaoh February 12, 2025 13:00
@@ -15,7 +15,7 @@ import { IKromaMintableERC20 } from "interfaces/universal/IKromaMintableERC20.so
/// use a KromaMintableRC20 as the L2 representation of an L1 token, or vice-versa.
/// Designed to be backwards compatible with the older StandardL2ERC20 token which was only
/// meant for use on L2.
contract KromaMintableERC20 is IKromaMintableERC20, ERC20, ISemver {
contract KromaMintableERC20 is ERC20, ISemver {
Copy link

Choose a reason for hiding this comment

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

We can remove L10.

Copy link
Author

Choose a reason for hiding this comment

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

It is used at the supportInterface function below, so removing it will generate a compile error.

Base automatically changed from feat/add-l1-contracts to feat/apply-kroma-v2.1.0 February 21, 2025 04:09
@dongchangYoo dongchangYoo force-pushed the feat/add-deploy-scripts branch 2 times, most recently from 48c784a to cd8c5c1 Compare February 25, 2025 05:51
@dongchangYoo dongchangYoo force-pushed the feat/add-deploy-scripts branch from cd8c5c1 to 2ed316a Compare February 26, 2025 02:54
In the previous PR2, `KromaPortal` was replaced with `OptimismPortal`,
and this PR addresses and resolves the issues that arose during the rebase of that change.
Comment on lines +30 to +31
// TODO: Temp. Remove the following line once the interface is fixed
"IL2OutputOracle", "IL1CrossDomainMessenger", "IProxyAdmin",

Choose a reason for hiding this comment

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

I think we've done enough work with Interface. Why don't we include this temporarily excluded part now?

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.

5 participants