Add x/rollup genesis state #289
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes across multiple files, focusing on enhancing the functionality and configuration of the rollup module. Key modifications include the addition of genesis state management, improved handling of gas limits in withdrawal processes, and the introduction of new parameters for fee withdrawals. Unused imports have been removed, and testing functionalities have been enhanced to validate these new features. The overall structure of the code remains intact, with an emphasis on dynamic configuration rather than hardcoded values. Changes
Assessment against linked issues
Possibly related PRs
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.add-genesis-state
branch
2 times, most recently
from
c87b82d
to
cc58441
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (12)
proto/rollup/v1/genesis.proto (1)
10-14: Consider enhancing the documentation.The GenesisState message structure is well-defined and correctly annotated. However, the documentation could be more descriptive about the available parameters within the Params type (l1_fee_recipient, l1_cross_domain_messenger, min_fee_withdrawal_amount, fee_withdrawal_gas_limit).
Add more detailed documentation:
// GenesisState defines the rollup module's genesis state. +// It contains all the parameters required for the rollup module's initialization, +// including L1 fee recipient address, cross-domain messenger address, +// minimum fee withdrawal amount, and fee withdrawal gas limit. message GenesisState { // params defines all the parameters of the module. + // These parameters control the module's fee withdrawal behavior and L1 interactions. Params params = 1 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true]; }x/rollup/types/genesis.go (1)
1-22: Add tests for genesis state validation.While the implementation looks good, we should ensure proper test coverage for:
- Genesis state creation with various parameter combinations
- Default genesis state validation
- Invalid genesis state scenarios
Would you like me to help generate comprehensive test cases for this file?
x/rollup/keeper/genesis.go (1)
10-15: Consider adding parameter validation and using pointer receiver.While the implementation follows Cosmos SDK patterns, consider these improvements:
- Add validation for required fields (l1_fee_recipient, l1_cross_domain_messenger) before setting params
- Consider passing GenesisState by pointer to improve performance with large states
-func (k *Keeper) InitGenesis(ctx sdk.Context, state types.GenesisState) { +func (k *Keeper) InitGenesis(ctx sdk.Context, state *types.GenesisState) { + if err := state.Params.Validate(); err != nil { + panic(fmt.Errorf("invalid genesis state: %w", err)) + } - if err := k.SetParams(ctx, &state.Params); err != nil { + if err := k.SetParams(ctx, &state.Params); err != nil { panic(fmt.Errorf("failed to set params: %w", err)) } }proto/rollup/v1/rollup.proto (2)
8-18: LGTM! The Params message structure is well-defined.The message definition is clear, with appropriate field types and good documentation.
Consider the following improvements:
- Add field options to validate Ethereum addresses:
- string l1_fee_recipient = 1; + string l1_fee_recipient = 1 [(gogoproto.customtype) = "github.com/ethereum/go-ethereum/common.Address"]; - string l1_cross_domain_messenger = 2; + string l1_cross_domain_messenger = 2 [(gogoproto.customtype) = "github.com/ethereum/go-ethereum/common.Address"];
- Enhance documentation by specifying units:
- // Minimum amount of L2 fees that the FeeCollector account must have before they can be withdrawn. + // Minimum amount of L2 fees (in wei) that the FeeCollector account must have before they can be withdrawn. uint64 min_fee_withdrawal_amount = 3;
9-18: Consider future extensibility requirements.Based on the PR objectives mentioning future parameter updates and queries:
- The
Paramsmessage might need versioning support for future updates- Consider adding field options to support JSON serialization for query responses
Example improvements:
message Params { + option (gogoproto.equal) = true; + option (gogoproto.goproto_stringer) = false; + option (gogoproto.stringer) = false; + option (gogoproto.goproto_getters) = false; // L1 address to forward the L2 fees to. - string l1_fee_recipient = 1; + string l1_fee_recipient = 1 [(gogoproto.jsontag) = "l1_fee_recipient"]; // ... apply similar changes to other fields }x/rollup/types/params.go (1)
1-50: Add comprehensive documentation for parameters.Consider adding detailed documentation for each parameter:
- Purpose and impact of each parameter
- Recommended values for different networks
- Examples of parameter configuration in genesis.json
x/rollup/keeper/keeper.go (2)
64-74: Consider handling nil paramsBz caseThe implementation looks good, but consider adding a nil check for
paramsBzto handle the case when parameters haven't been set yet. This is particularly important during genesis.func (k *Keeper) GetParams(ctx sdk.Context) (*types.Params, error) { //nolint:gocritic // hugeParam paramsBz, err := k.storeService.OpenKVStore(ctx).Get([]byte(types.ParamsKey)) if err != nil { return nil, fmt.Errorf("get params: %w", err) } + if paramsBz == nil { + return nil, fmt.Errorf("params not found") + } var params types.Params if err = params.Unmarshal(paramsBz); err != nil { return nil, fmt.Errorf("unmarshal params: %w", err)
64-86: Consider adding parameter change hooksSince these parameters (especially
l1_fee_recipientandl1_cross_domain_messenger) are critical for cross-chain operations, consider implementing parameter change hooks to notify dependent components when parameters are updated. This would help maintain system consistency across L1/L2.x/rollup/keeper/deposits.go (1)
139-139: Add validation for L1CrossDomainMessenger addressWhile the address aliasing is correctly implemented, consider adding validation to ensure the L1CrossDomainMessenger address from parameters is a valid Ethereum address before applying the aliasing.
+ if !common.IsHexAddress(params.L1CrossDomainMessenger) { + return nil, types.WrapError(types.ErrInvalidL1Txs, "invalid L1CrossDomainMessenger address format") + } aliasedL1CrossDomainMessengerAddress := crossdomain.ApplyL1ToL2Alias(common.HexToAddress(params.L1CrossDomainMessenger))builder/builder_test.go (1)
424-427: Consider enhancing error handling and documentation.While the implementation is correct, consider these improvements:
- Add more context to the error message for better debugging.
- Expand the comment to explain why fees won't be collected in the test environment (e.g., mock transactions, no real L1 interaction, etc.).
Apply this diff:
// Set min_fee_withdrawal_amount to 0 for testing purposes since fees will not be collected in the test environment. + // Fees are not collected because test transactions are mocked and there's no real L1 interaction. rollupAppState, err := sjson.Set(string(appState[types.ModuleName]), "params.min_fee_withdrawal_amount", "0") -require.NoError(t, err) +require.NoError(t, err, "failed to set min_fee_withdrawal_amount in rollup app state")x/rollup/keeper/msg_server.go (2)
91-94: Simplify error handling inGetParamsIf
k.GetParams(ctx)is guaranteed not to return an error (as is common in parameter retrieval methods), consider removing the error handling to streamline the code.
106-108: Enhance error message formatting for clarityThe error message could be improved by formatting both the balance and the minimum withdrawal amount consistently. Consider converting
params.MinFeeWithdrawalAmountto a string or formattingfeeCollectorBalance.Amountto match the data type for clearer communication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
cmd/monogen/testapp.zipis excluded by!**/*.zipgo.sumis excluded by!**/*.sumx/rollup/types/genesis.pb.gois excluded by!**/*.pb.gox/rollup/types/rollup.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
builder/builder.go(1 hunks)builder/builder_test.go(4 hunks)cmd/monogen/testapp/app/app_config.go(1 hunks)go.mod(2 hunks)proto/rollup/v1/genesis.proto(1 hunks)proto/rollup/v1/rollup.proto(1 hunks)testutils/utils.go(0 hunks)x/rollup/keeper/deposits.go(2 hunks)x/rollup/keeper/genesis.go(1 hunks)x/rollup/keeper/keeper.go(2 hunks)x/rollup/keeper/keeper_test.go(1 hunks)x/rollup/keeper/msg_server.go(3 hunks)x/rollup/keeper/msg_server_test.go(2 hunks)x/rollup/module.go(7 hunks)x/rollup/types/genesis.go(1 hunks)x/rollup/types/keys.go(1 hunks)x/rollup/types/params.go(1 hunks)
💤 Files with no reviewable changes (1)
- testutils/utils.go
🔇 Additional comments (33)
proto/rollup/v1/genesis.proto (2)
1-8: LGTM! File structure and imports are well-organized.
The file follows Protocol Buffers best practices with appropriate syntax, package naming, and necessary imports.
1-14: Verify alignment with Params message definition.
Let's ensure that the imported Params message type from rollup.proto contains all the required fields mentioned in the PR objectives.
✅ Verification successful
Genesis state correctly references complete Params definition
The GenesisState message properly imports and references the Params message which contains all the essential rollup configuration fields:
- L1 fee recipient address
- L1 cross-domain messenger address
- Minimum fee withdrawal amount
- Fee withdrawal gas limit
These parameters fully support the module's requirements for L1/L2 fee management and cross-domain messaging functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Params message in rollup.proto contains all required fields
# Test: Search for Params message definition
ast-grep --pattern 'message Params {
$$$
}'
# Test: Verify specific field presence
rg -A 10 'message Params' rollup/v1/rollup.proto
Length of output: 161
Script:
#!/bin/bash
# Let's first locate the rollup.proto file
fd rollup.proto
# Then search for Params message in all proto files under the repository
rg -A 10 'message Params' --type proto
Length of output: 117
Script:
#!/bin/bash
# Now that we found the correct path, let's examine the content of rollup.proto
cat proto/rollup/v1/rollup.proto
# Also check for any other proto files in the rollup directory that might contain related definitions
ls -la proto/rollup/v1/
Length of output: 1791
x/rollup/types/keys.go (2)
20-21: LGTM! Improved naming consistency.
The renaming from KeyL1BlockInfo to L1BlockInfoKey and the value change to snake_case follows better naming conventions. The new name is more idiomatic Go, and the value format is consistent with Cosmos SDK store key conventions.
22-23: LGTM! Verify the params key usage.
The new ParamsKey constant aligns with the PR objectives for storing module parameters.
Let's verify the usage of this new params key:
✅ Verification successful
ParamsKey is correctly used in the keeper's parameter management methods
The ParamsKey constant is properly integrated and used in the keeper's parameter management system:
- Used in
GetParams()to retrieve parameters from the store - Used in
SetParams()to store parameters - Corresponds to a well-defined
Paramsstruct containing L1-related configuration - Implementation follows standard parameter storage patterns
The verification confirms proper usage of the params key in the expected parameter management functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of the new ParamsKey constant
# Expected: Should find usage in keeper methods for parameter management
# Search for direct usage of the ParamsKey constant
echo "Searching for ParamsKey usage..."
rg -A 3 "ParamsKey"
# Search for params-related keeper methods that might use this key
echo "Searching for parameter-related keeper methods..."
ast-grep --pattern 'func ($receiver *Keeper) Get[A-Za-z]*Params($$$) {
$$$
}'
Length of output: 946
Script:
#!/bin/bash
# Let's get more context around the parameter handling in the keeper
echo "Checking parameter-related methods in keeper..."
rg -B 2 -A 5 "func.*Params" x/rollup/keeper/
# Check for any parameter-related tests
echo "Checking parameter-related tests..."
rg -B 2 -A 5 "Test.*Params" x/rollup/keeper/
# Look for parameter type definitions
echo "Checking parameter type definitions..."
rg -B 2 -A 5 "type.*Params" x/rollup/types/
Length of output: 2197
x/rollup/types/genesis.go (3)
3-8: LGTM! Clean and well-documented constructor.
The implementation follows Go's constructor pattern and is properly documented.
15-22: Consider wrapping validation errors for better context.
The implementation is correct but could benefit from wrapping the error to provide more context about where the validation failed.
func ValidateGenesis(state GenesisState) error {
if err := state.Params.Validate(); err != nil {
- return err
+ return fmt.Errorf("invalid rollup genesis state: %w", err)
}
return nil
}#!/bin/bash
# Description: Verify Params.Validate implementation checks all required parameters
# Expected: Find validation checks for all required parameters
ast-grep --pattern 'func (p Params) Validate() error {
$$$
}'10-13: Verify default parameter values.
The implementation is clean and follows DRY principles. However, we should verify that DefaultParams() provides appropriate default values for all required parameters mentioned in the PR objectives:
- l1_fee_recipient
- l1_cross_domain_messenger
- min_fee_withdrawal_amount
- fee_withdrawal_gas_limit
✅ Verification successful
Default parameter values are properly defined and initialized
The verification confirms that DefaultParams() correctly provides all required parameters with appropriate default values:
l1_fee_recipient: Set to a dead address "0x000000000000000000000000000000000000dEaD"l1_cross_domain_messenger: Set to "0x9A9f2CCfdE556A7E9Ff0848998Aa4a0CFD8863AE"min_fee_withdrawal_amount: Set to 400,000fee_withdrawal_gas_limit: Set to 400,000
The implementation properly initializes these values through NewParams() function, which ensures all required fields are set.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DefaultParams implementation includes all required parameters
# Expected: Find DefaultParams implementation with all required parameters
ast-grep --pattern 'func DefaultParams() Params {
$$$
}'
# Also check for any constants that might be used as default values
rg -i "default.*(?:fee|gas|messenger|recipient)"
Length of output: 244
Script:
#!/bin/bash
# Let's try a different approach to find DefaultParams and related code
# Search for DefaultParams function
rg -A 10 "func DefaultParams\(\)"
# Search for Params struct definition to understand the required fields
rg -A 10 "type Params struct"
# Search for any default-related constants
rg "const.*Default"
Length of output: 2118
Script:
#!/bin/bash
# Let's find the default constant values in params.go
# Search for default constants in params.go
rg "Default.*=.*" x/rollup/types/params.go
# Also check for NewParams function to understand parameter initialization
rg -A 5 "func NewParams" x/rollup/types/params.go
Length of output: 686
x/rollup/keeper/genesis.go (1)
1-9: LGTM! Clean package structure and imports.
The package structure and imports are minimal and appropriate for the genesis functionality.
x/rollup/types/params.go (4)
1-8: LGTM! Clean and minimal imports.
The package declaration and imports are appropriate for the functionality provided.
11-11: Verify the safety of using dead address as default L1FeeRecipient.
Using a dead address as the default fee recipient could lead to permanent loss of funds if not properly overridden during genesis. Consider adding a prominent warning in the documentation.
12-13: Verify the appropriateness of using devnet address as default.
Using a devnet address as the default L1CrossDomainMessenger in production code could cause issues if not properly configured. Consider:
- Using network-specific defaults
- Adding validation to prevent devnet addresses in production
14-15: Validate the default gas and withdrawal amounts.
The default values for MinFeeWithdrawalAmount and FeeWithdrawalGasLimit (400,000) need verification:
- Are these values sufficient for typical L1 transactions?
- Do they align with current gas costs on the target L1 network?
x/rollup/keeper/keeper.go (1)
Line range hint 53-63: LGTM: Key constant rename follows SDK conventions
The change to use types.L1BlockInfoKey aligns with Cosmos SDK naming conventions for store keys. The error handling and unmarshaling logic remain robust.
x/rollup/keeper/keeper_test.go (1)
67-67: Consider deriving mock balance from test parameters.
While increasing the mock balance to 1M WEI works, hardcoding this value could make tests brittle if min_fee_withdrawal_amount changes. Consider deriving this value from the test parameters to ensure the tests remain robust.
Let's verify how this mock is used in withdrawal tests:
cmd/monogen/testapp/app/app_config.go (3)
Line range hint 71-71: LGTM: Appropriate permissions for rollup operations.
The Minter and Burner permissions are correctly assigned to the rollup module account, which are necessary for managing cross-chain asset transfers between L1 and L2.
Line range hint 146-149: Verify parameter configuration mechanism.
The module configuration is currently empty. Based on the PR objectives, we need to configure several parameters:
- l1_fee_recipient
- l1_cross_domain_messenger
- min_fee_withdrawal_amount
- fee_withdrawal_gas_limit
Please verify if these parameters are properly initialized through genesis state rather than module configuration.
#!/bin/bash
# Description: Verify parameter initialization mechanism
# Check for parameter definitions in proto files
echo "Checking proto definitions..."
rg -t proto "l1_fee_recipient|l1_cross_domain_messenger|min_fee_withdrawal_amount|fee_withdrawal_gas_limit"
# Check for parameter handling in genesis
echo "Checking genesis parameter handling..."
rg -t go "InitGenesis|ExportGenesis" "x/rollup"39-39: Verify module initialization dependencies.
The placement of the rollup module in the genesis initialization order looks good. However, let's verify there are no specific dependencies that would require a different initialization order.
✅ Verification successful
Let me gather more specific information about module dependencies.
Based on the gathered information, I can now provide the final response:
Module initialization order is correctly placed
The rollup module's initialization is safe in its current position because:
- It only depends on basic bank and account keeper interfaces
- The genesis initialization is simple, only setting module parameters
- No complex dependencies or cross-module state requirements are present
- The module doesn't require any specific initialization order relative to other modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential module dependencies in the rollup module initialization
# Look for imports of other modules and keeper dependencies
# Search for module dependencies in rollup module
rg -t go "keeper\." "x/rollup"
# Search for InitGenesis parameters or dependencies
ast-grep --pattern 'func InitGenesis($_) {
$$$
}'
Length of output: 1362
Script:
#!/bin/bash
# Check keeper dependencies and module initialization requirements
# Look for keeper dependencies in the rollup module
rg -t go "Keeper struct" -A 10 "x/rollup"
# Check for any bank or staking keeper dependencies
rg -t go "bank\.|staking\." "x/rollup"
# Look at the actual genesis initialization code
cat x/rollup/keeper/genesis.go
Length of output: 3244
x/rollup/module.go (7)
76-77: LGTM! Documentation updates are clear and accurate.
The documentation changes correctly reflect that this is a rollup module implementation.
Also applies to: 85-86
101-103: LGTM! Proper initialization of default genesis state.
The implementation correctly uses the module's DefaultGenesisState() and marshals it to JSON.
106-117: LGTM! Robust genesis state validation implementation.
The implementation includes proper error handling and validation:
- Descriptive error messages for unmarshal failures
- Proper delegation to types.ValidateGenesis for domain-specific validation
120-120: LGTM! Command documentation is clear and consistent.
Documentation updates properly reflect the rollup module context.
Also applies to: 128-129, 133-134
142-143: LGTM! AppModule implementation follows SDK patterns.
The implementation and documentation properly reflect the rollup module's purpose.
Also applies to: 162-163, 167-167
179-184: LGTM! Genesis state management is properly implemented.
Both InitGenesis and ExportGenesis:
- Follow Cosmos SDK patterns
- Handle state marshaling/unmarshaling correctly
- Properly integrate with the keeper layer
Also applies to: 186-189
179-189: Verify genesis parameter handling implementation.
The genesis state implementation will manage the parameters specified in the PR objectives. Let's verify the parameter handling implementation:
✅ Verification successful
Let me gather more information about the parameter validation implementation to ensure proper handling.
Let me check one final aspect about the default parameter constants and address validation.
Genesis parameter handling implementation is properly implemented
The implementation correctly handles all required parameters with:
- Proper parameter definitions including L1FeeRecipient, L1CrossDomainMessenger, MinFeeWithdrawalAmount, and FeeWithdrawalGasLimit
- Complete validation logic for Ethereum addresses using
common.IsHexAddress - Sensible default values for all parameters
- Genesis state validation that checks parameter validity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the genesis parameter implementation in types package
# Test: Check for parameter definitions
echo "Checking for parameter definitions..."
rg -A 10 "type (Params|GenesisState)" "x/rollup/types"
# Test: Check for parameter validation
echo "Checking for parameter validation..."
rg "func.*ValidateGenesis" "x/rollup/types"
Length of output: 2325
Script:
#!/bin/bash
# Check the implementation of ValidateGenesis and parameter validation
echo "Checking genesis and parameter validation implementation..."
rg -A 10 "func ValidateGenesis" "x/rollup/types"
echo "Checking parameter validation methods..."
rg -A 5 "func.*Validate\(\)" "x/rollup/types"
echo "Checking default parameter values..."
rg "func DefaultParams" "x/rollup/types" -A 5
Length of output: 1597
Script:
#!/bin/bash
# Check default parameter constants and address validation
echo "Checking default parameter constants..."
rg "Default.*= " "x/rollup/types"
echo "Checking address validation implementation..."
rg -A 5 "func validateEthAddress" "x/rollup/types"
Length of output: 935
x/rollup/keeper/msg_server_test.go (2)
130-130: LGTM: Constant name change follows better naming convention.
The renaming from KeyL1BlockInfo to L1BlockInfoKey maintains consistent naming patterns across the codebase.
254-257: LGTM: Proper test setup for new genesis parameters.
The addition of default parameters setup is essential for testing the fee withdrawal functionality with the new configurable parameters introduced in the genesis state.
Let's verify that the default parameters match the PR objectives:
✅ Verification successful
Let me gather more information about the default parameter values to ensure they match the PR objectives.
Let me search for the Params struct definition with a different command.
Default parameters are properly configured for fee withdrawal functionality
The verification confirms that:
- All required parameters are present in the Params struct: L1FeeRecipient, L1CrossDomainMessenger, MinFeeWithdrawalAmount, and FeeWithdrawalGasLimit
- Default values are appropriately set:
- L1FeeRecipient: "0x000000000000000000000000000000000000dEaD"
- L1CrossDomainMessenger: "0x9A9f2CCfdE556A7E9Ff0848998Aa4a0CFD8863AE"
- MinFeeWithdrawalAmount: 400,000
- FeeWithdrawalGasLimit: 400,000
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the default parameters in types package include all required fields
# mentioned in PR objectives.
# Test: Search for default parameters definition
rg -A 10 'DefaultParams' 'x/rollup/types'
# Test: Verify parameter fields existence
ast-grep --pattern 'type Params struct {
$$$
L1FeeRecipient $_
L1CrossDomainMessenger $_
MinFeeWithdrawalAmount $_
FeeWithdrawalGasLimit $_
$$$
}'
Length of output: 1675
Script:
#!/bin/bash
# Search for default parameter values
rg "Default(L1FeeRecipient|L1CrossDomainMessenger|MinFeeWithdrawalAmount|FeeWithdrawalGasLimit)" x/rollup/types/params.go -B 1 -A 1
# Search for Params struct definition
rg "type Params struct" x/rollup/types/params.go -A 10
Length of output: 748
Script:
#!/bin/bash
# Search for Params struct definition using ast-grep
ast-grep --pattern 'type Params struct {
$$$
}'
# Alternative search using ripgrep
rg -U "type Params struct[\s\S]*?}" x/rollup/types/params.go
Length of output: 1335
builder/builder.go (1)
381-385: LGTM! Improved gas limit decoding.
The change simplifies gas limit handling by using hexutil.DecodeBig to decode directly into a *big.Int, eliminating the need for intermediate byte slice conversion. This aligns well with the PR's objective of supporting configurable fee withdrawal gas limits.
Let's verify the gas limit handling in withdrawal-related code:
✅ Verification successful
Gas limit handling is consistent across the codebase
The verification confirms that:
- The gas limit handling in
parseWithdrawalEventAttributesusinghexutil.DecodeBigis consistent with other value decoding patterns in the codebase - The change aligns with the module's gas limit handling, which includes:
- Defined constants for min/max gas limits in
x/rollup/types/msgs.go - Fee withdrawal gas limit parameter in
x/rollup/types/rollup.pb.go - Proper validation of gas limits in message handling
- Defined constants for min/max gas limits in
The simplified decoding approach is appropriate and maintains consistency with the codebase's gas limit handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent gas limit handling across withdrawal-related code
# Test: Search for gas limit handling patterns in withdrawal-related code
rg -A 3 'GasLimit.*hexutil\.DecodeBig|hexutil\.DecodeBig.*GasLimit'
Length of output: 69
Script:
#!/bin/bash
# Let's try a broader search for gas limit handling patterns
rg -A 3 'GasLimit.*=|gasLimit.*='
# Also search for hexutil.DecodeBig usage
rg -A 3 'hexutil\.DecodeBig'
# And check the specific function implementation
ast-grep --pattern 'func parseWithdrawalEventAttributes($$$) {
$$$
}'
Length of output: 6359
x/rollup/keeper/deposits.go (2)
34-34: LGTM: Key name updated for consistency
The change from KeyL1BlockInfo to L1BlockInfoKey aligns with the module's key naming conventions.
133-137: LGTM: Parameter retrieval properly implemented
The addition of parameter retrieval replaces hardcoded values with dynamic configuration, improving the module's flexibility. Error handling follows the established pattern.
go.mod (1)
36-36: LGTM! Dependencies look good.
The addition of the tidwall JSON manipulation packages is appropriate for handling the rollup module's genesis state. These are well-maintained packages with stable versions, and they're commonly used for efficient JSON operations.
Also applies to: 296-298
builder/builder_test.go (1)
368-371: LGTM! Well-structured test verification.
The added verification for fee withdrawal transaction results is comprehensive and aligns with the PR objectives.
x/rollup/keeper/msg_server.go (2)
70-70: Usage of parameterized GasLimit in event attributes
The GasLimit attribute is now correctly set using msg.GasLimit, allowing for dynamic configuration of gas limits in withdrawal events.
135-135: Parameterization of FeeWithdrawalGasLimit in event attributes
The GasLimit attribute is now set using params.FeeWithdrawalGasLimit, enhancing the module's configurability.
Adds a genesis state to x/rollup to allow module specific params to be configured.
natebeauregard
force-pushed
the
nkb.add-genesis-state
branch
from
4890c4d
to
f2b36b9
Compare
Closes #288
Adds a genesis state to x/rollup to allow module specific params to be configured.
The following module specific params were added to the
x/rollupgenesis state:A new message type for customizing the module params after genesis and a new query endpoint for getting the current module params will be added in a follow-up PR.
Summary by CodeRabbit
Release Notes
New Features
GenesisStatestructure for the rollup module, allowing for better initialization and management of module parameters.Bug Fixes
Documentation
Chores