Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Create a new x/rollup message type to withdraw collected fees to L1 #287

Merged
merged 4 commits into from
Nov 1, 2024

Conversation

natebeauregard
Copy link
Collaborator

@natebeauregard natebeauregard commented Oct 31, 2024

Closes #269

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.

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 a x/rollup genesis state is added.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new method for initiating fee withdrawals from Layer 2 to Layer 1.
    • Enhanced withdrawal message handling to include fee withdrawals.
  • Improvements

    • Streamlined the InitiateWithdrawal method by consolidating parameters into a single struct.
    • Updated transaction handling for better clarity and precision in encoding amounts.
    • Added a new constant for Layer 2 fee withdrawal transactions.
    • Enhanced the test suite for more comprehensive testing of fee-related operations.
  • Bug Fixes

    • Added specific error handling for fee withdrawal failures.
  • Tests

    • Expanded test coverage for the fee withdrawal process and improved mock setups for account management.

Copy link
Contributor

coderabbitai bot commented Oct 31, 2024

Walkthrough

The 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 InitiateWithdrawal method is modified to accept a structured parameter, improving clarity. Several methods are refactored to accommodate these changes, including updates to test cases and mock implementations. The overall structure of the codebase remains intact, with a focus on enhancing functionality and maintainability.

Changes

File Change Summary
bindings/L2ToL1MessagePasserExecuter.go Updated InitiateWithdrawal method to accept a single crossdomain.Withdrawal parameter.
builder/builder.go Refactored parseWithdrawalMessages to handle both MsgInitiateWithdrawal and MsgInitiateFeeWithdrawal. Updated storeWithdrawalMsgInEVM method. Added parseWithdrawalEventAttributes function.
builder/builder_test.go Renamed recipientAddr to userCosmosAddr. Introduced feeWithdrawalTx transaction in tests.
e2e/stack_test.go Updated depositValueHex assignment to use hexutil.EncodeBig.
evm/executer_test.go Refactored withdrawal object creation in tests for clarity.
proto/rollup/v1/tx.proto Added InitiateFeeWithdrawal RPC method and MsgInitiateFeeWithdrawal message type.
x/rollup/keeper/deposits.go Updated mintETH and mintERC20 methods to use hexutil.EncodeBig.
x/rollup/keeper/keeper.go Added accountkeeper field to Keeper struct and updated NewKeeper constructor.
x/rollup/keeper/keeper_test.go Enhanced KeeperTestSuite with accountKeeper mock.
x/rollup/keeper/msg_server.go Added InitiateFeeWithdrawal function and updated InitiateWithdrawal method for encoding.
x/rollup/keeper/msg_server_test.go Added TestInitiateFeeWithdrawal function to cover fee withdrawal scenarios.
x/rollup/module.go Added AccountKeeper field to ModuleInputs struct and updated ProvideModule function.
x/rollup/tests/integration/rollup_test.go Updated setupIntegrationApp to return feeCollectorAddr and modified balance query functions.
x/rollup/testutil/expected_keepers_mocks.go Added methods to MockBankKeeper and updated MockAccountKeeper.
x/rollup/types/errors.go Introduced ErrInitiateFeeWithdrawal for fee withdrawal errors.
x/rollup/types/events.go Added AttributeKeyL2FeeWithdrawalTx constant for fee withdrawal transactions.
x/rollup/types/expected_keepers.go Added SendCoinsFromModuleToModule method to BankKeeper interface and updated AccountKeeper methods.

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
Loading

Assessment against linked issues

Objective Addressed Explanation
Create a new x/rollup message type to withdraw collected fees to L1 (#269)
Implement withdrawal logic for FeeCollector when conditions are met (#269)

Possibly related PRs

  • Refactor L1 user deposit processing in x/rollup module #208: The changes in the main PR regarding the InitiateWithdrawal method's signature and its handling of withdrawal parameters are related to the modifications in the processL1UserDepositTxs function in this PR, which also deals with transaction processing and address handling.
  • Enable transfers during L1 deposits and add L1 wallet integration to the devnet #249: This PR enhances the deposit process by allowing transfers during deposits, which aligns with the main PR's changes to the InitiateWithdrawal method that now consolidates parameters into a single struct for better handling of withdrawal operations.
  • Derive Cosmos addresses from public keys #273: The updates in this PR to derive Cosmos addresses from public keys and the changes in address handling in the tests relate to the main PR's focus on modifying how withdrawal parameters are processed and utilized, particularly in the context of Ethereum and Cosmos address conversions.

Suggested reviewers

  • joshklop

Poem

🐰 In the meadow where bunnies play,
New withdrawals hop in a joyful way.
With fees collected, we send them afar,
To L1 they go, like a twinkling star!
Hooray for changes, so clear and bright,
Our rollup dances in the moonlight! 🌙✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

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

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

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

Other keywords and placeholders

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

CodeRabbit Configuration File (.coderabbit.yaml)

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

Documentation and Community

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

@natebeauregard natebeauregard changed the title Nkb.add fee withdrawal msg Create a new x/rollup message type to withdraw collected fees to L1 Oct 31, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 10

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

22-22: LGTM! Consider documenting module address usage.

The shift to GetModuleAddress aligns 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 the x/rollup module. 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 accountKeeper instead of accountkeeper to match the camelCase pattern used in bankkeeper.

-	accountkeeper types.AccountKeeper
+	accountKeeper types.AccountKeeper
proto/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 of SetupSubTest to 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:

  1. Using specific times (e.g., Times(1)) to verify exact call counts
  2. 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:

  1. Genesis state types in types/genesis.go
  2. Genesis state validation in ValidateGenesis
  3. Default genesis state in DefaultGenesis
  4. Genesis state export in ExportGenesis
x/rollup/testutil/expected_keepers_mocks.go (1)

150-161: LGTM with minor formatting observation.

The GetModuleAddress mock 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:

  1. Invalid sender address validation
  2. Verification of minimum withdrawal threshold
  3. Validation of the L1 target address format
  4. 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 using hexutil.EncodeBig

The switch to hexutil.EncodeBig for 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 mintERC20 function'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 Configurable

The minWithdrawalAmount and l1recipientAddr are currently hardcoded. Since the establishment of a genesis state for x/rollup is 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 for minWithdrawalAmount

The minWithdrawalAmount is set to 100. 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 conversion

There'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

📥 Commits

Reviewing files that changed from the base of the PR and between 5ea8460 and b271526.

⛔ Files ignored due to path filters (1)
  • x/rollup/types/tx.pb.go is 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.go with 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:

  1. Check FeeCollector balances against minimum withdrawal thresholds
  2. 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 queryBalance helper 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:

  1. Are both permissions necessary for the fee withdrawal functionality?
  2. 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
  • 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

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:

  1. Why are fees disabled in the integration app?
  2. 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:

  1. Include the fee withdrawal transaction in the test payload
  2. Verify the deposit and user withdrawal results
  3. 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: ⚠️ Potential issue

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:

  1. The relationship between BurnETH events and withdrawal transaction attributes
  2. Similar patterns of transaction ID attributes in events
  3. 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 AttributeKeyL2WithdrawalTx also uses EventTypeWithdrawalInitiated as its value
  • The pattern is validated in e2e tests where these attributes are used for transaction queries
  • Both AttributeKeyL2WithdrawalTx and AttributeKeyL2FeeWithdrawalTx are 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.

x/rollup/keeper/keeper.go Show resolved Hide resolved
bindings/L2ToL1MessagePasserExecuter.go Outdated Show resolved Hide resolved
proto/rollup/v1/tx.proto Outdated Show resolved Hide resolved
x/rollup/keeper/keeper_test.go Show resolved Hide resolved
x/rollup/tests/integration/rollup_test.go Show resolved Hide resolved
builder/builder_test.go Show resolved Hide resolved
e2e/stack_test.go Show resolved Hide resolved
builder/builder.go Outdated Show resolved Hide resolved
builder/builder.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 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:

  1. The permissionless nature of the operation
  2. The minimum balance requirement from the FeeCollector
  3. 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 limitations

There'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

📥 Commits

Reviewing files that changed from the base of the PR and between b271526 and 64c2725.

⛔ Files ignored due to path filters (1)
  • x/rollup/types/tx.pb.go is 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:

  1. The withdrawal hash construction is properly handled
  2. The event attributes are correctly processed
  3. 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:

  1. Proper withdrawal event handling with EventTypeWithdrawalInitiated emitted with correct attributes for both regular and fee withdrawals
  2. 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
  3. 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.

x/rollup/keeper/msg_server.go Show resolved Hide resolved
x/rollup/keeper/msg_server.go Outdated Show resolved Hide resolved
x/rollup/keeper/msg_server.go Outdated Show resolved Hide resolved
x/rollup/keeper/msg_server.go Show resolved Hide resolved
x/rollup/types/expected_keepers.go Show resolved Hide resolved
builder/builder.go Outdated Show resolved Hide resolved
builder/builder.go Outdated Show resolved Hide resolved
bindings/L2ToL1MessagePasserExecuter.go Outdated Show resolved Hide resolved
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.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 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 GetModuleAddress better 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 implementation

The 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 testing

The added mock methods provide a comprehensive testing interface for the fee withdrawal feature:

  1. GetBalance enables verification of minimum withdrawal amounts
  2. SendCoinsFromModuleToModule supports testing the actual transfer flow
  3. GetModuleAddress allows proper module account handling

This 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

📥 Commits

Reviewing files that changed from the base of the PR and between 64c2725 and 8601504.

⛔ Files ignored due to path filters (1)
  • x/rollup/types/tx.pb.go is 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:

  • SendCoinsFromModuleToModule is correctly used in the fee withdrawal flow to transfer fees from FeeCollector to rollup module
  • GetBalance is 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:

  1. Validates the fee collector address existence
  2. Checks the balance against minimum threshold
  3. 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 go

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

  1. First instance (lines 65-73):

    • AttributeKeySender
    • AttributeKeyL1Target
    • AttributeKeyValue
    • AttributeKeyGasLimit
    • AttributeKeyData
    • (Nonce comment)
  2. 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.

x/rollup/types/events.go Show resolved Hide resolved
bindings/L2ToL1MessagePasserExecuter.go Show resolved Hide resolved
@natebeauregard natebeauregard merged commit 562d332 into main Nov 1, 2024
16 checks passed
@natebeauregard natebeauregard deleted the nkb.add-fee-withdrawal-msg branch November 1, 2024 19:16
@coderabbitai coderabbitai bot mentioned this pull request Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a new x/rollup message type to withdraw collected fees to L1
2 participants