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: disable ETH ERC20 features #1450

Merged
merged 2 commits into from
Sep 15, 2021
Merged

feat: disable ETH ERC20 features #1450

merged 2 commits into from
Sep 15, 2021

Conversation

smartcontracts
Copy link
Contributor

@smartcontracts smartcontracts commented Sep 13, 2021 •
edited
Loading

Description
Disables ERC20 and WETH9 features in OVM_ETH, moves the contract to 0xdeaddead....0000.

Metadata

  • Fixes ENG-1365

@changeset-bot
Copy link

changeset-bot bot commented Sep 13, 2021 •
edited
Loading

🦋 Changeset detected

Latest commit: ebc5e529581bb026272d7a7731d39037caa1f753

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@eth-optimism/integration-tests Minor
@eth-optimism/l2geth Minor
@eth-optimism/contracts Minor
@eth-optimism/batch-submitter Patch
@eth-optimism/data-transport-layer Patch
@eth-optimism/message-relayer Patch
@eth-optimism/regenesis-surgery Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@smartcontracts smartcontracts marked this pull request as draft September 13, 2021 15:58
@smartcontracts smartcontracts marked this pull request as ready for review September 13, 2021 19:35
@smartcontracts
Copy link
Contributor Author

smartcontracts commented Sep 13, 2021 •
edited
Loading

TODO: add tests for OVM_ETH, specifically checking that ERC20 features are not accessible. Done

@smartcontracts smartcontracts force-pushed the sc/disable-ovm-eth branch 2 times, most recently from f3f54a8 to 8dda912 Compare September 14, 2021 18:01
@tynes
Copy link
Contributor

tynes commented Sep 14, 2021 •
edited
Loading

Is the idea of this PR to make it so that ETH balance shows up in the account itself instead of being in a contract? If so, I believe this needs to be updated as well. https://github.com/ethereum-optimism/optimism/blob/8dda91206e7b3b2a8f9839e278c90873f973594f/l2geth/core/state/statedb.go#L369-L379

Based on the usage of address(this).balance in OVM_SequencerFeeVault.sol, I believe this is the case

@smartcontracts
Copy link
Contributor Author

Balance will still be in the contract until further notice. Currently we need this in order to do ETH withdrawals but that will likely change in the update after this next one.

K-Ho
K-Ho reviewed Sep 15, 2021
View reviewed changes
Copy link
Contributor

@K-Ho K-Ho left a comment

Choose a reason for hiding this comment

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

Looks great! Just would suggest removing all of the ERC20 functions altogether instead of reverting for them

packages/contracts/contracts/L2/predeploys/OVM_ETH.sol Outdated Show resolved Hide resolved
packages/contracts/test/contracts/L2/predeploys/OVM_ETH.spec.ts Outdated
Comment on lines 24 to 33
describe('transfer', () => {
it('should revert', async () => {
await expect(
OVM_ETH.transfer(await signer2.getAddress(), 100)
).to.be.revertedWith(
'OVM_ETH: ERC20 features are disabled pending further community discussion.'
)
})
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above - do we even need to have these functions exist? We can just delete them + these tests for now

@smartcontracts
Copy link
Contributor Author

Looks great! Just would suggest removing all of the ERC20 functions altogether instead of reverting for them

Unfortunately I can't do this without making a huge diff because OVM_ETH inherits L2StandardERC20 which inherits ERC20 etc etc. Disabling the functions was the easiest option.

@K-Ho
Copy link
Contributor

K-Ho commented Sep 15, 2021

Looks great! Just would suggest removing all of the ERC20 functions altogether instead of reverting for them

Unfortunately I can't do this without making a huge diff because OVM_ETH inherits L2StandardERC20 which inherits ERC20 etc etc. Disabling the functions was the easiest option.

Didn't think of that - will go ahead and approve 👍

@smartcontracts smartcontracts force-pushed the sc/disable-ovm-eth branch 2 times, most recently from 33fc843 to ebc5e52 Compare September 15, 2021 02:45
@smartcontracts smartcontracts merged commit e9dfc82 into experimental Sep 15, 2021
@smartcontracts smartcontracts deleted the sc/disable-ovm-eth branch September 15, 2021 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tynes tynes tynes left review comments

@K-Ho K-Ho K-Ho approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@smartcontracts @tynes @K-Ho