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: update predeploys.md #314

Merged
merged 12 commits into from
Aug 12, 2024
Merged

Conversation

0xParticle
Copy link
Contributor

Description
Updated the predeploys.md spec to include details on the new OptimismMintableERC20Factory.
I only focused on the updates to keep it similar to the L1Block section (also an update), but did not make any comments about the actual public and external functions, or invariants. I can include all of this and turn it into a more explicit update or a full spec.

@0xParticle 0xParticle requested a review from tynes as a code owner August 5, 2024 17:37
### OptimismMintableERC20

The `OptimismMintableERC20Factory` creates ERC20 contracts on L2 that can be used to deposit
native L1 tokens into (`OptimismMintableERC20`). Anyone can deploy `OptimismMintableERC20`s contracts.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: trailing s reads strange in "OptimismMintableERC20s"

The `OptimismMintableERC20Factory` is updated to use `CREATE3` instead of `CREATE2`.
This will remove the compiler's dependency on address computation.

The salt SHOULD depend on the inputs:
Copy link
Contributor

Choose a reason for hiding this comment

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

A quick sentence for rationale here would be good, like these are used to ensure that there is a single unique token address per set of ERC20 metadata

@tynes
Copy link
Contributor

tynes commented Aug 5, 2024

Looks good to me, one could say that the SHOULDs could be changed to MUST since its not optional, see https://datatracker.ietf.org/doc/html/rfc2119

@0xParticle
Copy link
Contributor Author

0xParticle commented Aug 6, 2024

@tynes Do you think its worth mentioning something about the external functions and invariants here? I also haven't mentioned anything about the deployments one time update with the token list, which will be part of the first update.

@tynes
Copy link
Contributor

tynes commented Aug 6, 2024

external functions and invariants here

I do think that would be useful here and keep a high bar for the specs

@tynes
Copy link
Contributor

tynes commented Aug 6, 2024

I don't think we want to go deep into the one time update for the tokenlist here, that is something we should do a design doc for and not add to the specs

@0xParticle
Copy link
Contributor Author

external functions and invariants here

I do think that would be useful here and keep a high bar for the specs

Added both. Let me know if you agree.

@agusduha
Copy link
Contributor

agusduha commented Aug 7, 2024

We are adding a new contract inheriting form OptimismMintableERC20Factory to make these updates.

Should we create a new function createWithCreate3 to be called from createOptimismMintableERC20WithDecimals so that we can add it to the new contract? Or do u prefer to directly change the code in this function?

@0xParticle
Copy link
Contributor Author

We are adding a new contract inheriting form OptimismMintableERC20Factory to make these updates.

Should we create a new function createWithCreate3 to be called from createOptimismMintableERC20WithDecimals so that we can add it to the new contract? Or do u prefer to directly change the code in this function?

@tynes this would affect the specs. We could simply modify the existing createOptimismMintableERC20WithDecimals or adda new function that createOptimismMintableERC20WithDecimals will call (similar to how previous upgrades where extended). wdyt?

@tynes
Copy link
Contributor

tynes commented Aug 12, 2024

I thought we decided to use CREATEX and also not update the OptimismMintableERC20Factory to use it?


### Functions

#### `createWithCreate3`
Copy link
Contributor

Choose a reason for hiding this comment

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

This leaks an implementation detail into the name, not a fan of this style of naming

A reference implementation looks like the following:

```solidity
function createOptimismMintableERC20WithDecimals(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could get away without implementing the whole function here and instead precisely define the salt in natural language and the invariant that the mapping must be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perfect. Will also remove the reference implementation for the others

@tynes tynes merged commit bb2c54a into main Aug 12, 2024
1 check passed
@tynes tynes deleted the feat/OptimismMintableFactory-update branch August 12, 2024 23:10
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.

3 participants