feat: Added support for include_children to the TransactionRecordQuery class #1621
Conversation
|
[commit-verification-bot] To achieve verified status, please read: Remember, you require a GPG key and each commit must be signed with: Thank you for contributing! From the Hiero Python SDK Team |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds optional child-record retrieval to TransactionRecordQuery via an Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Query as TransactionRecordQuery
participant Network as Network/Service
participant Model as TransactionRecord
Client->>Query: set_transaction_id(tx_id)
Client->>Query: set_include_children(true)
Client->>Query: execute(client)
Query->>Query: _make_request()
Query->>Network: getTxRecordByTxID(txID, include_child_records=true)
Network-->>Query: response (parent + child records)
Query->>Query: _map_record_list(child_records)
Query->>Model: _from_proto(parent_proto, children=[...])
Model-->>Query: TransactionRecord (with children)
Query-->>Client: TransactionRecord (parent with children)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hiero_sdk_python/transaction/transaction_record.py (1)
174-185: Critical bug:childrenparameter is ignored.The
childrenparameter passed to_from_proto(line 123) is completely overwritten by the local variable at line 174. This means child records passed fromTransactionRecordQuery.execute()are never actually stored in the resultingTransactionRecord.- children: Optional[List["TransactionRecord"]] = None return cls( transaction_id=transaction_id, ... - children=children, + children=children, # Use the parameter, not a new local variableThe fix should remove line 174 entirely so that the
childrenparameter from line 123 is used:🐛 Proposed fix
for pending_airdrop in proto.new_pending_airdrops: new_pending_airdrops.append( PendingAirdropRecord._from_proto(pending_airdrop) ) - children: Optional[List["TransactionRecord"]] = None return cls( transaction_id=transaction_id, transaction_hash=proto.transactionHash,src/hiero_sdk_python/query/transaction_record_query.py (1)
90-92: Avoidprint()statements in production code.Using
print()for error logging is not appropriate for library code. This can cause unexpected output for SDK users and doesn't integrate with their logging configuration.Consider using Python's
loggingmodule or removing the print statement entirely since the exception is re-raised anyway.except Exception as e: - print(f"Exception in _make_request: {e}") raise
| def test_transaction_record_query_with_children_mapping(transaction_id): | ||
|
|
||
| child_proto = transaction_record_pb2.TransactionRecord( | ||
| memo="Child Record" | ||
| ) | ||
| child_proto.transactionID.CopyFrom(transaction_id._to_proto()) | ||
|
|
||
| record_response = transaction_get_record_pb2.TransactionGetRecordResponse() | ||
|
|
||
| record_response.header.nodeTransactionPrecheckCode = ResponseCode.OK | ||
|
|
||
| record_response.transactionRecord.memo = "Parent Record" | ||
| record_response.transactionRecord.transactionID.CopyFrom(transaction_id._to_proto()) | ||
|
|
||
| record_response.child_transaction_records.extend([child_proto, child_proto]) | ||
|
|
||
| query = TransactionRecordQuery(transaction_id) | ||
| children = query._map_record_list(record_response.child_transaction_records) | ||
|
|
||
| assert len(children) == 2 | ||
| assert children[0].transaction_memo == "Child Record" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test coverage gaps for the new include_children feature.
Per the coding guidelines, unit tests should verify:
- Fluent setters return
self: Missing test forset_include_childrenreturningselffor chaining. - Backward-compatible defaults: Missing test that
include_childrendefaults toFalse. - Constructor behavior: Missing test for
TransactionRecordQuery(transaction_id, include_children=True). - Request construction: Missing test that
_make_request()correctly setsinclude_child_recordson the protobuf.
💚 Suggested additional tests
def test_set_include_children_returns_self(transaction_id):
"""Test that set_include_children returns self for method chaining."""
query = TransactionRecordQuery(transaction_id)
result = query.set_include_children(True)
assert result is query # Fluent interface check
def test_include_children_default_is_false():
"""Test that include_children defaults to False."""
query = TransactionRecordQuery()
assert query.include_children is False
def test_constructor_with_include_children(transaction_id):
"""Test constructor accepts include_children parameter."""
query = TransactionRecordQuery(transaction_id, include_children=True)
assert query.include_children is True
assert query.transaction_id == transaction_id
def test_make_request_includes_children_flag(transaction_id):
"""Test that _make_request sets include_child_records correctly."""
query = TransactionRecordQuery(transaction_id, include_children=True)
request = query._make_request()
assert request.transactionGetRecord.include_child_records is TrueBased on learnings: "Assert fluent setters return self", "Assert backward-compatible defaults are maintained", "Test constructor behavior with valid inputs".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/hiero_sdk_python/account/account_info.py (2)
12-109: Fix staking_info wiring (wrong class + unused variable).
StakingInfois imported from the protobuf module and then used with.from_proto(), which doesn’t exist. Additionally, the computedstaking_infois never assigned toaccount_info, so it’s always lost. This will either raise at runtime or silently drop staking data.🐛 Proposed fix
-from hiero_sdk_python.hapi.services.basic_types_pb2 import StakingInfo +from hiero_sdk_python.staking_info import StakingInfo ... - account_info: "AccountInfo" = cls( + account_info: "AccountInfo" = cls( account_id=AccountId._from_proto(proto.accountID) if proto.accountID else None, contract_account_id=proto.contractAccountID, is_deleted=proto.deleted, proxy_received=Hbar.from_tinybars(proto.proxyReceived), key=PublicKey._from_proto(proto.key) if proto.key else None, balance=Hbar.from_tinybars(proto.balance), receiver_signature_required=proto.receiverSigRequired, expiration_time=( Timestamp._from_protobuf(proto.expirationTime) if proto.expirationTime else None ), auto_renew_period=( Duration._from_proto(proto.autoRenewPeriod) if proto.autoRenewPeriod else None ), token_relationships=[ TokenRelationship._from_proto(relationship) for relationship in proto.tokenRelationships ], account_memo=proto.memo, owned_nfts=proto.ownedNfts, max_automatic_token_associations=proto.max_automatic_token_associations, + staking_info=( + StakingInfo._from_proto(proto.staking_info) + if proto.HasField("staking_info") + else None + ), ) - - staking_info=( - StakingInfo.from_proto(proto.staking_info) - if proto.HasField("staking_info") - else None - )And in
_to_proto:- staking_info=( - self.staking_info.to_proto() - if self.staking_info is not None - else None - ), + staking_info=( + self.staking_info._to_proto() + if self.staking_info is not None + else None + ),
161-199: Fix staking_info labels in str/repr.
Current output prints the wholestaking_infoobject for “Staked Account ID”, “Staked Node ID”, and “Decline Staking Reward”. This is misleading.💡 Suggested adjustment
- (self.staking_info, "Staked Account ID"), - (self.staking_info, "Staked Node ID"), + (self.staking_info.staked_account_id if self.staking_info else None, "Staked Account ID"), + (self.staking_info.staked_node_id if self.staking_info else None, "Staked Node ID"), ... - if self.staking_info is not None: - lines.append(f"Decline Staking Reward: {self.staking_info}") + if self.staking_info is not None: + lines.append( + f"Decline Staking Reward: {self.staking_info.decline_reward}" + ) ... - f"staked_node_id={self.staking_info!r}, " - f"staked_account_id={self.staking_info!r}" + f"staking_info={self.staking_info!r}"src/hiero_sdk_python/transaction/transaction_record.py (1)
119-186: Children are discarded in _from_proto.
The method acceptschildrenbut immediately overwrites it withNone, so include‑children never surfaces in returned records.🐛 Proposed fix
- def _from_proto( + def _from_proto( cls, proto: transaction_record_pb2.TransactionRecord, transaction_id: Optional[TransactionId] = None, - children=None, + children: Optional[list["TransactionRecord"]] = None, ) -> "TransactionRecord": ... - children: Optional[List["TransactionRecord"]] = None return cls( transaction_id=transaction_id, transaction_hash=proto.transactionHash, transaction_memo=proto.memo, transaction_fee=proto.transactionFee, receipt=TransactionReceipt._from_proto(proto.receipt, transaction_id), token_transfers=token_transfers, nft_transfers=nft_transfers, transfers=transfers, new_pending_airdrops=new_pending_airdrops, - children=children, + children=children, call_result=( ContractFunctionResult._from_proto(proto.contractCallResult) if proto.HasField("contractCallResult") else None ), prng_number=proto.prng_number, prng_bytes=proto.prng_bytes, )src/hiero_sdk_python/query/transaction_record_query.py (1)
90-92: RemoveThe
Suggested fix
except Exception as e: - print(f"Exception in _make_request: {e}") raise
CHANGELOG.md
Outdated
| <<<<<<< HEAD | ||
| - Refactored AccountInfo class to use the staking_info | ||
| ======= | ||
| - chore: format tests/unit/mock_server.py with black (#1542) | ||
| >>>>>>> upstream |
There was a problem hiding this comment.
Resolve the merge conflict block before release.
The changelog contains conflict markers, which will break rendering and automation. Please remove the markers and keep the correct entries.
🔧 Suggested resolution
-<<<<<<< HEAD
-- Refactored AccountInfo class to use the staking_info
-=======
-- chore: format tests/unit/mock_server.py with black (`#1542`)
->>>>>>> upstream
+- Refactored AccountInfo class to use the staking_info
+- chore: format tests/unit/mock_server.py with black (`#1542`)📝 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.
| <<<<<<< HEAD | |
| - Refactored AccountInfo class to use the staking_info | |
| ======= | |
| - chore: format tests/unit/mock_server.py with black (#1542) | |
| >>>>>>> upstream | |
| - Refactored AccountInfo class to use the staking_info | |
| - chore: format tests/unit/mock_server.py with black (`#1542`) |
Signed-off-by: Jessica G <jwagantall@linuxfoundation.org>
Signed-off-by: mukundkumarjha <mukundiiitg@gmail.com>
Signed-off-by: mukundkumarjha <mukundiiitg@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hiero_sdk_python/transaction/transaction_record.py (1)
174-186: Preserve provided child records (currently overwritten toNone).Line 174 resets
childrentoNone, discarding the list passed fromTransactionRecordQuery.execute(). This breaks the newinclude_childrenfeature because the parent record will never retain its child records.🐛 Proposed fix
- children: Optional[List["TransactionRecord"]] = None + if children is None: + children = [] return cls( transaction_id=transaction_id, transaction_hash=proto.transactionHash, transaction_memo=proto.memo, transaction_fee=proto.transactionFee, receipt=TransactionReceipt._from_proto(proto.receipt, transaction_id), token_transfers=token_transfers, nft_transfers=nft_transfers, transfers=transfers, new_pending_airdrops=new_pending_airdrops, - children=children, + children=children, call_result=( ContractFunctionResult._from_proto(proto.contractCallResult) if proto.HasField("contractCallResult") else None ), prng_number=proto.prng_number, prng_bytes=proto.prng_bytes, )
| return self | ||
|
|
||
| def set_include_children(self, include_children): | ||
| self.include_children = include_children |
There was a problem hiding this comment.
Minor suggestion consider adding a docstring
| self.transaction_id : Optional[TransactionId] = transaction_id | ||
|
|
||
| self.transaction_id: Optional[TransactionId] = transaction_id | ||
| self.include_children = include_children |
There was a problem hiding this comment.
missing type hint
| self.include_children = include_children | |
| self.include_children: Optional[bool] = include_children |
|
|
||
| def _map_record_list( | ||
| self, | ||
| proto_records: List[transaction_get_record_pb2.TransactionGetRecordResponse], |
There was a problem hiding this comment.
must be
| proto_records: List[transaction_get_record_pb2.TransactionGetRecordResponse], | |
| proto_records: List[transaction_record_pb2.TransactionRecord], |
| cls, | ||
| proto: transaction_record_pb2.TransactionRecord, | ||
| transaction_id: Optional[TransactionId] = None, | ||
| children=None, |
There was a problem hiding this comment.
add type hinting children: Optional[List["TransactionRecord"]]=[]
| PendingAirdropRecord._from_proto(pending_airdrop) | ||
| ) | ||
|
|
||
| children: Optional[List["TransactionRecord"]] = None |
There was a problem hiding this comment.
this line can be romove
manishdait
left a comment
manishdait
left a comment
There was a problem hiding this comment.
@mukundkumarjha, Thanks for PR added some required changes
manishdait
left a comment
manishdait
left a comment
There was a problem hiding this comment.
@mukundkumarjha, Thanks for PR added some required changes
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@mukundkumarjha Thank you for your contribution!
I concur with @manishdait with the suggested changes.
If you need assistance please reach out.
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
Signed-off-by: Mukund Jha <mukundiiitg@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hiero_sdk_python/transaction/transaction_record.py (1)
123-186:⚠️ Potential issue | 🔴 Critical
childrenis discarded, so include_children always yields None.
Line 174 overwrites thechildrenparameter withNone, soTransactionRecordQuery.execute()can never populate children and the example will crash when it callslen(record.children).🔧 Proposed fix
- children: Optional[List["TransactionRecord"]] = None + if children is None: + children = [] return cls( @@ - children=children, + children=children,
| record = query.execute(client) | ||
|
|
||
| # 3. Demonstrate accessing the children | ||
| print(f"Parent Transaction ID: {record.transaction_id}") | ||
| print(f"Number of child records found: {len(record.children)}") | ||
|
|
||
| for i, child in enumerate(record.children): | ||
| print(f"Child {i+1} Status: {child.receipt.status}") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the file in question
fd "transaction_record_with_children.py" --type fRepository: hiero-ledger/hiero-sdk-python
Length of output: 129
🏁 Script executed:
# Check if the file exists and read its content
if [ -f "examples/transaction/transaction_record_with_children.py" ]; then
wc -l examples/transaction/transaction_record_with_children.py
echo "---"
cat -n examples/transaction/transaction_record_with_children.py
else
echo "File not found"
fiRepository: hiero-ledger/hiero-sdk-python
Length of output: 1383
🏁 Script executed:
# Verify ResponseCode exists in hiero_sdk_python
find . -type f -name "*.py" | xargs grep -l "class ResponseCode" | head -5Repository: hiero-ledger/hiero-sdk-python
Length of output: 112
🏁 Script executed:
# Check how ResponseCode is imported in other example files
find examples -type f -name "*.py" -exec grep -l "ResponseCode" {} \; | head -5Repository: hiero-ledger/hiero-sdk-python
Length of output: 3862
🏁 Script executed:
# Look for patterns of receipt.status usage in examples
rg "receipt\.status" examples --type py -B 2 -A 2 | head -40Repository: hiero-ledger/hiero-sdk-python
Length of output: 3540
🏁 Script executed:
# Check how ResponseCode is imported in those example files
head -20 examples/tokens/token_fee_schedule_update_transaction_fungible.py | grep -E "^(import|from)" | head -10Repository: hiero-ledger/hiero-sdk-python
Length of output: 498
🏁 Script executed:
# Check if there's a general pattern for error handling in examples
head -30 examples/tokens/token_create_transaction_token_fee_schedule_key.py | grep -E "^(import|from)" | head -10Repository: hiero-ledger/hiero-sdk-python
Length of output: 454
🏁 Script executed:
# Verify the ResponseCode import path can be used as suggested
head -5 src/hiero_sdk_python/response_code.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 154
🏁 Script executed:
# Check other transaction_record examples for patterns
find examples -name "*transaction_record*" -type fRepository: hiero-ledger/hiero-sdk-python
Length of output: 172
🏁 Script executed:
# Look at how examples handle query results without receipt checks (queries vs transactions)
rg "\.execute\(client\)" examples/query --type py -A 3 | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 988
🏁 Script executed:
# Check the other transaction_record query example
cat -n examples/query/transaction_record_query.py | head -40Repository: hiero-ledger/hiero-sdk-python
Length of output: 1536
🏁 Script executed:
# Verify ResponseCode is used consistently in query examples
rg "ResponseCode" examples/query --type py -B 1 -A 1 | head -30Repository: hiero-ledger/hiero-sdk-python
Length of output: 2088
Validate child receipt status using ResponseCode enum.
Line 30 prints the raw numeric status without converting to ResponseCode, making it unclear to users whether children succeeded or failed. This contradicts the pattern used throughout examples where all receipt statuses are validated against ResponseCode.SUCCESS.
✅ Suggested improvement
from hiero_sdk_python.query.transaction_record_query import TransactionRecordQuery
+from hiero_sdk_python.response_code import ResponseCode
@@
for i, child in enumerate(record.children):
- print(f"Child {i+1} Status: {child.receipt.status}")
+ status = ResponseCode(child.receipt.status)
+ print(f"Child {i+1} Status: {status.name}")
+ if status != ResponseCode.SUCCESS:
+ print(f"Warning: Child {i+1} failed with status {status.name}")
prajeeta15
left a comment
prajeeta15
left a comment
There was a problem hiding this comment.
@mukundkumarjha please address all the coderabbitai issues before pushing code.
|
@mukundkumarjha |
|
Hi @mukundkumarjha |
exploreriii
added
the
status: needs developer revision
Signed-off-by: Mukund Jha <mukundiiitg@gmail.com>
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 (4)
src/hiero_sdk_python/transaction/transaction_record.py (3)
106-132:⚠️ Potential issue | 🔴 CriticalUnreachable dead code — duplicate
returnin__repr__.The first
returnstatement at lines 106–119 makes lines 120–132 unreachable. This appears to be merge-conflict residue where both the old and new versions of__repr__were kept. Remove one of the two blocks (likely the one at lines 120–132, which includes theduplicates_countfield from the other PR).return ( f"TransactionRecord(transaction_id='{self.transaction_id}', " f"transaction_hash={self.transaction_hash}, " f"transaction_memo='{self.transaction_memo}', " f"transaction_fee={self.transaction_fee}, " f"receipt_status='{status}', " f"token_transfers={dict(self.token_transfers)}, " f"nft_transfers={dict(self.nft_transfers)}, " f"transfers={dict(self.transfers)}, " f"new_pending_airdrops={list(self.new_pending_airdrops)}, " f"call_result={self.call_result}, " f"prng_number={self.prng_number}, " f"prng_bytes={self.prng_bytes})" ) - return (f"TransactionRecord(transaction_id='{self.transaction_id}', " - f"transaction_hash={self.transaction_hash}, " - f"transaction_memo='{self.transaction_memo}', " - f"transaction_fee={self.transaction_fee}, " - f"receipt_status='{status}', " - f"token_transfers={dict(self.token_transfers)}, " - f"nft_transfers={dict(self.nft_transfers)}, " - f"transfers={dict(self.transfers)}, " - f"new_pending_airdrops={list(self.new_pending_airdrops)}, " - f"call_result={self.call_result}, " - f"prng_number={self.prng_number}, " - f"prng_bytes={self.prng_bytes}, " - f"duplicates_count={len(self.duplicates)})")
134-200:⚠️ Potential issue | 🔴 CriticalMerge-conflict residue: duplicate
_from_protosignatures and body logic.The method has two conflicting signatures (lines 135–140 vs 141–142) and two parallel implementations:
- Lines 172–193: Inline parsing logic (old version)
- Lines 194–200: Refactored helper-method calls (new version)
Both cannot coexist — this will produce syntax errors. The file needs to be cleaned up to keep only one version. The refactored helper version (lines 194–200) is cleaner and should be kept, but ensure the
childrenparameter is properly integrated.
203-223:⚠️ Potential issue | 🔴 CriticalMerge-conflict residue: duplicate constructor arguments in
cls()call.The
return cls(...)block contains duplicate arguments from both the old and new implementations:
call_resultappears twice (lines 214–218 and line 219)childrenis passed (line 213) but was already overwritten toNone(line 202)- Both
childrenandduplicateskeyword arguments are present, but the_from_protosignature can't have both without proper mergingThis entire block needs to be reconciled into a single clean constructor call. Example of what the merged version should look like:
return cls( transaction_id=tx_id, transaction_hash=proto.transactionHash, transaction_memo=proto.memo, transaction_fee=proto.transactionFee, receipt=TransactionReceipt._from_proto(proto.receipt, tx_id), token_transfers=token_transfers, nft_transfers=nft_transfers, transfers=transfers, new_pending_airdrops=new_pending_airdrops, call_result=call_result, prng_number=proto.prng_number, prng_bytes=proto.prng_bytes, children=children or [], duplicates=duplicates or [], )src/hiero_sdk_python/query/transaction_record_query.py (1)
302-336:⚠️ Potential issue | 🔴 Critical
execute()is missingtimeoutparameter andinclude_duplicateshandling.This version of
execute(from theinclude_childrenbranch) doesn't accept thetimeoutparameter, which is required by the base class contract (see existing entry at line 306). It also doesn't handleinclude_duplicates. The twoexecuteimplementations need to be merged into one that supports both features.Merged execute suggestion
def execute(self, client: Client, timeout: Optional[Union[int, float]] = None): self._before_execute(client) response = self._execute(client, timeout) primary_proto = response.transactionGetRecord.transactionRecord children = self._map_record_list( response.transactionGetRecord.child_transaction_records ) if self.include_children else [] duplicates = self._map_record_list( response.transactionGetRecord.duplicateTransactionRecords ) if self.include_duplicates else [] return TransactionRecord._from_proto( primary_proto, transaction_id=self.transaction_id, children=children, duplicates=duplicates, )As per coding guidelines, Review Focus 2: "All queries MUST use the base Query execution flow" and Review Focus 6: flag changes that "Alter base Query behavior".
| from typing import Optional, Any, Union, List | ||
| from typing import Optional, Any, Union | ||
| from hiero_sdk_python.hapi.services import ( | ||
| query_header_pb2, | ||
| transaction_get_record_pb2, | ||
| query_pb2, | ||
| ) | ||
| transaction_record_pb2, | ||
| ) |
There was a problem hiding this comment.
File has unresolved merge-conflict residue — will not parse as valid Python.
The file contains duplicate/conflicting code from what appears to be an incomplete merge with the include_duplicates feature branch. Multiple constructs are duplicated:
- Two
typingimports (lines 1–2) - Two
__init__signatures (lines 27–33) - Two
_make_requestimplementations (lines 102–143 vs 148–160) - Two
_map_record_listdefinitions (lines 162–188 vs 292–300) - Two
executemethods (lines 302–336 vs 306–347) - Two
set_transaction_iddefinitions (lines 41–43 vs 74–96)
Ruff's static analysis confirms numerous syntax errors throughout the file. The entire file needs to be properly rebased/merged against main before it can be reviewed meaningfully. The two feature branches (include_children and include_duplicates) need to be combined into a single coherent implementation.
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 8-8: Unexpected indentation
(invalid-syntax)
[warning] 9-9: Expected a statement
(invalid-syntax)
| def set_include_children(self, include_children): | ||
| self.include_children = include_children | ||
| return self |
There was a problem hiding this comment.
set_include_children lacks input validation, inconsistent with set_include_duplicates.
set_include_duplicates (lines 57–72) validates its input with isinstance(include_duplicates, bool) and raises TypeError for invalid types. set_include_children does no validation at all, creating an inconsistency.
def set_include_children(self, include_children):
+ if not isinstance(include_children, bool):
+ raise TypeError(
+ f"include_children must be a boolean, got {type(include_children).__name__}"
+ )
self.include_children = include_children
return selfAs per coding guidelines, "Ensure naming matches existing query patterns" — the validation pattern used by set_include_duplicates should be followed here.
| try: | ||
| if not self.transaction_id: | ||
| raise ValueError( | ||
| "Transaction ID must be set before making the request." | ||
| ) | ||
|
|
||
| query_header = self._make_request_header() | ||
| transaction_get_record = ( | ||
| transaction_get_record_pb2.TransactionGetRecordQuery() | ||
| ) | ||
| transaction_get_record.header.CopyFrom(query_header) | ||
|
|
||
| transaction_get_record.transactionID.CopyFrom( | ||
| self.transaction_id._to_proto() | ||
| ) | ||
| transaction_get_record.include_child_records = self.include_children | ||
| query = query_pb2.Query() | ||
| if not hasattr(query, "transactionGetRecord"): | ||
| raise AttributeError( | ||
| "Query object has no attribute 'transactionGetRecord'" | ||
| ) | ||
| query.transactionGetRecord.CopyFrom(transaction_get_record) | ||
|
|
||
| return query | ||
| except Exception as e: | ||
| print(f"Exception in _make_request: {e}") | ||
| raise |
There was a problem hiding this comment.
Remove try/except + print() from _make_request — introduces side effects.
Production query code must not use print() (per Review Focus 6: "Introduces side effects (logging, prints, stack traces)"). The try/except block at lines 141–143 catches all exceptions, prints to stdout, and re-raises — this adds no value and pollutes output. Additionally, the hasattr check at line 134 is unnecessary defensive coding for a well-known protobuf type.
Proposed simplified _make_request
def _make_request(self):
- try:
- if not self.transaction_id:
- raise ValueError(
- "Transaction ID must be set before making the request."
- )
-
- query_header = self._make_request_header()
- transaction_get_record = (
- transaction_get_record_pb2.TransactionGetRecordQuery()
- )
- transaction_get_record.header.CopyFrom(query_header)
-
- transaction_get_record.transactionID.CopyFrom(
- self.transaction_id._to_proto()
- )
- transaction_get_record.include_child_records = self.include_children
- query = query_pb2.Query()
- if not hasattr(query, "transactionGetRecord"):
- raise AttributeError(
- "Query object has no attribute 'transactionGetRecord'"
- )
- query.transactionGetRecord.CopyFrom(transaction_get_record)
-
- return query
- except Exception as e:
- print(f"Exception in _make_request: {e}")
- raise
+ if self.transaction_id is None:
+ raise ValueError("Transaction ID must be set before making the request.")
+
+ query_header = self._make_request_header()
+ transaction_get_record = transaction_get_record_pb2.TransactionGetRecordQuery()
+ transaction_get_record.header.CopyFrom(query_header)
+ transaction_get_record.transactionID.CopyFrom(self.transaction_id._to_proto())
+ transaction_get_record.include_child_records = self.include_children
+ transaction_get_record.includeDuplicates = self.include_duplicates
+
+ query = query_pb2.Query()
+ query.transactionGetRecord.CopyFrom(transaction_get_record)
+ return queryAs per coding guidelines, Review Focus 6: "Introduces side effects (logging, prints, stack traces)" should be flagged. Review Focus 3: "_make_request() MUST... Call _make_request_header() exactly once... Avoid manual QueryHeader mutation".
| new_pending_airdrops = cls._parse_pending_airdrops(proto) | ||
| call_result = cls._parse_contract_call_result(proto) | ||
|
|
||
| children: Optional[List["TransactionRecord"]] = None |
There was a problem hiding this comment.
Critical bug: children parameter is overwritten to None before use.
Line 202 declares children: Optional[List["TransactionRecord"]] = None, which shadows and discards the children parameter passed into _from_proto at line 139. This means the children field on the constructed TransactionRecord will always be None, completely breaking the include_children feature.
Remove this line so the children parameter flows through to the constructor:
- children: Optional[List["TransactionRecord"]] = None
return cls(
transaction_id=tx_id,
...
children=children,| query_pb2, | ||
| query_header_pb2, | ||
| ) | ||
| from hiero_sdk_python.hapi.services import query_header_pb2 |
There was a problem hiding this comment.
Remove duplicate import of query_header_pb2.
query_header_pb2 is already imported at line 15 within the block import (lines 8–16). This redefinition triggers Ruff F811.
-from hiero_sdk_python.hapi.services import query_header_pb2📝 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.
| from hiero_sdk_python.hapi.services import query_header_pb2 |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 17-17: Redefinition of unused query_header_pb2 from line 15: query_header_pb2 redefined here
Remove definition: query_header_pb2
(F811)
prajeeta15
left a comment
prajeeta15
left a comment
There was a problem hiding this comment.
@mukundkumarjha please address the recent coderabbitai issues
|
@mukundkumarjha this thread is extremely long, please can you help us to review this by resolving each conversation that is resolved, thank you |
6 participants
Description:
Related issue(s):
Fixes #1512
Notes for reviewer:
Checklist