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.

Sign up for GitHub

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

Jump to bottom

feat: introduce SuperchainERC20 redesign + ICrosschainERC20 #12321

Merged
merged 27 commits into from
Oct 15, 2024
Merged

feat: introduce SuperchainERC20 redesign + ICrosschainERC20 #12321

merged 27 commits into from
Oct 15, 2024
+1,007 −1,001

Conversation

agusduha
Copy link
Contributor

@agusduha agusduha commented Oct 4, 2024 •
edited
Loading

Description

The main idea is migrating the sendERC20 and relayERC20 functions from the SuperchainERC20 standard to a new SuperchainERC20Bridge. Also introducing ICrosschainERC20 interface for mint/burn functions to be cross-chain neutral and extensible to other L2s.

Specs:
ethereum-optimism/specs#384
ethereum-optimism/specs#413

Tests

  • Add SuperchainERC20Bridge tests
  • Refactor the old superchain standard tests

Additional context

The SuperchainERC20Bridge is a new predeploy.

Metadata

Closes #12322

agusduha and others added 15 commits September 25, 2024 15:20
@agusduha
feat: add superchain erc20 bridge (#61)
ae024a5 
* feat: add superchain erc20 bridge

* fix: interfaces and versions
@0xDiscotech
refactor: optimism superchain erc20 redesign (#62)
2a22161 
* refactor: use oz upgradeable erc20 as dependency

* chore: update interfaces

* fix: tests based on changes

* refactor: remove op as dependency

* feat: add check for supererc20 bridge on modifier

* chore: update tests and interfaces

* chore: update stack vars name on test

* chore: remove empty gitmodules file

* chore: update superchain weth errors
@agusduha
test: add superchain erc20 bridge tests (#65)
6aaf905 
* test: add superchain erc20 bridge tests

* test: add optimism superchain erc20 beacon tests

* test: remove unnecessary test

* test: tests fixes

* test: tests fixes
@0xDiscotech @agusduha
chore: update missing bridge on natspec (#69)
11fda67 
* chore: update missing bridge on natspec

* fix: natspecs

---------

Co-authored-by: agusduha <agusnduha@gmail.com>
@agusduha
fix: remove superchain erc20 base (#70)
7a69a7e
@0xDiscotech @agusduha
refactor: update isuperchainweth (#71)
07c9c77 
---------

Co-authored-by: agusduha <agusnduha@gmail.com>
@agusduha @0xDiscotech
feat: rename mint/burn and add SuperchainERC20 (#74)
d9bdad1 
* refactor: rename mint and burn functions on superchain erc20

* chore: rename optimism superchain erc20 to superchain erc20

* feat: create optimism superchain erc20 contract

* chore: update natspec and errors

* fix: superchain erc20 tests

* refactor: make superchain erc20 abstract

* refactor: move storage and erc20 metadata functions to implementation

* chore: update interfaces

* chore: update superchain erc20 events

* fix: tests

* fix: natspecs

* fix: add semmver lock and snapshots

* fix: remove unused imports

* fix: natspecs

---------

Co-authored-by: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com>
@agusduha
fix: refactor zero check (#76)
2e507b2
@agusduha
Merge branch 'develop' into fix/redesign-merge-conflicts
7fcaf6c
@agusduha
fix: pre pr
619f23c
@agusduha
Merge pull request #77 from defi-wonderland/fix/redesign-merge-conflicts
1981789 
fix: update redesign with dev
@agusduha
fix: semver natspec check failure (#79)
ae019b8 
* fix: semver natspec check failure

* fix: ignore mock contracts in semver natspec script

* fix: error message
@agusduha
feat: add crosschain erc20 interface (#80)
a5aca61 
* feat: add crosschain erc20 interface

* fix: refactor interfaces
@agusduha
fix: superchain bridge natspec (#83)
83757ea
@agusduha @0xng
@0xParticle @gotzenx
fix: superchain weth natspec (#84)
904a0ce 
Co-authored-by: 0xng <ng@defi.sucks>
Co-authored-by: 0xParticle <particle@defi.sucks>
Co-authored-by: gotzenx <78360669+gotzenx@users.noreply.github.com>
@agusduha agusduha requested a review from a team as a code owner October 4, 2024 14:11
@agusduha agusduha mentioned this pull request Oct 4, 2024
Closed
@agusduha
fix: stop inheriting superchain interfaces (#85)
3ebb1d1 
* fix: stop inheriting superchain interfaces

* fix: move events and erros into the implementation

* fix: make superchainERC20 inherits from crosschainERC20
@tynes
Copy link
Contributor

tynes commented Oct 4, 2024

What is the forge-std update from?

@skeletor-spaceman
Copy link
Collaborator

Hi @Dexaran ! I'll echo what I replied on the other issue: #12322 (comment)
This is not a valid security disclosure, it does not affect the standard as is. The erc20 code will remain unchanged.

mds1
mds1 reviewed Oct 9, 2024
View reviewed changes
Copy link
Contributor

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

Just a skim of the diff with a few small comments

packages/contracts-bedrock/scripts/checks/semver-natspec/main.go Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/L2/interfaces/IOptimismSuperchainERC20.sol
/// @notice Allows the L2StandardBridge to mint tokens.
/// @param _to Address to mint tokens to.
/// @param _amount Amount of tokens to mint.
interface IOptimismSuperchainERC20 is ISuperchainERC20 {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the SuperchainERC20 interface compared to the CrosschainERC20 interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has specific errors of the abstract implementation SuperchainERC20 that extends ICrosschainERC20. We extend it here so that the OptimismSuperchainERC20 has the correct ABI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I thought I heard that SuperchainERC20 was being renamed to CrosschainERC20, and therefore I'd only expect to see CrosschainERC20 throughout this PR. I may be out of the loop there though, so just want to better understand the relationship between the two

Copy link
Contributor

Choose a reason for hiding this comment

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

Unresolved this since I still see both and want to make sure I understand the decision/architecture here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SuperchainERC20 is a specific implementation of the ICrosschainERC20 designed for the Optimism ecosystem. It’s called Superchain because it relies on the SuperchainTokenBridge for minting and burning, which is only necessary within the Superchain. Other L2 networks will need to implement the ICrosschainERC20 in their own way.

packages/contracts-bedrock/src/L2/interfaces/ICrosschainERC20.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/L2/SuperchainTokenBridge.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/L2/SuperchainTokenBridge.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/src/L2/SuperchainWETH.sol Outdated Show resolved Hide resolved
@agusduha
fix: refactor common errors (#90)
af9694e 
* fix: refactor common errors

* fix: remove unused version
@agusduha
fix: reuse unauthorized error (#92)
6c46f9b
@tremarkley tremarkley mentioned this pull request Oct 10, 2024
Open
@tynes
Copy link
Contributor

tynes commented Oct 14, 2024

Regarding usage of interfaces, will leave it to @smartcontracts to make it simple + straight forward with ensuring that its done correctly. Given there is already a list of ignored files that don't follow the standard, this PR does not need to worry about fixing existing issues or following a particular style, as long as CI passes. ethereum-optimism/design-docs#135 will help to further the efforts of ensuring that the interfaces are used consistently

@tynes tynes mentioned this pull request Oct 14, 2024
Open
tynes
tynes reviewed Oct 14, 2024
View reviewed changes
Copy link
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

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

Test coverage is very thorough

tynes
tynes approved these changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

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

Test coverage is very thorough

@tynes tynes enabled auto-merge October 15, 2024 18:04
@tynes tynes added this pull request to the merge queue Oct 15, 2024
@tynes tynes removed this pull request from the merge queue due to a manual request Oct 15, 2024
@mslipper mslipper added this pull request to the merge queue Oct 15, 2024
Merged via the queue into ethereum-optimism:develop with commit cb2066b Oct 15, 2024
58 checks passed
This was referenced Oct 15, 2024
Merged
Open
@mds1 mds1 mentioned this pull request Oct 16, 2024
Open
mds1
mds1 reviewed Oct 16, 2024
View reviewed changes
packages/contracts-bedrock/src/L2/OptimismSuperchainERC20Factory.sol

/// @notice Mapping of the deployed OptimismSuperchainERC20 to the remote token address.
/// This is used to keep track of the token deployments.
mapping(address _localToken => address remoteToken_) public deployments;
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming nits:

  1. Is there a benefit for using underscores in named mappings? The name can't be used anywhere AFAIK and is only for readability, so may as well remove the underscores
  2. Naming the second arg in a mapping always seemed redundant to me, because it's the same as the variable. In other words, it feels confusing to me that remoteToken_ and deployments are the same thing—doesn't this mean we're giving two variable names to the same value? Using:
mapping(address _localToken => address) public deployments;

or

mapping(address _localToken => address) public remoteTokens;

feels more clear to me

mds1
mds1 reviewed Oct 16, 2024
View reviewed changes
packages/contracts-bedrock/src/L2/SuperchainERC20.sol

uint256 source = IL2ToL2CrossDomainMessenger(MESSENGER).crossDomainMessageSource();
emit CrosschainMinted(_to, _amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

another naming nit, should this be present tense of CrosschainMint for consistency with the commong methodName/EventNames used in ERC20s of transfer/Transfer and mint/Mint? Guessing we've bikeshed on this before, so this (like my other comments so far) is not super important

mds1
mds1 reviewed Oct 16, 2024
View reviewed changes
packages/contracts-bedrock/src/L2/SuperchainERC20.sol

emit RelayERC20(_from, _to, _amount, source);
emit CrosschainBurnt(_from, _amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

same as https://github.com/ethereum-optimism/optimism/pull/12321/files#r1802203673

@gotzenx gotzenx mentioned this pull request Oct 16, 2024
Open
This was referenced Oct 16, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tynes tynes tynes approved these changes

@mds1 mds1 mds1 left review comments

@maurelian maurelian Awaiting requested review from maurelian maurelian was automatically assigned from ethereum-optimism/contract-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SuperchainERC20: Redesign
7 participants
@agusduha @tynes @Dexaran @skeletor-spaceman @mds1 @mslipper @0xDiscotech