From 3d3e767ed576f0cc3f43763b34f08e21211c2fe2 Mon Sep 17 00:00:00 2001 From: Skeletor Spaceman Date: Mon, 5 Aug 2024 20:03:01 -0300 Subject: [PATCH 01/23] feat: ban-deposits-interop first RFC draft --- op-node/rollup/derive/attributes.go | 17 +++- op-node/rollup/derive/l1_block_info.go | 80 ++++++++++++++++++- .../contracts-bedrock/src/L2/CrossL2Inbox.sol | 6 ++ packages/contracts-bedrock/src/L2/L1Block.sol | 23 +++++- 4 files changed, 119 insertions(+), 7 deletions(-) diff --git a/op-node/rollup/derive/attributes.go b/op-node/rollup/derive/attributes.go index c65198bcec7..7584ec5abba 100644 --- a/op-node/rollup/derive/attributes.go +++ b/op-node/rollup/derive/attributes.go @@ -116,14 +116,27 @@ func (ba *FetchingAttributesBuilder) PreparePayloadAttributes(ctx context.Contex upgradeTxs = append(upgradeTxs, fjord...) } - l1InfoTx, err := L1InfoDepositBytes(ba.rollupCfg, sysConfig, seqNumber, l1Info, nextL2Time) + var hasDeposits = len(depositTxs) > 0 + l1InfoTx, err := L1InfoDepositBytes(ba.rollupCfg, sysConfig, seqNumber, l1Info, nextL2Time, hasDeposits) if err != nil { return nil, NewCriticalError(fmt.Errorf("failed to create l1InfoTx: %w", err)) } - txs := make([]hexutil.Bytes, 0, 1+len(depositTxs)+len(upgradeTxs)) + var txsLen = 1 + len(depositTxs) + len(upgradeTxs) + if hasDeposits { + txsLen += 1 // for the isDeposit resetting tx + } + txs := make([]hexutil.Bytes, 0, txsLen) txs = append(txs, l1InfoTx) txs = append(txs, depositTxs...) + if hasDeposits { + depositsCompleteTx, err := DepositsCompleteBytes(ba.rollupCfg, sysConfig, seqNumber, l1Info, nextL2Time) + if err != nil { + return nil, NewCriticalError(fmt.Errorf("failed to create depositsCompleteTx: %w", err)) + } + // after all the deposits have been processed, we need to include the isDeposit resetting tx + txs = append(txs, depositsCompleteTx) + } txs = append(txs, upgradeTxs...) var withdrawals *types.Withdrawals diff --git a/op-node/rollup/derive/l1_block_info.go b/op-node/rollup/derive/l1_block_info.go index 26f3f6711f5..a169e43e5d7 100644 --- a/op-node/rollup/derive/l1_block_info.go +++ b/op-node/rollup/derive/l1_block_info.go @@ -20,14 +20,19 @@ import ( const ( L1InfoFuncBedrockSignature = "setL1BlockValues(uint64,uint64,uint256,bytes32,uint64,bytes32,uint256,uint256)" L1InfoFuncEcotoneSignature = "setL1BlockValuesEcotone()" + DepositsCompleteSignature = "depositsComplete()" L1InfoArguments = 8 L1InfoBedrockLen = 4 + 32*L1InfoArguments L1InfoEcotoneLen = 4 + 32*5 // after Ecotone upgrade, args are packed into 5 32-byte slots + // TODO ^ we need to add space for the isDeposit flag here + // or should we create a new Len var for this? + DepositsCompleteLen = 4 // only the selector ) var ( L1InfoFuncBedrockBytes4 = crypto.Keccak256([]byte(L1InfoFuncBedrockSignature))[:4] L1InfoFuncEcotoneBytes4 = crypto.Keccak256([]byte(L1InfoFuncEcotoneSignature))[:4] + DepositsCompleteBytes4 = crypto.Keccak256([]byte(DepositsCompleteSignature))[:4] L1InfoDepositerAddress = common.HexToAddress("0xdeaddeaddeaddeaddeaddeaddeaddeaddead0001") L1BlockAddress = predeploys.L1BlockAddr ErrInvalidFormat = errors.New("invalid ecotone l1 block info format") @@ -55,6 +60,8 @@ type L1BlockInfo struct { BlobBaseFee *big.Int // added by Ecotone upgrade BaseFeeScalar uint32 // added by Ecotone upgrade BlobBaseFeeScalar uint32 // added by Ecotone upgrade + + IsDeposit bool // added by Interop upgrade } // Bedrock Binary Format @@ -160,6 +167,8 @@ func (info *L1BlockInfo) unmarshalBinaryBedrock(data []byte) error { // | 32 | BatcherHash | // +---------+--------------------------+ +// TODO: should we duplicate this function? +// or can we extend it somehow to only add the isDeposit flag at the end func (info *L1BlockInfo) marshalBinaryEcotone() ([]byte, error) { w := bytes.NewBuffer(make([]byte, 0, L1InfoEcotoneLen)) if err := solabi.WriteSignature(w, L1InfoFuncEcotoneBytes4); err != nil { @@ -197,6 +206,7 @@ func (info *L1BlockInfo) marshalBinaryEcotone() ([]byte, error) { if err := solabi.WriteAddress(w, info.BatcherAddr); err != nil { return nil, err } + // TODO: add isDeposit flag here return w.Bytes(), nil } @@ -238,6 +248,7 @@ func (info *L1BlockInfo) unmarshalBinaryEcotone(data []byte) error { if info.BatcherAddr, err = solabi.ReadAddress(r); err != nil { return err } + // TODO unmarshall isDeposit flag here if !solabi.EmptyReader(r) { return errors.New("too many bytes") } @@ -259,9 +270,32 @@ func L1BlockInfoFromBytes(rollupCfg *rollup.Config, l2BlockTime uint64, data []b return &info, info.unmarshalBinaryBedrock(data) } +func (info *L1BlockInfo) marshalDepositsComplete() ([]byte, error) { + w := bytes.NewBuffer(make([]byte, 0, DepositsCompleteLen)) + if err := solabi.WriteSignature(w, []byte(DepositsCompleteBytes4)); err != nil { + return nil, err + } + return w.Bytes(), nil +} + +func (info *L1BlockInfo) unmarshalDepositsComplete(data []byte) error { + if len(data) != DepositsCompleteLen { + return fmt.Errorf("data is unexpected length: %d", len(data)) + } + r := bytes.NewReader(data) + + if _, err := solabi.ReadAndValidateSignature(r, DepositsCompleteBytes4); err != nil { + return err + } + if !solabi.EmptyReader(r) { + return errors.New("too many bytes") + } + return nil +} + // L1InfoDeposit creates a L1 Info deposit transaction based on the L1 block, // and the L2 block-height difference with the start of the epoch. -func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, block eth.BlockInfo, l2BlockTime uint64) (*types.DepositTx, error) { +func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, block eth.BlockInfo, l2BlockTime uint64, isDeposit bool) (*types.DepositTx, error) { l1BlockInfo := L1BlockInfo{ Number: block.NumberU64(), Time: block.Time(), @@ -269,6 +303,7 @@ func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber BlockHash: block.Hash(), SequenceNumber: seqNumber, BatcherAddr: sysCfg.BatcherAddr, + IsDeposit: isDeposit, } var data []byte if isEcotoneButNotFirstBlock(rollupCfg, l2BlockTime) { @@ -323,8 +358,47 @@ func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber } // L1InfoDepositBytes returns a serialized L1-info attributes transaction. -func L1InfoDepositBytes(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, l1Info eth.BlockInfo, l2BlockTime uint64) ([]byte, error) { - dep, err := L1InfoDeposit(rollupCfg, sysCfg, seqNumber, l1Info, l2BlockTime) +func L1InfoDepositBytes(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, l1Info eth.BlockInfo, l2BlockTime uint64, isDeposit bool) ([]byte, error) { + dep, err := L1InfoDeposit(rollupCfg, sysCfg, seqNumber, l1Info, l2BlockTime, isDeposit) + if err != nil { + return nil, fmt.Errorf("failed to create L1 info tx: %w", err) + } + l1Tx := types.NewTx(dep) + opaqueL1Tx, err := l1Tx.MarshalBinary() + if err != nil { + return nil, fmt.Errorf("failed to encode L1 info tx: %w", err) + } + return opaqueL1Tx, nil +} + +func DepositsCompleteDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, block eth.BlockInfo, l2BlockTime uint64) (*types.DepositTx, error) { + var data []byte + source := L1InfoDepositSource{ + L1BlockHash: block.Hash(), + SeqNumber: seqNumber, + } + // Set a normal gas limit with `IsSystemTransaction` to ensure + // that the DepositCompleted Transaction does not run out of gas. + out := &types.DepositTx{ + SourceHash: source.SourceHash(), + From: L1InfoDepositerAddress, + To: &L1BlockAddress, + Mint: nil, + Value: big.NewInt(0), + Gas: 50_000, + IsSystemTransaction: true, + Data: data, + } + // With the regolith fork we disable the IsSystemTx functionality, and allocate real gas + if rollupCfg.IsRegolith(l2BlockTime) { + out.IsSystemTransaction = false + out.Gas = RegolithSystemTxGas + } + return out, nil +} + +func DepositsCompleteBytes(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, l1Info eth.BlockInfo, l2BlockTime uint64) ([]byte, error) { + dep, err := DepositsCompleteDeposit(rollupCfg, sysCfg, seqNumber, l1Info, l2BlockTime) if err != nil { return nil, fmt.Errorf("failed to create L1 info tx: %w", err) } diff --git a/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol b/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol index 970cb80d97a..b85d09580ad 100644 --- a/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol +++ b/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol @@ -29,6 +29,9 @@ error InvalidChainId(); /// @notice Thrown when trying to execute a cross chain message and the target call fails. error TargetCallFailed(); +/// @notice Thrown when trying to execute a cross chain message on a deposit transaction. +error NoExecutingDeposits(); + /// @custom:proxied /// @custom:predeploy 0x4200000000000000000000000000000000000022 /// @title CrossL2Inbox @@ -114,6 +117,9 @@ contract CrossL2Inbox is ICrossL2Inbox, ISemver, TransientReentrancyAware { payable reentrantAware { + // We need to know if this is being called on a depositTx + if (L1_BLOCK.isDeposit()) revert NoExecutingDeposits(); + if (_id.timestamp > block.timestamp) revert InvalidTimestamp(); if (!IDependencySet(Predeploys.L1_BLOCK_ATTRIBUTES).isInDependencySet(_id.chainId)) { revert InvalidChainId(); diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 8cfbb391f0f..59ac0de1ea6 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -57,6 +57,9 @@ contract L1Block is ISemver, IGasToken { /// @notice The latest L1 blob base fee. uint256 public blobBaseFee; + /// @notice The isDeposit flag. + bool public isDeposit; + /// @custom:semver 1.4.1-beta.1 function version() public pure virtual returns (string memory) { return "1.4.1-beta.1"; @@ -87,6 +90,11 @@ contract L1Block is ISemver, IGasToken { return token != Constants.ETHER; } + function isDeposit() returns (bool _isDeposit) external view { + require(msg.sender == CROSS_L2_INBOX, "L1Block: only the CrossL2Inbox can check if it is a deposit"); + returns isDeposit; + } + /// @custom:legacy /// @notice Updates the L1 block values. /// @param _number L1 blocknumber. @@ -97,6 +105,7 @@ contract L1Block is ISemver, IGasToken { /// @param _batcherHash Versioned hash to authenticate batcher by. /// @param _l1FeeOverhead L1 fee overhead. /// @param _l1FeeScalar L1 fee scalar. + /// @param _isDeposit isDeposit flag function setL1BlockValues( uint64 _number, uint64 _timestamp, @@ -105,7 +114,8 @@ contract L1Block is ISemver, IGasToken { uint64 _sequenceNumber, bytes32 _batcherHash, uint256 _l1FeeOverhead, - uint256 _l1FeeScalar + uint256 _l1FeeScalar, + bool _isDeposit ) external { @@ -119,6 +129,7 @@ contract L1Block is ISemver, IGasToken { batcherHash = _batcherHash; l1FeeOverhead = _l1FeeOverhead; l1FeeScalar = _l1FeeScalar; + isDeposit = _isDeposit; } /// @notice Updates the L1 block values for an Ecotone upgraded chain. @@ -133,7 +144,8 @@ contract L1Block is ISemver, IGasToken { /// 7. _blobBaseFee L1 blob base fee. /// 8. _hash L1 blockhash. /// 9. _batcherHash Versioned hash to authenticate batcher by. - function setL1BlockValuesEcotone() external { + /// 10. _isDeposit isDeposit flag + function setL1BlockValuesInterop() external { address depositor = DEPOSITOR_ACCOUNT(); assembly { // Revert if the caller is not the depositor account. @@ -149,6 +161,7 @@ contract L1Block is ISemver, IGasToken { sstore(blobBaseFee.slot, calldataload(68)) // uint256 sstore(hash.slot, calldataload(100)) // bytes32 sstore(batcherHash.slot, calldataload(132)) // bytes32 + sstore(isDeposit.slot, calldataload(133)) // boolean } } @@ -162,4 +175,10 @@ contract L1Block is ISemver, IGasToken { emit GasPayingTokenSet({ token: _token, decimals: _decimals, name: _name, symbol: _symbol }); } + + /// @notice Resets the isDeposit flag. + function depositsComplete() external { + if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); + isDeposit = false; + } } From 0d78e5a6a433e30d350cdaadbc018d4b803fefaf Mon Sep 17 00:00:00 2001 From: Skeletor Spaceman Date: Mon, 5 Aug 2024 20:29:34 -0300 Subject: [PATCH 02/23] fix: missing marshalling of tx --- op-node/rollup/derive/l1_block_info.go | 29 ++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/op-node/rollup/derive/l1_block_info.go b/op-node/rollup/derive/l1_block_info.go index a169e43e5d7..1cc3233fc21 100644 --- a/op-node/rollup/derive/l1_block_info.go +++ b/op-node/rollup/derive/l1_block_info.go @@ -293,6 +293,11 @@ func (info *L1BlockInfo) unmarshalDepositsComplete(data []byte) error { return nil } +func DepositCompletedFromBytes(rollupCfg *rollup.Config, l2BlockTime uint64, data []byte) (*L1BlockInfo, error) { + var info L1BlockInfo + return &info, info.unmarshalDepositsComplete(data) +} + // L1InfoDeposit creates a L1 Info deposit transaction based on the L1 block, // and the L2 block-height difference with the start of the epoch. func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, block eth.BlockInfo, l2BlockTime uint64, isDeposit bool) (*types.DepositTx, error) { @@ -372,7 +377,19 @@ func L1InfoDepositBytes(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNu } func DepositsCompleteDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, block eth.BlockInfo, l2BlockTime uint64) (*types.DepositTx, error) { - var data []byte + l1BlockInfo := L1BlockInfo{ + Number: block.NumberU64(), + Time: block.Time(), + BaseFee: block.BaseFee(), + BlockHash: block.Hash(), + SequenceNumber: seqNumber, + BatcherAddr: sysCfg.BatcherAddr, + IsDeposit: false, + } + data, err := l1BlockInfo.marshalDepositsComplete() + if err != nil { + return nil, fmt.Errorf("failed to marshal Bedrock l1 block info: %w", err) + } source := L1InfoDepositSource{ L1BlockHash: block.Hash(), SeqNumber: seqNumber, @@ -400,12 +417,12 @@ func DepositsCompleteDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, func DepositsCompleteBytes(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, l1Info eth.BlockInfo, l2BlockTime uint64) ([]byte, error) { dep, err := DepositsCompleteDeposit(rollupCfg, sysCfg, seqNumber, l1Info, l2BlockTime) if err != nil { - return nil, fmt.Errorf("failed to create L1 info tx: %w", err) + return nil, fmt.Errorf("failed to create DepositsComplete tx: %w", err) } - l1Tx := types.NewTx(dep) - opaqueL1Tx, err := l1Tx.MarshalBinary() + depositsCompleteTx := types.NewTx(dep) + opaqueDepositsCompleteTx, err := depositsCompleteTx.MarshalBinary() if err != nil { - return nil, fmt.Errorf("failed to encode L1 info tx: %w", err) + return nil, fmt.Errorf("failed to encode DepositsComplete tx: %w", err) } - return opaqueL1Tx, nil + return opaqueDepositsCompleteTx, nil } From 37f17de03ea29b7adcdff6006e4e270bc25d61d1 Mon Sep 17 00:00:00 2001 From: Skeletor Spaceman Date: Tue, 6 Aug 2024 12:10:30 -0300 Subject: [PATCH 03/23] feat: code cleanup and renaming --- op-node/rollup/derive/attributes.go | 18 ++++++++---------- op-node/rollup/types.go | 1 + packages/contracts-bedrock/src/L2/L1Block.sol | 6 ++++-- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/op-node/rollup/derive/attributes.go b/op-node/rollup/derive/attributes.go index 7584ec5abba..90b9aec6f6c 100644 --- a/op-node/rollup/derive/attributes.go +++ b/op-node/rollup/derive/attributes.go @@ -122,21 +122,19 @@ func (ba *FetchingAttributesBuilder) PreparePayloadAttributes(ctx context.Contex return nil, NewCriticalError(fmt.Errorf("failed to create l1InfoTx: %w", err)) } - var txsLen = 1 + len(depositTxs) + len(upgradeTxs) - if hasDeposits { - txsLen += 1 // for the isDeposit resetting tx - } - txs := make([]hexutil.Bytes, 0, txsLen) - txs = append(txs, l1InfoTx) - txs = append(txs, depositTxs...) - if hasDeposits { + var postDeposits []hexutil.Bytes + if ba.rollupCfg.IsInteropActivationBlock(nextL2Time) && hasDeposits { depositsCompleteTx, err := DepositsCompleteBytes(ba.rollupCfg, sysConfig, seqNumber, l1Info, nextL2Time) if err != nil { return nil, NewCriticalError(fmt.Errorf("failed to create depositsCompleteTx: %w", err)) } - // after all the deposits have been processed, we need to include the isDeposit resetting tx - txs = append(txs, depositsCompleteTx) + postDeposits = append(postDeposits, depositsCompleteTx) } + + txs := make([]hexutil.Bytes, 0, 1+len(depositTxs)+len(postDeposits)+len(upgradeTxs)) + txs = append(txs, l1InfoTx) + txs = append(txs, depositTxs...) + txs = append(txs, postDeposits...) txs = append(txs, upgradeTxs...) var withdrawals *types.Withdrawals diff --git a/op-node/rollup/types.go b/op-node/rollup/types.go index 892bcfa86bc..c4287f728d3 100644 --- a/op-node/rollup/types.go +++ b/op-node/rollup/types.go @@ -489,6 +489,7 @@ func (c *Config) IsHoloceneActivationBlock(l2BlockTime uint64) bool { !c.IsHolocene(l2BlockTime-c.BlockTime) } +// TODO rename to IsIsthmusActivationBlock (this will require quite a bit of renaming) func (c *Config) IsInteropActivationBlock(l2BlockTime uint64) bool { return c.IsInterop(l2BlockTime) && l2BlockTime >= c.BlockTime && diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 59ac0de1ea6..827c44df08b 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -92,7 +92,7 @@ contract L1Block is ISemver, IGasToken { function isDeposit() returns (bool _isDeposit) external view { require(msg.sender == CROSS_L2_INBOX, "L1Block: only the CrossL2Inbox can check if it is a deposit"); - returns isDeposit; + _isDeposit = isDeposit; } /// @custom:legacy @@ -145,7 +145,7 @@ contract L1Block is ISemver, IGasToken { /// 8. _hash L1 blockhash. /// 9. _batcherHash Versioned hash to authenticate batcher by. /// 10. _isDeposit isDeposit flag - function setL1BlockValuesInterop() external { + function setL1BlockValuesIsthmus() external { address depositor = DEPOSITOR_ACCOUNT(); assembly { // Revert if the caller is not the depositor account. @@ -162,6 +162,8 @@ contract L1Block is ISemver, IGasToken { sstore(hash.slot, calldataload(100)) // bytes32 sstore(batcherHash.slot, calldataload(132)) // bytes32 sstore(isDeposit.slot, calldataload(133)) // boolean + // TODO ^ not sure we can just add a boolean as the last slot + // (could not find how to client-side test this) } } From 4f9621ae0ce56edb6741eaa08c9e5c7988e3ad6f Mon Sep 17 00:00:00 2001 From: Skeletor Spaceman Date: Tue, 6 Aug 2024 16:55:21 -0300 Subject: [PATCH 04/23] feat: cleaner implementation, always set isDeposit on --- op-node/rollup/derive/attributes.go | 5 ++--- op-node/rollup/derive/l1_block_info.go | 20 +++++-------------- packages/contracts-bedrock/src/L2/L1Block.sol | 13 +++++++----- 3 files changed, 15 insertions(+), 23 deletions(-) diff --git a/op-node/rollup/derive/attributes.go b/op-node/rollup/derive/attributes.go index 90b9aec6f6c..a4e4c614c24 100644 --- a/op-node/rollup/derive/attributes.go +++ b/op-node/rollup/derive/attributes.go @@ -116,14 +116,13 @@ func (ba *FetchingAttributesBuilder) PreparePayloadAttributes(ctx context.Contex upgradeTxs = append(upgradeTxs, fjord...) } - var hasDeposits = len(depositTxs) > 0 - l1InfoTx, err := L1InfoDepositBytes(ba.rollupCfg, sysConfig, seqNumber, l1Info, nextL2Time, hasDeposits) + l1InfoTx, err := L1InfoDepositBytes(ba.rollupCfg, sysConfig, seqNumber, l1Info, nextL2Time) if err != nil { return nil, NewCriticalError(fmt.Errorf("failed to create l1InfoTx: %w", err)) } var postDeposits []hexutil.Bytes - if ba.rollupCfg.IsInteropActivationBlock(nextL2Time) && hasDeposits { + if ba.rollupCfg.IsInterop(nextL2Time) { depositsCompleteTx, err := DepositsCompleteBytes(ba.rollupCfg, sysConfig, seqNumber, l1Info, nextL2Time) if err != nil { return nil, NewCriticalError(fmt.Errorf("failed to create depositsCompleteTx: %w", err)) diff --git a/op-node/rollup/derive/l1_block_info.go b/op-node/rollup/derive/l1_block_info.go index 1cc3233fc21..b2be8d0859d 100644 --- a/op-node/rollup/derive/l1_block_info.go +++ b/op-node/rollup/derive/l1_block_info.go @@ -24,9 +24,7 @@ const ( L1InfoArguments = 8 L1InfoBedrockLen = 4 + 32*L1InfoArguments L1InfoEcotoneLen = 4 + 32*5 // after Ecotone upgrade, args are packed into 5 32-byte slots - // TODO ^ we need to add space for the isDeposit flag here - // or should we create a new Len var for this? - DepositsCompleteLen = 4 // only the selector + DepositsCompleteLen = 4 // only the selector ) var ( @@ -60,8 +58,6 @@ type L1BlockInfo struct { BlobBaseFee *big.Int // added by Ecotone upgrade BaseFeeScalar uint32 // added by Ecotone upgrade BlobBaseFeeScalar uint32 // added by Ecotone upgrade - - IsDeposit bool // added by Interop upgrade } // Bedrock Binary Format @@ -167,8 +163,6 @@ func (info *L1BlockInfo) unmarshalBinaryBedrock(data []byte) error { // | 32 | BatcherHash | // +---------+--------------------------+ -// TODO: should we duplicate this function? -// or can we extend it somehow to only add the isDeposit flag at the end func (info *L1BlockInfo) marshalBinaryEcotone() ([]byte, error) { w := bytes.NewBuffer(make([]byte, 0, L1InfoEcotoneLen)) if err := solabi.WriteSignature(w, L1InfoFuncEcotoneBytes4); err != nil { @@ -206,7 +200,6 @@ func (info *L1BlockInfo) marshalBinaryEcotone() ([]byte, error) { if err := solabi.WriteAddress(w, info.BatcherAddr); err != nil { return nil, err } - // TODO: add isDeposit flag here return w.Bytes(), nil } @@ -248,7 +241,6 @@ func (info *L1BlockInfo) unmarshalBinaryEcotone(data []byte) error { if info.BatcherAddr, err = solabi.ReadAddress(r); err != nil { return err } - // TODO unmarshall isDeposit flag here if !solabi.EmptyReader(r) { return errors.New("too many bytes") } @@ -300,7 +292,7 @@ func DepositCompletedFromBytes(rollupCfg *rollup.Config, l2BlockTime uint64, dat // L1InfoDeposit creates a L1 Info deposit transaction based on the L1 block, // and the L2 block-height difference with the start of the epoch. -func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, block eth.BlockInfo, l2BlockTime uint64, isDeposit bool) (*types.DepositTx, error) { +func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, block eth.BlockInfo, l2BlockTime uint64) (*types.DepositTx, error) { l1BlockInfo := L1BlockInfo{ Number: block.NumberU64(), Time: block.Time(), @@ -308,7 +300,6 @@ func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber BlockHash: block.Hash(), SequenceNumber: seqNumber, BatcherAddr: sysCfg.BatcherAddr, - IsDeposit: isDeposit, } var data []byte if isEcotoneButNotFirstBlock(rollupCfg, l2BlockTime) { @@ -363,8 +354,8 @@ func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber } // L1InfoDepositBytes returns a serialized L1-info attributes transaction. -func L1InfoDepositBytes(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, l1Info eth.BlockInfo, l2BlockTime uint64, isDeposit bool) ([]byte, error) { - dep, err := L1InfoDeposit(rollupCfg, sysCfg, seqNumber, l1Info, l2BlockTime, isDeposit) +func L1InfoDepositBytes(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, l1Info eth.BlockInfo, l2BlockTime uint64) ([]byte, error) { + dep, err := L1InfoDeposit(rollupCfg, sysCfg, seqNumber, l1Info, l2BlockTime) if err != nil { return nil, fmt.Errorf("failed to create L1 info tx: %w", err) } @@ -384,7 +375,6 @@ func DepositsCompleteDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, BlockHash: block.Hash(), SequenceNumber: seqNumber, BatcherAddr: sysCfg.BatcherAddr, - IsDeposit: false, } data, err := l1BlockInfo.marshalDepositsComplete() if err != nil { @@ -403,7 +393,7 @@ func DepositsCompleteDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, Mint: nil, Value: big.NewInt(0), Gas: 50_000, - IsSystemTransaction: true, + IsSystemTransaction: false, Data: data, } // With the regolith fork we disable the IsSystemTx functionality, and allocate real gas diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 827c44df08b..4f62cf582a4 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -132,6 +132,13 @@ contract L1Block is ISemver, IGasToken { isDeposit = _isDeposit; } + // TODO natspec + function setL1BlockValuesIsthmus() external { + isDeposit = true; + setL1BlockValuesEcotone(calldata); + // TODO ^ this is an example... we might need to do an assembly low-level call here + } + /// @notice Updates the L1 block values for an Ecotone upgraded chain. /// Params are packed and passed in as raw msg.data instead of ABI to reduce calldata size. /// Params are expected to be in the following order: @@ -144,8 +151,7 @@ contract L1Block is ISemver, IGasToken { /// 7. _blobBaseFee L1 blob base fee. /// 8. _hash L1 blockhash. /// 9. _batcherHash Versioned hash to authenticate batcher by. - /// 10. _isDeposit isDeposit flag - function setL1BlockValuesIsthmus() external { + function setL1BlockValuesEcotone() external { address depositor = DEPOSITOR_ACCOUNT(); assembly { // Revert if the caller is not the depositor account. @@ -161,9 +167,6 @@ contract L1Block is ISemver, IGasToken { sstore(blobBaseFee.slot, calldataload(68)) // uint256 sstore(hash.slot, calldataload(100)) // bytes32 sstore(batcherHash.slot, calldataload(132)) // bytes32 - sstore(isDeposit.slot, calldataload(133)) // boolean - // TODO ^ not sure we can just add a boolean as the last slot - // (could not find how to client-side test this) } } From 65b680416300dd11f0523f4595f8377295c98217 Mon Sep 17 00:00:00 2001 From: Skeletor Spaceman Date: Tue, 6 Aug 2024 19:50:22 -0300 Subject: [PATCH 05/23] feat: simplified deposits complete tx and added Isthmus L1Info tx --- op-node/rollup/derive/attributes.go | 2 +- op-node/rollup/derive/l1_block_info.go | 113 ++++++++++++------------- 2 files changed, 56 insertions(+), 59 deletions(-) diff --git a/op-node/rollup/derive/attributes.go b/op-node/rollup/derive/attributes.go index a4e4c614c24..a64ecf7a714 100644 --- a/op-node/rollup/derive/attributes.go +++ b/op-node/rollup/derive/attributes.go @@ -123,7 +123,7 @@ func (ba *FetchingAttributesBuilder) PreparePayloadAttributes(ctx context.Contex var postDeposits []hexutil.Bytes if ba.rollupCfg.IsInterop(nextL2Time) { - depositsCompleteTx, err := DepositsCompleteBytes(ba.rollupCfg, sysConfig, seqNumber, l1Info, nextL2Time) + depositsCompleteTx, err := DepositsCompleteBytes(seqNumber, l1Info) if err != nil { return nil, NewCriticalError(fmt.Errorf("failed to create depositsCompleteTx: %w", err)) } diff --git a/op-node/rollup/derive/l1_block_info.go b/op-node/rollup/derive/l1_block_info.go index b2be8d0859d..842f13a7788 100644 --- a/op-node/rollup/derive/l1_block_info.go +++ b/op-node/rollup/derive/l1_block_info.go @@ -20,6 +20,7 @@ import ( const ( L1InfoFuncBedrockSignature = "setL1BlockValues(uint64,uint64,uint256,bytes32,uint64,bytes32,uint256,uint256)" L1InfoFuncEcotoneSignature = "setL1BlockValuesEcotone()" + L1InfoFuncIsthmusSignature = "setL2BlockValuesIsthmus()" DepositsCompleteSignature = "depositsComplete()" L1InfoArguments = 8 L1InfoBedrockLen = 4 + 32*L1InfoArguments @@ -30,6 +31,7 @@ const ( var ( L1InfoFuncBedrockBytes4 = crypto.Keccak256([]byte(L1InfoFuncBedrockSignature))[:4] L1InfoFuncEcotoneBytes4 = crypto.Keccak256([]byte(L1InfoFuncEcotoneSignature))[:4] + L1InfoFuncIsthmusBytes4 = crypto.Keccak256([]byte(L1InfoFuncIsthmusSignature))[:4] DepositsCompleteBytes4 = crypto.Keccak256([]byte(DepositsCompleteSignature))[:4] L1InfoDepositerAddress = common.HexToAddress("0xdeaddeaddeaddeaddeaddeaddeaddeaddead0001") L1BlockAddress = predeploys.L1BlockAddr @@ -147,7 +149,7 @@ func (info *L1BlockInfo) unmarshalBinaryBedrock(data []byte) error { return nil } -// Ecotone Binary Format +// Isthmus & Ecotone Binary Format // +---------+--------------------------+ // | Bytes | Field | // +---------+--------------------------+ @@ -162,7 +164,17 @@ func (info *L1BlockInfo) unmarshalBinaryBedrock(data []byte) error { // | 32 | BlockHash | // | 32 | BatcherHash | // +---------+--------------------------+ +func (info *L1BlockInfo) marshalBinaryIsthmus() ([]byte, error) { + // Isthmus is the same as Ecotone + out, err := info.marshalBinaryEcotone() + if err != nil { + return nil, fmt.Errorf("failed to marshal Ecotone l1 block info: %w", err) + } + // replace the first 4 bytes of the signature with L1InfoFuncIsthmusBytes4 + copy(out[:4], L1InfoFuncIsthmusBytes4) // CHECK: is this correct? or is it too nasty? + return out, nil +} func (info *L1BlockInfo) marshalBinaryEcotone() ([]byte, error) { w := bytes.NewBuffer(make([]byte, 0, L1InfoEcotoneLen)) if err := solabi.WriteSignature(w, L1InfoFuncEcotoneBytes4); err != nil { @@ -203,15 +215,29 @@ func (info *L1BlockInfo) marshalBinaryEcotone() ([]byte, error) { return w.Bytes(), nil } +func (info *L1BlockInfo) unmarshalBinaryIsthmus(data []byte) error { + return info.unmarshalBinaryEcotoneAndIsthmus(data, true) +} func (info *L1BlockInfo) unmarshalBinaryEcotone(data []byte) error { + return info.unmarshalBinaryEcotoneAndIsthmus(data) +} + +// optional isIsthmus flag +func (info *L1BlockInfo) unmarshalBinaryEcotoneAndIsthmus(data []byte, isIsthmus ...bool) error { if len(data) != L1InfoEcotoneLen { return fmt.Errorf("data is unexpected length: %d", len(data)) } r := bytes.NewReader(data) var err error - if _, err := solabi.ReadAndValidateSignature(r, L1InfoFuncEcotoneBytes4); err != nil { - return err + if isIsthmus != nil && isIsthmus[0] { + if _, err := solabi.ReadAndValidateSignature(r, L1InfoFuncIsthmusBytes4); err != nil { + return err + } + } else { + if _, err := solabi.ReadAndValidateSignature(r, L1InfoFuncEcotoneBytes4); err != nil { + return err + } } if err := binary.Read(r, binary.BigEndian, &info.BaseFeeScalar); err != nil { return ErrInvalidFormat @@ -253,41 +279,22 @@ func isEcotoneButNotFirstBlock(rollupCfg *rollup.Config, l2BlockTime uint64) boo return rollupCfg.IsEcotone(l2BlockTime) && !rollupCfg.IsEcotoneActivationBlock(l2BlockTime) } +// isInteropButNotFirstBlock returns whether the specified block is subject to the Isthmus upgrade, +// but is not the actiation block itself. +func isInteropButNotFirstBlock(rollupCfg *rollup.Config, l2BlockTime uint64) bool { + return rollupCfg.IsInterop(l2BlockTime) && !rollupCfg.IsInteropActivationBlock(l2BlockTime) +} + // L1BlockInfoFromBytes is the inverse of L1InfoDeposit, to see where the L2 chain is derived from func L1BlockInfoFromBytes(rollupCfg *rollup.Config, l2BlockTime uint64, data []byte) (*L1BlockInfo, error) { var info L1BlockInfo if isEcotoneButNotFirstBlock(rollupCfg, l2BlockTime) { return &info, info.unmarshalBinaryEcotone(data) } - return &info, info.unmarshalBinaryBedrock(data) -} - -func (info *L1BlockInfo) marshalDepositsComplete() ([]byte, error) { - w := bytes.NewBuffer(make([]byte, 0, DepositsCompleteLen)) - if err := solabi.WriteSignature(w, []byte(DepositsCompleteBytes4)); err != nil { - return nil, err - } - return w.Bytes(), nil -} - -func (info *L1BlockInfo) unmarshalDepositsComplete(data []byte) error { - if len(data) != DepositsCompleteLen { - return fmt.Errorf("data is unexpected length: %d", len(data)) - } - r := bytes.NewReader(data) - - if _, err := solabi.ReadAndValidateSignature(r, DepositsCompleteBytes4); err != nil { - return err - } - if !solabi.EmptyReader(r) { - return errors.New("too many bytes") + if isInteropButNotFirstBlock(rollupCfg, l2BlockTime) { + return &info, info.unmarshalBinaryIsthmus(data) } - return nil -} - -func DepositCompletedFromBytes(rollupCfg *rollup.Config, l2BlockTime uint64, data []byte) (*L1BlockInfo, error) { - var info L1BlockInfo - return &info, info.unmarshalDepositsComplete(data) + return &info, info.unmarshalBinaryBedrock(data) } // L1InfoDeposit creates a L1 Info deposit transaction based on the L1 block, @@ -302,6 +309,7 @@ func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber BatcherAddr: sysCfg.BatcherAddr, } var data []byte + if isEcotoneButNotFirstBlock(rollupCfg, l2BlockTime) { l1BlockInfo.BlobBaseFee = block.BlobBaseFee() if l1BlockInfo.BlobBaseFee == nil { @@ -314,11 +322,19 @@ func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber } l1BlockInfo.BlobBaseFeeScalar = scalars.BlobBaseFeeScalar l1BlockInfo.BaseFeeScalar = scalars.BaseFeeScalar - out, err := l1BlockInfo.marshalBinaryEcotone() - if err != nil { - return nil, fmt.Errorf("failed to marshal Ecotone l1 block info: %w", err) + if isInteropButNotFirstBlock(rollupCfg, l2BlockTime) { + out, err := l1BlockInfo.marshalBinaryIsthmus() + if err != nil { + return nil, fmt.Errorf("failed to marshal Isthmus l1 block info: %w", err) + } + data = out + } else { + out, err := l1BlockInfo.marshalBinaryEcotone() + if err != nil { + return nil, fmt.Errorf("failed to marshal Ecotone l1 block info: %w", err) + } + data = out } - data = out } else { l1BlockInfo.L1FeeOverhead = sysCfg.Overhead l1BlockInfo.L1FeeScalar = sysCfg.Scalar @@ -367,25 +383,11 @@ func L1InfoDepositBytes(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNu return opaqueL1Tx, nil } -func DepositsCompleteDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, block eth.BlockInfo, l2BlockTime uint64) (*types.DepositTx, error) { - l1BlockInfo := L1BlockInfo{ - Number: block.NumberU64(), - Time: block.Time(), - BaseFee: block.BaseFee(), - BlockHash: block.Hash(), - SequenceNumber: seqNumber, - BatcherAddr: sysCfg.BatcherAddr, - } - data, err := l1BlockInfo.marshalDepositsComplete() - if err != nil { - return nil, fmt.Errorf("failed to marshal Bedrock l1 block info: %w", err) - } +func DepositsCompleteDeposit(seqNumber uint64, block eth.BlockInfo) (*types.DepositTx, error) { source := L1InfoDepositSource{ L1BlockHash: block.Hash(), SeqNumber: seqNumber, } - // Set a normal gas limit with `IsSystemTransaction` to ensure - // that the DepositCompleted Transaction does not run out of gas. out := &types.DepositTx{ SourceHash: source.SourceHash(), From: L1InfoDepositerAddress, @@ -394,18 +396,13 @@ func DepositsCompleteDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, Value: big.NewInt(0), Gas: 50_000, IsSystemTransaction: false, - Data: data, - } - // With the regolith fork we disable the IsSystemTx functionality, and allocate real gas - if rollupCfg.IsRegolith(l2BlockTime) { - out.IsSystemTransaction = false - out.Gas = RegolithSystemTxGas + Data: DepositsCompleteBytes4, } return out, nil } -func DepositsCompleteBytes(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber uint64, l1Info eth.BlockInfo, l2BlockTime uint64) ([]byte, error) { - dep, err := DepositsCompleteDeposit(rollupCfg, sysCfg, seqNumber, l1Info, l2BlockTime) +func DepositsCompleteBytes(seqNumber uint64, l1Info eth.BlockInfo) ([]byte, error) { + dep, err := DepositsCompleteDeposit(seqNumber, l1Info) if err != nil { return nil, fmt.Errorf("failed to create DepositsComplete tx: %w", err) } From 70b7db0d27eda6cc59add29e4c2c18850b1eb0b2 Mon Sep 17 00:00:00 2001 From: Skeletor Spaceman Date: Wed, 7 Aug 2024 11:37:07 -0300 Subject: [PATCH 06/23] feat: L1Block make ecotone a public function --- packages/contracts-bedrock/src/L2/L1Block.sol | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 4f62cf582a4..6c90f70738e 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -135,8 +135,7 @@ contract L1Block is ISemver, IGasToken { // TODO natspec function setL1BlockValuesIsthmus() external { isDeposit = true; - setL1BlockValuesEcotone(calldata); - // TODO ^ this is an example... we might need to do an assembly low-level call here + setL1BlockValuesEcotone(); // Internally call the Ecotone function } /// @notice Updates the L1 block values for an Ecotone upgraded chain. @@ -151,7 +150,7 @@ contract L1Block is ISemver, IGasToken { /// 7. _blobBaseFee L1 blob base fee. /// 8. _hash L1 blockhash. /// 9. _batcherHash Versioned hash to authenticate batcher by. - function setL1BlockValuesEcotone() external { + function setL1BlockValuesEcotone() public { address depositor = DEPOSITOR_ACCOUNT(); assembly { // Revert if the caller is not the depositor account. From 24deb1c1a08210fc39488f2759591df2b98fd19e Mon Sep 17 00:00:00 2001 From: Skeletor Spaceman Date: Wed, 7 Aug 2024 12:02:21 -0300 Subject: [PATCH 07/23] feat: re-organized shared functions --- op-node/rollup/derive/l1_block_info.go | 56 ++++++++++++++++---------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/op-node/rollup/derive/l1_block_info.go b/op-node/rollup/derive/l1_block_info.go index 842f13a7788..f307c7405a7 100644 --- a/op-node/rollup/derive/l1_block_info.go +++ b/op-node/rollup/derive/l1_block_info.go @@ -164,21 +164,33 @@ func (info *L1BlockInfo) unmarshalBinaryBedrock(data []byte) error { // | 32 | BlockHash | // | 32 | BatcherHash | // +---------+--------------------------+ + func (info *L1BlockInfo) marshalBinaryIsthmus() ([]byte, error) { - // Isthmus is the same as Ecotone - out, err := info.marshalBinaryEcotone() + out, err := marshalBinaryEcotoneOrIsthmus(info, true) if err != nil { - return nil, fmt.Errorf("failed to marshal Ecotone l1 block info: %w", err) + return nil, fmt.Errorf("failed to marshal Isthmus l1 block info: %w", err) } - // replace the first 4 bytes of the signature with L1InfoFuncIsthmusBytes4 - copy(out[:4], L1InfoFuncIsthmusBytes4) // CHECK: is this correct? or is it too nasty? return out, nil - } + func (info *L1BlockInfo) marshalBinaryEcotone() ([]byte, error) { - w := bytes.NewBuffer(make([]byte, 0, L1InfoEcotoneLen)) - if err := solabi.WriteSignature(w, L1InfoFuncEcotoneBytes4); err != nil { - return nil, err + out, err := marshalBinaryEcotoneOrIsthmus(info, true) + if err != nil { + return nil, fmt.Errorf("failed to marshal Ecotone l1 block info: %w", err) + } + return out, nil +} + +func marshalBinaryEcotoneOrIsthmus(info *L1BlockInfo, isIsthmus ...bool) ([]byte, error) { + w := bytes.NewBuffer(make([]byte, 0, L1InfoEcotoneLen)) // Ecotone and Isthmus have the same length + if isIsthmus != nil && isIsthmus[0] { + if err := solabi.WriteSignature(w, L1InfoFuncIsthmusBytes4); err != nil { + return nil, err + } + } else { + if err := solabi.WriteSignature(w, L1InfoFuncEcotoneBytes4); err != nil { + return nil, err + } } if err := binary.Write(w, binary.BigEndian, info.BaseFeeScalar); err != nil { return nil, err @@ -216,14 +228,13 @@ func (info *L1BlockInfo) marshalBinaryEcotone() ([]byte, error) { } func (info *L1BlockInfo) unmarshalBinaryIsthmus(data []byte) error { - return info.unmarshalBinaryEcotoneAndIsthmus(data, true) + return unmarshalBinaryEcotoneAndIsthmus(info, data, true) } func (info *L1BlockInfo) unmarshalBinaryEcotone(data []byte) error { - return info.unmarshalBinaryEcotoneAndIsthmus(data) + return unmarshalBinaryEcotoneAndIsthmus(info, data) } -// optional isIsthmus flag -func (info *L1BlockInfo) unmarshalBinaryEcotoneAndIsthmus(data []byte, isIsthmus ...bool) error { +func unmarshalBinaryEcotoneAndIsthmus(info *L1BlockInfo, data []byte, isIsthmus ...bool) error { if len(data) != L1InfoEcotoneLen { return fmt.Errorf("data is unexpected length: %d", len(data)) } @@ -288,12 +299,13 @@ func isInteropButNotFirstBlock(rollupCfg *rollup.Config, l2BlockTime uint64) boo // L1BlockInfoFromBytes is the inverse of L1InfoDeposit, to see where the L2 chain is derived from func L1BlockInfoFromBytes(rollupCfg *rollup.Config, l2BlockTime uint64, data []byte) (*L1BlockInfo, error) { var info L1BlockInfo - if isEcotoneButNotFirstBlock(rollupCfg, l2BlockTime) { - return &info, info.unmarshalBinaryEcotone(data) - } + // Important, this should be ordered from most recent to oldest if isInteropButNotFirstBlock(rollupCfg, l2BlockTime) { return &info, info.unmarshalBinaryIsthmus(data) } + if isEcotoneButNotFirstBlock(rollupCfg, l2BlockTime) { + return &info, info.unmarshalBinaryEcotone(data) + } return &info, info.unmarshalBinaryBedrock(data) } @@ -310,7 +322,7 @@ func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber } var data []byte - if isEcotoneButNotFirstBlock(rollupCfg, l2BlockTime) { + if isInteropButNotFirstBlock(rollupCfg, l2BlockTime) { l1BlockInfo.BlobBaseFee = block.BlobBaseFee() if l1BlockInfo.BlobBaseFee == nil { // The L2 spec states to use the MIN_BLOB_GASPRICE from EIP-4844 if not yet active on L1. @@ -322,16 +334,16 @@ func L1InfoDeposit(rollupCfg *rollup.Config, sysCfg eth.SystemConfig, seqNumber } l1BlockInfo.BlobBaseFeeScalar = scalars.BlobBaseFeeScalar l1BlockInfo.BaseFeeScalar = scalars.BaseFeeScalar - if isInteropButNotFirstBlock(rollupCfg, l2BlockTime) { - out, err := l1BlockInfo.marshalBinaryIsthmus() + if isEcotoneButNotFirstBlock(rollupCfg, l2BlockTime) { + out, err := l1BlockInfo.marshalBinaryEcotone() if err != nil { - return nil, fmt.Errorf("failed to marshal Isthmus l1 block info: %w", err) + return nil, fmt.Errorf("failed to marshal Ecotone l1 block info: %w", err) } data = out } else { - out, err := l1BlockInfo.marshalBinaryEcotone() + out, err := l1BlockInfo.marshalBinaryIsthmus() if err != nil { - return nil, fmt.Errorf("failed to marshal Ecotone l1 block info: %w", err) + return nil, fmt.Errorf("failed to marshal Isthmus l1 block info: %w", err) } data = out } From 785eaf4adfcba953e55aed1d3fe8a990bdcd20d7 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Tue, 6 Aug 2024 20:42:59 -0300 Subject: [PATCH 08/23] fix: contracts compiler errors --- .../contracts-bedrock/src/L2/CrossL2Inbox.sol | 7 ++++- packages/contracts-bedrock/src/L2/L1Block.sol | 26 ++++++++++++------- .../test/L2/GasPriceOracle.t.sol | 5 +++- .../contracts-bedrock/test/L2/L1Block.t.sol | 12 ++++++--- .../test/legacy/L1BlockNumber.t.sol | 4 ++- 5 files changed, 37 insertions(+), 17 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol b/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol index b85d09580ad..ba8d941e49f 100644 --- a/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol +++ b/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol @@ -17,6 +17,11 @@ interface IDependencySet { function isInDependencySet(uint256 _chainId) external view returns (bool); } +// TODO(disco): +interface IL1Block { + function isDeposit() external view returns (bool); +} + /// @notice Thrown when a non-written transient storage slot is attempted to be read from. error NotEntered(); @@ -118,7 +123,7 @@ contract CrossL2Inbox is ICrossL2Inbox, ISemver, TransientReentrancyAware { reentrantAware { // We need to know if this is being called on a depositTx - if (L1_BLOCK.isDeposit()) revert NoExecutingDeposits(); + if (IL1Block(Predeploys.L1_BLOCK_ATTRIBUTES).isDeposit()) revert NoExecutingDeposits(); if (_id.timestamp > block.timestamp) revert InvalidTimestamp(); if (!IDependencySet(Predeploys.L1_BLOCK_ATTRIBUTES).isInDependencySet(_id.chainId)) { diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 6c90f70738e..06790a6b404 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -22,6 +22,12 @@ contract L1Block is ISemver, IGasToken { addr_ = Constants.DEPOSITOR_ACCOUNT; } + address public constant CROSS_L2_INBOX = address(0x4200000000000000000000000000000000000022); + + // TODO(disco): check slot + /// @notice The isDeposit flag. + bool internal isDepositTransaction; + /// @notice The latest L1 block number known by the L2 system. uint64 public number; @@ -57,9 +63,6 @@ contract L1Block is ISemver, IGasToken { /// @notice The latest L1 blob base fee. uint256 public blobBaseFee; - /// @notice The isDeposit flag. - bool public isDeposit; - /// @custom:semver 1.4.1-beta.1 function version() public pure virtual returns (string memory) { return "1.4.1-beta.1"; @@ -90,9 +93,10 @@ contract L1Block is ISemver, IGasToken { return token != Constants.ETHER; } - function isDeposit() returns (bool _isDeposit) external view { + // TODO(disco): natspec + function isDeposit() external view returns (bool _isDeposit) { require(msg.sender == CROSS_L2_INBOX, "L1Block: only the CrossL2Inbox can check if it is a deposit"); - _isDeposit = isDeposit; + _isDeposit = isDepositTransaction; } /// @custom:legacy @@ -129,13 +133,15 @@ contract L1Block is ISemver, IGasToken { batcherHash = _batcherHash; l1FeeOverhead = _l1FeeOverhead; l1FeeScalar = _l1FeeScalar; - isDeposit = _isDeposit; + isDepositTransaction = _isDeposit; } - // TODO natspec + // TODO(disco) natspec function setL1BlockValuesIsthmus() external { - isDeposit = true; - setL1BlockValuesEcotone(); // Internally call the Ecotone function + isDepositTransaction = true; + // TODO(disco): need to check calldata is proxied here, otherwise we might need to do an assembly low-level call + // here + setL1BlockValuesEcotone(); } /// @notice Updates the L1 block values for an Ecotone upgraded chain. @@ -183,6 +189,6 @@ contract L1Block is ISemver, IGasToken { /// @notice Resets the isDeposit flag. function depositsComplete() external { if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); - isDeposit = false; + isDepositTransaction = false; } } diff --git a/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol b/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol index 21f7b98f666..f3abe50450f 100644 --- a/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol +++ b/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol @@ -27,6 +27,8 @@ contract GasPriceOracle_Test is CommonTest { uint256 constant l1FeeScalar = 10; uint32 constant blobBaseFeeScalar = 15; uint32 constant baseFeeScalar = 20; + // TODO(disco): check + bool constant isDeposit = true; /// @dev Sets up the test suite. function setUp() public virtual override { @@ -52,7 +54,8 @@ contract GasPriceOracleBedrock_Test is GasPriceOracle_Test { _sequenceNumber: sequenceNumber, _batcherHash: batcherHash, _l1FeeOverhead: l1FeeOverhead, - _l1FeeScalar: l1FeeScalar + _l1FeeScalar: l1FeeScalar, + _isDeposit: isDeposit }); } diff --git a/packages/contracts-bedrock/test/L2/L1Block.t.sol b/packages/contracts-bedrock/test/L2/L1Block.t.sol index ce67a669222..6db2cc10fe9 100644 --- a/packages/contracts-bedrock/test/L2/L1Block.t.sol +++ b/packages/contracts-bedrock/test/L2/L1Block.t.sol @@ -34,12 +34,13 @@ contract L1BlockBedrock_Test is L1BlockTest { uint64 s, bytes32 bt, uint256 fo, - uint256 fs + uint256 fs, + bool d ) external { vm.prank(depositor); - l1Block.setL1BlockValues(n, t, b, h, s, bt, fo, fs); + l1Block.setL1BlockValues(n, t, b, h, s, bt, fo, fs, d); assertEq(l1Block.number(), n); assertEq(l1Block.timestamp(), t); assertEq(l1Block.basefee(), b); @@ -48,6 +49,7 @@ contract L1BlockBedrock_Test is L1BlockTest { assertEq(l1Block.batcherHash(), bt); assertEq(l1Block.l1FeeOverhead(), fo); assertEq(l1Block.l1FeeScalar(), fs); + assertEq(l1Block.isDeposit(), d); } /// @dev Tests that `setL1BlockValues` can set max values. @@ -61,7 +63,8 @@ contract L1BlockBedrock_Test is L1BlockTest { _sequenceNumber: type(uint64).max, _batcherHash: bytes32(type(uint256).max), _l1FeeOverhead: type(uint256).max, - _l1FeeScalar: type(uint256).max + _l1FeeScalar: type(uint256).max, + _isDeposit: true }); } @@ -76,7 +79,8 @@ contract L1BlockBedrock_Test is L1BlockTest { _sequenceNumber: type(uint64).max, _batcherHash: bytes32(type(uint256).max), _l1FeeOverhead: type(uint256).max, - _l1FeeScalar: type(uint256).max + _l1FeeScalar: type(uint256).max, + _isDeposit: true }); } } diff --git a/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol b/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol index 3fad9769923..d8ca40ba561 100644 --- a/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol +++ b/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol @@ -32,7 +32,9 @@ contract L1BlockNumberTest is Test { _sequenceNumber: uint64(4), _batcherHash: bytes32(uint256(0)), _l1FeeOverhead: 2, - _l1FeeScalar: 3 + _l1FeeScalar: 3, + // TODO(disco) + _isDeposit: false }); } From 9ef55a26f2100796da81138df6995563cc0fb1ce Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 13:03:08 -0300 Subject: [PATCH 09/23] feat: create some set l1 block values ithmus test * refactor: not cross l2 inbox error --- packages/contracts-bedrock/src/L2/L1Block.sol | 14 +-- .../src/libraries/L1BlockErrors.sol | 3 + .../contracts-bedrock/test/L2/L1Block.t.sol | 101 +++++++++++++++++- 3 files changed, 110 insertions(+), 8 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 06790a6b404..184dd98f019 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -95,7 +95,7 @@ contract L1Block is ISemver, IGasToken { // TODO(disco): natspec function isDeposit() external view returns (bool _isDeposit) { - require(msg.sender == CROSS_L2_INBOX, "L1Block: only the CrossL2Inbox can check if it is a deposit"); + if (msg.sender != CROSS_L2_INBOX) revert NotCrossL2Inbox(); _isDeposit = isDepositTransaction; } @@ -175,6 +175,12 @@ contract L1Block is ISemver, IGasToken { } } + /// @notice Resets the isDeposit flag. + function depositsComplete() external { + if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); + isDepositTransaction = false; + } + /// @notice Sets the gas paying token for the L2 system. Can only be called by the special /// depositor account. This function is not called on every L2 block but instead /// only called by specially crafted L1 deposit transactions. @@ -185,10 +191,4 @@ contract L1Block is ISemver, IGasToken { emit GasPayingTokenSet({ token: _token, decimals: _decimals, name: _name, symbol: _symbol }); } - - /// @notice Resets the isDeposit flag. - function depositsComplete() external { - if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); - isDepositTransaction = false; - } } diff --git a/packages/contracts-bedrock/src/libraries/L1BlockErrors.sol b/packages/contracts-bedrock/src/libraries/L1BlockErrors.sol index c9ef3903aeb..44e156e158c 100644 --- a/packages/contracts-bedrock/src/libraries/L1BlockErrors.sol +++ b/packages/contracts-bedrock/src/libraries/L1BlockErrors.sol @@ -4,6 +4,9 @@ pragma solidity ^0.8.0; /// @notice Error returns when a non-depositor account tries to set L1 block values. error NotDepositor(); +/// @notice Error when a non-cross L2 Inbox sender tries to call the `isDeposit()` method. +error NotCrossL2Inbox(); + /// @notice Error when a chain ID is not in the interop dependency set. error NotDependency(); diff --git a/packages/contracts-bedrock/test/L2/L1Block.t.sol b/packages/contracts-bedrock/test/L2/L1Block.t.sol index 6db2cc10fe9..82979b3e867 100644 --- a/packages/contracts-bedrock/test/L2/L1Block.t.sol +++ b/packages/contracts-bedrock/test/L2/L1Block.t.sol @@ -10,7 +10,8 @@ import { Constants } from "src/libraries/Constants.sol"; // Target contract import { L1Block } from "src/L2/L1Block.sol"; -import "src/libraries/L1BlockErrors.sol"; +import { NotDepositor, NotCrossL2Inbox } from "src/libraries/L1BlockErrors.sol"; +import { Predeploys } from "src/libraries/Predeploys.sol"; contract L1BlockTest is CommonTest { address depositor; @@ -24,6 +25,38 @@ contract L1BlockTest is CommonTest { } } +contract L1BlockIsDeposit_Test is L1BlockTest { + /// @dev Tests that `isDeposit` reverts if the caller is not the cross L2 inbox. + function test_isDeposit_notCrossL2Inbox_reverts(address _caller) external { + vm.assume(_caller != Predeploys.CROSS_L2_INBOX); + vm.expectRevert(NotCrossL2Inbox.selector); + l1Block.isDeposit(); + } + + /// @dev Tests that `isDeposit` always returns the correct value. + function test_isDeposit_succeeds(bool _isDeposit) external { + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), false); + + /// @dev Assuming that `setL1BlockValues` will set the proper value. That function is tested as well + vm.prank(depositor); + l1Block.setL1BlockValues({ + _number: type(uint64).max, + _timestamp: type(uint64).max, + _basefee: type(uint256).max, + _hash: keccak256(abi.encode(1)), + _sequenceNumber: type(uint64).max, + _batcherHash: bytes32(type(uint256).max), + _l1FeeOverhead: type(uint256).max, + _l1FeeScalar: type(uint256).max, + _isDeposit: _isDeposit + }); + + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), _isDeposit); + } +} + contract L1BlockBedrock_Test is L1BlockTest { // @dev Tests that `setL1BlockValues` updates the values correctly. function testFuzz_updatesValues_succeeds( @@ -85,6 +118,33 @@ contract L1BlockBedrock_Test is L1BlockTest { } } +contract L1BlockSetL1BlockValuesIsthmus_Test is L1BlockTest { + /// @dev Tests that `setL1BlockValuesIsthmus` reverts if sender address is not the depositor + function test_setL1BlockValuesIsthmus_notDepositor_reverts(address _caller) external { + vm.assume(_caller != depositor); + vm.prank(_caller); + vm.expectRevert(NotDepositor.selector); + l1Block.setL1BlockValuesIsthmus(); + } + + /// @dev Tests that `setL1BlockValuesIsthmus` succeeds if sender address is the depositor + function test_setL1BlockValuesIsthmus_succeeds() external { + // Ensure the `isDepositTransaction` flag is false before calling `setL1BlockValuesIsthmus` + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), false); + + vm.prank(depositor); + l1Block.setL1BlockValuesIsthmus(); + + // Assert that the `isDepositTransaction` flag was properly set to true + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), true); + + // Assert `setL1BlockValuesEcotone` was properly called, forwarding the calldata to it + // TODO(disco) + } +} + contract L1BlockEcotone_Test is L1BlockTest { /// @dev Tests that setL1BlockValuesEcotone updates the values appropriately. function testFuzz_setL1BlockValuesEcotone_succeeds( @@ -172,6 +232,45 @@ contract L1BlockEcotone_Test is L1BlockTest { } } +contract L1BlockDepositsComplete_Test is L1BlockTest { + // @dev Tests that `depositsComplete` reverts if the caller is not the depositor. + function test_deposits_is_depositor_reverts(address _caller) external { + vm.assume(_caller != depositor); + vm.expectRevert(NotDepositor.selector); + l1Block.depositsComplete(); + } + + // @dev Tests that `depositsComplete` succeeds if the caller is the depositor. + function test_depositsComplete_succeeds() external { + // Set the `isDeposit` flag to true + vm.prank(depositor); + l1Block.setL1BlockValues({ + _number: type(uint64).max, + _timestamp: type(uint64).max, + _basefee: type(uint256).max, + _hash: keccak256(abi.encode(1)), + _sequenceNumber: type(uint64).max, + _batcherHash: bytes32(type(uint256).max), + _l1FeeOverhead: type(uint256).max, + _l1FeeScalar: type(uint256).max, + _isDeposit: true + }); + + // Assert that the `isDeposit` flag was properly set to true + vm.prank(Predeploys.CROSS_L2_INBOX); + assertTrue(l1Block.isDeposit()); + + // Call `depositsComplete` + vm.prank(depositor); + l1Block.depositsComplete(); + + // Assert that the `isDeposit` flag was properly set to false + /// @dev Assuming that `isDeposit()` wil return the proper value. That function is tested as well + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), false); + } +} + contract L1BlockCustomGasToken_Test is L1BlockTest { function testFuzz_setGasPayingToken_succeeds( address _token, From f463e8a19246f5c640240c590ebeeaf47c0cda3f Mon Sep 17 00:00:00 2001 From: Skeletor Spaceman Date: Wed, 7 Aug 2024 14:33:01 -0300 Subject: [PATCH 10/23] feat: revamp unmarshalBinaryIsthmusAndEcotone function --- op-node/rollup/derive/l1_block_info.go | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/op-node/rollup/derive/l1_block_info.go b/op-node/rollup/derive/l1_block_info.go index f307c7405a7..c91f9d0255f 100644 --- a/op-node/rollup/derive/l1_block_info.go +++ b/op-node/rollup/derive/l1_block_info.go @@ -228,27 +228,21 @@ func marshalBinaryEcotoneOrIsthmus(info *L1BlockInfo, isIsthmus ...bool) ([]byte } func (info *L1BlockInfo) unmarshalBinaryIsthmus(data []byte) error { - return unmarshalBinaryEcotoneAndIsthmus(info, data, true) + return unmarshalBinaryWithSignatureAndData(info, L1InfoFuncIsthmusBytes4, data) } func (info *L1BlockInfo) unmarshalBinaryEcotone(data []byte) error { - return unmarshalBinaryEcotoneAndIsthmus(info, data) + return unmarshalBinaryWithSignatureAndData(info, L1InfoFuncEcotoneBytes4, data) } -func unmarshalBinaryEcotoneAndIsthmus(info *L1BlockInfo, data []byte, isIsthmus ...bool) error { +func unmarshalBinaryWithSignatureAndData(info *L1BlockInfo, signature []byte, data []byte) error { if len(data) != L1InfoEcotoneLen { return fmt.Errorf("data is unexpected length: %d", len(data)) } r := bytes.NewReader(data) var err error - if isIsthmus != nil && isIsthmus[0] { - if _, err := solabi.ReadAndValidateSignature(r, L1InfoFuncIsthmusBytes4); err != nil { - return err - } - } else { - if _, err := solabi.ReadAndValidateSignature(r, L1InfoFuncEcotoneBytes4); err != nil { - return err - } + if _, err := solabi.ReadAndValidateSignature(r, signature); err != nil { + return err } if err := binary.Read(r, binary.BigEndian, &info.BaseFeeScalar); err != nil { return ErrInvalidFormat From 0567b207082bbab9a7cef0dd9a22b65d15510118 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 14:37:06 -0300 Subject: [PATCH 11/23] chore: checkpoint debugging test --- packages/contracts-bedrock/src/L2/L1Block.sol | 32 ++++++-- .../contracts-bedrock/test/L2/L1Block.t.sol | 82 +++++++++++++------ 2 files changed, 82 insertions(+), 32 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 184dd98f019..5285b512f42 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.15; import { ISemver } from "src/universal/ISemver.sol"; import { Constants } from "src/libraries/Constants.sol"; import { GasPayingToken, IGasToken } from "src/libraries/GasPayingToken.sol"; +import { Predeploys } from "src/libraries/Predeploys.sol"; import "src/libraries/L1BlockErrors.sol"; /// @custom:proxied @@ -22,8 +23,6 @@ contract L1Block is ISemver, IGasToken { addr_ = Constants.DEPOSITOR_ACCOUNT; } - address public constant CROSS_L2_INBOX = address(0x4200000000000000000000000000000000000022); - // TODO(disco): check slot /// @notice The isDeposit flag. bool internal isDepositTransaction; @@ -95,7 +94,7 @@ contract L1Block is ISemver, IGasToken { // TODO(disco): natspec function isDeposit() external view returns (bool _isDeposit) { - if (msg.sender != CROSS_L2_INBOX) revert NotCrossL2Inbox(); + if (msg.sender != Predeploys.CROSS_L2_INBOX) revert NotCrossL2Inbox(); _isDeposit = isDepositTransaction; } @@ -139,9 +138,29 @@ contract L1Block is ISemver, IGasToken { // TODO(disco) natspec function setL1BlockValuesIsthmus() external { isDepositTransaction = true; - // TODO(disco): need to check calldata is proxied here, otherwise we might need to do an assembly low-level call - // here - setL1BlockValuesEcotone(); + + // Capture and adjust calldata + bytes memory callData; + assembly { + // Allocate memory for calldata + let size := calldatasize() + callData := mload(0x40) // allocate free memory pointer + mstore(0x40, add(callData, add(size, 0x20))) // update free memory pointer + mstore(callData, sub(size, 4)) // set the size of the data minus the function selector + calldatacopy(add(callData, 0x20), 4, sub(size, 4)) // copy the calldata to the allocated memory, skipping + // the selector + } + + (bool success,) = address(this).call( + abi.encodePacked( + this.setL1BlockValuesEcotone.selector, + callData // Skip the selector of setL1BlockValuesIsthmus + ) + ); + + if (!success) { + revert("L1Block: failed to set L1 block values"); + } } /// @notice Updates the L1 block values for an Ecotone upgraded chain. @@ -175,6 +194,7 @@ contract L1Block is ISemver, IGasToken { } } + // TODO(disco): Natspec /// @notice Resets the isDeposit flag. function depositsComplete() external { if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); diff --git a/packages/contracts-bedrock/test/L2/L1Block.t.sol b/packages/contracts-bedrock/test/L2/L1Block.t.sol index 82979b3e867..1d08e434ce2 100644 --- a/packages/contracts-bedrock/test/L2/L1Block.t.sol +++ b/packages/contracts-bedrock/test/L2/L1Block.t.sol @@ -82,6 +82,8 @@ contract L1BlockBedrock_Test is L1BlockTest { assertEq(l1Block.batcherHash(), bt); assertEq(l1Block.l1FeeOverhead(), fo); assertEq(l1Block.l1FeeScalar(), fs); + + vm.prank(Predeploys.CROSS_L2_INBOX); assertEq(l1Block.isDeposit(), d); } @@ -118,32 +120,60 @@ contract L1BlockBedrock_Test is L1BlockTest { } } -contract L1BlockSetL1BlockValuesIsthmus_Test is L1BlockTest { - /// @dev Tests that `setL1BlockValuesIsthmus` reverts if sender address is not the depositor - function test_setL1BlockValuesIsthmus_notDepositor_reverts(address _caller) external { - vm.assume(_caller != depositor); - vm.prank(_caller); - vm.expectRevert(NotDepositor.selector); - l1Block.setL1BlockValuesIsthmus(); - } - - /// @dev Tests that `setL1BlockValuesIsthmus` succeeds if sender address is the depositor - function test_setL1BlockValuesIsthmus_succeeds() external { - // Ensure the `isDepositTransaction` flag is false before calling `setL1BlockValuesIsthmus` - vm.prank(Predeploys.CROSS_L2_INBOX); - assertEq(l1Block.isDeposit(), false); - - vm.prank(depositor); - l1Block.setL1BlockValuesIsthmus(); - - // Assert that the `isDepositTransaction` flag was properly set to true - vm.prank(Predeploys.CROSS_L2_INBOX); - assertEq(l1Block.isDeposit(), true); - - // Assert `setL1BlockValuesEcotone` was properly called, forwarding the calldata to it - // TODO(disco) - } -} +// contract L1BlockSetL1BlockValuesIsthmus_Test is L1BlockTest { +// /// @dev Tests that `setL1BlockValuesIsthmus` reverts if sender address is not the depositor +// function test_setL1BlockValuesIsthmus_notDepositor_reverts(address _caller) external { +// vm.assume(_caller != depositor); +// vm.prank(_caller); +// vm.expectRevert(NotDepositor.selector); +// l1Block.setL1BlockValuesIsthmus(); +// } + +// /// @dev Tests that `setL1BlockValuesIsthmus` succeeds if sender address is the depositor +// function test_setL1BlockValuesIsthmus_succeeds( +// uint32 baseFeeScalar, +// uint32 blobBaseFeeScalar, +// uint64 sequenceNumber, +// uint64 timestamp, +// uint64 number, +// uint256 baseFee, +// uint256 blobBaseFee, +// bytes32 hash, +// bytes32 batcherHash +// ) +// external +// { +// // Ensure the `isDepositTransaction` flag is false before calling `setL1BlockValuesIsthmus` +// vm.prank(Predeploys.CROSS_L2_INBOX); +// assertEq(l1Block.isDeposit(), false); + +// bytes memory setValuesEcotoneCalldata = Encoding.encodeSetL1BlockValuesEcotone( +// baseFeeScalar, blobBaseFeeScalar, sequenceNumber, timestamp, number, baseFee, blobBaseFee, hash, +// batcherHash +// ); + +// vm.prank(depositor); +// (bool success,) = +// address(l1Block).call(abi.encodePacked(L1Block.setL1BlockValuesIsthmus.selector, +// setValuesEcotoneCalldata)); +// assertTrue(success, "function call failed"); + +// // Assert that the `isDepositTransaction` flag was properly set to true +// // vm.prank(Predeploys.CROSS_L2_INBOX); +// // assertEq(l1Block.isDeposit(), true); + +// // Assert `setL1BlockValuesEcotone` was properly called, forwarding the calldata to it +// assertEq(l1Block.baseFeeScalar(), baseFeeScalar, "1"); +// assertEq(l1Block.blobBaseFeeScalar(), blobBaseFeeScalar, "2"); +// assertEq(l1Block.sequenceNumber(), sequenceNumber, "3"); +// assertEq(l1Block.timestamp(), timestamp, "4"); +// assertEq(l1Block.number(), number, "5"); +// assertEq(l1Block.basefee(), baseFee, "6"); +// assertEq(l1Block.blobBaseFee(), blobBaseFee, "7"); +// assertEq(l1Block.hash(), hash, "8"); +// assertEq(l1Block.batcherHash(), batcherHash, "9"); +// } +// } contract L1BlockEcotone_Test is L1BlockTest { /// @dev Tests that setL1BlockValuesEcotone updates the values appropriately. From 7ee25307c899a3a29238de68470263f0d3ceeb13 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Tue, 6 Aug 2024 20:42:59 -0300 Subject: [PATCH 12/23] fix: contracts compiler errors --- .../contracts-bedrock/src/L2/CrossL2Inbox.sol | 7 ++++- packages/contracts-bedrock/src/L2/L1Block.sol | 26 ++++++++++++------- .../test/L2/GasPriceOracle.t.sol | 5 +++- .../contracts-bedrock/test/L2/L1Block.t.sol | 12 ++++++--- .../test/legacy/L1BlockNumber.t.sol | 4 ++- 5 files changed, 37 insertions(+), 17 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol b/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol index b85d09580ad..ba8d941e49f 100644 --- a/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol +++ b/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol @@ -17,6 +17,11 @@ interface IDependencySet { function isInDependencySet(uint256 _chainId) external view returns (bool); } +// TODO(disco): +interface IL1Block { + function isDeposit() external view returns (bool); +} + /// @notice Thrown when a non-written transient storage slot is attempted to be read from. error NotEntered(); @@ -118,7 +123,7 @@ contract CrossL2Inbox is ICrossL2Inbox, ISemver, TransientReentrancyAware { reentrantAware { // We need to know if this is being called on a depositTx - if (L1_BLOCK.isDeposit()) revert NoExecutingDeposits(); + if (IL1Block(Predeploys.L1_BLOCK_ATTRIBUTES).isDeposit()) revert NoExecutingDeposits(); if (_id.timestamp > block.timestamp) revert InvalidTimestamp(); if (!IDependencySet(Predeploys.L1_BLOCK_ATTRIBUTES).isInDependencySet(_id.chainId)) { diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 6c90f70738e..06790a6b404 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -22,6 +22,12 @@ contract L1Block is ISemver, IGasToken { addr_ = Constants.DEPOSITOR_ACCOUNT; } + address public constant CROSS_L2_INBOX = address(0x4200000000000000000000000000000000000022); + + // TODO(disco): check slot + /// @notice The isDeposit flag. + bool internal isDepositTransaction; + /// @notice The latest L1 block number known by the L2 system. uint64 public number; @@ -57,9 +63,6 @@ contract L1Block is ISemver, IGasToken { /// @notice The latest L1 blob base fee. uint256 public blobBaseFee; - /// @notice The isDeposit flag. - bool public isDeposit; - /// @custom:semver 1.4.1-beta.1 function version() public pure virtual returns (string memory) { return "1.4.1-beta.1"; @@ -90,9 +93,10 @@ contract L1Block is ISemver, IGasToken { return token != Constants.ETHER; } - function isDeposit() returns (bool _isDeposit) external view { + // TODO(disco): natspec + function isDeposit() external view returns (bool _isDeposit) { require(msg.sender == CROSS_L2_INBOX, "L1Block: only the CrossL2Inbox can check if it is a deposit"); - _isDeposit = isDeposit; + _isDeposit = isDepositTransaction; } /// @custom:legacy @@ -129,13 +133,15 @@ contract L1Block is ISemver, IGasToken { batcherHash = _batcherHash; l1FeeOverhead = _l1FeeOverhead; l1FeeScalar = _l1FeeScalar; - isDeposit = _isDeposit; + isDepositTransaction = _isDeposit; } - // TODO natspec + // TODO(disco) natspec function setL1BlockValuesIsthmus() external { - isDeposit = true; - setL1BlockValuesEcotone(); // Internally call the Ecotone function + isDepositTransaction = true; + // TODO(disco): need to check calldata is proxied here, otherwise we might need to do an assembly low-level call + // here + setL1BlockValuesEcotone(); } /// @notice Updates the L1 block values for an Ecotone upgraded chain. @@ -183,6 +189,6 @@ contract L1Block is ISemver, IGasToken { /// @notice Resets the isDeposit flag. function depositsComplete() external { if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); - isDeposit = false; + isDepositTransaction = false; } } diff --git a/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol b/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol index 21f7b98f666..f3abe50450f 100644 --- a/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol +++ b/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol @@ -27,6 +27,8 @@ contract GasPriceOracle_Test is CommonTest { uint256 constant l1FeeScalar = 10; uint32 constant blobBaseFeeScalar = 15; uint32 constant baseFeeScalar = 20; + // TODO(disco): check + bool constant isDeposit = true; /// @dev Sets up the test suite. function setUp() public virtual override { @@ -52,7 +54,8 @@ contract GasPriceOracleBedrock_Test is GasPriceOracle_Test { _sequenceNumber: sequenceNumber, _batcherHash: batcherHash, _l1FeeOverhead: l1FeeOverhead, - _l1FeeScalar: l1FeeScalar + _l1FeeScalar: l1FeeScalar, + _isDeposit: isDeposit }); } diff --git a/packages/contracts-bedrock/test/L2/L1Block.t.sol b/packages/contracts-bedrock/test/L2/L1Block.t.sol index ce67a669222..6db2cc10fe9 100644 --- a/packages/contracts-bedrock/test/L2/L1Block.t.sol +++ b/packages/contracts-bedrock/test/L2/L1Block.t.sol @@ -34,12 +34,13 @@ contract L1BlockBedrock_Test is L1BlockTest { uint64 s, bytes32 bt, uint256 fo, - uint256 fs + uint256 fs, + bool d ) external { vm.prank(depositor); - l1Block.setL1BlockValues(n, t, b, h, s, bt, fo, fs); + l1Block.setL1BlockValues(n, t, b, h, s, bt, fo, fs, d); assertEq(l1Block.number(), n); assertEq(l1Block.timestamp(), t); assertEq(l1Block.basefee(), b); @@ -48,6 +49,7 @@ contract L1BlockBedrock_Test is L1BlockTest { assertEq(l1Block.batcherHash(), bt); assertEq(l1Block.l1FeeOverhead(), fo); assertEq(l1Block.l1FeeScalar(), fs); + assertEq(l1Block.isDeposit(), d); } /// @dev Tests that `setL1BlockValues` can set max values. @@ -61,7 +63,8 @@ contract L1BlockBedrock_Test is L1BlockTest { _sequenceNumber: type(uint64).max, _batcherHash: bytes32(type(uint256).max), _l1FeeOverhead: type(uint256).max, - _l1FeeScalar: type(uint256).max + _l1FeeScalar: type(uint256).max, + _isDeposit: true }); } @@ -76,7 +79,8 @@ contract L1BlockBedrock_Test is L1BlockTest { _sequenceNumber: type(uint64).max, _batcherHash: bytes32(type(uint256).max), _l1FeeOverhead: type(uint256).max, - _l1FeeScalar: type(uint256).max + _l1FeeScalar: type(uint256).max, + _isDeposit: true }); } } diff --git a/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol b/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol index 3fad9769923..d8ca40ba561 100644 --- a/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol +++ b/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol @@ -32,7 +32,9 @@ contract L1BlockNumberTest is Test { _sequenceNumber: uint64(4), _batcherHash: bytes32(uint256(0)), _l1FeeOverhead: 2, - _l1FeeScalar: 3 + _l1FeeScalar: 3, + // TODO(disco) + _isDeposit: false }); } From 9fd174efdb21e16e4aa2ba8d71f9962f5576a976 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 13:03:08 -0300 Subject: [PATCH 13/23] feat: create some set l1 block values ithmus test * refactor: not cross l2 inbox error --- packages/contracts-bedrock/src/L2/L1Block.sol | 14 +-- .../src/libraries/L1BlockErrors.sol | 3 + .../contracts-bedrock/test/L2/L1Block.t.sol | 101 +++++++++++++++++- 3 files changed, 110 insertions(+), 8 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 06790a6b404..184dd98f019 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -95,7 +95,7 @@ contract L1Block is ISemver, IGasToken { // TODO(disco): natspec function isDeposit() external view returns (bool _isDeposit) { - require(msg.sender == CROSS_L2_INBOX, "L1Block: only the CrossL2Inbox can check if it is a deposit"); + if (msg.sender != CROSS_L2_INBOX) revert NotCrossL2Inbox(); _isDeposit = isDepositTransaction; } @@ -175,6 +175,12 @@ contract L1Block is ISemver, IGasToken { } } + /// @notice Resets the isDeposit flag. + function depositsComplete() external { + if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); + isDepositTransaction = false; + } + /// @notice Sets the gas paying token for the L2 system. Can only be called by the special /// depositor account. This function is not called on every L2 block but instead /// only called by specially crafted L1 deposit transactions. @@ -185,10 +191,4 @@ contract L1Block is ISemver, IGasToken { emit GasPayingTokenSet({ token: _token, decimals: _decimals, name: _name, symbol: _symbol }); } - - /// @notice Resets the isDeposit flag. - function depositsComplete() external { - if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); - isDepositTransaction = false; - } } diff --git a/packages/contracts-bedrock/src/libraries/L1BlockErrors.sol b/packages/contracts-bedrock/src/libraries/L1BlockErrors.sol index c9ef3903aeb..44e156e158c 100644 --- a/packages/contracts-bedrock/src/libraries/L1BlockErrors.sol +++ b/packages/contracts-bedrock/src/libraries/L1BlockErrors.sol @@ -4,6 +4,9 @@ pragma solidity ^0.8.0; /// @notice Error returns when a non-depositor account tries to set L1 block values. error NotDepositor(); +/// @notice Error when a non-cross L2 Inbox sender tries to call the `isDeposit()` method. +error NotCrossL2Inbox(); + /// @notice Error when a chain ID is not in the interop dependency set. error NotDependency(); diff --git a/packages/contracts-bedrock/test/L2/L1Block.t.sol b/packages/contracts-bedrock/test/L2/L1Block.t.sol index 6db2cc10fe9..82979b3e867 100644 --- a/packages/contracts-bedrock/test/L2/L1Block.t.sol +++ b/packages/contracts-bedrock/test/L2/L1Block.t.sol @@ -10,7 +10,8 @@ import { Constants } from "src/libraries/Constants.sol"; // Target contract import { L1Block } from "src/L2/L1Block.sol"; -import "src/libraries/L1BlockErrors.sol"; +import { NotDepositor, NotCrossL2Inbox } from "src/libraries/L1BlockErrors.sol"; +import { Predeploys } from "src/libraries/Predeploys.sol"; contract L1BlockTest is CommonTest { address depositor; @@ -24,6 +25,38 @@ contract L1BlockTest is CommonTest { } } +contract L1BlockIsDeposit_Test is L1BlockTest { + /// @dev Tests that `isDeposit` reverts if the caller is not the cross L2 inbox. + function test_isDeposit_notCrossL2Inbox_reverts(address _caller) external { + vm.assume(_caller != Predeploys.CROSS_L2_INBOX); + vm.expectRevert(NotCrossL2Inbox.selector); + l1Block.isDeposit(); + } + + /// @dev Tests that `isDeposit` always returns the correct value. + function test_isDeposit_succeeds(bool _isDeposit) external { + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), false); + + /// @dev Assuming that `setL1BlockValues` will set the proper value. That function is tested as well + vm.prank(depositor); + l1Block.setL1BlockValues({ + _number: type(uint64).max, + _timestamp: type(uint64).max, + _basefee: type(uint256).max, + _hash: keccak256(abi.encode(1)), + _sequenceNumber: type(uint64).max, + _batcherHash: bytes32(type(uint256).max), + _l1FeeOverhead: type(uint256).max, + _l1FeeScalar: type(uint256).max, + _isDeposit: _isDeposit + }); + + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), _isDeposit); + } +} + contract L1BlockBedrock_Test is L1BlockTest { // @dev Tests that `setL1BlockValues` updates the values correctly. function testFuzz_updatesValues_succeeds( @@ -85,6 +118,33 @@ contract L1BlockBedrock_Test is L1BlockTest { } } +contract L1BlockSetL1BlockValuesIsthmus_Test is L1BlockTest { + /// @dev Tests that `setL1BlockValuesIsthmus` reverts if sender address is not the depositor + function test_setL1BlockValuesIsthmus_notDepositor_reverts(address _caller) external { + vm.assume(_caller != depositor); + vm.prank(_caller); + vm.expectRevert(NotDepositor.selector); + l1Block.setL1BlockValuesIsthmus(); + } + + /// @dev Tests that `setL1BlockValuesIsthmus` succeeds if sender address is the depositor + function test_setL1BlockValuesIsthmus_succeeds() external { + // Ensure the `isDepositTransaction` flag is false before calling `setL1BlockValuesIsthmus` + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), false); + + vm.prank(depositor); + l1Block.setL1BlockValuesIsthmus(); + + // Assert that the `isDepositTransaction` flag was properly set to true + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), true); + + // Assert `setL1BlockValuesEcotone` was properly called, forwarding the calldata to it + // TODO(disco) + } +} + contract L1BlockEcotone_Test is L1BlockTest { /// @dev Tests that setL1BlockValuesEcotone updates the values appropriately. function testFuzz_setL1BlockValuesEcotone_succeeds( @@ -172,6 +232,45 @@ contract L1BlockEcotone_Test is L1BlockTest { } } +contract L1BlockDepositsComplete_Test is L1BlockTest { + // @dev Tests that `depositsComplete` reverts if the caller is not the depositor. + function test_deposits_is_depositor_reverts(address _caller) external { + vm.assume(_caller != depositor); + vm.expectRevert(NotDepositor.selector); + l1Block.depositsComplete(); + } + + // @dev Tests that `depositsComplete` succeeds if the caller is the depositor. + function test_depositsComplete_succeeds() external { + // Set the `isDeposit` flag to true + vm.prank(depositor); + l1Block.setL1BlockValues({ + _number: type(uint64).max, + _timestamp: type(uint64).max, + _basefee: type(uint256).max, + _hash: keccak256(abi.encode(1)), + _sequenceNumber: type(uint64).max, + _batcherHash: bytes32(type(uint256).max), + _l1FeeOverhead: type(uint256).max, + _l1FeeScalar: type(uint256).max, + _isDeposit: true + }); + + // Assert that the `isDeposit` flag was properly set to true + vm.prank(Predeploys.CROSS_L2_INBOX); + assertTrue(l1Block.isDeposit()); + + // Call `depositsComplete` + vm.prank(depositor); + l1Block.depositsComplete(); + + // Assert that the `isDeposit` flag was properly set to false + /// @dev Assuming that `isDeposit()` wil return the proper value. That function is tested as well + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), false); + } +} + contract L1BlockCustomGasToken_Test is L1BlockTest { function testFuzz_setGasPayingToken_succeeds( address _token, From c86657fa632f58033831a02f9916fdb599203d75 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 14:37:06 -0300 Subject: [PATCH 14/23] chore: checkpoint debugging test --- packages/contracts-bedrock/src/L2/L1Block.sol | 32 ++++++-- .../contracts-bedrock/test/L2/L1Block.t.sol | 82 +++++++++++++------ 2 files changed, 82 insertions(+), 32 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 184dd98f019..5285b512f42 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.15; import { ISemver } from "src/universal/ISemver.sol"; import { Constants } from "src/libraries/Constants.sol"; import { GasPayingToken, IGasToken } from "src/libraries/GasPayingToken.sol"; +import { Predeploys } from "src/libraries/Predeploys.sol"; import "src/libraries/L1BlockErrors.sol"; /// @custom:proxied @@ -22,8 +23,6 @@ contract L1Block is ISemver, IGasToken { addr_ = Constants.DEPOSITOR_ACCOUNT; } - address public constant CROSS_L2_INBOX = address(0x4200000000000000000000000000000000000022); - // TODO(disco): check slot /// @notice The isDeposit flag. bool internal isDepositTransaction; @@ -95,7 +94,7 @@ contract L1Block is ISemver, IGasToken { // TODO(disco): natspec function isDeposit() external view returns (bool _isDeposit) { - if (msg.sender != CROSS_L2_INBOX) revert NotCrossL2Inbox(); + if (msg.sender != Predeploys.CROSS_L2_INBOX) revert NotCrossL2Inbox(); _isDeposit = isDepositTransaction; } @@ -139,9 +138,29 @@ contract L1Block is ISemver, IGasToken { // TODO(disco) natspec function setL1BlockValuesIsthmus() external { isDepositTransaction = true; - // TODO(disco): need to check calldata is proxied here, otherwise we might need to do an assembly low-level call - // here - setL1BlockValuesEcotone(); + + // Capture and adjust calldata + bytes memory callData; + assembly { + // Allocate memory for calldata + let size := calldatasize() + callData := mload(0x40) // allocate free memory pointer + mstore(0x40, add(callData, add(size, 0x20))) // update free memory pointer + mstore(callData, sub(size, 4)) // set the size of the data minus the function selector + calldatacopy(add(callData, 0x20), 4, sub(size, 4)) // copy the calldata to the allocated memory, skipping + // the selector + } + + (bool success,) = address(this).call( + abi.encodePacked( + this.setL1BlockValuesEcotone.selector, + callData // Skip the selector of setL1BlockValuesIsthmus + ) + ); + + if (!success) { + revert("L1Block: failed to set L1 block values"); + } } /// @notice Updates the L1 block values for an Ecotone upgraded chain. @@ -175,6 +194,7 @@ contract L1Block is ISemver, IGasToken { } } + // TODO(disco): Natspec /// @notice Resets the isDeposit flag. function depositsComplete() external { if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); diff --git a/packages/contracts-bedrock/test/L2/L1Block.t.sol b/packages/contracts-bedrock/test/L2/L1Block.t.sol index 82979b3e867..1d08e434ce2 100644 --- a/packages/contracts-bedrock/test/L2/L1Block.t.sol +++ b/packages/contracts-bedrock/test/L2/L1Block.t.sol @@ -82,6 +82,8 @@ contract L1BlockBedrock_Test is L1BlockTest { assertEq(l1Block.batcherHash(), bt); assertEq(l1Block.l1FeeOverhead(), fo); assertEq(l1Block.l1FeeScalar(), fs); + + vm.prank(Predeploys.CROSS_L2_INBOX); assertEq(l1Block.isDeposit(), d); } @@ -118,32 +120,60 @@ contract L1BlockBedrock_Test is L1BlockTest { } } -contract L1BlockSetL1BlockValuesIsthmus_Test is L1BlockTest { - /// @dev Tests that `setL1BlockValuesIsthmus` reverts if sender address is not the depositor - function test_setL1BlockValuesIsthmus_notDepositor_reverts(address _caller) external { - vm.assume(_caller != depositor); - vm.prank(_caller); - vm.expectRevert(NotDepositor.selector); - l1Block.setL1BlockValuesIsthmus(); - } - - /// @dev Tests that `setL1BlockValuesIsthmus` succeeds if sender address is the depositor - function test_setL1BlockValuesIsthmus_succeeds() external { - // Ensure the `isDepositTransaction` flag is false before calling `setL1BlockValuesIsthmus` - vm.prank(Predeploys.CROSS_L2_INBOX); - assertEq(l1Block.isDeposit(), false); - - vm.prank(depositor); - l1Block.setL1BlockValuesIsthmus(); - - // Assert that the `isDepositTransaction` flag was properly set to true - vm.prank(Predeploys.CROSS_L2_INBOX); - assertEq(l1Block.isDeposit(), true); - - // Assert `setL1BlockValuesEcotone` was properly called, forwarding the calldata to it - // TODO(disco) - } -} +// contract L1BlockSetL1BlockValuesIsthmus_Test is L1BlockTest { +// /// @dev Tests that `setL1BlockValuesIsthmus` reverts if sender address is not the depositor +// function test_setL1BlockValuesIsthmus_notDepositor_reverts(address _caller) external { +// vm.assume(_caller != depositor); +// vm.prank(_caller); +// vm.expectRevert(NotDepositor.selector); +// l1Block.setL1BlockValuesIsthmus(); +// } + +// /// @dev Tests that `setL1BlockValuesIsthmus` succeeds if sender address is the depositor +// function test_setL1BlockValuesIsthmus_succeeds( +// uint32 baseFeeScalar, +// uint32 blobBaseFeeScalar, +// uint64 sequenceNumber, +// uint64 timestamp, +// uint64 number, +// uint256 baseFee, +// uint256 blobBaseFee, +// bytes32 hash, +// bytes32 batcherHash +// ) +// external +// { +// // Ensure the `isDepositTransaction` flag is false before calling `setL1BlockValuesIsthmus` +// vm.prank(Predeploys.CROSS_L2_INBOX); +// assertEq(l1Block.isDeposit(), false); + +// bytes memory setValuesEcotoneCalldata = Encoding.encodeSetL1BlockValuesEcotone( +// baseFeeScalar, blobBaseFeeScalar, sequenceNumber, timestamp, number, baseFee, blobBaseFee, hash, +// batcherHash +// ); + +// vm.prank(depositor); +// (bool success,) = +// address(l1Block).call(abi.encodePacked(L1Block.setL1BlockValuesIsthmus.selector, +// setValuesEcotoneCalldata)); +// assertTrue(success, "function call failed"); + +// // Assert that the `isDepositTransaction` flag was properly set to true +// // vm.prank(Predeploys.CROSS_L2_INBOX); +// // assertEq(l1Block.isDeposit(), true); + +// // Assert `setL1BlockValuesEcotone` was properly called, forwarding the calldata to it +// assertEq(l1Block.baseFeeScalar(), baseFeeScalar, "1"); +// assertEq(l1Block.blobBaseFeeScalar(), blobBaseFeeScalar, "2"); +// assertEq(l1Block.sequenceNumber(), sequenceNumber, "3"); +// assertEq(l1Block.timestamp(), timestamp, "4"); +// assertEq(l1Block.number(), number, "5"); +// assertEq(l1Block.basefee(), baseFee, "6"); +// assertEq(l1Block.blobBaseFee(), blobBaseFee, "7"); +// assertEq(l1Block.hash(), hash, "8"); +// assertEq(l1Block.batcherHash(), batcherHash, "9"); +// } +// } contract L1BlockEcotone_Test is L1BlockTest { /// @dev Tests that setL1BlockValuesEcotone updates the values appropriately. From 63b9190a3ff5d5e7523ebd6263653524b9ace826 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 16:02:27 -0300 Subject: [PATCH 15/23] refactor: call public function --- packages/contracts-bedrock/src/L2/L1Block.sol | 24 +--- .../contracts-bedrock/test/L2/L1Block.t.sol | 106 +++++++++--------- 2 files changed, 53 insertions(+), 77 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 5285b512f42..25131905cb7 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -138,29 +138,7 @@ contract L1Block is ISemver, IGasToken { // TODO(disco) natspec function setL1BlockValuesIsthmus() external { isDepositTransaction = true; - - // Capture and adjust calldata - bytes memory callData; - assembly { - // Allocate memory for calldata - let size := calldatasize() - callData := mload(0x40) // allocate free memory pointer - mstore(0x40, add(callData, add(size, 0x20))) // update free memory pointer - mstore(callData, sub(size, 4)) // set the size of the data minus the function selector - calldatacopy(add(callData, 0x20), 4, sub(size, 4)) // copy the calldata to the allocated memory, skipping - // the selector - } - - (bool success,) = address(this).call( - abi.encodePacked( - this.setL1BlockValuesEcotone.selector, - callData // Skip the selector of setL1BlockValuesIsthmus - ) - ); - - if (!success) { - revert("L1Block: failed to set L1 block values"); - } + setL1BlockValuesEcotone(); } /// @notice Updates the L1 block values for an Ecotone upgraded chain. diff --git a/packages/contracts-bedrock/test/L2/L1Block.t.sol b/packages/contracts-bedrock/test/L2/L1Block.t.sol index 1d08e434ce2..3c56363f986 100644 --- a/packages/contracts-bedrock/test/L2/L1Block.t.sol +++ b/packages/contracts-bedrock/test/L2/L1Block.t.sol @@ -120,60 +120,58 @@ contract L1BlockBedrock_Test is L1BlockTest { } } -// contract L1BlockSetL1BlockValuesIsthmus_Test is L1BlockTest { -// /// @dev Tests that `setL1BlockValuesIsthmus` reverts if sender address is not the depositor -// function test_setL1BlockValuesIsthmus_notDepositor_reverts(address _caller) external { -// vm.assume(_caller != depositor); -// vm.prank(_caller); -// vm.expectRevert(NotDepositor.selector); -// l1Block.setL1BlockValuesIsthmus(); -// } - -// /// @dev Tests that `setL1BlockValuesIsthmus` succeeds if sender address is the depositor -// function test_setL1BlockValuesIsthmus_succeeds( -// uint32 baseFeeScalar, -// uint32 blobBaseFeeScalar, -// uint64 sequenceNumber, -// uint64 timestamp, -// uint64 number, -// uint256 baseFee, -// uint256 blobBaseFee, -// bytes32 hash, -// bytes32 batcherHash -// ) -// external -// { -// // Ensure the `isDepositTransaction` flag is false before calling `setL1BlockValuesIsthmus` -// vm.prank(Predeploys.CROSS_L2_INBOX); -// assertEq(l1Block.isDeposit(), false); - -// bytes memory setValuesEcotoneCalldata = Encoding.encodeSetL1BlockValuesEcotone( -// baseFeeScalar, blobBaseFeeScalar, sequenceNumber, timestamp, number, baseFee, blobBaseFee, hash, -// batcherHash -// ); - -// vm.prank(depositor); -// (bool success,) = -// address(l1Block).call(abi.encodePacked(L1Block.setL1BlockValuesIsthmus.selector, -// setValuesEcotoneCalldata)); -// assertTrue(success, "function call failed"); - -// // Assert that the `isDepositTransaction` flag was properly set to true -// // vm.prank(Predeploys.CROSS_L2_INBOX); -// // assertEq(l1Block.isDeposit(), true); - -// // Assert `setL1BlockValuesEcotone` was properly called, forwarding the calldata to it -// assertEq(l1Block.baseFeeScalar(), baseFeeScalar, "1"); -// assertEq(l1Block.blobBaseFeeScalar(), blobBaseFeeScalar, "2"); -// assertEq(l1Block.sequenceNumber(), sequenceNumber, "3"); -// assertEq(l1Block.timestamp(), timestamp, "4"); -// assertEq(l1Block.number(), number, "5"); -// assertEq(l1Block.basefee(), baseFee, "6"); -// assertEq(l1Block.blobBaseFee(), blobBaseFee, "7"); -// assertEq(l1Block.hash(), hash, "8"); -// assertEq(l1Block.batcherHash(), batcherHash, "9"); -// } -// } +contract L1BlockSetL1BlockValuesIsthmus_Test is L1BlockTest { + /// @dev Tests that `setL1BlockValuesIsthmus` reverts if sender address is not the depositor + function test_setL1BlockValuesIsthmus_notDepositor_reverts(address _caller) external { + vm.assume(_caller != depositor); + vm.prank(_caller); + vm.expectRevert(NotDepositor.selector); + l1Block.setL1BlockValuesIsthmus(); + } + + /// @dev Tests that `setL1BlockValuesIsthmus` succeeds if sender address is the depositor + function test_setL1BlockValuesIsthmus_succeeds( + uint32 baseFeeScalar, + uint32 blobBaseFeeScalar, + uint64 sequenceNumber, + uint64 timestamp, + uint64 number, + uint256 baseFee, + uint256 blobBaseFee, + bytes32 hash, + bytes32 batcherHash + ) + external + { + // Ensure the `isDepositTransaction` flag is false before calling `setL1BlockValuesIsthmus` + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), false); + + bytes memory setValuesEcotoneCalldata = abi.encodePacked( + baseFeeScalar, blobBaseFeeScalar, sequenceNumber, timestamp, number, baseFee, blobBaseFee, hash, batcherHash + ); + + vm.prank(depositor); + (bool success,) = + address(l1Block).call(abi.encodePacked(L1Block.setL1BlockValuesIsthmus.selector, setValuesEcotoneCalldata)); + assertTrue(success, "function call failed"); + + // Assert that the `isDepositTransaction` flag was properly set to true + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), true); + + // Assert `setL1BlockValuesEcotone` was properly called, forwarding the calldata to it + assertEq(l1Block.baseFeeScalar(), baseFeeScalar, "1"); + assertEq(l1Block.blobBaseFeeScalar(), blobBaseFeeScalar, "2"); + assertEq(l1Block.sequenceNumber(), sequenceNumber, "3"); + assertEq(l1Block.timestamp(), timestamp, "4"); + assertEq(l1Block.number(), number, "5"); + assertEq(l1Block.basefee(), baseFee, "6"); + assertEq(l1Block.blobBaseFee(), blobBaseFee, "7"); + assertEq(l1Block.hash(), hash, "8"); + assertEq(l1Block.batcherHash(), batcherHash, "9"); + } +} contract L1BlockEcotone_Test is L1BlockTest { /// @dev Tests that setL1BlockValuesEcotone updates the values appropriately. From 43fb36e0c9f8be2849ed9e729e070b91461b5bf2 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:05:35 -0300 Subject: [PATCH 16/23] fix: update is deposit var storage slot --- packages/contracts-bedrock/src/L2/L1Block.sol | 13 +++--- .../test/L2/GasPriceOracle.t.sol | 3 +- .../contracts-bedrock/test/L2/L1Block.t.sol | 43 +++++-------------- .../test/legacy/L1BlockNumber.t.sol | 4 +- 4 files changed, 17 insertions(+), 46 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 25131905cb7..eb3781795ef 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -23,10 +23,6 @@ contract L1Block is ISemver, IGasToken { addr_ = Constants.DEPOSITOR_ACCOUNT; } - // TODO(disco): check slot - /// @notice The isDeposit flag. - bool internal isDepositTransaction; - /// @notice The latest L1 block number known by the L2 system. uint64 public number; @@ -62,6 +58,10 @@ contract L1Block is ISemver, IGasToken { /// @notice The latest L1 blob base fee. uint256 public blobBaseFee; + // TODO(disco): update proxy slot + /// @notice The isDeposit flag. + bool private isDepositTransaction; + /// @custom:semver 1.4.1-beta.1 function version() public pure virtual returns (string memory) { return "1.4.1-beta.1"; @@ -108,7 +108,6 @@ contract L1Block is ISemver, IGasToken { /// @param _batcherHash Versioned hash to authenticate batcher by. /// @param _l1FeeOverhead L1 fee overhead. /// @param _l1FeeScalar L1 fee scalar. - /// @param _isDeposit isDeposit flag function setL1BlockValues( uint64 _number, uint64 _timestamp, @@ -117,8 +116,7 @@ contract L1Block is ISemver, IGasToken { uint64 _sequenceNumber, bytes32 _batcherHash, uint256 _l1FeeOverhead, - uint256 _l1FeeScalar, - bool _isDeposit + uint256 _l1FeeScalar ) external { @@ -132,7 +130,6 @@ contract L1Block is ISemver, IGasToken { batcherHash = _batcherHash; l1FeeOverhead = _l1FeeOverhead; l1FeeScalar = _l1FeeScalar; - isDepositTransaction = _isDeposit; } // TODO(disco) natspec diff --git a/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol b/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol index f3abe50450f..7a7b6debd53 100644 --- a/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol +++ b/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol @@ -54,8 +54,7 @@ contract GasPriceOracleBedrock_Test is GasPriceOracle_Test { _sequenceNumber: sequenceNumber, _batcherHash: batcherHash, _l1FeeOverhead: l1FeeOverhead, - _l1FeeScalar: l1FeeScalar, - _isDeposit: isDeposit + _l1FeeScalar: l1FeeScalar }); } diff --git a/packages/contracts-bedrock/test/L2/L1Block.t.sol b/packages/contracts-bedrock/test/L2/L1Block.t.sol index 3c56363f986..32a3c60a8bb 100644 --- a/packages/contracts-bedrock/test/L2/L1Block.t.sol +++ b/packages/contracts-bedrock/test/L2/L1Block.t.sol @@ -34,26 +34,18 @@ contract L1BlockIsDeposit_Test is L1BlockTest { } /// @dev Tests that `isDeposit` always returns the correct value. - function test_isDeposit_succeeds(bool _isDeposit) external { + function test_isDeposit_succeeds() external { + // Assert is false if the value is not updated vm.prank(Predeploys.CROSS_L2_INBOX); assertEq(l1Block.isDeposit(), false); - /// @dev Assuming that `setL1BlockValues` will set the proper value. That function is tested as well + /// @dev Assuming that `setL1BlockValuesIsthmus` will set the proper value. That function is tested as well vm.prank(depositor); - l1Block.setL1BlockValues({ - _number: type(uint64).max, - _timestamp: type(uint64).max, - _basefee: type(uint256).max, - _hash: keccak256(abi.encode(1)), - _sequenceNumber: type(uint64).max, - _batcherHash: bytes32(type(uint256).max), - _l1FeeOverhead: type(uint256).max, - _l1FeeScalar: type(uint256).max, - _isDeposit: _isDeposit - }); + l1Block.setL1BlockValuesIsthmus(); + // Assert is true if the value is updated vm.prank(Predeploys.CROSS_L2_INBOX); - assertEq(l1Block.isDeposit(), _isDeposit); + assertEq(l1Block.isDeposit(), true); } } @@ -73,7 +65,7 @@ contract L1BlockBedrock_Test is L1BlockTest { external { vm.prank(depositor); - l1Block.setL1BlockValues(n, t, b, h, s, bt, fo, fs, d); + l1Block.setL1BlockValues(n, t, b, h, s, bt, fo, fs); assertEq(l1Block.number(), n); assertEq(l1Block.timestamp(), t); assertEq(l1Block.basefee(), b); @@ -82,9 +74,6 @@ contract L1BlockBedrock_Test is L1BlockTest { assertEq(l1Block.batcherHash(), bt); assertEq(l1Block.l1FeeOverhead(), fo); assertEq(l1Block.l1FeeScalar(), fs); - - vm.prank(Predeploys.CROSS_L2_INBOX); - assertEq(l1Block.isDeposit(), d); } /// @dev Tests that `setL1BlockValues` can set max values. @@ -98,8 +87,7 @@ contract L1BlockBedrock_Test is L1BlockTest { _sequenceNumber: type(uint64).max, _batcherHash: bytes32(type(uint256).max), _l1FeeOverhead: type(uint256).max, - _l1FeeScalar: type(uint256).max, - _isDeposit: true + _l1FeeScalar: type(uint256).max }); } @@ -114,8 +102,7 @@ contract L1BlockBedrock_Test is L1BlockTest { _sequenceNumber: type(uint64).max, _batcherHash: bytes32(type(uint256).max), _l1FeeOverhead: type(uint256).max, - _l1FeeScalar: type(uint256).max, - _isDeposit: true + _l1FeeScalar: type(uint256).max }); } } @@ -272,17 +259,7 @@ contract L1BlockDepositsComplete_Test is L1BlockTest { function test_depositsComplete_succeeds() external { // Set the `isDeposit` flag to true vm.prank(depositor); - l1Block.setL1BlockValues({ - _number: type(uint64).max, - _timestamp: type(uint64).max, - _basefee: type(uint256).max, - _hash: keccak256(abi.encode(1)), - _sequenceNumber: type(uint64).max, - _batcherHash: bytes32(type(uint256).max), - _l1FeeOverhead: type(uint256).max, - _l1FeeScalar: type(uint256).max, - _isDeposit: true - }); + l1Block.setL1BlockValuesIsthmus(); // Assert that the `isDeposit` flag was properly set to true vm.prank(Predeploys.CROSS_L2_INBOX); diff --git a/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol b/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol index d8ca40ba561..3fad9769923 100644 --- a/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol +++ b/packages/contracts-bedrock/test/legacy/L1BlockNumber.t.sol @@ -32,9 +32,7 @@ contract L1BlockNumberTest is Test { _sequenceNumber: uint64(4), _batcherHash: bytes32(uint256(0)), _l1FeeOverhead: 2, - _l1FeeScalar: 3, - // TODO(disco) - _isDeposit: false + _l1FeeScalar: 3 }); } From dea7432e00d06928d3298439da7d6fc6814904b7 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:15:15 -0300 Subject: [PATCH 17/23] chore: update l2 cross inbox tests with the new is deposit check * feat: add is deposit test --- .../test/L2/CrossL2Inbox.t.sol | 71 ++++++++++++++++++- .../test/L2/GasPriceOracle.t.sol | 2 - 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/packages/contracts-bedrock/test/L2/CrossL2Inbox.t.sol b/packages/contracts-bedrock/test/L2/CrossL2Inbox.t.sol index 9350e6b2fbb..e01d5635b3c 100644 --- a/packages/contracts-bedrock/test/L2/CrossL2Inbox.t.sol +++ b/packages/contracts-bedrock/test/L2/CrossL2Inbox.t.sol @@ -9,7 +9,15 @@ import { Predeploys } from "src/libraries/Predeploys.sol"; import { TransientContext } from "src/libraries/TransientContext.sol"; // Target contracts -import { CrossL2Inbox, NotEntered, InvalidTimestamp, InvalidChainId, TargetCallFailed } from "src/L2/CrossL2Inbox.sol"; +import { + CrossL2Inbox, + IL1Block, + NotEntered, + NoExecutingDeposits, + InvalidTimestamp, + InvalidChainId, + TargetCallFailed +} from "src/L2/CrossL2Inbox.sol"; import { ICrossL2Inbox } from "src/L2/ICrossL2Inbox.sol"; /// @title CrossL2InboxWithModifiableTransientStorage @@ -84,6 +92,13 @@ contract CrossL2InboxTest is Test { // Ensure that the target call is payable if value is sent if (_value > 0) assumePayable(_target); + // Ensure is not a deposit transaction + vm.mockCall({ + callee: Predeploys.L1_BLOCK_ATTRIBUTES, + data: abi.encodeWithSelector(IL1Block.isDeposit.selector), + returnData: abi.encode(false) + }); + // Ensure that the target call does not revert vm.mockCall({ callee: _target, msgValue: _value, data: _message, returnData: abi.encode(true) }); @@ -137,6 +152,13 @@ contract CrossL2InboxTest is Test { _id1.timestamp = bound(_id1.timestamp, 0, block.timestamp); _id2.timestamp = bound(_id2.timestamp, 0, block.timestamp); + // Ensure is not a deposit transaction + vm.mockCall({ + callee: Predeploys.L1_BLOCK_ATTRIBUTES, + data: abi.encodeWithSelector(IL1Block.isDeposit.selector), + returnData: abi.encode(false) + }); + // Ensure that id1's chain ID is in the dependency set vm.mockCall({ callee: Predeploys.L1_BLOCK_ATTRIBUTES, @@ -181,6 +203,32 @@ contract CrossL2InboxTest is Test { assertEq(crossL2Inbox.chainId(), _id2.chainId); } + /// @dev Tests that the `executeMessage` function reverts if the transaction comes from a deposit. + function testFuzz_executeMessage_isDeposit_reverts( + ICrossL2Inbox.Identifier calldata _id, + address _target, + bytes calldata _message, + uint256 _value + ) + external + { + // Ensure it is a deposit transaction + vm.mockCall({ + callee: Predeploys.L1_BLOCK_ATTRIBUTES, + data: abi.encodeWithSelector(IL1Block.isDeposit.selector), + returnData: abi.encode(true) + }); + + // Ensure that the contract has enough balance to send with value + vm.deal(address(this), _value); + + // Expect a revert with the NoExecutingDeposits selector + vm.expectRevert(NoExecutingDeposits.selector); + + // Call the executeMessage function + crossL2Inbox.executeMessage{ value: _value }({ _id: _id, _target: _target, _message: _message }); + } + /// @dev Tests that the `executeMessage` function reverts when called with an identifier with an invalid timestamp. function testFuzz_executeMessage_invalidTimestamp_reverts( ICrossL2Inbox.Identifier calldata _id, @@ -193,6 +241,13 @@ contract CrossL2InboxTest is Test { // Ensure that the id's timestamp is invalid (greater than the current block timestamp) vm.assume(_id.timestamp > block.timestamp); + // Ensure is not a deposit transaction + vm.mockCall({ + callee: Predeploys.L1_BLOCK_ATTRIBUTES, + data: abi.encodeWithSelector(IL1Block.isDeposit.selector), + returnData: abi.encode(false) + }); + // Ensure that the contract has enough balance to send with value vm.deal(address(this), _value); @@ -216,6 +271,13 @@ contract CrossL2InboxTest is Test { // Ensure that the id's timestamp is valid (less than or equal to the current block timestamp) _id.timestamp = bound(_id.timestamp, 0, block.timestamp); + // Ensure is not a deposit transaction + vm.mockCall({ + callee: Predeploys.L1_BLOCK_ATTRIBUTES, + data: abi.encodeWithSelector(IL1Block.isDeposit.selector), + returnData: abi.encode(false) + }); + // Ensure that the chain ID is NOT in the dependency set vm.mockCall({ callee: Predeploys.L1_BLOCK_ATTRIBUTES, @@ -251,6 +313,13 @@ contract CrossL2InboxTest is Test { // Ensure that the target call reverts vm.mockCallRevert({ callee: _target, msgValue: _value, data: _message, revertData: abi.encode(false) }); + // Ensure is not a deposit transaction + vm.mockCall({ + callee: Predeploys.L1_BLOCK_ATTRIBUTES, + data: abi.encodeWithSelector(IL1Block.isDeposit.selector), + returnData: abi.encode(false) + }); + // Ensure that the chain ID is in the dependency set vm.mockCall({ callee: Predeploys.L1_BLOCK_ATTRIBUTES, diff --git a/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol b/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol index 7a7b6debd53..21f7b98f666 100644 --- a/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol +++ b/packages/contracts-bedrock/test/L2/GasPriceOracle.t.sol @@ -27,8 +27,6 @@ contract GasPriceOracle_Test is CommonTest { uint256 constant l1FeeScalar = 10; uint32 constant blobBaseFeeScalar = 15; uint32 constant baseFeeScalar = 20; - // TODO(disco): check - bool constant isDeposit = true; /// @dev Sets up the test suite. function setUp() public virtual override { From 840de88d56386baac984667defb857109ef21255 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:23:32 -0300 Subject: [PATCH 18/23] refactor: set is deposit as an unstructured storage slot var --- packages/contracts-bedrock/src/L2/L1Block.sol | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index eb3781795ef..1a1520d3a29 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -58,9 +58,10 @@ contract L1Block is ISemver, IGasToken { /// @notice The latest L1 blob base fee. uint256 public blobBaseFee; - // TODO(disco): update proxy slot - /// @notice The isDeposit flag. - bool private isDepositTransaction; + /// @notice Storage slot that the isDeposit is stored at. + /// @dev This is a custom slot that is not part of the standard storage layout. + /// keccak256(abi.encode(uint256(keccak256("l1Block.identifier.isDeposit")) - 1)) & ~bytes32(uint256(0xff)) + uint256 internal constant IS_DEPOSIT_SLOT = 0x921bd3a089295c6e5540e8fba8195448d253efd6f2e3e495b499b627dc36a300; /// @custom:semver 1.4.1-beta.1 function version() public pure virtual returns (string memory) { @@ -95,7 +96,9 @@ contract L1Block is ISemver, IGasToken { // TODO(disco): natspec function isDeposit() external view returns (bool _isDeposit) { if (msg.sender != Predeploys.CROSS_L2_INBOX) revert NotCrossL2Inbox(); - _isDeposit = isDepositTransaction; + assembly { + _isDeposit := sload(IS_DEPOSIT_SLOT) + } } /// @custom:legacy @@ -134,7 +137,11 @@ contract L1Block is ISemver, IGasToken { // TODO(disco) natspec function setL1BlockValuesIsthmus() external { - isDepositTransaction = true; + // Set the isDeposit flag to true. + assembly { + sstore(IS_DEPOSIT_SLOT, 1) + } + setL1BlockValuesEcotone(); } @@ -173,7 +180,11 @@ contract L1Block is ISemver, IGasToken { /// @notice Resets the isDeposit flag. function depositsComplete() external { if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); - isDepositTransaction = false; + + // Set the isDeposit flag to false. + assembly { + sstore(IS_DEPOSIT_SLOT, 0) + } } /// @notice Sets the gas paying token for the L2 system. Can only be called by the special From e758d9a634aa8251192b3475d56596e26477b99d Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:40:50 -0300 Subject: [PATCH 19/23] feat: add missing natspec --- packages/contracts-bedrock/src/L2/CrossL2Inbox.sol | 5 ++++- packages/contracts-bedrock/src/L2/L1Block.sol | 13 ++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol b/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol index ba8d941e49f..bf7e21888d0 100644 --- a/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol +++ b/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol @@ -17,8 +17,11 @@ interface IDependencySet { function isInDependencySet(uint256 _chainId) external view returns (bool); } -// TODO(disco): +/// @title IL1Block +/// @notice Interface for L1Block with only `isDeposit()` method. interface IL1Block { + /// @notice Returns whether the call was triggered from a a deposit or not. + /// @return True if the current call was triggered by a deposit transaction, and false otherwise. function isDeposit() external view returns (bool); } diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 1a1520d3a29..8b6522e32af 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -93,11 +93,12 @@ contract L1Block is ISemver, IGasToken { return token != Constants.ETHER; } - // TODO(disco): natspec - function isDeposit() external view returns (bool _isDeposit) { + /// @notice Returns whether the call was triggered from a a deposit or not. + /// @dev This function is only callable by the CrossL2Inbox contract. + function isDeposit() external view returns (bool isDeposit_) { if (msg.sender != Predeploys.CROSS_L2_INBOX) revert NotCrossL2Inbox(); assembly { - _isDeposit := sload(IS_DEPOSIT_SLOT) + isDeposit_ := sload(IS_DEPOSIT_SLOT) } } @@ -135,7 +136,8 @@ contract L1Block is ISemver, IGasToken { l1FeeScalar = _l1FeeScalar; } - // TODO(disco) natspec + /// @notice Updates the `isDeposit` flag and sets the L1 block values for an Isthmus upgraded chain. + /// @dev It updates the L1 block values through the `setL1BlockValuesEcotone` function. function setL1BlockValuesIsthmus() external { // Set the isDeposit flag to true. assembly { @@ -176,8 +178,9 @@ contract L1Block is ISemver, IGasToken { } } - // TODO(disco): Natspec /// @notice Resets the isDeposit flag. + /// @dev Only callable by the depositor account. + /// @dev Should be called after the deposits are complete. function depositsComplete() external { if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); From 49a02ff05bc7d20577d3d9fc73241facc592d3e0 Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:56:44 -0300 Subject: [PATCH 20/23] feat: update semver * fix: stick to natspec standards --- .../contracts-bedrock/src/L2/CrossL2Inbox.sol | 4 +- packages/contracts-bedrock/src/L2/L1Block.sol | 13 +++-- .../contracts-bedrock/test/L2/L1Block.t.sol | 48 +++++++++---------- 3 files changed, 32 insertions(+), 33 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol b/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol index bf7e21888d0..4a0dda1e933 100644 --- a/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol +++ b/packages/contracts-bedrock/src/L2/CrossL2Inbox.sol @@ -67,8 +67,8 @@ contract CrossL2Inbox is ICrossL2Inbox, ISemver, TransientReentrancyAware { bytes32 internal constant CHAINID_SLOT = 0x6e0446e8b5098b8c8193f964f1b567ec3a2bdaeba33d36acb85c1f1d3f92d313; /// @notice Semantic version. - /// @custom:semver 1.0.0-beta.3 - string public constant version = "1.0.0-beta.3"; + /// @custom:semver 1.1.0-beta.3 + string public constant version = "1.1.0-beta.3"; /// @notice Emitted when a cross chain message is being executed. /// @param msgHash Hash of message payload being executed. diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 8b6522e32af..4188f3389a9 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -59,13 +59,13 @@ contract L1Block is ISemver, IGasToken { uint256 public blobBaseFee; /// @notice Storage slot that the isDeposit is stored at. - /// @dev This is a custom slot that is not part of the standard storage layout. + /// This is a custom slot that is not part of the standard storage layout. /// keccak256(abi.encode(uint256(keccak256("l1Block.identifier.isDeposit")) - 1)) & ~bytes32(uint256(0xff)) uint256 internal constant IS_DEPOSIT_SLOT = 0x921bd3a089295c6e5540e8fba8195448d253efd6f2e3e495b499b627dc36a300; - /// @custom:semver 1.4.1-beta.1 + /// @custom:semver 1.5.1-beta.1 function version() public pure virtual returns (string memory) { - return "1.4.1-beta.1"; + return "1.5.1-beta.1"; } /// @notice Returns the gas paying token, its decimals, name and symbol. @@ -94,7 +94,7 @@ contract L1Block is ISemver, IGasToken { } /// @notice Returns whether the call was triggered from a a deposit or not. - /// @dev This function is only callable by the CrossL2Inbox contract. + /// @notice This function is only callable by the CrossL2Inbox contract. function isDeposit() external view returns (bool isDeposit_) { if (msg.sender != Predeploys.CROSS_L2_INBOX) revert NotCrossL2Inbox(); assembly { @@ -137,7 +137,7 @@ contract L1Block is ISemver, IGasToken { } /// @notice Updates the `isDeposit` flag and sets the L1 block values for an Isthmus upgraded chain. - /// @dev It updates the L1 block values through the `setL1BlockValuesEcotone` function. + /// It updates the L1 block values through the `setL1BlockValuesEcotone` function. function setL1BlockValuesIsthmus() external { // Set the isDeposit flag to true. assembly { @@ -179,8 +179,7 @@ contract L1Block is ISemver, IGasToken { } /// @notice Resets the isDeposit flag. - /// @dev Only callable by the depositor account. - /// @dev Should be called after the deposits are complete. + /// Should only be called by the depositor account after the deposits are complete. function depositsComplete() external { if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); diff --git a/packages/contracts-bedrock/test/L2/L1Block.t.sol b/packages/contracts-bedrock/test/L2/L1Block.t.sol index 32a3c60a8bb..4c0af9127f0 100644 --- a/packages/contracts-bedrock/test/L2/L1Block.t.sol +++ b/packages/contracts-bedrock/test/L2/L1Block.t.sol @@ -25,30 +25,6 @@ contract L1BlockTest is CommonTest { } } -contract L1BlockIsDeposit_Test is L1BlockTest { - /// @dev Tests that `isDeposit` reverts if the caller is not the cross L2 inbox. - function test_isDeposit_notCrossL2Inbox_reverts(address _caller) external { - vm.assume(_caller != Predeploys.CROSS_L2_INBOX); - vm.expectRevert(NotCrossL2Inbox.selector); - l1Block.isDeposit(); - } - - /// @dev Tests that `isDeposit` always returns the correct value. - function test_isDeposit_succeeds() external { - // Assert is false if the value is not updated - vm.prank(Predeploys.CROSS_L2_INBOX); - assertEq(l1Block.isDeposit(), false); - - /// @dev Assuming that `setL1BlockValuesIsthmus` will set the proper value. That function is tested as well - vm.prank(depositor); - l1Block.setL1BlockValuesIsthmus(); - - // Assert is true if the value is updated - vm.prank(Predeploys.CROSS_L2_INBOX); - assertEq(l1Block.isDeposit(), true); - } -} - contract L1BlockBedrock_Test is L1BlockTest { // @dev Tests that `setL1BlockValues` updates the values correctly. function testFuzz_updatesValues_succeeds( @@ -313,3 +289,27 @@ contract L1BlockCustomGasToken_Test is L1BlockTest { l1Block.setGasPayingToken(address(this), 18, "Test", "TST"); } } + +contract L1BlockIsDeposit_Test is L1BlockTest { + /// @dev Tests that `isDeposit` reverts if the caller is not the cross L2 inbox. + function test_isDeposit_notCrossL2Inbox_reverts(address _caller) external { + vm.assume(_caller != Predeploys.CROSS_L2_INBOX); + vm.expectRevert(NotCrossL2Inbox.selector); + l1Block.isDeposit(); + } + + /// @dev Tests that `isDeposit` always returns the correct value. + function test_isDeposit_succeeds() external { + // Assert is false if the value is not updated + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), false); + + /// @dev Assuming that `setL1BlockValuesIsthmus` will set the proper value. That function is tested as well + vm.prank(depositor); + l1Block.setL1BlockValuesIsthmus(); + + // Assert is true if the value is updated + vm.prank(Predeploys.CROSS_L2_INBOX); + assertEq(l1Block.isDeposit(), true); + } +} From eafba7a96c9da08fd82101872a6e26e4e1fbd29c Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 18:34:13 -0300 Subject: [PATCH 21/23] chore: enhance natspec --- packages/contracts-bedrock/src/L2/L1Block.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 4188f3389a9..3fbbf2b0669 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -179,7 +179,7 @@ contract L1Block is ISemver, IGasToken { } /// @notice Resets the isDeposit flag. - /// Should only be called by the depositor account after the deposits are complete. + /// Should only be called by the depositor account after the deposits are complete. function depositsComplete() external { if (msg.sender != DEPOSITOR_ACCOUNT()) revert NotDepositor(); From e162361d6fffae60ad19cfbf8fda673b09da42fc Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 20:52:28 -0300 Subject: [PATCH 22/23] chore: underscore slot constant name --- packages/contracts-bedrock/src/L2/L1Block.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index 3fbbf2b0669..c70e0ac0616 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -61,7 +61,7 @@ contract L1Block is ISemver, IGasToken { /// @notice Storage slot that the isDeposit is stored at. /// This is a custom slot that is not part of the standard storage layout. /// keccak256(abi.encode(uint256(keccak256("l1Block.identifier.isDeposit")) - 1)) & ~bytes32(uint256(0xff)) - uint256 internal constant IS_DEPOSIT_SLOT = 0x921bd3a089295c6e5540e8fba8195448d253efd6f2e3e495b499b627dc36a300; + uint256 internal constant _IS_DEPOSIT_SLOT = 0x921bd3a089295c6e5540e8fba8195448d253efd6f2e3e495b499b627dc36a300; /// @custom:semver 1.5.1-beta.1 function version() public pure virtual returns (string memory) { @@ -98,7 +98,7 @@ contract L1Block is ISemver, IGasToken { function isDeposit() external view returns (bool isDeposit_) { if (msg.sender != Predeploys.CROSS_L2_INBOX) revert NotCrossL2Inbox(); assembly { - isDeposit_ := sload(IS_DEPOSIT_SLOT) + isDeposit_ := sload(_IS_DEPOSIT_SLOT) } } @@ -141,7 +141,7 @@ contract L1Block is ISemver, IGasToken { function setL1BlockValuesIsthmus() external { // Set the isDeposit flag to true. assembly { - sstore(IS_DEPOSIT_SLOT, 1) + sstore(_IS_DEPOSIT_SLOT, 1) } setL1BlockValuesEcotone(); @@ -185,7 +185,7 @@ contract L1Block is ISemver, IGasToken { // Set the isDeposit flag to false. assembly { - sstore(IS_DEPOSIT_SLOT, 0) + sstore(_IS_DEPOSIT_SLOT, 0) } } From fb0f259a5b7346a03975df38c6cb5935544ebb2d Mon Sep 17 00:00:00 2001 From: 0xDiscotech <131301107+0xDiscotech@users.noreply.github.com> Date: Wed, 7 Aug 2024 20:57:52 -0300 Subject: [PATCH 23/23] Revert "chore: underscore slot constant name" This reverts commit e162361d6fffae60ad19cfbf8fda673b09da42fc. --- packages/contracts-bedrock/src/L2/L1Block.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/contracts-bedrock/src/L2/L1Block.sol b/packages/contracts-bedrock/src/L2/L1Block.sol index c70e0ac0616..3fbbf2b0669 100644 --- a/packages/contracts-bedrock/src/L2/L1Block.sol +++ b/packages/contracts-bedrock/src/L2/L1Block.sol @@ -61,7 +61,7 @@ contract L1Block is ISemver, IGasToken { /// @notice Storage slot that the isDeposit is stored at. /// This is a custom slot that is not part of the standard storage layout. /// keccak256(abi.encode(uint256(keccak256("l1Block.identifier.isDeposit")) - 1)) & ~bytes32(uint256(0xff)) - uint256 internal constant _IS_DEPOSIT_SLOT = 0x921bd3a089295c6e5540e8fba8195448d253efd6f2e3e495b499b627dc36a300; + uint256 internal constant IS_DEPOSIT_SLOT = 0x921bd3a089295c6e5540e8fba8195448d253efd6f2e3e495b499b627dc36a300; /// @custom:semver 1.5.1-beta.1 function version() public pure virtual returns (string memory) { @@ -98,7 +98,7 @@ contract L1Block is ISemver, IGasToken { function isDeposit() external view returns (bool isDeposit_) { if (msg.sender != Predeploys.CROSS_L2_INBOX) revert NotCrossL2Inbox(); assembly { - isDeposit_ := sload(_IS_DEPOSIT_SLOT) + isDeposit_ := sload(IS_DEPOSIT_SLOT) } } @@ -141,7 +141,7 @@ contract L1Block is ISemver, IGasToken { function setL1BlockValuesIsthmus() external { // Set the isDeposit flag to true. assembly { - sstore(_IS_DEPOSIT_SLOT, 1) + sstore(IS_DEPOSIT_SLOT, 1) } setL1BlockValuesEcotone(); @@ -185,7 +185,7 @@ contract L1Block is ISemver, IGasToken { // Set the isDeposit flag to false. assembly { - sstore(_IS_DEPOSIT_SLOT, 0) + sstore(IS_DEPOSIT_SLOT, 0) } }