Include prefix in address conversion #271
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing error handling and address conversion processes across several files. Modifications include updates to the 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 (
|
joshklop
force-pushed
the
joshklop.addr-conversion-prefix
branch
from
6027e1c to
6858e7b
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: 1
🧹 Outside diff range and nitpick comments (7)
utils/utils.go (1)
27-27: Enhance function documentation.The current comment is too brief. Consider adding details about:
- The purpose of the prefix parameter
- The format of the returned string
- Possible error conditions
Example:
// EvmToCosmosAddress converts an Ethereum address to a bech32-encoded string using the specified prefix. // The prefix determines the address type (e.g., "cosmos" for standard accounts). // Returns the bech32-encoded address string or an error if the conversion fails.x/rollup/keeper/deposits.go (4)
124-135: Enhance error messages for address conversion failures.The error messages could be more specific about which address (sender or recipient) failed conversion to help with debugging.
Apply this diff to improve error messages:
- return nil, fmt.Errorf("evm to cosmos address: %v", err) + return nil, fmt.Errorf("failed to convert sender EVM address %s to cosmos address: %v", from.String(), err) - return nil, fmt.Errorf("evm to cosmos address: %v", err) + return nil, fmt.Errorf("failed to convert recipient EVM address %s to cosmos address: %v", tx.To().String(), err)
243-243: Use Bech32 address string in error message for better readability.The error message uses the SDK address which may not be human-readable. Consider using the original Bech32 address string.
Apply this diff:
- return nil, fmt.Errorf("failed to send ETH deposit coins from rollup module to user account %v: %v", recipientAddrSDK, err) + return nil, fmt.Errorf("failed to send ETH deposit coins from rollup module to user account %s: %v", recipientAddr, err)
292-293: Enhance error message for failed coin transfer.The error message could be more specific about the ERC20 token being transferred.
Apply this diff:
- return nil, fmt.Errorf("failed to send ERC-20 deposit coins from rollup module to user account %v: %v", userAddr, err) + return nil, fmt.Errorf("failed to send ERC-20 token %s deposit coins from rollup module to user account %s: %v", erc20addr, userAddr, err)
195-198: Enhance error message for cross-domain address conversion.The error message could provide more context about the cross-domain message processing.
Apply this diff:
- return nil, fmt.Errorf("evm to cosmos address: %v", err) + return nil, fmt.Errorf("failed to convert cross-domain recipient EVM address %s to cosmos address: %v", finalizeBridgeERC20.To.String(), err)builder/builder_test.go (1)
317-323: Consider using a constant for the "cosmos" prefix.While the implementation correctly uses the new prefix parameter, having "cosmos" as a magic string could be improved by defining it as a constant. This would make it easier to maintain and modify the prefix if needed.
Consider defining a constant at the package level:
const CosmosAddressPrefix = "cosmos"Then use it in the conversions:
-recipientAddr, err := utils.EvmToCosmosAddress("cosmos", *depositTxETH.To()) +recipientAddr, err := utils.EvmToCosmosAddress(CosmosAddressPrefix, *depositTxETH.To()) -mintAddr, err := utils.EvmToCosmosAddress("cosmos", from) +mintAddr, err := utils.EvmToCosmosAddress(CosmosAddressPrefix, from)e2e/stack_test.go (1)
334-335: Consider extracting the "cosmos" prefix as a constant.While the implementation is correct, consider extracting the "cosmos" string as a package-level constant to improve maintainability and ensure consistency across the codebase.
+const ( + cosmosAddressPrefix = "cosmos" +) -senderAddr, err := utils.EvmToCosmosAddress("cosmos", *withdrawalTx.Sender) +senderAddr, err := utils.EvmToCosmosAddress(cosmosAddressPrefix, *withdrawalTx.Sender)Also applies to: 339-339
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- builder/builder_test.go (2 hunks)
- e2e/stack_test.go (3 hunks)
- utils/utils.go (2 hunks)
- x/rollup/keeper/deposits.go (7 hunks)
- x/rollup/tests/integration/rollup_test.go (5 hunks)
🔇 Additional comments (9)
utils/utils.go (2)
7-7: LGTM! Appropriate import for bech32 encoding.The switch to using the
bech32package directly is a good choice as it provides more explicit control over address encoding.
28-33: LGTM! Clean implementation with proper error handling.The implementation is concise and handles errors appropriately. The use of
bech32.ConvertAndEncodeis the correct approach for this conversion.Let's verify that all callers have been updated to handle the new signature:
✅ Verification successful
All callers of
EvmToCosmosAddressare properly updated with the new signature and error handlingThe verification shows that all callers:
- Pass the required prefix parameter (either "cosmos" or using
sdk.GetConfig().GetBech32AccountAddrPrefix())- Handle the returned error appropriately through either:
require.NoError(t, err)in test files- Error logging with context in keeper files
- Error propagation with wrapping in business logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all callers of EvmToCosmosAddress are updated to handle the new signature # Search for function calls to ensure they include the prefix parameter and handle errors rg -A 2 "EvmToCosmosAddress\(" --type goLength of output: 2909
x/rollup/tests/integration/rollup_test.go (3)
53-57: LGTM: Proper error handling for address conversions.The error handling for EVM to Cosmos address conversions is well implemented, with appropriate error checks using
require.NoError.
67-69: LGTM: Consistent error handling for ERC20 address conversion.The error handling pattern is consistently applied to the ERC20 user address conversion.
94-97: Verify the withdrawal target address format.While the
Senderfield correctly uses the Cosmos address, consider validating theTargetaddress format to ensure it's a valid Ethereum address. This would prevent potential issues with invalid withdrawal addresses.Also applies to: 104-107
builder/builder_test.go (2)
Line range hint
326-356: LGTM! Changes look good.The withdrawal message construction and deposit check are properly implemented with the converted addresses.
Line range hint
317-356: Verify all usages of EvmToCosmosAddress are updated.Let's ensure all calls to
EvmToCosmosAddressacross the codebase have been updated to include the prefix parameter.✅ Verification successful
All usages of EvmToCosmosAddress have been properly updated with prefix parameter
Based on the search results, all calls to
EvmToCosmosAddressacross the codebase are correctly using two parameters:
- Production code in
x/rollup/keeper/deposits.gousessdk.GetConfig().GetBech32AccountAddrPrefix()- Test files consistently use either
"cosmos"orsdk.GetConfig().GetBech32AccountAddrPrefix()- The function definition in
utils/utils.gocorrectly accepts both parameters🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of EvmToCosmosAddress to verify they include the prefix parameter # Expected: All calls should have two arguments (prefix and address) echo "Searching for EvmToCosmosAddress usage patterns..." rg "EvmToCosmosAddress\([^,)]+\)" --type go echo -e "\nSearching for updated usage patterns..." rg "EvmToCosmosAddress\([^,]+,[^)]+\)" --type goLength of output: 2215
e2e/stack_test.go (2)
319-320: LGTM! Proper error handling for address conversion.The addition of error handling for the address conversion is a good practice, and the consistent use of the "cosmos" prefix parameter aligns with the changes across the codebase.
536-538: LGTM! Consistent address conversion implementation.The implementation maintains consistency with other address conversions in the file and includes proper error handling.
joshklop
force-pushed
the
joshklop.addr-conversion-prefix
branch
from
9e589c0 to
9e8be02
Compare
The previous method of converting EVM to Cosmos addresses used a global variable in the Cosmos SDK that may not be set in e2e tests. This led to difficult-to-debug errors.
joshklop
force-pushed
the
joshklop.addr-conversion-prefix
branch
from
9e8be02 to
64b7a4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
utils/utils.go (1)
27-33: Enhance function documentation for clarityWhile the implementation is solid, the documentation could be more descriptive about:
- The expected prefix format
- The returned address format
- Possible error conditions
Consider updating the comment to:
-// EvmToCosmosAddress converts an EVM address to a string +// EvmToCosmosAddress converts an Ethereum address to a bech32 encoded Cosmos address string. +// The prefix parameter determines the bech32 address prefix (e.g., "cosmos", "osmo"). +// Returns the bech32 encoded address string or an error if the conversion fails.e2e/stack_test.go (1)
536-538: Improve error handling in ERC20 test flow.The error handling for address conversion is consistent with other test cases. However, consider adding test cases for invalid addresses to ensure the error handling works as expected.
Add a test case with an invalid address:
_, err = utils.EvmToCosmosAddress("cosmos", common.Address{}) require.Error(t, err, "should fail for zero address")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- builder/builder_test.go (2 hunks)
- e2e/stack_test.go (3 hunks)
- scripts/go-wrapper.sh (1 hunks)
- utils/utils.go (2 hunks)
- x/rollup/keeper/deposits.go (7 hunks)
- x/rollup/tests/integration/rollup_test.go (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- builder/builder_test.go
- x/rollup/tests/integration/rollup_test.go
🔇 Additional comments (6)
scripts/go-wrapper.sh (1)
9-10: LGTM! Proper environment variable handlingThe export of LDFLAGS before go command execution is correct and preserves any existing flags from the environment.
utils/utils.go (2)
7-7: LGTM: Import change aligns with new implementationThe addition of the bech32 package is appropriate for the new address encoding approach.
28-33: Verify the impact of breaking signature changeThe implementation looks good with proper error handling and standard bech32 encoding. However, since this is a breaking change, we should verify all callers have been updated.
✅ Verification successful
Let me analyze the results from the executed script. I can see all the usages of
EvmToCosmosAddressin the codebase, and they all follow the same signature pattern:
- All calls pass a prefix string as the first argument (either "cosmos" or using
sdk.GetConfig().GetBech32AccountAddrPrefix())- All calls pass an EVM address as the second argument
- All calls properly handle the error return value
Since all callers are already using the correct signature and handling errors appropriately, I can generate the final response:
Breaking signature change verification completed - all callers are compatible
All usages of
EvmToCosmosAddressacross the codebase are already using the correct signature with proper error handling, including:
- x/rollup/keeper/deposits.go
- x/rollup/tests/integration/rollup_test.go
- e2e/stack_test.go
- builder/builder_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of EvmToCosmosAddress to ensure they've been updated with the new signature rg -l "EvmToCosmosAddress" | while read -r file; do echo "=== $file ===" rg -A 2 "EvmToCosmosAddress" "$file" doneLength of output: 2741
x/rollup/keeper/deposits.go (3)
124-129: LGTM! Improved address conversion with proper error handling.The changes correctly retrieve the Bech32 prefix from SDK config and add comprehensive error handling for address conversion.
Also applies to: 131-135
195-198: LGTM! Consistent address conversion implementation.The changes maintain consistency with the address conversion pattern used elsewhere in the codebase.
Also applies to: 202-202
231-234: Rename recipientSDKAddr to recipientAddr for consistency.As per previous review comment, consider renaming
recipientSDKAddrto match the naming convention used withmintSDKAddr.Also applies to: 240-240
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests