feat: Add support for populating ContractId num value using MirrorNode query
#1565
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1565 +/- ##
==========================================
+ Coverage 92.89% 92.93% +0.04%
==========================================
Files 140 140
Lines 8765 8805 +40
==========================================
+ Hits 8142 8183 +41
+ Misses 623 622 -1 🚀 New features to boost your workflow:
|
WalkthroughAdds Mirror Node population and protobuf (de)serialization for ContractId via EVM-address, and changes AccountId population methods to return new instances instead of mutating in place. Tests, examples, and a wait-for-mirror-node utility were added/updated to reflect the new behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ContractId
participant MirrorNode
Client->>ContractId: from_evm_address(shard, realm, evm_address)
Note over ContractId: construct ContractId (evm_address set, numeric contract unset)
Client->>ContractId: populate_contract_num(client)
ContractId->>MirrorNode: HTTP query by EVM address (perform_query_to_mirror_node)
MirrorNode-->>ContractId: response with contract id (or error)
Note over ContractId: create new ContractId with populated contract number
ContractId-->>Client: return new ContractId instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/account_id_population_e2e_test.py (1)
27-65: Test validates immutability and population correctness well.The test properly validates:
- Original
mirror_account_id.numremains0(immutability check on line 60)- Returned
new_account_idhas populated values matching the receipt'saccount_id- EVM address is preserved in the returned instance
One minor observation: the test could benefit from an explicit type assertion to protect against breaking changes.
Consider adding a type assertion for stronger contract protection
new_account_id = mirror_account_id.populate_account_num(env.client) assert mirror_account_id.num == 0 + assert isinstance(new_account_id, AccountId), "populate_account_num must return AccountId" assert new_account_id.evm_address == mirror_account_id.evm_address
| import requests | ||
|
|
||
| from typing import TYPE_CHECKING, Any, Dict, Optional | ||
| from typing import TYPE_CHECKING, Any, Dict |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use built-in dict instead of typing.Dict.
typing.Dict is deprecated since Python 3.9. Use the built-in dict for type hints.
Suggested fix
-from typing import TYPE_CHECKING, Any, Dict
+from typing import TYPE_CHECKING, AnyThen update the return type on line 139:
-def perform_query_to_mirror_node(url: str, timeout: float = 10) -> Dict[str, Any]:
+def perform_query_to_mirror_node(url: str, timeout: float = 10) -> dict[str, Any]:🧰 Tools
🪛 Ruff (0.14.13)
5-5: typing.Dict is deprecated, use dict instead
(UP035)
| @pytest.mark.integration | ||
| def test_populate_contract_id_num(env): | ||
| """Test populate ContractId num from mirror node.""" | ||
| # Create a contract transaction | ||
| file_receipt = ( | ||
| FileCreateTransaction() | ||
| .set_keys(env.operator_key.public_key()) | ||
| .set_contents(STATEFUL_CONTRACT_BYTECODE) | ||
| .set_file_memo("integration test contract") | ||
| .execute(env.client) | ||
| ) | ||
| assert file_receipt.status == ResponseCode.SUCCESS | ||
| file_id = file_receipt.file_id | ||
| assert file_id is not None | ||
|
|
||
| constructor_params = ContractFunctionParameters().add_bytes32( | ||
| b"Initial message from constructor" | ||
| ) | ||
| contract_receipt = ( | ||
| ContractCreateTransaction() | ||
| .set_admin_key(env.operator_key.public_key()) | ||
| .set_gas(CONTRACT_DEPLOY_GAS) | ||
| .set_constructor_parameters(constructor_params) | ||
| .set_bytecode_file_id(file_id) | ||
| .execute(env.client) | ||
| ) | ||
| assert contract_receipt.status == ResponseCode.SUCCESS | ||
| contract_id_with_num = contract_receipt.contract_id | ||
| assert contract_id_with_num is not None | ||
|
|
||
| # Query contract info to get evm_address | ||
| info = ContractInfoQuery().set_contract_id(contract_id_with_num).execute(env.client) | ||
| contract_id_with_evm_addr = ContractId.from_evm_address( | ||
| 0, 0, info.contract_account_id | ||
| ) | ||
|
|
||
| # Wait for mirrornode to update | ||
| time.sleep(5) | ||
|
|
||
| final_contract_id = contract_id_with_evm_addr.populate_contract_num(env.client) | ||
|
|
||
| assert final_contract_id.shard == contract_id_with_num.shard | ||
| assert final_contract_id.realm == contract_id_with_num.realm | ||
| assert final_contract_id.contract == contract_id_with_num.contract | ||
| assert final_contract_id.evm_address == contract_id_with_evm_addr.evm_address |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding type assertion for returned ContractId.
To protect against breaking changes and ensure the method contract is maintained, add a type assertion on the returned value.
Add type assertion
final_contract_id = contract_id_with_evm_addr.populate_contract_num(env.client)
+ assert isinstance(final_contract_id, ContractId), "populate_contract_num must return ContractId"
+
assert final_contract_id.shard == contract_id_with_num.shard🧰 Tools
🪛 Ruff (0.14.13)
26-26: Redefinition of unused env from line 22: env redefined here
(F811)
| new_account_id = account_id.populate_evm_address(mock_client) | ||
|
|
||
| assert account_id.evm_address == evm_address | ||
| assert account_id.evm_address == None | ||
| assert new_account_id.evm_address == evm_address |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use is None instead of == None for None comparison.
Per Python conventions (PEP 8), comparisons to None should use is operator rather than ==.
Fix None comparison
new_account_id = account_id.populate_evm_address(mock_client)
- assert account_id.evm_address == None
+ assert account_id.evm_address is None
assert new_account_id.evm_address == 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.
| new_account_id = account_id.populate_evm_address(mock_client) | |
| assert account_id.evm_address == evm_address | |
| assert account_id.evm_address == None | |
| assert new_account_id.evm_address == evm_address | |
| new_account_id = account_id.populate_evm_address(mock_client) | |
| assert account_id.evm_address is None | |
| assert new_account_id.evm_address == evm_address |
🧰 Tools
🪛 Ruff (0.14.13)
797-797: Comparison to None should be cond is None
Replace with cond is None
(E711)
| @pytest.mark.parametrize( | ||
| "contract_str, expected", | ||
| [ | ||
| ("1.2.101", (1, 2, 101, None, None)), | ||
| ("0.0.0", (0, 0, 0, None, None)), | ||
| ("1.2.3-abcde", (1, 2, 3, None, "abcde")), | ||
| ( | ||
| "1.2.abcdef0123456789abcdef0123456789abcdef01", | ||
| (1, 2, 0, bytes.fromhex("abcdef0123456789abcdef0123456789abcdef01"), None), | ||
| ), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using tuple for parametrize argument names.
Ruff suggests using a tuple instead of a string for the first argument of pytest.mark.parametrize for consistency.
Suggested fix
`@pytest.mark.parametrize`(
- "contract_str, expected",
+ ("contract_str", "expected"),
[
("1.2.101", (1, 2, 101, None, None)),
...
],
)📝 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.
| @pytest.mark.parametrize( | |
| "contract_str, expected", | |
| [ | |
| ("1.2.101", (1, 2, 101, None, None)), | |
| ("0.0.0", (0, 0, 0, None, None)), | |
| ("1.2.3-abcde", (1, 2, 3, None, "abcde")), | |
| ( | |
| "1.2.abcdef0123456789abcdef0123456789abcdef01", | |
| (1, 2, 0, bytes.fromhex("abcdef0123456789abcdef0123456789abcdef01"), None), | |
| ), | |
| ], | |
| ) | |
| `@pytest.mark.parametrize`( | |
| ("contract_str", "expected"), | |
| [ | |
| ("1.2.101", (1, 2, 101, None, None)), | |
| ("0.0.0", (0, 0, 0, None, None)), | |
| ("1.2.3-abcde", (1, 2, 3, None, "abcde")), | |
| ( | |
| "1.2.abcdef0123456789abcdef0123456789abcdef01", | |
| (1, 2, 0, bytes.fromhex("abcdef0123456789abcdef0123456789abcdef01"), None), | |
| ), | |
| ], | |
| ) |
🧰 Tools
🪛 Ruff (0.14.13)
62-62: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
| contract_id = ContractId.from_string("0.0.1-wronx") | ||
|
|
||
| with pytest.raises(ValueError, match="Checksum mismatch for 0.0.1"): | ||
| contract_id.validate_checksum(client) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Escape regex metacharacters in match pattern.
The pattern "Checksum mismatch for 0.0.1" contains dots (.) which are regex metacharacters matching any character. Use a raw string with escaped dots or re.escape() to ensure exact matching.
Proposed fix
- with pytest.raises(ValueError, match="Checksum mismatch for 0.0.1"):
+ with pytest.raises(ValueError, match=r"Checksum mismatch for 0\.0\.1"):
contract_id.validate_checksum(client)📝 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.
| contract_id = ContractId.from_string("0.0.1-wronx") | |
| with pytest.raises(ValueError, match="Checksum mismatch for 0.0.1"): | |
| contract_id.validate_checksum(client) | |
| contract_id = ContractId.from_string("0.0.1-wronx") | |
| with pytest.raises(ValueError, match=r"Checksum mismatch for 0\.0\.1"): | |
| contract_id.validate_checksum(client) |
🧰 Tools
🪛 Ruff (0.14.13)
341-341: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
| ): | ||
| with pytest.raises( | ||
| ValueError, | ||
| match="Invalid contract_id format received: invalid.account.format", | ||
| ): | ||
| contract_id.populate_contract_num(client) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Escape regex metacharacters in match pattern.
The pattern contains dots which are regex metacharacters. This could cause false positives if the error message format changes.
Proposed fix
with pytest.raises(
ValueError,
- match="Invalid contract_id format received: invalid.account.format",
+ match=r"Invalid contract_id format received: invalid\.account\.format",
):📝 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="Invalid contract_id format received: invalid.account.format", | |
| ): | |
| contract_id.populate_contract_num(client) | |
| ): | |
| with pytest.raises( | |
| ValueError, | |
| match=r"Invalid contract_id format received: invalid\.account\.format", | |
| ): | |
| contract_id.populate_contract_num(client) |
🧰 Tools
🪛 Ruff (0.14.13)
503-503: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
1 similar comment
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
|
Please take a look at the CodeRabbit review suggestions. |
manishdait
force-pushed
the
feat/contract-id-mirror-population
branch
from
a043739 to
500229a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/account/account_id_populate_from_mirror.py (1)
17-66: Check receipt.status against ResponseCode in the auto‑create flow.
Examples should validate receipt status explicitly before continuing, otherwise users may proceed with a failed transfer and get confusing downstream errors.✅ Proposed fix
from hiero_sdk_python import ( Client, AccountId, PrivateKey, TransferTransaction, Hbar, TransactionGetReceiptQuery, + ResponseCode, ) @@ if not receipt.children: print("Auto account creation failed: no child receipts found") sys.exit(1) + + if receipt.status != ResponseCode.SUCCESS: + print( + "Auto account creation failed: " + f"{ResponseCode(receipt.status).name}" + ) + sys.exit(1)src/hiero_sdk_python/contract/contract_id.py (1)
79-92: Potential inconsistency:_to_protosets bothcontractNumandevm_address.When
evm_addressis set, the protobuf should ideally havecontractNum=0rather than whatever value is inself.contract. The test at line 243 ofcontract_id_test.pyexpectsproto.contractNum == 0whenevm_addressis set, but the current implementation passesself.contractregardless.Proposed fix
def _to_proto(self): """ Converts the ContractId instance to a protobuf ContractID object. Returns: basic_types_pb2.ContractID: The corresponding protobuf ContractID object. """ + if self.evm_address is not None: + return basic_types_pb2.ContractID( + shardNum=self.shard, + realmNum=self.realm, + contractNum=0, + evm_address=self.evm_address, + ) return basic_types_pb2.ContractID( shardNum=self.shard, realmNum=self.realm, contractNum=self.contract, - evm_address=self.evm_address, )
♻️ Duplicate comments (4)
src/hiero_sdk_python/account/account_id.py (1)
326-333: Parse shard/realm from mirror response to avoid incorrect IDs.
If the mirror returns a non‑zero shard/realm, reusingself.shard/self.realmyields a wrongAccountId. Useparse_from_string()to preserve the full response.🐛 Proposed fix
- num = int(account_id.split(".")[-1]) - return AccountId( - shard=self.shard, - realm=self.realm, - num=num, - evm_address=self.evm_address, - ) + shard, realm, num, _ = parse_from_string(account_id) + return AccountId( + shard=int(shard), + realm=int(realm), + num=int(num), + evm_address=self.evm_address, + )tests/unit/contract_id_test.py (1)
504-517: Typo in test function name.The function name has a typo:
test_populate_contract_num_invlid_responseshould betest_populate_contract_num_invalid_response.Proposed fix
-def test_populate_contract_num_invlid_response(client): +def test_populate_contract_num_invalid_response(client): """Should raise error when populating contract number invalid response."""tests/unit/account_id_test.py (1)
795-798: Useis Noneinstead of== Nonefor None comparison.Per Python conventions (PEP 8), comparisons to
Noneshould use theisoperator rather than==.Proposed fix
new_account_id = account_id.populate_evm_address(mock_client) - assert account_id.evm_address == None + assert account_id.evm_address is None assert new_account_id.evm_address == evm_addresssrc/hiero_sdk_python/contract/contract_id.py (1)
247-256: Consider parsing full shard/realm/contract from mirror response.The current implementation only extracts the contract number from the mirror node response while preserving
self.shardandself.realm. If the mirror node returns a different shard/realm than expected (e.g., "1.2.123" when self has shard=0, realm=0), this could create an inconsistent ContractId.Consider using
parse_from_string(contract_id)to extract all three components from the authoritative mirror node response, as was suggested in a previous review.Suggested fix
try: - contract = int(contract_id.split(".")[-1]) + shard, realm, contract, _ = parse_from_string(contract_id) return ContractId( - shard=self.shard, - realm=self.realm, - contract=contract, + shard=int(shard), + realm=int(realm), + contract=int(contract), evm_address=self.evm_address, ) except (ValueError, AttributeError) as e: raise ValueError(f"Invalid contract_id format received: {contract_id}") from e
manishdait
force-pushed
the
feat/contract-id-mirror-population
branch
from
e6c6733 to
9ae865c
Compare
|
Hi, this is WorkflowBot.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/unit/contract_id_test.py (4)
343-348: Escape dots in regexmatchfor checksum mismatch.This avoids accidental matches when the message changes.
♻️ Proposed fix
- with pytest.raises(ValueError, match="Checksum mismatch for 0.0.1"): + with pytest.raises(ValueError, match=r"Checksum mismatch for 0\.0\.1"): contract_id.validate_checksum(client)
62-73: Use a tuple forpytest.mark.parametrizeargument names (PT006).Keeps Ruff happy and aligns with pytest style.
♻️ Proposed fix
`@pytest.mark.parametrize`( - "contract_str, expected", + ("contract_str", "expected"), [ ("1.2.101", (1, 2, 101, None, None)), ("0.0.0", (0, 0, 0, None, None)), ("1.2.3-abcde", (1, 2, 3, None, "abcde")), @@ ) @@ `@pytest.mark.parametrize`( - "evm_address_str, expected", + ("evm_address_str", "expected"), [ ( "abcdef0123456789abcdef0123456789abcdef01", (0, 0, 0, bytes.fromhex("abcdef0123456789abcdef0123456789abcdef01")),Also applies to: 366-378
101-103: Add commas to prevent implicit string concatenation ininvalid_idcases.Without commas, two intended test cases become one, reducing coverage.
🐛 Proposed fix
- " 1.2.abcdef0123456789abcdef0123456789abcdef01 " - "1.2.0xabcdef0123456789abcdef0123456789abcdef01" + " 1.2.abcdef0123456789abcdef0123456789abcdef01 ", + "1.2.0xabcdef0123456789abcdef0123456789abcdef01",
503-510: Escape dots in regexmatchfor invalid contract_id format.♻️ Proposed fix
with pytest.raises( ValueError, - match="Invalid contract_id format received: invalid.account.format", + match=r"Invalid contract_id format received: invalid\.account\.format", ): contract_id.populate_contract_num(client)
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/contract_id_test.py (1)
22-31: Consider addinghasattrassertions to protect against breaking changes.Per coding guidelines PRIORITY 1, tests should explicitly assert public attributes exist. While the value assertions implicitly check this, explicit
hasattrchecks provide clearer error messages if attributes are renamed or removed.Suggested enhancement
def test_default_initialization(): """Test ContractId initialization with default values.""" contract_id = ContractId() + assert hasattr(contract_id, 'shard'), "ContractId missing 'shard' attribute" + assert hasattr(contract_id, 'realm'), "ContractId missing 'realm' attribute" + assert hasattr(contract_id, 'contract'), "ContractId missing 'contract' attribute" + assert hasattr(contract_id, 'evm_address'), "ContractId missing 'evm_address' attribute" + assert hasattr(contract_id, 'checksum'), "ContractId missing 'checksum' attribute" assert contract_id.shard == 0 assert contract_id.realm == 0 assert contract_id.contract == 0 assert contract_id.evm_address is None assert contract_id.checksum is None
♻️ Duplicate comments (5)
tests/unit/contract_id_test.py (5)
62-73: Use tuple for parametrize argument names.Ruff PT006 suggests using a tuple instead of a string for the first argument of
pytest.mark.parametrizefor consistency.
345-348: Escape regex metacharacters inmatchpattern.The pattern
"Checksum mismatch for 0.0.1"contains dots which are regex metacharacters. Use a raw string with escaped dots for exact matching.
366-378: Use tuple for parametrize argument names.Ruff PT006 suggests using a tuple instead of a string for the first argument.
506-511: Escape regex metacharacters inmatchpattern.The pattern contains dots which are regex metacharacters. This could cause false positives.
483-496: Add assertions to verify immutability contract: return type, new instance creation, and original object unchanged.Per coding guidelines PRIORITY 1, the test must explicitly verify that
populate_contract_numreturns a newContractIdinstance (not mutating the original). This is critical for the PR objective of returning immutable instances.Proposed fix
def test_populate_contract_num_success(client): """Should populate contract number using mirror node response.""" evm_address = bytes.fromhex("abcdef0123456789abcdef0123456789abcdef01") contract_id = ContractId(shard=0, realm=0, evm_address=evm_address) with patch( "hiero_sdk_python.contract.contract_id.perform_query_to_mirror_node", return_value={"contract_id": "0.0.123"}, ): populated = contract_id.populate_contract_num(client) + assert isinstance(populated, ContractId), "populate_contract_num should return ContractId instance" + assert populated is not contract_id, "populate_contract_num should return a new instance" + assert contract_id.contract == 0, "Original ContractId should remain unchanged" assert populated.contract == 123 assert populated.evm_address == evm_address
| def test_from_bytes_success(): | ||
| """Should deserialize ContractId correctly from protobuf bytes.""" | ||
| original = ContractId(shard=1, realm=2, contract=3) | ||
| data = original.to_bytes() | ||
|
|
||
| reconstructed = ContractId.from_bytes(data) | ||
|
|
||
| assert reconstructed == original | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add return type assertion for from_bytes.
Per coding guidelines PRIORITY 1, explicitly assert the return type to protect against breaking changes.
Proposed fix
def test_from_bytes_success():
"""Should deserialize ContractId correctly from protobuf bytes."""
original = ContractId(shard=1, realm=2, contract=3)
data = original.to_bytes()
reconstructed = ContractId.from_bytes(data)
+ assert isinstance(reconstructed, ContractId), "from_bytes should return ContractId instance"
assert reconstructed == original
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@manishdait This is looking great!
Please be sure to address merge conflicts and the suggestions recommended from code rabbit. If you have any questions, please let us know, we are happy to help!
Thank you!!
manishdait
force-pushed
the
feat/contract-id-mirror-population
branch
from
9ae865c to
2f6d1c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
src/hiero_sdk_python/utils/entity_id_helper.py (1)
5-5: Replacetyping.Dictwith built-indict.
typing.Dictis deprecated in modern Python; usedict[...]for annotations instead.♻️ Proposed fix
-from typing import TYPE_CHECKING, Any, Dict +from typing import TYPE_CHECKING, Any @@ -def perform_query_to_mirror_node(url: str, timeout: float = 10) -> Dict[str, Any]: +def perform_query_to_mirror_node(url: str, timeout: float = 10) -> dict[str, Any]:Also applies to: 139-139
src/hiero_sdk_python/account/account_id.py (1)
326-333: Parse shard/realm from the mirror response instead of reusingself.
Using only the last segment can yield an incorrect AccountId if the mirror returns non‑zero shard/realm or a checksum. Parse the full ID string.🐛 Proposed fix
- num = int(account_id.split(".")[-1]) - return AccountId( - shard=self.shard, - realm=self.realm, - num=num, - evm_address=self.evm_address, - ) + shard, realm, num, _ = parse_from_string(account_id) + return AccountId( + shard=int(shard), + realm=int(realm), + num=int(num), + evm_address=self.evm_address, + )src/hiero_sdk_python/contract/contract_id.py (1)
247-255: Parse shard/realm/contract from the mirror response.
Using only the last segment risks incorrect IDs when shard/realm are non‑zero or checksums appear. Mirror response should be treated as authoritative.🐛 Proposed fix
- contract = int(contract_id.split(".")[-1]) - return ContractId( - shard=self.shard, - realm=self.realm, - contract=contract, - evm_address=self.evm_address, - ) + shard, realm, contract, _ = parse_from_string(contract_id) + return ContractId( + shard=int(shard), + realm=int(realm), + contract=int(contract), + evm_address=self.evm_address, + )tests/integration/account_id_population_e2e_test.py (1)
54-58: Replace fixed sleeps with retry/polling to avoid mirror lag flakes.
Hard-coded sleeps are timing-dependent; use a bounded retry with clear failure context.🧪 Proposed fix
+def _wait_for_mirror(fn, timeout_s: float = 30, interval_s: float = 2): + end = time.monotonic() + timeout_s + last_exc: Exception | None = None + while time.monotonic() < end: + try: + return fn() + except (RuntimeError, ValueError) as exc: + last_exc = exc + time.sleep(interval_s) + raise AssertionError(f"Mirror node did not update within {timeout_s}s") from last_exc @@ - # Wait for mirrornode to update - time.sleep(5) - mirror_account_id = AccountId.from_evm_address(evm_address, 0, 0) - new_account_id = mirror_account_id.populate_account_num(env.client) + new_account_id = _wait_for_mirror( + lambda: mirror_account_id.populate_account_num(env.client) + ) @@ - # Wait for mirror_node to update - time.sleep(5) - - new_account_id = account_id.populate_evm_address(env.client) + new_account_id = _wait_for_mirror( + lambda: account_id.populate_evm_address(env.client) + )Also applies to: 93-96
tests/unit/contract_id_test.py (4)
347-352: Escape regex metacharacters inmatch=patterns.Dots in the expected message should be escaped to avoid regex false positives.
Suggested fix
- with pytest.raises(ValueError, match="Checksum mismatch for 0.0.1"): + with pytest.raises(ValueError, match=r"Checksum mismatch for 0\.0\.1"): contract_id.validate_checksum(client) @@ with pytest.raises( ValueError, - match="Invalid contract_id format received: invalid.account.format", + match=r"Invalid contract_id format received: invalid\.account\.format", ): contract_id.populate_contract_num(client)Also applies to: 512-515
62-64: Use tuples forpytest.mark.parametrizeargument names.Ruff PT006 flags string argument names; use tuples in both parametrizations.
Suggested fix
`@pytest.mark.parametrize`( - "contract_str, expected", + ("contract_str", "expected"), [ ("1.2.101", (1, 2, 101, None, None)), ("0.0.0", (0, 0, 0, None, None)), ("1.2.3-abcde", (1, 2, 3, None, "abcde")), ( "1.2.abcdef0123456789abcdef0123456789abcdef01", (1, 2, 0, bytes.fromhex("abcdef0123456789abcdef0123456789abcdef01"), None), ), ], ) @@ `@pytest.mark.parametrize`( - "evm_address_str, expected", + ("evm_address_str", "expected"), [ ( "abcdef0123456789abcdef0123456789abcdef01", (0, 0, 0, bytes.fromhex("abcdef0123456789abcdef0123456789abcdef01")), ),Also applies to: 370-372
465-472: Add return-type assertion forfrom_bytes.This guards against breaking changes in public factory methods. As per coding guidelines, assert return types in unit tests.
Suggested fix
reconstructed = ContractId.from_bytes(data) + assert isinstance(reconstructed, ContractId), "from_bytes should return ContractId instance" assert reconstructed == original
488-501: Strengthenpopulate_contract_numsuccess assertions (type/immutability/shard/realm).The test should verify the new-instance contract and shard/realm preservation. As per coding guidelines, add these assertions.
Suggested fix
with patch( "hiero_sdk_python.contract.contract_id.perform_query_to_mirror_node", return_value={"contract_id": "0.0.123"}, ): populated = contract_id.populate_contract_num(client) + assert isinstance(populated, ContractId), "populate_contract_num should return ContractId instance" + assert populated is not contract_id, "populate_contract_num should return a new instance" assert populated.contract == 123 assert populated.evm_address == evm_address + assert populated.shard == 0, "Shard should be preserved" + assert populated.realm == 0, "Realm should be preserved"tests/integration/contract_id_population_e2e_test.py (1)
62-65: Replace fixed sleep with bounded retry to avoid flakes.Mirror-node propagation is variable; a fixed sleep makes the test flaky. Use a retry loop with a timeout for deterministic behavior. As per coding guidelines, avoid timing-dependent assertions in integration tests.
Suggested fix
- # Wait for mirror_node to update - time.sleep(5) - - final_contract_id = contract_id_with_evm_addr.populate_contract_num(env.client) + # Wait for mirror node to update (bounded retry) + deadline = time.monotonic() + 30 + while True: + try: + final_contract_id = contract_id_with_evm_addr.populate_contract_num(env.client) + break + except (RuntimeError, ValueError): + if time.monotonic() >= deadline: + raise AssertionError( + "Mirror node did not return contract_id within 30s" + ) + time.sleep(1)tests/unit/account_id_test.py (1)
797-797: Useis Nonefor None comparison.This avoids Ruff E711 and follows Python idioms.
Suggested fix
- assert account_id.evm_address == None + assert account_id.evm_address is None
| - Added first-class support for EVM address aliases in `AccountId`, including parsing, serialization, Mirror Node population helpers. | ||
| - Add automated bot to recommend next issues to contributors after their first PR merge (#1380) | ||
| - Added dry-run support and refactored `.github/workflows/bot-workflows.yml` to use dedicated script `.github/scripts/bot-workflows.js` for improved maintainability and testability. (`#1288`) | ||
| - Added MirrorNode based population for `ContractId`, including parsing and serialization support. |
There was a problem hiding this comment.
Hyphenate “MirrorNode-based”.
Minor grammar fix per LanguageTool.
✏️ Proposed fix
-- Added MirrorNode based population for `ContractId`, including parsing and serialization support.
+- Added MirrorNode-based population for `ContractId`, including parsing and serialization support.📝 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.
| - Added MirrorNode based population for `ContractId`, including parsing and serialization support. | |
| - Added MirrorNode-based population for `ContractId`, including parsing and serialization support. |
🧰 Tools
🪛 LanguageTool
[grammar] ~124-~124: Use a hyphen to join words.
Context: ...estability. (#1288) - Added MirrorNode based population for ContractId, inclu...
(QB_NEW_EN_HYPHEN)
| time.sleep(5) | ||
|
|
||
| try: | ||
| mirror_account_id.populate_account_num(client) | ||
| new_account_id = mirror_account_id.populate_account_num(client) | ||
| except Exception as e: |
There was a problem hiding this comment.
Avoid fixed sleeps; poll the mirror node in examples.
Mirror propagation is variable; a retry loop makes the example reliable for users.
💡 Suggested improvement
+def _wait_for_mirror(fn, timeout_s: float = 30, interval_s: float = 2):
+ end = time.time() + timeout_s
+ last_exc: Exception | None = None
+ while time.time() < end:
+ try:
+ return fn()
+ except (RuntimeError, ValueError) as exc:
+ last_exc = exc
+ time.sleep(interval_s)
+ raise RuntimeError(f"Mirror node did not update within {timeout_s}s") from last_exc
@@
- time.sleep(5)
-
try:
- new_account_id = mirror_account_id.populate_account_num(client)
+ new_account_id = _wait_for_mirror(
+ lambda: mirror_account_id.populate_account_num(client)
+ )
@@
- time.sleep(5)
-
try:
- new_account_id = created_account_id.populate_evm_address(client)
+ new_account_id = _wait_for_mirror(
+ lambda: created_account_id.populate_evm_address(client)
+ )Also applies to: 110-114
🧰 Tools
🪛 Ruff (0.14.13)
84-84: Do not catch blind exception: Exception
(BLE001)
| from tests.integration.utils import env | ||
|
|
||
|
|
||
| @pytest.mark.integration | ||
| def test_populate_contract_id_num(env): |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the test file
find . -name "contract_id_population_e2e_test.py" -type f | head -1 | xargs wc -lRepository: hiero-ledger/hiero-sdk-python
Length of output: 130
🏁 Script executed:
# Read the test file around the specified lines
find . -name "contract_id_population_e2e_test.py" -type f | head -1 | xargs cat -n | head -40Repository: hiero-ledger/hiero-sdk-python
Length of output: 1643
🏁 Script executed:
# Check the utils module to see how env is defined
find . -path "*/tests/integration/utils.py" -type f | head -1 | xargs cat -n | head -100Repository: hiero-ledger/hiero-sdk-python
Length of output: 4730
🏁 Script executed:
# Check for sleep calls in the test file
grep -n "sleep\|time\.sleep" ./tests/integration/contract_id_population_e2e_test.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 93
🏁 Script executed:
# Verify if ruff would actually flag F811 for this pattern
# First check if ruff is available and what it reports
if command -v ruff &> /dev/null; then
ruff check ./tests/integration/contract_id_population_e2e_test.py --select F811 2>&1 || true
else
echo "ruff not available, checking with grep for shadowing pattern"
grep -A 5 "from tests.integration.utils import env" ./tests/integration/contract_id_population_e2e_test.py | head -10
fiRepository: hiero-ledger/hiero-sdk-python
Length of output: 1117
🏁 Script executed:
# Check how pytest fixtures are typically loaded in conftest.py or other test files
find ./tests -name "conftest.py" -type f | head -1 | xargs cat -n 2>/dev/null | head -50 || echo "No conftest.py found in tests root"Repository: hiero-ledger/hiero-sdk-python
Length of output: 2171
🏁 Script executed:
# Search for other test files that use the env fixture to see the pattern
rg "from tests.integration.utils import" tests/ -A 3 --context 5 | head -40Repository: hiero-ledger/hiero-sdk-python
Length of output: 3774
🏁 Script executed:
# Check for conftest.py in the integration tests directory
find ./tests/integration -name "conftest.py" -type f | head -5Repository: hiero-ledger/hiero-sdk-python
Length of output: 55
🏁 Script executed:
# List all files in tests/integration to understand structure
ls -la ./tests/integration/ | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 1616
🏁 Script executed:
# Check the root tests directory for conftest.py
find ./tests -maxdepth 2 -name "conftest.py" -type fRepository: hiero-ledger/hiero-sdk-python
Length of output: 97
🏁 Script executed:
# Check pytest configuration to understand fixture discovery
ls -la ./tests/ | grep -E "conftest|pytest"Repository: hiero-ledger/hiero-sdk-python
Length of output: 55
🏁 Script executed:
# Look at a working test file that correctly uses env as fixture
grep -B5 -A10 "def test_integration_contract_bytecode_query_can_execute" ./tests/integration/contract_bytecode_query_e2e_test.py | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 660
🏁 Script executed:
# Check if there's a way fixtures are being discovered - look at pytest configuration
cat ./pyproject.toml | grep -A 20 "\[tool.pytest" 2>/dev/null || echo "No pytest config found in pyproject.toml"Repository: hiero-ledger/hiero-sdk-python
Length of output: 585
🏁 Script executed:
# Check if a conftest.py needs to be created in integration tests and what would be needed
head -50 ./tests/unit/conftest.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 1821
🏁 Script executed:
# Check pytest.ini or setup.cfg
cat ./pytest.ini 2>/dev/null || cat ./setup.cfg 2>/dev/null | grep -A 10 "\[tool:pytest\]" || echo "No pytest config in standard files"Repository: hiero-ledger/hiero-sdk-python
Length of output: 202
🏁 Script executed:
# Verify the complete implementation needed - what does the unit conftest show?
wc -l ./tests/unit/conftest.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 101
🏁 Script executed:
# Understand how pytest_plugins works - check if it's only valid in conftest.py
# by examining the pytest documentation pattern and testing scenario
python3 <<'EOF'
import sys
# pytest_plugins is a special variable that must be in conftest.py
# If placed in a regular test file, pytest won't process it
# Let's verify this is indeed how pytest works
print("pytest_plugins variable scope analysis:")
print("- pytest_plugins must be defined in conftest.py (or pytest plugins)")
print("- If defined in regular test modules, pytest will NOT process it")
print("- Placing it in a regular test file would NOT solve the F811 issue")
print("\nThe correct fix requires:")
print("1. Create tests/integration/conftest.py")
print("2. Add: pytest_plugins = ['tests.integration.utils']")
print("3. Remove: from tests.integration.utils import env (from all test files)")
EOFRepository: hiero-ledger/hiero-sdk-python
Length of output: 159
Create tests/integration/conftest.py to register the env fixture and remove the import from test files.
The F811 shadowing error is real: importing env at line 22 while using it as a fixture parameter at line 26 causes a redefinition conflict. However, the suggested fix in the original review is incomplete. The pytest_plugins variable only works when declared in conftest.py, not in individual test files.
The correct fix:
- Create
tests/integration/conftest.pywith:
pytest_plugins = ["tests.integration.utils"]- Remove the import from this test file (and all integration test files):
-from tests.integration.utils import envPytest will then discover the env fixture from tests.integration.utils automatically without shadowing.
🧰 Tools
🪛 Ruff (0.14.13)
26-26: Redefinition of unused env from line 22: env redefined here
(F811)
There was a problem hiding this comment.
Everything LGTM I think its ready to merge
One remaining concern from the latest CodeRabbit review:
- fixed sleep(5) in integration tests/examples (potential flakiness)
- env fixture import vs conftest.py (pytest best practice / F811)
These aren’t correctness blockers, but would be good to address either in this PR or a follow-up.
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
I concur with @undefinedIsMyLife
If you have any questions about the suggestions please reach out.
We'd be happy to assist.
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
… pattern Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
Signed-off-by: Manish Dait <[email protected]>
manishdait
force-pushed
the
feat/contract-id-mirror-population
branch
from
2f6d1c6 to
4755c7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/account_id_population_e2e_test.py (1)
40-54: Assert receipt status for clearer diagnostics.Add explicit status checks before relying on child receipts.
✅ Suggested update
from hiero_sdk_python.crypto.private_key import PrivateKey from hiero_sdk_python.hbar import Hbar +from hiero_sdk_python.response_code import ResponseCode from hiero_sdk_python.query.transaction_get_receipt_query import ( TransactionGetReceiptQuery, ) @@ transfer_receipt = ( TransactionGetReceiptQuery() .set_transaction_id(transfer_tx.transaction_id) .set_include_children(True) .execute(env.client) ) assert transfer_receipt is not None + assert ( + transfer_receipt.status == ResponseCode.SUCCESS + ), f"Transfer failed: {ResponseCode(transfer_receipt.status).name}" assert ( len(transfer_receipt.children) > 0 ), "Expected child transaction for auto-account creation"src/hiero_sdk_python/utils/entity_id_helper.py (1)
17-33: Add return type annotation and fix docstring.The function lacks a return type annotation, and the docstring incorrectly says "Returns: An instance of cls..." which appears to be a copy-paste error. The function returns a tuple of strings.
Suggested fix
-def parse_from_string(address: str): +def parse_from_string(address: str) -> tuple[str, str, str, str | None]: """ Parse an address string of the form: <shard>.<realm>.<num>[-<checksum>] Examples: "0.0.123" "0.0.123-abcde" Returns: - An instance of cls with shard, realm, num, and optional checksum. + A tuple of (shard, realm, num, checksum) where checksum may be None. + + Raises: + ValueError: If the address format is invalid. """
|
Thanks for the review! |
|
Regarding the env fixture import: The current pattern, where env is imported directly from tests.integration.utils, works correctly and all tests continue to pass. The only downside is a linter/pytest warning (F811) Moving env to conftest.py would be a best-practice improvement, but it is not required for correctness. All other integration tests follow the same pattern, which helps maintain consistency across the test suite. If desired, as a future improvement we could move the env fixture to tests/integration/conftest.py to avoid shadowing warnings and remove the from tests.integration.utils import env lines in all test files. This could be done in a follow-up PR without affecting the current functionality. |
|
@exploreriii @aceppaluni @AntonioCeppellini , |
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@manishdait Thank you so much!!
I think this is good for now to keep the PR in scope.
We can pose a follow up issue after to follow best practices.
7 participants
Description:
This PR adds support for populating the ContractId numeric value using a Mirror Node query, similar to the existing
AccountId.populate_account_num()functionality introduced in PR #1009.Chnaged Made
from_evm_addressmethod to ContractId.populate_contract_num(client)method to ContractId to resolve the contract number via Mirror Node._from_proto()to correctly handleevm_addresswhen present in the protobuf representation.to_evm_address()to use the method form EntityHelper utils.to_bytes()andfrom_bytes().Related issue(s):
Fixes #1564
Checklist