ERC-20 Withdrawals #312
ERC-20 Withdrawals #312
Conversation
WalkthroughThe changes in this pull request introduce significant enhancements to the transaction handling and withdrawal message processing within the rollup module. Key updates include the addition of a new withdrawal message type for ERC-20 tokens, improvements to address validation, and refined error handling across various methods. The rollup's protobuf definitions have been updated to accommodate these new features, including changes to existing messages and the introduction of new RPC methods. Overall, the modifications enhance the flexibility and robustness of withdrawal operations for both ETH and ERC-20 tokens. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
natebeauregard
force-pushed
the
nkb.erc-20-withdrawals
branch
2 times, most recently
from
b360d06 to
f74ba08
Compare
![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 (12)
x/rollup/keeper/msg_server_test.go (2)
157-157: Use specific error types in mocks forBurnCoinsfailuresThe mock setup for
BurnCoinsreturnssdkerrors.ErrUnknownRequest, which is a generic error. For clearer test scenarios, consider returning a more specific error related to burning coins, such astypes.ErrBurnETH, to better reflect the failure condition being tested.s.bankKeeper.EXPECT().BurnCoins(s.ctx, types.ModuleName, gomock.Any()). - Return(sdkerrors.ErrUnknownRequest).AnyTimes() + Return(types.ErrBurnETH).AnyTimes()
171-193: Ensure comprehensive assertions for both ETH and ERC20 withdrawalsWhile both ETH and ERC20 withdrawal responses are checked for errors and non-nil responses, consider adding separate assertions for each to ensure that both scenarios are thoroughly validated. This enhances the robustness of the test by explicitly confirming the expected outcomes of each withdrawal type.
// Existing assertions s.Require().NoError(ethWithdrawalErr) s.Require().NoError(erc20WithdrawalErr) s.Require().NotNil(ethWithdrawalResp) s.Require().NotNil(erc20WithdrawalResp) // Consider adding specific assertions or additional checks as neededx/rollup/keeper/msg_server.go (2)
55-55: Clarify logging message for ETH withdrawalsThe debug log message specifies "Withdrawing ETH L2 assets". To improve clarity, consider rephrasing it to "Initiating ETH withdrawal from L2" to more accurately reflect the action being performed.
ctx.Logger().Debug( - "Withdrawing ETH L2 assets", + "Initiating ETH withdrawal from L2", "sender", msg.Sender, "value", msg.Value )
88-176: Ensure consistent error handling inInitiateERC20WithdrawalmethodWhile the method correctly wraps errors with
types.WrapError, verify that all potential error points use consistent and descriptive error messages. This improves maintainability and aids in debugging.For example, in line 97:
return nil, types.WrapError(types.ErrInvalidSender, "invalid sender address: %v; error: %v", msg.Sender, err)Ensure all error wraps provide clear context.
builder/builder.go (2)
294-296: IncludeMsgInitiateERC20Withdrawalin withdrawal message typesThe variable names for message types can be improved for clarity. Consider defining a slice of withdrawal message types to streamline the condition checking.
withdrawalMsgTypes := []string{ cdctypes.MsgTypeURL(new(rolluptypes.MsgInitiateWithdrawal)), cdctypes.MsgTypeURL(new(rolluptypes.MsgInitiateERC20Withdrawal)), cdctypes.MsgTypeURL(new(rolluptypes.MsgInitiateFeeWithdrawal)), } if slices.Contains(withdrawalMsgTypes, msg.TypeUrl) { // Existing logic }
366-375: Simplify sender address parsing logicThe current logic checks if the sender address is a hex address before attempting to parse it as a Bech32 address. Considering the sender can be either an Ethereum address or a Cosmos address, ensure that this approach reliably handles all valid address formats.
Consider using a helper function to encapsulate this logic for reusability and clarity.
x/rollup/types/events.go (1)
24-24: Add comment for the newEventTypeBurnERC20constantFor consistency and maintainability, consider adding a brief comment explaining the purpose of the
EventTypeBurnERC20constant, similar to other constants in the file.EventTypeBurnERC20 = "burn_erc20" // Event type for burning ERC-20 tokensx/rollup/types/errors.go (2)
22-23: Correct grammar in error message forErrUpdateParamsThe error message for
ErrUpdateParamshas a grammatical error. Changing "failed to update params" to "failed to update parameters" improves clarity.ErrUpdateParams = registerErr( - "failed to update params" + "failed to update parameters" )
22-22: Ensure consistent error definitionsThe newly added
ErrInitiateERC20Withdrawalerror should include appropriate context in its message. Verify that it follows the pattern used by the other error definitions.Consider revising the error message for clarity:
ErrInitiateERC20Withdrawal = registerErr("failed to initiate ERC-20 token withdrawal")x/rollup/types/params.go (1)
15-16: Consider environment-specific default addressesThe hardcoded devnet address might not be suitable for all environments (mainnet, testnet, etc.).
Consider making the default address configurable based on the network environment.
x/rollup/keeper/withdrawals.go (1)
27-47: Consider adding recovery mechanism for failed burnsThe current implementation might leave the system in an inconsistent state if the burn fails after the transfer succeeds.
Consider implementing one of these solutions:
- Use a transactional approach to ensure atomicity
- Add a recovery mechanism for failed burns
- Implement a two-phase commit process
x/rollup/types/msgs.go (1)
112-112: Track TODO comment in issue trackerThe missing validations for
MsgUpdateParamsandMsgInitiateFeeWithdrawalshould be tracked and implemented to ensure proper message validation.Would you like me to create a GitHub issue to track the implementation of these validations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/rollup/types/rollup.pb.gois excluded by!**/*.pb.gox/rollup/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
builder/builder.go(2 hunks)proto/rollup/v1/rollup.proto(1 hunks)proto/rollup/v1/tx.proto(3 hunks)x/rollup/keeper/deposits.go(1 hunks)x/rollup/keeper/keeper.go(1 hunks)x/rollup/keeper/keeper_test.go(1 hunks)x/rollup/keeper/msg_server.go(3 hunks)x/rollup/keeper/msg_server_test.go(4 hunks)x/rollup/keeper/withdrawals.go(1 hunks)x/rollup/tests/integration/rollup_test.go(3 hunks)x/rollup/types/errors.go(1 hunks)x/rollup/types/events.go(1 hunks)x/rollup/types/msgs.go(1 hunks)x/rollup/types/params.go(2 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
x/rollup/keeper/msg_server_test.go
132-132: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (11)
x/rollup/keeper/msg_server_test.go (1)
197-204: Verify that expectedEventTypes aligns with emitted events
The expectedEventTypes slice includes event types for both ETH and ERC20 withdrawals. Ensure that the order and types of events in expectedEventTypes match exactly with the events emitted during the test execution. This prevents potential index out-of-range errors and ensures accurate validation.
Run the following script to ensure that the emitted events match the expected event types:
✅ Verification successful
Let me try a different approach to verify the event types by searching for the actual event emission in the test file.
Let me try one more approach to find the event emissions by looking at the test implementation and the handler code.
Let me check the actual event emission in the message server implementation.
Based on the test implementation context we found earlier, I can now provide a final response:
The expectedEventTypes order matches the event emission sequence
From the test implementation, we can see two test cases:
- Single withdrawal test case expects exactly 3 events:
sdk.EventTypeMessage,types.EventTypeWithdrawalInitiated, andtypes.EventTypeBurnETH - Combined test case expects 6 events in the correct order for both ETH and ERC20 withdrawals
The test validates the order by iterating through events and comparing types at matching indices, ensuring proper sequence validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare expected and actual event types in TestInitiateWithdrawal
# Expected event types
echo "Expected Event Types:"
echo -e "sdk.EventTypeMessage\nrollup.EventTypeWithdrawalInitiated\nrollup.EventTypeBurnETH\nsdk.EventTypeMessage\nrollup.EventTypeWithdrawalInitiated\nrollup.EventTypeBurnERC20"
# Actual emitted events
echo "Actual Emitted Event Types:"
grep -o 'Type:.*' x/rollup/keeper/msg_server_test.go | sed 's/Type://g'
Length of output: 534
Script:
#!/bin/bash
# Search for event emission patterns in the test file
echo "Searching for event emissions in the test file:"
rg -A 2 "EmitEvent|NewEvent" x/rollup/keeper/msg_server_test.go
echo -e "\nSearching for event type declarations:"
rg "EventType.*=" x/rollup/keeper/msg_server_test.go
Length of output: 390
Script:
#!/bin/bash
# Get the full test context
echo "Test implementation context:"
rg -B 5 -A 10 "expectedEventTypes.*:=.*\[" x/rollup/keeper/msg_server_test.go
echo -e "\nHandler implementation context:"
ast-grep --pattern 'func (k *Keeper) InitiateWithdrawal($$$) {
$$$
}'
Length of output: 1237
Script:
#!/bin/bash
# Search for the message server implementation
echo "Message server implementation:"
rg -B 2 -A 15 "func.*InitiateWithdrawal.*context.Context.*sdk.Msg.*" x/rollup/keeper/msg_server.go
echo -e "\nERC20 withdrawal implementation:"
rg -B 2 -A 15 "func.*InitiateERC20Withdrawal.*context.Context.*sdk.Msg.*" x/rollup/keeper/msg_server.go
Length of output: 358
x/rollup/keeper/msg_server.go (1)
153-173: Double-check the correctness of ABI encoding and event emission
Ensure that the ABI encoding for finalizeBridgeERC20 and the subsequent relayMessage is accurate and aligns with the expected contract interfaces. Additionally, verify that event attributes like types.AttributeKeySender use the correct addresses as per protocol specifications.
To confirm, review the ABI definitions and cross-reference with the contract implementations.
proto/rollup/v1/rollup.proto (2)
14-15: Verify L1 standard bridge contract address format
The new field l1_standard_bridge should be properly validated to ensure it's a valid Ethereum address.
Consider adding a comment specifying the expected format (e.g., "0x" prefixed hex string of length 42).
17-17:
Caution: Proto field numbers have been reordered
The field numbers for min_fee_withdrawal_amount and fee_withdrawal_gas_limit have been changed from 3->4 and 4->5 respectively. This change requires careful consideration:
- Ensure all clients are updated to handle the new field numbers
- Consider potential data migration needs
- Verify no breaking changes in serialization/deserialization
Run the following to check for other proto message references:
Also applies to: 19-19
x/rollup/types/msgs.go (1)
77-77: Verify the impact of message type change
The message type has been made more descriptive, but this is a breaking change that could affect existing integrations.
✅ Verification successful
Let's search for both the new message type and common patterns where message types are typically handled in blockchain codebases. This will help us understand if there are any dependencies on this message type and how it's being processed.
Let's search more specifically in codec files where message types are typically registered, keeper files where they're handled, protobuf definitions, and test files to get a complete picture of the message type usage.
Based on the comprehensive search results, I can now provide the final response:
Message type change appears safe to proceed
The message type change from "l2withdrawal" to "l2_eth_withdrawal" appears safe as:
- No references to the old "l2withdrawal" type were found in the codebase
- The message type is only used internally in the
MsgInitiateWithdrawal.Type()method - The message handling is done through protobuf service definitions (as seen in rollup/v1/msgs.proto) rather than through string type matching
- No test files were found using either the old or new message type string
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct string references to the old message type
rg -l "l2withdrawal" --type go
# Search for any message type handling in the codebase
ast-grep --pattern 'Type() string { return "l2withdrawal" }'
Length of output: 93
Script:
#!/bin/bash
# Search for any references to the new message type
rg "l2_eth_withdrawal" --type go -A 3 -B 3
# Search for any message type handling or switch/case statements that might process message types
ast-grep --pattern 'case "$_": $$$'
# Search for any message type registration or handling patterns
rg "RegisterConcrete|HandleMessage|ProcessMessage" --type go -A 3 -B 3
Length of output: 441
Script:
#!/bin/bash
# Search for message type registration in codec files
fd -e go codec.go -x rg . -A 5 -B 5
# Search for message handling in keeper files
fd -e go keeper.go -x rg . -A 5 -B 5
# Look for any protobuf definitions that might reference this message type
fd -e proto -x rg . -A 5 -B 5
# Search for test files that might be using this message type
rg "l2withdrawal|l2_eth_withdrawal" --type go --glob "*_test.go" -A 3 -B 3
Length of output: 15995
x/rollup/keeper/keeper.go (1)
106-110:
Improve robustness of ERC20 denomination helper
The current implementation has several potential issues:
- No validation of input address format
- Assumes address starts with "0x"
- Missing test coverage
// getERC20Denom returns the Monomer L2 coin denom for the given ERC-20 L1 address.
// The "erc20/{l1erc20addr}" format is used for the L2 coin denom.
+// The input address must be a valid hex address starting with "0x".
func getERC20Denom(erc20Addr string) string {
+ if len(erc20Addr) < 2 || erc20Addr[:2] != "0x" {
+ panic(fmt.Sprintf("invalid ERC20 address format: %s", erc20Addr))
+ }
return "erc20/" + erc20Addr[2:]
}x/rollup/keeper/keeper_test.go (2)
64-67: LGTM! Function rename reflects broader usage
The rename from mockBurnETH to mockBurn appropriately reflects its usage for both ETH and ERC20 tokens.
69-72: LGTM! Function rename reflects broader usage
The rename from mockMintETH to mockMint appropriately reflects its usage for both ETH and ERC20 tokens.
proto/rollup/v1/tx.proto (2)
26-27: LGTM: New RPC method follows protobuf conventions.
The InitiateERC20Withdrawal RPC method is well-defined and properly integrated into the Msg service.
84-108: LGTM: New message types are well-structured.
The MsgInitiateERC20Withdrawal and its response type are properly defined with:
- Clear field documentation
- Appropriate cosmos-sdk scalar types
- Proper field numbering
- Consistent naming conventions
x/rollup/keeper/deposits.go (1)
249-249: LGTM: Improved code maintainability.
Good refactoring to use getERC20Denom function, which centralizes the ERC20 denomination logic and makes it easier to modify the format in the future.
Add a message type to initiate ERC-20 withdrawals on L2. The ERC-20 token will be burned on L2 and will encode messages to send to the L1CrossDomainMessenger and L1StandardBridge to release the deposited ERC-20 token on L1 after the withdrawal is successfully proven and finalized.
natebeauregard
force-pushed
the
nkb.erc-20-withdrawals
branch
from
8abcc73 to
7a80b04
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
x/rollup/keeper/withdrawals.go (2)
28-47: Consider implementing transaction atomicityThe burn operation should be atomic to prevent partial state changes in case of failures.
Consider wrapping the operations in a transaction:
func (k *Keeper) burnERC20(...) error { + return k.storeService.OnWrite(ctx.Context(), func(ctx context.Context) error { coins := sdk.NewCoins(sdk.NewCoin(getERC20Denom(erc20addr), amount)) if err := k.bankkeeper.SendCoinsFromAccountToModule(...) { return err } return k.bankkeeper.BurnCoins(...) + }) }
35-40: Add logging for withdrawal operationsConsider adding structured logging to track withdrawal operations for better observability.
coins := sdk.NewCoins(sdk.NewCoin(getERC20Denom(erc20addr), amount)) +k.Logger(ctx).Info("initiating ERC20 withdrawal", + "user", userAddr.String(), + "token", erc20addr, + "amount", amount.String())builder/builder.go (1)
Line range hint
366-402: Add validation for required withdrawal attributesThe
parseWithdrawalEventAttributesfunction should validate that all required attributes are present before returning the withdrawal parameters. Missing attributes could lead to incomplete or invalid withdrawals.Consider adding validation:
func parseWithdrawalEventAttributes(withdrawalEvent *abcitypes.Event) (*crossdomain.Withdrawal, error) { var params crossdomain.Withdrawal + requiredAttributes := map[string]bool{ + rolluptypes.AttributeKeySender: false, + rolluptypes.AttributeKeyL1Target: false, + rolluptypes.AttributeKeyValue: false, + rolluptypes.AttributeKeyGasLimit: false, + } for _, attr := range withdrawalEvent.Attributes { switch attr.Key { case rolluptypes.AttributeKeySender: + requiredAttributes[attr.Key] = true // ... existing code ... case rolluptypes.AttributeKeyL1Target: + requiredAttributes[attr.Key] = true // ... existing code ... case rolluptypes.AttributeKeyValue: + requiredAttributes[attr.Key] = true // ... existing code ... case rolluptypes.AttributeKeyGasLimit: + requiredAttributes[attr.Key] = true // ... existing code ... } } + for attr, found := range requiredAttributes { + if !found { + return nil, fmt.Errorf("missing required attribute: %s", attr) + } + } return ¶ms, nil }x/rollup/keeper/msg_server.go (1)
93-93: Enhance logging with hex-formatted addressesThe debug log should use hex-formatted addresses for better readability and consistency with Ethereum conventions.
-ctx.Logger().Debug("Withdrawing ERC-20 L2 assets", "sender", msg.Sender, "value", msg.Value, "tokenAddress", msg.TokenAddress) +ctx.Logger().Debug("Withdrawing ERC-20 L2 assets", "sender", msg.Sender, "value", msg.Value, "tokenAddress", common.HexToAddress(msg.TokenAddress).Hex())x/rollup/tests/integration/rollup_test.go (1)
Line range hint
201-201: Use a valid Ethereum address for test dataThe ERC20 token address used in tests is not a valid Ethereum address.
Use a properly formatted Ethereum address:
-erc20TokenAddress := "0x0123456789abcdef" +erc20TokenAddress := "0x0123456789abcdef0123456789abcdef01234567"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/rollup/types/rollup.pb.gois excluded by!**/*.pb.gox/rollup/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
builder/builder.go(2 hunks)proto/rollup/v1/rollup.proto(1 hunks)proto/rollup/v1/tx.proto(3 hunks)x/rollup/keeper/deposits.go(1 hunks)x/rollup/keeper/keeper.go(1 hunks)x/rollup/keeper/keeper_test.go(1 hunks)x/rollup/keeper/msg_server.go(3 hunks)x/rollup/keeper/msg_server_test.go(5 hunks)x/rollup/keeper/withdrawals.go(1 hunks)x/rollup/tests/integration/rollup_test.go(3 hunks)x/rollup/types/errors.go(1 hunks)x/rollup/types/events.go(1 hunks)x/rollup/types/msgs.go(1 hunks)x/rollup/types/params.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- x/rollup/types/events.go
- x/rollup/types/params.go
- x/rollup/keeper/keeper_test.go
- x/rollup/types/errors.go
- x/rollup/keeper/deposits.go
- x/rollup/types/msgs.go
- proto/rollup/v1/rollup.proto
🧰 Additional context used
🪛 Gitleaks (8.21.2)
x/rollup/keeper/msg_server_test.go
201-201: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (13)
proto/rollup/v1/tx.proto (1)
93-104: Ensure consistent naming with L1 bridge contracts
The field names should align with the L1 bridge contract parameter names for better maintainability:
token_addressshould match the L1StandardBridge parameter nameextra_datashould match the L1CrossDomainMessenger parameter name
x/rollup/keeper/withdrawals.go (2)
28-34: Add input validation for ERC-20 address and amount
The previous review comment about input validation is still applicable. Please implement the suggested validations for the ERC-20 address format and amount.
35-44: Enhance error handling and add event emission
The previous review comment about error handling and event emission is still applicable. Please implement the suggested improvements for better error messages and event tracking.
builder/builder.go (2)
294-296: LGTM: ERC-20 withdrawal message type integration
The addition of erc20WithdrawalMsgType and its inclusion in the withdrawal message check is well-implemented and consistent with existing withdrawal types.
366-375: Verify L2CrossDomainMessenger address handling
The implementation correctly handles both Ethereum and Cosmos address formats for different withdrawal types. However, let's verify the L2CrossDomainMessenger address usage for ERC-20 withdrawals.
#!/bin/bash
# Description: Verify L2CrossDomainMessenger address usage in ERC-20 withdrawals
# Search for L2CrossDomainMessenger address definition and usage
rg -A 5 "L2CrossDomainMessenger"
# Search for ERC-20 withdrawal message handling to verify address conversion
ast-grep --pattern 'MsgInitiateERC20Withdrawal'The previous review comment about validating zero addresses after conversion is still applicable. Consider adding the suggested validation:
if common.IsHexAddress(attr.Value) {
sender = common.HexToAddress(attr.Value)
+ if sender == (common.Address{}) {
+ return nil, fmt.Errorf("invalid sender address after conversion")
+ }
} else {x/rollup/keeper/msg_server.go (2)
110-113: Add nil check for params
As noted in previous reviews, we should add a nil check for params.
Add nil check after getting params:
params, err := k.GetParams(ctx)
if err != nil {
return nil, types.WrapError(types.ErrInitiateERC20Withdrawal, "failed to get params: %v", err)
}
+if params == nil {
+ return nil, types.WrapError(types.ErrInitiateERC20Withdrawal, "params is nil")
+}141-148: 🛠️ Refactor suggestion
Consider gas limit validation
The relayMessage is packed without validating the gas limit, which could lead to failed transactions on L1.
Add gas limit validation:
+// Validate minimum gas limit
+if len(msg.GasLimit) == 0 || new(big.Int).SetBytes(msg.GasLimit).Cmp(big.NewInt(21000)) < 0 {
+ return nil, types.WrapError(types.ErrInitiateERC20Withdrawal, "gas limit must be at least 21000")
+}x/rollup/tests/integration/rollup_test.go (1)
133-144: Add error test cases for ERC20 withdrawals
The test only covers the success case. As noted in previous reviews, we need additional error cases.
Add test cases for error scenarios:
+// try to withdraw more tokens than deposited
+_, err = integrationApp.RunMsg(&rolluptypes.MsgInitiateERC20Withdrawal{
+ Sender: erc20userCosmosAddr,
+ Target: l1WithdrawalAddr,
+ TokenAddress: erc20tokenAddr.String(),
+ Value: math.NewIntFromBigInt(erc20depositAmount).Add(math.OneInt()),
+ GasLimit: new(big.Int).SetUint64(100_000_000).Bytes(),
+})
+require.Error(t, err)
+
+// try to withdraw with invalid token address
+_, err = integrationApp.RunMsg(&rolluptypes.MsgInitiateERC20Withdrawal{
+ Sender: erc20userCosmosAddr,
+ Target: l1WithdrawalAddr,
+ TokenAddress: "invalid_address",
+ Value: math.NewIntFromBigInt(erc20depositAmount),
+ GasLimit: new(big.Int).SetUint64(100_000_000).Bytes(),
+})
+require.Error(t, err)x/rollup/keeper/msg_server_test.go (5)
107-107: LGTM: Generalized mock naming
The rename from mockMintETH to mockMint appropriately reflects the generalization needed to support both ETH and ERC20 tokens.
134-134: Consider splitting ETH and ERC20 withdrawal tests
The //nolint:dupl comment suggests significant code duplication between ETH and ERC20 withdrawal tests. Splitting these into separate test files would improve readability and maintenance.
150-150: Return appropriate error types in mocks
Using types.ErrBurnETH for a SendCoinsFromAccountToModule failure doesn't accurately represent the error scenario. Consider using sdkerrors.ErrInsufficientFunds instead.
201-201: Use a valid Ethereum address for erc20TokenAddress
The current address is only 16 characters long (excluding '0x'). Ethereum addresses should be 40 characters long (excluding '0x').
🧰 Tools
🪛 Gitleaks (8.21.2)
201-201: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
204-266: LGTM: Comprehensive test coverage for ERC20 withdrawals
The test cases effectively cover:
- Successful withdrawal scenario
- Invalid sender address
- Insufficient funds
- Burn operation failures
- Correct event emission sequence including ERC20-specific events
Add a message type to initiate ERC-20 withdrawals on L2. The ERC-20 token will be burned on L2 and will encode messages to send to the L1CrossDomainMessenger and L1StandardBridge to release the deposited ERC-20 token on L1 after the withdrawal is successfully proven and finalized.
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Bug Fixes
Tests