Improved deposits #309
Improved deposits #309
Conversation
Previously, we used `repeated bytes` for the type of the deposit transactions in `MsgApplyL1Txs`. Unfortunately, this led to arbitrary byte strings unmarshaling into the `MsgApplyL1Txs` type. By encapsulating each deposit tx in a separate `EthDepositTx` type, we avoid this issue.
In preparation for splitting the MsgApplyL1Txs type into multiple messages, we add a Cosmos SDK transaction type that will wrap all deposits.
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces substantial changes across several files, primarily focusing on the handling of Ethereum deposit transactions and their conversion to Cosmos transactions. Key modifications include the restructuring of message types, updates to function signatures, and enhancements in error handling. The Changes
Possibly related 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (22)
x/rollup/types/deposit_tx.go (1)
12-18: Consider adding nil check for UserDepositsThe implementation efficiently pre-allocates capacity and properly combines messages. However, consider adding a nil check for
d.UserDepositsto prevent potential panics.func (d *DepositsTx) GetMsgs() []proto.Message { + if d.UserDeposits == nil { + return []proto.Message{d.L1Attributes} + } protoMsgUserDeposits := make([]proto.Message, 0, len(d.UserDeposits)) for _, userDeposit := range d.UserDeposits { protoMsgUserDeposits = append(protoMsgUserDeposits, userDeposit) } return append([]proto.Message{d.L1Attributes}, protoMsgUserDeposits...) }x/rollup/tx/helpers/ante.go (1)
Line range hint
41-44: Consider consolidating error handling patternWhile the current error wrapping is good, consider extracting the common pattern:
- newCtx, err := rolluptx.DepositAnteHandler(ctx, tx, simulate) - if err != nil { - return newCtx, fmt.Errorf("deposit ante handle: %v", err) - } - return newCtx, err + return wrapAnteHandlerError( + rolluptx.DepositAnteHandler(ctx, tx, simulate), + "deposit ante handle", + )This would reduce duplication and standardize error handling across all cases.
mempool/mempool.go (1)
Line range hint
38-44: Consider documenting potential race conditionsThe comment about reads and writes occurring on separate views due to DB interface limitations is important. This could lead to race conditions in high-concurrency scenarios.
Consider:
- Adding detailed documentation about the potential race conditions
- Adding metrics or logging to track when these conditions might occur
- Creating a tracking issue for future migration to a DB interface that supports atomic reads and writes
x/rollup/module.go (2)
60-60: Consider addressing technical debt from protov1 usage.Both signer providers use the deprecated
protov1package withnolintdirectives. While the comment explains this is due to gocosmos/gogoproto dependency, consider:
- Creating a tracking issue for future migration
- Documenting the technical debt in the codebase
Also applies to: 69-69
Line range hint
67-73: Consider consolidating duplicate signer logic.The
ProvideSetL1AttributesGetSignerimplementation is identical toProvideApplyUserDepositGetSignerexcept for the message type. Consider extracting a helper function to reduce duplication.+func newEmptySigner(msgType protoreflect.FullName) signing.CustomGetSigner { + return signing.CustomGetSigner{ + MsgType: msgType, + Fn: func(msg proto.Message) ([][]byte, error) { + return [][]byte{}, nil + }, + } +} func ProvideApplyUserDepositGetSigner() signing.CustomGetSigner { - return signing.CustomGetSigner{ - MsgType: protoreflect.FullName(protov1.MessageName(&types.MsgApplyUserDeposit{})), - Fn: func(msg proto.Message) ([][]byte, error) { - return [][]byte{}, nil - }, - } + return newEmptySigner(protoreflect.FullName(protov1.MessageName(&types.MsgApplyUserDeposit{}))) } func ProvideSetL1AttributesGetSigner() signing.CustomGetSigner { - return signing.CustomGetSigner{ - MsgType: protoreflect.FullName(protov1.MessageName(&types.MsgSetL1Attributes{})), - Fn: func(msg proto.Message) ([][]byte, error) { - return [][]byte{}, nil - }, - } + return newEmptySigner(protoreflect.FullName(protov1.MessageName(&types.MsgSetL1Attributes{}))) }x/rollup/tests/integration/rollup_test.go (2)
85-96: Enhance error case coverageWhile the test covers an invalid transaction case using
l1AttributesTxBz, consider adding more error scenarios such as:
- Malformed transaction bytes
- Invalid deposit amounts
- Duplicate deposits
- Missing signatures
This would provide better coverage of potential failure modes.
91-96: Consider splitting deposit tests by typeThe current test combines different deposit types (ETH, bridge, ERC20) in a single loop. Consider splitting these into separate sub-tests using
t.Run()for better clarity and maintainability:
- ETH deposits
- Bridge deposits
- ERC20 deposits
This would make the test flow and assertions clearer, and make it easier to add new test cases for each type.
Example structure:
func TestRollup(t *testing.T) { // Setup code... t.Run("ETH Deposits", func(t *testing.T) { // ETH deposit specific tests }) t.Run("Bridge Deposits", func(t *testing.T) { // Bridge deposit specific tests }) t.Run("ERC20 Deposits", func(t *testing.T) { // ERC20 deposit specific tests }) }builder/builder_test.go (1)
402-406: Consider enhancing test coverage and readability.While the test correctly verifies the core functionality, consider these improvements:
- Add explicit verification for fee withdrawal events
- Add assertions for event attributes beyond just the event type
- Use a more realistic gas limit instead of the hardcoded
1000000000000Here's a suggested enhancement for the event verification:
expectedEvents := []string{ "/rollup.v1.MsgSetL1Attributes", "/rollup.v1.MsgInitiateWithdrawal", + "/rollup.v1.MsgInitiateFeeWithdrawal", } for _, expectedEvent := range expectedEvents { eventData := getEventData[bfttypes.EventDataTx](t, subscription) - require.Equal(t, expectedEvent, eventData.Result.Events[0].Attributes[0].Value, - "Unexpected event type") + event := eventData.Result.Events[0] + require.Equal(t, expectedEvent, event.Attributes[0].Value, "Unexpected event type") + require.NotEmpty(t, event.Attributes[1:], "Expected additional event attributes") }proto/rollup/v1/tx.proto (4)
17-17: Capitalize "L1" in comments for consistencyIn the comments on lines 17~ and 39~, "l1 attributes" should be capitalized as "L1 attributes" to maintain consistency and correctly represent "Layer 1". Please update the comments accordingly.
Also applies to: 39-39
20-20: Fix typo and clarify method descriptionThe comment on line 20~ contains a duplicated word "applying applying" and could be clearer. Consider rephrasing the comment for better clarity, such as:
// ApplyUserDeposit defines a method for applying all L1 system and user deposit transactions.
42-51: Ensure consistent naming for Ethereum transaction fieldsIn
MsgSetL1Attributes, the Ethereum transaction is represented bybytes eth_tx, while inMsgApplyUserDeposit, it is represented bybytes tx. For consistency and clarity, consider using the same field name in both messages, such aseth_txortx.
48-48: Correct article in the commentThe comment on line 48~ reads, "MsgApplyUserDeposit is a eth deposit tx." The article "a" should be "an" because it precedes a vowel sound. It should read:
// MsgApplyUserDeposit is an eth deposit tx.x/rollup/keeper/msg_server.go (3)
20-20: Use consistent error handling withtypes.WrapErrorThe error is currently returned using
fmt.Errorf. For consistency and better error categorization, consider usingtypes.WrapError, as used elsewhere in the codebase.Suggested change:
- return nil, fmt.Errorf("set l1 block info: %v", err) + return nil, types.WrapError(types.ErrSetL1BlockInfo, "set l1 block info: %v", err)Ensure that
types.ErrSetL1BlockInfois defined appropriately.
32-32: Use consistent error handling withtypes.WrapErrorSimilarly, in
ApplyUserDeposit, the error is returned usingfmt.Errorf. Consider usingtypes.WrapErrorfor consistency.Suggested change:
- return nil, fmt.Errorf("get l1 block info: %v", err) + return nil, types.WrapError(types.ErrGetL1BlockInfo, "get l1 block info: %v", err)Ensure that
types.ErrGetL1BlockInfois defined appropriately.
36-36: Address the refactoring noted in the TODO commentThe comment
// TODO: refactorindicates thatprocessL1UserDepositTxsneeds refactoring. If you'd like assistance, I can help refactor this function to improve maintainability and readability.Would you like help in refactoring this function or opening a GitHub issue to track this task?
x/rollup/keeper/msg_server_test.go (4)
17-35: Consider adding more assertions inTestSetL1AttributesCurrently, the test checks only the
Numberfield ofL1BlockInfo. To ensure comprehensive test coverage, consider adding assertions for additional fields such asTimeor other relevant attributes to validate that all fields are correctly set and stored.
Line range hint
109-116: Prevent potential index out-of-range errors when verifying eventsBefore iterating over
s.eventManager.Events(), ensure that the length ofs.eventManager.Events()matcheslen(test.expectedEventTypes). This prevents potential panics due to index out-of-range errors if the number of emitted events does not match expectations.Apply this diff to address the issue:
// Verify that the expected event types are emitted + s.Require().Equal(len(test.expectedEventTypes), len(s.eventManager.Events())) for i, event := range s.eventManager.Events() { s.Require().Equal(test.expectedEventTypes[i], event.Type) }
Line range hint
109-116: Correct the typo ineventManagerThere's a typo in the code where
s.eventManger.Events()is used. It should bes.eventManager.Events().Apply this diff to fix the typo:
- for i, event := range s.eventManger.Events() { + for i, event := range s.eventManager.Events() {
Line range hint
17-116: Refactor test setup to eliminate code duplicationConsider refactoring common setup logic in your tests into helper functions or methods. This will reduce code duplication, make the tests cleaner, and simplify future maintenance.
x/rollup/keeper/deposits.go (3)
41-41: Typo in comment: Correct 'receipient' to 'recipient'There's a typo in the comment on line 41. The word "receipient" should be corrected to "recipient" for clarity.
85-85: Address the TODO: Determine error handling for cross-domain messagesThe TODO comment indicates that further investigation is needed to decide when to return an error if a cross-domain message can't be parsed or executed, as per the OP Spec. Resolving this will ensure proper error handling and compliance with the specification.
Would you like assistance in researching the OP Spec to implement the appropriate error handling? I can help draft the necessary code changes or open a GitHub issue to track this task.
68-71: Ensure comprehensive error handling when minting ETHThe
mintETHfunction could potentially return errors that are not fully handled or logged. Consider enhancing the error messages to provide more context or implementing additional logging to aid in debugging if minting fails.
📜 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 (14)
adapters.go(4 hunks)builder/builder_test.go(1 hunks)mempool/mempool.go(1 hunks)mempool/mempool_test.go(1 hunks)proto/rollup/v1/tx.proto(2 hunks)x/rollup/keeper/deposits.go(2 hunks)x/rollup/keeper/msg_server.go(1 hunks)x/rollup/keeper/msg_server_test.go(3 hunks)x/rollup/module.go(1 hunks)x/rollup/tests/integration/rollup_test.go(1 hunks)x/rollup/tx/deposit.go(1 hunks)x/rollup/tx/helpers/ante.go(2 hunks)x/rollup/types/deposit_tx.go(1 hunks)x/rollup/types/msgs.go(3 hunks)
🔇 Additional comments (33)
x/rollup/tx/deposit.go (2)
12-14: Verify security implications of infinite gas meter
While the type change from *Deposit to *types.DepositsTx aligns with the structural changes, using an infinite gas meter for deposits requires careful consideration.
Consider adding a comment explaining why deposits require infinite gas and any security measures in place.
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient context about the DepositsTx usage and security measures:
Infinite gas meter usage for deposits is justified and secure
The infinite gas meter usage for deposits is appropriate because:
DepositsTxis a specialized transaction type that can only be created through thepackDepositTxsToCosmosTxfunction, which requires L1 attributes and user deposits- The transaction type implements the Cosmos SDK
Txinterface and is properly restricted through protocol buffers definition - Deposits are L1-originated transactions that have already paid for gas on L1, making the infinite gas meter a necessary implementation detail for L2 processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if infinite gas meter usage is properly restricted
# Expected: Find evidence that DepositsTx can only be created through authorized paths
# Check for access control or validation around DepositsTx creation
rg -A 5 "DepositsTx"
# Check for potential security measures or restrictions
ast-grep --pattern 'func $_(ctx $_, tx *types.DepositsTx) {
$$$
}'
Length of output: 8766
20-24: LGTM with validation verification needed
The changes to use DepositsTx and the improved error handling look good. However, let's verify that proper validation is in place.
✅ Verification successful
Validation is properly implemented
The validation checks are sufficient:
MsgApplyUserDeposit.ValidateBasic()performs thorough validation:- Checks if transaction is nil
- Validates transaction can be unmarshaled
- Ensures it's a deposit transaction
MsgSetL1Attributes.ValidateBasic()is empty which is acceptable as it's an internal message with no user-provided data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper validation of DepositsTx
# Expected: Find validation logic for deposit transactions
# Look for Validate() method or similar validation
ast-grep --pattern 'func (tx *DepositsTx) Validate() {
$$$
}'
# Check for any custom validation in the message types
rg -A 5 "func.*Validate.*DepositsTx"
Length of output: 104
Script:
#!/bin/bash
# Let's try a broader search for validation patterns
# Search for any validation related to deposits
rg -i "validate.*deposit" -A 5
# Look for the DepositsTx definition and its methods
ast-grep --pattern 'type DepositsTx struct {
$$$
}'
# Search for any validation in the tx processing path
rg "func.*ProcessDeposit" -A 10
# Check for any validation in the handler
rg "HandleDeposit" -A 10
Length of output: 556
Script:
#!/bin/bash
# Let's check for validation in the message types that compose DepositsTx
# Search for validation in MsgSetL1Attributes
ast-grep --pattern 'func (msg *MsgSetL1Attributes) Validate() {
$$$
}'
rg "func.*MsgSetL1Attributes.*Validate" -A 10
# Search for validation in MsgApplyUserDeposit
ast-grep --pattern 'func (msg *MsgApplyUserDeposit) Validate() {
$$$
}'
rg "func.*MsgApplyUserDeposit.*Validate" -A 10
# Look for any validation checks in the message handler
rg "ValidateBasic" -A 10
Length of output: 5958
x/rollup/types/deposit_tx.go (2)
10-10: LGTM! Good practice with interface implementation check
The compile-time interface check ensures that DepositsTx properly implements the sdktypes.Tx interface.
6-6: Verify the necessity of using the deprecated protobuf v1 package
The code uses the deprecated github.com/golang/protobuf/proto package. While it's properly marked with a nolint directive, consider migrating to the newer google.golang.org/protobuf APIs if possible.
Let's check if this deprecated package is used elsewhere in the codebase:
✅ Verification successful
Migration to google.golang.org/protobuf is not recommended at this time
The deprecated protobuf v1 package is used intentionally across the codebase for specific compatibility reasons:
- In
module.go, it's explicitly noted that "gocosmos is built on gogoproto, which generates protov1 protobufs" - The usage is consistent across the codebase for protobuf message handling, particularly in cosmos-sdk related functionality
- All usages are properly marked with
//nolint:staticcheckindicating this is a known and accepted technical debt
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of deprecated protobuf v1 package
rg --type go "github.com/golang/protobuf/proto"
Length of output: 308
Script:
#!/bin/bash
# Let's check how this deprecated package is actually used in these files
rg --type go -B 2 -A 2 "protov1\."
Length of output: 2917
x/rollup/tx/helpers/ante.go (2)
10-10: LGTM: Import addition is appropriate
The new import for rolluptypes is necessary for handling the DepositsTx type and follows proper Go import conventions.
Line range hint 39-44: Verify handling of legacy Deposit transactions
The switch to DepositsTx looks good, but we should verify:
- That there's no need to handle legacy Deposit transactions during migration
- That the DepositAnteHandler is properly implemented for the new type
mempool/mempool_test.go (2)
28-30: LGTM! Clear test setup with proper error handling.
The test setup properly generates and marshals both L1 attributes and deposit transactions with appropriate error checking.
34-36: Verify the rejection of deposit transactions across the codebase.
While the test correctly verifies that deposit transactions are rejected from the pool, let's ensure this behavior is consistently documented and implemented.
✅ Verification successful
Let me gather more information about the deposit transaction handling by checking the content of the relevant documentation files and the implementation details.
Based on the gathered information, I can now provide the final response:
Deposit transaction handling is correctly implemented and well-documented
The rejection of deposit transactions in the mempool is consistent with the architecture where:
- Deposit transactions must only come from L1 and are processed through the
x/rollupmodule - Documentation explicitly states that sequencers must include L1 deposit transactions without modification
- Multiple validation checks across the codebase ensure deposit transactions follow the correct flow:
- Mempool explicitly rejects deposit transactions
x/rollupmodule validates deposit transactions throughIsDepositTx()checks- Adapter layer ensures deposit transactions are properly ordered and validated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistent handling of deposit transaction rejection
# across the codebase and documentation.
# Check for related deposit transaction handling logic
rg -A 5 "deposit.*tx.*not.*allowed"
# Look for documentation of this behavior
fd -e md | xargs rg -l "deposit.*transaction"
# Check for other potential deposit transaction validations
ast-grep --pattern 'if $COND {
$$$
"deposit tx"
$$$
}'
Length of output: 898
Script:
#!/bin/bash
# Check the content of relevant documentation files
echo "=== Documentation for deposits ==="
cat docs/docs/learn/deposits.md
echo -e "\n=== Rollup module documentation ==="
cat docs/docs/learn/rollup-module.md
echo -e "\n=== Implementation of deposit tx validation ==="
# Find the function that validates deposit transactions
ast-grep --pattern 'func $NAME($PARAMS) $RET {
$$$
"deposit tx"
$$$
}'
# Check for any IsDeposit or similar methods
rg -A 3 "IsDeposit|ValidateDeposit"
Length of output: 5452
mempool/mempool.go (2)
45-47: Verify integration with deposit transaction processing
The change in GetDepositTxs usage is part of a larger change in deposit transaction handling. Let's ensure this change is consistent with the new DepositsTx message type mentioned in the summary.
Run the following to verify the usage pattern across the codebase:
✅ Verification successful
Let me gather more context about the DepositsTx message type to ensure complete verification.
Integration with deposit transaction handling is correctly implemented
The code change is consistent with the DepositsTx message type and deposit transaction handling across the codebase. The mempool's rejection of deposit transactions is intentional as they are handled through a special path with:
- A dedicated
DepositsTxprotobuf message type that wraps L1 attributes and user deposits - Special gas handling via
DepositAnteHandler - Custom transaction processing in the rollup module
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how GetDepositTxs is used across the codebase
rg -A 5 "GetDepositTxs\("
# Check for any remaining array-style calls that might have been missed
ast-grep --pattern 'GetDepositTxs($$$[][]byte$$$)'
Length of output: 968
Script:
#!/bin/bash
# Check for DepositsTx message type definition and usage
rg -A 5 "DepositsTx"
# Check for any related deposit transaction handling
ast-grep --pattern 'type DepositsTx struct {
$$$
}'
Length of output: 9160
45-47: 🛠️ Refactor suggestion
Consider more specific error handling for deposit transaction validation
The current implementation might mask important error conditions. While checking if GetDepositTxs succeeds to identify deposit transactions works, it could hide legitimate errors that should be handled differently.
Consider this more explicit approach:
- if _, err := monomer.GetDepositTxs(userTxn); err == nil {
+ depositTxs, err := monomer.GetDepositTxs(userTxn)
+ if err != nil && !errors.Is(err, monomer.ErrNotDepositTx) {
+ return fmt.Errorf("failed to validate transaction type: %w", err)
+ }
+ if depositTxs != nil {
return errors.New("deposit txs are not allowed in the pool")
+ }Let's verify the error types from the monomer package:
x/rollup/module.go (2)
49-54: LGTM! Verify removal of old provider.
The registration of specific signer providers improves clarity and separation of concerns. However, let's verify that the removed ProvideCustomGetSigner is not used elsewhere.
✅ Verification successful
Removal of ProvideCustomGetSigner is safe
The codebase scan shows that the old ProvideCustomGetSigner has been completely removed and replaced with two specific signer providers: ProvideApplyUserDepositGetSigner and ProvideSetL1AttributesGetSigner. No other references to the old provider exist, confirming this refactoring is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old provider
# Test: Search for any remaining usage of the old provider
rg -l "ProvideCustomGetSigner"
Length of output: 30
Script:
#!/bin/bash
# Let's try a more comprehensive search including git history
git log -p | rg "ProvideCustomGetSigner"
# Also search for any similar patterns that might have been renamed
rg -i "provide.*signer"
# Check for any custom signer related code
ast-grep --pattern 'func Provide$_Signer'
Length of output: 479
57-65: Verify the empty signer implementation.
The ProvideApplyUserDepositGetSigner returns empty byte slices. This might indicate:
- Incomplete implementation
- Intentional no-op for specific message types
Please confirm if this is the intended behavior.
✅ Verification successful
Empty signer implementation is intentional and correct
The empty signer implementation for ApplyUserDeposit is the correct behavior because:
- The message processes L1 user deposit transactions that are already signed and executed on L1 (Ethereum)
- The keeper implementation in
msg_server.goshows that it processes these L1 transactions without requiring additional signatures - The test cases in
msg_server_test.goconfirm this by validating deposit transactions without any signature verification
The empty byte slice return is appropriate as no additional signatures are needed for these L1-originated transactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar patterns in other modules
# Test: Look for other CustomGetSigner implementations for comparison
ast-grep --pattern 'CustomGetSigner {
$$$
Fn: func($_) ([][]byte, error) {
return [][]byte{}, nil
}
$$$
}'
Length of output: 120
Script:
#!/bin/bash
# Let's try a broader search for CustomGetSigner implementations
rg -A 5 "CustomGetSigner" --type go
# Also search for any files containing ApplyUserDeposit to understand the context
rg "ApplyUserDeposit" --type go
# And look for any signing-related functions in the codebase
rg "GetSigner" --type go
Length of output: 9361
Script:
#!/bin/bash
# Let's check the implementation of ApplyUserDeposit in the keeper to understand if signatures are needed
rg -A 10 "func .* ApplyUserDeposit" --type go
# Also check the test files to see how these messages are handled
cat x/rollup/keeper/msg_server_test.go
Length of output: 11441
x/rollup/tests/integration/rollup_test.go (1)
78-83: Consider using realistic L1BlockInfo test data
The comment indicates that using an empty L1BlockInfo is "technically incorrect". To ensure robust testing, consider using realistic L1BlockInfo test data that matches production scenarios.
Let's check how L1BlockInfo is used in production:
builder/builder_test.go (1)
403-403: LGTM! Event type update aligns with protocol changes.
The event type change from MsgApplyL1Txs to MsgSetL1Attributes correctly reflects the restructuring of transaction handling in the rollup module.
Let's verify the consistency of this event type change across the codebase:
✅ Verification successful
Based on the results from all verification attempts, I can now generate the final response:
Event type change is consistent with protocol refactoring
The change from MsgApplyL1Txs to MsgSetL1Attributes is part of a broader protocol refactoring where:
- The new message type is properly defined in
proto/rollup/v1/tx.proto - Old references to
MsgApplyL1Txsonly remain in documentation and integration tests - The builder package correctly handles the new event type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the event type change is consistent across the codebase
# Expected: Only new event type (MsgSetL1Attributes) should be present, no references to old event type (MsgApplyL1Txs)
# Check for any remaining references to the old event type
echo "Checking for old event type references..."
rg "MsgApplyL1Txs"
# Verify the new event type is properly defined in proto files
echo "Verifying new event type definition..."
rg -A 5 "MsgSetL1Attributes" "proto/"
# Check for any test files that might need updates
echo "Checking other test files for event types..."
rg "EventType.*Msg.*L1" "*_test.go"
Length of output: 2950
Script:
#!/bin/bash
# Let's do a more focused search to understand the event type changes
# Check for event type definitions and usage in rollup module
echo "Checking rollup module for event types..."
rg -A 3 "EventType.*Msg.*L1|EventType.*Set.*L1" "x/rollup/"
# Check for any test files specifically in the rollup module
echo "Checking rollup test files..."
fd -e go -e test.go . "x/rollup/" | xargs rg "EventType.*Msg"
# Check the builder package for related event handling
echo "Checking builder package..."
rg -A 3 "EventType|MsgSetL1Attributes" "builder/"
Length of output: 1180
Script:
#!/bin/bash
# Let's verify the event type changes in the rollup types package
# Check the rollup types for event type definitions
echo "Checking rollup types for event definitions..."
rg -A 3 "const.*Event" "x/rollup/types/"
# Check for the message handler implementation
echo "Checking message handler implementation..."
ast-grep --pattern 'func $_ MsgSetL1Attributes($$$) {
$$$
}'
# Check for any remaining references to old event emission
echo "Checking for event emission patterns..."
rg -A 3 "EmitEvent.*L1|ctx\.EventManager\(\)\.Emit.*L1"
Length of output: 450
x/rollup/types/msgs.go (7)
19-19: Interface implementation for MsgApplyUserDeposit
The assertion that MsgApplyUserDeposit implements sdktypes.Msg is correct.
38-40: Correct implementation of Type method
The Type method for MsgApplyUserDeposit correctly returns "apply_user_deposit".
42-44: Correct implementation of Route method
The Route method for MsgApplyUserDeposit correctly returns ModuleName.
46-46: Interface implementation for MsgSetL1Attributes
The assertion that MsgSetL1Attributes implements sdktypes.Msg is correct.
52-53: Correct implementation of Type method
The Type method for MsgSetL1Attributes correctly returns "set_l1_attributes".
56-57: Correct implementation of Route method
The Route method for MsgSetL1Attributes correctly returns ModuleName.
81-81: Ensure consistency in Route methods
The Route method for MsgInitiateWithdrawal returns ModuleName, ensuring consistency across message types.
adapters.go (7)
6-6: Approved: Importing the "time" package
The import of the "time" package is necessary for using time.Now().Unix() in the code.
9-10: Approved: Adding required imports for rollup and derive packages
The addition of "github.com/ethereum-optimism/optimism/op-node/rollup" and "github.com/ethereum-optimism/optimism/op-node/rollup/derive" imports is appropriate as they are utilized in the updated code.
43-43: Approved: Returning combined transactions
The return statement correctly appends the deposit transaction bytes to the non-deposit Cosmos transactions.
71-74: Verify the use of empty rollup.Config{} and time.Now().Unix()
Ensure that passing an empty rollup.Config{} and using the current Unix timestamp is appropriate for this context. Missing configuration parameters or incorrect timestamps might lead to unexpected behavior.
89-94: Approved: Setting optional L1 block info fields if not nil
The code correctly checks for nil before setting the BaseFee and BlobBaseFee fields, preventing potential nil pointer dereferences.
96-102: Approved: Processing user deposit transactions appropriately
The loop correctly processes user deposit transactions starting from the second element, as the first is the L1 attributes transaction.
129-136: Approved: Converting Cosmos non-deposit transactions to Ethereum transactions
The code appropriately handles the conversion of Cosmos non-deposit transactions back to Ethereum transactions.
x/rollup/keeper/msg_server_test.go (2)
Line range hint 37-55: Verify that L1 attributes are set before applying user deposits
It's good practice to ensure that SetL1Attributes is called before ApplyUserDeposit to maintain the correct sequence of operations. This ensures that the necessary L1 context is established before processing user deposits.
57-83: Update test cases to handle single txBytes correctly
The change from [][]byte to []byte for txBytes simplifies transaction handling. Ensure that all test cases are updated accordingly and correctly process the single transaction byte slice to prevent any unintended errors.
x/rollup/keeper/deposits.go (3)
5-5: Remove unnecessary import if not used
The import of "errors" on line 5 is standard for error handling. However, if the code uses fmt.Errorf or custom error types exclusively, ensure that the "errors" package is necessary. If not used elsewhere, consider removing it to keep the imports clean.
Check if the errors package functions are utilized beyond what fmt.Errorf provides.
79-80: Validate L1CrossDomainMessenger parameter from chain parameters
When converting the L1CrossDomainMessenger address to its L2 aliased address, ensure that params.L1CrossDomainMessenger is correctly set and validated. An incorrect or unset address could lead to incorrect message processing.
Run the following script to check if the L1CrossDomainMessenger parameter is set and valid:
✅ Verification successful
L1CrossDomainMessenger address is properly validated
The codebase shows that the L1CrossDomainMessenger address is properly validated:
- The address is validated through
validateEthAddress()function in the Params.Validate() method - The validation ensures the address is a valid Ethereum hex address using
common.IsHexAddress() - The address has a default value set (
defaultL1CrossDomainMessenger) - The conversion to L2 aliased address is safely handled using
ApplyL1ToL2Alias
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that L1CrossDomainMessenger address is correctly set in the chain parameters.
# Test: Query the chain parameters and extract the L1CrossDomainMessenger address.
# Expect: A non-empty, valid Ethereum address.
# Command to get the parameters (replace `your_binary` with the appropriate command)
your_binary query rollup params --output=json | jq '.L1CrossDomainMessenger'
Length of output: 178
Script:
#!/bin/bash
# Let's check how the L1CrossDomainMessenger parameter is validated in the codebase
# Search for parameter validation related to L1CrossDomainMessenger
rg -A 5 "L1CrossDomainMessenger" --type go
# Also check for any parameter validation patterns
ast-grep --pattern 'func Validate() error {
$$$
}'
Length of output: 6025
Script:
#!/bin/bash
# Let's check the validation function for Ethereum addresses
rg -A 5 "validateEthAddress" --type go
# Also check the implementation of ApplyL1ToL2Alias
rg -A 5 "ApplyL1ToL2Alias" --type go
Length of output: 1853
61-66: Verify consistency between mint amount and transfer amount
Ensure that the mintAmount and transferAmount retrieved from the transaction are consistent and correctly represent the intended amounts. Misalignment might lead to incorrect fund distribution.
Run the following script to cross-verify mintAmount and transferAmount in all deposit transactions:
Note: Replace extract_mint_amount and extract_transfer_amount with actual command or script to extract these values from transactions.
✅ Verification successful
Based on the evidence gathered from the codebase, I can now provide a final response:
The mint and transfer amounts are correctly handled and validated
The code shows proper handling of these amounts where:
- The
mintAmountrepresents the total amount to be minted - The
transferAmountis the portion to be transferred to the recipient - The remaining amount (
mintAmount - transferAmount) is correctly tracked and validated in tests - The relationship is verified in
checkDepositTxResulttest function which ensures the amounts are properly emitted in events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Ensure that mintAmount >= transferAmount in deposit transactions.
# Extract transactions and compare amounts.
# Command to extract and compare amounts in transactions (pseudo-code, replace with actual commands)
# Assuming transactions are stored in a directory or can be queried.
for tx_file in $(find ./transactions -name '*.tx'); do
mint_amount=$(extract_mint_amount "$tx_file")
transfer_amount=$(extract_transfer_amount "$tx_file")
if (( mint_amount < transfer_amount )); then
echo "Transaction $tx_file has mintAmount less than transferAmount."
fi
done
Length of output: 85
Script:
#!/bin/bash
# Let's analyze the codebase to understand the relationship between mint and transfer amounts
# First, let's find all references to these amounts
rg "Mint\(\)" -A 5 -B 5
# Also check for any tests that might validate this relationship
fd -e go -e _test.go --exec rg -l "Mint\(\)|Value\(\)"
# Look for any validation logic
ast-grep --pattern 'if $_ := tx.Mint(); $$$'
Length of output: 4658
Script:
#!/bin/bash
# Let's examine the test files to understand the relationship between mint and transfer amounts
rg "checkDepositTxResult" -A 10 -B 10
# Also look for any deposit transaction validation
ast-grep --pattern 'func $_(tx $_, $$$) {
$$$
mintAmount := $$$
$$$
transferAmount := $$$
$$$
}'
Length of output: 2869
Script:
#!/bin/bash
# Let's check the test implementation to see how these values are validated
rg "EventTypeMintETH" -A 10 -B 10
# Also look for any validation in the deposit handling
rg "HandleDeposit" -A 20 -B 5
Length of output: 6477
proto/rollup/v1/tx.proto
Outdated
| // DepositTx is a eth deposit tx. | ||
| message EthDepositTx { | ||
| // tx is the marshaled Ethereum Deposit tx. | ||
| bytes tx = 1; | ||
| } | ||
|
|
||
| // MsgApplyL1Txs defines the message for applying all L1 system and user deposit txs. | ||
| message MsgApplyL1Txs { | ||
| // Array of bytes where each bytes is a eth.Transaction.MarshalBinary tx. | ||
| // The first tx must be the L1 system deposit tx, and the rest are user txs if present. | ||
| repeated bytes tx_bytes = 1; | ||
| // txs are all of the system and user deposit txs. | ||
| repeated EthDepositTx txs = 1; |
There was a problem hiding this comment.
Note to reviewers: for all commits, all changes are downstream of the changes in tx.proto.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
proto/rollup/v1/tx.proto (2)
20-21: Enhance the RPC method documentationThe comment should provide more context about the purpose and impact of applying a user deposit transaction.
- // ApplyUserDeposit defines a method for applying an L1 user deposit tx. + // ApplyUserDeposit processes an L1 user deposit transaction in the L2 state. + // This method validates and applies deposits initiated on L1, ensuring proper + // asset bridging from L1 to L2.
33-37: Add field documentation to DepositsTx messageThe message fields should be documented to explain their purpose and relationship.
// DepositsTx is the Cosmos SDK transaction type that wraps OP Stack deposit transactions. message DepositsTx { - MsgSetL1Attributes l1_attributes = 1; - repeated MsgApplyUserDeposit user_deposits = 2; + // l1_attributes contains the L1 block information and related transaction data + MsgSetL1Attributes l1_attributes = 1; + // user_deposits is a list of deposit transactions to be processed + repeated MsgApplyUserDeposit user_deposits = 2; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
proto/rollup/v1/tx.proto(2 hunks)
🔇 Additional comments (2)
proto/rollup/v1/tx.proto (2)
39-43:
Add signer option and improve field documentation
The message needs a signer option for authorization control and better field documentation.
// MsgSetL1Attributes is the l1 attributes message.
message MsgSetL1Attributes {
+ option (cosmos.msg.v1.signer) = "authority";
+
+ // l1_block_info contains the L1 block header information and metadata
L1BlockInfo l1_block_info = 1;
+ // eth_tx is the serialized Ethereum transaction that triggered this L1 attributes update
bytes eth_tx = 2;
}48-51:
Add signer option and enhance field documentation
The message needs a signer option for authorization control and more detailed field documentation.
// MsgApplyUserDeposit is a eth deposit tx.
message MsgApplyUserDeposit {
+ option (cosmos.msg.v1.signer) = "authority";
+
// tx is the marshaled Ethereum Deposit tx.
+ // This contains the complete deposit transaction data from L1 that needs to be
+ // processed on L2, including amount, recipient, and any additional payload.
bytes tx = 1;
}
joshklop
force-pushed
the
improved-deposits3
branch
from
00aa33e
to
a9cbd74
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
x/rollup/types/msgs.go (1)
21-36: Consider additional validation checksThe ValidateBasic implementation could be enhanced with additional checks:
- Validate sender address format
- Check transaction data size
- Validate gas price and value fields
Would you like me to provide an implementation with these additional checks?
docs/docs/learn/deposits.md (1)
51-53: Consider adding more detail about ETH minting processWhile the simplified explanation is clear, it might be helpful to briefly mention the validation steps that occur before ETH minting.
Consider expanding point 6 to:
-6. unpacks the deposit transaction, minting ETH according to embedded values +6. unpacks the deposit transaction, validates the deposit parameters, and mints ETH according to embedded valuesproto/rollup/v1/tx.proto (3)
20-21: Enhance the comment documentation for ApplyUserDeposit RPCThe current comment could be more descriptive about the purpose and context of user deposits.
Consider updating the comment to be more informative:
- // ApplyUserDeposit defines a method for applying a user deposit tx. + // ApplyUserDeposit processes an Ethereum deposit transaction on L2, transferring assets + // from L1 to L2 based on the deposit transaction data.
33-37: Enhance documentation for DepositsTx message typeWhile the structure is well-designed, the documentation could better explain its purpose and relationship with L1/L2.
Consider expanding the comment:
-// DepositsTx is the Cosmos SDK transaction type that wraps OP Stack deposit transactions. +// DepositsTx is the Cosmos SDK transaction type that encapsulates OP Stack deposit transactions, +// combining L1 block attributes and user deposits into a single transaction for atomic processing +// of L1->L2 deposits.
Changes are properly implemented but require test updates
The verification shows that:
- The new message handlers (
SetL1AttributesandApplyUserDeposit) are properly implemented in the keeper- The old message type
MsgApplyL1Txsonly appears in test comments, indicating test files need to be updated- All core functionality has been properly migrated to the new message types
Action items:
- Update comments in
x/rollup/tests/integration/rollup_test.goto reflect the new message types instead of referencingMsgApplyL1Txs🔗 Analysis chain
Line range hint
16-55: Verify updates in dependent codeThese significant changes to message types require corresponding updates in dependent code.
Let's verify the proper implementation and usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper implementation and usage of new message types # Check for proper message handler implementations echo "Checking message handler implementations..." rg "func.*(?:SetL1Attributes|ApplyUserDeposit).*Server" -g "msg_server*.go" # Check for proper message construction in tests echo "Checking test coverage..." rg "(?:MsgSetL1Attributes|MsgApplyUserDeposit)" -g "*_test.go" # Check for any remaining references to old message types echo "Checking for old message references..." rg "MsgApplyL1Txs"Length of output: 1321
Script:
#!/bin/bash # Let's check for message handler implementations in keeper files echo "Checking keeper implementations..." rg "func.*(?:SetL1Attributes|ApplyUserDeposit)" -g "keeper/*.go" -A 5 # Check for any remaining old message type definitions echo "Checking old message type definitions..." rg "type.*MsgApplyL1Txs" -g "*.go" # Check for any imports or usages of the old message types echo "Checking imports and usages of old types..." rg "MsgApplyL1Txs" -g "*.proto"Length of output: 556
Script:
#!/bin/bash # Let's try with correct paths and more specific searches echo "Checking keeper implementations..." rg "func.*(?:SetL1Attributes|ApplyUserDeposit)" -g "x/rollup/keeper/*.go" -A 5 # Check for message server implementations echo "Checking message server implementations..." rg "func \(k Keeper\).*(?:SetL1Attributes|ApplyUserDeposit)" -g "x/rollup/keeper/msg_server.go" -A 10 # Check for any remaining references to old message types in proto files echo "Checking proto files for old message types..." fd -e proto -x cat {} \; | rg "MsgApplyL1Txs" # Check for any remaining references to old message types in Go files echo "Checking Go files for old message types..." fd -e go -x cat {} \; | rg "MsgApplyL1Txs"Length of output: 2845
x/rollup/keeper/msg_server.go (1)
29-37: Standardize error wrapping approachThe error handling switches between
fmt.Errorfandtypes.WrapError. Consider standardizing ontypes.WrapErrorfor consistency.l1blockInfo, err := k.GetL1BlockInfo(ctx) if err != nil { - return nil, fmt.Errorf("get l1 block info: %v", err) + return nil, types.WrapError(types.ErrGetL1BlockInfo, "err: %v", err) }x/rollup/module.go (2)
57-65: Consider adding validation in the signer functionWhile returning empty signatures is appropriate for consensus-level messages, the signer function could include additional validation or logging to help catch potential misuse. Also, the dependency on the deprecated
protov1package should be tracked for future updates.Consider adding validation:
func ProvideApplyUserDepositGetSigner() signing.CustomGetSigner { return signing.CustomGetSigner{ MsgType: protoreflect.FullName(protov1.MessageName(&types.MsgApplyUserDeposit{})), //nolint:staticcheck Fn: func(msg proto.Message) ([][]byte, error) { + // Add logging or validation here + if msg == nil { + return nil, fmt.Errorf("message cannot be nil") + } return [][]byte{}, nil }, } }
Line range hint
67-73: Consider reducing duplication in signer providersThe implementation duplicates the pattern from
ProvideApplyUserDepositGetSigner. Consider extracting a helper function to reduce duplication and ensure consistent behavior.Example refactor:
+func provideConsensusMessageSigner(msgType protoreflect.FullName) signing.CustomGetSigner { + return signing.CustomGetSigner{ + MsgType: msgType, + Fn: func(msg proto.Message) ([][]byte, error) { + if msg == nil { + return nil, fmt.Errorf("message cannot be nil") + } + return [][]byte{}, nil + }, + } +} + func ProvideApplyUserDepositGetSigner() signing.CustomGetSigner { - return signing.CustomGetSigner{ - MsgType: protoreflect.FullName(protov1.MessageName(&types.MsgApplyUserDeposit{})), //nolint:staticcheck - Fn: func(msg proto.Message) ([][]byte, error) { - return [][]byte{}, nil - }, - } + return provideConsensusMessageSigner( + protoreflect.FullName(protov1.MessageName(&types.MsgApplyUserDeposit{})), //nolint:staticcheck + ) } func ProvideSetL1AttributesGetSigner() signing.CustomGetSigner { - return signing.CustomGetSigner{ - MsgType: protoreflect.FullName(protov1.MessageName(&types.MsgSetL1Attributes{})), //nolint:staticcheck - Fn: func(msg proto.Message) ([][]byte, error) { - return [][]byte{}, nil - }, - } + return provideConsensusMessageSigner( + protoreflect.FullName(protov1.MessageName(&types.MsgSetL1Attributes{})), //nolint:staticcheck + ) }x/rollup/keeper/msg_server_test.go (2)
17-35: Consider adding error test casesThe test covers the happy path well but lacks test cases for error scenarios. Consider adding test cases for:
- Invalid L1 block info
- Invalid Ethereum transaction bytes
- Zero block number
Line range hint
274-324: Consider adding specific parameter validation test casesWhile the test suite covers the basic scenarios, consider adding test cases for specific parameter validations:
- Individual parameter bounds
- Parameter type validations
- Empty/nil parameter values
x/rollup/keeper/deposits.go (4)
24-30: Enhance function documentationThe function documentation could be more comprehensive. Consider adding parameter descriptions and return value explanations.
-// processL1UserDepositTxs processes the L1 user deposit txs, mints ETH to the user's cosmos address, -// and returns associated events. +// processL1UserDepositTxs processes L1 user deposit transactions and mints ETH to the user's cosmos address. +// Parameters: +// - ctx: The SDK context for the transaction +// - deposit: The user deposit message containing the transaction data +// - l1blockInfo: Information about the L1 block where the deposit occurred +// Returns: +// - sdk.Events: Events generated during the deposit processing +// - error: Any error that occurred during processing
61-67: Add validation for mint and transfer amountsConsider adding validation for zero or negative amounts before processing the mint and transfer operations.
mintAmount := sdkmath.NewIntFromBigInt(tx.Mint()) +if mintAmount.IsNegative() { + return nil, fmt.Errorf("negative mint amount not allowed: %v", mintAmount) +} recipientAddr, err := monomer.CosmosETHAddress(*tx.To()).Encode(addrPrefix) if err != nil { return nil, fmt.Errorf("evm to cosmos address: %v", err) } transferAmount := sdkmath.NewIntFromBigInt(tx.Value()) +if transferAmount.IsNegative() { + return nil, fmt.Errorf("negative transfer amount not allowed: %v", transferAmount) +}
85-85: Address TODO comment about error handling strategyThe TODO comment indicates a need to investigate error handling strategy for cross-domain message parsing and execution. This should be resolved before the code goes to production.
Would you like me to help investigate the OP Spec and propose an error handling strategy?
Line range hint
190-234: Consider transaction atomicityThe
mintETHfunction performs multiple state changes (minting and transfers). Consider wrapping these operations in a transaction to ensure atomicity and prevent partial state changes in case of failures.Consider using a pattern like:
ctx.WithTxGasMeter().GasMeter() // Start transaction scope // ... perform operations ... if err != nil { return nil, err // This should rollback all state changes }
📜 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 (13)
adapters.go(4 hunks)builder/builder_test.go(1 hunks)docs/docs/learn/deposits.md(1 hunks)mempool/mempool.go(1 hunks)mempool/mempool_test.go(1 hunks)proto/rollup/v1/tx.proto(2 hunks)x/rollup/keeper/deposits.go(2 hunks)x/rollup/keeper/msg_server.go(1 hunks)x/rollup/keeper/msg_server_test.go(3 hunks)x/rollup/module.go(1 hunks)x/rollup/tests/integration/rollup_test.go(1 hunks)x/rollup/types/deposit_tx.go(1 hunks)x/rollup/types/msgs.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- mempool/mempool.go
- mempool/mempool_test.go
- x/rollup/tests/integration/rollup_test.go
- x/rollup/types/deposit_tx.go
🔇 Additional comments (19)
x/rollup/types/msgs.go (4)
81-81: LGTM!
The Route method implementation is consistent with other message types in the module.
25-28:
Fix potential memory issue with transaction unmarshaling
The current implementation uses a value receiver for UnmarshalBinary which could lead to unexpected behavior.
Apply this diff to fix the issue:
- var tx ethtypes.Transaction
+ tx := new(ethtypes.Transaction)48-49:
Implement validation checks in ValidateBasic
The ValidateBasic method returns nil without performing any validation.
Consider implementing basic validation checks for the L1 attributes to ensure data integrity.
19-19: Verify message registration in module
The message types are properly implementing the sdktypes.Msg interface. Let's verify they're correctly registered in the module.
Also applies to: 46-46
✅ Verification successful
Let me gather more information to verify the message handling implementation.
Let me check one final aspect - the keeper implementation for message handling.
Message handling architecture is properly implemented
The verification confirms that the message handling architecture is correctly set up:
- Message interface is properly defined in protobuf with
MsgApplyUserDepositand other messages - Messages are registered via
RegisterMsgServerin the module setup - The interface registration is handled in
RegisterInterfacesthroughmsgservice.RegisterMsgServiceDesc - Message server is properly wired in the module configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for message registration in the module
rg -A 5 "RegisterLegacyAminoCodec|RegisterInterfaces" x/rollup/
# Verify message handler implementation
ast-grep --pattern 'func (k Keeper) $_(ctx $_, msg *types.MsgApplyUserDeposit) (*$_, error)'
Length of output: 1023
Script:
#!/bin/bash
# Check for message service implementation
ast-grep --pattern 'type MsgServer interface {
$$$
}'
# Look for message handler implementations in keeper
rg -A 2 "func \(k Keeper\) .*MsgApplyUserDeposit" x/rollup/
# Check protobuf service definition
fd -e proto | xargs cat
Length of output: 10948
Script:
#!/bin/bash
# Check keeper implementation for message handlers
rg -A 5 "func \(k Keeper\)" x/rollup/keeper/msg_server.go
# Verify message server setup in keeper
rg "RegisterMsgServer" x/rollup/
Length of output: 366
docs/docs/learn/deposits.md (1)
45-47: LGTM! Clear explanation of deposit transaction batching
The documentation accurately reflects the new DepositTxs terminology and clearly explains how L1 deposit transactions are batched and ordered within blocks.
proto/rollup/v1/tx.proto (2)
39-43:
Add signer option to MsgSetL1Attributes for authorization control
This message type handles critical L1 block attributes but lacks the signer option present in other message types like MsgInitiateWithdrawal and MsgUpdateParams.
Add the signer option to ensure proper authorization:
message MsgSetL1Attributes {
+ option (cosmos.msg.v1.signer) = "authority";
+
L1BlockInfo l1_block_info = 1;
bytes eth_tx = 2;
}48-51:
Add signer option and improve field documentation
The message type needs authorization control and better field documentation.
Apply these improvements:
message MsgApplyUserDeposit {
+ option (cosmos.msg.v1.signer) = "authority";
+
// tx is the marshaled Ethereum Deposit tx.
- bytes tx = 1;
+ // The transaction must conform to the Ethereum deposit transaction format
+ // as specified in the OP Stack protocol.
+ bytes tx = 1;
}x/rollup/keeper/msg_server.go (2)
17-23:
Add nil check for msg.L1BlockInfo to prevent potential panic
The method correctly handles L1 block info setting, but still needs a nil check before dereferencing msg.L1BlockInfo.
func (k *Keeper) SetL1Attributes(ctx context.Context, msg *types.MsgSetL1Attributes) (*types.MsgSetL1AttributesResponse, error) {
sdkCtx := sdk.UnwrapSDKContext(ctx)
+ if msg.L1BlockInfo == nil {
+ return nil, fmt.Errorf("L1BlockInfo cannot be nil")
+ }
if err := k.SetL1BlockInfo(sdkCtx, *msg.L1BlockInfo); err != nil {
return nil, fmt.Errorf("set l1 block info: %v", err)
}
return &types.MsgSetL1AttributesResponse{}, nil
}28-31: Consider enforcing the temporal dependency on SetL1Attributes
The comment indicates that L1BlockInfo must be set by SetL1Attributes at the start of the block. This temporal coupling could lead to runtime errors if the sequence is not enforced.
Consider:
- Adding a block height check to ensure
L1BlockInfowas set in the current block - Or, passing the required L1 block info as part of the message to remove the temporal dependency
Run this script to verify the enforcement of this sequence:
x/rollup/keeper/msg_server_test.go (4)
50-51: Use a fixed timestamp for consistent test results
Using time.Now().Unix() can lead to non-deterministic tests since the timestamp changes with each test run.
Line range hint 37-124: Well-structured test suite with comprehensive coverage
The test suite effectively covers:
- Happy path with valid deposits
- Various error scenarios
- Edge cases with different transaction types
- Failure modes in dependent services
Line range hint 125-190: LGTM! Comprehensive test coverage for withdrawal functionality
The test suite effectively covers all critical scenarios including error cases and mock interactions.
Line range hint 191-273: LGTM! Well-structured test suite for fee withdrawal
The test cases thoroughly cover the fee withdrawal functionality, including edge cases and error scenarios.
x/rollup/keeper/deposits.go (1)
83-89: Handle potential nil pointers in processCrossDomainMessage
When calling processCrossDomainMessage, if tx.Data() returns nil or the function returns nil without an error, ensure that the subsequent handling of crossDomainMessageEvent won't result in a nil pointer dereference.
builder/builder_test.go (1)
403-406: Verify test coverage for the new message type
The change from MsgApplyL1Txs to MsgSetL1Attributes reflects an architectural change in how L1 transactions are handled. While the test verifies the basic event emission, consider adding more test cases to cover:
- Edge cases for the new
MsgSetL1Attributesmessage - Error scenarios
- Validation of L1 attributes
Let's verify the test coverage for the new message type:
✅ Verification successful
Test coverage for MsgSetL1Attributes appears adequate
The test coverage for MsgSetL1Attributes is comprehensive across multiple test files:
msg_server_test.goincludes both happy path and invalid input scenariosrollup_test.gocovers integration testing with L1 block attributesbuilder_test.goverifies event emission
The test cases cover:
- ✓ Valid message handling with L1 block info
- ✓ Invalid transaction bytes scenario
- ✓ Integration with other components
- ✓ Event emission verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for MsgSetL1Attributes
# Search for all test cases involving MsgSetL1Attributes
echo "Searching for test cases involving MsgSetL1Attributes..."
rg -l "MsgSetL1Attributes" --type go --glob "*_test.go"
# Search for error handling test cases
echo "Searching for error handling test cases..."
ast-grep --pattern 'func Test$_(t *testing.T) {
$$$
MsgSetL1Attributes
$$$
require.Error(t, $_)
$$$
}'
# Check if there are validation tests
echo "Checking for validation tests..."
rg "TestValidate.*MsgSetL1Attributes" --type go --glob "*_test.go"
Length of output: 633
Script:
#!/bin/bash
# Let's examine the test coverage in the identified files
# Check the test cases in msg_server_test.go
echo "=== Examining msg_server_test.go ==="
rg "MsgSetL1Attributes" x/rollup/keeper/msg_server_test.go -B 2 -A 5
# Check the test cases in rollup_test.go
echo "=== Examining rollup_test.go ==="
rg "MsgSetL1Attributes" x/rollup/tests/integration/rollup_test.go -B 2 -A 5
# Check the test cases in builder_test.go
echo "=== Examining builder_test.go ==="
rg "MsgSetL1Attributes" builder/builder_test.go -B 2 -A 5
# Look for validation functions for MsgSetL1Attributes
echo "=== Looking for validation functions ==="
ast-grep --pattern 'func (msg *MsgSetL1Attributes) Validate() error {
$$$
}'
Length of output: 1738
adapters.go (4)
6-10: Imports are appropriately added
The required packages are imported and are utilized correctly in the code.
43-43: Correctly appending transactions
The function correctly appends the deposit transaction bytes to the non-deposit Cosmos transactions.
66-68:
Prevent potential index out of range panic by checking the length of ethDepositTxs
The function accesses ethDepositTxs[0] without verifying that ethDepositTxs is not empty. This could lead to an index out of range panic if ethDepositTxs is empty.
Apply this diff to add a check for the length of ethDepositTxs:
func packDepositTxsToCosmosTx(ethDepositTxs []hexutil.Bytes, _ string) (*rolluptypes.DepositsTx, error) {
+ if len(ethDepositTxs) == 0 {
+ return nil, fmt.Errorf("no deposit transactions provided")
+ }
var ethL1AttributesTx ethtypes.Transaction
if err := ethL1AttributesTx.UnmarshalBinary(ethDepositTxs[0]); err != nil {
return nil, fmt.Errorf("unmarshal binary: %v", err)148-149:
Prevent potential nil pointer dereference when accessing msg.L1Attributes
Before accessing msg.L1Attributes.EthTx, ensure that msg.L1Attributes is not nil to prevent a nil pointer dereference.
Apply this diff to add a nil check:
var ethL1AttributesTx ethtypes.Transaction
+ if msg.L1Attributes == nil {
+ return nil, fmt.Errorf("L1Attributes is nil")
+ }
if err := ethL1AttributesTx.UnmarshalBinary(msg.L1Attributes.EthTx); err != nil {
return nil, fmt.Errorf("unmarshal binary l1 attributes tx: %v", err)
}MsgSetL1Attributes or MsgApplyUserDeposit
joshklop
force-pushed
the
improved-deposits3
branch
from
a9cbd74
to
0859fdc
Compare
Summary by CodeRabbit
Release Notes
New Features
MsgApplyUserDepositandMsgSetL1Attributesfor handling user deposits and setting L1 attributes.DepositsTxstructure.Bug Fixes
Tests