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

interop: add OptimismSuperchainERC20Factory to predeploys #332

Merged
merged 7 commits into from
Aug 21, 2024

Conversation

0xParticle
Copy link
Contributor

@0xParticle 0xParticle commented Aug 13, 2024

Description

Updates the predeploys.md file to include the SuperchainERC20Factory specs, as a follow up to the Design Doc

Closes #10873

Some comments and open questions:

  • I'm using past tense for the event, following the current implementation, but I know there's been discussions around it. Should we stick to the past or begin the transition? what would be a better name?
  • Should the Beacon Contract be a predeploy? if so, should I include it in the predeploys.md file?
  • The current function and event names (createOptimismSuperchainERC20 and OptimismMintableERC20Created) are following the structure from the OptimismMintableERC20Factory. We can change it to something simpler, like deploy, wdyt?
  • In the design doc, the OptimismMintableERC20Created included the token metadata and did not have the deployer address. I removed the metadata to be in sync with the OptimismMintableERC20Created event. Metadata can later be fetched from the superToken address. Any take on this?
  • Should I update the L2StandardBridge in the same file to cover the changes? we already have a liquidity-migration.md file that explains it, but it makes sense to have it repeated here I think.

@0xParticle 0xParticle requested a review from tynes as a code owner August 13, 2024 19:51

### OptimismSuperchainERC20

The `OptimismSuperchainERC20Factory` creates ERC20 contracts that compile to the `SuperchainERC20` [standard](token-bridging.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "compile to" -> "implement"


| Constant | Value |
| -------- | ----- |
| Address | TBD |
Copy link
Contributor

Choose a reason for hiding this comment

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

0x4200000000000000000000000000000000000026 is the next available address

This will ensure the same address deployment across different chains,
which is necessary for the [standard](token-bridging.md) implementation.
The safest way to use `CREATE3` is through
[CreateX](https://github.com/pcaversaccio/createx), which will be a preinstall in the OP stack.
Copy link
Contributor

Choose a reason for hiding this comment

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

Using language like "will be" results in stale specs, you can assume that its part of the OP Stack already here

@tynes
Copy link
Contributor

tynes commented Aug 13, 2024

Should the Beacon Contract be a predeploy?

Yes it should be, we will want to include it in predeploys.md. You can do it as part of this PR or as a follow up. We will need compute the address that the impl is deployed to and then return that from implementation()(address). We do the pre computation as part of consensus already, see ecotone specs derivation for gas price oracle deployment for how we do it. For now we can leave that out tho.

@tynes
Copy link
Contributor

tynes commented Aug 13, 2024

I'm using past tense for the event

Personally i would vote on moving away from past tense but we should not bikeshed on this for too long and just pick something, there exist events that are not past tense that cannot be modified

@tynes
Copy link
Contributor

tynes commented Aug 13, 2024

The current function and event names

I like the simple name deploy to be honest but perhaps we just keep in style with the longer name

@tynes
Copy link
Contributor

tynes commented Aug 13, 2024

Should I update the L2StandardBridge in the same file to cover the changes?

I think moving the changes to predeploys.md and then linking to them from the liquidity migration doc makes the most sense. This keeps context around the predeploy changes in a single place

@0xParticle
Copy link
Contributor Author

I'm using past tense for the event

Personally i would vote on moving away from past tense but we should not bikeshed on this for too long and just pick something, there exist events that are not past tense that cannot be modified

what would you prefer instead? Something like OptimismSuperchainERC20Creation?

@0xParticle
Copy link
Contributor Author

0xParticle commented Aug 14, 2024

Should the Beacon Contract be a predeploy?

Yes it should be, we will want to include it in predeploys.md. You can do it as part of this PR or as a follow up. We will need compute the address that the impl is deployed to and then return that from implementation()(address). We do the pre computation as part of consensus already, see ecotone specs derivation for gas price oracle deployment for how we do it. For now we can leave that out tho.

Will do in this PR.
For address derivation, following the Ecotone and Fjord specs for GasPrice Oralce, I should do

cast compute-address --nonce=0 fromAddress

Do you know what the fromAddress could be in this case?

@0xParticle
Copy link
Contributor Author

@tynes I just pushed a new update addressing your comments.

  • Added the Beacon Contract description. I didn't add functions and events details as its just using the ERC-1967 interface.
  • Added the changes for the L2StandardBridge. I think it makes sense to remove the whole liquidity-migration.md file now, as I've included the updates for both the OptimismMintableERC20Factory and L2StandardBridge in the predeploys.md file. Wdyt?

@0xParticle
Copy link
Contributor Author

0xParticle commented Aug 14, 2024

Also pushed an update to the upgrade.md file to include modifications to the OptimismMintableERC20Factory and L2StandardBridge, and deployment of BeaconContract and OptimismSuperchainERC20Factory.

Let me know if I should remove this change from this PR

@tynes tynes merged commit 8c8e8d9 into main Aug 21, 2024
1 check passed
@tynes tynes deleted the feat/SuperchainERC20Factory branch August 21, 2024 22:02
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.

SuperchainERC20: Factory Spec
2 participants