feat: Extend AccountId functionality and improve alias handling
#1009
Conversation
|
Request review if available @hiero-ledger/hiero-sdk-python-triage |
manishdait
force-pushed
the
feat/account-id-imp
branch
2 times, most recently
from
09b92b4 to
3282819
Compare
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! From the Hiero Python SDK Team |
manishdait
force-pushed
the
feat/account-id-imp
branch
from
1a85749 to
ddcc5c9
Compare
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @manishdait
Great work!
Overall mostly nit pits, will leave it up to you what is actually worth it
Thanks again
examples/transaction/transfer_transaction_hbar_with_evm_address.py
Outdated
Show resolved
Hide resolved
examples/transaction/transfer_transaction_hbar_with_evm_address.py
Outdated
Show resolved
Hide resolved
examples/transaction/transfer_transaction_hbar_with_evm_address.py
Outdated
Show resolved
Hide resolved
examples/transaction/transfer_transaction_hbar_with_evm_address.py
Outdated
Show resolved
Hide resolved
examples/transaction/transfer_transaction_hbar_with_evm_address.py
Outdated
Show resolved
Hide resolved
|
Hello, this is the Office Hour Bot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request or receive assistance from a maintainer. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here to be notified of any changes. |
manishdait
force-pushed
the
feat/account-id-imp
branch
from
ddcc5c9 to
ba98f72
Compare
📝 WalkthroughWalkthroughAdds first-class EVM address alias support to AccountId (parsing, serialization, equality/hash), mirror-node population helpers and solidity-address conversion, updates local mirror URL, adds an example demonstrating mirror-driven AccountId population, and includes unit and integration tests for the new flows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AccountId
participant Client
participant MirrorNode as "Mirror Node"
rect rgb(230,245,255)
Note over AccountId,MirrorNode: populate_account_num (evm_address -> num)
User->>AccountId: populate_account_num(client)
AccountId->>Client: build GET /api/v1/accounts/{evmAddress}
Client->>MirrorNode: GET /api/v1/accounts/{evmAddress}
MirrorNode-->>Client: 200 JSON {account:{shard,realm,num}}
Client-->>AccountId: parsed JSON
AccountId->>AccountId: set shard/realm/num
AccountId-->>User: return populated AccountId
end
rect rgb(255,245,235)
Note over AccountId,MirrorNode: populate_evm_address (shard.realm.num -> evm_address)
User->>AccountId: populate_evm_address(client)
AccountId->>Client: build GET /api/v1/accounts/{shard}.{realm}.{num}
Client->>MirrorNode: GET /api/v1/accounts/{shard}.{realm}.{num}
MirrorNode-->>Client: 200 JSON {evm_address:"0x..."}
Client-->>AccountId: parsed JSON
AccountId->>AccountId: set evm_address
AccountId-->>User: return populated AccountId
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1009 +/- ##
==========================================
+ Coverage 91.79% 91.90% +0.11%
==========================================
Files 139 139
Lines 8466 8562 +96
==========================================
+ Hits 7771 7869 +98
+ Misses 695 693 -2 🚀 New features to boost your workflow:
|
manishdait
force-pushed
the
feat/account-id-imp
branch
3 times, most recently
from
3151f07 to
e036b6c
Compare
|
Hi @manishdait, This pull request has had no commit activity for 10 days. Are you still working on the issue? please push a commit to keep the PR active or it will be closed due to inactivity. From the Python SDK Team |
manishdait
force-pushed
the
feat/account-id-imp
branch
from
e036b6c to
a541cc3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.mdexamples/account/account_id_populate_from_mirror.pysrc/hiero_sdk_python/account/account_id.pysrc/hiero_sdk_python/client/network.pysrc/hiero_sdk_python/utils/entity_id_helper.pytests/integration/account_id_population_e2e_test.pytests/unit/account_id_test.pytests/unit/entity_id_helper_test.py
🧰 Additional context used
📓 Path-based instructions (3)
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/account_id_population_e2e_test.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/account_id_test.pytests/unit/entity_id_helper_test.py
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/account/account_id_populate_from_mirror.py
🧬 Code graph analysis (4)
tests/integration/account_id_population_e2e_test.py (1)
src/hiero_sdk_python/account/account_id.py (5)
AccountId(24-383)to_evm_address(336-341)from_evm_address(131-164)populate_account_num(286-313)populate_evm_address(315-334)
tests/unit/entity_id_helper_test.py (1)
src/hiero_sdk_python/utils/entity_id_helper.py (2)
perform_query_to_mirror_node(129-149)to_solidity_address(151-160)
src/hiero_sdk_python/account/account_id.py (2)
src/hiero_sdk_python/crypto/evm_address.py (1)
EvmAddress(1-55)src/hiero_sdk_python/utils/entity_id_helper.py (2)
perform_query_to_mirror_node(129-149)to_solidity_address(151-160)
examples/account/account_id_populate_from_mirror.py (1)
src/hiero_sdk_python/account/account_id.py (4)
AccountId(24-383)from_evm_address(131-164)populate_account_num(286-313)populate_evm_address(315-334)
🪛 Ruff (0.14.10)
tests/integration/account_id_population_e2e_test.py
24-24: Redefinition of unused env from line 13
(F811)
65-65: Redefinition of unused env from line 13
(F811)
src/hiero_sdk_python/utils/entity_id_helper.py
5-5: typing.Dict is deprecated, use dict instead
(UP035)
133-133: Avoid specifying long messages outside the exception class
(TRY003)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
149-149: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
149-149: Avoid specifying long messages outside the exception class
(TRY003)
155-155: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/account_id_test.py
93-93: Comparison to None should be cond is None
Replace with cond is None
(E711)
152-152: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
212-212: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
650-650: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
679-679: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
695-695: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
704-704: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
751-751: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
tests/unit/entity_id_helper_test.py
142-142: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
164-164: Assertion always fails, replace with pytest.fail()
(PT015)
164-164: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
166-166: Found assertion on exception e in except block, use pytest.raises() instead
(PT017)
src/hiero_sdk_python/account/account_id.py
82-82: Avoid specifying long messages outside the exception class
(TRY003)
121-128: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Use X | Y for type annotations
Convert to X | Y
(UP007)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
154-156: Prefer TypeError exception for invalid type
(TRY004)
154-156: Avoid specifying long messages outside the exception class
(TRY003)
234-234: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Avoid specifying long messages outside the exception class
(TRY003)
292-292: Avoid specifying long messages outside the exception class
(TRY003)
301-301: Avoid specifying long messages outside the exception class
(TRY003)
304-307: Avoid specifying long messages outside the exception class
(TRY003)
311-311: Consider moving this statement to an else block
(TRY300)
313-313: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
313-313: Avoid specifying long messages outside the exception class
(TRY003)
318-318: Avoid specifying long messages outside the exception class
(TRY003)
326-326: Avoid specifying long messages outside the exception class
(TRY003)
329-331: Avoid specifying long messages outside the exception class
(TRY003)
examples/account/account_id_populate_from_mirror.py
54-54: Do not catch blind exception: Exception
(BLE001)
93-93: Do not catch blind exception: Exception
(BLE001)
119-119: Do not catch blind exception: Exception
(BLE001)
149-149: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.12)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (8)
src/hiero_sdk_python/utils/entity_id_helper.py (1)
151-159: LGTM with minor docstring clarification.The implementation correctly packs shard/realm/num into a 20-byte Solidity-compatible address. Consider clarifying the docstring since the function only converts account numbers to "long-zero" EVM addresses (not the reverse).
tests/unit/account_id_test.py (1)
1-14: Comprehensive test coverage for EVM address functionality.The tests thoroughly cover the new EVM address support including initialization, parsing, serialization, equality/hashing, and mirror node population flows. Good defensive testing with both happy paths and error cases.
tests/integration/account_id_population_e2e_test.py (2)
23-61: LGTM - Good integration test coverage.The test properly validates the end-to-end flow: creating an account via EVM address transfer, waiting for mirror node propagation, then populating and verifying the account number. The 5-second sleep is reasonable for mirror node propagation.
Note: The Ruff F811 warning about
envredefinition is a false positive - line 13 imports the module while the function parameterenvis a pytest fixture.
64-96: LGTM - Proper integration test for EVM address population.The test correctly validates the populate_evm_address flow and verifies the populated address matches the original.
examples/account/account_id_populate_from_mirror.py (1)
1-28: Well-structured example demonstrating AccountId mirror population.The example follows SDK example patterns correctly: modular functions, clear docstrings, proper error handling with
sys.exit(1), and demonstrates the newpopulate_account_numandpopulate_evm_addressmethods clearly for users.src/hiero_sdk_python/account/account_id.py (2)
195-202: LGTM - Correct alias deserialization logic.The alias handling correctly distinguishes between 20-byte EVM addresses (used directly) and public key aliases (which have a 2-byte protobuf prefix to strip).
62-128: Well-implementedfrom_stringenhancement for EVM address support.The parsing logic correctly handles all documented formats: standard
shard.realm.num, checksummed format, hex aliases, and standalone EVM addresses. The comments explaining whyshard=0, realm=0are used for EVM addresses are helpful.src/hiero_sdk_python/client/network.py (1)
31-31: Port 5551 is correct for the solo mirror node REST API.The change from
8080to5551aligns with the standard Hedera solo local network setup, where the local mirror node's REST API listens on port 5551.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/hiero_sdk_python/utils/entity_id_helper.py (1)
146-148: Remove debugThe
print(e)statement is a debug artifact that should be removed from library code. Additionally, the exception should preserve the chain withfrom efor proper traceback context.🔎 Proposed fix
except requests.RequestException as e: - print(e) - raise RuntimeError(f"Unexpected error while querying mirror node: {url}") + raise RuntimeError(f"Unexpected error while querying mirror node: {url}") from esrc/hiero_sdk_python/account/account_id.py (1)
309-313: Add exception chaining to preserve traceback context.The
raise ValueErrorshould includefrom eto preserve the exception chain for debugging.🔎 Proposed fix
try: self.num = int(account_id.split(".")[-1]) return self - except (ValueError, AttributeError): - raise ValueError(f"Invalid account format received: {account_id}") + except (ValueError, AttributeError) as e: + raise ValueError(f"Invalid account format received: {account_id}") from e
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdexamples/account/account_id_populate_from_mirror.pysrc/hiero_sdk_python/account/account_id.pysrc/hiero_sdk_python/utils/entity_id_helper.pytests/unit/account_id_test.pytests/unit/entity_id_helper_test.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/entity_id_helper_test.pytests/unit/account_id_test.py
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/account/account_id_populate_from_mirror.py
🧬 Code graph analysis (3)
tests/unit/entity_id_helper_test.py (1)
src/hiero_sdk_python/utils/entity_id_helper.py (4)
parse_from_string(16-32)generate_checksum(34-82)perform_query_to_mirror_node(129-148)to_solidity_address(150-159)
examples/account/account_id_populate_from_mirror.py (1)
src/hiero_sdk_python/account/account_id.py (6)
AccountId(24-383)from_string(63-128)to_evm_address(336-341)from_evm_address(131-164)populate_account_num(286-313)populate_evm_address(315-334)
src/hiero_sdk_python/account/account_id.py (2)
src/hiero_sdk_python/crypto/evm_address.py (1)
EvmAddress(1-55)src/hiero_sdk_python/utils/entity_id_helper.py (5)
parse_from_string(16-32)validate_checksum(84-110)format_to_string_with_checksum(118-127)perform_query_to_mirror_node(129-148)to_solidity_address(150-159)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/utils/entity_id_helper.py
5-5: typing.Dict is deprecated, use dict instead
(UP035)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/account_id_test.py
152-152: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
212-212: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
650-650: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
695-695: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
751-751: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
examples/account/account_id_populate_from_mirror.py
54-54: Do not catch blind exception: Exception
(BLE001)
93-93: Do not catch blind exception: Exception
(BLE001)
119-119: Do not catch blind exception: Exception
(BLE001)
149-149: Do not catch blind exception: Exception
(BLE001)
src/hiero_sdk_python/account/account_id.py
82-82: Avoid specifying long messages outside the exception class
(TRY003)
121-128: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Use X | Y for type annotations
Convert to X | Y
(UP007)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
154-156: Prefer TypeError exception for invalid type
(TRY004)
154-156: Avoid specifying long messages outside the exception class
(TRY003)
234-234: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Avoid specifying long messages outside the exception class
(TRY003)
292-292: Avoid specifying long messages outside the exception class
(TRY003)
301-301: Avoid specifying long messages outside the exception class
(TRY003)
304-307: Avoid specifying long messages outside the exception class
(TRY003)
311-311: Consider moving this statement to an else block
(TRY300)
313-313: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
313-313: Avoid specifying long messages outside the exception class
(TRY003)
318-318: Avoid specifying long messages outside the exception class
(TRY003)
326-326: Avoid specifying long messages outside the exception class
(TRY003)
329-331: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.11)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (22)
CHANGELOG.md (1)
74-74: LGTM!The changelog entry is now clear and properly describes the added functionality for EVM address aliases in
AccountId.tests/unit/entity_id_helper_test.py (3)
1-14: LGTM!Imports are properly organized and all referenced modules exist in the SDK structure.
126-143: LGTM!Tests for
to_solidity_addresscover valid inputs, zero values edge case, and out-of-range validation with propermatchpatterns. Good coverage per coding guidelines.
145-209: LGTM!Comprehensive test coverage for
perform_query_to_mirror_nodeincluding:
- Success case with mocked response
- All exception paths (RequestException, HTTPError, ConnectionError, Timeout)
- Input validation (None, empty string)
Tests are properly isolated with mocks and use
pytest.raiseswithmatchpatterns for clear failure diagnostics.src/hiero_sdk_python/utils/entity_id_helper.py (2)
2-5: LGTM!New imports for
struct,requests, and typing utilities are appropriate for the added functionality.
150-159: LGTM!The
to_solidity_addressfunction correctly packs shard/realm/num into a 20-byte Solidity-compatible address format with proper validation.tests/unit/account_id_test.py (6)
33-36: LGTM!Good fixture for generating EVM addresses from ECDSA keys for testing.
86-95: LGTM!Test properly verifies AccountId initialization with
evm_address, asserting all fields including thatalias_keyis None whenevm_addressis set.
323-415: LGTM!Comprehensive proto conversion tests for EVM address handling, including serialization and deserialization paths. Tests correctly verify that
accountNumis set to 0 when an alias (evm_address) is present.
653-778: LGTM!Thorough test coverage for
populate_account_numandpopulate_evm_addressmethods including:
- Successful mirror node queries
- Missing response fields
- Invalid response formats
- Precondition validation (evm_address/num required)
- RuntimeError wrapping for mirror node failures
Tests properly use mocks to avoid network calls per unit test isolation requirements.
792-846: LGTM!Byte serialization round-trip tests properly verify that
shard,realm,num,alias_key, andevm_addressare all preserved throughto_bytes()/from_bytes()conversions.
856-897: LGTM!Comprehensive tests for
from_evm_addresscovering valid hex strings (with/without0xprefix), None input, invalid types, and invalid hex strings.examples/account/account_id_populate_from_mirror.py (3)
1-29: LGTM!Module docstring clearly explains the example's purpose and the imports are valid top-level SDK imports per coding guidelines.
164-177: LGTM!The
main()function properly orchestrates the example flow: setup client → generate EVM address → auto-create account → demonstrate population methods.
80-92: No changes needed—this code pattern is correct.The
execute()method automatically callsfreeze_with(client)if the transaction hasn't been frozen yet (see line 331 insrc/hiero_sdk_python/transaction/transaction.py). The transaction is then automatically signed with the operator key if not already signed. This implicit freeze-and-sign pattern is valid and is the intended SDK behavior for transactions using operator auto-signing.While other examples explicitly call
.freeze_with(client)before.execute(client), both approaches are equivalent and correct.src/hiero_sdk_python/account/account_id.py (7)
6-17: LGTM!Imports are appropriate for the new EVM address functionality, including the
EvmAddressclass and utility functions for mirror node queries and Solidity address conversion.
38-60: LGTM!Constructor properly extended with
evm_addressparameter while maintaining backward compatibility. Documentation is updated accordingly.
62-128: LGTM!The
from_stringmethod properly handles all supported formats with clear documentation. Past review feedback has been addressed including comments explaining default values and comprehensive error messages.
130-164: LGTM!The
from_evm_addressclassmethod properly handles both string andEvmAddressinputs with appropriate validation and error handling.
195-203: LGTM!The
_from_protomethod correctly distinguishes between 20-byte EVM addresses and public key aliases. The 2-byte prefix stripping for key aliases removes the protobuf header (field tag + length) from serialized public keys.
315-334: LGTM!The
populate_evm_addressmethod correctly validates thatnumis set before querying the mirror node. The conditionif self.num is None or self.num == 0properly handles both the None case and the default 0 value (which indicates num hasn't been populated yet).
363-383: LGTM!The
__eq__and__hash__methods are correctly updated to includeevm_addressin comparisons, maintaining consistency between equality and hashing semantics.
manishdait
force-pushed
the
feat/account-id-imp
branch
2 times, most recently
from
d09c763 to
7fc256e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/hiero_sdk_python/account/account_id.py (1)
309-313: Add exception chaining for debugging context.The
exceptblock should preserve the original exception for debugging purposes. This was flagged in a previous review and by static analysis (B904).🔎 Proposed fix
try: self.num = int(account_id.split(".")[-1]) return self - except (ValueError, AttributeError): - raise ValueError(f"Invalid account format received: {account_id}") + except (ValueError, AttributeError) as e: + raise ValueError(f"Invalid account format received: {account_id}") from esrc/hiero_sdk_python/utils/entity_id_helper.py (1)
146-147: Add exception chaining to preserve the original error context.The
exceptblock capturesebut doesn't chain it in the raised exception. This makes debugging harder when unexpected errors occur.🔎 Proposed fix
except requests.RequestException as e: - raise RuntimeError(f"Unexpected error while querying mirror node: {url}") + raise RuntimeError(f"Unexpected error while querying mirror node: {url}") from e
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdexamples/account/account_id_populate_from_mirror.pysrc/hiero_sdk_python/account/account_id.pysrc/hiero_sdk_python/utils/entity_id_helper.pytests/unit/account_id_test.pytests/unit/entity_id_helper_test.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/account_id_test.pytests/unit/entity_id_helper_test.py
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/account/account_id_populate_from_mirror.py
🧬 Code graph analysis (1)
tests/unit/entity_id_helper_test.py (1)
src/hiero_sdk_python/utils/entity_id_helper.py (4)
parse_from_string(16-32)generate_checksum(34-82)perform_query_to_mirror_node(129-147)to_solidity_address(149-158)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/utils/entity_id_helper.py
5-5: typing.Dict is deprecated, use dict instead
(UP035)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
147-147: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/account_id_test.py
152-152: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
212-212: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
650-650: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
695-695: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
751-751: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
examples/account/account_id_populate_from_mirror.py
54-54: Do not catch blind exception: Exception
(BLE001)
93-93: Do not catch blind exception: Exception
(BLE001)
119-119: Do not catch blind exception: Exception
(BLE001)
149-149: Do not catch blind exception: Exception
(BLE001)
src/hiero_sdk_python/account/account_id.py
82-82: Avoid specifying long messages outside the exception class
(TRY003)
121-128: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Use X | Y for type annotations
Convert to X | Y
(UP007)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
154-156: Prefer TypeError exception for invalid type
(TRY004)
154-156: Avoid specifying long messages outside the exception class
(TRY003)
234-234: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Avoid specifying long messages outside the exception class
(TRY003)
292-292: Avoid specifying long messages outside the exception class
(TRY003)
301-301: Avoid specifying long messages outside the exception class
(TRY003)
304-307: Avoid specifying long messages outside the exception class
(TRY003)
311-311: Consider moving this statement to an else block
(TRY300)
313-313: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
313-313: Avoid specifying long messages outside the exception class
(TRY003)
318-318: Avoid specifying long messages outside the exception class
(TRY003)
326-326: Avoid specifying long messages outside the exception class
(TRY003)
329-331: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.10)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (12)
CHANGELOG.md (1)
74-74: LGTM!The changelog entry is now specific and descriptive, accurately summarizing the EVM address alias support added to
AccountId. This addresses the previous feedback about vagueness.tests/unit/account_id_test.py (4)
33-36: LGTM!The
evm_addressfixture follows the established pattern for generating test fixtures and properly derives an EVM address from an ECDSA key.
151-180: LGTM!Comprehensive parametrized test covering all valid
from_stringformats including standard, checksum, and EVM address inputs. The test correctly validates all AccountId fields.
226-261: LGTM!Good coverage of invalid input formats. The parametrized test correctly validates that various malformed inputs raise
ValueErrorwith an appropriate error message.
792-846: LGTM!Comprehensive roundtrip tests for
to_bytes/from_bytescovering basic numeric IDs, alias keys, and EVM addresses. Each test properly validates all fields are preserved through serialization.tests/unit/entity_id_helper_test.py (2)
126-143: LGTM!Good coverage of
to_solidity_addressincluding valid inputs, zero values, and boundary conditions. The out-of-range test properly validates the 32-bit shard limit.
145-209: LGTM!Comprehensive test coverage for
perform_query_to_mirror_nodecovering success case, various exception types (RequestException, HTTPError, ConnectionError, Timeout), and invalid URL inputs. Tests use proper mocking and meaningful error message matching.examples/account/account_id_populate_from_mirror.py (1)
164-177: LGTM!The main function properly orchestrates the example flow: client setup, EVM address generation, auto account creation, and both population examples. The TLS configuration was appropriately removed.
src/hiero_sdk_python/account/account_id.py (4)
62-128: LGTM!Well-structured parsing logic that handles multiple input formats gracefully. The error message clearly lists all supported formats, and the 20-byte check correctly distinguishes EVM addresses from public key aliases.
130-164: LGTM!Factory method properly validates input and handles both string and
EvmAddresstypes. The comments explain the shard/realm defaults and deferred num resolution clearly.
315-334: LGTM!Method properly validates that
numis available before querying the mirror node, addresses the previous review feedback about the condition check, and includes proper error handling with exception chaining.
381-383: Consider mutability implications for hash.Including
evm_addressin the hash means that callingpopulate_evm_address()will change the hash of an AccountId. This could cause issues if AccountIds are stored in sets or used as dictionary keys before population. This is a design choice, but worth documenting or considering immutable patterns.
manishdait
force-pushed
the
feat/account-id-imp
branch
from
7fc256e to
a52f9fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
tests/unit/account_id_test.py (1)
86-95: Fix docstring typo.The docstring says "Test AccountId initialization with alias key" but the test is for
evm_address. This was flagged in a previous review and appears unaddressed.🔎 Proposed fix
def test_initialization_with_evm_address(evm_address): - """Test AccountId initialization with alias key.""" + """Test AccountId initialization with evm_address.""" account_id = AccountId(shard=0, realm=0, num=0, evm_address=evm_address)src/hiero_sdk_python/utils/entity_id_helper.py (1)
146-147: Add exception chaining to preserve context.The
RequestExceptionhandler catches the exception but doesn't chain it in the raise, losing debugging context. This was flagged in a previous review and remains unaddressed.🔎 Proposed fix
except requests.RequestException as e: - raise RuntimeError(f"Unexpected error while querying mirror node: {url}") + raise RuntimeError(f"Unexpected error while querying mirror node: {url}") from eexamples/account/account_id_populate_from_mirror.py (2)
80-85: Consider addingfreeze_with(client)beforeexecute.Per SDK transaction lifecycle best practices, transactions should be frozen before execution. While
execute()may handle this internally, explicitly callingfreeze_with(client)demonstrates the documented pattern for users who copy-paste examples.🔎 Proposed fix
transfer_tx = ( TransferTransaction() .add_hbar_transfer(evm_account_id, Hbar(1).to_tinybars()) .add_hbar_transfer(client.operator_account_id, Hbar(-1).to_tinybars()) + .freeze_with(client) .execute(client) )
97-103: Add receipt status validation before checking children.Per coding guidelines, examples should validate
receipt.statusagainstResponseCodeenums to ensure the transaction succeeded before inspecting child receipts.🔎 Proposed fix
Add import at the top:
from hiero_sdk_python import ResponseCodeThen add validation:
+ if receipt.status != ResponseCode.SUCCESS: + print(f"Transfer failed with status: {ResponseCode(receipt.status).name}") + sys.exit(1) + if not receipt.children: print("Auto account creation failed: no child receipts found") sys.exit(1)src/hiero_sdk_python/account/account_id.py (1)
309-313: Add exception chaining to preserve context.The exception at line 313 should be chained to preserve debugging context. This was flagged in a previous review and remains unaddressed.
🔎 Proposed fix
try: self.num = int(account_id.split(".")[-1]) return self - except (ValueError, AttributeError): - raise ValueError(f"Invalid account format received: {account_id}") + except (ValueError, AttributeError) as e: + raise ValueError(f"Invalid account format received: {account_id}") from e
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.mdexamples/account/account_id_populate_from_mirror.pysrc/hiero_sdk_python/account/account_id.pysrc/hiero_sdk_python/utils/entity_id_helper.pytests/unit/account_id_test.pytests/unit/entity_id_helper_test.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/entity_id_helper_test.pytests/unit/account_id_test.py
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/account/account_id_populate_from_mirror.py
🧬 Code graph analysis (4)
tests/unit/entity_id_helper_test.py (1)
src/hiero_sdk_python/utils/entity_id_helper.py (4)
parse_from_string(16-32)generate_checksum(34-82)perform_query_to_mirror_node(129-147)to_solidity_address(149-158)
tests/unit/account_id_test.py (4)
src/hiero_sdk_python/account/account_id.py (13)
AccountId(24-383)to_evm_address(336-341)checksum(227-229)from_string(63-128)_is_evm_address(245-258)_to_proto(205-224)_from_proto(180-203)validate_checksum(231-242)from_evm_address(131-164)populate_account_num(286-313)populate_evm_address(315-334)to_bytes(343-345)from_bytes(167-177)src/hiero_sdk_python/crypto/evm_address.py (1)
EvmAddress(1-55)src/hiero_sdk_python/crypto/public_key.py (6)
to_evm_address(474-492)to_string(467-472)from_string(266-301)_to_proto(328-344)_from_proto(310-320)from_bytes(48-66)src/hiero_sdk_python/utils/entity_id_helper.py (1)
validate_checksum(84-110)
src/hiero_sdk_python/account/account_id.py (3)
src/hiero_sdk_python/crypto/evm_address.py (1)
EvmAddress(1-55)src/hiero_sdk_python/crypto/public_key.py (5)
PublicKey(22-556)from_string(266-301)from_bytes(48-66)_from_proto(310-320)to_string(467-472)src/hiero_sdk_python/utils/entity_id_helper.py (2)
perform_query_to_mirror_node(129-147)to_solidity_address(149-158)
examples/account/account_id_populate_from_mirror.py (1)
src/hiero_sdk_python/account/account_id.py (5)
AccountId(24-383)from_string(63-128)from_evm_address(131-164)populate_account_num(286-313)populate_evm_address(315-334)
🪛 Ruff (0.14.10)
src/hiero_sdk_python/utils/entity_id_helper.py
5-5: typing.Dict is deprecated, use dict instead
(UP035)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
147-147: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/account_id_test.py
152-152: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
212-212: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
650-650: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
695-695: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
751-751: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
src/hiero_sdk_python/account/account_id.py
82-82: Avoid specifying long messages outside the exception class
(TRY003)
121-128: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Use X | Y for type annotations
Convert to X | Y
(UP007)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
154-156: Prefer TypeError exception for invalid type
(TRY004)
154-156: Avoid specifying long messages outside the exception class
(TRY003)
234-234: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Avoid specifying long messages outside the exception class
(TRY003)
292-292: Avoid specifying long messages outside the exception class
(TRY003)
301-301: Avoid specifying long messages outside the exception class
(TRY003)
304-307: Avoid specifying long messages outside the exception class
(TRY003)
311-311: Consider moving this statement to an else block
(TRY300)
313-313: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
313-313: Avoid specifying long messages outside the exception class
(TRY003)
318-318: Avoid specifying long messages outside the exception class
(TRY003)
326-326: Avoid specifying long messages outside the exception class
(TRY003)
329-331: Avoid specifying long messages outside the exception class
(TRY003)
examples/account/account_id_populate_from_mirror.py
54-54: Do not catch blind exception: Exception
(BLE001)
93-93: Do not catch blind exception: Exception
(BLE001)
119-119: Do not catch blind exception: Exception
(BLE001)
149-149: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.13)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (11)
CHANGELOG.md (1)
74-74: LGTM!The changelog entry is clear, specific, and accurately describes the PR changes including parsing, serialization, and Mirror Node population helpers for EVM address aliases.
tests/unit/account_id_test.py (2)
653-666: Verify mock method name matches implementation.The test mocks
get_mirror_rest_urlwhich matches the actual method inaccount_id.py(line 294). This is correct - the previous review comment aboutget_mirror_node_rest_urlwas based on outdated code.
792-847: Good coverage of serialization roundtrips.The
to_bytes/from_bytestests comprehensively cover:
- Basic numeric AccountId
- AccountId with alias_key
- AccountId with evm_address
All tests verify field preservation after roundtrip, which protects against breaking changes.
src/hiero_sdk_python/utils/entity_id_helper.py (1)
149-158: LGTM!The
to_solidity_addressfunction correctly:
- Validates shard fits in signed 32-bit range
- Packs shard (4 bytes), realm (8 bytes), and num (8 bytes) into 20 bytes using big-endian format
- Returns the hex representation as expected for long-zero EVM addresses
tests/unit/entity_id_helper_test.py (2)
126-143: Good test coverage forto_solidity_address.The tests cover:
- Valid input producing correct hex output
- Zero values edge case
- Out-of-range shard validation with proper
matchparameterTests are deterministic and provide clear failure diagnostics.
145-209: Comprehensive error handling tests forperform_query_to_mirror_node.Excellent coverage of:
- Success path with mocked response
RequestException→RuntimeErrormappingHTTPErrorhandlingConnectionErrorhandlingTimeouthandling- Invalid URL validation (None and empty string)
All tests use
pytest.raiseswithmatchparameters per coding guidelines.examples/account/account_id_populate_from_mirror.py (1)
1-28: Good example structure and documentation.The docstring clearly explains:
- How to run the example
- What steps are demonstrated
Imports are valid and follow SDK conventions. The example provides a clear end-to-end demonstration of the new AccountId population functionality.
src/hiero_sdk_python/account/account_id.py (4)
195-202: Alias deserialization logic is correct.The distinction between EVM addresses (exactly 20 bytes) and public key aliases (with 2-byte prefix) correctly handles the protobuf encoding differences:
- EVM addresses are stored as raw 20 bytes
- Public keys are stored as serialized Key proto messages with field tag + length prefix
This aligns with Java/JavaScript SDK behavior.
315-334:populate_evm_addressvalidation is correct.The check
if self.num is None or self.num == 0properly prevents mirror node queries when the numeric account ID is unknown. This addresses the concern from previous reviews wherenum=0(default for EVM-created AccountIds) would have bypassed the check.
130-164:from_evm_addressimplementation is well-designed.The method:
- Properly validates inputs (None check, type validation)
- Handles both string and EvmAddress inputs
- Documents that
num=0because the numeric ID is unknown at creation time- Uses exception chaining for the string conversion error
Good defensive implementation.
363-383: Equality and hash correctly includeevm_address.Both
__eq__and__hash__now includeevm_addressalongside the other identity fields (shard, realm, num, alias_key). This maintains the invariant that equal objects have equal hash values.
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
|
@manishdait This is looking great! The PR currently has merge conflicts with main. Please let us know if you need assistance :) Thank you for the great work! |
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
…irror node Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
manishdait
force-pushed
the
feat/account-id-imp
branch
from
a52f9fb to
ed5e762
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/hiero_sdk_python/account/account_id.py (1)
309-313: Fix exception chaining.Line 313 should preserve the exception chain for debugging.
🔎 Proposed fix
try: self.num = int(account_id.split(".")[-1]) return self - except (ValueError, AttributeError): - raise ValueError(f"Invalid account format received: {account_id}") + except (ValueError, AttributeError) as e: + raise ValueError(f"Invalid account format received: {account_id}") from esrc/hiero_sdk_python/utils/entity_id_helper.py (1)
146-147: Fix exception chaining in generic error handler.The final
exceptclause captures the exception but doesn't chain it. This loses valuable debugging context.🔎 Proposed fix
except requests.RequestException as e: - raise RuntimeError(f"Unexpected error while querying mirror node: {url}") + raise RuntimeError(f"Unexpected error while querying mirror node: {url}") from eexamples/account/account_id_populate_from_mirror.py (2)
80-85: Missing freeze_with(client) before execute.Per SDK transaction lifecycle (Priority 1 in coding guidelines), transactions should explicitly call
freeze_with(client)beforeexecute(). Whileexecute()may handle this internally, the documented pattern includes explicit freezing.This was flagged in a previous review and remains unaddressed.
🔎 Proposed fix
transfer_tx = ( TransferTransaction() .add_hbar_transfer(evm_account_id, Hbar(1).to_tinybars()) .add_hbar_transfer(client.operator_account_id, Hbar(-1).to_tinybars()) + .freeze_with(client) .execute(client) )
87-96: Missing receipt status validation before accessing children.Per coding guidelines (Priority 1), examples should validate
receipt.statusagainstResponseCodeenums before inspecting child receipts. This ensures the transaction succeeded and provides clear error messages if it failed.This was flagged in a previous review and remains unaddressed.
🔎 Proposed fix
+from hiero_sdk_python import ResponseCode + ... receipt = ( TransactionGetReceiptQuery() .set_transaction_id(transfer_tx.transaction_id) .set_include_children(True) .execute(client) ) except Exception as e: print(f"Failed during auto account creation tx: {e}") sys.exit(1) + if receipt.status != ResponseCode.SUCCESS: + print(f"Transfer failed with status: {ResponseCode(receipt.status).name}") + sys.exit(1) + if not receipt.children:
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.mdexamples/account/account_id_populate_from_mirror.pysrc/hiero_sdk_python/account/account_id.pysrc/hiero_sdk_python/client/network.pysrc/hiero_sdk_python/utils/entity_id_helper.pytests/integration/account_id_population_e2e_test.pytests/unit/account_id_test.pytests/unit/entity_id_helper_test.py
🧰 Additional context used
📓 Path-based instructions (3)
tests/integration/**/*
⚙️ CodeRabbit configuration file
tests/integration/**/*: You are acting as a senior maintainer reviewing integration tests for the hiero-sdk-python project. Your goal is to ensure end-to-end tests validate real network behavior safely and deterministically.CRITICAL PRINCIPLES - Safety & Diagnosability:
- Prioritize safety: No implicit or default mainnet usage.
- Secrets and credentials must be injected safely (env vars, not hardcoded).
- Test failures must be diagnosable with clear error messages.
- Tests must assert observable network behavior, not just
SUCCESS.- Failure-path tests must assert specific
ResponseCodevalues (e.g.,TOKEN_HAS_NO_PAUSE_KEY).PRIORITY 1 - End-to-End Behavior:
- Tests should be end-to-end: construct → freeze → sign → execute → verify.
- Validate resulting balances, ownership, and state changes (not just transaction success).
- Assert transaction receipts contain expected data (IDs, serial numbers, etc.).
- Verify network state after operations (e.g., account balance changed, token transferred).
PRIORITY 2 - Test Structure & Maintainability:
- One major behavior per test (clear focus).
- Tests should be readable: clear names, brief docstrings, key inline comments.
- Minimal abstraction layers - prefer clarity over DRY.
- Is the file too monolithic? Flag if tests should be split into smaller modules.
- Are helper functions good candidates for pytest fixtures or shared utilities?
PRIORITY 3 - Isolation & Cleanup:
- Local account creation over operator reuse (avoid state pollution).
- Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?
- Recommend teardown strategies or fixture scoping improvements.
- Tests should not depend on execution order (avoid brittle assumptions).
PRIORITY 4 - Assertions & Coverage:
- Do tests validate only success/failure, or also assert resulting state?
- Suggest additional assertions that would strengthen correctness (balances, allowances, ownership).
- Cover happy paths AND ...
Files:
tests/integration/account_id_population_e2e_test.py
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/entity_id_helper_test.pytests/unit/account_id_test.py
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: You are acting as a senior maintainer reviewing SDK examples. Your goal is to ensure examples work verbatim for users who copy-paste them.Priority 1 - Correctness:
- Verify transaction lifecycle chain (construction -> freeze_with -> sign -> execute).
- Ensure
freeze_with(client)is called BEFORE signing.- Validate that methods referenced actually exist in the
hiero_sdk_pythoncodebase.- Ensure response validation checks
receipt.statusagainstResponseCodeenums (e.g.,ResponseCode.SUCCESS).Priority 2 - Transaction Lifecycle:
- Check method chaining logic.
- Verify correct signing order (especially for multi-sig).
- Ensure explicit
.execute(client)calls.- Verify response property extraction (e.g., using
.token_id,.account_id,.serial_numbers).- Ensure error handling uses
ResponseCode(receipt.status).namefor clarity.Priority 3 - Naming & Clarity:
- Enforce role-based naming:
operator_id/_key,treasury_account_id/_key,receiver_id/_key.- Use
_idsuffix for AccountId and_keysuffix for PrivateKey variables.- Validate negative examples explicitly check for failure codes (e.g.,
TOKEN_HAS_NO_PAUSE_KEY).- Ensure logical top-to-bottom flow without ambiguity.
Priority 4 - Consistency:
- Verify standard patterns:
def main(),if __name__ == "__main__":,load_dotenv().- IMPORT RULES:
- Accept both top-level imports (e.g.,
from hiero_sdk_python import PrivateKey) and fully qualified imports (e.g.,from hiero_sdk_python.crypto.private_key import PrivateKey).- STRICTLY validate that the import path actually exists in the project structure. Compare against other files in
/examplesor your knowledge of the SDK file tree.- Flag hallucinations immediately (e.g.,
hiero_sdk_python.keysdoes not exist).- Check for
try-exceptblocks withsys.exit(1)for critical failures.Priority 5 - User Experience:
- Ensure comments explain SDK usage patterns (for users,...
Files:
examples/account/account_id_populate_from_mirror.py
🧬 Code graph analysis (3)
tests/integration/account_id_population_e2e_test.py (5)
src/hiero_sdk_python/account/account_id.py (5)
AccountId(24-383)to_evm_address(336-341)from_evm_address(131-164)populate_account_num(286-313)populate_evm_address(315-334)src/hiero_sdk_python/crypto/private_key.py (3)
PrivateKey(14-471)generate_ecdsa(125-130)public_key(305-309)src/hiero_sdk_python/query/transaction_get_receipt_query.py (1)
TransactionGetReceiptQuery(15-330)src/hiero_sdk_python/transaction/transfer_transaction.py (2)
TransferTransaction(24-238)add_hbar_transfer(98-110)src/hiero_sdk_python/crypto/public_key.py (1)
to_evm_address(474-492)
tests/unit/entity_id_helper_test.py (1)
src/hiero_sdk_python/utils/entity_id_helper.py (2)
perform_query_to_mirror_node(129-147)to_solidity_address(149-158)
examples/account/account_id_populate_from_mirror.py (1)
src/hiero_sdk_python/account/account_id.py (6)
AccountId(24-383)from_string(63-128)to_evm_address(336-341)from_evm_address(131-164)populate_account_num(286-313)populate_evm_address(315-334)
🪛 Ruff (0.14.10)
tests/integration/account_id_population_e2e_test.py
24-24: Redefinition of unused env from line 13
(F811)
65-65: Redefinition of unused env from line 13
(F811)
src/hiero_sdk_python/utils/entity_id_helper.py
5-5: typing.Dict is deprecated, use dict instead
(UP035)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
141-141: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
147-147: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
src/hiero_sdk_python/account/account_id.py
82-82: Avoid specifying long messages outside the exception class
(TRY003)
121-128: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Use X | Y for type annotations
Convert to X | Y
(UP007)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
154-156: Prefer TypeError exception for invalid type
(TRY004)
154-156: Avoid specifying long messages outside the exception class
(TRY003)
234-234: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Avoid specifying long messages outside the exception class
(TRY003)
292-292: Avoid specifying long messages outside the exception class
(TRY003)
301-301: Avoid specifying long messages outside the exception class
(TRY003)
304-307: Avoid specifying long messages outside the exception class
(TRY003)
311-311: Consider moving this statement to an else block
(TRY300)
313-313: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
313-313: Avoid specifying long messages outside the exception class
(TRY003)
318-318: Avoid specifying long messages outside the exception class
(TRY003)
326-326: Avoid specifying long messages outside the exception class
(TRY003)
329-331: Avoid specifying long messages outside the exception class
(TRY003)
examples/account/account_id_populate_from_mirror.py
54-54: Do not catch blind exception: Exception
(BLE001)
93-93: Do not catch blind exception: Exception
(BLE001)
119-119: Do not catch blind exception: Exception
(BLE001)
149-149: Do not catch blind exception: Exception
(BLE001)
tests/unit/account_id_test.py
152-152: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
212-212: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
650-650: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
695-695: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
751-751: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
⏰ 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). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.11)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (24)
src/hiero_sdk_python/client/network.py (1)
31-31: LGTM! Local mirror URL updated.The local development mirror node URL has been updated to align with the actual deployment port. This change is consistent with the integration tests that rely on mirror node queries.
src/hiero_sdk_python/account/account_id.py (10)
6-17: LGTM! Imports support new EVM address functionality.The new imports are properly used throughout the file to implement EVM address alias support.
38-60: LGTM! Constructor properly extended for EVM address support.The
evm_addressparameter is cleanly added with appropriate documentation and default value.
62-128: LGTM! Comprehensive parsing logic with clear error messages.The updated
from_stringmethod properly handles:
- Standard
shard.realm.numformat- EVM addresses (detected and routed to
from_evm_address)- Hex aliases (20-byte EVM vs public key aliases)
The error messages are comprehensive and the comments clarify the EVM address default behavior. Good job addressing past review feedback.
130-164: LGTM! Well-designed factory method with proper validation.The
from_evm_addressmethod includes:
- Input validation with clear error messages
- Type conversion handling
- Proper exception chaining
- Clear comments explaining the
num=0default
166-177: LGTM! Clean deserialization method.Properly delegates to
_from_protofor protobuf deserialization.
195-202: LGTM! Correct alias deserialization logic.The method properly distinguishes between:
- 20-byte EVM addresses (used as-is)
- Public key aliases (2-byte protobuf prefix stripped)
This aligns with the protobuf serialization format where public keys include field tags but EVM addresses are raw bytes.
221-222: LGTM! Protobuf serialization correctly handles EVM address.The serialization properly sets the alias field from
evm_address.address_byteswhen present, maintaining symmetry with_from_proto.
243-258: LGTM! Robust EVM address detection.The
_is_evm_addressmethod properly validates:
- Optional
0xprefix- Exact 40-character length (20 bytes)
- Valid hexadecimal format
Good response to past review feedback about making the detection more robust.
315-334: LGTM! Mirror node population with proper validation.The method correctly validates that
numis set and non-zero before querying the mirror node. Good fix from past review feedback.
336-345: LGTM! Clean accessor and serialization methods.Both methods are well-implemented:
to_evm_address: Returns stored EVM address or derives from account numberto_bytes: Delegates to protobuf serializationCHANGELOG.md (1)
77-77: LGTM! Clear changelog entry.The entry concisely describes the EVM address alias support added to AccountId. Good response to past review feedback requesting more concrete description.
src/hiero_sdk_python/utils/entity_id_helper.py (1)
149-158: LGTM! Correct Solidity address conversion.The function properly packs entity ID components into a 20-byte Solidity-style address:
- 4 bytes (shard)
- 8 bytes (realm)
- 8 bytes (num)
Validates shard fits in 32-bit range, which is the most likely overflow. Good response to past review feedback on the docstring.
tests/integration/account_id_population_e2e_test.py (1)
15-21: LGTM! Clean fixture for generating EVM addresses.The fixture properly generates an ECDSA private key and derives the EVM address.
tests/unit/entity_id_helper_test.py (2)
126-143: LGTM - Comprehensive coverage of to_solidity_address.The tests cover the happy path, edge cases (zero values), and boundary conditions (out-of-range shard). The use of
matchparameter inpytest.raisesensures clear failure messages as per coding guidelines.
145-209: LGTM - Excellent coverage of perform_query_to_mirror_node.Tests comprehensively cover:
- Success path with proper mocking
- All exception types (HTTPError, ConnectionError, Timeout, RequestException)
- Input validation (None and empty string)
The tests are deterministic, use proper mocking to avoid network calls, and include descriptive match parameters for clear failure diagnostics.
tests/unit/account_id_test.py (6)
33-95: LGTM - EVM address initialization tests are comprehensive.The new
evm_addressfixture and initialization tests properly:
- Assert that
evm_addressattribute exists (protecting against breaking changes)- Verify correct initialization with and without
evm_address- Maintain backward compatibility by asserting
evm_address is Nonein existing scenarios
151-275: LGTM - Comprehensive from_string and parsing coverage.The tests thoroughly cover:
- Valid EVM address formats (raw hex, 0x-prefixed, shard.realm.hex)
- Alias types including EvmAddress
_is_evm_addressvalidation edge cases- Invalid formats and types with clear error messages
Per coding guidelines, the tests protect against breaking changes by validating return types and error messages.
323-414: LGTM - Proto conversion tests properly validate EVM address handling.The tests ensure:
to_protocorrectly serializesevm_addressinto thealiasfieldfrom_protocorrectly deserializes 20-byte alias asevm_address- Existing tests updated to assert
evm_address is Nonefor backward compatibility
494-622: LGTM - Equality, hashing, and string representation tests are thorough.Tests properly verify:
- Equality comparison with
evm_address(same, different, None)- Hash consistency with
evm_address- String representation includes
evm_address.to_string()when present
653-790: Comprehensive coverage of mirror node population methods.The tests extensively cover:
- Success paths with proper mocking
- Missing field validation (evm_address, account, num)
- Invalid response format handling
- Mirror node failure wrapping with context
Per coding guidelines, tests must fail loudly with useful error messages - these tests achieve that goal.
792-896: LGTM - Byte serialization and from_evm_address tests are comprehensive.Tests properly verify:
- Roundtrip byte serialization preserves all fields (num, alias_key, evm_address)
from_evm_addresshandles both raw hex and 0x-prefixed strings- Error handling for None, invalid types, and invalid strings
to_evm_addressreturns stored evm_address when presentPer coding guidelines, these tests protect against breaking changes by asserting attribute existence and type validation.
examples/account/account_id_populate_from_mirror.py (2)
36-103: Example successfully demonstrates mirror node population, but setup and lifecycle could be improved.The example correctly demonstrates the new
populate_account_numandpopulate_evm_addressmethods. The overall flow is clear and educational for users.
106-173: LGTM - populate methods are demonstrated correctly.The
populate_account_num_exampleandpopulate_evm_address_examplefunctions clearly demonstrate:
- Creating AccountId from EVM address
- Calling populate methods with proper error handling
- Validating results against expected values
- Using appropriate delays for mirror node propagation
The pattern is clear and educational for SDK users.
| def __hash__(self) -> int: | ||
| """Returns a hash value for the AccountId instance.""" | ||
| return hash((self.shard, self.realm, self.num, self.alias_key)) | ||
| return hash((self.shard, self.realm, self.num, self.alias_key, self.evm_address)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the __hash__ implementation and surrounding code
cat -n src/hiero_sdk_python/account/account_id.py | sed -n '370,390p'Repository: hiero-ledger/hiero-sdk-python
Length of output: 676
🏁 Script executed:
# Find the populate methods mentioned in the review
rg "def populate_(account_num|evm_address)" src/hiero_sdk_python/account/account_id.py -A 5Repository: hiero-ledger/hiero-sdk-python
Length of output: 667
🏁 Script executed:
# Search for AccountId usage in sets or dicts throughout the codebase
rg -t py "AccountId.*set\(|{.*AccountId|set.*AccountId" --max-count=20Repository: hiero-ledger/hiero-sdk-python
Length of output: 209
🏁 Script executed:
# Check the __eq__ implementation to understand the equality contract
rg "def __eq__" src/hiero_sdk_python/account/account_id.py -A 3Repository: hiero-ledger/hiero-sdk-python
Length of output: 200
🏁 Script executed:
# Get the full populate_account_num method
rg "def populate_account_num" src/hiero_sdk_python/account/account_id.py -A 20Repository: hiero-ledger/hiero-sdk-python
Length of output: 944
🏁 Script executed:
# Get the full populate_evm_address method
rg "def populate_evm_address" src/hiero_sdk_python/account/account_id.py -A 20Repository: hiero-ledger/hiero-sdk-python
Length of output: 914
🏁 Script executed:
# Search for AccountId usage in sets (corrected regex)
rg "set\(\[" --max-count=10 -t py | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 55
🏁 Script executed:
# Search for AccountId usage as dict keys or in collections
rg "AccountId" --max-count=30 -t py | grep -E "(set|dict|{|}|\[|\])" | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 2043
🏁 Script executed:
# Check if there are any tests that use AccountId in sets/dicts
find . -name "*test*.py" -type f -exec grep -l "AccountId" {} \; | head -5Repository: hiero-ledger/hiero-sdk-python
Length of output: 2511
🏁 Script executed:
# Get the complete populate_account_num method with more context
rg "def populate_account_num" src/hiero_sdk_python/account/account_id.py -A 30Repository: hiero-ledger/hiero-sdk-python
Length of output: 1325
🏁 Script executed:
# Check the transaction_record.py to see how AccountId is used as dict key
rg "\.transfers\[" tests/unit/transaction_record_test.py -B 2 -A 2Repository: hiero-ledger/hiero-sdk-python
Length of output: 992
🏁 Script executed:
# Check if there's any documentation or comments warning about hash/populate interaction
rg "populate|mutable|hash" src/hiero_sdk_python/account/account_id.py -i | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 854
Address hash mutability risk with populate methods.
The __hash__ implementation includes num and evm_address, but both are mutated by populate_account_num() and populate_evm_address() respectively. Since AccountId is used as a dict key in real code (e.g., TransactionRecord.transfers), this creates a data corruption risk: if an AccountId is added to a dict, then a populate method is called, the hash changes and subsequent lookups will fail.
Consider:
- Documenting that AccountId should not be used in collections before population
- Making populate methods return new instances instead of mutating self
- Only hashing immutable fields (shard, realm) to accept hash collisions
| """ | ||
| Integration tests for AccountId. | ||
| """ | ||
|
|
||
| import time | ||
| import pytest | ||
|
|
||
| from hiero_sdk_python.account.account_id import AccountId | ||
| from hiero_sdk_python.crypto.private_key import PrivateKey | ||
| from hiero_sdk_python.hbar import Hbar | ||
| from hiero_sdk_python.query.transaction_get_receipt_query import TransactionGetReceiptQuery | ||
| from hiero_sdk_python.transaction.transfer_transaction import TransferTransaction | ||
| from tests.integration.utils import env | ||
|
|
||
| @pytest.fixture | ||
| def evm_address(): | ||
| """Returns an emv_aaddress.""" | ||
| private_key = PrivateKey.generate_ecdsa() | ||
| public_key = private_key.public_key() | ||
|
|
||
| return public_key.to_evm_address() | ||
|
|
||
| @pytest.mark.integration | ||
| def test_populate_account_id_num(env, evm_address): | ||
| """Test populate AccountId num from mirror node.""" | ||
| evm_address_account = AccountId.from_evm_address(evm_address, 0, 0) | ||
|
|
||
| # Auto account creation by doing transfer to an evm_address | ||
| transfer_tx = ( | ||
| TransferTransaction() | ||
| .add_hbar_transfer(evm_address_account, Hbar(1).to_tinybars()) | ||
| .add_hbar_transfer(env.operator_id, Hbar(-1).to_tinybars()) | ||
| ) | ||
| transfer_tx.execute(env.client) | ||
|
|
||
| transfer_receipt = ( | ||
| TransactionGetReceiptQuery() | ||
| .set_transaction_id(transfer_tx.transaction_id) | ||
| .set_include_children(True) | ||
| .execute(env.client) | ||
| ) | ||
|
|
||
| assert transfer_receipt is not None | ||
| assert len(transfer_receipt.children) > 0 | ||
|
|
||
| account_id = transfer_receipt.children[0].account_id | ||
|
|
||
| assert account_id is not None | ||
|
|
||
| # Wait for mirrornode to update | ||
| time.sleep(5) | ||
|
|
||
| mirror_account_id = AccountId.from_evm_address(evm_address, 0, 0) | ||
|
|
||
| assert mirror_account_id.num == 0 | ||
|
|
||
| mirror_account_id.populate_account_num(env.client) | ||
|
|
||
| assert mirror_account_id.shard == account_id.shard | ||
| assert mirror_account_id.realm == account_id.realm | ||
| assert mirror_account_id.num == account_id.num | ||
|
|
||
|
|
||
| @pytest.mark.integration | ||
| def test_populate_account_id_evm_address(env, evm_address): | ||
| """Test populate AccountId evm address from mirror node.""" | ||
| evm_address_account = AccountId.from_evm_address(evm_address, 0, 0) | ||
|
|
||
| # Auto account creation by doing transfer to an evm_address | ||
| transfer_tx = ( | ||
| TransferTransaction() | ||
| .add_hbar_transfer(evm_address_account, Hbar(1).to_tinybars()) | ||
| .add_hbar_transfer(env.operator_id, Hbar(-1).to_tinybars()) | ||
| ) | ||
| transfer_tx.execute(env.client) | ||
|
|
||
| transfer_receipt = ( | ||
| TransactionGetReceiptQuery() | ||
| .set_transaction_id(transfer_tx.transaction_id) | ||
| .set_include_children(True) | ||
| .execute(env.client) | ||
| ) | ||
|
|
||
| assert transfer_receipt is not None | ||
| assert len(transfer_receipt.children) > 0 | ||
|
|
||
| account_id = transfer_receipt.children[0].account_id | ||
|
|
||
| # Wait for mirrornode to update | ||
| time.sleep(5) | ||
|
|
||
| assert account_id.evm_address is None | ||
| account_id.populate_evm_address(env.client) | ||
|
|
||
| assert account_id.evm_address is not None | ||
| assert account_id.evm_address == evm_address |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding cleanup and resource management.
The tests create accounts on the network but don't clean them up. While Hedera doesn't require explicit cleanup for accounts, consider:
- Resource tracking: Log created account IDs for manual cleanup if needed
- Fixture scope: Could the
evm_addressfixture be session-scoped to reduce test execution time? - Idempotency: Tests don't verify they can run multiple times without interference
💡 Optional improvement
import logging
logger = logging.getLogger(__name__)
@pytest.fixture(scope="session")
def evm_address():
"""Returns an EVM address for the test session."""
private_key = PrivateKey.generate_ecdsa()
public_key = private_key.public_key()
address = public_key.to_evm_address()
logger.info(f"Generated EVM address for tests: {address.to_string()}")
return addressBased on learnings, integration tests should consider cleanup strategies and fixture scoping for maintainability.
🧰 Tools
🪛 Ruff (0.14.10)
24-24: Redefinition of unused env from line 13
(F811)
65-65: Redefinition of unused env from line 13
(F811)
| @pytest.mark.integration | ||
| def test_populate_account_id_num(env, evm_address): | ||
| """Test populate AccountId num from mirror node.""" | ||
| evm_address_account = AccountId.from_evm_address(evm_address, 0, 0) | ||
|
|
||
| # Auto account creation by doing transfer to an evm_address | ||
| transfer_tx = ( | ||
| TransferTransaction() | ||
| .add_hbar_transfer(evm_address_account, Hbar(1).to_tinybars()) | ||
| .add_hbar_transfer(env.operator_id, Hbar(-1).to_tinybars()) | ||
| ) | ||
| transfer_tx.execute(env.client) | ||
|
|
||
| transfer_receipt = ( | ||
| TransactionGetReceiptQuery() | ||
| .set_transaction_id(transfer_tx.transaction_id) | ||
| .set_include_children(True) | ||
| .execute(env.client) | ||
| ) | ||
|
|
||
| assert transfer_receipt is not None | ||
| assert len(transfer_receipt.children) > 0 | ||
|
|
||
| account_id = transfer_receipt.children[0].account_id | ||
|
|
||
| assert account_id is not None | ||
|
|
||
| # Wait for mirrornode to update | ||
| time.sleep(5) | ||
|
|
||
| mirror_account_id = AccountId.from_evm_address(evm_address, 0, 0) | ||
|
|
||
| assert mirror_account_id.num == 0 | ||
|
|
||
| mirror_account_id.populate_account_num(env.client) | ||
|
|
||
| assert mirror_account_id.shard == account_id.shard | ||
| assert mirror_account_id.realm == account_id.realm | ||
| assert mirror_account_id.num == account_id.num | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the actual test file
find . -type f -name "account_id_population_e2e_test.py" | head -5Repository: hiero-ledger/hiero-sdk-python
Length of output: 126
🏁 Script executed:
# Also check for other integration tests to understand patterns
find tests/integration -type f -name "*test.py" | head -10Repository: hiero-ledger/hiero-sdk-python
Length of output: 596
🏁 Script executed:
# Search for populate_account_num usage to understand error handling
rg "populate_account_num" -A 5 -B 5Repository: hiero-ledger/hiero-sdk-python
Length of output: 12882
🏁 Script executed:
# Search for retry patterns in integration tests
rg "retry|retries|max_retries" tests/integration/ -B 2 -A 5Repository: hiero-ledger/hiero-sdk-python
Length of output: 55
🏁 Script executed:
# Search for mirror node query patterns and error handling
rg "populate_" tests/integration/ -A 3 -B 2Repository: hiero-ledger/hiero-sdk-python
Length of output: 2155
🏁 Script executed:
cat -n tests/integration/account_id_population_e2e_test.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 3741
🏁 Script executed:
# Find the populate_account_num implementation
rg "def populate_account_num" -A 20Repository: hiero-ledger/hiero-sdk-python
Length of output: 3662
🏁 Script executed:
# Check for existing retry utilities or patterns in tests
rg "import time|sleep|retry|exponential" tests/integration/ | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 1080
🏁 Script executed:
# Let me check the full populate_account_num and populate_evm_address implementations
rg "def populate_account_num|def populate_evm_address" -A 30 src/Repository: hiero-ledger/hiero-sdk-python
Length of output: 5026
🏁 Script executed:
# Check for perform_query_to_mirror_node to understand error handling
rg "def perform_query_to_mirror_node" -A 15Repository: hiero-ledger/hiero-sdk-python
Length of output: 1522
🏁 Script executed:
# Check if there are any retry utilities or helper functions in the integration test utils
cat -n tests/integration/utils.py 2>/dev/null | head -100Repository: hiero-ledger/hiero-sdk-python
Length of output: 4716
Add retry logic and improve observability for mirror node propagation delays.
The test relies on a fixed 5-second time.sleep() which is fragile—mirror node propagation isn't guaranteed within that window, making the test flaky. The test also lacks logging to diagnose failures.
Concerns:
time.sleep(5)without retry logic can cause intermittent test failures when mirror node is slower than expected.- No logging of transaction IDs, account IDs, or intermediate state makes failures difficult to diagnose.
- When
populate_account_num()fails, there's no context in the test to understand why.
Implementation note: If adding retry logic, ensure it handles both RuntimeError (network failures) and ValueError (account not yet in mirror node), as both can occur during mirror node propagation delays.
🧰 Tools
🪛 Ruff (0.14.10)
24-24: Redefinition of unused env from line 13
(F811)
| @pytest.mark.integration | ||
| def test_populate_account_id_evm_address(env, evm_address): | ||
| """Test populate AccountId evm address from mirror node.""" | ||
| evm_address_account = AccountId.from_evm_address(evm_address, 0, 0) | ||
|
|
||
| # Auto account creation by doing transfer to an evm_address | ||
| transfer_tx = ( | ||
| TransferTransaction() | ||
| .add_hbar_transfer(evm_address_account, Hbar(1).to_tinybars()) | ||
| .add_hbar_transfer(env.operator_id, Hbar(-1).to_tinybars()) | ||
| ) | ||
| transfer_tx.execute(env.client) | ||
|
|
||
| transfer_receipt = ( | ||
| TransactionGetReceiptQuery() | ||
| .set_transaction_id(transfer_tx.transaction_id) | ||
| .set_include_children(True) | ||
| .execute(env.client) | ||
| ) | ||
|
|
||
| assert transfer_receipt is not None | ||
| assert len(transfer_receipt.children) > 0 | ||
|
|
||
| account_id = transfer_receipt.children[0].account_id | ||
|
|
||
| # Wait for mirrornode to update | ||
| time.sleep(5) | ||
|
|
||
| assert account_id.evm_address is None | ||
| account_id.populate_evm_address(env.client) | ||
|
|
||
| assert account_id.evm_address is not None | ||
| assert account_id.evm_address == evm_address |
There was a problem hiding this comment.
Add retry logic and unhappy path coverage.
Similar reliability concerns as the previous test, plus missing negative test cases:
-
Same timing issue: Fixed 5-second sleep on line 90 is fragile.
-
Missing unhappy paths: What happens if:
- Mirror node returns invalid EVM address format?
- Account doesn't exist in mirror node?
- Network connection fails?
-
Incomplete validation: Consider asserting the EVM address format (0x-prefixed, 40 hex chars) not just equality.
🔎 Additional test cases to consider
@pytest.mark.integration
def test_populate_evm_address_account_not_found(env):
"""Test populate_evm_address fails gracefully when account doesn't exist."""
non_existent_account = AccountId(0, 0, 999999999)
with pytest.raises(RuntimeError, match="Failed to populate evm_address"):
non_existent_account.populate_evm_address(env.client)
@pytest.mark.integration
def test_populate_account_num_invalid_evm_address(env):
"""Test populate_account_num fails when EVM address doesn't exist."""
fake_evm = EvmAddress.from_string("0x" + "0" * 40)
account = AccountId.from_evm_address(fake_evm, 0, 0)
with pytest.raises(RuntimeError, match="Failed to populate account number"):
account.populate_account_num(env.client)As per coding guidelines, integration tests should cover both happy and unhappy paths with proper error assertions.
🧰 Tools
🪛 Ruff (0.14.10)
65-65: Redefinition of unused env from line 13
(F811)
| with pytest.raises(ValueError, match="Checksum mismatch for 0.0.100"): | ||
| account_id.validate_checksum(client) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Regex metacharacters in match patterns should be escaped.
Per past review feedback (still unaddressed), the match parameter contains unescaped metacharacters (. in "0.0.100"). While it works in this context, using raw strings with proper escaping is more robust and explicit.
🔎 Proposed fix
- with pytest.raises(ValueError, match="Checksum mismatch for 0.0.100"):
+ with pytest.raises(ValueError, match=r"Checksum mismatch for 0\.0\.100"):
account_id.validate_checksum(client)Apply similar fixes at lines 695 and 751:
# Line 695
match=r"Invalid account format received: invalid\.account\.format"
# Line 751
match=r"Mirror node response missing 'evm_address': \{\}"📝 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.
| with pytest.raises(ValueError, match="Checksum mismatch for 0.0.100"): | |
| account_id.validate_checksum(client) | |
| with pytest.raises(ValueError, match=r"Checksum mismatch for 0\.0\.100"): | |
| account_id.validate_checksum(client) |
🧰 Tools
🪛 Ruff (0.14.10)
650-650: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
4 participants
Description:
This PR enhances the AccountId implementation to properly handle
aliasfield (bytes alias = 4).The alias is now correctly decoded and classified as either:
Additionally, the PR introduces several helper methods improvements for working with Account IDs, alias values, and mirror node queries.
Changes Made:
evm_addressfield to AccountIdfrom_string()method to support:shard.realm.num)shard.realm.alias-key)shard.realm.evm-address)00.... or 0x00...)from_evm_address()method to create AccountId fromEvmAddressevm_addressandaccount_numfield by querying mirror node:populate_evm_address()populate_account_num()to_bytes()andfrom_bytes()for protobuf byte conversionEVM addressget_mirror_node_rest_url()to network.pyRelated issue(s):
Fixes #1008
Notes for reviewer:
Checklist