-
Notifications
You must be signed in to change notification settings - Fork 3.9k
feat[contracts]: Use standard RLP transaction format #566
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
d7c8ab1
399c0e2
d8d1fa1
fb79895
5bd50f7
c615b87
cbbbd01
34c7632
42e9e8d
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 |
|---|---|---|
|
|
@@ -7,8 +7,7 @@ pragma experimental ABIEncoderV2; | |
| 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_EIP155Tx } from "../../libraries/codec/Lib_EIP155Tx.sol"; | ||
| import { Lib_ExecutionManagerWrapper } from "../../libraries/wrappers/Lib_ExecutionManagerWrapper.sol"; | ||
|
|
||
| /* Contract Imports */ | ||
|
|
@@ -21,13 +20,20 @@ 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. | ||
| * providing EIP155 formatted transaction encodings. | ||
| * | ||
| * Compiler used: optimistic-solc | ||
| * Runtime target: OVM | ||
| */ | ||
| contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount { | ||
|
|
||
| /************* | ||
| * Libraries * | ||
| *************/ | ||
|
|
||
| using Lib_EIP155Tx for Lib_EIP155Tx.EIP155Tx; | ||
|
|
||
|
|
||
| /************* | ||
| * Constants * | ||
| *************/ | ||
|
|
@@ -44,20 +50,12 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount { | |
|
|
||
| /** | ||
| * Executes a signed transaction. | ||
| * @param _transaction Signed EOA transaction. | ||
| * @param _signatureType Hashing scheme used for the transaction (e.g., ETH signed message). | ||
| * @param _v Signature `v` parameter. | ||
| * @param _r Signature `r` parameter. | ||
| * @param _s Signature `s` parameter. | ||
| * @param _encodedTransaction Signed EIP155 transaction. | ||
| * @return Whether or not the call returned (rather than reverted). | ||
| * @return Data returned by the call. | ||
| */ | ||
| function execute( | ||
| bytes memory _transaction, | ||
| Lib_OVMCodec.EOASignatureType _signatureType, | ||
| uint8 _v, | ||
| bytes32 _r, | ||
| bytes32 _s | ||
| bytes memory _encodedTransaction | ||
| ) | ||
| override | ||
| public | ||
|
|
@@ -66,66 +64,46 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount { | |
| bytes memory | ||
| ) | ||
| { | ||
| bool isEthSign = _signatureType == Lib_OVMCodec.EOASignatureType.ETH_SIGNED_MESSAGE; | ||
| // Attempt to decode the transaction. | ||
| Lib_EIP155Tx.EIP155Tx memory transaction = Lib_EIP155Tx.decode( | ||
| _encodedTransaction, | ||
| Lib_ExecutionManagerWrapper.ovmCHAINID() | ||
smartcontracts marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ); | ||
|
|
||
| // Address of this contract within the ovm (ovmADDRESS) should be the same as the | ||
| // 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. | ||
| require( | ||
| Lib_ECDSAUtils.recover( | ||
| _transaction, | ||
| isEthSign, | ||
| _v, | ||
| _r, | ||
| _s | ||
| ) == address(this), | ||
| transaction.sender() == address(this), | ||
|
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. Notably the way this code path is currently done is that we re-encode the raw RLP within A quick aside: can you briefly talk about the reasoning for making the
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. Hmmm yeah interesting, I hadn't thought of passing the
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. Ok looking at this now, this introduces enough of a diff that we may want to leave it for another PR + treat it as an optimization if this method is too expensive.
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. Yeah, it's a bit of a tradeoff. Ultimately the most gas-efficient version could just require |
||
| "Signature provided for EOA transaction execution is invalid." | ||
| ); | ||
|
|
||
| Lib_OVMCodec.EIP155Transaction memory decodedTx = Lib_OVMCodec.decodeEIP155Transaction( | ||
| _transaction, | ||
| isEthSign | ||
| ); | ||
|
|
||
| // Grab the chain ID of the current network. | ||
| uint256 chainId; | ||
| assembly { | ||
| chainId := chainid() | ||
| } | ||
|
|
||
| // Need to make sure that the transaction chainId is correct. | ||
| require( | ||
| decodedTx.chainId == chainId, | ||
| "Transaction chainId does not match expected OVM chainId." | ||
| ); | ||
|
|
||
| // Need to make sure that the transaction nonce is right. | ||
| require( | ||
| decodedTx.nonce == Lib_ExecutionManagerWrapper.ovmGETNONCE(), | ||
| transaction.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. | ||
| // require( | ||
| // gasleft() >= SafeMath.add(decodedTx.gasLimit, EXECUTION_VALIDATION_GAS_OVERHEAD), | ||
| // gasleft() >= SafeMath.add(transaction.gasLimit, EXECUTION_VALIDATION_GAS_OVERHEAD), | ||
| // "Gas is not sufficient to execute the transaction." | ||
| // ); | ||
|
|
||
| // Transfer fee to relayer. | ||
| require( | ||
| ovmETH.transfer( | ||
| msg.sender, | ||
| SafeMath.mul(decodedTx.gasLimit, decodedTx.gasPrice) | ||
| SafeMath.mul(transaction.gasLimit, transaction.gasPrice) | ||
| ), | ||
| "Fee was not transferred to relayer." | ||
| ); | ||
|
|
||
| // Contract creations are signalled by sending a transaction to the zero address. | ||
| if (decodedTx.to == address(0)) { | ||
| if (transaction.isCreate) { | ||
| (address created, bytes memory revertdata) = Lib_ExecutionManagerWrapper.ovmCREATE( | ||
| decodedTx.data | ||
| transaction.data | ||
| ); | ||
|
|
||
| // Return true if the contract creation succeeded, false w/ revertdata otherwise. | ||
|
|
@@ -140,7 +118,7 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount { | |
| // cases, but since this is a contract we'd end up bumping the nonce twice. | ||
| Lib_ExecutionManagerWrapper.ovmINCREMENTNONCE(); | ||
|
|
||
| return decodedTx.to.call(decodedTx.data); | ||
| return transaction.to.call(transaction.data); | ||
| } | ||
| } | ||
| } | ||
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.
Technically the better way to do this is use the raw RLP sent via RPC so that it doesn't need to be reserialized here. The
txmetais created ineth_sendRawTransactionhandler andrawTransactioncan be set thereThere 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.
How would you do this?
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.
This is the line that needs updating:
optimism/l2geth/internal/ethapi/api.go
Line 1738 in eef1df4
The RLP encoded tx is in scope as a
hexutil.BytesThere 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.
When the
RollupClientfetches a tx, it sets therawTransactionhere:optimism/l2geth/rollup/client.go
Line 329 in eef1df4