-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[WIP] feat: Have predeploys use OVM solc only #475
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
7387bff
e8baf2e
a283439
36869a5
d76f0ce
3c9350c
a4f042a
e77b1cd
7881bc6
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 |
|---|---|---|
| @@ -1,23 +1,27 @@ | ||
| // SPDX-License-Identifier: MIT | ||
| // @unsupported: evm | ||
| pragma solidity >0.5.0 <0.8.0; | ||
| pragma experimental ABIEncoderV2; | ||
|
|
||
| /* Interface Imports */ | ||
| import { iOVM_ERC20 } from "../../iOVM/predeploys/iOVM_ERC20.sol"; | ||
|
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. Could this be |
||
| import { iOVM_ECDSAContractAccount } from "../../iOVM/accounts/iOVM_ECDSAContractAccount.sol"; | ||
|
|
||
| /* Library Imports */ | ||
| 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"; | ||
| import { Lib_SafeMathWrapper } from "../../libraries/wrappers/Lib_SafeMathWrapper.sol"; | ||
| import { Lib_ExecutionManagerWrapper } from "../../libraries/wrappers/Lib_ExecutionManagerWrapper.sol"; | ||
|
|
||
| /* External Imports */ | ||
| import { SafeMath } from "@openzeppelin/contracts/math/SafeMath.sol"; | ||
|
|
||
| /** | ||
| * @title OVM_ECDSAContractAccount | ||
| * @dev The ECDSA Contract Account can be used as the implementation for a ProxyEOA deployed by the | ||
| * ovmCREATEEOA operation. It enables backwards compatibility with Ethereum's Layer 1, by | ||
| * providing eth_sign and EIP155 formatted transaction encodings. | ||
| * | ||
| * Compiler used: solc | ||
| * Compiler used: optimistic-solc | ||
| * Runtime target: OVM | ||
| */ | ||
| contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount { | ||
|
|
@@ -29,7 +33,7 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount { | |
| // TODO: should be the amount sufficient to cover the gas costs of all of the transactions up | ||
| // to and including the CALL/CREATE which forms the entrypoint of the transaction. | ||
| uint256 constant EXECUTION_VALIDATION_GAS_OVERHEAD = 25000; | ||
| address constant ETH_ERC20_ADDRESS = 0x4200000000000000000000000000000000000006; | ||
| iOVM_ERC20 constant ETH_ERC20 = iOVM_ERC20(0x4200000000000000000000000000000000000006); | ||
|
|
||
|
|
||
| /******************** | ||
|
|
@@ -66,75 +70,76 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount { | |
| // recovered address of the user who signed this message. This is how we manage to shim | ||
| // account abstraction even though the user isn't a contract. | ||
| // Need to make sure that the transaction nonce is right and bump it if so. | ||
| Lib_SafeExecutionManagerWrapper.safeREQUIRE( | ||
| require( | ||
| Lib_ECDSAUtils.recover( | ||
| _transaction, | ||
| isEthSign, | ||
| _v, | ||
| _r, | ||
| _s | ||
| ) == Lib_SafeExecutionManagerWrapper.safeADDRESS(), | ||
| ) == address(this), | ||
| "Signature provided for EOA transaction execution is invalid." | ||
| ); | ||
|
|
||
| Lib_OVMCodec.EIP155Transaction memory decodedTx = Lib_OVMCodec.decodeEIP155Transaction(_transaction, isEthSign); | ||
|
|
||
| // Need to make sure that the transaction chainId is correct. | ||
| Lib_SafeExecutionManagerWrapper.safeREQUIRE( | ||
| decodedTx.chainId == Lib_SafeExecutionManagerWrapper.safeCHAINID(), | ||
| require( | ||
| decodedTx.chainId == Lib_ExecutionManagerWrapper.ovmCHAINID(), | ||
|
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. could alternatively use Yul here now; so real preference from me though. |
||
| "Transaction chainId does not match expected OVM chainId." | ||
| ); | ||
|
|
||
| // Need to make sure that the transaction nonce is right. | ||
| Lib_SafeExecutionManagerWrapper.safeREQUIRE( | ||
| decodedTx.nonce == Lib_SafeExecutionManagerWrapper.safeGETNONCE(), | ||
| require( | ||
| decodedTx.nonce == Lib_ExecutionManagerWrapper.ovmGETNONCE(), | ||
| "Transaction nonce does not match the expected nonce." | ||
| ); | ||
|
|
||
| // TEMPORARY: Disable gas checks for mainnet. | ||
| // // Need to make sure that the gas is sufficient to execute the transaction. | ||
| // Lib_SafeExecutionManagerWrapper.safeREQUIRE( | ||
| // gasleft() >= Lib_SafeMathWrapper.add(decodedTx.gasLimit, EXECUTION_VALIDATION_GAS_OVERHEAD), | ||
| // Lib_ExecutionManagerWrapper.safeREQUIRE( | ||
|
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. nit: this can be replaced with an explicit require, just like the nonce check above |
||
| // gasleft() >= SafeMath.add(decodedTx.gasLimit, EXECUTION_VALIDATION_GAS_OVERHEAD), | ||
| // "Gas is not sufficient to execute the transaction." | ||
| // ); | ||
|
|
||
| // Transfer fee to relayer. | ||
| address relayer = Lib_SafeExecutionManagerWrapper.safeCALLER(); | ||
| uint256 fee = Lib_SafeMathWrapper.mul(decodedTx.gasLimit, decodedTx.gasPrice); | ||
| (bool success, ) = Lib_SafeExecutionManagerWrapper.safeCALL( | ||
| gasleft(), | ||
| ETH_ERC20_ADDRESS, | ||
| abi.encodeWithSignature("transfer(address,uint256)", relayer, fee) | ||
| ); | ||
| Lib_SafeExecutionManagerWrapper.safeREQUIRE( | ||
| success == true, | ||
| require( | ||
| ETH_ERC20.transfer( | ||
| msg.sender, | ||
| SafeMath.mul(decodedTx.gasLimit, decodedTx.gasPrice) | ||
| ), | ||
| "Fee was not transferred to relayer." | ||
| ); | ||
|
|
||
| // Contract creations are signalled by sending a transaction to the zero address. | ||
| if (decodedTx.to == address(0)) { | ||
| (address created, bytes memory revertData) = Lib_SafeExecutionManagerWrapper.safeCREATE( | ||
| gasleft(), | ||
| decodedTx.data | ||
| ); | ||
| address created; | ||
| bytes memory code = decodedTx.data; | ||
| bytes memory revertdata; | ||
| assembly { | ||
| created := create(0, add(code, 0x20), mload(code)) | ||
| if iszero(created) { | ||
| let size := returndatasize() | ||
| revertdata := mload(0x40) | ||
| mstore(0x40, add(revertdata, and(add(add(size, 0x20), 0x1f), not(0x1f)))) | ||
| mstore(revertdata, size) | ||
| returndatacopy(add(revertdata, 0x20), 0x0, size) | ||
| } | ||
|
Comment on lines
+121
to
+127
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 block is a little brittle, at minimum I think adding comments is required here. This could have been left as-is though, right? I have mixed feelings--on one hand, this block of assembly would make no sense in the EVM context, so it is a little confusing to read, and the functionality feels very "wrapper-ey" to me. OTOH, I get that it does add overhead to a reviewer's life. |
||
| } | ||
|
|
||
| // Return true if the contract creation succeeded, false w/ revertData otherwise. | ||
| // Return true if the contract creation succeeded, false w/ revertdata otherwise. | ||
| if (created != address(0)) { | ||
| return (true, abi.encode(created)); | ||
| } else { | ||
| return (false, revertData); | ||
| return (false, revertdata); | ||
| } | ||
| } else { | ||
| // We only want to bump the nonce for `ovmCALL` because `ovmCREATE` automatically bumps | ||
| // the nonce of the calling account. Normally an EOA would bump the nonce for both | ||
| // cases, but since this is a contract we'd end up bumping the nonce twice. | ||
| Lib_SafeExecutionManagerWrapper.safeINCREMENTNONCE(); | ||
| Lib_ExecutionManagerWrapper.ovmINCREMENTNONCE(); | ||
|
|
||
| return Lib_SafeExecutionManagerWrapper.safeCALL( | ||
| gasleft(), | ||
| decodedTx.to, | ||
| decodedTx.data | ||
| ); | ||
| return decodedTx.to.call(decodedTx.data); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,17 +3,14 @@ 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 { | ||
|
|
@@ -29,14 +26,8 @@ contract OVM_ProxyEOA { | |
| * Constructor * | ||
| ***************/ | ||
|
|
||
| /** | ||
| * @param _implementation Address of the initial implementation contract. | ||
| */ | ||
| constructor( | ||
| address _implementation | ||
| ) | ||
| { | ||
| _setImplementation(_implementation); | ||
| constructor() { | ||
| _setImplementation(0x4200000000000000000000000000000000000003); | ||
|
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 is an interesting use case in terms of configurability, etc. I know this was already the status quo, but we should really update the key here to be comply with this EIP. |
||
| } | ||
|
|
||
|
|
||
|
|
@@ -47,20 +38,16 @@ contract OVM_ProxyEOA { | |
| 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 { | ||
| Lib_SafeExecutionManagerWrapper.safeREVERT( | ||
| string(returndata) | ||
| ); | ||
| assembly { | ||
| revert(add(returndata, 0x20), mload(returndata)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -78,8 +65,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 +83,11 @@ contract OVM_ProxyEOA { | |
| address | ||
| ) | ||
| { | ||
| return Lib_Bytes32Utils.toAddress( | ||
| Lib_SafeExecutionManagerWrapper.safeSLOAD( | ||
| IMPLEMENTATION_KEY | ||
| ) | ||
| ); | ||
| bytes32 addr32; | ||
| assembly { | ||
| addr32 := sload(IMPLEMENTATION_KEY) | ||
| } | ||
| return Lib_Bytes32Utils.toAddress(addr32); | ||
| } | ||
|
|
||
|
|
||
|
|
@@ -113,9 +100,9 @@ contract OVM_ProxyEOA { | |
| ) | ||
| internal | ||
| { | ||
| Lib_SafeExecutionManagerWrapper.safeSSTORE( | ||
| IMPLEMENTATION_KEY, | ||
| Lib_Bytes32Utils.fromAddress(_implementation) | ||
| ); | ||
| bytes32 addr32 = Lib_Bytes32Utils.fromAddress(_implementation); | ||
| assembly { | ||
| sstore(IMPLEMENTATION_KEY, addr32) | ||
| } | ||
| } | ||
| } | ||
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.
nit: this file diff seems unrelated and probably should have been a separate PR