Add L1 Data Fee Deduction #270
Conversation
WalkthroughThe pull request introduces modifications to enhance the functionality of the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
natebeauregard
force-pushed
the
nkb.l1-data-fee
branch
2 times, most recently
from
2439591 to
a6b24b1
Compare
| simulate bool, | ||
| next sdk.AnteHandler, | ||
| ) (sdk.Context, error) { | ||
| l1BlockInfo, err := d.rollupKeeper.GetL1BlockInfo(ctx) |
There was a problem hiding this comment.
I looked into adding unit/integration test coverage for a while and eventually decided to split it out for the time being since it was blowing up in scope.
I created #281 to refactor how we pass around the rollup keeper to use an interface that can be mocked instead of a struct and to add unit tests for ante.go and l1_data_decorator.go.
Also, this logic will eventually be tested in our e2e tests once #267 merges.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
x/rollup/tx/helpers/l1_data_ante.go (1)
9-11: Enhance function documentation for better clarity.
While the current documentation explains the basic purpose, consider adding more details about:
- The gas calculation method used
- How it aligns with the OP Stack Specification
- The expected parameters in
HandlerOptions
Example enhancement:
// NewL1DataAnteHandler has a single decorator (NewConsumeGasForL1DataDecorator)
// to consume gas to compensate the sequencer for posting the transaction to Ethereum.
+// It implements the OP Stack Specification for L1 data fee calculations based on
+// transaction size and current L1 base fee. The HandlerOptions must include a
+// non-nil RollupKeeper for accessing L1 block information.x/rollup/keeper/keeper.go (2)
35-35: Consider enhancing the documentation.
While the updated comment is more specific, it could be more descriptive about the purpose and behavior of the method.
Consider updating the comment to:
-// EmitEvents prepares a `message` event with the module name and emits it
-// along with the provided events.
+// EmitEvents prepares and emits a sequence of events, prefixing them with a
+// standard module-specific message event that includes the rollup module name
+// as an attribute. This ensures all emitted events are properly tagged with
+// their originating module.49-49: Add comprehensive documentation for L1 data fee integration.
Given this method's importance in L1 data fee calculations, please add detailed documentation explaining:
- The purpose and role in L1 data fee deduction
- The structure and meaning of L1BlockInfo
- When and how this method is expected to be called
- Any assumptions about data availability
Example documentation:
// GetL1BlockInfo retrieves the current L1 block information used for calculating
// L1 data fees according to the OP Stack Specification. This information is
// crucial for determining gas costs for posting data to L1 and ensuring proper
// sequencer compensation.
//
// Returns nil and an error if the information is not available or invalid.
// The caller should handle these cases appropriately to prevent incorrect
// fee calculations.cmd/monogen/testapp/app/app.go (1)
Line range hint 1-50: Architecture suggestion: Consider documenting the L1 data fee integration.
Given the significant architectural change of integrating L1 data fees, consider adding documentation in the package comments to explain:
- The role of L1 data fees in the OP Stack
- How the
RollupKeeperfacilitates L1 data fee calculations - The relationship between the ante handler and L1 data fee processing
Add this documentation at the package level:
package app
+// Package app implements the Monomer application with L1 data fee support.
+// It integrates with the OP Stack Specification by calculating and deducting
+// L1 data fees through the RollupKeeper and specialized ante handlers.
+
import (x/rollup/tx/helpers/ante.go (1)
35-35: Consider passing HandlerOptions by reference to improve efficiency
Passing the HandlerOptions struct by value may lead to unnecessary copying of the struct's contents, which can impact performance if the struct grows larger. Consider passing it by reference to optimize resource usage.
Apply this diff to modify the function signature:
-func NewAnteHandler(options HandlerOptions) (*AnteHandler, error) { //nolint:gocritic // hugeParam
+func NewAnteHandler(options *HandlerOptions) (*AnteHandler, error) {And update references to options accordingly.
x/rollup/tx/helpers/l1_data_decorator.go (1)
48-48: Use integer literal for divisor to prevent unintended floating-point conversion.
The constant divisor is currently defined using 16e6, which is a floating-point literal. Since big.NewInt expects an integer, it's clearer and safer to define divisor as an integer literal.
Apply this diff:
const (
// scaledBaseFee calculation: scaledBaseFee = l1BaseFee * l1BaseFeeScalar * 16
baseFeeMultiplier = 16
- divisor = 16e6
+ divisor = 16000000
)📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
cmd/monogen/testapp.zipis excluded by!**/*.zip
📒 Files selected for processing (5)
- cmd/monogen/testapp/app/app.go (1 hunks)
- x/rollup/keeper/keeper.go (3 hunks)
- x/rollup/tx/helpers/ante.go (2 hunks)
- x/rollup/tx/helpers/l1_data_ante.go (1 hunks)
- x/rollup/tx/helpers/l1_data_decorator.go (1 hunks)
🔇 Additional comments (12)
x/rollup/tx/helpers/l1_data_ante.go (3)
1-8: LGTM: Clean package structure and imports.
The package name and imports are well-organized and appropriate for the L1 data fee handling functionality.
16-21: Verify L1 data gas consumption calculation.
The implementation looks clean, but given the critical nature of gas calculations:
- Ensure
NewConsumeGasForL1DataDecoratorcorrectly implements the OP Stack Specification for L1 data fees - Verify the decorator's order in relation to other ante handlers in the chain
Let's examine the decorator implementation and its usage:
#!/bin/bash
# Description: Check L1 data decorator implementation and usage
echo "Checking decorator implementation:"
rg -A 10 "type ConsumeGasForL1DataDecorator struct"
echo "Checking decorator usage in other ante handlers:"
rg -A 5 "NewConsumeGasForL1DataDecorator"11-14: Consider additional validation for HandlerOptions.
While the RollupKeeper validation is good, consider if other fields in HandlerOptions should also be validated to ensure robust initialization.
Let's check the HandlerOptions structure:
x/rollup/keeper/keeper.go (1)
5-5: LGTM: Import required for error handling.
The fmt import is appropriately added for error wrapping in the new GetL1BlockInfo method.
cmd/monogen/testapp/app/app.go (1)
165-171: LGTM: AnteHandler integration looks good.
The integration of RollupKeeper into the HandlerOptions aligns with the PR objectives for L1 data fee handling. The error handling is properly maintained.
Let's verify the HandlerOptions structure and its usage:
✅ Verification successful
Let me gather more information about the usage of HandlerOptions to ensure complete verification.
Let me check one more aspect to verify the L1DataAnteHandler implementation since it's a crucial part of the changes.
HandlerOptions integration is correctly implemented and used
The verification confirms that:
- HandlerOptions struct properly includes RollupKeeper
- NewAnteHandler correctly initializes both auth and L1 data ante handlers
- L1DataAnteHandler properly validates and uses RollupKeeper for L1 data fee handling
- The implementation aligns with the PR objectives for L1 data fee deductions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the HandlerOptions structure and its usage
# Test: Check if HandlerOptions is consistently used across the codebase
rg -A 5 "type HandlerOptions struct"
Length of output: 476
Script:
#!/bin/bash
# Check the NewAnteHandler function implementation and usage
ast-grep --pattern 'func NewAnteHandler(options HandlerOptions)'
Length of output: 1669
Script:
#!/bin/bash
# Check the L1DataAnteHandler implementation
ast-grep --pattern 'func NewL1DataAnteHandler(options HandlerOptions)'
Length of output: 901
x/rollup/tx/helpers/ante.go (5)
17-18: Addition of l1DataAnteHandler enhances transaction processing
The introduction of l1DataAnteHandler to the AnteHandler struct effectively extends the ante handler chain to include L1 data fee calculations, aligning with the PR objectives and ensuring compliance with the OP Stack Specification.
21-33: HandlerOptions struct consolidates dependencies for clarity
Defining the new HandlerOptions struct encapsulates all the necessary dependencies for constructing the AnteHandler, including the RollupKeeper. This promotes better code organization and maintainability.
35-44: NewAnteHandler correctly initializes ante handlers with proper error handling
The NewAnteHandler function initializes both authAnteHandler and l1DataAnteHandler using the provided options. The error handling ensures that any issues during initialization are properly captured and reported.
49-52: Verify that NewL1DataAnteHandler utilizes all necessary options
Ensure that NewL1DataAnteHandler(options) correctly leverages all relevant fields from HandlerOptions, particularly the RollupKeeper, to initialize the L1 data ante handler appropriately.
77-80: Confirm the sequence of ante handlers for correct fee deduction
The l1DataAnteHandler is invoked after the authAnteHandler in the AnteHandle method. Verify that this order ensures accurate processing of authentication before L1 data fee deduction, which is critical for maintaining transaction integrity and compliance.
x/rollup/tx/helpers/l1_data_decorator.go (2)
57-57: Verify that divisor is not zero to prevent division by zero errors.
Although divisor is a constant set to 16000000, it's good practice to ensure that it is not zero before performing division to prevent potential runtime panics.
71-74: Ensure that sdk.Tx is compatible with RLP encoding.
The getL1GasUsed function attempts to RLP-encode sdk.Tx. Verify that all transaction types implementing sdk.Tx are compatible with rlp.EncodeToBytes. Incompatible types may lead to encoding errors or unexpected behavior.
| if !l1DataCost.IsUint64() { | ||
| return ctx, fmt.Errorf("l1 data cost overflow: %s", l1DataCost) | ||
| } |
There was a problem hiding this comment.
is this a possibility or is it technically impossible?
There was a problem hiding this comment.
In practice I don't think that this block should ever be hit, however I wanted to add the extra safeguard since we're dealing with a lot of multiplication here. This would prevent against the case where one of the L1 attributes either unexpectedly increased or was misconfigured in the system deposit tx.
That being said, I'm not sure how useful this check is. Do you think I should just remove this block for the overflow case since it shouldn't ever be hit in production?
Add gas consumption for L1 data fees as well to fulfill the OP Stack Spec and compensate the sequencer for posting data on L1. To do this, a new decorator to the default AnteHandler to calculate the L1 data fee is added.
natebeauregard
force-pushed
the
nkb.l1-data-fee
branch
from
a6b24b1 to
a858661
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
x/rollup/tx/l1_data.go (1)
68-68: Clarify the origin of the 68 in signatureCost
In the getL1GasUsed function, the constant signatureCost is defined as 68 * 16. It's not immediately clear where the value 68 comes from. For maintainability and readability, consider adding a comment explaining the significance of 68 in this context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
cmd/monogen/testapp.zipis excluded by!**/*.zip
📒 Files selected for processing (4)
- cmd/monogen/testapp/app/app.go (1 hunks)
- x/rollup/keeper/keeper.go (3 hunks)
- x/rollup/tx/helpers/ante.go (2 hunks)
- x/rollup/tx/l1_data.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/monogen/testapp/app/app.go
🔇 Additional comments (7)
x/rollup/keeper/keeper.go (3)
5-5: LGTM!
The fmt import is appropriately added for error handling in the new GetL1BlockInfo method.
35-35: LGTM!
The updated comment clearly describes the method's purpose and behavior.
49-59: Consider thread safety and error handling implications.
Since this method will be used in the ante handler for L1 data fee calculations:
- Consider documenting thread safety guarantees, as this will be called during transaction processing.
- The error handling should be carefully considered as it directly impacts fee calculations. Missing or invalid L1 block info could lead to incorrect fee computations.
Let's verify the usage context:
x/rollup/tx/helpers/ante.go (4)
13-14: Addition of rollupKeeper field to AnteHandler struct
Including the rollupKeeper in the AnteHandler struct enhances the handler's ability to access rollup-specific functionalities, aligning with the PR objectives to integrate L1 data fee calculations.
17-20: Update NewAnteHandler signature to include rollupKeeper
Passing rollupKeeper as a separate argument to NewAnteHandler improves modularity and addresses previous feedback. This change ensures better dependency injection and adheres to best practices.
27-28: Initialize new fields in AnteHandler
Updating the struct initialization to include authAnteHandler and rollupKeeper ensures all necessary components are properly set up for the ante handler's operation.
45-52: Integrate L1DataAnteHandler after authAnteHandler
Adding rolluptx.L1DataAnteHandler after the authAnteHandler call ensures that L1 data fees are accurately calculated and deducted, fulfilling the PR objectives. Please verify that the order of ante handlers is correct and that integrating the new handler does not introduce unintended side effects.
Consider testing transactions with L1 data to ensure that the fee deduction behaves as expected and complies with the OP Stack Specification.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
x/rollup/tx/l1_data.go (1)
66-68: Provide context for constants to enhance readability
The constants zeroCost, oneCost, and signatureCost are used without explanation. Adding comments or references to their origins would improve code clarity and maintainability.
Consider updating the code to:
const zeroCost = 4 // Cost per zero byte in L1 gas calculation
const oneCost = 16 // Cost per non-zero byte in L1 gas calculation
const signatureCost = 68 * 16 // Fixed cost for transaction signature in L1 gas calculation
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/rollup/tx/l1_data.go(1 hunks)
🔇 Additional comments (1)
x/rollup/tx/l1_data.go (1)
61-64: Ensure sdk.Tx is compatible with RLP encoding
The rlp.EncodeToBytes function is used to encode sdk.Tx, but it's important to verify that sdk.Tx implements the necessary interfaces for RLP encoding. Failing to do so may result in runtime errors during encoding.
Please confirm that sdk.Tx can be successfully RLP encoded. You can run the following script to check for any potential issues:
✅ Verification successful
Based on the gathered information, I can now generate the final response:
RLP encoding is correctly used for L1 data gas calculation
The RLP encoding is used specifically to calculate the L1 data gas cost for transactions that will be posted to Ethereum L1 by the sequencer. The code handles both deposit transactions (which are Ethereum transactions) and regular Cosmos SDK transactions. The encoding is used only for size calculation purposes and doesn't affect the transaction's execution.
- The
getL1GasUsedfunction uses RLP encoding to estimate the gas cost of posting the transaction to L1 - The codebase shows proper handling of both Ethereum transactions (
MsgApplyL1Txs) and Cosmos SDK transactions - The RLP encoding is used as a size estimation mechanism, not for transaction execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that sdk.Tx can be RLP encoded without errors.
# Test: Attempt to encode an empty or sample sdk.Tx object.
# Expected result: No errors during encoding.
# Note: Replace `sampleTx` with an actual instance of sdk.Tx in your test environment.
go run -e 'package main; import (
"fmt"
"github.com/ethereum/go-ethereum/rlp"
sdk "github.com/cosmos/cosmos-sdk/types"
); func main() {
var sampleTx sdk.Tx // Initialize sampleTx appropriately.
_, err := rlp.EncodeToBytes(sampleTx)
if err != nil {
fmt.Println("RLP encoding error:", err)
} else {
fmt.Println("RLP encoding successful")
}
}'
Length of output: 424
Script:
#!/bin/bash
# Let's examine the actual type being encoded and its implementation
rg -A 5 "func \(.*\) L1Data" x/rollup/tx/
# Also check where this tx is created/initialized
rg -B 5 "rlp\.EncodeToBytes\(tx\)" x/rollup/tx/
# Look for any custom RLP encoding implementations
ast-grep --pattern 'func ($x *$type) EncodeRLP($y *$z) $$$'
Length of output: 573
Script:
#!/bin/bash
# Let's check the type and implementation of tx in the context
rg -B 10 "func getL1GasUsed" x/rollup/tx/l1_data.go
# Check for any type definitions or interfaces related to this tx
rg -A 5 "type.*Tx.*struct" x/rollup/tx/
# Look for any imports that might give us clues about the tx type
rg "import \(" x/rollup/tx/l1_data.go -A 10
Length of output: 1104
Script:
#!/bin/bash
# Let's check how this tx is being used in the codebase
rg -B 5 "getL1GasUsed" x/rollup/tx/
# Look for any transaction type definitions or implementations
rg -A 10 "type.*Transaction" .
# Check for any RLP-related implementations or interfaces
rg -A 5 "rlp\." x/rollup/tx/
Length of output: 42820
Script:
#!/bin/bash
# Let's check the actual implementation of the tx being passed to getL1GasUsed
rg -B 10 "getL1GasUsed" x/rollup/tx/l1_data.go
# Check for any message or transaction definitions in the rollup module
rg -A 5 "type Msg" x/rollup/
# Look for where this tx is created/constructed
rg -B 5 "func.*AnteHandle" x/rollup/
Length of output: 5414
| // L1DataAnteHandler will consume gas to compensate the sequencer for posting | ||
| // the transaction to Ethereum. The gas cost is calculated based on the Ecotone | ||
| // upgrade and the sequencer is expected to post the transaction using blobs. | ||
| func L1DataAnteHandler(ctx sdk.Context, tx sdk.Tx, rollupKeeper *rollupkeeper.Keeper) (sdk.Context, error) { //nolint:gocritic // hugeparam |
There was a problem hiding this comment.
Should the behavior change if the tx is being simulated? We currently don't accept a simulate arg like other ante handlers, not sure if that's an issue.
There was a problem hiding this comment.
There shouldn't be any behavior difference for us during simulate mode since we're basing the gas consumption on the current L1 attributes and the size of the tx. The current L1 attributes could change between the simulated tx and the real tx and result in an out-of-gas error. However, the user is expected to account for that when setting their gas limit for the real tx (normally 1.1x or 1.2x of the gas used for the simulated tx).
Also, simulate is passed into DepositAnteHandler but it's never used there.
This reverts commit db7bef5.
Closes #268
Overview
Currently, Monomer applications will contain all execution gas fees in the
FeeCollectormodule account.However, we need to add gas consumption for L1 data fees as well to fulfill the OP Stack Spec and compensate the sequencer for posting data on L1. To do this, we can add a new decorator to the default
AnteHandlerto calculate the L1 data fee. For posting batches via blobs, we can use the following formula:(zeroes*4 + ones*16) * (16*l1BaseFee*l1BaseFeeScalar + l1BlobBaseFee*l1BlobBaseFeeScalar) / 16e6Implementation
ConsumeGasForL1DataDecorator) to the MonomerAnteHandlerto include cosuming gas for L1 data fees.Summary by CodeRabbit
New Features
Improvements
AnteHandlerto integrate rollup operations, improving transaction handling flow.