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 query server #293

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Add x/rollup query server #293

merged 1 commit into from
Nov 7, 2024

Conversation

natebeauregard
Copy link
Collaborator

@natebeauregard natebeauregard commented Nov 6, 2024

Closes #291

Add a query server to x/rollup and expose endpoints for retrieving the following values from the store:

  • L1BlockInfo
  • Params

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new rollup module with enhanced configuration capabilities, including genesis state management and parameter validation.
    • Added gRPC query methods for retrieving Layer 1 block information and rollup parameters.
    • Enhanced test coverage for rollup functionality, ensuring robust validation of fee withdrawal transactions and parameter queries.
    • Implemented a new GenesisState message for managing the genesis state of the rollup module.
  • Bug Fixes

    • Improved error handling in various methods related to fee withdrawals and parameter retrieval.
  • Chores

    • Updated dependencies for better JSON handling and streamlined codebase.
    • Refined naming conventions for constants and methods to enhance clarity and maintainability.

Copy link
Contributor

coderabbitai bot commented Nov 6, 2024

Warning

Rate limit exceeded

@natebeauregard has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 9 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between f1e2225 and a97c121.

Walkthrough

The pull request introduces multiple changes across several files to enhance the rollup module's functionality. Key updates include the addition of a query server for retrieving L1 block information and parameters, modifications to gas limit handling in withdrawal processes, and the introduction of new protobuf definitions for the rollup module. The codebase sees a reduction in dependencies, improved test coverage, and a structured approach to managing genesis states and parameters. Overall, the changes aim to streamline interactions with the rollup module and enhance its configurability.

Changes

File Path Change Summary
builder/builder.go Removed math/big import; updated gasLimit handling in parseWithdrawalEventAttributes to use hexutil.DecodeBig.
builder/builder_test.go Enhanced test cases; added encoding/json import; modified setupTestEnvironment to set min_fee_withdrawal_amount to 0; activated fee withdrawal transaction result checks in TestBuildRollupTxs.
cmd/monogen/testapp/app/app_config.go Added rolluptypes.ModuleName to genesisModuleOrder; updated moduleAccPerms to include Minter and Burner permissions for the rollup module; modified appConfig to wrap rollup module configuration.
go.mod Added dependencies for JSON manipulation (sjson, gjson, match, pretty); modified replace directive for go-ethereum to a specific fork.
proto/rollup/v1/genesis.proto Introduced GenesisState message with a Params field; defined serialization options.
proto/rollup/v1/query.proto Added Query service with Params and L1BlockInfo RPC methods; defined request and response message types.
proto/rollup/v1/rollup.proto Introduced Params message with fields for fee management and cross-domain interactions.
testutils/utils.go Removed hardcoded address in GenerateERC20DepositTx; retained comment for future configuration.
x/rollup/keeper/deposits.go Removed setL1BlockInfo method; modified processL1UserDepositTxs to use dynamic parameters for L1CrossDomainMessenger.
x/rollup/keeper/genesis.go Introduced InitGenesis and ExportGenesis functions for managing rollup genesis state.
x/rollup/keeper/grpc_query.go Added gRPC query server methods for L1BlockInfo and Params.
x/rollup/keeper/grpc_query_test.go Introduced tests for querying parameters and L1 block information.
x/rollup/keeper/keeper.go Updated GetL1BlockInfo key; added methods for setting and getting L1 block info and parameters.
x/rollup/keeper/keeper_test.go Enhanced test suite setup; modified expected balance for fee collector.
x/rollup/keeper/msg_server.go Updated method names and gas limit handling; removed hardcoded constants in InitiateFeeWithdrawal.
x/rollup/keeper/msg_server_test.go Renamed constant for L1 block info; initialized parameters in tests.
x/rollup/module.go Enhanced module functionality and renamed implementations to clarify rollup-specific context; added InitGenesis method.
x/rollup/types/genesis.go Introduced functions for creating and validating genesis state for rollup.
x/rollup/types/keys.go Renamed KeyL1BlockInfo to L1BlockInfoKey; added ParamsKey constant.
x/rollup/types/params.go Defined constants and functions for Ethereum transaction parameters, including validation logic.
x/rollup/types/errors.go Updated error message for ErrL1BlockInfo; introduced new error ErrParams.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant QueryServer
    participant Keeper

    Client->>QueryServer: Request L1BlockInfo
    QueryServer->>Keeper: GetL1BlockInfo()
    Keeper-->>QueryServer: Return L1BlockInfo
    QueryServer-->>Client: Respond with L1BlockInfo
Loading
sequenceDiagram
    participant Client
    participant QueryServer
    participant Keeper

    Client->>QueryServer: Request Params
    QueryServer->>Keeper: GetParams()
    Keeper-->>QueryServer: Return Params
    QueryServer-->>Client: Respond with Params
Loading

Assessment against linked issues

Objective Addressed Explanation
Add a query server to x/rollup
Expose endpoints for retrieving L1BlockInfo
Expose endpoints for retrieving Params

Possibly related issues

Possibly related PRs

Poem

🐇 In the land of code where bunnies play,
New features hop in, brightening the day.
With queries for L1, and params in tow,
The rollup's now ready, watch it grow!
No more hardcoded, just flexible flair,
Hopping through changes, we dance in the air! 🌟


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.

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

🧹 Outside diff range and nitpick comments (25)
x/rollup/types/keys.go (1)

20-23: LGTM! Consider enhancing the documentation.

The store key constants follow a consistent naming pattern and align well with the query server implementation objectives. The snake_case values and clear comments make the purpose of each constant evident.

Consider adding a brief comment block above these constants to document their role in the query server implementation:

const (
+	// Store keys used by the query server to retrieve specific data:
+	// - L1BlockInfo: Information about L1 blocks
+	// - Params: Module parameters
+
	// L1BlockInfoKey is the store key for the L1BlockInfo
	L1BlockInfoKey = "l1_block_info"
	// ParamsKey is the store key for the x/rollup module parameters
	ParamsKey = "params"
)
x/rollup/types/genesis.go (4)

1-2: Consider adding common validation imports.

Genesis files typically benefit from imports like fmt for error formatting and validation utilities. Consider if you need these for more detailed error messages in ValidateGenesis.


3-8: Consider validating input params.

While the implementation is correct, consider validating the input params before creating the GenesisState to ensure data integrity at creation time.

 func NewGenesisState(params Params) *GenesisState {
+	if err := params.Validate(); err != nil {
+		panic(fmt.Sprintf("invalid params for genesis state: %s", err))
+	}
 	return &GenesisState{
 		Params: params,
 	}
 }

15-22: Consider additional genesis validations.

While params validation is good, genesis validation often requires additional checks. Consider:

  1. Validating any initial state or data structures
  2. Checking for required initial configurations
  3. Verifying consistency between different genesis state fields
 func ValidateGenesis(state GenesisState) error {
 	if err := state.Params.Validate(); err != nil {
 		return err
 	}
+	// Add additional validations as needed:
+	// - Verify initial state consistency
+	// - Check required configurations
+	// - Validate relationships between fields
 	return nil
 }

1-22: Consider documenting genesis constraints and invariants.

While the implementation provides basic genesis state management, it would be valuable to:

  1. Document the expected invariants and constraints for a valid genesis state
  2. Add comprehensive tests covering various genesis scenarios
  3. Consider adding helper methods for common genesis state modifications

This will help ensure the rollup module's genesis state remains consistent and well-maintained as the module evolves.

x/rollup/keeper/genesis.go (1)

11-11: Document reasoning for linter suppressions

Both functions have //nolint:gocritic // hugeParam directives. While it's common for genesis functions to handle large state objects, please add a more detailed comment explaining why this warning is suppressed and why it's safe to do so.

Consider adding more context:

-//nolint:gocritic // hugeParam
+//nolint:gocritic // hugeParam: genesis state is necessarily large as it contains the entire module state

Also applies to: 18-18

x/rollup/keeper/grpc_query_test.go (1)

8-16: Enhance test coverage for params query.

While the basic functionality is tested, consider adding:

  1. Negative test cases (e.g., invalid requests)
  2. Verification of individual parameter values
  3. Table-driven tests for different parameter scenarios
  4. Tests for parameter persistence across context updates

Example enhancement:

 func (s *KeeperTestSuite) TestParamsQuery() {
+    testCases := []struct {
+        name      string
+        params    types.Params
+        expectErr bool
+    }{
+        {
+            name:      "default params",
+            params:    types.DefaultParams(),
+            expectErr: false,
+        },
+        // Add more test cases
+    }
+
+    for _, tc := range testCases {
+        s.Run(tc.name, func() {
             params := types.DefaultParams()
             err := s.rollupKeeper.SetParams(sdk.UnwrapSDKContext(s.ctx), &params)
             s.Require().NoError(err)

             response, err := s.rollupKeeper.Params(s.ctx, &types.QueryParamsRequest{})
             s.Require().NoError(err)
             s.Require().Equal(&types.QueryParamsResponse{Params: params}, response)
+
+            // Verify persistence
+            ctx := sdk.UnwrapSDKContext(s.ctx).WithBlockHeight(s.ctx.BlockHeight() + 1)
+            response, err = s.rollupKeeper.Params(ctx, &types.QueryParamsRequest{})
+            s.Require().NoError(err)
+            s.Require().Equal(&types.QueryParamsResponse{Params: params}, response)
+        })
+    }
 }
x/rollup/keeper/grpc_query.go (3)

15-25: Consider enhancing error handling and request validation

The implementation is solid but could benefit from the following improvements:

  1. More descriptive error messages for better debugging
  2. Context timeout handling for long-running queries
  3. Additional request validation beyond the nil check

Consider applying these enhancements:

 func (k *Keeper) L1BlockInfo(ctx context.Context, req *types.QueryL1BlockInfoRequest) (*types.QueryL1BlockInfoResponse, error) {
 	if req == nil {
-		return nil, status.Error(codes.InvalidArgument, "invalid request")
+		return nil, status.Error(codes.InvalidArgument, "L1BlockInfo: request cannot be nil")
 	}
+
+	if ctx.Err() != nil {
+		return nil, status.Error(codes.DeadlineExceeded, "L1BlockInfo: context timeout or canceled")
+	}
+
 	l1BlockInfo, err := k.GetL1BlockInfo(sdk.UnwrapSDKContext(ctx))
 	if err != nil {
-		return nil, fmt.Errorf("get L1 block info: %w", err)
+		return nil, status.Errorf(codes.Internal, "L1BlockInfo: failed to retrieve block info: %v", err)
 	}
 	return &types.QueryL1BlockInfoResponse{L1BlockInfo: *l1BlockInfo}, nil
 }

27-37: Apply consistent error handling improvements

For consistency with the L1BlockInfo improvements, consider enhancing this method similarly:

 func (k *Keeper) Params(ctx context.Context, req *types.QueryParamsRequest) (*types.QueryParamsResponse, error) {
 	if req == nil {
-		return nil, status.Error(codes.InvalidArgument, "invalid request")
+		return nil, status.Error(codes.InvalidArgument, "Params: request cannot be nil")
 	}
+
+	if ctx.Err() != nil {
+		return nil, status.Error(codes.DeadlineExceeded, "Params: context timeout or canceled")
+	}
+
 	params, err := k.GetParams(sdk.UnwrapSDKContext(ctx))
 	if err != nil {
-		return nil, fmt.Errorf("get params: %w", err)
+		return nil, status.Errorf(codes.Internal, "Params: failed to retrieve params: %v", err)
 	}
 	return &types.QueryParamsResponse{Params: *params}, nil
 }

1-37: Well-structured implementation meeting PR objectives

The implementation successfully fulfills the PR objectives by providing query endpoints for both L1BlockInfo and Params. The code follows Cosmos SDK patterns and includes proper error handling. The structure is clean and maintainable.

Consider adding metrics or logging in the future to help monitor query performance and errors in production.

proto/rollup/v1/rollup.proto (1)

14-17: Consider adding field constraints for better validation.

For the numeric fields:

  • min_fee_withdrawal_amount: Consider adding a [(gogoproto.moretags)] = "validate:\"gte=0\"" to ensure non-negative values
  • fee_withdrawal_gas_limit: Consider adding a minimum value constraint to prevent unrealistic gas limits

Example:

  // Minimum amount of L2 fees that the FeeCollector account must have before they can be withdrawn.
- uint64 min_fee_withdrawal_amount = 3;
+ uint64 min_fee_withdrawal_amount = 3 [(gogoproto.moretags) = "validate:\"gte=0\""];
  // L1 gas limit for withdrawing fees to the L1 recipient address.
- uint64 fee_withdrawal_gas_limit = 4;
+ uint64 fee_withdrawal_gas_limit = 4 [(gogoproto.moretags) = "validate:\"gte=21000\""];
proto/rollup/v1/query.proto (1)

11-22: Enhance service and RPC documentation

While the basic documentation is present, consider adding more detailed documentation for each RPC method, including:

  • Expected behavior
  • Response field descriptions
  • Usage examples or common scenarios
  • Any rate limiting or pagination details (if applicable)

Example enhancement:

 // Query defines all query endpoints for the rollup module.
 service Query {
-  // Params returns all rollup module parameters.
+  // Params returns all rollup module parameters.
+  // This endpoint allows clients to retrieve the current configuration
+  // of the rollup module, including fee settings and cross-domain parameters.
   rpc Params(QueryParamsRequest) returns (QueryParamsResponse) {
     option (google.api.http).get = "/rollup/params";
   }

-  // L1BlockInfo returns the block info derived from L1.
+  // L1BlockInfo returns the block info derived from L1.
+  // This endpoint provides access to Layer 1 block information that has been
+  // processed by the rollup module, including block numbers, timestamps,
+  // and any relevant L1-specific metadata.
   rpc L1BlockInfo(QueryL1BlockInfoRequest) returns (QueryL1BlockInfoResponse) {
     option (google.api.http).get = "/rollup/l1_block_info";
   }
x/rollup/keeper/keeper_test.go (2)

35-41: Clarify the distinction between SetupTest and SetupSubTest.

Both methods currently perform identical operations. Consider:

  1. Adding comments to explain when each should be used
  2. Implementing cleanup in SetupTest for suite-level setup

Line range hint 43-58: Consider adding cleanup mechanism in setup.

The setup implementation looks good and follows Cosmos SDK patterns. However, consider adding cleanup of the store and resetting mock expectations between tests to prevent test pollution.

 func (s *KeeperTestSuite) setup() {
+	// Reset any existing mock expectations
+	if s.accountKeeper != nil {
+		s.accountKeeper.EXPECT().Reset()
+	}
+	if s.bankKeeper != nil {
+		s.bankKeeper.EXPECT().Reset()
+	}
+
 	storeKey := storetypes.NewKVStoreKey(types.StoreKey)
x/rollup/keeper/keeper.go (3)

Line range hint 53-62: Add nil check for non-existent L1BlockInfo.

The method should handle the case when L1BlockInfo doesn't exist in the store (when l1BlockInfoBz is nil).

 func (k *Keeper) GetL1BlockInfo(ctx sdk.Context) (*types.L1BlockInfo, error) {
   l1BlockInfoBz, err := k.storeService.OpenKVStore(ctx).Get([]byte(types.L1BlockInfoKey))
   if err != nil {
     return nil, fmt.Errorf("get l1 block info: %w", err)
   }
+  if l1BlockInfoBz == nil {
+    return nil, fmt.Errorf("l1 block info not found")
+  }
   var l1BlockInfo types.L1BlockInfo
   if err = l1BlockInfo.Unmarshal(l1BlockInfoBz); err != nil {
     return nil, fmt.Errorf("unmarshal l1 block info: %w", err)
   }
   return &l1BlockInfo, nil
 }

64-77: Maintain consistency in error messages.

The error messages use WrapError while GetL1BlockInfo uses fmt.Errorf. Consider using consistent error handling across both methods.

 func (k *Keeper) SetL1BlockInfo(ctx sdk.Context, info types.L1BlockInfo) error {
   infoBytes, err := info.Marshal()
   if err != nil {
-    return types.WrapError(err, "marshal L1 block info")
+    return fmt.Errorf("marshal l1 block info: %w", err)
   }
   if err = k.storeService.OpenKVStore(ctx).Set([]byte(types.L1BlockInfoKey), infoBytes); err != nil {
-    return types.WrapError(err, "set latest L1 block info")
+    return fmt.Errorf("set l1 block info: %w", err)
   }
   return nil
 }

91-101: Maintain consistency in parameter types.

SetParams accepts a pointer (*types.Params) while SetL1BlockInfo accepts a value (types.L1BlockInfo). Consider using consistent parameter types across similar methods.

-func (k *Keeper) SetParams(ctx sdk.Context, params *types.Params) error {
+func (k *Keeper) SetParams(ctx sdk.Context, params types.Params) error {
   paramsBz, err := params.Marshal()
   if err != nil {
     return fmt.Errorf("marshal params: %w", err)
   }
   err = k.storeService.OpenKVStore(ctx).Set([]byte(types.ParamsKey), paramsBz)
   if err != nil {
     return fmt.Errorf("set params: %w", err)
   }
   return nil
 }
cmd/monogen/testapp/app/app_config.go (1)

Line range hint 39-146: Overall integration looks good with some verification points.

The rollup module integration follows Cosmos SDK conventions and covers all necessary configuration points:

  1. Genesis initialization order
  2. Module account permissions
  3. Module configuration

For maintainability, consider:

  1. Documenting the reason for the module's position in the genesis order
  2. Adding comments explaining the required permissions
  3. Creating a separate configuration type if specific parameters are needed in the future
x/rollup/module.go (1)

112-117: Enhance error handling in ValidateGenesis.

Consider wrapping the validation error to provide more context about where the error occurred in the validation chain.

 var data types.GenesisState
 if err := cdc.UnmarshalJSON(bz, &data); err != nil {
   return fmt.Errorf("failed to unmarshal %s genesis state: %w", types.ModuleName, err)
 }

-return types.ValidateGenesis(data)
+if err := types.ValidateGenesis(data); err != nil {
+  return fmt.Errorf("invalid %s genesis state: %w", types.ModuleName, err)
+}
+return nil
x/rollup/keeper/msg_server_test.go (1)

130-130: LGTM! Consider enhancing test readability.

The key rename from KeyL1BlockInfo to L1BlockInfoKey aligns with standardized naming conventions. The test thoroughly verifies all L1 block info fields.

Consider extracting the L1 block info verification into a helper function to improve test readability:

func (s *KeeperTestSuite) verifyL1BlockInfo(expectedBlockInfo eth.BlockInfo) {
    l1BlockInfoBz := s.rollupStore.Get([]byte(types.L1BlockInfoKey))
    s.Require().NotNil(l1BlockInfoBz)

    l1BlockInfo := &types.L1BlockInfo{}
    err := l1BlockInfo.Unmarshal(l1BlockInfoBz)
    s.Require().NoError(err)
    s.Require().Equal(expectedBlockInfo.NumberU64(), l1BlockInfo.Number)
    s.Require().Equal(expectedBlockInfo.BaseFee().Bytes(), l1BlockInfo.BaseFee)
    s.Require().Equal(expectedBlockInfo.Time(), l1BlockInfo.Time)
    s.Require().Equal(expectedBlockInfo.Hash().Bytes(), l1BlockInfo.BlockHash)
}
builder/builder.go (4)

381-385: LGTM: Gas limit handling improvement

The change simplifies gas limit parsing by using hexutil.DecodeBig directly instead of intermediate conversions, making the code more efficient and maintainable.

Consider adding validation to ensure the gas limit is within acceptable bounds:

 gasLimit, err := hexutil.DecodeBig(attr.Value)
 if err != nil {
     return nil, fmt.Errorf("decode gas limit: %v", err)
 }
+if gasLimit.Sign() < 0 {
+    return nil, fmt.Errorf("gas limit must be non-negative")
+}
 params.GasLimit = gasLimit

Line range hint 64-77: Critical: Improve mempool error handling

The current implementation panics on mempool database errors, which is noted as a temporary solution. This could lead to service disruption in production.

Consider implementing proper error handling and recovery mechanisms:

  1. Return errors instead of panicking
  2. Implement a retry mechanism for transient failures
  3. Add monitoring and alerting for mempool operations
  4. Document the recovery procedure for database inconsistencies

Would you like me to help create a GitHub issue to track this improvement?


Line range hint 249-259: Enhance message type handling

The current implementation uses string-based type URLs for message identification. This approach could be fragile if type URLs change.

Consider creating constants or using the SDK's type registry for more type-safe message identification:

-userWithdrawalMsgType := cdctypes.MsgTypeURL(new(rolluptypes.MsgInitiateWithdrawal))
-feeWithdrawalMsgType := cdctypes.MsgTypeURL(new(rolluptypes.MsgInitiateFeeWithdrawal))
-if msg.TypeUrl == userWithdrawalMsgType || msg.TypeUrl == feeWithdrawalMsgType {
+const (
+    UserWithdrawalType = "/monomer.rollup.v1.MsgInitiateWithdrawal"
+    FeeWithdrawalType  = "/monomer.rollup.v1.MsgInitiateFeeWithdrawal"
+)
+
+if msg.TypeUrl == UserWithdrawalType || msg.TypeUrl == FeeWithdrawalType {

Line range hint 156-196: Consider atomic event publishing

The current implementation publishes events sequentially without a transaction boundary. A failure in publishing one event could leave the system in an inconsistent state where some events are published while others are not.

Consider these improvements:

  1. Implement a transaction-like boundary around event publishing
  2. Add compensation logic to handle partial failures
  3. Consider implementing an event buffer that can be retried
  4. Add monitoring for event publishing failures

Would you like me to propose a detailed design for implementing these improvements?

builder/builder_test.go (1)

422-428: Improve error handling and code formatting.

While the implementation is functionally correct, consider these improvements:

  1. Add more specific error context
  2. Fix indentation for better readability

Apply this diff:

-	appState := testapp.MakeGenesisAppState(t, app)
-
-	// Set min_fee_withdrawal_amount to 0 for testing purposes since fees will not be collected in the test environment.
-	rollupAppState, err := sjson.Set(string(appState[types.ModuleName]), "params.min_fee_withdrawal_amount", "0")
-	require.NoError(t, err)
-	appState[types.ModuleName] = json.RawMessage(rollupAppState)
+    appState := testapp.MakeGenesisAppState(t, app)
+
+    // Set min_fee_withdrawal_amount to 0 for testing purposes since fees will not be collected in the test environment.
+    rollupAppState, err := sjson.Set(string(appState[types.ModuleName]), "params.min_fee_withdrawal_amount", "0")
+    if err != nil {
+        t.Fatalf("failed to set min_fee_withdrawal_amount: %v", err)
+    }
+    appState[types.ModuleName] = json.RawMessage(rollupAppState)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

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

⛔ Files ignored due to path filters (5)
  • 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/query.pb.go is excluded by !**/*.pb.go
  • x/rollup/types/rollup.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (20)
  • 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/query.proto (1 hunks)
  • proto/rollup/v1/rollup.proto (1 hunks)
  • testutils/utils.go (0 hunks)
  • x/rollup/keeper/deposits.go (1 hunks)
  • x/rollup/keeper/genesis.go (1 hunks)
  • x/rollup/keeper/grpc_query.go (1 hunks)
  • x/rollup/keeper/grpc_query_test.go (1 hunks)
  • x/rollup/keeper/keeper.go (2 hunks)
  • x/rollup/keeper/keeper_test.go (2 hunks)
  • x/rollup/keeper/msg_server.go (4 hunks)
  • x/rollup/keeper/msg_server_test.go (2 hunks)
  • x/rollup/module.go (6 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 (30)
proto/rollup/v1/genesis.proto (2)

1-9: LGTM! File structure and imports are well-organized.

The file follows protobuf best practices with appropriate syntax, package naming, and necessary imports.


10-14: Verify genesis validation implementation.

The GenesisState message structure looks good with appropriate field options. However, since params is a required field, we should verify that proper validation is implemented in the genesis.go file.

Let's verify the genesis validation implementation:

✅ Verification successful

Genesis validation is properly implemented

The genesis validation is well-implemented with appropriate checks:

  1. The ValidateGenesis function in x/rollup/types/genesis.go validates the GenesisState by calling Validate() on its params field.
  2. The params validation in x/rollup/types/params.go performs proper validation of critical fields:
    • Validates L1FeeRecipient address format
    • Validates L1CrossDomainMessenger address format

The implementation aligns with the proto definition where params is marked as non-nullable and required.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for genesis validation implementation
# Expected: Find ValidateGenesis function that validates params

# Search for ValidateGenesis function
ast-grep --pattern 'func ValidateGenesis($_) {
  $$$
}'

# Search for genesis validation in test files
rg -g '*_test.go' -A 5 'TestValidateGenesis'

Length of output: 102


Script:

#!/bin/bash
# Let's try a broader search for genesis-related validation code

# Search for any genesis-related functions
rg -g '*.go' -A 10 'func.*[Gg]enesis'

# Search for any validation-related functions in x/rollup
fd -e go . | xargs rg -l 'func.*[Vv]alidate'

# Look for files that might contain genesis validation
fd -g '*genesis*.go'

Length of output: 22632


Script:

#!/bin/bash
# Let's check the params validation implementation since genesis validation depends on it

# Search for Params validation implementation
ast-grep --pattern 'func (p Params) Validate() error {
  $$$
}'

# Also check for any test coverage
rg -g '*_test.go' -A 5 'TestParams.*Validate'

Length of output: 113


Script:

#!/bin/bash
# Let's check the params validation implementation with a broader search

# Look for Validate method in params.go
rg -g 'params.go' -A 10 'func.*Validate'

# Check the actual implementation of ValidateGenesis
rg -g 'genesis.go' -A 5 'func.*ValidateGenesis'

# Look for any validation-related tests
rg -g '*_test.go' 'Test.*Validate'

Length of output: 1101

x/rollup/types/genesis.go (1)

10-13: Verify DefaultParams implementation.

The implementation looks good, but let's verify the DefaultParams() function to ensure it provides appropriate default values.

✅ Verification successful

DefaultParams implementation is well-defined with appropriate default values

The implementation is verified and provides reasonable defaults:

  • L1FeeRecipient: A dead address (0x000000...dEaD) for fee collection
  • L1CrossDomainMessenger: A specific contract address for cross-domain messaging
  • MinFeeWithdrawalAmount: 400,000 units
  • FeeWithdrawalGasLimit: 400,000 gas units

Each parameter has clear documentation in the Params struct and sensible default values that align with their intended purposes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify DefaultParams implementation
# Expected: Find the DefaultParams function and its default values

# Search for DefaultParams implementation
ast-grep --pattern 'func DefaultParams() Params {
  $$$
}'

# Also check for any tests of default params
rg -l "TestDefaultParams" 

Length of output: 335


Script:

#!/bin/bash
# Let's check the default values used in DefaultParams

# First, find the default constant declarations
rg "Default.*\s*=" x/rollup/types/params.go

# Then get the Params struct definition to understand the types
ast-grep --pattern 'type Params struct {
  $$$
}'

# Also check if there are any param tests
rg -A 10 "TestParams" x/rollup/types/params_test.go

Length of output: 1698

x/rollup/keeper/genesis.go (2)

1-8: LGTM: Package structure follows Cosmos SDK conventions

The package structure and imports are clean and follow standard Cosmos SDK module patterns.


10-15: 🛠️ Refactor suggestion

Consider following Cosmos SDK genesis patterns

Several improvements could enhance this implementation:

  1. Genesis functions in Cosmos SDK typically return errors instead of panicking. Consider changing the signature to:
-func (k *Keeper) InitGenesis(ctx sdk.Context, state types.GenesisState) {
+func (k *Keeper) InitGenesis(ctx sdk.Context, state types.GenesisState) error {
  1. Add basic validation of the genesis state before setting params.
  2. Document the panic behavior in the function comment if keeping it.

Let's verify this pattern in other Cosmos SDK modules:

x/rollup/keeper/grpc_query_test.go (1)

1-6: LGTM: Package and imports are well-structured.

The package name and imports are appropriate for testing the keeper's gRPC query functionality.

x/rollup/keeper/grpc_query.go (2)

1-11: LGTM: Clean and well-organized imports

The imports are comprehensive and include all necessary packages for gRPC server implementation and error handling.


13-13: LGTM: Proper interface implementation check

The compile-time interface check ensures the Keeper implements all methods defined in the QueryServer interface.

proto/rollup/v1/rollup.proto (1)

8-18: LGTM! Consider adding address validation.

The Params message is well-structured with clear documentation. However, since l1_fee_recipient and l1_cross_domain_messenger are Ethereum addresses, consider adding validation to ensure they follow the correct format.

Let's check if there are any address validation patterns in the codebase:

proto/rollup/v1/query.proto (3)

1-10: LGTM: File setup and imports are well-structured

The file follows protobuf best practices with appropriate syntax, package naming, and necessary imports for gRPC and REST gateway functionality.


24-31: LGTM: Params query messages are well-defined

The message definitions follow protobuf best practices:

  • Empty request message is appropriate for this use case
  • Non-nullable params field ensures response consistency
  • Field numbering and types are correct

33-40: LGTM: L1BlockInfo query messages are well-structured

The message definitions are consistent with the module's patterns and protobuf best practices.

Let's verify the L1BlockInfo type definition:

✅ Verification successful

L1BlockInfo type is properly defined and referenced

The verification confirms that the L1BlockInfo message type exists in proto/rollup/v1/rollup.proto with all necessary fields for representing L1 block information:

  • number: block number
  • time: block timestamp
  • base_fee: L1 base fee as big.Int
  • block_hash: block hash
  • sequence_number: L2 block sequence within epoch

The query messages correctly reference this well-structured type.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify L1BlockInfo type definition exists and is properly defined
# Expected: Find the L1BlockInfo message definition in rollup.proto

rg -A 5 "message L1BlockInfo" proto/rollup/v1/rollup.proto

Length of output: 306

x/rollup/types/params.go (2)

1-8: LGTM! Clean imports.

The package declaration and imports are minimal and appropriate for the functionality.


11-16: Review hardcoded values and improve documentation.

A few concerns about the default values:

  1. Using a devnet address (DefaultL1CrossDomainMessenger) as a default in production code could be risky.
  2. The gas limits (400,000) lack documentation explaining their rationale.
  3. The dead address usage for DefaultL1FeeRecipient should be documented.

Let's check if these values are consistent with other modules:

Consider:

  1. Adding detailed comments explaining the rationale behind these values
  2. Making the L1CrossDomainMessenger address network-specific
  3. Using named constants for the gas values to improve maintainability
 const (
+        // DefaultL1FeeRecipient is set to a dead address to ensure fees are effectively burned
        DefaultL1FeeRecipient string = "0x000000000000000000000000000000000000dEaD"
-        // DefaultL1CrossDomainMessenger uses the devnet address of the L1 cross domain messenger contract as the default value.
+        // DefaultL1CrossDomainMessenger represents the L1 cross domain messenger contract address.
+        // TODO: This should be network-specific and not use a devnet address
        DefaultL1CrossDomainMessenger string = "0x9A9f2CCfdE556A7E9Ff0848998Aa4a0CFD8863AE"
+        // DefaultGasLimit represents the standard gas limit for fee-related operations
+        DefaultGasLimit uint64 = 400_000
-        DefaultMinFeeWithdrawalAmount uint64 = 400_000
-        DefaultFeeWithdrawalGasLimit  uint64 = 400_000
+        DefaultMinFeeWithdrawalAmount uint64 = DefaultGasLimit
+        DefaultFeeWithdrawalGasLimit  uint64 = DefaultGasLimit
 )
x/rollup/keeper/keeper_test.go (1)

75-75: Consider extracting the magic number into a constant.

The hardcoded balance of 1,000,000 WEI would be more maintainable as a named constant.

+const (
+	// DefaultMockFeeCollectorBalance represents the default balance for fee collector in tests
+	DefaultMockFeeCollectorBalance = 1_000_000
+)
+
 func (s *KeeperTestSuite) mockFeeCollector() {
-	s.bankKeeper.EXPECT().GetBalance(s.ctx, mockFeeCollectorAddress, types.WEI).Return(sdk.NewCoin(types.WEI, math.NewInt(1_000_000))).AnyTimes()
+	s.bankKeeper.EXPECT().GetBalance(s.ctx, mockFeeCollectorAddress, types.WEI).Return(sdk.NewCoin(types.WEI, math.NewInt(DefaultMockFeeCollectorBalance))).AnyTimes()

Additionally, verify that this increased balance aligns with the test requirements across the module.

x/rollup/keeper/keeper.go (1)

Line range hint 53-101: Implementation aligns well with PR objectives.

The keeper methods provide a solid foundation for the query server implementation, offering all necessary functionality to retrieve and store both L1BlockInfo and Params. The implementation follows consistent patterns and includes proper error handling.

cmd/monogen/testapp/app/app_config.go (4)

Line range hint 18-19: LGTM! Import statements are correctly organized.

The new imports for the rollup module are properly placed and follow the standard Go import grouping convention.


Line range hint 143-146: Verify if the rollup module requires specific configuration parameters.

The module is configured with an empty configuration struct. Let's verify if any specific parameters should be set here.

#!/bin/bash
# Description: Check for configuration parameters in the rollup module

# Search for configuration parameters in proto files
fd -e proto | xargs rg -A 10 "message.*Module.*{|message.*Params.*{"

# Search for parameter usage in the module
rg -A 5 "GetParams|SetParams"

39-39: Verify rollup module initialization dependencies.

The placement after core Cosmos SDK modules looks correct. However, we should verify if the rollup module has any dependencies that require specific initialization ordering.

✅ Verification successful

Let me gather more specific information about the rollup module's initialization and dependencies.


Based on the gathered information, I can now generate the final response:

Module initialization order for rollup module is correctly placed

The rollup module's dependencies and initialization are safe with the current placement:

  • InitGenesis is simple and only sets module parameters
  • While the module depends on bank and auth modules (for keeper functionality), these are core Cosmos SDK modules that are already initialized before the rollup module
  • No complex initialization dependencies or ordering requirements are found in the module's implementation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential module dependencies in the rollup module's InitGenesis function
# and any references to other modules' keeper instances.

# Search for keeper dependencies in rollup module
rg -A 5 "type Keeper struct" 

# Search for InitGenesis implementation to check module dependencies
ast-grep --pattern 'func (k Keeper) InitGenesis($ctx, $state) $ret {
  $$$
}'

Length of output: 814


Script:

#!/bin/bash
# Search for InitGenesis implementation in rollup module specifically
rg -g 'x/rollup/**' -A 10 'InitGenesis'

# Check for any explicit module dependencies or ordering requirements in rollup module docs
fd -g 'x/rollup/**/*.md' -x cat {}

# Look for any bank or account keeper usage in rollup module
rg -g 'x/rollup/**' -A 3 'bankkeeper|accountkeeper'

Length of output: 7494


Line range hint 71-71: Verify the necessity of Minter and Burner permissions.

The rollup module has been granted both Minter and Burner permissions. Let's verify that these permissions are actually required by the module's operations.

✅ Verification successful

Let me gather more specific information about minting and burning operations in the rollup module.


Minter and Burner permissions are necessary for the rollup module's core functionality

Based on the code analysis, the Minter and Burner permissions are actively used and required:

  • Minting is used for:

    • Processing ETH deposits (MintCoins in keeper/deposits.go)
    • Handling ERC-20 token deposits
  • Burning is used for:

    • Processing fee withdrawals (BurnCoins in keeper/msg_server.go)
    • Handling general token withdrawals (BurnCoins in keeper/withdrawals.go)

The permissions align with the module's core cross-chain token bridging functionality, where tokens need to be minted when bridged in and burned when bridged out.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for minting and burning operations in the rollup module

# Search for minting operations
rg -A 5 "MintCoins|mint" 

# Search for burning operations
rg -A 5 "BurnCoins|burn"

Length of output: 4404


Script:

#!/bin/bash
# Search for minting and burning operations specifically in the rollup module
rg -A 5 "MintCoins|BurnCoins|Mint\(|Burn\(" x/rollup/

# Check for bank keeper usage in rollup keeper
ast-grep --pattern 'bankKeeper interface {
  $$$
}'

# Check rollup keeper initialization and bank keeper references
rg -A 5 "bank.Keeper" x/rollup/

Length of output: 9207

x/rollup/module.go (2)

170-174: LGTM! Well-implemented service registration.

The implementation correctly registers both message and query servers, which aligns with the PR objective of implementing a query server for the x/rollup module to expose L1BlockInfo and Params endpoints.


173-174: Verify query server implementation in keeper.

The query server registration looks good, but let's verify that the keeper properly implements the required query endpoints for L1BlockInfo and Params as specified in the PR objectives.

✅ Verification successful

The verification results show that both required query endpoints (L1BlockInfo and Params) are properly implemented in the keeper's grpc_query.go file. Each implementation includes proper nil request checks and calls to the corresponding getter methods. Let me verify the interface implementation declaration with one more check:


Query server implementation verified successfully

The keeper correctly implements the QueryServer interface with both required endpoints:

  • L1BlockInfo query implementation with proper request validation
  • Params query implementation with proper request validation
  • Explicit interface implementation declaration through var _ types.QueryServer = (*Keeper)(nil)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the keeper implements the required query endpoints

# Test: Check if the keeper implements QueryServer interface
ast-grep --pattern 'type Keeper struct {
  $$$
}

var _ types.QueryServer = (*Keeper)(nil)'

# Test: Check for L1BlockInfo and Params query implementations
rg -A 5 'func \(k \*Keeper\) (L1BlockInfo|Params)\('

Length of output: 1086


Script:

#!/bin/bash
# Check for QueryServer interface implementation declaration
rg "var _ types.QueryServer = \(\*Keeper\)\(nil\)"

Length of output: 123

x/rollup/keeper/deposits.go (1)

Line range hint 4-136: Verify L1 block info storage after removal of setL1BlockInfo.

The removal of setL1BlockInfo method raises concerns about L1 block info persistence. While processL1AttributesTx still derives the block info, it's not clear where this information is being stored for future use.

Let's verify if L1 block info storage is handled elsewhere:

go.mod (2)

36-36: LGTM! JSON handling dependencies are properly configured.

The addition of the tidwall packages (sjson, gjson, match, pretty) with pinned versions is appropriate for JSON manipulation in the query server implementation. These are well-maintained libraries with good performance characteristics.

Also applies to: 296-298


Line range hint 364-364: LGTM! Ethereum fork replacement is properly configured.

The replacement of go-ethereum with a specific commit of joshklop/op-geth is appropriate for optimism-specific modifications.

builder/builder_test.go (2)

5-5: LGTM!

The addition of the encoding/json import is necessary for JSON manipulation in the test setup.


368-371: LGTM!

The fee withdrawal transaction validation is well-implemented with proper error handling and assertions.

x/rollup/keeper/msg_server.go (3)

30-30: Method SetL1BlockInfo visibility change to public

The method SetL1BlockInfo is now public, allowing it to be accessed from outside the keeper package. This is appropriate if external packages or modules need to set L1 block information.


70-70: Correct gas limit encoding in withdrawal events

The gas limit is now properly encoded using hexutil.EncodeBig(new(big.Int).SetBytes(msg.GasLimit)), ensuring accurate representation of the gas limit in the emitted events.


135-135: Using parameterized FeeWithdrawalGasLimit in events

The gas limit for fee withdrawal is now retrieved from parameters using params.FeeWithdrawalGasLimit, enhancing configurability and allowing the gas limit to be adjusted without code changes.

x/rollup/keeper/genesis.go Outdated Show resolved Hide resolved
x/rollup/keeper/grpc_query_test.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 Outdated Show resolved Hide resolved
x/rollup/module.go Outdated Show resolved Hide resolved
x/rollup/module.go Outdated Show resolved Hide resolved
x/rollup/keeper/msg_server_test.go Outdated Show resolved Hide resolved
x/rollup/keeper/deposits.go Outdated Show resolved Hide resolved
x/rollup/keeper/msg_server.go Outdated Show resolved Hide resolved
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: 3

🧹 Outside diff range and nitpick comments (1)
x/rollup/keeper/deposits.go (1)

124-124: Ensure safe address conversion for aliasedL1CrossDomainMessengerAddress

Consider handling potential issues when converting params.L1CrossDomainMessenger to an address. This can prevent unexpected behavior if the address is invalid.

Apply this diff to encapsulate the address conversion with error handling:

-aliasedL1CrossDomainMessengerAddress := crossdomain.ApplyL1ToL2Alias(common.HexToAddress(params.L1CrossDomainMessenger))
+originalAddress := common.HexToAddress(params.L1CrossDomainMessenger)
+aliasedL1CrossDomainMessengerAddress := crossdomain.ApplyL1ToL2Alias(originalAddress)

Ensure that originalAddress is valid before applying the alias.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b87726a and f1e2225.

⛔ Files ignored due to path filters (1)
  • x/rollup/types/query.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (2)
  • x/rollup/keeper/deposits.go (1 hunks)
  • x/rollup/types/errors.go (1 hunks)

x/rollup/types/errors.go Show resolved Hide resolved
x/rollup/types/errors.go Show resolved Hide resolved
x/rollup/keeper/deposits.go Outdated Show resolved Hide resolved
proto/rollup/v1/query.proto Show resolved Hide resolved
x/rollup/keeper/deposits.go Show resolved Hide resolved
@natebeauregard natebeauregard merged commit eef648c into main Nov 7, 2024
16 checks passed
@natebeauregard natebeauregard deleted the nkb.add-query-server branch November 7, 2024 19:41
@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 query server
2 participants