Create a new x/rollup message type to withdraw collected fees to L1 #287
Conversation
WalkthroughThe pull request introduces significant changes to the handling of withdrawal messages in the rollup module. It adds a new message type for initiating fee withdrawals from a fee collector account to a Layer 1 recipient. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FeeCollector
participant RollupModule
participant L1
User->>FeeCollector: InitiateFeeWithdrawal
FeeCollector->>RollupModule: Check balance
RollupModule->>RollupModule: Validate minimum amount
RollupModule->>L1: Transfer fees
L1-->>RollupModule: Confirm transfer
RollupModule-->>FeeCollector: Update status
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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 (
|
natebeauregard
changed the title
![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 (13)
x/rollup/types/expected_keepers.go (1)
22-22: LGTM! Consider documenting module address usage.The shift to
GetModuleAddressaligns well with the permissionless fee withdrawal design. The removal of account management methods indicates a more focused interface.Consider adding module address constants (e.g.,
FeeCollector, withdrawal target) in a separate constants file within thex/rollupmodule. This would centralize the module naming and make it easier to track module address usage throughout the codebase.x/rollup/types/errors.go (1)
20-20: Consider enhancing the error description for better debugging.The error registration follows the established pattern and is correctly implemented. However, consider making the error message more descriptive to help with debugging, such as including potential failure conditions (e.g., "failed to initiate fee withdrawal: insufficient balance or invalid target address").
- ErrInitiateFeeWithdrawal = registerErr("failed to initiate fee withdrawal") + ErrInitiateFeeWithdrawal = registerErr("failed to initiate fee withdrawal: insufficient balance or invalid target address")Also, consider adding a comment above the error to document when this error is thrown:
// ErrInitiateFeeWithdrawal is returned when a fee withdrawal attempt fails, // typically due to insufficient balance in the FeeCollector account or // when the withdrawal amount is below the minimum threshold.x/rollup/keeper/keeper.go (1)
15-19: Consider consistent field naming.The field naming is correct, but for consistency with other keeper fields, consider using
accountKeeperinstead ofaccountkeeperto match the camelCase pattern used inbankkeeper.- accountkeeper types.AccountKeeper + accountKeeper types.AccountKeeperproto/rollup/v1/tx.proto (1)
59-65: Enhance message documentation with implementation details.While the message definition is correct, the documentation should be expanded to include:
- The fact that this is a permissionless operation
- Reference to the hardcoded minimum withdrawal amount requirement
- Information about the predefined L1 recipient address
Consider updating the documentation like this:
-// MsgInitiateFeeWithdrawal defines the message for initiating an L2 fee withdrawal to the L1 fee recipient address. +// MsgInitiateFeeWithdrawal defines a permissionless message for initiating an L2 fee withdrawal. +// The withdrawal will only succeed if the FeeCollector balance exceeds a minimum threshold. +// Fees are sent to a predefined L1 recipient address configured in the x/rollup module. message MsgInitiateFeeWithdrawal {x/rollup/keeper/keeper_test.go (2)
41-41: Consider resetting mock expectations between tests.While the mock initialization is correct, consider adding
s.accountKeeper.EXPECT().Reset()at the start ofSetupSubTestto ensure clean test isolation. This prevents expectations from previous subtests affecting current ones.Also applies to: 47-47
55-56: Consider using specific mock expectations instead of AnyTimes().Using
AnyTimes()in mock expectations can mask issues by being too permissive. Consider:
- Using specific times (e.g.,
Times(1)) to verify exact call counts- Verifying specific coin amounts instead of
gomock.Any()-s.bankKeeper.EXPECT().SendCoinsFromAccountToModule(s.ctx, gomock.Any(), types.ModuleName, gomock.Any()).Return(nil).AnyTimes() +s.bankKeeper.EXPECT().SendCoinsFromAccountToModule(s.ctx, gomock.Any(), types.ModuleName, gomock.Any()).Return(nil).Times(1)Also applies to: 60-61
x/rollup/module.go (1)
Line range hint
58-63: Consider implementing genesis state handling.The PR objectives mention that minimum withdrawal amount and L1 recipient address will be configurable via genesis state. However, the current implementation doesn't include genesis state handling in the module. Consider implementing:
- Genesis state types in
types/genesis.go- Genesis state validation in
ValidateGenesis- Default genesis state in
DefaultGenesis- Genesis state export in
ExportGenesisx/rollup/testutil/expected_keepers_mocks.go (1)
150-161: LGTM with minor formatting observation.The
GetModuleAddressmock implementation is correct and aligns with the fee withdrawal requirements. However, there appear to be gaps in the line numbering (152 and 160 are missing), which might indicate formatting inconsistencies in the mock generation process.Consider regenerating the mocks to ensure consistent line numbering, though this doesn't affect functionality.
x/rollup/keeper/msg_server_test.go (1)
214-217: Consider adding more edge cases to the test suite.To enhance test coverage, consider adding the following test cases:
- Invalid sender address validation
- Verification of minimum withdrawal threshold
- Validation of the L1 target address format
- Test cases with different balance amounts near the threshold
Example test case structure:
"invalid sender address": { setupMocks: func() { // existing mocks }, sender: "invalid_address", shouldError: true, }, "balance exactly at minimum threshold": { setupMocks: func() { s.bankKeeper.EXPECT().GetBalance(s.ctx, gomock.Any(), types.WEI). Return(sdk.NewCoin(types.WEI, types.MinimumFeeWithdrawalAmount)) }, shouldError: false, },x/rollup/keeper/deposits.go (1)
267-267: Improved hex encoding of amounts usinghexutil.EncodeBigThe switch to
hexutil.EncodeBigfor encoding amounts in event attributes is a good improvement as it:
- Preserves leading zeros and sign information
- Aligns better with Ethereum's encoding practices
- Provides more consistent hex representation
For consistency, consider applying the same encoding pattern to the
mintERC20function's event attributes. Here's the suggested change:mintEvent := sdk.NewEvent( types.EventTypeMintERC20, sdk.NewAttribute(types.AttributeKeyL1DepositTxType, types.L1UserDepositTxType), sdk.NewAttribute(types.AttributeKeyToCosmosAddress, userAddr), sdk.NewAttribute(types.AttributeKeyERC20Address, erc20addr), - sdk.NewAttribute(types.AttributeKeyValue, hexutil.Encode(amount.BigInt().Bytes())), + sdk.NewAttribute(types.AttributeKeyValue, hexutil.EncodeBig(amount.BigInt())), )Also applies to: 269-269
x/rollup/keeper/msg_server.go (2)
96-96: Address the TODO: Make Parameters ConfigurableThe
minWithdrawalAmountandl1recipientAddrare currently hardcoded. Since the establishment of a genesis state forx/rollupis planned, consider making these parameters configurable now to enhance flexibility and ease future modifications.Would you like assistance in implementing the configuration for these parameters?
98-99: Clarify Units forminWithdrawalAmountThe
minWithdrawalAmountis set to100. If this value represents WEI, it is a minimal amount. Consider clarifying the units (e.g., WEI, GWEI, ETH) to ensure the minimum withdrawal threshold is set appropriately.builder/builder.go (1)
366-372: Resolve TODO: Use updated Cosmos to Ethereum address conversionThere's a TODO comment indicating the need to use an updated method for converting a Cosmos address to an Ethereum address. Addressing this is important for accurate sender address mapping.
Consider implementing the updated conversion method or referencing a utility function if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/rollup/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
bindings/L2ToL1MessagePasserExecuter.go(2 hunks)builder/builder.go(2 hunks)builder/builder_test.go(5 hunks)e2e/stack_test.go(1 hunks)evm/executer_test.go(2 hunks)proto/rollup/v1/tx.proto(2 hunks)x/rollup/keeper/deposits.go(1 hunks)x/rollup/keeper/keeper.go(1 hunks)x/rollup/keeper/keeper_test.go(3 hunks)x/rollup/keeper/msg_server.go(3 hunks)x/rollup/keeper/msg_server_test.go(4 hunks)x/rollup/module.go(3 hunks)x/rollup/tests/integration/rollup_test.go(8 hunks)x/rollup/testutil/expected_keepers_mocks.go(3 hunks)x/rollup/types/errors.go(1 hunks)x/rollup/types/events.go(1 hunks)x/rollup/types/expected_keepers.go(1 hunks)
🧰 Additional context used
🪛 Gitleaks
x/rollup/types/events.go
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (33)
x/rollup/types/expected_keepers.go (1)
13-17: LGTM! Verify module-to-module transfer usage.
The addition of SendCoinsFromModuleToModule is well-placed and follows cosmos-sdk conventions. This method will be crucial for fee withdrawal operations.
Let's verify the intended usage of this method:
✅ Verification successful
Verified: SendCoinsFromModuleToModule is correctly used for fee withdrawal operations
The method is properly used in the fee withdrawal flow:
- In
msg_server.go, it transfers fees from FeeCollector to Rollup module before burning - Integration tests verify the complete flow with proper module setup
- Test mocks confirm the expected interaction patterns
- Module permissions are correctly configured in app configs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new method is used for fee withdrawal operations
# Expected: Usage between FeeCollector module and other modules
# Search for direct usage of SendCoinsFromModuleToModule
rg -l "SendCoinsFromModuleToModule"
# Search for FeeCollector module references to understand the context
rg "FeeCollector" -A 5 -B 5
Length of output: 12397
x/rollup/types/events.go (1)
6-6: LGTM! The new attribute key follows conventions.
The new constant AttributeKeyL2FeeWithdrawalTx is well-placed and follows the established naming patterns for event attributes.
Let's verify the usage of this new attribute key:
✅ Verification successful
✅ Attribute key is properly integrated and used
The new AttributeKeyL2FeeWithdrawalTx constant is correctly used in msg_server.go for emitting events during fee withdrawal operations, specifically in the EventTypeBurnETH event alongside other relevant attributes like the source address and withdrawal value.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new attribute key is used in the fee withdrawal implementation
# Expected: Usage in keeper/msg_server.go for emitting events during fee withdrawals
# Search for direct usage of the constant
rg -l "AttributeKeyL2FeeWithdrawalTx"
# Search for the string value in case it's used directly
rg -l "l2_fee_withdrawal_tx"
Length of output: 146
Script:
#!/bin/bash
# Let's check how the attribute key is used in msg_server.go
rg -B 5 -A 5 "AttributeKeyL2FeeWithdrawalTx" x/rollup/keeper/msg_server.go
Length of output: 471
🧰 Tools
🪛 Gitleaks
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
x/rollup/keeper/keeper.go (1)
27-34: LGTM: AccountKeeper integration looks good.
The addition of accountKeeper parameter and its initialization follows the established pattern.
bindings/L2ToL1MessagePasserExecuter.go (2)
8-8: LGTM: Import aligns with PR objectives.
The addition of the crossdomain package from Optimism aligns with the goal of replicating OPStack's FeeVault functionality.
Line range hint 37-51: LGTM: Well-structured refactoring that aligns with objectives.
The refactoring to use crossdomain.Withdrawal improves code organization and type safety while supporting the PR's objectives:
- Encapsulates withdrawal parameters in a single struct
- Enables permissionless execution through structured parameter passing
- Reuses shared withdrawal code structure
Let's verify the struct definition to ensure all required fields are present:
✅ Verification successful
Based on the search results, I can see that the crossdomain.Withdrawal struct is imported from an external package github.com/ethereum-optimism/optimism/op-chain-ops/crossdomain. This is a well-known and trusted dependency from the Optimism project, which provides the standard withdrawal functionality for OP Stack chains.
The refactoring is well-structured and secure.
- The code correctly uses the standard Optimism crossdomain withdrawal struct
- The implementation follows the OP Stack withdrawal pattern
- All required fields are handled through the external dependency
- The integration is consistent with the documented withdrawal flow in the project's docs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Withdrawal struct definition
# Expected: Should show the struct definition with all required fields
rg -A 10 "type Withdrawal struct"
Length of output: 35
Script:
#!/bin/bash
# Let's try searching in different ways to locate the Withdrawal struct definition
# Search for files containing "crossdomain" in their path
fd crossdomain
# Search for Withdrawal-related code patterns
ast-grep --pattern 'type Withdrawal struct {
$$$
}'
# Broader search for Withdrawal-related code
rg "Withdrawal" -A 5
Length of output: 86618
Script:
#!/bin/bash
# Let's search for the crossdomain.Withdrawal struct definition in Go files
rg -g "*.go" "type Withdrawal struct" -A 10
# Also search for any imports of crossdomain package to locate its definition
rg -g "*.go" "\".*crossdomain\""
Length of output: 626
proto/rollup/v1/tx.proto (2)
20-22: LGTM! Well-documented RPC method addition.
The new InitiateFeeWithdrawal RPC method is properly defined and follows the established pattern of the existing withdrawal method.
59-68: Verify message usage in the codebase.
Let's ensure the message types are being used correctly in the implementation.
✅ Verification successful
Let me gather more information to verify if there's a proper message handler implementation in the keeper.
Let me check for the actual message handler implementation since we only found tests but no keeper implementation.
Message implementation is complete and properly integrated
The message types are being used correctly in the codebase with:
- Full keeper implementation in
msg_server.gowith proper fee withdrawal logic - Message registration in the module setup
- Test coverage in
msg_server_test.go - Generated gRPC service definitions in
tx.pb.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of InitiateFeeWithdrawal message types
# Test 1: Check for proper message handling in the keeper
echo "Checking message server implementation..."
rg -A 5 "func.*InitiateFeeWithdrawal.*MsgInitiateFeeWithdrawal.*MsgInitiateFeeWithdrawalResponse"
# Test 2: Check for proper message registration
echo "Checking message registration..."
rg "RegisterMsgServer|RegisterService.*InitiateFeeWithdrawal"
Length of output: 1749
Script:
#!/bin/bash
# Check for the keeper implementation of InitiateFeeWithdrawal
echo "Checking keeper implementation..."
rg -A 10 "func.*InitiateFeeWithdrawal.*context.Context.*MsgInitiateFeeWithdrawal" x/rollup/keeper/
# Check for any test implementations
echo "Checking test implementations..."
rg -A 10 "TestInitiateFeeWithdrawal" x/rollup/
Length of output: 1017
Script:
#!/bin/bash
# Search for msg_server.go files that might contain the implementation
echo "Searching for message server implementation..."
fd "msg_server.go" x/rollup/keeper/ --exec cat {}
# Also check for any pending implementation files
echo "Checking for pending implementation files..."
rg -l "InitiateFeeWithdrawal" x/rollup/keeper/
Length of output: 6757
x/rollup/keeper/keeper_test.go (2)
7-7: LGTM: Required imports added for fee collector functionality.
The new imports for math and authtypes are necessary for handling fee collector operations.
Also applies to: 13-13
23-28: LGTM: Test suite structure properly enhanced.
The additions to KeeperTestSuite appropriately support testing the new fee withdrawal functionality with necessary fields for context, keepers, store, and event management.
evm/executer_test.go (3)
51-58: LGTM! Well-structured withdrawal parameter creation.
The use of crossdomain.NewWithdrawal improves code organization by grouping related parameters together, making the code more maintainable and easier to understand.
59-59: LGTM! Clear separation of hash computation.
Separating the hash computation from parameter creation improves code clarity and aligns with the PR's objective of properly handling withdrawal attributes.
73-73: Consider adding fee-specific test cases.
While the current implementation correctly tests the basic withdrawal functionality, consider adding test cases specific to fee withdrawals that verify:
- Minimum withdrawal amount checks
- Fee collector account balance validation
- Permissionless execution scenarios
Let's check if there are any existing fee-specific test cases:
x/rollup/module.go (4)
15-15: LGTM: Import addition is appropriate.
The addition of the authkeeper import is necessary for the new fee withdrawal functionality to access account information.
31-34: LGTM: ModuleInputs struct changes are well-structured.
The addition of AccountKeeper follows the established dependency injection pattern and maintains consistent field ordering. This change appropriately supports the new fee withdrawal functionality.
58-58: LGTM: Appropriate use of linter directive.
The //nolint:gocritic // hugeParam directive is correctly applied to suppress the large parameter warning, which is a common pattern in Cosmos SDK modules with dependency injection.
59-59: LGTM: Keeper initialization properly includes new dependency.
The NewKeeper call correctly includes the AccountKeeper parameter, maintaining consistency with the module's initialization pattern.
x/rollup/testutil/expected_keepers_mocks.go (1)
57-69: LGTM: Well-structured mock implementations for fee withdrawal support.
The new GetBalance and SendCoinsFromModuleToModule methods are correctly implemented with proper mock patterns and align perfectly with the PR's fee withdrawal objectives. They provide the necessary functionality to:
- Check FeeCollector balances against minimum withdrawal thresholds
- Handle module-to-module transfers during the fee withdrawal process
Also applies to: 113-125
x/rollup/keeper/msg_server_test.go (3)
7-7: LGTM: Import required for fee collector module.
The added import is necessary for accessing the fee collector module constants used in the new test cases.
91-91: LGTM: Improved mock expectations with explicit context.
The mock expectations now explicitly verify the context parameter instead of using gomock.Any(), making the tests more precise and reliable.
Also applies to: 98-98, 165-165, 172-172
213-277: LGTM: Comprehensive test coverage for fee withdrawal functionality.
The test implementation thoroughly covers the core functionality including success and failure scenarios, module interactions, and event emissions. The table-driven test structure is well-organized and follows the established patterns in the codebase.
x/rollup/tests/integration/rollup_test.go (4)
34-35: LGTM! Clear constant definition and improved variable naming.
The constant definition and variable renaming improve code clarity. The initialFeeCollectorBalance is appropriately set for testing purposes, and renaming recipientAddr to userCosmosAddr better reflects its purpose.
Also applies to: 37-37, 59-59
193-207: LGTM! Well-structured balance query refactoring.
The refactoring of balance query functions improves code organization by:
- Using a common
queryBalancehelper function to reduce code duplication - Making the functions more generic and reusable
- Maintaining clear separation of concerns
143-146: Verify rollup module permissions.
The rollup module is granted both Minter and Burner permissions. Please verify:
- Are both permissions necessary for the fee withdrawal functionality?
- Are there any security implications of granting both permissions?
Let's check how these permissions are used:
✅ Verification successful
Both minting and burning permissions are necessary and properly used
The code analysis confirms that both permissions are used appropriately for specific rollup module operations:
-
Minting is used for:
- Processing ETH deposits (
deposits.go): Mints WEI tokens when deposits are received - Processing ERC-20 deposits: Mints corresponding tokens for deposited assets
- Processing ETH deposits (
-
Burning is used for:
- Processing withdrawals (
withdrawals.go): Burns tokens when users withdraw assets - Fee withdrawal functionality (
msg_server.go): Burns collected fees from the rollup module
- Processing withdrawals (
The permissions align with the module's core functionality of handling cross-chain asset transfers and fee management, with proper security checks in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mint/burn operations in the rollup module
ast-grep --pattern 'MintCoins($ctx, $mod, $coins)'
ast-grep --pattern 'BurnCoins($ctx, $mod, $coins)'
Length of output: 105
Script:
#!/bin/bash
# Search for mint/burn operations in the rollup module with broader patterns
rg -A 3 "Mint.*Coins|Burn.*Coins" --type go
# Search for bankkeeper interface or implementation to see required permissions
rg -A 5 "type.*[Bb]ankKeeper.*interface" --type go
# Look for any files containing rollup module logic
fd -e go . x/rollup/
Length of output: 8140
170-174: Clarify the comment about disabled fees.
The comment states "fees are disabled in the simulated integration app" but we're still initializing the fee collector with funds. Please clarify:
- Why are fees disabled in the integration app?
- How does this affect the test's validity for production scenarios?
Let's check if fees are indeed disabled:
x/rollup/keeper/deposits.go (1)
Line range hint 108-109: Track TODO for configurable L1CrossDomainMessenger address
This TODO aligns with the PR objectives mentioning that parameters will be made configurable once a genesis state for x/rollup is established. This is important for the fee withdrawal functionality being implemented.
Let's check if there are any other hardcoded addresses in the codebase:
Would you like me to create a GitHub issue to track the implementation of the genesis state configuration for these addresses?
builder/builder_test.go (3)
316-316: LGTM! Clear variable naming.
The renaming from recipientAddr to userCosmosAddr better reflects the variable's purpose as the user's Cosmos address.
325-331: LGTM! Proper withdrawal transaction setup.
The withdrawal transaction is correctly configured with appropriate parameters including sender address, target address, value, and gas limit.
347-347: LGTM! Comprehensive test coverage.
The test has been properly updated to:
- Include the fee withdrawal transaction in the test payload
- Verify the deposit and user withdrawal results
- Assert the correct number of transactions (3) in the built block
Also applies to: 359-365, 387-388
x/rollup/keeper/msg_server.go (4)
5-9: Imports Added Correctly
The necessary imports ("math/big", "cosmossdk.io/math", and authtypes "github.com/cosmos/cosmos-sdk/x/auth/types") have been added to support the new functionality.
68-68: Simplified Encoding of Withdrawal Value
Using hexutil.EncodeBig(msg.Value.BigInt()) simplifies the encoding process of the withdrawal value.
120-132: Validate Fee Burning Process
The process of sending fees from the FeeCollector module to the Rollup module and then burning them appears correct. Ensure that this aligns with the intended fee withdrawal mechanism and that there are no unintended side effects.
134-152:
Verify Correctness of Event Attributes
In the emitted events, ensure that the attribute keys and their corresponding values are correctly assigned. Specifically, the attribute types.AttributeKeyL2FeeWithdrawalTx is assigned the value types.EventTypeWithdrawalInitiated, which represents an event type rather than a transaction identifier. Confirm that this assignment aligns with the intended event structure.
Run the following script to review the definitions and usages of these attributes:
This will help ensure that the attributes are being used as intended.
✅ Verification successful
Let me gather more context about the event structure and usage patterns.
This will help us understand:
- The relationship between BurnETH events and withdrawal transaction attributes
- Similar patterns of transaction ID attributes in events
- How these events are processed downstream
Based on the gathered context, I can now provide the final response:
Event Attribute Usage is Correct
The usage of types.EventTypeWithdrawalInitiated as a value for AttributeKeyL2FeeWithdrawalTx is intentional and consistent with the codebase patterns. This is evidenced by:
- Similar usage in regular withdrawals where
AttributeKeyL2WithdrawalTxalso usesEventTypeWithdrawalInitiatedas its value - The pattern is validated in e2e tests where these attributes are used for transaction queries
- Both
AttributeKeyL2WithdrawalTxandAttributeKeyL2FeeWithdrawalTxare consistently defined and used to track withdrawal transactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the definitions and usages of event attributes.
# Search for the definition of AttributeKeyL2FeeWithdrawalTx
rg 'AttributeKeyL2FeeWithdrawalTx' -t go
# Search for the definition of EventTypeWithdrawalInitiated
rg 'EventTypeWithdrawalInitiated' -t go
Length of output: 1193
Script:
#!/bin/bash
# Search for the usage patterns of these events in tests and implementation
rg "EventTypeBurnETH.*AttributeKeyL2" -A 5 -B 5 -t go
# Search for any other similar withdrawal transaction attributes
rg "AttributeKeyL2.*Tx" -t go
# Look for event handling or processing of these events
rg "HandleEvent.*withdrawal" -i -t go
Length of output: 1344
builder/builder.go (1)
14-14: Import statement for cross-domain functionalities approved
The addition of the crossdomain package import enhances cross-domain functionalities as required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
bindings/L2ToL1MessagePasserExecuter.go (1)
38-40: Enhance the error message for nil target.While the nil check is good, the error message could be more descriptive to help with debugging.
- return fmt.Errorf("l1 target address is nil") + return fmt.Errorf("cannot initiate withdrawal: L1 target address is nil")proto/rollup/v1/tx.proto (1)
59-65: Enhance message documentation for clarity.Consider adding more details to the message documentation to explicitly mention:
- The permissionless nature of the operation
- The minimum balance requirement from the FeeCollector
- The predetermined L1 recipient address
Apply this enhancement:
-// MsgInitiateFeeWithdrawal defines the message for initiating an L2 fee withdrawal to the L1 fee recipient address. +// MsgInitiateFeeWithdrawal defines the message for initiating a permissionless L2 fee withdrawal to the predetermined L1 fee recipient address. +// The withdrawal will only succeed if the FeeCollector module account balance exceeds the configured minimum withdrawal amount. message MsgInitiateFeeWithdrawal {builder/builder.go (1)
370-371: Consider documenting the current address conversion limitationsThere's a TODO about using updated cosmos -> eth address conversion. Consider documenting the current limitations and potential implications of the current implementation in the code comments.
- // TODO: use updated cosmos -> eth address conversion + // TODO: use updated cosmos -> eth address conversion + // Current implementation uses a simple byte conversion which may not handle all edge cases. + // This should be updated when the new conversion mechanism is available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/rollup/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
bindings/L2ToL1MessagePasserExecuter.go(2 hunks)builder/builder.go(2 hunks)builder/builder_test.go(6 hunks)e2e/stack_test.go(2 hunks)proto/rollup/v1/tx.proto(2 hunks)x/rollup/keeper/deposits.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/stack_test.go
- x/rollup/keeper/deposits.go
🔇 Additional comments (15)
bindings/L2ToL1MessagePasserExecuter.go (3)
8-8: LGTM: Import addition is appropriate.
The new import for the crossdomain package aligns with the PR objectives of replicating OPStack's FeeVault functionality.
37-37: LGTM: Method signature change improves encapsulation.
The change to use crossdomain.Withdrawal struct aligns with the PR objectives and improves the method's interface by consolidating related parameters.
47-48: Verify sender validation for permissionless execution.
Since the PR objectives mention permissionless execution, we should verify that appropriate validation exists for the sender and value fields.
proto/rollup/v1/tx.proto (3)
20-22: LGTM! Well-documented RPC method addition.
The new InitiateFeeWithdrawal RPC method is properly defined and documented, following the established pattern of the service.
67-68: LGTM! Response type follows established pattern.
The empty response message type is appropriate and consistent with other response types in the service.
Line range hint 20-68: Verify integration with withdrawal processing system.
The message definitions look good, but let's verify the integration with the withdrawal processing system to ensure:
- The withdrawal hash construction is properly handled
- The event attributes are correctly processed
- The message server implementation correctly accesses the FeeCollector balance
✅ Verification successful
Integration with withdrawal processing system is properly implemented
Based on the verification results, the implementation shows:
- Proper withdrawal event handling with
EventTypeWithdrawalInitiatedemitted with correct attributes for both regular and fee withdrawals - Complete fee collector balance handling:
- Checks for minimum withdrawal amount
- Proper balance retrieval via
GetBalance - Correct fund transfer from fee collector to module account
- Burns withdrawn fees from module account
- Comprehensive test coverage including edge cases and error scenarios
The integration follows established patterns and maintains proper separation of concerns between the message handling, balance management, and event emission.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration with withdrawal processing system
# Test 1: Check withdrawal hash construction
echo "Checking withdrawal hash construction..."
rg -A 5 "BuildWithdrawalHash|BuildFeeWithdrawalHash"
# Test 2: Check event attribute handling
echo "Checking event attribute handling..."
rg -A 5 "WithdrawalInitiated|FeeWithdrawalInitiated"
# Test 3: Check FeeCollector balance access
echo "Checking FeeCollector balance access..."
rg -A 5 "FeeCollector.*GetBalance"
Length of output: 4336
Script:
#!/bin/bash
# Let's check for withdrawal hash construction and fee collector balance handling in a different way
# Check for withdrawal-related functions in the keeper
echo "Checking withdrawal implementation..."
rg -A 10 "func.*InitiateWithdrawal|func.*InitiateFeeWithdrawal" x/rollup/keeper/
# Check for fee collector balance handling
echo "Checking fee collector balance handling..."
rg -A 10 "feeCollector|FeeCollector" x/rollup/keeper/
# Check for withdrawal hash computation
echo "Checking withdrawal hash computation..."
ast-grep --pattern 'func $_ ($_, $_) {
$$$
hash := $$$
$$$
}'
Length of output: 10813
builder/builder.go (4)
14-14: LGTM: Import addition aligns with requirements
The addition of the crossdomain package from optimism is appropriate for implementing the fee withdrawal functionality.
294-296: LGTM: Clean implementation of dual withdrawal message support
The implementation elegantly handles both user and fee withdrawal messages using a unified code path, which aligns with the requirement to reuse shared code from the existing withdrawal flow.
308-358: LGTM: Improved withdrawal message storage implementation
The refactored implementation correctly uses event attributes for withdrawal parameters and properly handles the message nonce. The error handling is comprehensive and the function maintains a clear separation of concerns.
Note: The event assignment issue mentioned in the previous review still applies.
294-396: Verify the implementation meets security requirements
The implementation looks solid and meets the PR objectives. However, let's verify that the permissionless execution is properly secured.
builder/builder_test.go (5)
316-316: LGTM! Clear variable naming.
The rename from recipientAddr to userCosmosAddr better reflects the variable's role as the sender's cosmos address.
332-334: LGTM! Fee withdrawal message setup is correct.
The MsgInitiateFeeWithdrawal message is correctly initialized with the sender address. The minimal structure aligns with the current implementation where L1 recipient and minimum withdrawal amount are hardcoded.
347-347: LGTM! Transaction list includes all required transactions.
The transaction list correctly includes the deposit, withdrawal, and fee withdrawal transactions.
366-366: Skipping comment on TODO.
This TODO has already been addressed in previous review comments regarding the need for genesis state configuration.
523-525: LGTM! Event attribute verification indices are correctly updated.
The indices for mint amount, recipient address, and transfer amount verification are properly adjusted to match the event attribute ordering.
Adds a message server implementation for the InitiateFeeWithdrawal message to mimic the behavior of the OPStack's FeeVault contracts. Users can permissionlessly triggers fee withdrawals on L2 if the FeeCollector module account balance is above the predefined minimum withdrawal amount. The fees are then able to be withdrawn to the predefined target address on L1.
Updates the builder to use the withdrawal tx event attributes for constructing the withdrawal hash instead of the withdrawal message.
natebeauregard
force-pushed
the
nkb.add-fee-withdrawal-msg
branch
from
64c2725 to
8601504
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
x/rollup/types/expected_keepers.go (1)
22-22: LGTM! Interface follows single responsibility principle.The simplified AccountKeeper interface with
GetModuleAddressbetter aligns with its core responsibility in the fee withdrawal flow.Consider documenting these architectural decisions in the module's README or design docs:
- Why account creation/setting was removed from this interface
- How the simplified interface supports permissionless withdrawals
- The relationship between FeeCollector module and withdrawal process
proto/rollup/v1/tx.proto (1)
59-65: Consider adding validation tags for the sender field.While the message structure is correct, consider adding gogoproto validation tags to ensure the sender field is not empty:
// The signer address of the user initiating the fee withdrawal. - string sender = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; + string sender = 1 [ + (cosmos_proto.scalar) = "cosmos.AddressString", + (gogoproto.nullable) = false, + (amino.dont_omitempty) = true + ];x/rollup/keeper/msg_server.go (1)
113-124: Consider adding transaction atomicity protection.While the implementation correctly handles the two-step transfer and burn process, consider wrapping these operations in a transaction to ensure atomicity. If the burn fails after the transfer, the fees would be stuck in the rollup module.
Consider using the SDK's transaction management:
ctx.WithTxGasHandler(...).x/rollup/testutil/expected_keepers_mocks.go (2)
150-161: Note: Missing line number in implementationThe implementation is correct and properly supports the fee withdrawal feature by allowing tests to retrieve module account addresses. However, there appears to be a missing line number (152) in the annotated code, though this doesn't affect functionality as this is an auto-generated file.
Line range hint
1-161: Architecture: Mock implementations properly support fee withdrawal testingThe added mock methods provide a comprehensive testing interface for the fee withdrawal feature:
GetBalanceenables verification of minimum withdrawal amountsSendCoinsFromModuleToModulesupports testing the actual transfer flowGetModuleAddressallows proper module account handlingThis combination ensures proper testing coverage for the fee withdrawal functionality from L2 to L1.
builder/builder_test.go (1)
366-366: Consider adding placeholder test for fee withdrawal.While waiting for the genesis state implementation, consider adding a basic test to verify that the fee withdrawal transaction is at least processed without errors. This would provide a foundation for more comprehensive testing once the minimum withdrawal amount is configurable.
Also applies to: 387-388
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/rollup/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
bindings/L2ToL1MessagePasserExecuter.go(2 hunks)builder/builder.go(2 hunks)builder/builder_test.go(6 hunks)e2e/stack_test.go(2 hunks)evm/executer_test.go(2 hunks)proto/rollup/v1/tx.proto(2 hunks)x/rollup/keeper/deposits.go(2 hunks)x/rollup/keeper/keeper.go(1 hunks)x/rollup/keeper/keeper_test.go(3 hunks)x/rollup/keeper/msg_server.go(4 hunks)x/rollup/keeper/msg_server_test.go(4 hunks)x/rollup/module.go(3 hunks)x/rollup/tests/integration/rollup_test.go(8 hunks)x/rollup/testutil/expected_keepers_mocks.go(3 hunks)x/rollup/types/errors.go(1 hunks)x/rollup/types/events.go(1 hunks)x/rollup/types/expected_keepers.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- e2e/stack_test.go
- evm/executer_test.go
- x/rollup/keeper/deposits.go
- x/rollup/keeper/keeper.go
- x/rollup/keeper/keeper_test.go
- x/rollup/keeper/msg_server_test.go
- x/rollup/module.go
- x/rollup/tests/integration/rollup_test.go
- x/rollup/types/errors.go
🧰 Additional context used
🪛 Gitleaks
x/rollup/types/events.go
6-6: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (23)
x/rollup/types/expected_keepers.go (1)
13-18: LGTM! Verify usage of new bank keeper methods.
The addition of SendCoinsFromModuleToModule and GetBalance methods aligns well with the fee withdrawal requirements.
Let's verify the implementation of these methods:
✅ Verification successful
Bank keeper methods are properly implemented and used in fee withdrawal flow
The verification confirms that:
SendCoinsFromModuleToModuleis correctly used in the fee withdrawal flow to transfer fees from FeeCollector to rollup moduleGetBalanceis used to check FeeCollector's balance in tests- Both methods have proper mock implementations and test coverage
- The usage aligns with the intended fee withdrawal functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of new bank keeper methods
# Expected: Find implementations in mock and concrete keepers, and their usage in withdrawal logic
# Check for concrete keeper implementation
rg -A 2 "func.*SendCoinsFromModuleToModule"
# Check for usage in withdrawal logic
rg -A 5 "SendCoinsFromModuleToModule.*FeeCollector"
# Check balance checks implementation
rg -A 2 "GetBalance.*FeeCollector"
Length of output: 3195
bindings/L2ToL1MessagePasserExecuter.go (2)
8-8: LGTM: Import addition is appropriate.
The new import for the crossdomain package aligns with the PR's objective of implementing L2 to L1 fee withdrawal functionality.
44-45: Verify sender permissions for fee withdrawal.
Since this implementation is part of the fee withdrawal functionality, we should verify that the sender has the necessary permissions to initiate withdrawals.
Let's check how the sender permissions are enforced:
✅ Verification successful
Let me gather more information about the sender permissions by checking the message server implementation.
Let me try a different approach to find the message handler implementation.
Let me try one more search to find the actual implementation in the keeper.
Let me check the proto definition to understand the sender permissions better.
Based on the gathered information, I can now provide the final response:
The sender permissions are correctly enforced through Cosmos SDK's message signer validation
The MsgInitiateFeeWithdrawal message is properly configured with option (cosmos.msg.v1.signer) = "sender" in the protobuf definition, which ensures that the transaction must be signed by the sender address. This is a standard Cosmos SDK authentication mechanism that verifies the sender's signature before the message is processed. No additional permission checks are needed as the implementation in the keeper focuses on managing the fee collector's balance and withdrawal logistics rather than access control.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for permission checks or access control related to fee withdrawals
rg -A 5 "fee.*withdraw|withdraw.*permission|access.*control"
Length of output: 4199
Script:
#!/bin/bash
# Search for the message server implementation of InitiateFeeWithdrawal
ast-grep --pattern 'func (k msgServer) InitiateFeeWithdrawal($$$) {
$$$
}'
Length of output: 78
Script:
#!/bin/bash
# Search for InitiateFeeWithdrawal handler implementation
rg -A 10 "func.*InitiateFeeWithdrawal.*context.*Msg"
Length of output: 1580
Script:
#!/bin/bash
# Search for InitiateFeeWithdrawal implementation in the keeper
rg -A 15 "func \(k \*Keeper\).*InitiateFeeWithdrawal|func \(ms msgServer\).*InitiateFeeWithdrawal"
Length of output: 1229
Script:
#!/bin/bash
# Check the proto definition for sender permissions
cat proto/rollup/v1/tx.proto
Length of output: 2711
proto/rollup/v1/tx.proto (3)
20-22: LGTM! The RPC method is well-defined.
The InitiateFeeWithdrawal RPC method is correctly defined with clear documentation and appropriate request/response types.
67-68: LGTM! The response message is appropriately defined.
The empty response message is suitable as no additional data needs to be returned from the fee withdrawal operation.
59-68: Architecture looks good, with room for future enhancements.
The message types are well-structured to support the planned genesis state configuration for L1 recipient address and minimum withdrawal amount. The separation of concerns is maintained by keeping configuration parameters outside the message definition.
Let's verify the planned genesis state implementation:
✅ Verification successful
Genesis state implementation is confirmed as planned but not yet implemented
The codebase contains multiple TODOs indicating that genesis state configuration for fee-related parameters is indeed planned but not yet implemented:
- Minimum withdrawal amount will be configurable via genesis state (msg_server.go)
- L1 recipient address will be moved from hardcoded to genesis configuration (multiple files)
- Test cases are prepared to be updated once genesis state is implemented
The current proto message structure correctly anticipates these future enhancements, maintaining clean separation between message definition and configuration parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if genesis state preparation is in progress
# Test: Look for genesis-related files or TODOs
rg -i "genesis.*rollup|todo.*genesis" --type proto --type go
Length of output: 92
Script:
#!/bin/bash
# Search for genesis-related files and configurations
fd -e proto -e go | xargs rg -i "genesis.*rollup|todo.*genesis"
# Also search for any genesis-related structs or types
ast-grep --pattern 'type Genesis$_'
# Look for any fee-related genesis configurations
fd -e proto -e go | xargs rg -i "fee.*genesis|genesis.*fee"
Length of output: 1180
x/rollup/keeper/msg_server.go (5)
34-34: LGTM: Appropriate log level adjustment.
Changed from Info to Debug level which is more suitable for system-level information.
63-63: LGTM: Improved value encoding method.
Using hexutil.EncodeBig directly is a cleaner approach that properly handles all edge cases with big integers.
98-109: LGTM: Proper balance and address validation.
The implementation correctly:
- Validates the fee collector address existence
- Checks the balance against minimum threshold
- Returns appropriate errors with context
91-96: Verify the minimum withdrawal amount threshold.
The hardcoded minimum withdrawal amount of 100 WEI seems quite low. While this will be made configurable later, consider if this provides adequate protection against withdrawal spam attacks that could drain L1 gas.
#!/bin/bash
# Search for any other minimum thresholds in the codebase for comparison
rg -i "minimum.*withdrawal|min.*withdrawal" --type go126-144: Verify event attribute consistency.
The implementation emits two events with appropriate attributes, but let's verify the attribute keys are consistent with other withdrawal events in the system.
✅ Verification successful
The search results show two instances of EventTypeWithdrawalInitiated events in the same file. Comparing their attributes:
-
First instance (lines 65-73):
- AttributeKeySender
- AttributeKeyL1Target
- AttributeKeyValue
- AttributeKeyGasLimit
- AttributeKeyData
- (Nonce comment)
-
Second instance (lines 129-137, from review):
- AttributeKeySender
- AttributeKeyL1Target
- AttributeKeyValue
- AttributeKeyGasLimit
- AttributeKeyData
- (Nonce comment)
Let me verify the attribute key constants are properly defined:
Event attributes are consistent across withdrawal events
The verification shows that both instances of EventTypeWithdrawalInitiated use identical attribute keys that are properly defined in types/events.go. The attribute structure is consistent:
- Both use the same set of attributes: Sender, L1Target, Value, GasLimit, and Data
- All attribute keys are properly defined constants
- Both correctly leave the Nonce attribute to be set by Monomer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other withdrawal event emissions to verify consistency
ast-grep --pattern 'sdk.NewEvent(
types.EventTypeWithdrawalInitiated,
$$$
)'
Length of output: 1593
Script:
#!/bin/bash
# Search for attribute key definitions
rg "AttributeKey.*? =" x/rollup/types/
Length of output: 1034
x/rollup/testutil/expected_keepers_mocks.go (2)
57-70: LGTM: GetBalance mock implementation is correct
The mock implementation correctly supports the fee withdrawal feature by allowing tests to verify balance checks against the fee collector account.
113-125: LGTM: SendCoinsFromModuleToModule mock implementation is correct
The mock implementation properly supports testing module-to-module transfers, which is essential for the fee withdrawal flow from the fee collector module.
builder/builder.go (5)
14-14: LGTM: Import aligns with PR objectives
The addition of the crossdomain package from Optimism aligns with the PR objective of replicating FeeVault.sol functionality.
294-296: LGTM: Clean implementation of dual withdrawal message support
The implementation elegantly handles both user and fee withdrawal messages while maintaining code reusability.
330-336: Fix potential issue with event assignment
The event variable assignment in the loop could lead to incorrect behavior. Please refer to the previous review comment for the recommended fix using a nil initialization and conditional assignment pattern.
360-395: Add validation for required withdrawal parameters
The function should validate that all required fields (Sender, Target, Value, GasLimit, and Data) are properly set after parsing. Please refer to the previous review comment for implementation details.
297-298: Consider addressing the TODO comment about withdrawal message handling
The TODO suggests exploring PostHandler for managing withdrawals across all transaction types. This architectural decision could impact the system's ability to handle withdrawals consistently.
Let's analyze the current transaction handling:
Would you like me to help design a PostHandler-based solution for withdrawal handling?
builder/builder_test.go (5)
316-316: LGTM! Clear variable naming.
The rename from recipientAddr to userCosmosAddr better reflects the variable's purpose and type.
325-330: LGTM! Withdrawal message structure is correct.
The withdrawal message is properly constructed with the renamed userCosmosAddr as the sender.
332-334: LGTM! Fee withdrawal message is properly structured.
The MsgInitiateFeeWithdrawal message is correctly initialized with the sender field, aligning with the PR objectives for implementing permissionless fee withdrawal functionality.
347-347: LGTM! Transaction list properly includes all required transactions.
The transaction list correctly includes deposit, withdrawal, and fee withdrawal transactions in a logical order.
523-525: LGTM! Event checking is properly implemented.
The event attribute verification is correctly implemented for both mint and transfer amounts.
Closes #269
Adds a message server implementation for the
InitiateFeeWithdrawalmessage to mimic the behavior of the OPStack's FeeVault contracts.Users can permissionlessly triggers fee withdrawals on L2 if the
FeeCollectormodule account balance is above the predefined minimum withdrawal amount. The fees are then able to be withdrawn to the predefined target address on L1.Also, updates the builder to use the withdrawal tx event attributes forconstructing the withdrawal hash instead of the withdrawal message.
The minimum withdrawal amount and L1 recipient address are currently hardcoded in
x/rollup, however they'll be updated to be configurable once ax/rollupgenesis state is added.Summary by CodeRabbit
Release Notes
New Features
Improvements
InitiateWithdrawalmethod by consolidating parameters into a single struct.Bug Fixes
Tests