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.

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, l2geth): native ETH value support for ovmCALL #1038

Merged
merged 22 commits into from
Jun 10, 2021
Merged
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d46d449
feat(contracts): add ovmCALL-types with native value
ben-chain May 26, 2021
94bd4b3
test(integration-tests): ovmCALL-types with value (compiler and wrapper)
ben-chain May 27, 2021
47a4f38
fix(contracts): account for intrinsic gas of OVM_ETH sends
ben-chain Jun 2, 2021
1b7150d
fix(contracts): merge conflict bug
smartcontracts Jun 2, 2021
37bc9b8
fix(contracts): update gas benchmark
smartcontracts Jun 2, 2021
897c7a7
feat(contracts, integration-tests): use new value-compatible compiler
ben-chain Jun 2, 2021
1397cda
feat(contracts,l2geth): support value calls in OVM_ECDSAContractAccount
ben-chain Jun 2, 2021
104d823
fix(contracts): ovmDELEGATECALL does not change message context
ben-chain Jun 4, 2021
760a2a7
feat(contracts): sending value between EOAs
ben-chain Jun 8, 2021
b4fb7ac
test(integration-tests): ovmDELEGATECALL preserves ovmCALLVALUE
ben-chain Jun 8, 2021
4c10635
test(integration-tests): assert ovmSELFBALANCEs correct
ben-chain Jun 8, 2021
f50151a
test(integration-tests): intrinsic gas for eth value calls
ben-chain Jun 8, 2021
fee1f3d
test(integration-tests): update gas values
ben-chain Jun 8, 2021
ed92d53
chore(contracts): lint
ben-chain Jun 8, 2021
b6b8275
feat(contracts, l2geth): eth_calls with nonzero value
ben-chain Jun 8, 2021
97c4231
chore: minor fixups and comments based on PR feedback
ben-chain Jun 9, 2021
f88da35
test(integration-tests): add requested tests from PR reviews
ben-chain Jun 9, 2021
0c802d5
test(integration-tests): ovmSELFBALANCE is preserved in ovmDELEGATECALLs
ben-chain Jun 9, 2021
edc8cbd
fix(contracts): fix bug where ovmDELEGATECALL could fail if balance w…
ben-chain Jun 10, 2021
9dcecd8
chore: add changeset
ben-chain Jun 10, 2021
4c02ee3
fix(contracts): update intrinsic gas for worst-case value sends
ben-chain Jun 10, 2021
3dc67db
chore: address final PR nits/improvements
ben-chain Jun 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
chore: minor fixups and comments based on PR feedback
  • Loading branch information
ben-chain committed Jun 10, 2021
commit 97c4231aed3045d5c98f9f256aa6eea46842aa44
1 change: 0 additions & 1 deletion integration-tests/contracts/ValueCalls.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
// SPDX-License-Identifier: MIT
// @unsupported: evm
pragma solidity >=0.7.0;

contract ValueContext {
17 changes: 7 additions & 10 deletions integration-tests/test/native-eth-ovm-calls.spec.ts
Original file line number Diff line number Diff line change
@@ -230,9 +230,8 @@ describe('Native ETH value integration tests', () => {
getContractInterface('OVM_ExecutionManager', false),
env.l1Wallet.provider
)
CALL_WITH_VALUE_INTRINSIC_GAS = (
await OVM_ExecutionManager.CALL_WITH_VALUE_INTRINSIC_GAS()
).toNumber()
const CALL_WITH_VALUE_INTRINSIC_GAS_BIGNUM = await OVM_ExecutionManager.CALL_WITH_VALUE_INTRINSIC_GAS()
CALL_WITH_VALUE_INTRINSIC_GAS = CALL_WITH_VALUE_INTRINSIC_GAS_BIGNUM.toNumber()

const Factory__ValueGasMeasurer = await ethers.getContractFactory(
'ValueGasMeasurer',
@@ -246,13 +245,11 @@ describe('Native ETH value integration tests', () => {
const value = 1
const gasLimit = 1_000_000
// sending to 0x0000... should consume the minimal possible gas for a nonzero ETH send
const minimalSendGas = (
await ValueGasMeasurer.callStatic.measureGasOfSend(
ethers.constants.AddressZero,
value,
gasLimit
)
).toNumber()
const minimalSendGas = await ValueGasMeasurer.callStatic.measureGasOfSend(
ethers.constants.AddressZero,
value,
gasLimit
)

const buffer = 1.2
expect(minimalSendGas * buffer).to.be.lte(CALL_WITH_VALUE_INTRINSIC_GAS)
Original file line number Diff line number Diff line change
@@ -871,6 +871,11 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
);
}


/***************************************
* Public Functions: ETH Value Opcodes *
***************************************/

/**
* @notice Overrides BALANCE.
* @param _contract Address of the contract to query the OVM_ETH balance of.
@@ -921,6 +926,7 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
return ovmBALANCE(ovmADDRESS());
}


/***************************************
* Public Functions: Execution Context *
***************************************/
@@ -1086,15 +1092,21 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
uint256 messageValue = _nextMessageContext.ovmCALLVALUE;
// If there is value in this message, we need to transfer the ETH over before switching contexts.
if (messageValue > 0) {
// Handle out-of-intrinsic gas consistent with EVM behavior -- the subcall "appears to revert"
// Handle out-of-intrinsic gas consistent with EVM behavior -- the subcall "appears to revert" if we don't have enough gas to transfer the ETH.
// Similar to dynamic gas cost of value exceeding gas here:
// https://github.com/ethereum/go-ethereum/blob/c503f98f6d5e80e079c1d8a3601d188af2a899da/core/vm/interpreter.go#L268-L273
if (gasleft() < CALL_WITH_VALUE_INTRINSIC_GAS) {
return (false, hex"");
}

// If not out of intrinsic gas, we guarantee that that intrinsic gas is reserved for consumption by the EM and OVM_ETH.
// If there *is* enough gas to transfer ETH, then we need to make sure this amount of gas is reserved (i.e. not
// given to the _contract.call below) to guarantee that _handleExternalMessage can't run out of gas.
// In particular, in the event that the call fails, we will need to transfer the ETH back to the sender.
// Taking the lesser of _gasLimit and gasleft() - CALL_WITH_VALUE_INTRINSIC_GAS guarantees that the second
// _attemptForcedEthTransfer below, if needed, always has enough gas to succeed.
_gasLimit = Math.min(
_gasLimit,
gasleft() - CALL_WITH_VALUE_INTRINSIC_GAS
gasleft() - CALL_WITH_VALUE_INTRINSIC_GAS // Cannot overflow due to the above check.
);

// Now transfer the value of the call.
@@ -1104,7 +1116,8 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
messageValue
);

// If the ETH transfer fails (e.g. due to insufficient balance), then treat this as a revert.
// If the ETH transfer fails (should only be possible in the case of insufficient balance), then treat this as a revert.
// This mirrors EVM behavior, see https://github.com/ethereum/go-ethereum/blob/2dee31930c9977af2a9fcb518fb9838aa609a7cf/core/vm/evm.go#L298
if (!transferredOvmEth) {
return (false, hex"");
}
@@ -1148,15 +1161,17 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {

// If the message threw an exception, its value should be returned back to the sender.
// So, we force it back, BEFORE returning the messageContext to the previous addresses.
// This operation is part of the reason we "reserved the intrinsic gas" above.
if (messageValue > 0 && !success) {
bool transferredOvmEth = _attemptForcedEthTransfer(
prevMessageContext.ovmADDRESS,
messageValue
);

// Since we transferred it in above and the call reverted, the transfer back should always pass.
// If it did not, this is an OOG, and we have to make the parent out-of-gas as well.
// TODO: should we also enforce there is always enough extra gas to make it past this step? This would mean this condition is only triggered due to some critical bug in the ERC20 implementation.
// This code path should NEVER be triggered since we sent `messageValue` worth of OVM_ETH into the target
// and reserved sufficient gas to execute the transfer, but in case there is some edge case which has
// been missed, we revert the entire frame (and its parent) to make sure the ETH gets sent back.
if (!transferredOvmEth) {
_revertWithFlag(RevertFlag.OUT_OF_GAS);
}
Original file line number Diff line number Diff line change
@@ -148,8 +148,14 @@ interface iOVM_ExecutionManager {
function ovmEXTCODECOPY(address _contract, uint256 _offset, uint256 _length) external returns (bytes memory _code);
function ovmEXTCODESIZE(address _contract) external returns (uint256 _size);
function ovmEXTCODEHASH(address _contract) external returns (bytes32 _hash);
function ovmBALANCE(address _contract) external returns (uint256 _balance); // TODO: where to put this one?
function ovmSELFBALANCE() external returns (uint256 _balance); // TODO: where to put this one?


/*********************
* ETH Value Opcodes *
*********************/

function ovmBALANCE(address _contract) external returns (uint256 _balance);
function ovmSELFBALANCE() external returns (uint256 _balance);


/***************************************