Add x/rollup query server #293
Conversation
|
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 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. WalkthroughThe 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
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
sequenceDiagram
participant Client
participant QueryServer
participant Keeper
Client->>QueryServer: Request Params
QueryServer->>Keeper: GetParams()
Keeper-->>QueryServer: Return Params
QueryServer-->>Client: Respond with Params
Assessment against linked issues
Possibly related issues
Possibly related PRs
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
fmtfor error formatting and validation utilities. Consider if you need these for more detailed error messages inValidateGenesis.
3-8: Consider validating input params.While the implementation is correct, consider validating the input
paramsbefore 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:
- Validating any initial state or data structures
- Checking for required initial configurations
- 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:
- Document the expected invariants and constraints for a valid genesis state
- Add comprehensive tests covering various genesis scenarios
- 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 suppressionsBoth functions have
//nolint:gocritic // hugeParamdirectives. 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 stateAlso 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:
- Negative test cases (e.g., invalid requests)
- Verification of individual parameter values
- Table-driven tests for different parameter scenarios
- 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), ¶ms) 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 validationThe implementation is solid but could benefit from the following improvements:
- More descriptive error messages for better debugging
- Context timeout handling for long-running queries
- 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 improvementsFor 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 objectivesThe 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 valuesfee_withdrawal_gas_limit: Consider adding a minimum value constraint to prevent unrealistic gas limitsExample:
// 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 documentationWhile 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:
- Adding comments to explain when each should be used
- 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
l1BlockInfoBzis 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
WrapErrorwhileGetL1BlockInfousesfmt.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.
SetParamsaccepts a pointer (*types.Params) whileSetL1BlockInfoaccepts 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:
- Genesis initialization order
- Module account permissions
- Module configuration
For maintainability, consider:
- Documenting the reason for the module's position in the genesis order
- Adding comments explaining the required permissions
- 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 nilx/rollup/keeper/msg_server_test.go (1)
130-130: LGTM! Consider enhancing test readability.The key rename from
KeyL1BlockInfotoL1BlockInfoKeyaligns 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 improvementThe change simplifies gas limit parsing by using
hexutil.DecodeBigdirectly 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 handlingThe 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:
- Return errors instead of panicking
- Implement a retry mechanism for transient failures
- Add monitoring and alerting for mempool operations
- 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 handlingThe 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 publishingThe 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:
- Implement a transaction-like boundary around event publishing
- Add compensation logic to handle partial failures
- Consider implementing an event buffer that can be retried
- 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:
- Add more specific error context
- 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
⛔ Files ignored due to path filters (5)
cmd/monogen/testapp.zipis excluded by!**/*.zipgo.sumis excluded by!**/*.sumx/rollup/types/genesis.pb.gois excluded by!**/*.pb.gox/rollup/types/query.pb.gois excluded by!**/*.pb.gox/rollup/types/rollup.pb.gois 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:
- The
ValidateGenesisfunction inx/rollup/types/genesis.govalidates the GenesisState by callingValidate()on its params field. - The params validation in
x/rollup/types/params.goperforms 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:
- 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 {- Add basic validation of the genesis state before setting params.
- 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:
- Using a devnet address (
DefaultL1CrossDomainMessenger) as a default in production code could be risky. - The gas limits (400,000) lack documentation explaining their rationale.
- The dead address usage for
DefaultL1FeeRecipientshould be documented.
Let's check if these values are consistent with other modules:
Consider:
- Adding detailed comments explaining the rationale behind these values
- Making the L1CrossDomainMessenger address network-specific
- 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 (
MintCoinsinkeeper/deposits.go) - Handling ERC-20 token deposits
- Processing ETH deposits (
-
Burning is used for:
- Processing fee withdrawals (
BurnCoinsinkeeper/msg_server.go) - Handling general token withdrawals (
BurnCoinsinkeeper/withdrawals.go)
- Processing fee withdrawals (
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.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
x/rollup/keeper/deposits.go (1)
124-124: Ensure safe address conversion foraliasedL1CrossDomainMessengerAddressConsider handling potential issues when converting
params.L1CrossDomainMessengerto 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
originalAddressis valid before applying the alias.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/rollup/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
x/rollup/keeper/deposits.go(1 hunks)x/rollup/types/errors.go(1 hunks)
natebeauregard
force-pushed
the
nkb.add-query-server
branch
from
f1e2225
to
d523c47
Compare
natebeauregard
force-pushed
the
nkb.add-query-server
branch
from
d523c47
to
a97c121
Compare
Closes #291
Add a query server to x/rollup and expose endpoints for retrieving the following values from the store:
Summary by CodeRabbit
Release Notes
New Features
GenesisStatemessage for managing the genesis state of the rollup module.Bug Fixes
Chores