-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat[contracts]: Add value transfer support to ECDSAContractAccount #619
Conversation
🦋 Changeset detectedLatest commit: 89792d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
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.
Awesome! This looks great! Looks like you're already working to get the tests to pass so that's great.
@ben-chain made the great point that we should add the value check to L2Geth to make sure we don't accept a transaction trying to send a value greater than their balance.
Additionally, after adding the quick L2Geth change it would be good to have an integration test which verifies everything is indeed working perfectly.
@@ -102,6 +102,12 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount { | |||
|
|||
// Contract creations are signalled by sending a transaction to the zero address. | |||
if (transaction.isCreate) { | |||
// TEMPORARY: Disable value transfer for contract creations. |
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.
Generally I find these TEMPORARY
comments to be a bit unhelpful to those without context.
Could you briefly describe why it's disable, or what will be required so that we can enable it?
Please rebase again on top of |
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.
Changing my review status. The comment request is not a blocker.
packages/contracts/contracts/optimistic-ethereum/libraries/rlp/Lib_RLPWriter.sol
Outdated
Show resolved
Hide resolved
67128a0
to
29240f9
Compare
* feat[contracts]: Use standard RLP transaction format * fix[l2geth]: Encode transaction as RLP * fix: Correct gas estimation in integration tests * fix: Correct gas estimation in integration tests * Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol Co-authored-by: ben-chain <ben@pseudonym.party> * fix[contracts]: Use isCreate instead of checking target address * fix[contracts]: Minor optimization in SequencerEntrypoint * fix[contracts]: Pass max gas to contract call in EOA contract Co-authored-by: ben-chain <ben@pseudonym.party>
29240f9
to
bd0dd6d
Compare
* feat: Attempt to decode txs as RLP first (#563) Co-authored-by: smartcontracts <smartcontracts@doge.org> * l2geth: remove eth_sendRawEthSignTransaction endpoint (#589) * feat[contracts]: Use standard RLP transaction format (#566) * feat[contracts]: Use standard RLP transaction format * fix[l2geth]: Encode transaction as RLP * fix: Correct gas estimation in integration tests * fix: Correct gas estimation in integration tests * Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol Co-authored-by: ben-chain <ben@pseudonym.party> * fix[contracts]: Use isCreate instead of checking target address * fix[contracts]: Minor optimization in SequencerEntrypoint * fix[contracts]: Pass max gas to contract call in EOA contract Co-authored-by: ben-chain <ben@pseudonym.party> * feat[contracts]: Make ProxyEOA compatible with eip1967 (#592) * feat[contracts]: Make ProxyEOA compatible with eip1967 * fix[contracts]: Fix bug introduced by indirect constant * chore[contracts]: Add changeset * Update .changeset/old-cycles-invite.md Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> * l2geth: remove ovmsigner (#591) * l2geth: remove ovmsigner Also reduce the diff Co-authored-by: smartcontracts * l2geth: add changeset * l2geth: set rlp encoded tx in txmeta in RPC layer (#644) * l2geth: set rlp encoded tx in txmeta in RPC layer * l2geth: remove extra setter of txmeta * chore: add changeset * feat: Have ExecutionManager pass data upwards (#643) * feat[contracts]: Make ExecutionManager return data * fix[l2geth]: fix linting error * fix[contracts]: Fix build error * fix[contracts]: fix failing unit tests * Add changeset Co-authored-by: Karl Floersch <karl@karlfloersch.com> * rpc: only allow txs with no calldata when there is value (#645) * l2geth: api checks for 0 value * chore: add changeset * l2geth: remove check for specific gasprice * feat[contracts]: Add value transfer support to ECDSAContractAccount (#619) * feat[contracts]: Use standard RLP transaction format (#566) * feat[contracts]: Use standard RLP transaction format * fix[l2geth]: Encode transaction as RLP * fix: Correct gas estimation in integration tests * fix: Correct gas estimation in integration tests * Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol Co-authored-by: ben-chain <ben@pseudonym.party> * fix[contracts]: Use isCreate instead of checking target address * fix[contracts]: Minor optimization in SequencerEntrypoint * fix[contracts]: Pass max gas to contract call in EOA contract Co-authored-by: ben-chain <ben@pseudonym.party> * feat[contracts]: Add value transfer to contract account * fix[contracts]: Tweak transfer logic and add tests * fix[geth]: Remove logic that rejects value gt 0 txs * fix: nonce issue in rpc tests * fix: use correct wallet in rpc value tests * Update rpc.spec.ts * cleanup: remove double definition * chore: add changeset * chore: add changeset * tests: delete dead test * l2geth: log the tx value * l2geth: pass through zero value at top level * test: receipt passes * test: more specifically set balance Co-authored-by: ben-chain <ben@pseudonym.party> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> * dtl: remove legacy encoding (#618) * dtl: remove legacy decoding * tests: remove dead test * chore: add changeset * Add Goerli v3 deployment (#651) * Add Goerli v3 deployment * Add Goerli v3 to README * dtlL fix syncing off by one (#687) * dtl: syncing off by one error * chore: add changeset * dtl: index the value field (#686) * chore: add changeset * chore: add changeset * dtl: pass through value field * core-utils: update and test toRpcString * lint: fix * l2geth: parse value fields * chore: add changeset * rpc: gas fixes (#695) * l2geth: prevent fees lower than 21000 * l2geth: remove old check for too high tx gaslimit * tests: update to use new min gas estimated value * chore: add changeset * test: update expected values * test: remove dead test * examples: fix waffle example + gas changes in tests (#724) * examples: fix waffle example * tests: update gas price in assertion * chore: add changeset * l2geth: estimate gas assertion in decimal * test: use configurable key * ops: delete extra whitespace (#731) * fix: prevent eth sendtransaction (#725) * api: prevent unsafe calls * api: fill in txmeta * chore: add changeset * chore: add changeset * l2geth + contracts: standard interface for systems contracts and userland contracts (#721) * l2geth: fix call returndata parsing * contracts: standardize simulateMessage and run to return bytes * chore: add changeset * chore: add changeset * l2geth: more simple decoding * contracts: remove named arguments * chore: fix linter errors * Add contract deployment to Kovan (#715) * fix: remove type check in rollup client (#750) * l2geth: remove tx type check in client * chore: add changeset * dtl: prevent null reference in L1 handler (#757) * dtl: prevent reference of null value * chore: add changeset * test: eth_call exceptions (#800) * feat[l2geth]: Pass up contract revert reasons during DoEstimateGas (#774) * wip: Starting work on geth revert reasons during estimate gas fix: error in comment fix: I got things backwards fix: Use UnpackValues instead of Unpack Update l2geth/accounts/abi/abi.go Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> * Add integration test for reverts fix: build error * chore: Add changeset Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> * chore: add changeset (#831) * Migrate ETH between gateways (#778) * add migrate ETH functionality * contracts: add eth gateway docstring (#832) Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> Co-authored-by: smartcontracts <smartcontracts@doge.org> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> Co-authored-by: smartcontracts <kelvinfichter@gmail.com> Co-authored-by: ben-chain <ben@pseudonym.party> Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> Co-authored-by: Maurelian <maurelian@protonmail.ch> Co-authored-by: Kevin Ho <kevinjho1996@gmail.com>
…thereum-optimism#619) * feat[contracts]: Use standard RLP transaction format (ethereum-optimism#566) * feat[contracts]: Use standard RLP transaction format * fix[l2geth]: Encode transaction as RLP * fix: Correct gas estimation in integration tests * fix: Correct gas estimation in integration tests * Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol Co-authored-by: ben-chain <ben@pseudonym.party> * fix[contracts]: Use isCreate instead of checking target address * fix[contracts]: Minor optimization in SequencerEntrypoint * fix[contracts]: Pass max gas to contract call in EOA contract Co-authored-by: ben-chain <ben@pseudonym.party> * feat[contracts]: Add value transfer to contract account * fix[contracts]: Tweak transfer logic and add tests * fix[geth]: Remove logic that rejects value gt 0 txs * fix: nonce issue in rpc tests * fix: use correct wallet in rpc value tests * Update rpc.spec.ts * cleanup: remove double definition * chore: add changeset * chore: add changeset * tests: delete dead test * l2geth: log the tx value * l2geth: pass through zero value at top level * test: receipt passes * test: more specifically set balance Co-authored-by: ben-chain <ben@pseudonym.party> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
* feat: Attempt to decode txs as RLP first (#563) Co-authored-by: smartcontracts <smartcontracts@doge.org> * l2geth: remove eth_sendRawEthSignTransaction endpoint (#589) * feat[contracts]: Use standard RLP transaction format (#566) * feat[contracts]: Use standard RLP transaction format * fix[l2geth]: Encode transaction as RLP * fix: Correct gas estimation in integration tests * fix: Correct gas estimation in integration tests * Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol Co-authored-by: ben-chain <ben@pseudonym.party> * fix[contracts]: Use isCreate instead of checking target address * fix[contracts]: Minor optimization in SequencerEntrypoint * fix[contracts]: Pass max gas to contract call in EOA contract Co-authored-by: ben-chain <ben@pseudonym.party> * feat[contracts]: Make ProxyEOA compatible with eip1967 (#592) * feat[contracts]: Make ProxyEOA compatible with eip1967 * fix[contracts]: Fix bug introduced by indirect constant * chore[contracts]: Add changeset * Update .changeset/old-cycles-invite.md Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> * l2geth: remove ovmsigner (#591) * l2geth: remove ovmsigner Also reduce the diff Co-authored-by: smartcontracts * l2geth: add changeset * l2geth: set rlp encoded tx in txmeta in RPC layer (ethereum-optimism#644) * l2geth: set rlp encoded tx in txmeta in RPC layer * l2geth: remove extra setter of txmeta * chore: add changeset * feat: Have ExecutionManager pass data upwards (ethereum-optimism#643) * feat[contracts]: Make ExecutionManager return data * fix[l2geth]: fix linting error * fix[contracts]: Fix build error * fix[contracts]: fix failing unit tests * Add changeset Co-authored-by: Karl Floersch <karl@karlfloersch.com> * rpc: only allow txs with no calldata when there is value (ethereum-optimism#645) * l2geth: api checks for 0 value * chore: add changeset * l2geth: remove check for specific gasprice * feat[contracts]: Add value transfer support to ECDSAContractAccount (ethereum-optimism#619) * feat[contracts]: Use standard RLP transaction format (#566) * feat[contracts]: Use standard RLP transaction format * fix[l2geth]: Encode transaction as RLP * fix: Correct gas estimation in integration tests * fix: Correct gas estimation in integration tests * Update packages/contracts/contracts/optimistic-ethereum/OVM/predeploys/OVM_SequencerEntrypoint.sol Co-authored-by: ben-chain <ben@pseudonym.party> * fix[contracts]: Use isCreate instead of checking target address * fix[contracts]: Minor optimization in SequencerEntrypoint * fix[contracts]: Pass max gas to contract call in EOA contract Co-authored-by: ben-chain <ben@pseudonym.party> * feat[contracts]: Add value transfer to contract account * fix[contracts]: Tweak transfer logic and add tests * fix[geth]: Remove logic that rejects value gt 0 txs * fix: nonce issue in rpc tests * fix: use correct wallet in rpc value tests * Update rpc.spec.ts * cleanup: remove double definition * chore: add changeset * chore: add changeset * tests: delete dead test * l2geth: log the tx value * l2geth: pass through zero value at top level * test: receipt passes * test: more specifically set balance Co-authored-by: ben-chain <ben@pseudonym.party> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> * dtl: remove legacy encoding (ethereum-optimism#618) * dtl: remove legacy decoding * tests: remove dead test * chore: add changeset * Add Goerli v3 deployment (ethereum-optimism#651) * Add Goerli v3 deployment * Add Goerli v3 to README * dtlL fix syncing off by one (ethereum-optimism#687) * dtl: syncing off by one error * chore: add changeset * dtl: index the value field (ethereum-optimism#686) * chore: add changeset * chore: add changeset * dtl: pass through value field * core-utils: update and test toRpcString * lint: fix * l2geth: parse value fields * chore: add changeset * rpc: gas fixes (ethereum-optimism#695) * l2geth: prevent fees lower than 21000 * l2geth: remove old check for too high tx gaslimit * tests: update to use new min gas estimated value * chore: add changeset * test: update expected values * test: remove dead test * examples: fix waffle example + gas changes in tests (ethereum-optimism#724) * examples: fix waffle example * tests: update gas price in assertion * chore: add changeset * l2geth: estimate gas assertion in decimal * test: use configurable key * ops: delete extra whitespace (ethereum-optimism#731) * fix: prevent eth sendtransaction (ethereum-optimism#725) * api: prevent unsafe calls * api: fill in txmeta * chore: add changeset * chore: add changeset * l2geth + contracts: standard interface for systems contracts and userland contracts (ethereum-optimism#721) * l2geth: fix call returndata parsing * contracts: standardize simulateMessage and run to return bytes * chore: add changeset * chore: add changeset * l2geth: more simple decoding * contracts: remove named arguments * chore: fix linter errors * Add contract deployment to Kovan (ethereum-optimism#715) * fix: remove type check in rollup client (ethereum-optimism#750) * l2geth: remove tx type check in client * chore: add changeset * dtl: prevent null reference in L1 handler (ethereum-optimism#757) * dtl: prevent reference of null value * chore: add changeset * test: eth_call exceptions (ethereum-optimism#800) * feat[l2geth]: Pass up contract revert reasons during DoEstimateGas (ethereum-optimism#774) * wip: Starting work on geth revert reasons during estimate gas fix: error in comment fix: I got things backwards fix: Use UnpackValues instead of Unpack Update l2geth/accounts/abi/abi.go Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> * Add integration test for reverts fix: build error * chore: Add changeset Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> * chore: add changeset (ethereum-optimism#831) * Migrate ETH between gateways (ethereum-optimism#778) * add migrate ETH functionality * contracts: add eth gateway docstring (ethereum-optimism#832) Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> Co-authored-by: smartcontracts <smartcontracts@doge.org> Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> Co-authored-by: smartcontracts <kelvinfichter@gmail.com> Co-authored-by: ben-chain <ben@pseudonym.party> Co-authored-by: Georgios Konstantopoulos <me@gakonst.com> Co-authored-by: Maurelian <maurelian@protonmail.ch> Co-authored-by: Kevin Ho <kevinjho1996@gmail.com>
Description
Adds basic value transfer support to
OVM_ECDSAContractAccount
. As per convo with @ben-chain, we've restricted value transfer as follows:CALL
.CALL
is not executed and the user must provide no input data.