Skip to content

interop: add OptimismSuperchainERC20Factory to predeploys#332

Merged
tynes merged 7 commits intomainfrom
feat/SuperchainERC20Factory
Aug 21, 2024
Merged

interop: add OptimismSuperchainERC20Factory to predeploys#332
tynes merged 7 commits intomainfrom
feat/SuperchainERC20Factory

Conversation

@0xParti
Copy link
Copy Markdown
Contributor

@0xParti 0xParti 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.

@0xParti 0xParti 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@0xParti
Copy link
Copy Markdown
Contributor Author

0xParti commented Aug 14, 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

what would you prefer instead? Something like OptimismSuperchainERC20Creation?

@0xParti
Copy link
Copy Markdown
Contributor Author

0xParti 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?

@0xParti
Copy link
Copy Markdown
Contributor Author

0xParti commented Aug 14, 2024

@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?

@0xParti
Copy link
Copy Markdown
Contributor Author

0xParti 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

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