refactor: update validation functions and add hex address validation … #181
Conversation
…tests - Removed unused dependency on github.com/BurntSushi/toml from go.mod and go.sum. - Renamed validation functions for clarity: IsValidDNS to ValidateDNS, IsValidPeerOrSeed to ValidatePeerOrSeed, IsValidInteger to ValidateInteger, IsValidAddress to ValidateAddress, and IsValidAddresses to ValidateAddresses. - Introduced new validation functions: ValidateAnyHexAddressOrAddress and ValidateAddressesOrHexAddresses for improved address validation. - Added unit tests for hex address validation in a new test file bech32_test.go.
Benzbeeb
requested review from
WasinWatt,
songwongtp and
traviolus
as code owners
WalkthroughThe changes standardize validation function naming from Changes
Sequence Diagram(s)Hex Public Key to Bech32 Address Conversion in Genesis Account HandlingsequenceDiagram
participant User
participant InputValidator
participant GenesisAccountHandler
participant Bech32Converter
User->>InputValidator: Enter address (hex or Bech32)
InputValidator->>GenesisAccountHandler: Pass validated address
GenesisAccountHandler->>GenesisAccountHandler: Check if address starts with "0x"
alt Address is hex
GenesisAccountHandler->>Bech32Converter: Convert hex to Bech32
Bech32Converter-->>GenesisAccountHandler: Return Bech32 address
end
GenesisAccountHandler->>State: Store Bech32 address
Fee Whitelist Handling with Mixed Address FormatssequenceDiagram
participant User
participant InputValidator
participant FeeWhitelistHandler
participant Bech32Converter
User->>InputValidator: Enter comma-separated addresses (hex/Bech32)
InputValidator->>FeeWhitelistHandler: Pass validated addresses
loop For each address
FeeWhitelistHandler->>FeeWhitelistHandler: Check if address starts with "0x"
alt Address is hex
FeeWhitelistHandler->>Bech32Converter: Convert hex to Bech32
Bech32Converter-->>FeeWhitelistHandler: Return Bech32 address
end
FeeWhitelistHandler->>Map: Store Bech32 address
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 comments (1)
common/validate_test.go (1)
209-209: Update test expectation to match the refined error handling.The pipeline failure indicates that the error message expectation in the "missing port" test case needs to be updated to match the refined error handling in
ValidateURLWithPort.Based on the pipeline failure, the expected error message should be changed:
{ name: "missing port", url: "http://example.com", wantErr: true, - errMsg: "URL must include a port number", + errMsg: "invalid port number: must be between 1 and 65535", },This aligns with the change in Line 209 of
common/validate.gowhere the error handling was refined to allow "missing port in address" errors to pass through to the port validation logic.
🧹 Nitpick comments (4)
crypto/bech32.go (1)
110-149: Consider adding input length validation.The function correctly handles the core conversion logic, but could benefit from stricter input validation to prevent potential issues with malformed inputs.
Consider adding validation for expected public key lengths:
func PubKeyToBech32Address(pubKeyHex string) (string, error) { // Remove "0x" prefix if present pubKeyHex = strings.TrimPrefix(pubKeyHex, "0x") // Decode the hex string to bytes pubKeyBytes, err := hex.DecodeString(pubKeyHex) if err != nil { return "", fmt.Errorf("failed to decode hex string: %w", err) } + + // Validate input length + if len(pubKeyBytes) != 20 && len(pubKeyBytes) != 33 && len(pubKeyBytes) != 65 { + return "", fmt.Errorf("invalid input length: expected 20 (address hash), 33 (compressed pubkey), or 65 (uncompressed pubkey) bytes, got %d", len(pubKeyBytes)) + } // Check if the input is already a 20-byte hash (RIPEMD160) var addressHash []bytecrypto/bech32_test.go (1)
9-73: Remove unusedhrpfield from test struct.The test struct includes an
hrpfield that is never used sincePubKeyToBech32Addressdoesn't accept an HRP parameter. The function uses a hardcoded HRP internally.func TestPubKeyToBech32Address(t *testing.T) { tests := []struct { name string pubKeyHex string - hrp string expected string expectErr bool }{ { name: "Valid init address with 0x prefix (1)", pubKeyHex: "0x932d1475bbad306322a839238d56fe5dc9184744", - hrp: "init", expected: "init1jvk3gadm45cxxg4g8y3c64h7thy3s36yat0ezy", }, { name: "Valid init address without 0x prefix (2)", pubKeyHex: "0x552bfcf61b41b22eab0a520b896b072a1cd22b8c", - hrp: "init", expected: "init1254leasmgxeza2c22g9cj6c89gwdy2uvwv05qu", }, { name: "Valid init address without 0x prefix", pubKeyHex: "932d1475bbad306322a839238d56fe5dc9184744", - hrp: "init", expected: "init1jvk3gadm45cxxg4g8y3c64h7thy3s36yat0ezy", }, { name: "Valid init capital address with 0x prefix ", pubKeyHex: "0x932D1475BBAD306322A839238D56FE5DC9184744", - hrp: "init", expected: "init1jvk3gadm45cxxg4g8y3c64h7thy3s36yat0ezy", }, { name: "Invalid init address with 0x prefix", pubKeyHex: "0x932D1475BBAD306322A839238D56FE5DC91847441", - hrp: "init", expectErr: true, }, { name: "Invalid init address without 0x prefix", pubKeyHex: "932D1475BBAD306322A839238D56FE5DC91847441", - hrp: "init", expectErr: true, }, { name: "Invalid init address", pubKeyHex: "invalid", - hrp: "init", expectErr: true, }, }Otherwise, the test coverage is comprehensive and well-structured, covering valid hex addresses with different formats and invalid inputs.
common/validate_test.go (1)
226-257: Rename test function to match the function being tested.The test function
TestValidateHexAddressis actually testingValidateAnyHexAddressOrAddress, which is misleading.-func TestValidateHexAddress(t *testing.T) { +func TestValidateAnyHexAddressOrAddress(t *testing.T) {Also update the error message in line 253:
- t.Errorf("IsValidHexAddress() %s error = %v, wantErr %v", tt.name, err, tt.wantErr) + t.Errorf("ValidateAnyHexAddressOrAddress() %s error = %v, wantErr %v", tt.name, err, tt.wantErr)The test coverage itself looks good with valid and invalid hex address cases.
common/validate.go (1)
229-229: Consider standardizing ValidateDNS return type for consistency.While the function was renamed from
IsValidDNStoValidateDNSto follow the naming convention, it still returnsboolinstead oferrorlike otherValidate*functions. This creates inconsistency in the validation API.Consider updating the function signature and implementation:
-func ValidateDNS(dns string) bool { +func ValidateDNS(dns string) error { // Regular expression for validating DNS names dnsRegex := `^([a-zA-Z0-9]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.)+[a-zA-Z]{2,}$` re := regexp.MustCompile(dnsRegex) - // Validate DNS name - return re.MatchString(dns) + if !re.MatchString(dns) { + return fmt.Errorf("invalid DNS name: %s", dns) + } + return nil }And update the usage in line 285:
- if net.ParseIP(host) == nil && !ValidateDNS(host) { + if net.ParseIP(host) == nil && ValidateDNS(host) != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
common/validate.go(8 hunks)common/validate_test.go(1 hunks)crypto/bech32.go(3 hunks)crypto/bech32_test.go(1 hunks)go.mod(0 hunks)models/initia/run_l1_node.go(3 hunks)models/minitia/launch.go(5 hunks)models/opinit_bots/init.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🧰 Additional context used
🧬 Code Graph Analysis (4)
models/opinit_bots/init.go (1)
common/validate.go (1)
ValidateInteger(323-332)
crypto/bech32_test.go (1)
crypto/bech32.go (1)
PubKeyToBech32Address(111-149)
common/validate_test.go (1)
common/validate.go (2)
ValidateAnyHexAddressOrAddress(361-376)ValidateAddressesOrHexAddresses(378-386)
models/initia/run_l1_node.go (1)
common/validate.go (1)
ValidatePeerOrSeed(241-305)
🪛 GitHub Actions: Basic Checks
common/validate_test.go
[error] 217-217: TestValidateURLWithPort/missing_port failed: Expected error message to contain 'URL must include a port number' but got 'invalid port number: must be between 1 and 65535'.
⏰ 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 (18)
crypto/bech32.go (1)
20-20: LGTM!The
InitHRPconstant is well-named and follows the established pattern for Bech32 human-readable prefixes in the Cosmos ecosystem.models/opinit_bots/init.go (1)
1030-1030: LGTM!The function name change from
IsValidIntegertoValidateIntegeraligns with the standardization effort mentioned in the PR objectives. The validation logic remains unchanged and is appropriate for validating L1 start height input.models/initia/run_l1_node.go (3)
583-583: LGTM! Function renaming correctly applied.The update from
common.IsValidPeerOrSeedtocommon.ValidatePeerOrSeedis consistent with the validation function standardization effort in this PR.
651-651: LGTM! Function renaming correctly applied.The update from
common.IsValidPeerOrSeedtocommon.ValidatePeerOrSeedis consistent with the validation function standardization effort in this PR.
1689-1689: LGTM! Function renaming correctly applied.The update from
common.IsValidPeerOrSeedtocommon.ValidatePeerOrSeedis consistent with the validation function standardization effort in this PR.common/validate_test.go (1)
259-290: LGTM! Comprehensive test coverage for multiple address validation.The test cases properly cover comma-separated hex addresses, mixed address types, and invalid addresses.
common/validate.go (7)
209-209: LGTM! Refined error handling for missing port scenario.The change correctly allows "missing port in address" errors to pass through to port validation logic, ensuring appropriate error messages are returned.
241-241: LGTM! Function renaming correctly applied.The update from
IsValidPeerOrSeedtoValidatePeerOrSeedis consistent with the validation function standardization effort.
285-285: LGTM! Updated function call matches renaming.The call correctly uses the renamed
ValidateDNSfunction.
323-323: LGTM! Function renaming correctly applied.The update from
IsValidIntegertoValidateIntegeris consistent with the validation function standardization effort.
334-334: LGTM! Function renaming correctly applied.The update from
IsValidAddresstoValidateAddressis consistent with the validation function standardization effort.
344-350: LGTM! Function renaming correctly applied.The update from
IsValidAddressestoValidateAddressesand the internal call toValidateAddressare consistent with the validation function standardization effort.
361-386: LGTM! Well-implemented new validation functions.The new functions
ValidateAnyHexAddressOrAddressandValidateAddressesOrHexAddressesproperly extend address validation to support both hex-prefixed and Bech32 addresses. The implementation correctly:
- Validates hex addresses using
crypto.PubKeyToBech32Address- Validates Bech32 addresses using existing
ValidateAddress- Provides clear error messages for invalid formats
- Handles comma-separated lists appropriately
These functions integrate well with the existing validation framework.
models/minitia/launch.go (5)
25-25: LGTM!The crypto package import is correctly added and properly positioned within the local imports section.
2119-2129: LGTM! Hex address conversion is properly implemented.The implementation correctly:
- Checks for hex prefix "0x"
- Converts hex public keys to Bech32 addresses
- Preserves the original address for display
- Handles errors appropriately
3087-3098: LGTM! Fee whitelist hex address handling is consistent.The implementation properly:
- Converts hex addresses to Bech32 format before processing
- Uses a cache to prevent duplicate addresses
- Maintains consistency with the genesis account address handling
2048-2048: ValidateAnyHexAddressOrAddress implementation verifiedThe function in common/validate.go (lines 361–376) correctly handles both “0x”-prefixed hex addresses (via PubKeyToBech32Address) and Bech32 addresses (via ValidateAddress), returning an error for any other format. No issues found.
1993-1993: Validation logic confirmedThe
ValidateAddressesOrHexAddressesfunction correctly splits on commas and validates each segment as either a hex or Bech32 address. The implementation incommon/validate.goand the accompanying tests incommon/validate_test.gocover mixed formats and invalid entries, so no further changes are needed.
- Updated the PubKeyToBech32Address function to include padding for odd-length hex strings. - Introduced a new helper function, getPaddedBytes, to manage padding logic. - Revised test cases in bech32_test.go to cover additional valid and invalid address scenarios, removing unnecessary hrp parameters.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crypto/bech32.go (1)
20-20: Consider making HRP configurable for module addresses.The hardcoded
InitHRPconstant works for the current use case, but consider making it configurable to support different address types like module addresses mentioned in previous reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crypto/bech32.go(3 hunks)crypto/bech32_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crypto/bech32_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 (2)
crypto/bech32.go (2)
5-5: LGTM!The
encoding/heximport is correctly added and needed for the new hex decoding functionality.
110-144: Well-structured function with good error handling.The function correctly handles hex decoding, padding, and Bech32 encoding. The error handling is comprehensive and the logic flow is clear.
3 participants
…tests
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
Refactor
Tests
Chores