[CP-508] add address creation from eth #389
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughA new class method, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Address
User->>Address: from_eth_address(eth_address)
Address->>Address: Remove "0x" prefix if present
Address->>Address: Convert hex string to bytes
Address-->>User: Return Address instance
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to construct Address instances directly from Ethereum hex addresses and validates the new functionality with tests.
- Introduces
Address.from_eth_addressfor parsing hex-encoded Ethereum addresses - Adds
TestAddressintests/test_wallet.pywith round-trip tests for Bech32 ↔ Ethereum conversions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_wallet.py | Added TestAddress class with tests for from_acc_bech32 and from_eth_address |
| pyinjective/wallet.py | Implemented Address.from_eth_address to convert hex-encoded Ethereum addresses to Address |
Comments suppressed due to low confidence (4)
pyinjective/wallet.py:236
- Add input validation to ensure the hex string (excluding prefix) is exactly 40 characters long (20 bytes) and raise a clear
ValueErrorfor invalid lengths.
def from_eth_address(cls, eth_address: str) -> "Address":
tests/test_wallet.py:42
- Consider adding a test case for hex addresses without the '0x' prefix to verify that
from_eth_addresshandles non-prefixed inputs correctly.
address = Address.from_eth_address("0xbdaedec95d563fb05240d6e01821008454c24c36")
tests/test_wallet.py:42
- Add a test for uppercase '0X' prefix to ensure
from_eth_addresscorrectly handles case-insensitive prefixes.
address = Address.from_eth_address("0xbdaedec95d563fb05240d6e01821008454c24c36")
pyinjective/wallet.py:236
- [nitpick] Consider renaming
from_eth_addresstofrom_ethereum_addressfor clarity and consistency with other method names.
def from_eth_address(cls, eth_address: str) -> "Address":
| @classmethod | ||
| def from_eth_address(cls, eth_address: str) -> "Address": | ||
| """Create an address instance from a hex-encoded Ethereum address""" | ||
| eth_address_without_prefix = eth_address[2:] if eth_address.startswith("0x") else eth_address |
There was a problem hiding this comment.
The prefix check is case-sensitive; consider using eth_address.lower().startswith("0x") to also support uppercase '0X'.
Suggested change
| eth_address_without_prefix = eth_address[2:] if eth_address.startswith("0x") else eth_address | |
| eth_address_without_prefix = eth_address[2:] if eth_address.lower().startswith("0x") else eth_address |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_wallet.py (1)
33-46: Good bidirectional conversion tests, but consider adding edge case coverage.The tests effectively validate the core functionality and bidirectional conversion between Bech32 and Ethereum address formats. The hardcoded test data appears valid and the assertions correctly verify round-trip conversions.
Consider adding tests for edge cases:
def test_from_eth_edge_cases(self): # Test without 0x prefix address1 = Address.from_eth_address("bdaedec95d563fb05240d6e01821008454c24c36") address2 = Address.from_eth_address("0xbdaedec95d563fb05240d6e01821008454c24c36") assert address1 == address2 # Test invalid inputs (should raise ValueError) with pytest.raises(ValueError): Address.from_eth_address("invalid_hex") with pytest.raises(ValueError): Address.from_eth_address("0x123") # Too short with pytest.raises(ValueError): Address.from_eth_address("0x" + "a" * 42) # Too long
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyinjective/wallet.py(1 hunks)tests/test_wallet.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_wallet.py (1)
pyinjective/wallet.py (7)
Address(199-307)from_acc_bech32(133-134)from_acc_bech32(221-223)to_acc_bech32(163-165)to_acc_bech32(247-249)get_ethereum_address(268-269)from_eth_address(236-240)
Comment on lines
+235
to
+240
| @classmethod | ||
| def from_eth_address(cls, eth_address: str) -> "Address": | ||
| """Create an address instance from a hex-encoded Ethereum address""" | ||
| eth_address_without_prefix = eth_address[2:] if eth_address.startswith("0x") else eth_address | ||
| bytes_representation = bytes.fromhex(eth_address_without_prefix) | ||
| return cls(bytes_representation) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling for robustness.
The method lacks validation for Ethereum address format requirements. Consider adding:
- Length validation: Ethereum addresses are exactly 20 bytes (40 hex characters)
- Hex validation: Better error handling for invalid hex strings
- Format validation: Ensure proper address format
@classmethod
def from_eth_address(cls, eth_address: str) -> "Address":
"""Create an address instance from a hex-encoded Ethereum address"""
- eth_address_without_prefix = eth_address[2:] if eth_address.startswith("0x") else eth_address
- bytes_representation = bytes.fromhex(eth_address_without_prefix)
- return cls(bytes_representation)
+ # Remove 0x prefix if present
+ eth_address_without_prefix = eth_address[2:] if eth_address.startswith("0x") else eth_address
+
+ # Validate hex string length (should be 40 characters for 20 bytes)
+ if len(eth_address_without_prefix) != 40:
+ raise ValueError(f"Invalid Ethereum address length: expected 40 characters, got {len(eth_address_without_prefix)}")
+
+ try:
+ bytes_representation = bytes.fromhex(eth_address_without_prefix)
+ except ValueError as e:
+ raise ValueError(f"Invalid hex string in Ethereum address: {e}")
+
+ return cls(bytes_representation)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @classmethod | |
| def from_eth_address(cls, eth_address: str) -> "Address": | |
| """Create an address instance from a hex-encoded Ethereum address""" | |
| eth_address_without_prefix = eth_address[2:] if eth_address.startswith("0x") else eth_address | |
| bytes_representation = bytes.fromhex(eth_address_without_prefix) | |
| return cls(bytes_representation) | |
| @classmethod | |
| def from_eth_address(cls, eth_address: str) -> "Address": | |
| """Create an address instance from a hex-encoded Ethereum address""" | |
| # Remove 0x prefix if present | |
| eth_address_without_prefix = eth_address[2:] if eth_address.startswith("0x") else eth_address | |
| # Validate hex string length (should be 40 characters for 20 bytes) | |
| if len(eth_address_without_prefix) != 40: | |
| raise ValueError( | |
| f"Invalid Ethereum address length: expected 40 characters, got {len(eth_address_without_prefix)}" | |
| ) | |
| try: | |
| bytes_representation = bytes.fromhex(eth_address_without_prefix) | |
| except ValueError as e: | |
| raise ValueError(f"Invalid hex string in Ethereum address: {e}") | |
| return cls(bytes_representation) |
🤖 Prompt for AI Agents
In pyinjective/wallet.py around lines 235 to 240, the from_eth_address method
lacks input validation and error handling. Add checks to ensure the input string
is a valid Ethereum address by verifying it starts with "0x" or is 40 hex
characters long, confirm the length after removing the prefix is exactly 40
characters, and wrap the bytes.fromhex call in a try-except block to catch
invalid hex strings, raising a clear error if validation fails.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solves CP-508
Summary by CodeRabbit