Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add x/rollup genesis state #289

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Add x/rollup genesis state #289

merged 1 commit into from
Nov 7, 2024

Conversation

natebeauregard
Copy link
Collaborator

@natebeauregard natebeauregard commented Nov 5, 2024

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/rollup genesis state:

  • l1_fee_recipient: L1 address to forward the L2 fees to.
  • l1_cross_domain_messenger: L1 address of the cross-domain messenger contract.
  • min_fee_withdrawal_amount: Minimum amount of L2 fees that the FeeCollector account must have before they can be withdrawn.
  • fee_withdrawal_gas_limit: L1 gas limit for withdrawing fees to the L1 recipient address.

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

    • Introduced a new GenesisState structure for the rollup module, allowing for better initialization and management of module parameters.
    • Added parameter management capabilities, allowing dynamic configuration of withdrawal processes.
    • Enhanced testing functionality for builder and rollup modules, ensuring robust validation of transactions.
    • Added new constants and methods for improved parameter handling and genesis state validation.
  • Bug Fixes

    • Improved error handling in transaction processing and parameter retrieval, ensuring only valid transactions are processed.
  • Documentation

    • Updated comments and documentation to clarify the purpose and functionality of the rollup module and its components.
  • Chores

    • Removed unused imports and updated dependencies for better performance and maintainability.

Copy link
Contributor

coderabbitai bot commented Nov 5, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

The 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

File Change Summary
builder/builder.go Removed unused import math/big. Updated gas limit handling in parseWithdrawalEventAttributes function.
builder/builder_test.go Added JSON manipulation imports. Modified setupTestEnvironment for testing. Enhanced assertions in TestBuildRollupTxs.
cmd/monogen/testapp/app/app_config.go Added rolluptypes.ModuleName to genesisModuleOrder and permissions for the rollup module.
go.mod Added new dependencies for JSON manipulation (tidwall packages).
proto/rollup/v1/genesis.proto Introduced GenesisState message for rollup module.
proto/rollup/v1/rollup.proto Added Params message with fields for rollup configuration.
testutils/utils.go Removed hardcoded address comment in GenerateERC20DepositTx.
x/rollup/keeper/deposits.go Updated key for L1 block info. Improved error handling in deposit transaction processing.
x/rollup/keeper/genesis.go Introduced InitGenesis and ExportGenesis functions for rollup genesis state management.
x/rollup/keeper/keeper.go Added GetParams and SetParams methods. Updated key for retrieving L1 block info.
x/rollup/keeper/keeper_test.go Increased mock fee collector balance in tests.
x/rollup/keeper/msg_server.go Updated gas limit handling in InitiateWithdrawal and InitiateFeeWithdrawal methods.
x/rollup/keeper/msg_server_test.go Renamed constant for L1 block info. Added parameter setup in fee withdrawal tests.
x/rollup/module.go Updated comments and method signatures to reflect rollup module functionality. Enhanced genesis state handling.
x/rollup/types/genesis.go Introduced functions for creating and validating the genesis state.
x/rollup/types/keys.go Renamed constant KeyL1BlockInfo to L1BlockInfoKey and added ParamsKey.
x/rollup/types/params.go Added constants and functions for Ethereum parameters and validation logic.

Assessment against linked issues

Objective Addressed Explanation
Add a genesis state to x/rollup to configure module specific variables instead of using hardcoded values (288)
Include L1 recipient address for fee withdrawals in the genesis state (288)
Define minimum funds for FeeCollector before withdrawals in the genesis state (288)
Specify L1CrossDomainMessenger contract address in the genesis state (288)

Possibly related PRs

🐇 In the meadow, changes bloom,
A rollup's state dispels the gloom.
With gas limits now in sight,
Our code dances, oh so bright!
Parameters flow, no more hard-coded,
In this rabbit's warren, joy is loaded! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@natebeauregard natebeauregard force-pushed the nkb.add-genesis-state branch 2 times, most recently from c87b82d to cc58441 Compare November 5, 2024 18:35
@natebeauregard natebeauregard marked this pull request as ready for review November 5, 2024 18:44
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Add validation for required fields (l1_fee_recipient, l1_cross_domain_messenger) before setting params
  2. 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:

  1. 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"];
  1. 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:

  1. The Params message might need versioning support for future updates
  2. 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:

  1. Purpose and impact of each parameter
  2. Recommended values for different networks
  3. Examples of parameter configuration in genesis.json
x/rollup/keeper/keeper.go (2)

64-74: Consider handling nil paramsBz case

The implementation looks good, but consider adding a nil check for paramsBz to 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 hooks

Since these parameters (especially l1_fee_recipient and l1_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 address

While 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:

  1. Add more context to the error message for better debugging.
  2. 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 in GetParams

If 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 clarity

The error message could be improved by formatting both the balance and the minimum withdrawal amount consistently. Consider converting params.MinFeeWithdrawalAmount to a string or formatting feeCollectorBalance.Amount to match the data type for clearer communication.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 562d332 and cc58441.

⛔ Files ignored due to path filters (4)
  • cmd/monogen/testapp.zip is excluded by !**/*.zip
  • go.sum is excluded by !**/*.sum
  • x/rollup/types/genesis.pb.go is excluded by !**/*.pb.go
  • x/rollup/types/rollup.pb.go is 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 Params struct 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,000
  • fee_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:

  1. Using network-specific defaults
  2. 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:

  1. Are these values sufficient for typical L1 transactions?
  2. 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 parseWithdrawalEventAttributes using hexutil.DecodeBig is 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

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.

x/rollup/keeper/genesis.go Show resolved Hide resolved
x/rollup/types/params.go Outdated Show resolved Hide resolved
x/rollup/types/params.go Outdated Show resolved Hide resolved
x/rollup/keeper/keeper.go Show resolved Hide resolved
x/rollup/keeper/deposits.go Show resolved Hide resolved
Copy link
Collaborator

@joshklop joshklop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a few small suggestions.

x/rollup/types/genesis.go Outdated Show resolved Hide resolved
x/rollup/types/params.go Outdated Show resolved Hide resolved
x/rollup/types/params.go Outdated Show resolved Hide resolved
x/rollup/types/params.go Outdated Show resolved Hide resolved
x/rollup/keeper/genesis.go Show resolved Hide resolved
Adds a genesis state to x/rollup to allow module specific params to be
configured.
@natebeauregard natebeauregard merged commit a51efc6 into main Nov 7, 2024
16 checks passed
@natebeauregard natebeauregard deleted the nkb.add-genesis-state branch November 7, 2024 19:18
@coderabbitai coderabbitai bot mentioned this pull request Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add x/rollup genesis state
2 participants