EIP4844 smart contract updated #8712

anikaraghu · 2023-12-20T22:16:22Z

Description
Fully backwards compatible smart contract changes to support the updated EIP4844 pricing function and new parameters

Tests

foundry tests added

Additional context

Add any other context about the problem you're solving.

Metadata

Fixes #[Link to Issue]

roberto-bayardo · 2023-12-20T22:21:31Z

packages/contracts-bedrock/src/L2/GasPriceOracle.sol

+    /// @custom:semver 1.2.0
+    string public constant version = "1.2.0";
+
+    /// @notice Flag that indicates whether the network is in ecotone mode.


Suggested change

/// @notice Flag that indicates whether the network is in ecotone mode.

/// @notice indicates whether the network has gone through the Ecotone upgrade

codecov · 2023-12-20T22:23:52Z

Codecov Report

Attention: 66 lines in your changes are missing coverage. Please review.

Comparison is base (79e62bf) 34.57% compared to head (fc71f2b) 34.25%.
Report is 15 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #8712      +/-   ##
===========================================
- Coverage    34.57%   34.25%   -0.33%     
===========================================
  Files          167      167              
  Lines         7170     7229      +59     
  Branches      1213     1219       +6     
===========================================
- Hits          2479     2476       -3     
- Misses        4539     4603      +64     
+ Partials       152      150       -2

Flag	Coverage Δ
cannon-go-tests	`63.48% <ø> (ø)`
chain-mon-tests	`27.14% <ø> (ø)`
common-ts-tests	`26.74% <ø> (ø)`
contracts-bedrock-tests	`19.55% <0.00%> (-0.69%)`	⬇️
contracts-ts-tests	`12.25% <ø> (ø)`
core-utils-tests	`44.03% <ø> (ø)`
sdk-next-tests	`42.18% <ø> (ø)`
sdk-tests	`42.18% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ckages/contracts-bedrock/src/L2/GasPriceOracle.sol	`0.00% <0.00%> (ø)`
packages/contracts-bedrock/src/L2/L1Block.sol	`17.39% <0.00%> (-71.50%)`	⬇️

... and 3 files with indirect coverage changes

packages/contracts-bedrock/src/L2/GasPriceOracle.sol

roberto-bayardo · 2023-12-20T22:27:23Z

packages/contracts-bedrock/src/L2/GasPriceOracle.sol

+    /// @notice Post-ecotone L1 gas estimation calculation.
+    /// @param _data Unsigned fully RLP-encoded transaction to get the L1 gas for.
+    /// @return Amount of L1 gas used to publish the transaction.
+    function _getL1GasUsedEcotone(bytes memory _data) internal pure returns (uint256) {


this isn't computing a "gas" value, but the approximation of "compressed bytes", so I'd rename it something more accurate to make it better match the spec.

_getCompressedTxSize() ?

from Slack discussion: just delete this function after applying the suggested edit above to the "main" function.

tynes · 2023-12-20T23:01:35Z

packages/contracts-bedrock/src/L2/L1Block.sol

+        bytes32 _batcherHash;
+
+        assembly {
+            let offset := 0x4


I think having an offset for first pass to get passing tests is fine but ideally we don't need to have the intermediate adds and we just hardcode in the values. We should also be able to sstore directly in the yul but I am fine with that as a nice to have

tynes · 2023-12-20T23:03:44Z

packages/contracts-bedrock/src/L2/L1Block.sol

+
+        assembly {
+            let offset := 0x4
+            _basefeeScalar := shr(224, calldataload(offset)) // uint32


For the basefeescalar and the blobbasefeescalar we technically do not need to even parse them into their own values on the stack, since they are next to each other in the calldata and in storage we can sstore(slot, shr(160, calldataload(4)) or something like that

tynes · 2023-12-20T23:04:35Z

packages/contracts-bedrock/test/L2/L1Block.t.sol

@@ -3,32 +3,53 @@ pragma solidity 0.8.15;

 // Testing utilities
 import { CommonTest } from "test/setup/CommonTest.sol";
+import "forge-std/console.sol";


nit: dead code

roberto-bayardo · 2023-12-20T23:30:45Z

packages/contracts-bedrock/src/L2/GasPriceOracle.sol

+                total += 16;
+            }
+        }
+        return (total + (68 * 16 * 16)) / 16;


Suggested change

return (total + (68 * 16 * 16)) / 16;

return (total / 16) + 68

roberto-bayardo · 2023-12-20T23:39:07Z

packages/contracts-bedrock/src/L2/GasPriceOracle.sol

@@ -93,7 +162,23 @@ contract GasPriceOracle is ISemver {
                total += 16;
            }
        }
-        uint256 unsigned = total + overhead();
+        uint256 unsigned = total + L1Block(Predeploys.L1_BLOCK_ATTRIBUTES).l1FeeOverhead();


Suggested change

uint256 unsigned = total + L1Block(Predeploys.L1_BLOCK_ATTRIBUTES).l1FeeOverhead();

uint256 unsigned = total + isEcotone ? 0 : L1Block(Predeploys.L1_BLOCK_ATTRIBUTES).l1FeeOverhead();

...From Slack discussion where the proposal is to just make this return "calldataGas".

then you can drop the _getL1GasUsedEcotone() function entirely.

worth noting that with this approach the _getL1FeeBedrock / _getL1FeeEcotone methods are checking the isEcotone flag twice. are we ok with that additional gas usage?

roberto-bayardo · 2023-12-20T23:44:29Z

packages/contracts-bedrock/src/L2/GasPriceOracle.sol

+        uint256 l1GasUsed = _getL1GasUsedEcotone(_data);
+        uint256 scaledBasefee = basefeeScalar() * 16 * l1BaseFee();
+        uint256 scaledBlobBasefee = blobBasefeeScalar() * blobBasefee();
+        uint256 unscaled = l1GasUsed * (scaledBasefee + scaledBlobBasefee);
+        uint256 divisor = 10 ** DECIMALS;
+        uint256 scaled = unscaled / divisor;


We want to move the division by 16 to the end to preserve precision.
If we change l1GasUsed as suggested above:

Suggested change

uint256 l1GasUsed = _getL1GasUsedEcotone(_data);

uint256 scaledBasefee = basefeeScalar() * 16 * l1BaseFee();

uint256 scaledBlobBasefee = blobBasefeeScalar() * blobBasefee();

uint256 unscaled = l1GasUsed * (scaledBasefee + scaledBlobBasefee);

uint256 divisor = 10 ** DECIMALS;

uint256 scaled = unscaled / divisor;

uint256 l1GasUsed = _getL1GasUsed(_data);

uint256 scaledBasefee = basefeeScalar() * 16 * l1BaseFee();

uint256 scaledBlobBasefee = blobBasefeeScalar() * blobBasefee();

uint256 unscaled = l1GasUsed * (scaledBasefee + scaledBlobBasefee);

uint256 divisor = 16 * 10 ** DECIMALS;

uint256 scaled = unscaled / divisor;

tynes · 2023-12-20T23:59:22Z

packages/contracts-bedrock/src/L2/GasPriceOracle.sol

-        uint256 scaled = unscaled / divisor;
-        return scaled;
+        if (isEcotone) {
+            _getL1FeeEcotone(_data);


This needs to return

anikaraghu · 2023-12-21T03:14:00Z

packages/contracts-bedrock/src/L2/GasPriceOracle.sol

@@ -93,7 +159,10 @@ contract GasPriceOracle is ISemver {
                total += 16;
            }
        }
-        uint256 unsigned = total + overhead();
+        uint256 unsigned = total;
+        if (!isEcotone) {


Note that with this approach we check the isEcotone flag twice when we call this from _getL1FeeBedrock/Ecotone. Thoughts on this tradeoff(alternative being have a separate internal function)

roberto-bayardo · 2023-12-21T19:17:11Z

Taking over this PR by reopening here: #8726
(Anika is out for the holidays)

tynes · 2023-12-22T04:21:06Z

Closing in favor of the other PR

Updated pricing function and set packed values

3eb5599

roberto-bayardo reviewed Dec 20, 2023

View reviewed changes

packages/contracts-bedrock/src/L2/GasPriceOracle.sol Show resolved Hide resolved

roberto-bayardo reviewed Dec 20, 2023

View reviewed changes

tynes reviewed Dec 20, 2023

View reviewed changes

roberto-bayardo reviewed Dec 20, 2023

View reviewed changes

tynes reviewed Dec 20, 2023

View reviewed changes

anikaraghu mentioned this pull request Dec 21, 2023

EIP4844 L1 Smart Contract Updates #8715

Closed

anikaraghu force-pushed the anika/4844-contracts branch from 4b523e2 to 865ee19 Compare December 21, 2023 03:12

anikaraghu commented Dec 21, 2023

View reviewed changes

Comments

fc71f2b

anikaraghu force-pushed the anika/4844-contracts branch from 865ee19 to fc71f2b Compare December 21, 2023 03:14

tynes closed this Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP4844 smart contract updated #8712

EIP4844 smart contract updated #8712

anikaraghu commented Dec 20, 2023

roberto-bayardo Dec 20, 2023

codecov bot commented Dec 20, 2023 •

edited

Loading

roberto-bayardo Dec 20, 2023

roberto-bayardo Dec 20, 2023

tynes Dec 20, 2023

tynes Dec 20, 2023

tynes Dec 20, 2023

roberto-bayardo Dec 20, 2023 •

edited

Loading

roberto-bayardo Dec 20, 2023

anikaraghu Dec 21, 2023

roberto-bayardo Dec 20, 2023

tynes Dec 20, 2023

anikaraghu Dec 21, 2023

roberto-bayardo commented Dec 21, 2023

tynes commented Dec 22, 2023

	/// @notice Flag that indicates whether the network is in ecotone mode.
	/// @notice indicates whether the network has gone through the Ecotone upgrade

	return (total + (68 * 16 * 16)) / 16;
	return (total / 16) + 68

	uint256 unsigned = total + L1Block(Predeploys.L1_BLOCK_ATTRIBUTES).l1FeeOverhead();
	uint256 unsigned = total + isEcotone ? 0 : L1Block(Predeploys.L1_BLOCK_ATTRIBUTES).l1FeeOverhead();

EIP4844 smart contract updated #8712

EIP4844 smart contract updated #8712

Conversation

anikaraghu commented Dec 20, 2023

Choose a reason for hiding this comment

codecov bot commented Dec 20, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberto-bayardo Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberto-bayardo commented Dec 21, 2023

tynes commented Dec 22, 2023

codecov bot commented Dec 20, 2023 •

edited

Loading

roberto-bayardo Dec 20, 2023 •

edited

Loading