Support ERC-20 deposits in x/rollup #223
Conversation
WalkthroughThis pull request introduces significant enhancements to the handling of ERC20 token deposits and cross-domain messaging within the Ethereum ecosystem. Key changes include the addition of new data structures for managing token transfers, modifications to the stack configuration for improved modularity, and updates to testing frameworks to ensure comprehensive coverage of both ETH and ERC20 transactions. The integration of new functions for parsing cross-domain messages and minting tokens further strengthens the interoperability between Ethereum and Cosmos. Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
natebeauregard
force-pushed
the
nkb.erc20-deposits
branch
from
d3f7ea4 to
aef7d65
Compare
|
Force-pushed to rebase on main to get the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
testutils/utils.go (1)
89-128: LGTM!The
GenerateERC20DepositTxfunction correctly generates an ERC20 deposit transaction using the Ethereum ABI to pack the necessary data. The logic is sound and the implementation is straightforward.Just a reminder to address the TODO comment in the future:
// TODO: remove hardcoded address once a genesis state is configured // L2 aliased L1CrossDomainMessenger proxy address From: crossdomain.ApplyL1ToL2Alias(common.HexToAddress("0x9A9f2CCfdE556A7E9Ff0848998Aa4a0CFD8863AE")),Replace the hardcoded L1CrossDomainMessenger proxy address with a proper genesis state configuration when available.
x/rollup/keeper/deposits.go (3)
30-30: Address the TODO comment.Please consider switching from JSON to binary marshaling for better performance and storage efficiency.
105-108: Address the TODO comment.Please remove the hardcoded L1CrossDomainMessenger address once a genesis state is configured.
215-218: Suggestion: Move the helper function to the utils package in a separate PR.The past review comment suggests moving the
unpackInputsIntoInterfacehelper function to the utils package. While this is a valid suggestion, it can be addressed in a separate PR to keep this PR focused on the ERC-20 deposit functionality.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- bindings/erc20_deposit_args.go (1 hunks)
- e2e/stack.go (4 hunks)
- e2e/stack_test.go (13 hunks)
- testutils/utils.go (3 hunks)
- x/rollup/keeper/deposits.go (4 hunks)
- x/rollup/tests/integration/rollup_test.go (4 hunks)
- x/rollup/types/events.go (1 hunks)
Additional comments not posted (24)
bindings/erc20_deposit_args.go (2)
9-16: LGTM!The
RelayMessageArgsstruct is well-defined with appropriate fields and types for relaying messages on the blockchain. The struct is exported, which aligns with its intended usage across the codebase.
18-25: LGTM!The
FinalizeBridgeERC20Argsstruct is well-defined with appropriate fields and types for finalizing ERC20 token transfers between different blockchain environments. The struct is exported, which aligns with its intended usage across the codebase.x/rollup/types/events.go (2)
14-14: LGTM!The new constant
AttributeKeyERC20Addressaligns with the PR objective of supporting ERC-20 token deposits. It follows the naming convention of other attribute keys in the file and will likely be used to store or reference the address of an ERC-20 token in events.
19-19: LGTM!The new constant
EventTypeMintERC20aligns with the PR objective of supporting ERC-20 token deposits and minting on Layer 2. It follows the naming convention of other event types in the file and will be used to indicate the minting of ERC-20 tokens.x/rollup/tests/integration/rollup_test.go (6)
26-26: LGTM!The import statement for the
utilspackage from themonomermodule is valid and necessary.
38-41: LGTM!The variable declarations for the ERC20 token address, user address, and deposit amount are valid and necessary for the test case. The variable names are descriptive and follow the naming conventions.
43-48: LGTM!The code correctly generates an ERC20 deposit transaction using the
GenerateERC20DepositTxfunction and converts it to bytes using theTxToBytesfunction. This is necessary for testing the ERC20 deposit functionality in the test case.
56-58: LGTM!Querying the user's ERC20 balance before the deposit and asserting that it is zero is a good practice. It ensures that the test case starts with the expected initial state of the user's ERC20 balance.
68-77: LGTM!Including the ERC20 deposit transaction bytes in the
MsgApplyL1Txsmessage is necessary to process the ERC20 deposit. Querying the user's ERC20 balance after the deposit and asserting that it is equal to the deposit amount is a good practice to verify that the deposit was processed correctly.
156-171: LGTM!Refactoring the
queryUserBalancefunction to accept adenomparameter makes it more generic and reusable for querying balances of different token types. Adding thequeryUserERC20Balancefunction is necessary for querying the user's ERC20 balance in the test case. The code changes improve the flexibility and reusability of the balance querying functions.testutils/utils.go (1)
6-6: Looks good!The import statement for the
"strings"package is correctly added to the import block. The package is used appropriately in theGenerateERC20DepositTxfunction.e2e/stack.go (2)
48-50: LGTM! The modular design improves separation of concerns.The changes to the
StackConfigstruct, replacing theL1Portalfield withL1Deployments,OptimismPortal, andL1StandardBridge, indicate a shift towards a more modular design. This separation of concerns related to different components of the Optimism stack enhances the overall architecture and improves integration with Optimism's components.
165-169: Excellent! The L1 standard bridge integration improves interoperability.The instantiation of a new
L1StandardBridgeusing theopbindingspackage enhances the functionality of the stack. By integrating this new bridge component and passing theope2econfig.L1Deployments.L1StandardBridgeProxyaddress, the stack is now capable of interacting with the L1 standard bridge. This change improves interoperability with Ethereum Layer 1.The error handling for the creation of the
L1StandardBridgeis also a good practice, ensuring that any issues during instantiation are properly reported.x/rollup/keeper/deposits.go (5)
Line range hint
42-61: LGTM!The function logic is correct, and the error handling is appropriate.
126-166: LGTM!The function logic is correct, and it handles unrecognized messages gracefully by returning nil and not erroring.
The past review comment about swallowing errors for unrecognized messages has been addressed and can be marked as resolved.
Line range hint
169-186: LGTM!The function logic is correct, and the updated error messages provide better context.
187-213: LGTM!The function logic is correct, and the coin denom format is appropriate for bridged ERC-20 tokens.
111-120: Verify the cross-domain message handling.The cross-domain message handling logic looks correct. However, please ensure that the
parseAndExecuteCrossDomainMessagefunction is thoroughly tested to handle all possible scenarios.Run the following script to verify the cross-domain message handling:
e2e/stack_test.go (6)
Line range hint
239-450: Comprehensive and well-structured ETH deposit and withdrawal tests.The changes to the
ethRollupFlowfunction are comprehensive and well-structured. The function thoroughly tests the ETH deposit and withdrawal flows by interacting with the L1 and L2 chains, waiting for transactions to be processed, and verifying the expected behavior through balance checks and event emissions.Some additional suggestions to further improve the function:
- Consider extracting the deposit and withdrawal logic into separate functions to improve readability and maintainability.
- Add more detailed comments explaining the purpose of each section of the code to make it easier for other developers to understand the test flow.
451-540: New test function for ERC-20 deposits.The new
erc20RollupFlowfunction is a good addition to the test suite. It covers the important aspects of the ERC-20 deposit flow, including deploying the WETH9 contract on L1, minting tokens to the user, approving the token transfer, bridging the tokens to L2, and verifying that the tokens are minted on L2.Some suggestions for further improvement:
- Add comments to explain the purpose of each section of the code to make it easier for other developers to understand the test flow.
- Consider extending the function to test ERC-20 withdrawals in a future PR to ensure that the full ERC-20 bridge flow is covered.
566-577: New function to check if ERC-20 tokens are minted on L2.The new
requireERC20IsMintedfunction is a useful addition to the test suite. It helps to ensure that ERC-20 tokens are properly minted on L2 by searching for themint_erc20event with the specified attributes.The function is well-structured and uses clear naming conventions for the event attributes, making it easy to understand its purpose.
One minor suggestion:
- Add a comment to explain the purpose of the function to make it even clearer for other developers.
Line range hint
578-592: LGTM!The
l2TxSearchfunction is a useful utility for searching transactions on L2. It is well-structured, uses clear variable names, and handles errors appropriately by usingrequire.NoError.
Line range hint
593-619: LGTM!The
createL1TransactOptsfunction is a useful utility for creating transaction options for L1 transactions. It is well-structured, uses clear variable names, and handles errors appropriately by usingrequire.NoError.
Line range hint
632-651: Useful utility for waiting for L2 output proposals.The
waitForL2OutputProposalfunction is a useful utility for waiting for L2 output proposals containing withdrawal transactions. It is well-structured, uses clear variable names, and handles errors appropriately by usingrequire.NoError.One suggestion for improvement:
- Consider adding a timeout to the waiting loop to prevent it from waiting indefinitely in case of issues with the L2 output proposals. This could help to fail the test faster in case of problems and provide better visibility into potential issues.
natebeauregard
force-pushed
the
nkb.erc20-deposits
branch
from
ec66856 to
23f31e0
Compare
|
Force-pushed to rebase on main and resolve merge conflicts |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
x/rollup/keeper/deposits.go (2)
30-30: Reminder: Address the TODO comment.The TODO comment indicates that binary marshaling should be used instead of JSON marshaling for better efficiency. Please ensure this is addressed in a future update.
Line range hint
63-124: LGTM with a reminder!The function logic is correct, and the implementation is accurate. The error handling and logging are appropriate. The logic to check for cross-domain messages and call the
parseAndExecuteCrossDomainMessagefunction is correct.Please ensure the TODO comments are addressed once a genesis state is configured:
- Remove the logic to get the from address once Refactor L1 user deposit processing in x/rollup module #208 is merged.
- Remove the hardcoded address once a genesis state is configured.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/rollup/keeper/deposits.go (4 hunks)
Additional comments not posted (7)
x/rollup/keeper/deposits.go (7)
Line range hint
41-60: LGTM!The function logic is correct, and the implementation is accurate. The error handling and logging are appropriate.
130-167: LGTM!The function logic is correct, and the implementation is accurate. The error handling and logging are appropriate. The logic to recognize
finalizeBridgeERC20messages from the L1StandardBridge and call themintERC20function is correct. It correctly returns nil and does not error if a message is not recognized.The function can be extended to recognize other messages in the future if needed.
Line range hint
170-187: LGTM!The function logic is correct, and the implementation is accurate. The error handling for minting and sending coins is appropriate. The
MintETHevent is created correctly.
190-214: LGTM!The function logic is correct, and the implementation is accurate. The error handling for minting and sending coins is appropriate. The
MintERC20event is created correctly. The "erc20/{l1erc20addr}" format for the coin denom is appropriate.
216-244: LGTM!The function logic is correct, and the implementation is accurate. The error handling for unpacking input data and setting fields is appropriate. The logic to check if the function selector matches the method ID is correct.
This function can be used in other parts of the codebase where unpacking method inputs is needed.
216-219: Duplicate comment: Any thoughts from reviewers on if this helper func would be better in the utils package?This comment was made by
natebeauregardon a previous commit and is still valid for the current changes.
130-130: Duplicate comment: Should we just swallow all errors here? Or should we only swallow errors for unrecognized L1StandardBridge messages?This comment was made by
natebeauregardon a previous commit and is still valid for the current changes.In my opinion, we should only swallow errors for unrecognized L1StandardBridge messages. This would allow other errors, such as parsing errors or errors from the
mintERC20function, to be propagated and handled appropriately. Swallowing all errors could hide potential issues and make debugging harder.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
x/rollup/keeper/deposits.go (2)
Line range hint
31-41: Consider using a binary serialization format like protobuf for L1 block info.Marshaling the L1 block info to JSON can be inefficient in terms of performance and size compared to binary serialization formats like protobuf.
As mentioned in the TODO comment, consider transitioning from JSON to binary marshaling for L1 block information to improve performance and reduce the storage size.
114-116: Address the TODO comment about removing the hardcoded address.The hardcoded L1CrossDomainMessenger address could lead to issues if the address changes in the future.
Consider creating an issue to track the removal of the hardcoded address once a genesis state is configured, as mentioned in the TODO comment.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- x/rollup/keeper/deposits.go (3 hunks)
- x/rollup/tests/integration/rollup_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/rollup/tests/integration/rollup_test.go
Additional comments not posted (5)
x/rollup/keeper/deposits.go (5)
Line range hint
44-64: LGTM!The function is correctly processing the L1 Attributes tx and deriving the L1 block info from the tx data. The error handling and logging are appropriate.
122-125: Investigate error handling for cross-domain messages.The TODO comment mentions investigating when to return an error if a cross-domain message can't be parsed or executed.
Consult the Optimism specs to determine the appropriate error handling behavior for cross-domain messages that can't be parsed or executed.
137-174: Investigate error handling for unrecognized cross-domain messages.The function currently only recognizes
finalizeBridgeERC20messages from the L1StandardBridge, and other types of cross-domain messages are not handled.As mentioned in the TODO comment, investigate when to return an error if a cross-domain message can't be parsed or executed. Consider creating an issue to track this investigation and determine the appropriate error handling behavior for unrecognized cross-domain messages based on the Optimism specs.
Line range hint
177-194: LGTM!The function is correctly minting ETH to the user's account and returning a
MintETHevent with the appropriate attributes. The error handling is adequate.
197-221: LGTM!The function is correctly minting the bridged ERC-20 token to the user's account using the specified denom format and returning a
MintERC20event with the appropriate attributes. The error handling is adequate.
natebeauregard
force-pushed
the
nkb.erc20-deposits
branch
from
c751040 to
db339b7
Compare
|
Force-pushed to format commits for merge |
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- bindings/erc20_deposit_args.go (1 hunks)
- e2e/stack.go (4 hunks)
- e2e/stack_test.go (13 hunks)
- testutils/utils.go (3 hunks)
- utils/utils.go (2 hunks)
- x/rollup/keeper/deposits.go (3 hunks)
- x/rollup/tests/integration/rollup_test.go (4 hunks)
- x/rollup/types/events.go (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- bindings/erc20_deposit_args.go
- e2e/stack.go
- e2e/stack_test.go
- testutils/utils.go
- x/rollup/keeper/deposits.go
- x/rollup/tests/integration/rollup_test.go
- x/rollup/types/events.go
Adds support for ERC-20 deposits by parsing the
deposit tx data. ERC-20 tokens are minted on L2
in the "erc20/{l1erc20addr}" format.
Safeguards are in place to ensure that the deposit
tx originated from the L1CrossDomainMessenger so
that users can't forge tx data to mint unlimited
ERC-20 tokens.
natebeauregard
force-pushed
the
nkb.erc20-deposits
branch
from
db339b7 to
2d5b47e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
x/rollup/keeper/deposits.go (1)
228-255: Consider moving theunpackInputsIntoInterfacefunction to a separate utils package.This function is a generic helper for unpacking method input data and is not specific to the rollup module. Moving it to a dedicated utils package would improve code organization and reusability across different modules.
e2e/stack_test.go (1)
450-536: LGTM!The
erc20RollupFlowfunction is a great addition to the test suite. It correctly tests the ERC-20 deposit flow, using the appropriate contracts and methods, and includes appropriate waiting mechanisms and assertions to ensure the correctness of the flow.The function is well-structured, easy to follow, and well-documented with comments.
One minor suggestion for improvement:
Consider extracting the ERC-20 contract deployment and token minting logic into a separate helper function to improve readability and reusability. This would allow the
erc20RollupFlowfunction to focus solely on testing the ERC-20 deposit flow, making it even easier to follow and maintain.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- bindings/erc20_deposit_args.go (1 hunks)
- e2e/stack.go (4 hunks)
- e2e/stack_test.go (13 hunks)
- testutils/utils.go (3 hunks)
- x/rollup/keeper/deposits.go (3 hunks)
- x/rollup/tests/integration/rollup_test.go (4 hunks)
- x/rollup/types/events.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- bindings/erc20_deposit_args.go
- e2e/stack.go
- testutils/utils.go
- x/rollup/tests/integration/rollup_test.go
- x/rollup/types/events.go
Additional comments not posted (4)
x/rollup/keeper/deposits.go (3)
134-179: LGTM!The function correctly parses and executes recognized cross-domain messages for minting ERC-20 tokens. Returning nil without an error for unrecognized messages aligns with the TODO comment.
To address the TODO comment, investigate the Optimism specification to determine the appropriate error handling for unrecognized or invalid cross-domain messages. Create a follow-up issue to track this investigation and update the function accordingly.
208-215: Using theerc20/{l1erc20addr}format for the coin denomination is a good approach.This format ensures a unique denomination for each bridged ERC-20 token based on its L1 address. It allows for permissionless and trustless bridging of ERC-20 tokens without the need for pre-registration or maintaining a mapping between L1 and Cosmos token addresses.
185-188: The updated error messages improve clarity.The new error messages provide more context about the specific operation that failed, making it easier to identify and debug issues related to minting and sending ETH coins.
e2e/stack_test.go (1)
Line range hint
239-449: LGTM!The
ethRollupFlowfunction is well-structured, easy to follow, and correctly tests the ETH deposit and withdrawal flow. The function uses the appropriate contracts and methods for the deposit and withdrawal transactions, includes appropriate waiting mechanisms and assertions, and provides good visibility into the test results through logging.The changes to rename the function and use the
OptimismPortalcontract are appropriate and improve the clarity and correctness of the test.
Adds support for ERC-20 deposits by parsing the deposit tx data. ERC-20 tokens are minted on L2 in the
erc20/{L1 ERC-20 address}format.Safeguards are in place to ensure that the deposit tx originated from the L1CrossDomainMessenger so that users can't forge tx data to mint unlimited ERC-20 tokens.
x/rollupintegration tests and e2e tests with ERC-20 deposits were added to verify the deposit was successful.This is an initial increment to get a functional implementation for ERC-20 deposits. Future work to either add a genesis state to
x/rollupto contain theL1CrossDomainMessengeraddress or to extract the ERC-20 deposit logic to a new cosmos module will be done in a future increment.Closes #210
Summary by CodeRabbit
New Features
Bug Fixes