Enable transfers during L1 deposits and add L1 wallet integration to the devnet #249
Conversation
Updates the x/rollup deposit flow to transfer minted ETH to the `To` address in the deposit tx. The amount of transferred ETH is determined by the `Value` field in the deposit tx. Any extra minted ETH, determined by the `Mint` field, will be sent to the sender address of the deposit tx.
Adds an L1 wallet integration to interact with the devnet Ethereum network and deposit ETH on L2 networks.
WalkthroughThe pull request involves several changes across multiple files, primarily focusing on wallet integration and transaction handling between Ethereum and Cosmos. Key modifications include the removal of the Changes
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 (
|
| // TODO: make L1 http port configurable | ||
| nodeCfg.HTTPPort = 39473 |
There was a problem hiding this comment.
I opted to include chaging the http port to be configurable in a follow-up PR for now, lmk if you think I should update it in this PR instead
| AttributeKeyFromCosmosAddress = "from_address" | ||
| AttributeKeyToCosmosAddress = "to_address" | ||
| AttributeKeyMintCosmosAddress = "mint_address" | ||
| AttributeKeySender = "sender" | ||
| AttributeKeyL1Target = "target" | ||
| AttributeKeyValue = "value" | ||
| AttributeKeyMint = "mint" |
There was a problem hiding this comment.
I don't love these event attribute names and feel like they may be confusing. I'll think about the best way to format our mint events and may update these
natebeauregard
changed the title
| @@ -121,13 +121,15 @@ func (k *Keeper) processL1UserDepositTxs( | |||
| ctx.Logger().Error("Failed to get sender address", "evmAddress", from, "err", err) | |||
| return nil, types.WrapError(types.ErrInvalidL1Txs, "failed to get sender address: %v", err) | |||
| } | |||
| cosmAddr := utils.EvmToCosmosAddress(from) | |||
| mintAddr := utils.EvmToCosmosAddress(from) | |||
There was a problem hiding this comment.
question to reviewers - do you think senderAddr or fromAddr is a better name here than mintAddr?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (17)
x/rollup/types/events.go (1)
12-12: Approve addition ofAttributeKeyMintwith minor suggestionThe new attribute key
AttributeKeyMintwith value "mint" is a welcome addition that directly supports the PR objective of handling extra ETH minting. Its inclusion enhances the event attribute set to accommodate the new minting functionality.Consider moving this constant next to the other mint-related constant
AttributeKeyMintCosmosAddressfor better organization and readability. Here's a suggested reordering:AttributeKeyFromCosmosAddress = "from_address" AttributeKeyToCosmosAddress = "to_address" AttributeKeyMintCosmosAddress = "mint_address" + AttributeKeyMint = "mint" AttributeKeySender = "sender" AttributeKeyL1Target = "target" AttributeKeyValue = "value" - AttributeKeyMint = "mint" AttributeKeyGasLimit = "gas_limit"x/rollup/tests/integration/rollup_test.go (3)
55-56: LGTM: Improved address handling for minting and transfers.The introduction of
mintAddrandrecipientAddrclearly distinguishes between the minting and receiving addresses, aligning with the PR objectives.Consider adding a comment explaining the roles of
mintAddrandrecipientAddrto enhance code readability:+// mintAddr is the address where ETH is minted mintAddr := utils.EvmToCosmosAddress(from) +// recipientAddr is the address that receives the transferred ETH recipientAddr := utils.EvmToCosmosAddress(*depositTx.To())
81-85: LGTM: Accurate balance verifications post-transaction.The updated balance checks correctly verify the deposit and transfer process, aligning with the PR objectives.
For consistency with the initial balance checks, consider using
math.ZeroInt()in the comparison:-require.Equal(t, new(big.Int).Sub(mintAmount, transferAmount), queryUserETHBalance(t, queryClient, mintAddr, integrationApp).BigInt()) +require.Equal(t, new(big.Int).Sub(mintAmount, transferAmount), queryUserETHBalance(t, queryClient, mintAddr, integrationApp).BigInt()) -require.Equal(t, transferAmount, queryUserETHBalance(t, queryClient, recipientAddr, integrationApp).BigInt()) +require.Equal(t, transferAmount, queryUserETHBalance(t, queryClient, recipientAddr, integrationApp).BigInt())This change would make the assertions more consistent with the initial balance checks and improve readability.
102-104: LGTM: Consistent address and amount handling in successful withdrawal.The use of
recipientAddrandtransferAmountin the successful withdrawal is consistent with earlier changes and aligns with the PR objectives.Consider adding a comment before the final balance check to clarify its purpose:
+// Verify that the withdrawal was successful by checking the recipient's balance // query the recipient address ETH balance and assert it's zero require.Equal(t, math.ZeroInt(), queryUserETHBalance(t, queryClient, recipientAddr, integrationApp))This addition would improve the test's readability and make the purpose of the final check more explicit.
Also applies to: 110-111
e2e/stack_test.go (1)
271-271: LGTM! Consider adding a comment for clarity.The change from
big.NewInt(0)todepositAmountis correct and aligns with the PR objectives. This modification ensures that the actual deposit amount is used in the transaction, improving the accuracy of the test scenario.Consider adding a brief comment explaining the significance of this change, such as:
// Pass the actual deposit amount to accurately test the deposit flowopdevnet/wallet/keplr-integration.js (5)
7-10: Reminder: Address the TODO comment forcoinMinimalDenomThere is a TODO comment indicating that
coinMinimalDenomshould be updated to"wei"once thex/rollupmodule is updated. Please ensure this is addressed when appropriate for consistency.Would you like assistance in updating the
coinMinimalDenomor creating a GitHub issue to track this task?
25-27: EnsurecoinMinimalDenomis consistently updated incurrenciesIn the
currenciesconfiguration,coinMinimalDenomis set to"ETH"without a corresponding TODO comment. For consistency withstakeCurrency, consider adding a TODO comment to update this to"wei"once thex/rollupmodule is updated.
32-34: EnsurecoinMinimalDenomis consistently updated infeeCurrenciesSimilarly, in the
feeCurrenciesconfiguration,coinMinimalDenomis set to"ETH"without a TODO comment. For consistency, consider adding a TODO comment to update this value to"wei"when appropriate.
Line range hint
64-65: Correct the method to obtain the offline signerIn the
enableKeplrfunction, the line:const offlineSigner = window.getOfflineSigner(chainId.toString());should use
window.keplr.getOfflineSigner()instead ofwindow.getOfflineSigner(). The corrected line would be:const offlineSigner = window.keplr.getOfflineSigner(chainId.toString());This ensures that you are accessing the
getOfflineSignermethod provided by Keplr.Apply this diff to fix the issue:
- const offlineSigner = window.getOfflineSigner(chainId.toString()); + const offlineSigner = window.keplr.getOfflineSigner(chainId.toString());
79-79: Consider using a non-blocking notification instead ofalertUsing
alert()disrupts the user experience as it blocks interaction until dismissed. Consider implementing a non-blocking notification (e.g., a toast message or updating the UI) to inform the user that the Monomer L2 chain has been registered with Keplr.opdevnet/wallet/index.html (1)
75-86: Improve Accessibility by Adding Labels to Input FieldsFor better accessibility, associate each input field with a
<label>element. This assists users who rely on screen readers.Example:
<label for="amount">Amount in ETH:</label> <input type="number" step="any" id="amount" placeholder="Amount in ETH" /> <label for="recipient">Cosmos Recipient Address:</label> <input type="text" id="recipient" placeholder="Cosmos Recipient Address" />opdevnet/wallet/bech32.js (2)
65-69: Handle error cases explicitly infromWordsUnsafefunctionThe
fromWordsUnsafefunction returnsundefinedif the conversion fails, which may lead to unexpected behavior. Consider explicitly returningnullto indicate failure.Apply this diff to return
nullwhen conversion fails:function fromWordsUnsafe(words) { const res = convert(words, 5, 8, false); if (Array.isArray(res)) return res; + return null; }
151-155: Handle error cases explicitly indecodeUnsafefunctionSimilar to
fromWordsUnsafe, thedecodeUnsafefunction returnsundefinedupon failure. Explicitly returningnullcan make error handling more consistent and clear.Apply this diff to return
nullwhen decoding fails:function decodeUnsafe(str, LIMIT) { const res = __decode(str, LIMIT); if (typeof res === 'object') return res; + return null; }opdevnet/wallet/metamask-integration.js (2)
97-97: Avoid logging sensitive user information to the console.Logging user addresses can pose security risks if the console output is accessed by unauthorized parties. Consider removing or obfuscating this log statement.
Apply this diff:
- console.log('Converted Cosmos address to Ethereum address:', cosmosAddress, ethAddress); + // Removed logging of user addresses for security reasons.
64-64: Consider making the gas limit configurable or dynamic.Hardcoding the gas limit to
500000may not be optimal for all transactions. Consider calculating the required gas limit dynamically or allowing it to be configurable to accommodate different transaction complexities.x/rollup/keeper/deposits.go (2)
210-212: Consider using consistent error wrappingFor consistency in error handling across the codebase, consider using
types.WrapErrorinstead offmt.Errorfwithin themintETHfunction.Apply the following changes:
In line 211:
-if err := k.bankkeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(types.ETH, mintAmount))); err != nil { - return nil, fmt.Errorf("failed to mint ETH deposit coins to the rollup module: %v", err) + return nil, types.WrapError(err, "failed to mint ETH deposit coins to the rollup module")In line 227:
- return nil, fmt.Errorf("failed to send ETH deposit coins from rollup module to user account %v: %v", recipientAddr, err) + return nil, types.WrapError(err, "failed to send ETH deposit coins to recipient address: %v", recipientAddr)In line 240:
- return nil, fmt.Errorf("failed to send ETH deposit coins from rollup module to user account %v: %v", mintAddr, err) + return nil, types.WrapError(err, "failed to send remaining ETH to mint address: %v", mintAddr)Also applies to: 226-227, 239-240
214-216: Clarify error message when transfer amount exceeds mint amountConsider improving the error message for better clarity by including additional context.
Apply this change:
-return nil, fmt.Errorf("transfer amount %v is greater than mint amount %v", transferAmount, mintAmount) +return nil, fmt.Errorf("transfer amount %v exceeds mint amount %v for transaction from %v to %v", transferAmount, mintAmount, mintAddr.String(), recipientAddr.String())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- Makefile (1 hunks)
- builder/builder_test.go (5 hunks)
- e2e/stack_test.go (1 hunks)
- opdevnet/l1.go (1 hunks)
- opdevnet/wallet/bech32.js (1 hunks)
- opdevnet/wallet/index.html (4 hunks)
- opdevnet/wallet/keplr-integration.js (4 hunks)
- opdevnet/wallet/metamask-integration.js (1 hunks)
- testutils/utils.go (1 hunks)
- x/rollup/keeper/deposits.go (2 hunks)
- x/rollup/tests/integration/rollup_test.go (2 hunks)
- x/rollup/types/events.go (1 hunks)
🧰 Additional context used
🪛 Biome
opdevnet/wallet/bech32.js
[error] 4-5: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
opdevnet/wallet/metamask-integration.js
[error] 40-40: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (25)
x/rollup/types/events.go (3)
8-8: Approve addition ofAttributeKeyMintCosmosAddressThe new attribute key
AttributeKeyMintCosmosAddresswith value "mint_address" aligns with the PR objective of handling extra ETH minting. Its addition enhances the event attribute set to accommodate the new minting functionality.Could you please clarify why the "cosmos" prefix is retained in the constant name (
AttributeKeyMintCosmosAddress) while it's omitted in the actual value ("mint_address")? This seems inconsistent with the previous changes.
7-7: Approve renaming ofAttributeKeyToCosmosAddressThe simplification of the attribute key from "to_cosmos_address" to "to_address" is consistent with the previous change and supports the integration of L1 wallets. This modification makes the attribute more versatile, suitable for both Cosmos and Ethereum addresses.
To ensure this change doesn't break existing functionality, please run the following script to check for any occurrences of the old attribute key:
#!/bin/bash # Description: Check for occurrences of the old attribute key "to_cosmos_address" rg --type go "to_cosmos_address"
6-6: Approve renaming ofAttributeKeyFromCosmosAddressThe simplification of the attribute key from "from_cosmos_address" to "from_address" aligns with the PR objective of integrating L1 wallets. This change makes the attribute more generic, accommodating both Cosmos and Ethereum addresses.
To ensure this change doesn't break existing functionality, please run the following script to check for any occurrences of the old attribute key:
✅ Verification successful
Verified renaming of
AttributeKeyFromCosmosAddressNo remaining occurrences of "from_cosmos_address" were found, ensuring the renaming does not break existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for occurrences of the old attribute key "from_cosmos_address" rg --type go "from_cosmos_address"Length of output: 126
Script:
#!/bin/bash # Description: Check for occurrences of the old attribute key "from_cosmos_address" rg --type go "from_cosmos_address"Length of output: 34
Makefile (1)
34-36: Verify the impact of replacingkeplr-integrationwithwallet-integrationThe change from
keplr-integrationtowallet-integrationaligns with the PR objective of integrating L1 wallets. However, please ensure that:
- Any existing workflows or scripts that depended on the
keplr-integrationtarget are updated accordingly.- The new path
opdevnet/walletcontains the necessary files for wallet integration testing.- The introduction of the
github.com/eliben/static-serverdependency is documented and approved.To verify the changes, please run the following commands:
testutils/utils.go (1)
85-85: LGTM! This change aligns with the PR objectives.The addition of
depositRawTx.Value = big.NewInt(50)is consistent with the PR's goal of implementing L1 wallet integration and enhancing the deposit flow. This change:
- Sets a specific value (50) for the ETH transfer during the deposit, as described in the PR objectives.
- Implicitly tests the new behavior where extra ETH (Mint - Value = 100 - 50 = 50) would be directed to the sender's address.
This modification provides a concrete test case for the new deposit flow behavior, which is crucial for ensuring the correct implementation of the PR's objectives.
x/rollup/tests/integration/rollup_test.go (4)
51-52: LGTM: Improved clarity in transaction amount handling.The separation of
mintAmountandtransferAmountaligns well with the PR objectives, providing a clearer distinction between minted and transferred amounts in the deposit process.
58-62: LGTM: Comprehensive initial balance checks.The addition of balance checks for both
mintAddrandrecipientAddrenhances the test's robustness by ensuring both addresses start with zero balance.
92-94: LGTM: Consistent address and amount handling in withdrawal attempt.The use of
mintAddrandtransferAmountin the withdrawal attempt is consistent with the earlier changes and aligns with the PR objectives.
Line range hint
1-180: Overall assessment: Well-implemented changes that align with PR objectives.The modifications to the
TestRollupfunction effectively implement the expanded deposit flow, accurately handling the separation of minting and transferring ETH. The changes improve test clarity and robustness, ensuring proper verification of the new functionality.Key improvements:
- Clear distinction between
mintAddrandrecipientAddr.- Accurate balance checks before and after transactions.
- Consistent use of
transferAmountin withdrawal tests.The suggested minor improvements, if implemented, would further enhance code readability and consistency.
opdevnet/wallet/keplr-integration.js (1)
70-70: Verify there are no remaining references toregisterChainsWithKeplrSince the function has been renamed to
registerChainWithKeplr, please ensure there are no remaining references to the old function nameregisterChainsWithKeplrelsewhere in the codebase to prevent potential runtime errors.Run the following script to verify:
opdevnet/wallet/index.html (4)
6-6: Title Update Reflects Expanded FunctionalityThe title has been appropriately updated to "Monomer Devnet Wallet Integration," which aligns with the broader scope of both L1 and L2 wallet integrations.
15-15: CSS Flex Direction Correctly Stacks Elements VerticallyThe addition of
flex-direction: column;ensures that the elements are stacked vertically, enhancing the layout for a better user experience.
77-77: Update Connected Account Display Upon Wallet ConnectionEnsure that the span with
id="account"is updated dynamically to display the connected wallet address once the user connects their L1 wallet.Run the following script to check if the
accountelement is updated appropriately:#!/bin/bash # Description: Search for code updating the 'account' span. # Test: Look for JavaScript code that modifies the innerHTML or textContent of the element with id 'account'. # Expect: There should be code that updates this element upon successful connection. rg --type js --pcre2 'document.getElementById\(("|\')account("|\')\)\.(innerHTML|textContent)\s*='
69-71: Ensure Local Scripts Are Loaded CorrectlyThe script
bech32.jsis included without a path or integrity check. Ensure that it's properly referenced and consider hosting it securely.Run the following script to verify the presence and correct loading of
bech32.js:opdevnet/wallet/bech32.js (1)
1-4:⚠️ Potential issueInclude license information for the copied code
The code in this file is copied and modified from the bech32 npm package. To comply with the original library's license, please ensure that any required license information or attributions are included.
Please verify the licensing requirements of the bech32 npm package and include the necessary license text at the top of the file if required.
x/rollup/keeper/deposits.go (4)
124-127: Correct extraction of addresses and amountsThe
mintAddr,recipientAddr,mintAmount, andtransferAmountare accurately derived from the transaction data.
129-132: Proper error handling aftermintETHcallThe error handling after calling
k.mintETHis appropriate, with sufficient logging and error wrapping.
Line range hint
204-253:mintETHfunction effectively handles minting and transferring ETHThe updated
mintETHfunction correctly mints the total amount and distributes the transfer amount to the recipient, with proper validations and error handling.
129-132: Verify all usages of updatedmintETHfunctionEnsure that all calls to
mintETHhave been updated to match the new function signature with additional parameters.Run the following script to find all usages of
mintETH:This will list all invocations of
mintETH. Verify that each call includes the new parameters:mintAddr,recipientAddr,mintAmount, andtransferAmount.✅ Verification successful
All usages of
mintETHhave been updated to match the new function signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to 'mintETH' to verify they use the new signature. rg --type go 'k\.mintETH\('Length of output: 145
builder/builder_test.go (6)
15-15: Importinghexutilis appropriateThe addition of
github.com/ethereum/go-ethereum/common/hexutilis necessary for hexadecimal encoding in test assertions later in the code.
317-317: Correctly converting EVM address to Cosmos addressAssigning
recipientAddrusingutils.EvmToCosmosAddress(*depositTxETH.To())properly converts the recipient address from EVM to Cosmos format, ensuring consistency across the test.
321-322: Proper derivation ofmintAddrfrom senderThe
mintAddris correctly obtained from the sender's address usingutils.EvmToCosmosAddress(from), which aligns with the requirement to track the minting address accurately.
324-326: Usage ofrecipientAddrandValueinMsgInitiateWithdrawalis appropriateIn initializing
withdrawalTx, settingSendertorecipientAddr.String()andValuetomath.NewIntFromBigInt(depositTxETH.Value())aligns with the test scenario where the recipient initiates a withdrawal of the deposited amount. This ensures that the withdrawal is correctly attributed to the recipient and that the value matches the deposit.
354-354: Updated function call to match new signatureThe call to
checkDepositTxResultnow includesmintAmount,transferAmount,mintAddr.String(), andrecipientAddr.String(), matching the updated function signature. This ensures that the test correctly validates all aspects of the deposit transaction.
500-500: Ensure all usages ofcheckDepositTxResultare updatedThe function
checkDepositTxResultnow acceptsmintAmount,transferAmount *big.Int,mintAddr, andrecipientAddras parameters. Please verify that all calls to this function throughout the codebase have been updated to reflect the new signature to prevent potential runtime errors.✅ Verification successful
All usages of
checkDepositTxResulthave been correctly updated to match the new signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `checkDepositTxResult` to ensure they match the new signature. # Expected: All function calls should have 7 parameters matching the updated function signature. rg --type go 'checkDepositTxResult\(' -A 1Length of output: 465
Adds an L1 wallet integration to interact with the devnet Ethereum network and deposit ETH on L2 networks.
Since Metamask (L1) and Keplr (L2) wallets have different private keys, the
x/rollupdeposit flow had to be expanded to supports transfers on deposits as well. The amount of transferred ETH is determined by theValuefield in the deposit tx and is sent to theToaddress in the deposit tx.Any extra minted ETH, determined by the
Mintfield minus the amount of transferred ETH (Value), will be sent to the sender address of the deposit tx.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores