feat: gas station evm address #189
Conversation
WalkthroughIntroduces an AddressType enum (Cosmos vs EVM), EVM HD path and derivation helpers, and threads addressType through mnemonic-to-address, key creation/recovery, and call sites across crypto, io, config, and init modules; also updates several test chain IDs and integration test inputs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
io/keyfile.go (1)
39-49: Critical: Missing AddressType field in RecoverKey return value.The
RecoverKeyfunction accepts anaddressTypeparameter but doesn't populate theAddressTypefield in the returnedKeystruct (lines 45-48). This causes data loss and could lead to incorrect behavior when the key is later used or serialized.Compare with
NewKey(lines 17-23) andGenerateKey(line 36), which correctly set all three fields includingAddressType.🔎 Proposed fix
func RecoverKey(hrp, mnemonic string, addressType crypto.AddressType) (*Key, error) { address, err := crypto.MnemonicToBech32Address(hrp, mnemonic, addressType) if err != nil { return nil, fmt.Errorf("failed to derive address: %w", err) } return &Key{ - Mnemonic: mnemonic, - Address: address, + Mnemonic: mnemonic, + Address: address, + AddressType: addressType, }, nil }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
config/config.gocrypto/bech32.goio/keyfile.gomodels/initialize.gomodels/opinit_bots/config.gomodels/opinit_bots/init.gomodels/opinit_bots/setup.gomodels/relayer/init.go
🧰 Additional context used
🧬 Code graph analysis (5)
models/initialize.go (1)
crypto/bech32.go (2)
MnemonicToBech32Address(33-66)EVMAddressType(22-22)
models/relayer/init.go (2)
io/keyfile.go (2)
GenerateKey(25-37)RecoverKey(39-49)crypto/bech32.go (1)
CosmosAddressType(21-21)
io/keyfile.go (1)
crypto/bech32.go (3)
AddressType(18-18)GenerateMnemonic(149-161)MnemonicToBech32Address(33-66)
crypto/bech32.go (2)
io/keyfile.go (1)
Key(11-15)context/context.go (1)
Key(10-10)
models/opinit_bots/init.go (3)
models/opinit_bots/bots.go (1)
BotNameToKeyName(47-53)io/keyfile.go (1)
NewKey(17-23)crypto/bech32.go (1)
CosmosAddressType(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Integration Tests on Ubuntu
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crypto/bech32.go (1)
18-23: LGTM: AddressType enum follows Go best practices.The AddressType definition is clean and idiomatic.
💡 Optional: Add String() method for better error messages
Consider implementing a
String()method to improve debugging and error messages:func (a AddressType) String() string { switch a { case CosmosAddressType: return "Cosmos" case EVMAddressType: return "EVM" default: return fmt.Sprintf("Unknown(%d)", a) } }This would make error messages more readable (e.g., "invalid address type: EVM" instead of "invalid address type: 1").
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
crypto/bech32.go
🧰 Additional context used
🧬 Code graph analysis (1)
crypto/bech32.go (2)
io/keyfile.go (1)
Key(11-15)context/context.go (1)
Key(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Integration Tests on Ubuntu
🔇 Additional comments (4)
crypto/bech32.go (4)
25-30: LGTM: HD paths follow BIP-44 standard.The EVMHDPath correctly uses coin type 60 for Ethereum, and CosmosHDPath uses 118 for Cosmos, both conforming to BIP-44 standards.
72-87: LGTM: Cosmos address derivation is correct.The function correctly implements Cosmos SDK address derivation using compressed public keys with SHA256 and RIPEMD160 hashing, returning raw 20-byte address hash as expected.
89-111: LGTM: EVM address derivation follows Ethereum standard.The implementation correctly follows the Ethereum address generation algorithm:
- Uncompressed public key (65 bytes)
- Removes 0x04 prefix to get 64-byte X,Y coordinates
- Applies Keccak256 hash (correctly using legacy Keccak, not standardized SHA3)
- Takes last 20 bytes as the address
Returns raw bytes for consistent handling with the Cosmos path.
33-70: Excellent fix for the critical bech32 conversion issue!The previous critical issue has been resolved correctly. The bech32 bit conversion (line 60) is now properly applied to address bytes from both Cosmos and EVM derivation paths before encoding (line 64). This centralized approach is cleaner than duplicating the conversion in each helper function.
The breaking change to the function signature (added
addressTypeparameter) has been properly handled—all callers inio/keyfile.goandmodels/initialize.gohave been updated to pass the required parameter.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
registry/registry_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Integration Tests on Ubuntu
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
cmd/opinit_integration_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/opinit_integration_test.go (1)
testutil/alias.go (4)
PressEnter(10-10)WaitFetching(16-19)PressTab(12-12)TypeText(22-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Integration Tests on Ubuntu
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
cmd/opinit_integration_test.go (1)
137-141: [rewritten comment]
[classification tag]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/relayer_integration_test.go (1)
72-74: Update comments to reflect the network name change.Comments still reference "minievm" but the code now searches for "Evm". Update these for consistency.
🔎 Proposed fix
- steps = append(steps, pressDownSteps...) // press down until its minievm + steps = append(steps, pressDownSteps...) // press down until its evm steps = append(steps, testutil.Steps{ - testutil.PressEnter, // press enter to confirm selecting minievm + testutil.PressEnter, // press enter to confirm selecting evm
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
cmd/relayer_integration_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Integration Tests on Ubuntu
🔇 Additional comments (1)
cmd/relayer_integration_test.go (1)
52-59: Verify that the registry contains a network with PrettyName "Evm".The test searches for
network.PrettyNamematching "Evm" (case-insensitive), but this depends on external registry data loaded fromhttps://registry.testnet.initia.xyz/chains.jsonorhttps://registry.initia.xyz/chains.json. Confirm that the registry has been updated to include a network with the PrettyName "Evm", otherwise the test will fail.Additionally, variable names and comments still reference the "minievm" pattern (lines 42, 53, 58, 62–63, 72–74), which is now inconsistent with the search term "Evm".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crypto/bech32_test.go (2)
138-140: Consider a more precise HRP prefix check.The current
assert.Containschecks if the HRP exists anywhere in the address string. For Bech32 addresses, the HRP is always a prefix followed by "1". Consider using a prefix check for better precision:assert.True(t, strings.HasPrefix(address, tt.hrp+"1"), "Address should start with HRP followed by '1'")This is a minor improvement since line 142 already validates the exact address when
expectedis provided.
88-102: Optional: Consider additional test vectors for broader coverage.The current test cases provide solid baseline coverage using a single mnemonic and HRP. For more comprehensive validation, you could consider adding:
- Test cases with different HRPs (e.g., "cosmos", "osmo") to ensure the function works with various prefixes
- Additional mnemonic test vectors to increase confidence in the derivation logic
However, the current coverage is appropriate for initial implementation and can be expanded later if needed.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
crypto/bech32_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
crypto/bech32_test.go (1)
crypto/bech32.go (4)
AddressType(18-18)CosmosAddressType(21-21)EVMAddressType(22-22)MnemonicToBech32Address(33-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Integration Tests on Ubuntu
🔇 Additional comments (1)
crypto/bech32_test.go (1)
78-147: Excellent test coverage for the new address type functionality.The table-driven test structure is clean and comprehensive, covering both Cosmos and EVM address types as well as various error conditions. The test cases validate the core behavior of
MnemonicToBech32Addresswith the newAddressTypeparameter.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
User-facing Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.