Skip to content

Commit

Permalink
contracts-bedrock: fix custom gas token withdrawals to L1 (ethereum-o…
Browse files Browse the repository at this point in the history 
…ptimism#10685)

* contracts-bedrock: fix custom gas token withdrawals to L1

The previous implementation of the `FeeVault` used the `StandardBridge`
to withdraw `ether`. Under custom gas token, this meant that withdrawals
to L1 didn't work and withdrawals to L2 were the only supported path.
This updates the `L2ToL1Messenger` to withdraw to L1 via the `L2ToL1Messenger`.
This fixes the ability to withdraw to L1 while custom gas token is
enabled while also saving some gas on both L1 and L2 by removing
extra call frames. The reason why this was originally implemented using
the `StandardBridge` was to reduce additional changes during the
development of bedrock so that more effort could be spent thinking
about the new protocol design, as the legacy implementation used
the `StandardBridge` for doing the withdrawal.

Having the `StandardBridge` used for `ether` deposits and withdrawals
is unncessarily expensive and a bit of a leaky abstraction.

Gas savings are scalability improvements, both on L1 and on L2.

Also uses a new variant of `SafeCall` that passes through all gas
in its `transfer`. The implementation ensures to assert on the return
value of the call to hold the invariant true that the `totalProcessed`
will always be consistent. If the call reverts, then its possible for
the `ether` to not be transferred and `totalProcessed` to be incremented.

Also bump the semver of the contracts to a beta version. They will
be bumped out of beta at a release of the contracts.

The tests are also improved for both the `SafeCall` library as well
as the `FeeVault`.

* op-e2e: withdrawal tests

* e2e: update test with comments

* feevault: fix tech debt

Updates the `FeeVault` to use modern style guide.
Its not great to leak `immutable` style into the ABI
so add extra getters and mark the legacy getters as
deprecated. Doing this now will help to migrate to the
new getters in 1 year from now.

Also add in the storage layout spacer that was previously
noticed in an audit, see ethereum-optimism#9477.

* semver-lock: update + compiler warning

* snapshots: regenerate

* lint: fix

* tests: fixup

* gas-snapshot: update

* test: fix

* contracts-bedrock: reuse existing implementation of send in SafeCall

Co-authored-by: Matt Solomon <[email protected]>

* contracts-bedrock: update gap in FeeVault for inheritance to start at 50 gap

* contracts-bedrock: add missing assertions in tests for SequencerFeeVault

* contracts-bedrock: remove redundant if clause in SafeCall tests

* contracts-bedrock: fix gas relation in SafeCall tests

* contracts-bedrock: update send method tests for SafeCall

* op-e2e: add missing check in custom gas token tests

* contracts-bedrock: fix types in SafeCall tests

* contracts-bedrock: update snapshots

* contracts-bedrock: update semver-lock

* op-e2e: fix custom_gas_token test

* op-e2e: fix custom gas token test

* contracts-bedrock: fix tests for custom gas token

* op-e2e: fix tests for custom gas token

* op-e2e: fix build

---------

Co-authored-by: Diego <[email protected]>
Co-authored-by: Matt Solomon <[email protected]>
  • Loading branch information
@tynes @0xfuturistic @mds1
3 people authored Jun 6, 2024
1 parent 2a01915 commit c04ca0c
Show file tree
Hide file tree
Showing 22 changed files with 447 additions and 114 deletions.
140 changes: 140 additions & 0 deletions op-e2e/custom_gas_token_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,144 @@ func TestCustomGasToken(t *testing.T) {
require.Equal(t, withdrawAmount, diff)
}

// checkFeeWithdrawal ensures that the FeeVault can be withdrawn from
checkFeeWithdrawal := func(t *testing.T, enabled bool) {
feeVault, err := bindings.NewSequencerFeeVault(predeploys.SequencerFeeVaultAddr, l2Client)
require.NoError(t, err)

// Alice will be sending transactions
aliceOpts, err := bind.NewKeyedTransactorWithChainID(cfg.Secrets.Alice, cfg.L2ChainIDBig())
require.NoError(t, err)

// Get the recipient of the funds
recipient, err := feeVault.RECIPIENT(&bind.CallOpts{})
require.NoError(t, err)

// This test depends on the withdrawal network being L1 which is represented
// by 0 in the enum.
withdrawalNetwork, err := feeVault.WITHDRAWALNETWORK(&bind.CallOpts{})
require.NoError(t, err)
require.Equal(t, withdrawalNetwork, uint8(0))

// Get the balance of the recipient on L1
var recipientBalanceBefore *big.Int
if enabled {
recipientBalanceBefore, err = weth9.BalanceOf(&bind.CallOpts{}, recipient)
} else {
recipientBalanceBefore, err = l1Client.BalanceAt(context.Background(), recipient, nil)
}
require.NoError(t, err)

// Get the min withdrawal amount for the FeeVault
amount, err := feeVault.MINWITHDRAWALAMOUNT(&bind.CallOpts{})
require.NoError(t, err)

l1opts, err := bind.NewKeyedTransactorWithChainID(cfg.Secrets.Alice, cfg.L1ChainIDBig())
require.NoError(t, err)

optimismPortal, err := bindings.NewOptimismPortal(cfg.L1Deployments.OptimismPortalProxy, l1Client)
require.NoError(t, err)

depositAmount := new(big.Int).Mul(amount, big.NewInt(14))
l1opts.Value = depositAmount

var receipt *types.Receipt

// Alice deposits funds
if enabled {
// approve + transferFrom flow
// Cannot use `transfer` because of the tracking of balance in the OptimismPortal
dep, err := weth9.Deposit(l1opts)
waitForTx(t, dep, err, l1Client)

l1opts.Value = nil
tx, err := weth9.Approve(l1opts, cfg.L1Deployments.OptimismPortalProxy, depositAmount)
waitForTx(t, tx, err, l1Client)

require.NoError(t, err)
deposit, err := optimismPortal.DepositERC20Transaction(l1opts, cfg.Secrets.Addresses().Alice, depositAmount, depositAmount, 500_000, false, []byte{})
waitForTx(t, deposit, err, l1Client)

receipt, err = wait.ForReceiptOK(context.Background(), l1Client, deposit.Hash())
require.NoError(t, err)
} else {
// send ether to the portal directly, alice already has funds on L2
tx, err := optimismPortal.DepositTransaction(l1opts, cfg.Secrets.Addresses().Alice, depositAmount, 500_000, false, []byte{})
waitForTx(t, tx, err, l1Client)

receipt, err = wait.ForReceiptOK(context.Background(), l1Client, tx.Hash())
require.NoError(t, err)
}

// Compute the deposit transaction hash + poll for it
depositEvent, err := receipts.FindLog(receipt.Logs, optimismPortal.ParseTransactionDeposited)
require.NoError(t, err, "Should emit deposit event")
depositTx, err := derive.UnmarshalDepositLogEvent(&depositEvent.Raw)
require.NoError(t, err)
_, err = wait.ForReceiptOK(context.Background(), l2Client, types.NewTx(depositTx).Hash())
require.NoError(t, err)

// Get Alice's balance on L2
aliceBalance, err := l2Client.BalanceAt(context.Background(), cfg.Secrets.Addresses().Alice, nil)
require.NoError(t, err)
require.GreaterOrEqual(t, aliceBalance.Uint64(), amount.Uint64())

// Send funds to the FeeVault so its balance is above the min withdrawal amount
aliceOpts.Value = amount
feeVaultTx, err := feeVault.Receive(aliceOpts)
waitForTx(t, feeVaultTx, err, l2Client)

// Ensure that the balance of the vault is large enough to withdraw
vaultBalance, err := l2Client.BalanceAt(context.Background(), predeploys.SequencerFeeVaultAddr, nil)
require.NoError(t, err)
require.GreaterOrEqual(t, vaultBalance.Uint64(), amount.Uint64())

// Ensure there is code at the vault address
code, err := l2Client.CodeAt(context.Background(), predeploys.SequencerFeeVaultAddr, nil)
require.NoError(t, err)
require.NotEmpty(t, code)

// Poke the fee vault to withdraw
l2Opts, err := bind.NewKeyedTransactorWithChainID(cfg.Secrets.Bob, cfg.L2ChainIDBig())
require.NoError(t, err)
withdrawalTx, err := feeVault.Withdraw(l2Opts)
waitForTx(t, withdrawalTx, err, l2Client)

// Get the receipt and the amount withdrawn
receipt, err = l2Client.TransactionReceipt(context.Background(), withdrawalTx.Hash())
require.NoError(t, err)

inclusionHeight := receipt.BlockNumber.Uint64()
it, err := feeVault.FilterWithdrawal(&bind.FilterOpts{
Start: inclusionHeight,
End: &inclusionHeight,
})
require.NoError(t, err)
require.True(t, it.Next())

withdrawnAmount := it.Event.Value

// Finalize the withdrawal
proveReceipt, finalizeReceipt, resolveClaimReceipt, resolveReceipt := ProveAndFinalizeWithdrawal(t, cfg, sys, "verifier", cfg.Secrets.Alice, receipt)
require.Equal(t, types.ReceiptStatusSuccessful, proveReceipt.Status)
require.Equal(t, types.ReceiptStatusSuccessful, finalizeReceipt.Status)
if e2eutils.UseFaultProofs() {
require.Equal(t, types.ReceiptStatusSuccessful, resolveClaimReceipt.Status)
require.Equal(t, types.ReceiptStatusSuccessful, resolveReceipt.Status)
}

// Assert that the recipient's balance did increase
var recipientBalanceAfter *big.Int
if enabled {
recipientBalanceAfter, err = weth9.BalanceOf(&bind.CallOpts{}, recipient)
} else {
recipientBalanceAfter, err = l1Client.BalanceAt(context.Background(), recipient, nil)
}
require.NoError(t, err)

require.Equal(t, recipientBalanceAfter, new(big.Int).Add(recipientBalanceBefore, withdrawnAmount))
}

checkL1TokenNameAndSymbol := func(t *testing.T, enabled bool) {
systemConfig, err := bindings.NewSystemConfig(cfg.L1Deployments.SystemConfigProxy, l1Client)
require.NoError(t, err)
Expand Down Expand Up @@ -278,6 +416,7 @@ func TestCustomGasToken(t *testing.T) {
checkL1TokenNameAndSymbol(t, enabled)
checkL2TokenNameAndSymbol(t, enabled)
checkWETHTokenNameAndSymbol(t, enabled)
checkFeeWithdrawal(t, enabled)

// Activate custom gas token feature (devnet does not have this activated at genesis)
setCustomGasToken(t, cfg, sys, weth9Address)
Expand All @@ -289,6 +428,7 @@ func TestCustomGasToken(t *testing.T) {
checkL1TokenNameAndSymbol(t, enabled)
checkL2TokenNameAndSymbol(t, enabled)
checkWETHTokenNameAndSymbol(t, enabled)
checkFeeWithdrawal(t, enabled)
}

// callViaSafe will use the Safe smart account at safeAddress to send a transaction to target using the provided data. The transaction signature is constructed from
Expand Down
4 changes: 2 additions & 2 deletions op-e2e/e2eutils/wait/waits.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func ForReceipt(ctx context.Context, client *ethclient.Client, hash common.Hash,
if errors.Is(err, ethereum.NotFound) || (err != nil && strings.Contains(err.Error(), "transaction indexing is in progress")) {
select {
case <-ctx.Done():
return nil, ctx.Err()
return nil, fmt.Errorf("timed out waiting for tx %s: %w: %w", hash, err, ctx.Err())
case <-ticker.C:
continue
}
Expand All @@ -59,7 +59,7 @@ func ForReceipt(ctx context.Context, client *ethclient.Client, hash common.Hash,
continue
}
if err != nil {
return nil, fmt.Errorf("failed to get receipt: %w", err)
return nil, fmt.Errorf("failed to get receipt for tx %s: %w", hash, err)
}
if receipt.Status != status {
printDebugTrace(ctx, client, hash)
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts-bedrock/.gas-snapshot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_0() (gas: 5619
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_1() (gas: 4074035)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_0() (gas: 466947)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_1() (gas: 3512629)
GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 72629)
GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 72624)
GasBenchMark_L2OutputOracle:test_proposeL2Output_benchmark() (gas: 92973)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark() (gas: 68433)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark_1() (gas: 68903)
Expand Down
22 changes: 11 additions & 11 deletions packages/contracts-bedrock/semver-lock.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,23 @@
"sourceCodeHash": "0x2ed7a2d6d66839fb3d207952f44b001bce349334adc40ce66d0503ce64e48548"
},
"src/L1/DataAvailabilityChallenge.sol": {
"initCodeHash": "0x39d938938b13abb2455959960c2ee96d48149fd16a7fb69f1f6699460d5019be",
"initCodeHash": "0x9e9ac7238791b2eaade946e58b98f6f1f43f60b8b393664d85a553857a0ab5c7",
"sourceCodeHash": "0xf6c72a2cca24cfa7c9274d720e93b05d665a2751cca3d747105e6c511ccffc73"
},
"src/L1/DelayedVetoable.sol": {
"initCodeHash": "0x84f78e56211500a768b2c5bbafde8e7b411bd64c237297370d7d22326b68357c",
"sourceCodeHash": "0xc59b8574531162e016d7342aeb6e79d05574e90dbea6c0e5ede35b65010ad894"
},
"src/L1/L1CrossDomainMessenger.sol": {
"initCodeHash": "0xff2c621c5ae8f1190779b13d8da4ee9fcd64f202d6e5ee309f0028bca17a8b8f",
"initCodeHash": "0x1f97347da46930d5b3cd40bd2699a6b78812e5f597793c8d59a90b0ef6800da8",
"sourceCodeHash": "0x56a2d3abed97eb7b292db758aac1e36fc08a12bfa44f7969824e26866a1417fa"
},
"src/L1/L1ERC721Bridge.sol": {
"initCodeHash": "0xec73b46e68ea29298707f7b9709e7948afe303907b6c4e8b161e2ded2c85ee9c",
"sourceCodeHash": "0xd57acdbd001941e75cf4326ba7c1bdad809912f10b1e44ffaebe073917cdd296"
},
"src/L1/L1StandardBridge.sol": {
"initCodeHash": "0xf46dc2d8e171bd5aebbafe31386e9d27323a124b1b99f378cb8f67ab22225a7b",
"initCodeHash": "0x508977d2517b49649bef57899e2c2519c271953708b2c84325377dae64a92baf",
"sourceCodeHash": "0x0d07e526c1891abb81c7e94f73642caa8ee386ab036b3b2a67f1b21ca85089c5"
},
"src/L1/L2OutputOracle.sol": {
Expand Down Expand Up @@ -60,8 +60,8 @@
"sourceCodeHash": "0x40d708140ee6345e146e358c169a191dbbc991782560a2dcbf90ba45a82f7117"
},
"src/L2/BaseFeeVault.sol": {
"initCodeHash": "0x2744d34573be83206d1b75d049d18a7bb37f9058e68c0803e5008c46b0dc2474",
"sourceCodeHash": "0xd787bd2a192ba5025fa0a8de2363af66a8de20de226e411bdb576adb64636cd0"
"initCodeHash": "0x623bf6892f0bdb536f2916bc9eb45e52012ad2c80893ff87d750757966b9be68",
"sourceCodeHash": "0x3a725791a0f5ed84dc46dcdae26f6170a759b2fe3dc360d704356d088b76cfd6"
},
"src/L2/CrossL2Inbox.sol": {
"initCodeHash": "0x46e15ac5de81ea415061d049730da25acf31040d6d5d70fe3a9bf4cac100c282",
Expand All @@ -80,19 +80,19 @@
"sourceCodeHash": "0xd8ec2f814690d1ffd55e5b8496bca5a179d6d1772d61f71cdf8296c9058dc2c6"
},
"src/L2/L1FeeVault.sol": {
"initCodeHash": "0x2744d34573be83206d1b75d049d18a7bb37f9058e68c0803e5008c46b0dc2474",
"sourceCodeHash": "0x3a94f273937d8908fb37dd2c495a6a0b9c3941fe68ccea51723f84eb343ba225"
"initCodeHash": "0x623bf6892f0bdb536f2916bc9eb45e52012ad2c80893ff87d750757966b9be68",
"sourceCodeHash": "0x29c64c43e3deedfa63572d36ce511febb99e500352a003242aed339716d3e3ce"
},
"src/L2/L2CrossDomainMessenger.sol": {
"initCodeHash": "0x5d1d1c0c3cc2171832438cbafb6e55c142eab43a7d7ea09ee818a7e3c50d9314",
"initCodeHash": "0xad95e4e98adf35ea2baeb15348d751fc76e1bb0d001474d4eaefba47f7aeaf5f",
"sourceCodeHash": "0x440d299b7429c44f6faa4005b33428f9edc1283027d9c78a63eb3d76452bfa47"
},
"src/L2/L2ERC721Bridge.sol": {
"initCodeHash": "0xcafa012b2d8f1bb05c11cbbff9749c0fe6f995c9afb1d26d2d71f03384e34a22",
"sourceCodeHash": "0xa7646a588275046f92525ef121e5a0fe149e7752ea51fe62f7e0686a21153542"
},
"src/L2/L2StandardBridge.sol": {
"initCodeHash": "0xdfe717975f7c76e80201f715a0d68f3fef02beca6a9e0f378a115e175c6a5944",
"initCodeHash": "0xb6af582e9edaae531d48559b7cd0ca5813a112361ea19b8cb46292726ad88d40",
"sourceCodeHash": "0xb4a9f333f04008f610eb55fa6ff7e610bab340d53156cb50ec65a575c9576b0e"
},
"src/L2/L2ToL1MessagePasser.sol": {
Expand All @@ -104,8 +104,8 @@
"sourceCodeHash": "0x249218d69909750f5245a42d247a789f1837c24863bded94dc577fcbec914175"
},
"src/L2/SequencerFeeVault.sol": {
"initCodeHash": "0xd62e193d89b1661d34031227a45ce1eade9c2a89b0bd7f362f511d03cceef294",
"sourceCodeHash": "0xa304b4b556162323d69662b4dd9a1d073d55ec661494465489bb67f1e465e7b3"
"initCodeHash": "0xb94145f571e92ee615c6fe903b6568e8aac5fe760b6b65148ffc45d2fb0f5433",
"sourceCodeHash": "0x8f2a54104e5e7105ba03ba37e3ef9b6684a447245f0e0b787ba4cca12957b97c"
},
"src/L2/WETH.sol": {
"initCodeHash": "0xde72ae96910e95249623c2d695749847e4c4adeaf96a7a35033afd77318a528a",
Expand Down
39 changes: 39 additions & 0 deletions packages/contracts-bedrock/snapshots/abi/BaseFeeVault.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,32 @@
"stateMutability": "view",
"type": "function"
},
{
"inputs": [],
"name": "minWithdrawalAmount",
"outputs": [
{
"internalType": "uint256",
"name": "amount_",
"type": "uint256"
}
],
"stateMutability": "view",
"type": "function"
},
{
"inputs": [],
"name": "recipient",
"outputs": [
{
"internalType": "address",
"name": "recipient_",
"type": "address"
}
],
"stateMutability": "view",
"type": "function"
},
{
"inputs": [],
"name": "totalProcessed",
Expand Down Expand Up @@ -96,6 +122,19 @@
"stateMutability": "nonpayable",
"type": "function"
},
{
"inputs": [],
"name": "withdrawalNetwork",
"outputs": [
{
"internalType": "enum FeeVault.WithdrawalNetwork",
"name": "network_",
"type": "uint8"
}
],
"stateMutability": "view",
"type": "function"
},
{
"anonymous": false,
"inputs": [
Expand Down
39 changes: 39 additions & 0 deletions packages/contracts-bedrock/snapshots/abi/L1FeeVault.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,32 @@
"stateMutability": "view",
"type": "function"
},
{
"inputs": [],
"name": "minWithdrawalAmount",
"outputs": [
{
"internalType": "uint256",
"name": "amount_",
"type": "uint256"
}
],
"stateMutability": "view",
"type": "function"
},
{
"inputs": [],
"name": "recipient",
"outputs": [
{
"internalType": "address",
"name": "recipient_",
"type": "address"
}
],
"stateMutability": "view",
"type": "function"
},
{
"inputs": [],
"name": "totalProcessed",
Expand Down Expand Up @@ -96,6 +122,19 @@
"stateMutability": "nonpayable",
"type": "function"
},
{
"inputs": [],
"name": "withdrawalNetwork",
"outputs": [
{
"internalType": "enum FeeVault.WithdrawalNetwork",
"name": "network_",
"type": "uint8"
}
],
"stateMutability": "view",
"type": "function"
},
{
"anonymous": false,
"inputs": [
Expand Down
Loading

0 comments on commit c04ca0c

Please sign in to comment.