-
Notifications
You must be signed in to change notification settings - Fork 3.9k
refactor[contracts]: Port OVM_ProxyEOA to use ovm-solc #545
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@eth-optimism/contracts": patch | ||
| --- | ||
|
|
||
| Ports OVM_ProxyEOA to use optimistic-solc instead of the standard solc compiler. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,17 @@ | ||
| // SPDX-License-Identifier: MIT | ||
| // @unsupported: evm | ||
| pragma solidity >0.5.0 <0.8.0; | ||
|
|
||
| /* Library Imports */ | ||
| import { Lib_Bytes32Utils } from "../../libraries/utils/Lib_Bytes32Utils.sol"; | ||
| import { Lib_OVMCodec } from "../../libraries/codec/Lib_OVMCodec.sol"; | ||
| import { Lib_ECDSAUtils } from "../../libraries/utils/Lib_ECDSAUtils.sol"; | ||
| import { Lib_SafeExecutionManagerWrapper } from "../../libraries/wrappers/Lib_SafeExecutionManagerWrapper.sol"; | ||
|
|
||
| /** | ||
| * @title OVM_ProxyEOA | ||
| * @dev The Proxy EOA contract uses a delegate call to execute the logic in an implementation contract. | ||
| * In combination with the logic implemented in the ECDSA Contract Account, this enables a form of upgradable | ||
| * 'account abstraction' on layer 2. | ||
| * | ||
| * Compiler used: solc | ||
| * Compiler used: optimistic-solc | ||
| * Runtime target: OVM | ||
| */ | ||
| contract OVM_ProxyEOA { | ||
|
|
@@ -22,48 +20,34 @@ contract OVM_ProxyEOA { | |
| * Constants * | ||
| *************/ | ||
|
|
||
| address constant DEFAULT_IMPLEMENTATION = 0x4200000000000000000000000000000000000003; | ||
| bytes32 constant IMPLEMENTATION_KEY = 0xdeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeaddead; | ||
|
|
||
|
|
||
| /*************** | ||
| * Constructor * | ||
| ***************/ | ||
|
|
||
| /** | ||
| * @param _implementation Address of the initial implementation contract. | ||
| */ | ||
| constructor( | ||
| address _implementation | ||
| ) | ||
| { | ||
| _setImplementation(_implementation); | ||
| } | ||
|
|
||
|
|
||
| /********************* | ||
| * Fallback Function * | ||
| *********************/ | ||
|
|
||
| fallback() | ||
| external | ||
| { | ||
| (bool success, bytes memory returndata) = Lib_SafeExecutionManagerWrapper.safeDELEGATECALL( | ||
| gasleft(), | ||
| getImplementation(), | ||
| msg.data | ||
| ); | ||
| (bool success, bytes memory returndata) = getImplementation().delegatecall(msg.data); | ||
|
|
||
| if (success) { | ||
| assembly { | ||
| return(add(returndata, 0x20), mload(returndata)) | ||
| } | ||
| } else { | ||
maurelian marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Lib_SafeExecutionManagerWrapper.safeREVERT( | ||
| string(returndata) | ||
| ); | ||
| assembly { | ||
| revert(add(returndata, 0x20), mload(returndata)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // WARNING: We use the deployed bytecode of this contract as a template to create ProxyEOA | ||
| // contracts. As a result, we must *not* perform any constructor logic. Use initialization | ||
| // functions if necessary. | ||
|
|
||
|
|
||
| /******************** | ||
| * Public Functions * | ||
|
|
@@ -78,8 +62,8 @@ contract OVM_ProxyEOA { | |
| ) | ||
| external | ||
| { | ||
| Lib_SafeExecutionManagerWrapper.safeREQUIRE( | ||
| Lib_SafeExecutionManagerWrapper.safeADDRESS() == Lib_SafeExecutionManagerWrapper.safeCALLER(), | ||
| require( | ||
| msg.sender == address(this), | ||
| "EOAs can only upgrade their own EOA implementation" | ||
| ); | ||
|
|
||
|
|
@@ -96,11 +80,17 @@ contract OVM_ProxyEOA { | |
| address | ||
| ) | ||
| { | ||
| return Lib_Bytes32Utils.toAddress( | ||
| Lib_SafeExecutionManagerWrapper.safeSLOAD( | ||
| IMPLEMENTATION_KEY | ||
| ) | ||
| ); | ||
| bytes32 addr32; | ||
| assembly { | ||
| addr32 := sload(IMPLEMENTATION_KEY) | ||
| } | ||
|
|
||
| address implementation = Lib_Bytes32Utils.toAddress(addr32); | ||
| if (implementation == address(0)) { | ||
| return DEFAULT_IMPLEMENTATION; | ||
| } else { | ||
| return implementation; | ||
| } | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -113,9 +103,9 @@ contract OVM_ProxyEOA { | |
| ) | ||
| internal | ||
| { | ||
| Lib_SafeExecutionManagerWrapper.safeSSTORE( | ||
| IMPLEMENTATION_KEY, | ||
| Lib_Bytes32Utils.fromAddress(_implementation) | ||
| ); | ||
| bytes32 addr32 = Lib_Bytes32Utils.fromAddress(_implementation); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I double checked the test files to make sure these are aligned correctly, looks good. |
||
| assembly { | ||
| sstore(IMPLEMENTATION_KEY, addr32) | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,6 @@ import { iOVM_SafetyChecker } from "../../iOVM/execution/iOVM_SafetyChecker.sol" | |
|
|
||
| /* Contract Imports */ | ||
| import { OVM_ECDSAContractAccount } from "../accounts/OVM_ECDSAContractAccount.sol"; | ||
| import { OVM_ProxyEOA } from "../accounts/OVM_ProxyEOA.sol"; | ||
| import { OVM_DeployerWhitelist } from "../predeploys/OVM_DeployerWhitelist.sol"; | ||
|
|
||
| /** | ||
|
|
@@ -534,9 +533,25 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver { | |
| address prevADDRESS = messageContext.ovmADDRESS; | ||
| messageContext.ovmADDRESS = eoa; | ||
|
|
||
| // Now actually create the account and get its bytecode. We're not worried about reverts | ||
| // (other than out of gas, which we can't capture anyway) because this contract is trusted. | ||
| OVM_ProxyEOA proxyEOA = new OVM_ProxyEOA(0x4200000000000000000000000000000000000003); | ||
| // Creates a duplicate of the OVM_ProxyEOA located at 0x42....09. Uses the following | ||
smartcontracts marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // "magic" prefix to deploy an exact copy of the code: | ||
| // PUSH1 0x0D # size of this prefix in bytes | ||
| // CODESIZE | ||
| // SUB # subtract prefix size from codesize | ||
| // DUP1 | ||
| // PUSH1 0x0D | ||
| // PUSH1 0x00 | ||
| // CODECOPY # copy everything after prefix into memory at pos 0 | ||
| // PUSH1 0x00 | ||
| // RETURN # return the copied code | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ben-chain would love to have your eyes here |
||
| address proxyEOA = Lib_EthUtils.createContract(abi.encodePacked( | ||
| hex"600D380380600D6000396000f3", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed that this does indeed decompile to the opcodes in the comment above
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This bytestring looks good; it would also be super obvious from integration tests if it weren't looking. Since we do not have constructors, then this code could obviously also be put in place without a constructor. I assume we are going this way for ease of implementation in geth, right @smartcontracts? if so, we could use the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call on using SETCODE for this in the future |
||
| ovmEXTCODECOPY( | ||
| 0x4200000000000000000000000000000000000009, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Recommend to resolve these addresses |
||
| 0, | ||
| ovmEXTCODESIZE(0x4200000000000000000000000000000000000009) | ||
| ) | ||
| )); | ||
|
|
||
| // Reset the address now that we're done deploying. | ||
| messageContext.ovmADDRESS = prevADDRESS; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -254,5 +254,8 @@ export const makeContractDeployConfig = async ( | |
| ERC1820Registry: { | ||
| factory: getContractFactory('ERC1820Registry'), | ||
| }, | ||
| OVM_ProxyEOA: { | ||
| factory: getContractFactory('OVM_ProxyEOA', undefined, true), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are these additional arguments?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that we should make these additional arguments an object. Although probably deserves its own issue. |
||
| }, | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ import { | |
| } from '../constants' | ||
| import { getStorageXOR } from '../' | ||
| import { UNSAFE_BYTECODE } from '../dummy' | ||
| import { getContractFactory } from '../../../src' | ||
|
|
||
| export class ExecutionManagerTestRunner { | ||
| private snapshot: string | ||
|
|
@@ -47,13 +48,15 @@ export class ExecutionManagerTestRunner { | |
| Helper_TestRunner: Contract | ||
| Factory__Helper_TestRunner_CREATE: ContractFactory | ||
| OVM_DeployerWhitelist: Contract | ||
| OVM_ProxyEOA: Contract | ||
| } = { | ||
| OVM_SafetyChecker: undefined, | ||
| OVM_StateManager: undefined, | ||
| OVM_ExecutionManager: undefined, | ||
| Helper_TestRunner: undefined, | ||
| Factory__Helper_TestRunner_CREATE: undefined, | ||
| OVM_DeployerWhitelist: undefined, | ||
| OVM_ProxyEOA: undefined, | ||
| } | ||
|
|
||
| // Default pre-state with contract deployer whitelist NOT initialized. | ||
|
|
@@ -65,6 +68,10 @@ export class ExecutionManagerTestRunner { | |
| codeHash: NON_NULL_BYTES32, | ||
| ethAddress: '$OVM_DEPLOYER_WHITELIST', | ||
| }, | ||
| ['0x4200000000000000000000000000000000000009']: { | ||
| codeHash: NON_NULL_BYTES32, | ||
| ethAddress: '$OVM_PROXY_EOA', | ||
| }, | ||
| }, | ||
| contractStorage: { | ||
| ['0x4200000000000000000000000000000000000002']: { | ||
|
|
@@ -217,6 +224,12 @@ export class ExecutionManagerTestRunner { | |
|
|
||
| this.contracts.OVM_DeployerWhitelist = DeployerWhitelist | ||
|
|
||
| this.contracts.OVM_ProxyEOA = await getContractFactory( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we deploy the reference version of the |
||
| 'OVM_ProxyEOA', | ||
| AddressManager.signer, | ||
| true | ||
| ).deploy() | ||
|
|
||
| this.contracts.OVM_ExecutionManager = await ( | ||
| await smoddit('OVM_ExecutionManager') | ||
| ).deploy( | ||
|
|
@@ -266,6 +279,8 @@ export class ExecutionManagerTestRunner { | |
| return this.contracts.Helper_TestRunner.address | ||
| } else if (kv === '$OVM_DEPLOYER_WHITELIST') { | ||
| return this.contracts.OVM_DeployerWhitelist.address | ||
| } else if (kv === '$OVM_PROXY_EOA') { | ||
| return this.contracts.OVM_ProxyEOA.address | ||
| } else if (kv.startsWith('$DUMMY_OVM_ADDRESS_')) { | ||
| return ExecutionManagerTestRunner.getDummyAddress(kv) | ||
| } else { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for a separate PR, but harping again that this should really use this EIP here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should use that EIP, note that this will also break our regenesis script
https://github.com/ethereum-optimism/scripts/blob/main/scripts/state-surgery.js
Will add an issue over there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ethereum-optimism/scripts#12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue here: #550