Derive Cosmos addresses from public keys #273
Conversation
WalkthroughThe changes in this pull request primarily involve the transition from using 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 (
|
joshklop
force-pushed
the
joshklop.fix-address-conversion
branch
2 times, most recently
from
ce8d1a6
to
bcc0c62
Compare
joshklop
force-pushed
the
joshklop.fix-address-conversion
branch
from
bcc0c62
to
fc7bc42
Compare
|
I was planning to do this later, but it's required for fixing the e2e tests. |
![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 (5)
monomer_test.go (1)
226-239: Add documentation explaining the test's purpose.While the test implementation is correct, adding a comment block explaining the test's purpose and the relationship between Cosmos and Ethereum addresses would improve maintainability.
+// TestCosmosETHAddress verifies that public keys are correctly converted to Cosmos Ethereum addresses +// and properly encoded using the Cosmos address format. It validates that our implementation matches +// the Cosmos SDK's address derivation process. func TestCosmosETHAddress(t *testing.T) {monomer.go (2)
27-27: Consider documenting the reason for disabling staticcheck.The
ripemd160package has astaticcheckdisable comment. While this is a commonly used cryptographic hash function in blockchain address derivation, it would be helpful to document why the staticcheck warning is being ignored.
244-266: Strong architectural design for address handling.The implementation demonstrates good architectural choices:
- Clear separation of concerns between address creation and encoding
- Type safety through the custom
CosmosETHAddresstype- Follows standard cryptographic practices for address derivation
- Integrates well with both Ethereum and Cosmos ecosystems
Consider adding examples in the documentation to demonstrate typical usage patterns.
Add usage examples to the documentation:
// Example: // // pubKey := // ... your secp256k1 public key // addr := PubkeyToCosmosETHAddress(pubKey) // bech32Addr, err := addr.Encode("cosmos") // if err != nil { // // handle error // }x/rollup/keeper/deposits.go (1)
124-128: Maintain consistent error handling patterns.While the address conversion is correct, consider using
types.WrapErrorfor consistency with other error handling in this file:mintAddr, err := monomer.CosmosETHAddress(from).Encode(addrPrefix) if err != nil { ctx.Logger().Error("Failed to convert EVM to Cosmos address", "err", err) - return nil, fmt.Errorf("evm to cosmos address: %v", err) + return nil, types.WrapError(types.ErrInvalidL1Txs, "failed to convert EVM to Cosmos address: %v", err) }e2e/stack_test.go (1)
Line range hint
1-644: Consider adding integration test for address conversion edge cases.While the current tests cover the happy path, consider adding test cases for:
- Invalid address formats
- Zero addresses
- Contract addresses
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- builder/builder_test.go (1 hunks)
- e2e/stack_test.go (7 hunks)
- monomer.go (3 hunks)
- monomer_test.go (2 hunks)
- utils/utils.go (0 hunks)
- x/rollup/keeper/deposits.go (2 hunks)
- x/rollup/tests/integration/rollup_test.go (3 hunks)
💤 Files with no reviewable changes (1)
- utils/utils.go
🔇 Additional comments (19)
monomer_test.go (1)
10-12: LGTM: Required imports for Cosmos address handling.The new imports are appropriate for handling secp256k1 keys and Cosmos SDK types.
x/rollup/tests/integration/rollup_test.go (2)
26-27: LGTM: Import changes align with the transition to monomer package.The addition of monomer imports and removal of utils import is consistent with the PR objective of deriving Cosmos addresses from public keys.
55-58: LGTM: Address conversion implementation is consistent and robust.The transition to
monomer.CosmosETHAddress(...).Encode(...)is well-implemented with proper error handling and consistent use of the system's Bech32 prefix.Let's verify the consistency of address conversion across the codebase:
Also applies to: 67-68
✅ Verification successful
Address conversion changes are consistent across the codebase
The verification confirms:
- No instances of the old
EvmToCosmosAddressmethod remain in the codebase- The new
monomer.CosmosETHAddress(...).Encode(...)pattern is consistently used across all relevant files:
- Integration tests (
x/rollup/tests/integration/rollup_test.go)- Keeper logic (
x/rollup/keeper/deposits.go)- E2E tests (
e2e/stack_test.go)- Builder tests (
builder/builder_test.go)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all Ethereum to Cosmos address conversions use the new monomer package # and that there are no remaining instances of the old utils.EvmToCosmosAddress. # Test 1: Search for any remaining instances of the old conversion method echo "Checking for old conversion method..." rg "EvmToCosmosAddress" # Test 2: Verify consistent usage of the new conversion method echo "Verifying new conversion method usage..." rg "CosmosETHAddress.*Encode"Length of output: 1595
monomer.go (3)
245-250: LGTM! Well-documented type definition.The
CosmosETHAddresstype is well-documented with clear references to ADR-28 and explicit limitations regarding secp256k1 keys.
264-266: LGTM! Clean and correct implementation.The
Encodemethod correctly uses the Cosmos SDK's bech32 package for address encoding with appropriate error handling.
252-262: 🛠️ Refactor suggestionConsider adding input validation for secp256k1 keys.
While the function documentation states that passing non-secp256k1 keys results in undefined behavior, it might be better to add explicit validation to prevent potential issues.
Consider adding validation:
func PubkeyToCosmosETHAddress(pubKey *ecdsa.PublicKey) CosmosETHAddress { + if pubKey.Curve != secp256k1.S256() { + panic("PubkeyToCosmosETHAddress only supports secp256k1 keys") + } sha := sha256.Sum256(secp256k1.CompressPubkey(pubKey.X, pubKey.Y))x/rollup/keeper/deposits.go (3)
Line range hint
130-134: Same error handling inconsistency as above.This segment has the same error handling inconsistency as noted in the previous comment.
194-197: Same error handling inconsistency as above.This segment has the same error handling inconsistency as noted in the first comment.
Line range hint
124-197: Verify complete migration from utils.EvmToCosmosAddress.Let's verify that all instances of the old function have been replaced and the new function is properly imported.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Migration from utils.EvmToCosmosAddress to monomer.CosmosETHAddress is complete
The verification shows:
- No remaining instances of
utils.EvmToCosmosAddressin the codebase- Proper import of
monomerpackage whereCosmosETHAddressis used- Consistent usage of
CosmosETHAddressacross the codebase, particularly in deposit-related operations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of the old function and verify imports # Test 1: Check for any remaining instances of the old function echo "Checking for remaining instances of EvmToCosmosAddress..." rg "EvmToCosmosAddress" # Test 2: Verify proper import of the new function echo "Verifying monomer package imports..." rg "github.com/polymerdao/monomer" # Test 3: Check for usage of CosmosETHAddress echo "Checking usage of CosmosETHAddress..." rg "CosmosETHAddress"Length of output: 19372
builder/builder_test.go (3)
316-317: LGTM: Clean transition to monomer package for recipient address conversionThe conversion from Ethereum to Cosmos address format using
monomer.CosmosETHAddress().Encode()is implemented correctly with proper error handling.
321-322: LGTM: Clean transition to monomer package for mint address conversionThe conversion from Ethereum to Cosmos address format using
monomer.CosmosETHAddress().Encode()is implemented correctly with proper error handling.
316-322: Verify test coverage for address conversionThe changes replace the old address conversion utility with the new monomer package implementation. Let's verify that all test cases related to address conversion are passing.
✅ Verification successful
Let me gather more context about the test coverage by checking the implementation and test files more thoroughly.
Let me try one more verification to check the test coverage and assertions:
Let me check the actual test implementation for address conversion:
Based on the gathered information, I can now provide the final response:
Address conversion changes are properly tested
The new
monomer.CosmosETHAddressimplementation is well-tested with:
- Dedicated unit test
TestCosmosETHAddressthat verifies the core conversion functionality- Integration tests in multiple files:
builder/builder_test.go: Tests deposit flow address conversione2e/stack_test.go: Tests withdrawal flow and user address conversionsThe old
EvmToCosmosAddressutility has been completely replaced, and all test cases are using the new implementation consistently.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other tests that might be using the old address conversion utility # Test: Search for any remaining references to the old conversion utility rg "EvmToCosmosAddress" # Test: Verify all test files that use the new conversion utility rg "CosmosETHAddress.*Encode.*cosmos"Length of output: 535
Script:
#!/bin/bash # Check the implementation and test files for address conversion # Look for any potential address conversion implementations or related tests ast-grep --pattern 'func $_($$$) { $$$ CosmosETHAddress($$$) $$$ }' # Check test files for any address-related assertions rg -B2 -A2 "require.*addr" ".*_test.go"Length of output: 169
Script:
#!/bin/bash # Search for test files containing address conversion tests rg -B3 -A3 "require.*NoError.*\b(addr|address)" --type go # Look for any potential test failures or errors related to address conversion rg "func Test.*Address" --type goLength of output: 675
Script:
#!/bin/bash # Check the implementation of TestCosmosETHAddress rg -B5 -A15 "func TestCosmosETHAddress" monomer_test.go # Check for any related test assertions in builder_test.go rg -B3 -A3 "CosmosETHAddress.*Encode.*cosmos.*require" builder_test.goLength of output: 999
e2e/stack_test.go (7)
251-251: LGTM! Clear variable naming improves code readability.The renaming of
userAddresstouserETHAddressand introduction ofuserCosmosETHAddressmakes the code more explicit about address types and their purposes.Also applies to: 257-258
264-264: LGTM! Consistent use of address types in deposit flow.The deposit flow correctly uses the appropriate address types:
userETHAddressfor checking L1 balanceuserCosmosETHAddressfor the deposit targetAlso applies to: 271-271
297-298: LGTM! Proper event filtering with correct address types.The filter parameters correctly use:
userETHAddressfor the depositor addressuserCosmosETHAddressfor the target address
320-322: LGTM! Proper address encoding for Cosmos.The code correctly encodes the ETH address into Cosmos format using the new
Encodemethod.
332-332: LGTM! Proper withdrawal flow with correct address handling.The withdrawal flow correctly:
- Uses
userCosmosETHAddressas the sender- Properly encodes the sender address for Cosmos
- Maintains proper type safety throughout the flow
Also applies to: 335-344
404-404: LGTM! Consistent balance checking in finalization flow.The finalization flow maintains consistency in using
userETHAddressfor L1 balance checks.Also applies to: 437-437
537-537: LGTM! Proper address conversion in ERC20 flow.The ERC20 flow correctly uses
monomer.CosmosETHAddressfor address conversion.
| func TestCosmosETHAddress(t *testing.T) { | ||
| privKey, err := secp256k1.GeneratePrivateKey() | ||
| require.NoError(t, err) | ||
| pubKey := privKey.PubKey().ToECDSA() | ||
| got := monomer.PubkeyToCosmosETHAddress(pubKey) | ||
| wantBytes := (&cosmossecp256k1.PubKey{ | ||
| Key: privKey.PubKey().SerializeCompressed(), // https://github.com/cosmos/cosmos-sdk/blob/346044afd0ecd4738c13993d2ac75da8e242266d/crypto/keys/secp256k1/secp256k1.go#L44-L45 | ||
| }).Address().Bytes() | ||
| require.Equal(t, wantBytes, common.Address(got).Bytes()) | ||
| // We have to use the `cosmos` hrp here because sdk.AccAddress.String() uses the global SDK config variable that uses the `cosmos` hrp. | ||
| gotEncoded, err := got.Encode("cosmos") | ||
| require.NoError(t, err) | ||
| require.Equal(t, sdk.AccAddress(wantBytes).String(), gotEncoded) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test cases for edge cases.
The current test only covers the happy path. Consider adding test cases for:
- Invalid public keys
- Different address prefixes
- Zero addresses
- Known test vectors from the Cosmos SDK
This would ensure robust handling of various scenarios.
Would you like me to provide an implementation for these additional test cases?
|
Now that I think about it, I believe the address conversions in the |
Yeah good catch. I copied the |
There was a problem hiding this comment.
Monomer changes LGTM. I'll wait to approve until the frontend wallet server changes are in as well. I wonder if we can use some of the helpers from the Evmos JS converter.
|
Hmm, it seems to work fine for me. Perhaps I'm doing something wrong though. Mind checking on your end @natebeauregard ? |
@joshklop When you say |
I'm able to deposit funds to L2 and transfer them around. I would expect that the different address translation would cause them to be sent to a different address (i.e., not my keplr address). However, thinking about this again, it actually makes sense: the way you did address translation in frontend happened to be compatible with both address conversion methods.
I have not. Might be interesting to try it though. I'll see if I can get to it tomorrow, pretty unlikely however. |
Sounds good, thanks for the clarification. I can test the prefix case tm morning and can approve/merge if it works! |
|
Alternative prefixes work as well :) |
Summary by CodeRabbit
Release Notes
New Features
CosmosETHAddresstype for improved handling of Ethereum addresses in the Cosmos context.Bug Fixes
Tests
CosmosETHAddressfunctionality.Chores