Correct hex address formatting with checksum - #58
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughBumps package version from 0.1.2 to 0.1.3 in packages/utils/package.json. Modifies Bech32 -> hex conversion to derive the hex address using Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Bech32ToHex function to use checksum-validated Ethereum addresses by leveraging ethers' getAddress function and CosmJS's toHex utility, ensuring proper address formatting with mixed case checksums. The version is bumped to 0.1.3.
- Replaces manual Buffer-based hex conversion with
getAddress(toHex(data))for EIP-55 checksum address formatting - Updates test expectations to match checksummed address format
- Bumps package version to 0.1.3
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/utils/src/ethers/bech32Conversion.ts | Replaces manual Buffer conversion with getAddress(toHex(data)) and adds new imports |
| packages/utils/tests/address.test.ts | Updates expected hex address to use EIP-55 checksummed format |
| packages/utils/package.json | Bumps version from 0.1.2 to 0.1.3 |
Comments suppressed due to low confidence (1)
packages/utils/src/ethers/bech32Conversion.ts:1
- Unused import normalizeBech32.
import { fromBech32, normalizeBech32, toHex } from '@cosmjs/encoding';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/utils/src/ethers/bech32Conversion.ts (1)
9-17: <!-- [scratchpad_start]
[task verification_complete][observations]
- getAddress() used only once in codebase: bech32Conversion.ts line 15
- No try-catch wrapping anywhere
- Test file has only happy path test (no error cases)
- Related HexToBech32 function uses explicit validation/null returns (defensive pattern)
- Function signature: string | null (explicitly signals null should be returned for errors)
- fromBech32() is also uncaught (could throw on invalid bech32)
[analysis]
- Review concern is VALID: getAddress() throws if data not 20 bytes
- Inconsistency confirmed: signature says string | null but code can throw
- HexToBech32 in same file validates lengths and returns null (pattern shows codebase favors null over throws)
- No documented constraint that kii bech32 ALWAYS produces 20-byte data
- fromBech32() also has similar unhandled potential
[conclusion]
The review comment correctly identifies a type safety issue:
- Function signature promises string | null (never throws)
- Implementation can throw from getAddress() if data isn't 20 bytes
- This is an inconsistency that should be resolved
- Suggested fix (try-catch) aligns with pattern in HexToBech32
- Verification complete: issue is valid and should be addressed
[tag]
Wrap
getAddress()in error handling to match return type promise.The concern in the review comment is valid. The function signature declares
string | nullreturn type, which implicitly promises to return null for error cases rather than throwing. However,getAddress()will throw if the input is not a valid 20-byte address, creating an inconsistency.Additionally,
fromBech32()on line 12 also lacks error handling and can throw for invalid bech32 input. The relatedHexToBech32function in the same file (lines 24-32) follows a defensive pattern with explicit validation and null returns, establishing the codebase's preferred error handling strategy.The suggested try-catch wrapper is the appropriate fix:
export function Bech32ToHex(bechAddress: string): string | null { if (!bechAddress || typeof bechAddress !== 'string') return null; const { prefix, data } = fromBech32(bechAddress); if (prefix !== 'kii') return null; try { const hex = getAddress(toHex(data)); return hex; } catch { return null; } }Consider also wrapping the
fromBech32()call for completeness.
🧹 Nitpick comments (1)
packages/utils/src/ethers/bech32Conversion.ts (1)
1-3: Remove unused importnormalizeBech32.The
normalizeBech32import is not used anywhere in this file.Apply this diff:
-import { fromBech32, normalizeBech32, toHex } from '@cosmjs/encoding'; +import { fromBech32, toHex } from '@cosmjs/encoding'; import { getAddress, isAddress } from 'ethers'; import { toBech32 } from '@cosmjs/encoding';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gz
📒 Files selected for processing (3)
packages/utils/package.json(1 hunks)packages/utils/src/ethers/bech32Conversion.ts(2 hunks)packages/utils/tests/address.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint and Format
packages/utils/src/ethers/bech32Conversion.ts
[warning] 1-1:
'normalizeBech32' is defined but never used
⏰ 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: Upload results
- GitHub Check: analyze
🔇 Additional comments (2)
packages/utils/package.json (1)
3-3: LGTM! Appropriate version bump for a bug fix.The patch version increment from 0.1.2 to 0.1.3 correctly follows semantic versioning for a non-breaking bug fix.
packages/utils/tests/address.test.ts (1)
4-4: LGTM! Test expectation correctly updated for checksummed address.The updated hex address features EIP-55 checksumming (mixed case), which correctly reflects the fix in
Bech32ToHex. The test now validates that the function returns a properly checksummed Ethereum address.
Description
Make the bech32 conversion return a checksummed Hex address.
Type of change
Please delete options that are not relevant.