feat: transaction record query include duplicates#1646
Conversation
Signed-off-by: Siddhartha Ganguly <gangulysiddhartha22@gmail.com>
Signed-off-by: gangulysiddhartha22-cmyk <gangulysiddhartha22@gmail.com>
gangulysiddhartha22-cmyk
changed the title
WalkthroughThis PR adds support for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Query as TransactionRecordQuery
participant Client
participant CryptoAPI as Crypto Service
participant Response as Proto Response
participant Mapper as _map_record_list()
participant TxRecord as TransactionRecord
User->>Query: set_transaction_id(tx_id)
User->>Query: set_include_duplicates(True)
User->>Client: execute(query)
Client->>Query: execute(client)
Query->>CryptoAPI: getTxRecordByTxID (with includeDuplicates=True)
CryptoAPI-->>Response: Returns record + duplicateTransactionRecords
Query->>Mapper: _map_record_list(duplicateTransactionRecords)
loop For each duplicate proto record
Mapper->>TxRecord: _from_proto(proto, transaction_id)
TxRecord-->>Mapper: TransactionRecord instance
end
Mapper-->>Query: List[TransactionRecord]
Query->>TxRecord: _from_proto(main_record, transaction_id, duplicates=[...])
TxRecord-->>Query: TransactionRecord with duplicates
Query-->>Client: TransactionRecord
Client-->>User: Result with duplicates list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 5
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/query/transaction_record_query.py (1)
19-27:⚠️ Potential issue | 🔴 CriticalConstructor references undefined
include_duplicates.
include_duplicatesisn’t a parameter, so__init__will raiseNameErroron instantiation. Add a parameter (defaultFalse) or set explicitly.🛠️ Proposed fix
- def __init__(self, transaction_id: Optional[TransactionId] = None): + def __init__( + self, + transaction_id: Optional[TransactionId] = None, + include_duplicates: bool = False, + ): """ Initializes the TransactionRecordQuery with the provided transaction ID. """ super().__init__() self.transaction_id : Optional[TransactionId] = transaction_id - self.include_duplicates: bool = bool(include_duplicates) + self.include_duplicates: bool = bool(include_duplicates)
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
903c0a0 to
7a9de74
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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)
113-141: 🧹 Nitpick | 🔵 TrivialUpdate docstring to document the
duplicatesparameter.The
_from_protomethod now accepts aduplicatesparameter, but the docstring's Args section doesn't document it. This could cause confusion for future maintainers.📝 Proposed docstring update
Args: proto (transaction_record_pb2.TransactionRecord): The raw protobuf transaction record containing all transaction data. transaction_id (Optional[TransactionId]): Optional transaction ID to associate with the record. If not provided, will be extracted from the protobuf message if available. + duplicates (Optional[list['TransactionRecord']]): Optional list of + duplicate transaction records to associate with this record. + Defaults to an empty list if not provided.tests/integration/transaction_record_query_e2e_test.py (1)
103-103:⚠️ Potential issue | 🟡 MinorMissing
@pytest.mark.integrationdecorator.The
test_transaction_record_query_can_execute_fungible_transfertest is missing the@pytest.mark.integrationmarker, which may cause it to run in unit test suites unexpectedly.🔧 Proposed fix
+@pytest.mark.integration def test_transaction_record_query_can_execute_fungible_transfer():
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
7a9de74 to
6a947f0
Compare
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)
tests/unit/transaction_record_test.py (1)
70-83: 🧹 Nitpick | 🔵 TrivialAdd assertion for
duplicatesin default initialization test.Per coding guidelines, tests should protect against breaking changes by asserting public attributes exist. Add an assertion for the new
duplicatesfield.♻️ Proposed addition
def test_transaction_record_default_initialization(): """Test the default initialization of the TransactionRecord class""" record = TransactionRecord() assert record.transaction_id is None assert record.transaction_hash is None assert record.transaction_memo is None assert record.transaction_fee is None assert record.receipt is None assert record.token_transfers == defaultdict(lambda: defaultdict(int)) assert record.nft_transfers == defaultdict(list[TokenNftTransfer]) assert record.transfers == defaultdict(int) assert record.new_pending_airdrops == [] assert record.prng_number is None assert record.prng_bytes is None + assert hasattr(record, 'duplicates'), "TransactionRecord should have duplicates attribute" + assert record.duplicates == []As per coding guidelines: "Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id'))."
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
6a947f0 to
b67b068
Compare
Signed-off-by: Siddhartha Ganguly <gangulysiddhartha22@gmail.com>
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
b67b068 to
4b9b909
Compare
|
@manishdait Ready for review. Thanks! |
manishdait
left a comment
manishdait
left a comment
There was a problem hiding this comment.
@gangulysiddhartha22-cmyk, thanks, I’ve added the requested changes.
Signed-off-by: Siddhartha Ganguly <gangulysiddhartha22@gmail.com>
|
@manishdait all your suggestions have been implemented. |
prajeeta15
left a comment
prajeeta15
left a comment
There was a problem hiding this comment.
this looks good to me . but I would ask @manishdait to review it too for confirmation and additional changes.
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
This LGTM!
Further feedback from @manishdait
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (68.57%) is below the target coverage (92.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #1646 +/- ##
==========================================
- Coverage 92.89% 92.79% -0.11%
==========================================
Files 140 140
Lines 8765 8781 +16
==========================================
+ Hits 8142 8148 +6
- Misses 623 633 +10 🚀 New features to boost your workflow:
|
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
c843904 to
181c26b
Compare
|
@manishdait Added unit tests. Ready for another look. Thanks! |
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
181c26b to
5142de6
Compare
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
5142de6 to
8310306
Compare
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
8310306 to
ea8a064
Compare
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
6808467 to
425fa27
Compare
|
Fixed the QueryHeader protobuf crash in the unit test. Requesting re-assessment Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/transaction_record_query_test.py (1)
118-125: 🧹 Nitpick | 🔵 TrivialRemove redundant try/except—let pytest handle exceptions natively.
The try/except with
pytest.fail()obscures the original traceback and provides less debugging information. If an unexpected exception occurs, pytest will fail the test with the full stack trace automatically.🔧 Suggested fix
- try: - # Get the cost of executing the query - should be 2 tinybars based on the mock response - cost = query.get_cost(client) - assert cost.to_tinybars() == 2 - - result = query.execute(client) - except Exception as e: - pytest.fail(f"Unexpected exception raised: {e}") + # Get the cost of executing the query - should be 2 tinybars based on the mock response + cost = query.get_cost(client) + assert cost.to_tinybars() == 2 + + result = query.execute(client)
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
425fa27 to
d8a8f93
Compare
AntonioCeppellini
left a comment
AntonioCeppellini
left a comment
There was a problem hiding this comment.
HI @gangulysiddhartha22-cmyk nice work so far
| """ | ||
|
|
||
| def __init__(self, transaction_id: Optional[TransactionId] = None): | ||
| def __init__(self, transaction_id: Optional[TransactionId] = None, include_duplicates: bool = False)->None: |
There was a problem hiding this comment.
False)->None: should become False) -> None:
| "include_duplicates must be a boolean (True or False)" | ||
| ) | ||
| self.include_duplicates = include_duplicates | ||
| return self |
There was a problem hiding this comment.
are you using balck for formatting?
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. Quick Fix for CHANGELOG.md ConflictsIf your conflict is only in CHANGELOG.md, you can resolve it easily using the GitHub web editor:
For all other merge conflicts, please read: Thank you for contributing! |
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
8da712f to
453af5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (30)
src/hiero_sdk_python/channels.py (1)
12-213: 🧹 Nitpick | 🔵 TrivialLGTM! Formatting improvements enhance readability.
The formatting changes (blank lines in docstrings and multi-line wrapping at lines 91-93 and 211-213) improve code readability without altering behavior.
Optional: Consider adding type annotations.
Static analysis flags missing return type annotations for
__init__and property methods. While these are pre-existing issues and outside this PR's scope, adding them would improve type safety.📝 Optional type annotation improvements
- def __init__(self, grpc_channel=None): + def __init__(self, grpc_channel=None) -> None:`@property` - def file(self): + def file(self) -> file_service_pb2_grpc.FileServiceStub | None:Similarly for other property methods (
smart_contract,topic,freeze,network,token,schedule,util,address_book).src/hiero_sdk_python/tokens/token_fee_schedule_update_transaction.py (1)
41-45:⚠️ Potential issue | 🟠 MajorPublic API return type change in
set_token_id.
Returningselfchanges the public method’s return type; please confirm this is intentional and document the change (or add deprecation guidance if needed) to preserve API stability expectations.As per coding guidelines, public token APIs must not change return types without explicit stability considerations.
src/hiero_sdk_python/query/account_info_query.py (1)
58-74: 🧹 Nitpick | 🔵 TrivialConsider simplifying exception handling.
The broad
try/except Exceptionblock catches all exceptions just to re-raise them. Since theValueErrorfor missingaccount_idis already explicit and there's no recovery logic, the try/except wrapper adds complexity without benefit.♻️ Suggested simplification
def _make_request(self): - try: - if not self.account_id: - raise ValueError("Account ID must be set before making the request.") - - query_header = self._make_request_header() - - crypto_info_query = crypto_get_info_pb2.CryptoGetInfoQuery() - crypto_info_query.header.CopyFrom(query_header) - crypto_info_query.accountID.CopyFrom(self.account_id._to_proto()) - - query = query_pb2.Query() - query.cryptoGetInfo.CopyFrom(crypto_info_query) - - return query - except Exception as e: - print(f"Exception in _make_request: {e}") - raise + if not self.account_id: + raise ValueError("Account ID must be set before making the request.") + + query_header = self._make_request_header() + + crypto_info_query = crypto_get_info_pb2.CryptoGetInfoQuery() + crypto_info_query.header.CopyFrom(query_header) + crypto_info_query.accountID.CopyFrom(self.account_id._to_proto()) + + query = query_pb2.Query() + query.cryptoGetInfo.CopyFrom(crypto_info_query) + + return querysrc/hiero_sdk_python/tokens/token_freeze_status.py (1)
34-39: 🧹 Nitpick | 🔵 TrivialNote: Custom
__eq__may break hashability contract.While this code isn't part of the current PR's changed lines, the custom
__eq__allowing integer comparison creates a subtle issue:TokenFreezeStatus.FROZEN == 1returnsTrue, buthash(TokenFreezeStatus.FROZEN) != hash(1). This violates Python's contract that equal objects must have equal hashes, which can cause unexpected behavior when enum values are used in sets or as dictionary keys alongside integers.If integer comparison is intentional, consider also overriding
__hash__to returnhash(self.value).♻️ Optional fix to preserve hashability contract
def __eq__(self, other: Any) -> bool: """Checks equality with another TokenFreezeStatus or an integer.""" if isinstance(other, TokenFreezeStatus): return self.value == other.value if isinstance(other, int): return self.value == other return False + + def __hash__(self) -> int: + """Returns hash based on value to maintain hashability contract.""" + return hash(self.value)src/hiero_sdk_python/consensus/topic_update_transaction.py (1)
245-301: 🧹 Nitpick | 🔵 TrivialReduce
_build_proto_bodycyclomatic complexity to satisfy Codacy.Codacy flags Line 245 for complexity > 12. Consider extracting helper builders (e.g.,
_build_custom_fees,_build_fee_exempt_key_list,_build_memo) to lower complexity and isolate optional-field handling.src/hiero_sdk_python/crypto/public_key.py (1)
52-71:⚠️ Potential issue | 🟡 MinorAvoid misleading ambiguity warnings for non‑Ed25519 inputs.
_warn_ed25519_ambiguity()runs before length checks, so ECDSA/DER inputs also warn even though they’re not ambiguous. Consider warning only when the decoded input is 32 bytes (the actual ambiguity case).💡 Suggested fix
- _warn_ed25519_ambiguity("PublicKey.from_bytes") - - # 1) 32-byte raw ⇒ Ed25519 - if len(pub) == 32: - return cls._from_bytes_ed25519(pub) + # 1) 32-byte raw ⇒ Ed25519 (ambiguous with Ed25519 seed) + if len(pub) == 32: + _warn_ed25519_ambiguity("PublicKey.from_bytes") + return cls._from_bytes_ed25519(pub) @@ - _warn_ed25519_ambiguity("PublicKey.from_string") - # 1) Remove the unecessary prefix and decode the hex hex_str = hex_str.removeprefix("0x") try: data = bytes.fromhex(hex_str) except ValueError as exc: raise ValueError( f"Invalid hex-encoded public key string: {hex_str!r}" ) from exc + if len(data) == 32: + _warn_ed25519_ambiguity("PublicKey.from_string")Also applies to: 276-315
src/hiero_sdk_python/file/file_append_transaction.py (1)
330-375:⚠️ Potential issue | 🔴 CriticalFix early return that stops multi-chunk execution.
Line 373 returns inside the loop, so only the first chunk is executed. Move the return after the loop to execute all chunks.🛠️ Proposed fix
- # Return the first response (as per JavaScript implementation) - return responses[0] if responses else None + # Return the first response (as per JavaScript implementation) + return responses[0] if responses else Nonesrc/hiero_sdk_python/executable.py (2)
199-292:⚠️ Potential issue | 🟡 MinorStabilize
requestIdacross a single execution attempt.
_get_request_id()is called multiple times, generating different IDs for the same attempt, which breaks log correlation across retries and delay logs. Consider generating it once and reusing it.🛠️ Proposed fix
@@ - logger = client.logger + logger = client.logger + request_id = self._get_request_id() @@ - logger.trace( + logger.trace( "Executing", - "requestId", - self._get_request_id(), + "requestId", + request_id, @@ - logger.trace("Executing gRPC call", "requestId", self._get_request_id()) + logger.trace("Executing gRPC call", "requestId", request_id) @@ - _delay_for_attempt( - self._get_request_id(), + _delay_for_attempt( + request_id, @@ - _delay_for_attempt( - self._get_request_id(), + _delay_for_attempt( + request_id, @@ - logger.error( + logger.error( "Exceeded maximum attempts for request", "requestId", - self._get_request_id(), + request_id,
381-385: 🧹 Nitpick | 🔵 TrivialCreate a dedicated
ExecutableMethodErrorexception for better error handling clarity.Raising generic
Exceptionis flagged by Ruff (TRY002/TRY003) and makes error handling less precise. Replace it with a custom exception that follows the existing pattern in your codebase.Add to
src/hiero_sdk_python/exceptions.py:class ExecutableMethodError(Exception): """Raised when no transaction or query method is available to execute."""Then update
src/hiero_sdk_python/executable.py:
- Import the exception:
from hiero_sdk_python.exceptions import MaxAttemptsError, ExecutableMethodError- Update the docstring to document
ExecutableMethodErrorinstead of genericException- Replace the generic raise with
raise ExecutableMethodError("No method to execute")src/hiero_sdk_python/tokens/token_kyc_status.py (1)
38-44:⚠️ Potential issue | 🟠 MajorDefine
__hash__when overriding__eq__to restore set/dict compatibility.Overriding
__eq__without defining__hash__violates Python's equality/hash contract, making instances unhashable. This breaks any code that stores these enums in sets or uses them as dictionary keys.Add a
__hash__method and returnNotImplementedinstead ofFalsefor incompatible types:Suggested fix
def __eq__(self, other: Any) -> bool: """Checks equality with another TokenKycStatus or an integer.""" if isinstance(other, TokenKycStatus): return self.value == other.value if isinstance(other, int): return self.value == other - return False + return NotImplemented + + def __hash__(self) -> int: + return hash(self.value)Note: This same issue exists in
TokenFreezeStatusandTokenPauseStatus.src/hiero_sdk_python/tokens/token_pause_status.py (1)
47-59:⚠️ Potential issue | 🟠 MajorCustom
__eq__breaks hashability for public API enumTokenPauseStatus instances are unhashable due to custom
__eq__without corresponding__hash__, preventing valid Python patterns like set operations or dict keys. This violates the hash contract (equal objects must hash equally) and breaks user expectations for a public token enum. Add__hash__returninghash(self.value)to align equality and hashability semantics.Note: TokenFreezeStatus has identical issue.
src/hiero_sdk_python/tokens/token_key_validation.py (1)
44-50:⚠️ Potential issue | 🟠 MajorAdd
__hash__to restore hashability when overriding__eq__The custom
__eq__method disables hashing (Python sets__hash__ = None), preventing enum members from being used in sets or as dict keys. This violates standard Enum behavior and creates a latent API stability issue for users who may attempt to use these enums in collections.Add a compatible
__hash__implementation:Fix
def __eq__(self, other: Any) -> bool: """Checks equality with another TokenKeyValidation or an integer.""" if isinstance(other, TokenKeyValidation): return self.value == other.value if isinstance(other, int): return self.value == other return False + + def __hash__(self) -> int: + return hash(self.value)Note: This issue also affects
TokenFreezeStatus,TokenPauseStatus, andTokenKycStatus—all should be updated consistently.src/hiero_sdk_python/tokens/token_airdrop_pending_id.py (1)
46-47:⚠️ Potential issue | 🟡 MinorFix grammatical error in validation message.
The error message uses awkward phrasing. "must be required" should be "must be provided".
✏️ Suggested fix
if (token_id is None) == (nft_id is None): - raise ValueError("Exactly one of 'token_id' or 'nft_id' must be required.") + raise ValueError("Exactly one of 'token_id' or 'nft_id' must be provided.")src/hiero_sdk_python/hbar.py (1)
177-185: 🧹 Nitpick | 🔵 TrivialPotential parsing inconsistency between regex and string split.
The regex
FROM_STRING_PATTERNvalidates the format butfrom_stringthen usesamount.split(" ")to extract parts. If the input has multiple spaces (e.g.,"10 ℏ"), the regex would reject it, but if validation is bypassed, split could produce unexpected results.Currently safe since the regex check precedes the split, but consider extracting values directly from the match groups for consistency:
♻️ Suggested improvement using match groups
def from_string(cls, amount: str, unit: HbarUnit = HbarUnit.HBAR) -> "Hbar": match = FROM_STRING_PATTERN.match(amount) if not match: raise ValueError(f"Invalid Hbar format: '{amount}'") - parts = amount.split(" ") - value = Decimal(parts[0]) - unit = HbarUnit.from_string(parts[1]) if len(parts) == 2 else unit + value = Decimal(match.group(1)) + unit_str = match.group(3) # Group 3 captures the unit symbol + unit = HbarUnit.from_string(unit_str) if unit_str else unit return cls(value, unit=unit)src/hiero_sdk_python/tokens/token_nft_allowance.py (1)
194-216: 🧹 Nitpick | 🔵 TrivialConsider extracting the duplicated
_from_proto_fieldhelper.This helper method is duplicated verbatim in
TokenAllowance,HbarAllowance, andTokenNftAllowance(seesrc/hiero_sdk_python/tokens/token_allowance.py:116-137andsrc/hiero_sdk_python/tokens/hbar_allowance.py:119-140). Consider extracting it to a shared utility module.src/hiero_sdk_python/query/topic_info_query.py (1)
91-109:⚠️ Potential issue | 🟡 MinorRemove debug print statements from
_make_request.The
print()andtraceback.print_exc()calls are debug artifacts that should not be in production code. The exception is already re-raised, so the caller can handle it appropriately.🧹 Proposed fix to remove debug output
try: if not self.topic_id: raise ValueError("Topic ID must be set before making the request.") query_header = self._make_request_header() topic_info_query = consensus_get_topic_info_pb2.ConsensusGetTopicInfoQuery() topic_info_query.header.CopyFrom(query_header) topic_info_query.topicID.CopyFrom(self.topic_id._to_proto()) query = query_pb2.Query() query.consensusGetTopicInfo.CopyFrom(topic_info_query) return query - except Exception as e: - print(f"Exception in _make_request: {e}") - traceback.print_exc() - raiseThe entire try/except block can be removed since the exceptions (ValueError for missing topic_id, or any protobuf errors) will naturally propagate to the caller.
src/hiero_sdk_python/tokens/token_update_transaction.py (1)
93-115:⚠️ Potential issue | 🟡 MinorDocstring param name doesn’t match the signature.
The docstring references
token_key_validation, but the parameter istoken_key_verification_mode. Please align the docstring to avoid confusion.✏️ Suggested docstring fix
- token_key_validation (TokenKeyValidation, optional): The validation mode for token keys. + token_key_verification_mode (TokenKeyValidation, optional): The validation mode for token keys.src/hiero_sdk_python/tokens/token_update_nfts_transaction.py (1)
106-119:⚠️ Potential issue | 🟠 MajorEnsure metadata field is only present when explicitly provided.
When
self.metadataisNone,BytesValue(value=None)produces an empty wrapper instead of leaving the field absent. This causes the metadata field to always be present in the protobuf message, breaking field optionality semantics. Match the correct pattern already used inTokenUpdateTransaction:Proposed fix
return TokenUpdateNftsTransactionBody( token=self.token_id._to_proto(), serial_numbers=self.serial_numbers, - metadata=BytesValue(value=self.metadata), + metadata=BytesValue(value=self.metadata) if self.metadata else None, )src/hiero_sdk_python/query/transaction_get_receipt_query.py (1)
147-178:⚠️ Potential issue | 🟠 MajorAvoid stdout/traceback in _make_request.
Printing exceptions introduces noisy side effects and can leak sensitive data. Let the exception propagate or use structured logging if needed.Proposed fix (remove print/traceback wrapper)
- try: - if not self.transaction_id: - raise ValueError( - "Transaction ID must be set before making the request." - ) - - query_header = query_header_pb2.QueryHeader() - query_header.responseType = query_header_pb2.ResponseType.ANSWER_ONLY - - transaction_get_receipt = ( - transaction_get_receipt_pb2.TransactionGetReceiptQuery() - ) - transaction_get_receipt.header.CopyFrom(query_header) - transaction_get_receipt.transactionID.CopyFrom( - self.transaction_id._to_proto() - ) - - transaction_get_receipt.include_child_receipts = self.include_children - transaction_get_receipt.includeDuplicates = self.include_duplicates - - query = query_pb2.Query() - if not hasattr(query, "transactionGetReceipt"): - raise AttributeError( - "Query object has no attribute 'transactionGetReceipt'" - ) - query.transactionGetReceipt.CopyFrom(transaction_get_receipt) - - return query - except Exception as e: - print(f"Exception in _make_request: {e}") - traceback.print_exc() - raise + if not self.transaction_id: + raise ValueError( + "Transaction ID must be set before making the request." + ) + + query_header = query_header_pb2.QueryHeader() + query_header.responseType = query_header_pb2.ResponseType.ANSWER_ONLY + + transaction_get_receipt = ( + transaction_get_receipt_pb2.TransactionGetReceiptQuery() + ) + transaction_get_receipt.header.CopyFrom(query_header) + transaction_get_receipt.transactionID.CopyFrom( + self.transaction_id._to_proto() + ) + + transaction_get_receipt.include_child_receipts = self.include_children + transaction_get_receipt.includeDuplicates = self.include_duplicates + + query = query_pb2.Query() + if not hasattr(query, "transactionGetReceipt"): + raise AttributeError( + "Query object has no attribute 'transactionGetReceipt'" + ) + query.transactionGetReceipt.CopyFrom(transaction_get_receipt) + + return querysrc/hiero_sdk_python/consensus/topic_message_submit_transaction.py (1)
26-52:⚠️ Potential issue | 🟠 MajorInitialize
custom_fee_limitsto avoid AttributeError.
add_custom_fee_limit()assumes the list exists, but it isn’t initialized in__init__.🛠️ Suggested fix
self._transaction_ids: List[TransactionId] = [] self._signing_keys: List["PrivateKey"] = [] + self.custom_fee_limits: List[CustomFeeLimit] = []src/hiero_sdk_python/account/account_create_transaction.py (1)
68-82:⚠️ Potential issue | 🟡 MinorType annotations mislead that Optional values are safe, but
_build_proto_bodycrashes if None is explicitly passed.
auto_renew_periodanddecline_staking_rewardare typed asOptional[Duration]andOptional[bool]respectively, but have non-None defaults (AUTO_RENEW_PERIODandFalse). However, if a caller explicitly passesNone, the code will fail:auto_renew_period.secondswill raiseAttributeErrorwhen accessed in_build_proto_body. The type hint conflicts with the implementation and should either be corrected (remove Optional) or the code should validate/normalize.src/hiero_sdk_python/consensus/topic_id.py (1)
36-40:⚠️ Potential issue | 🟠 MajorChecksum field changes
TopicIdequality semanticsAdding
checksumto the dataclass means equality now depends on checksum, which is a backward-incompatible behavioral change for comparisons and set/dict keys. If equality should remain based on shard/realm/num, mark the checksum field as non-comparable.Proposed fix
- checksum: str | None = field(default=None, init=False) + checksum: str | None = field(default=None, init=False, compare=False)src/hiero_sdk_python/tokens/token_id.py (1)
38-42:⚠️ Potential issue | 🟠 MajorChecksum inclusion changes
TokenIdequality behaviorWith
checksumas a dataclass field,TokenId(0,0,1)andTokenId.from_string("0.0.1-xxxx")are no longer equal, which is a backward-incompatible behavior change. If equality should remain based on shard/realm/num, mark checksum as non-comparable.Proposed fix
- checksum: str | None = field(default=None, init=False) + checksum: str | None = field(default=None, init=False, compare=False)As per coding guidelines, default values and behaviors of token APIs must remain stable.
src/hiero_sdk_python/tokens/custom_fee.py (1)
75-126:⚠️ Potential issue | 🟠 MajorPreserve backward compatibility for
_from_protobuf/_to_protobufRenaming these methods is a breaking change for external subclasses or callers. Consider adding deprecated aliases to keep the old names working while encouraging migration.
Proposed fix
+import warnings @@ `@staticmethod` def _from_proto( custom_fee: "CustomFeeProto", ) -> "CustomFee": # Changed from _from_protobuf @@ raise ValueError(f"Unrecognized fee case: {fee_case}") + + `@staticmethod` + def _from_protobuf(custom_fee: "CustomFeeProto") -> "CustomFee": + warnings.warn( + "_from_protobuf() is deprecated; use _from_proto()", + DeprecationWarning, + stacklevel=2, + ) + return CustomFee._from_proto(custom_fee) @@ `@abstractmethod` def _to_proto(self) -> "CustomFeeProto": # Changed from _to_protobuf """Converts this CustomFee to its protobuf representation. @@ ... + + def _to_protobuf(self) -> "CustomFeeProto": + warnings.warn( + "_to_protobuf() is deprecated; use _to_proto()", + DeprecationWarning, + stacklevel=2, + ) + return self._to_proto()As per coding guidelines, token APIs must not rename or remove public methods without deprecation.
src/hiero_sdk_python/transaction/transaction.py (1)
816-823: 🧹 Nitpick | 🔵 TrivialUse exception chaining for better error traceability.
When re-raising exceptions within an
exceptblock, useraise ... from eto preserve the original traceback and improve debugging.♻️ Proposed fix
except (ImportError, AttributeError) as e: - raise ValueError( + raise ValueError( f"Failed to import transaction class for type '{transaction_type}': {e}" - ) + ) from esrc/hiero_sdk_python/tokens/nft_id.py (1)
28-59: 🧹 Nitpick | 🔵 TrivialConsider removing redundant validation in
__post_init__.The
__init__method already validates thattoken_idandserial_numberare notNone(lines 34-37), so theNonechecks in__post_init__(lines 48-49) are redundant and will never trigger. The type checks (lines 50-57) are still valuable.♻️ Suggested simplification
def __post_init__(self) -> None: """Validate the NftId after initialization.""" - if self.token_id is None: - raise TypeError("token_id is required") if not isinstance(self.token_id, TokenId): raise TypeError( f"token_id must be of type TokenId, got {type(self.token_id)}" ) if not isinstance(self.serial_number, int): raise TypeError( f"serial_number must be an integer, got {type(self.serial_number)}" ) if self.serial_number < 0: raise ValueError("serial_number must be non-negative")src/hiero_sdk_python/query/token_nft_info_query.py (1)
74-77:⚠️ Potential issue | 🟡 MinorRemove debug print statements and traceback from production code.
Using
print()andtraceback.print_exc()in production code is not ideal. These should either be replaced with proper logging or removed entirely, as the exception is already being re-raised. This introduces side effects (console output) that are inconsistent with other query implementations.🛠️ Proposed fix
except Exception as e: - print(f"Exception in _make_request: {e}") - traceback.print_exc() raiseOr if logging is desired:
+import logging + +logger = logging.getLogger(__name__) + ... except Exception as e: - print(f"Exception in _make_request: {e}") - traceback.print_exc() + logger.exception("Exception in _make_request") raisesrc/hiero_sdk_python/tokens/token_transfer_list.py (1)
22-47:⚠️ Potential issue | 🟡 MinorFix typo in docstring.
Line 33 has a typo: "Thhe" should be "The".
📝 Proposed fix
""" Initializes a new TokenTransferList instance. Args: - token (TokenId): Thhe ID of the token being transferred. + token (TokenId): The ID of the token being transferred. transfers (optional, list[TokenTransfer]): A list of fungible token transfers.src/hiero_sdk_python/tokens/token_transfer.py (1)
58-88:⚠️ Potential issue | 🟡 MinorFix unclosed parenthesis in docstring.
Line 66 has an unclosed parenthesis in the Args section:
proto (basic_types_pb2.TokenTransferList:should beproto (basic_types_pb2.TokenTransferList):.📝 Proposed fix
""" Construct a list of TokenTransfer from the protobuf of TokenTransferList. Args: - proto (basic_types_pb2.TokenTransferList: + proto (basic_types_pb2.TokenTransferList): The protobuf representation of a TokenTransferList """src/hiero_sdk_python/client/client.py (1)
149-164: 🧹 Nitpick | 🔵 TrivialConsider merging
endswithcalls for cleaner code.The static analyzer suggests combining the two
endswithchecks into a single call with a tuple.♻️ Suggested improvement
mirror_address = self.network.get_mirror_address() - if mirror_address.endswith(":50212") or mirror_address.endswith(":443"): + if mirror_address.endswith((":50212", ":443")): self.mirror_channel = grpc.secure_channel( mirror_address, grpc.ssl_channel_credentials() )
| cryptoAllowances=[ | ||
| allowance._to_proto() for allowance in self.hbar_allowances | ||
| ], | ||
| tokenAllowances=[ | ||
| allowance._to_proto() for allowance in self.token_allowances | ||
| ], | ||
| nftAllowances=[allowance._to_proto() for allowance in self.nft_allowances], |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent formatting across similar list comprehensions.
Lines 354-359 format cryptoAllowances and tokenAllowances as multi-line, but line 360 keeps nftAllowances as single-line. For consistency, consider applying the same multi-line formatting to all three list comprehensions.
✨ Proposed consistent formatting
body = CryptoApproveAllowanceTransactionBody(
cryptoAllowances=[
allowance._to_proto() for allowance in self.hbar_allowances
],
tokenAllowances=[
allowance._to_proto() for allowance in self.token_allowances
],
- nftAllowances=[allowance._to_proto() for allowance in self.nft_allowances],
+ nftAllowances=[
+ allowance._to_proto() for allowance in self.nft_allowances
+ ],
)| """ | ||
| Returns a human-friendly string representation of the account balance. | ||
|
|
There was a problem hiding this comment.
These docstring formatting changes are unrelated to the PR objectives.
The PR objectives focus on adding include_duplicates support to TransactionRecordQuery, but this file modifies only blank lines in docstrings for AccountBalance.__str__ and AccountBalance.__repr__. Including unrelated formatting changes increases review noise and makes it harder to understand the PR's scope.
Please consider removing these changes and keeping the PR focused on the transaction record duplicate functionality.
Also applies to: 73-73
| staked_account_id (Optional[AccountId]): The account to which this account is staked. | ||
| staked_node_id (Optional[int]): The node to which this account is staked. | ||
| decline_staking_reward (bool): Whether this account declines receiving staking rewards. | ||
| decline_staking_reward (bool): Whether this account declines receiving staking rewards. |
There was a problem hiding this comment.
Minor docstring inconsistency with field type.
The docstring on line 43 declares decline_staking_reward as bool, but the actual field declaration on line 61 is Optional[bool]. The docstring should match the field type.
📝 Suggested fix
- decline_staking_reward (bool): Whether this account declines receiving staking rewards.
+ decline_staking_reward (Optional[bool]): Whether this account declines receiving staking rewards.Also applies to: 61-61
| chunk_valid_start = timestamp_pb2.Timestamp( | ||
| seconds=base_timestamp.seconds, | ||
| nanos=base_timestamp.nanos + i | ||
| seconds=base_timestamp.seconds, nanos=base_timestamp.nanos + i | ||
| ) | ||
| chunk_transaction_id = TransactionId( | ||
| account_id=self.transaction_id.account_id, | ||
| valid_start=chunk_valid_start | ||
| valid_start=chunk_valid_start, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src && find . -name "topic_message_submit_transaction.py" -type f | head -5Repository: hiero-ledger/hiero-sdk-python
Length of output: 137
🏁 Script executed:
cd src && cat -n hiero_sdk_python/consensus/topic_message_submit_transaction.py | sed -n '250,290p'Repository: hiero-ledger/hiero-sdk-python
Length of output: 1849
🏁 Script executed:
rg "Timestamp" src/hiero_sdk_python/consensus/topic_message_submit_transaction.py -B 2 -A 2 | head -30Repository: hiero-ledger/hiero-sdk-python
Length of output: 331
🏁 Script executed:
rg "timestamp_pb2|from google.protobuf|import.*timestamp" src/hiero_sdk_python/ -A 1 | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 1558
🏁 Script executed:
rg "def get_required_chunks" src/hiero_sdk_python/ -A 5Repository: hiero-ledger/hiero-sdk-python
Length of output: 1171
🏁 Script executed:
rg "get_required_chunks" src/hiero_sdk_python/consensus/topic_message_submit_transaction.py -A 10Repository: hiero-ledger/hiero-sdk-python
Length of output: 2748
🏁 Script executed:
rg "1_?000_?000_?000|1e9|nanos.*%" src/hiero_sdk_python/ --type pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 747
🏁 Script executed:
cat -n src/hiero_sdk_python/transaction/transaction_id.py | sed -n '1,100p'Repository: hiero-ledger/hiero-sdk-python
Length of output: 4658
🏁 Script executed:
cat -n src/hiero_sdk_python/timestamp.py | head -50Repository: hiero-ledger/hiero-sdk-python
Length of output: 1884
🌐 Web query:
protobuf Timestamp nanos field constraints 0 999999999
💡 Result:
In google.protobuf.Timestamp, the nanos field is constrained to 0 through 999,999,999 inclusive (nanosecond fraction of the second). Negative seconds values are allowed, but if there is a fractional part it must still be represented with a non-negative nanos that counts forward in time. [1], [2], [3]
(Contrast: google.protobuf.Duration allows nanos to be -999,999,999 through +999,999,999 with sign rules.) [4]
Normalize chunk timestamp nanos to stay within protobuf bounds.
Adding i to nanos can exceed 999,999,999, producing an invalid Timestamp. The protobuf Timestamp.nanos field must remain in the range [0, 999,999,999].
🛠️ Suggested fix (carry nanos into seconds)
- chunk_valid_start = timestamp_pb2.Timestamp(
- seconds=base_timestamp.seconds, nanos=base_timestamp.nanos + i
- )
+ nanos = base_timestamp.nanos + i
+ seconds = base_timestamp.seconds + nanos // 1_000_000_000
+ chunk_valid_start = timestamp_pb2.Timestamp(
+ seconds=seconds, nanos=nanos % 1_000_000_000
+ )| def set_auto_renew_period( | ||
| self, seconds: Union[Duration, int] | ||
| ) -> "TopicUpdateTransaction": |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for declared Python version requirements in common configuration files
echo "=== Checking setup files ==="
for file in pyproject.toml setup.py setup.cfg .python-version requirements.txt poetry.lock; do
if [ -f "$file" ]; then
echo "Found: $file"
case "$file" in
pyproject.toml)
grep -n "python" "$file" | head -20
;;
setup.py)
grep -n "python_requires" "$file"
;;
setup.cfg)
grep -n "python_requires" "$file"
;;
.python-version)
cat "$file"
;;
esac
echo ""
fi
done
# Also check for any Ruff configuration
if [ -f "pyproject.toml" ] || [ -f "ruff.toml" ] || [ -f ".ruff.toml" ]; then
echo "=== Ruff configuration ==="
rg -A 5 "tool.ruff|target-version" 2>/dev/null || echo "No ruff config found"
fiRepository: hiero-ledger/hiero-sdk-python
Length of output: 876
🏁 Script executed:
#!/bin/bash
# Check the imports and the specific lines 126-128
echo "=== Checking imports at top of file ==="
head -30 src/hiero_sdk_python/consensus/topic_update_transaction.py | grep -n "Union\|from typing"
echo ""
echo "=== Checking lines 120-135 ==="
sed -n '120,135p' src/hiero_sdk_python/consensus/topic_update_transaction.py | cat -nRepository: hiero-ledger/hiero-sdk-python
Length of output: 735
Use PEP 604 union syntax to align with project standards.
The project targets Python ≥3.10 and Ruff is configured to flag Union usage (UP007). Replace Union[Duration, int] with the modern Duration | int syntax.
Change
-from typing import Union, Optional, List
+from typing import Optional, List
@@
- def set_auto_renew_period(
- self, seconds: Union[Duration, int]
- ) -> "TopicUpdateTransaction":
+ def set_auto_renew_period(
+ self, seconds: Duration | int
+ ) -> "TopicUpdateTransaction":🧰 Tools
🪛 Ruff (0.14.14)
[warning] 127-127: Use X | Y for type annotations
Convert to X | Y
(UP007)
| • PAUSE_NOT_APPLICABLE – pause not relevant | ||
| • PAUSED – token is paused | ||
| • UNPAUSED – token is active |
There was a problem hiding this comment.
Replace ambiguous EN DASH characters in docstring
Ruff flags the – characters as ambiguous (RUF002). Replace with - to silence the lint and avoid encoding ambiguity.
🧹 Suggested cleanup
- • PAUSE_NOT_APPLICABLE – pause not relevant
- • PAUSED – token is paused
- • UNPAUSED – token is active
+ • PAUSE_NOT_APPLICABLE - pause not relevant
+ • PAUSED - token is paused
+ • UNPAUSED - token is active📝 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.
| • PAUSE_NOT_APPLICABLE – pause not relevant | |
| • PAUSED – token is paused | |
| • UNPAUSED – token is active | |
| • PAUSE_NOT_APPLICABLE - pause not relevant | |
| • PAUSED - token is paused | |
| • UNPAUSED - token is active |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 19-19: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
[warning] 20-20: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
[warning] 21-21: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
| from typing import List, Optional | ||
| from google.protobuf.wrappers_pb2 import BytesValue | ||
|
|
||
| from hiero_sdk_python.tokens.token_id import TokenId | ||
| from hiero_sdk_python.transaction.transaction import Transaction | ||
| from hiero_sdk_python.channels import _Channel | ||
| from hiero_sdk_python.executable import _Method | ||
| from hiero_sdk_python.hapi.services.token_update_nfts_pb2 import TokenUpdateNftsTransactionBody | ||
| from hiero_sdk_python.hapi.services.token_update_nfts_pb2 import ( | ||
| TokenUpdateNftsTransactionBody, | ||
| ) | ||
| from hiero_sdk_python.hapi.services import transaction_pb2 | ||
| from hiero_sdk_python.hapi.services.schedulable_transaction_body_pb2 import ( | ||
| SchedulableTransactionBody, | ||
| ) | ||
| from google.protobuf.wrappers_pb2 import BytesValue | ||
| from hiero_sdk_python.hapi.services import token_update_nfts_pb2 |
There was a problem hiding this comment.
Remove duplicate BytesValue import to avoid lint failure.
The symbol is imported twice, which triggers redefinition errors in linters.
🧹 Proposed fix
from hiero_sdk_python.hapi.services.schedulable_transaction_body_pb2 import (
SchedulableTransactionBody,
)
-from google.protobuf.wrappers_pb2 import BytesValue
from hiero_sdk_python.hapi.services import token_update_nfts_pb2🧰 Tools
🪛 Ruff (0.14.14)
[warning] 9-9: typing.List is deprecated, use list instead
(UP035)
[error] 23-23: Redefinition of unused BytesValue from line 10: BytesValue redefined here
Remove definition: BytesValue
(F811)
| raise ValueError( | ||
| f"Expected TransactionResponseProto but got {type(response)}" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using TypeError for type validation consistency.
The static analyzer suggests using TypeError here since you're validating the type of the response. This would be consistent with the TypeError you raise on line 110 for proto_request type validation.
♻️ Suggested fix
if not isinstance(response, TransactionResponseProto):
- raise ValueError(
+ raise TypeError(
f"Expected TransactionResponseProto but got {type(response)}"
)🧰 Tools
🪛 Ruff (0.14.14)
[warning] 136-138: Prefer TypeError exception for invalid type
(TRY004)
[warning] 136-138: Avoid specifying long messages outside the exception class
(TRY003)
| def add_hbar_transfer( | ||
| self, account_id: AccountId, amount: Union[int, Hbar] | ||
| ) -> "TransferTransaction": |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Check for Python version requirements in project config files
find . -maxdepth 2 -type f \( -name "pyproject.toml" -o -name "setup.cfg" -o -name "setup.py" \) | head -5Repository: hiero-ledger/hiero-sdk-python
Length of output: 89
🏁 Script executed:
cat ./pyproject.tomlRepository: hiero-ledger/hiero-sdk-python
Length of output: 2580
🏁 Script executed:
cat -n src/hiero_sdk_python/transaction/transfer_transaction.py | head -150Repository: hiero-ledger/hiero-sdk-python
Length of output: 6764
🏁 Script executed:
# Search for PEP 604 pipe syntax usage in the codebase
rg '\w+\s*\|\s*\w+' src/ --type py | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 2524
🏁 Script executed:
# Search for Union usage in the codebase
rg 'Union\[' src/ --type py | head -30Repository: hiero-ledger/hiero-sdk-python
Length of output: 3538
Use PEP 604 union syntax for consistency with project standards.
The project targets Python 3.10+ and has Ruff's UP rule enabled. Replace Union[int, Hbar] with int | Hbar on lines 103 and 119 (as well as line 69 in the internal _add_hbar_transfer method) to align with PEP 604 syntax already used elsewhere in the codebase.
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 103-103: Use X | Y for type annotations
Convert to X | Y
(UP007)
| if hasattr(key, "_to_proto"): | ||
| key = key._to_proto() # Handle PublicKey objects (convert to proto first) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM!
The hasattr check correctly identifies PublicKey-like objects and converts them to protobuf before calling HasField(). This aligns with the _to_proto() pattern used throughout the codebase (e.g., PublicKey._to_proto(), TransactionId._to_proto()).
Consider updating the type annotation and docstring to reflect that this function accepts both Key (protobuf) and PublicKey objects:
📝 Optional: Improve type annotation
-def format_key(key: Key) -> str:
+def format_key(key) -> str:
"""
Converts a protobuf Key into a nicely formatted string:
- If key is None, return "None"
- If ed25519, show "ed25519(hex-encoded)"
- If thresholdKey, keyList, or something else, show a short label.
+
+ Args:
+ key: A protobuf Key or a PublicKey object with a _to_proto() method.
"""
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
453af5e to
fe923cf
Compare
…e-duplicates Signed-off-by: Siddhartha Ganguly <gangulysiddhartha22@gmail.com>
gangulysiddhartha22-cmyk
force-pushed
the
feature/1635-transaction-record-query-include-duplicates
branch
from
fe923cf to
ad6885b
Compare
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 (24)
src/hiero_sdk_python/utils/entity_id_helper.py (2)
17-33:⚠️ Potential issue | 🟡 MinorReturn type mismatch and inaccurate docstring.
Several issues with this function:
- Docstring error: Mentions "An instance of cls" but there's no
clsparameter - appears to be copy-paste from a class method.- Missing type annotation: No return type specified.
- Returns strings instead of integers: The regex groups return strings, but callers using
validate_checksumexpect integers forshard,realm, andnum(seeAccountId.validate_checksum,ContractId.validate_checksum, etc. in the relevant snippets).🔧 Proposed fix
-def parse_from_string(address: str): +def parse_from_string(address: str) -> tuple[int, int, int, Optional[str]]: """ 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 shard/realm/num are integers + and checksum is an optional string. """ match = ID_REGEX.match(address) if not match: raise ValueError("Invalid format for entity ID") shard, realm, num, checksum = match.groups() - return shard, realm, num, checksum + return int(shard), int(realm), int(num), checksum
153-154: 🧹 Nitpick | 🔵 TrivialMinor: Include exception details in timeout error message for consistency.
The timeout error message omits the exception
e, unlike the other exception handlers. While timeout messages are typically self-explanatory, includingemaintains consistency and may provide additional context.♻️ Suggested fix
except requests.exceptions.Timeout as e: - raise RuntimeError(f"Mirror node request timed out for {url}") from e + raise RuntimeError(f"Mirror node request timed out for {url}: {e}") from esrc/hiero_sdk_python/hbar.py (1)
34-55:⚠️ Potential issue | 🟡 MinorReject non‑finite
Decimalvalues as well.
Decimal('NaN')/Decimal('Infinity')bypass the current finiteness check and can later break conversion. Add aDecimal.is_finite()guard to match the float path.🛠️ Suggested fix
if isinstance(amount, float) and not math.isfinite(amount): raise ValueError("Hbar amount must be finite") + if isinstance(amount, Decimal) and not amount.is_finite(): + raise ValueError("Hbar amount must be finite")src/hiero_sdk_python/tokens/token_freeze_status.py (1)
34-39: 🧹 Nitpick | 🔵 TrivialCustom
__eq__breaks hash invariant - consider adding__hash__.Overriding
__eq__to allow integer comparison (status == 1) breaks Python's invariant thata == bimplieshash(a) == hash(b). This can cause subtle bugs when enums are used as dictionary keys or in sets alongside integers.If integer comparison is intentional, explicitly define
__hash__to restore hashability:♻️ Proposed fix
def __eq__(self, other: Any) -> bool: """Checks equality with another TokenFreezeStatus or an integer.""" if isinstance(other, TokenFreezeStatus): return self.value == other.value if isinstance(other, int): return self.value == other return False + + def __hash__(self) -> int: + """Returns hash based on enum value.""" + return hash(self.value)src/hiero_sdk_python/tokens/token_pause_status.py (1)
47-59: 🧹 Nitpick | 🔵 TrivialCustom
__eq__breaks hash invariant - consider adding__hash__.Same issue as in
TokenFreezeStatus: overriding__eq__to compare with integers breaks the hash contract. Consider adding an explicit__hash__method.♻️ Proposed fix
if isinstance(other, int): return self.value == other return False + + def __hash__(self) -> int: + """Returns hash based on enum value.""" + return hash(self.value)src/hiero_sdk_python/tokens/token_kyc_status.py (1)
38-44: 🧹 Nitpick | 🔵 TrivialCustom
__eq__breaks hash invariant - consider adding__hash__.Same issue as the other status enums: overriding
__eq__to compare with integers breaks the hash contract. Consider adding an explicit__hash__method for consistency.♻️ Proposed fix
if isinstance(other, int): return self.value == other return False + + def __hash__(self) -> int: + """Returns hash based on enum value.""" + return hash(self.value)src/hiero_sdk_python/tokens/token_key_validation.py (1)
44-50:⚠️ Potential issue | 🟠 MajorOverriding
Enum.__eq__breaks hashability—a breaking change for public token APIs.Custom
__eq__makesTokenKeyValidationunhashable, preventing use as dict keys or set members. This breaks the enum contract and may violate existing user code. Per REVIEW FOCUS 1 (API Stability): "Token APIs are public and user-facing. Default values and behaviors MUST remain stable."Choose one of:
- Remove
__eq__and add__int__: Let standard enum equality work; offer explicit int conversion where needed.- Keep
__eq__but restore__hash__: Explicitly restore hashability with__hash__ = Enum.__hash__and returnNotImplementedfor unsupported types.Option 1: Replace with explicit `__int__`
- def __eq__(self, other: Any) -> bool: - """Checks equality with another TokenKeyValidation or an integer.""" - if isinstance(other, TokenKeyValidation): - return self.value == other.value - if isinstance(other, int): - return self.value == other - return False + def __int__(self) -> int: + """Returns the integer value for explicit comparisons.""" + return int(self.value)Option 2: Restore hashability if int comparison is essential
def __eq__(self, other: Any) -> bool: """Checks equality with another TokenKeyValidation or an integer.""" if isinstance(other, TokenKeyValidation): return self.value == other.value if isinstance(other, int): return self.value == other - return False + return NotImplemented + + __hash__ = Enum.__hash__src/hiero_sdk_python/tokens/custom_fee.py (1)
75-126:⚠️ Potential issue | 🟠 MajorPreserve backward compatibility for _to_protobuf/_from_protobuf.
Renaming these hooks can break downstream subclasses or callers relying on the previous method names. Please keep deprecated wrappers to maintain API stability.🔧 Suggested compatibility shim
`@staticmethod` def _from_proto( custom_fee: "CustomFeeProto", ) -> "CustomFee": # Changed from _from_protobuf @@ raise ValueError(f"Unrecognized fee case: {fee_case}") + + `@staticmethod` + def _from_protobuf(custom_fee: "CustomFeeProto") -> "CustomFee": + import warnings + warnings.warn( + "CustomFee._from_protobuf is deprecated; use _from_proto instead.", + DeprecationWarning, + stacklevel=2, + ) + return CustomFee._from_proto(custom_fee) @@ `@abstractmethod` def _to_proto(self) -> "CustomFeeProto": # Changed from _to_protobuf @@ ... + + def _to_protobuf(self) -> "CustomFeeProto": + import warnings + warnings.warn( + "CustomFee._to_protobuf is deprecated; use _to_proto instead.", + DeprecationWarning, + stacklevel=2, + ) + return self._to_proto()As per coding guidelines: Token APIs are public and user-facing; method names and signatures must remain stable.
src/hiero_sdk_python/account/account_info.py (1)
142-173:⚠️ Potential issue | 🟡 MinorPreserve node id 0 when serializing staking_info.
Line 170 uses a truthiness check, so a valid
staked_node_id=0is dropped. Use an explicitis not Noneguard.✅ Suggested fix
- staked_node_id=self.staked_node_id if self.staked_node_id else None, + staked_node_id=( + self.staked_node_id + if self.staked_node_id is not None + else None + ),src/hiero_sdk_python/tokens/token_airdrop_pending_id.py (1)
46-47: 🧹 Nitpick | 🔵 TrivialValidation aligns with protobuf
oneofsemantics, but error message has a grammatical issue.The XOR logic
(token_id is None) == (nft_id is None)correctly enforces mutual exclusivity matching the protobufoneofstructure. However, the error message "must be required" is grammatically awkward.📝 Suggested message fix
if (token_id is None) == (nft_id is None): - raise ValueError("Exactly one of 'token_id' or 'nft_id' must be required.") + raise ValueError("Exactly one of 'token_id' or 'nft_id' must be provided.")src/hiero_sdk_python/file/file_append_transaction.py (2)
299-313:⚠️ Potential issue | 🟠 MajorNormalize nanoseconds when offsetting timestamps.
Line 307 adds
idirectly tonanos. Ifbase_timestamp.nanosis near 1e9, this produces an invalid protobuf Timestamp and can break multi‑chunk submissions.Suggested fix
- chunk_valid_start = timestamp_pb2.Timestamp( - seconds=base_timestamp.seconds, nanos=base_timestamp.nanos + i - ) + total_nanos = base_timestamp.nanos + i + extra_seconds, nanos = divmod(total_nanos, 1_000_000_000) + chunk_valid_start = timestamp_pb2.Timestamp( + seconds=base_timestamp.seconds + extra_seconds, nanos=nanos + )
51-81:⚠️ Potential issue | 🟡 MinorRemove quoted forward reference on
_signing_keys.The file has
from __future__ import annotationsenabled, and Ruff's UP037 rule is configured (as part of the "UP" ruleset in pyproject.toml). Line 79 uses a quoted forward referenceList["PrivateKey"]which will trigger UP037. Remove the quotes to useList[PrivateKey]directly.Note: The missing
-> Nonereturn annotation on__init__is not enforced by the current Ruff configuration (ANN204 is not selected) and does not need to be added.Suggested fix
- self._signing_keys: List["PrivateKey"] = ( - [] - ) # Use string annotation to avoid import issues + self._signing_keys: List[PrivateKey] = []src/hiero_sdk_python/consensus/topic_message_submit_transaction.py (1)
18-52:⚠️ Potential issue | 🟠 MajorValidate constructor inputs for
chunk_size/max_chunks.
Negative values bypass setter validation and can yield invalid chunk counts (including no-op execution). Align constructor behavior with the setters.🛠️ Proposed validation
- self.chunk_size: int = chunk_size or 1024 - self.max_chunks: int = max_chunks or 20 + if chunk_size is not None and chunk_size <= 0: + raise ValueError("chunk_size must be positive") + if max_chunks is not None and max_chunks <= 0: + raise ValueError("max_chunks must be positive") + self.chunk_size = chunk_size or 1024 + self.max_chunks = max_chunks or 20src/hiero_sdk_python/consensus/topic_message.py (1)
150-180: 🧹 Nitpick | 🔵 TrivialMake
chunking_enabledkeyword-only to avoid positional-bool ambiguity.This internal method is unused in the codebase, making the refactoring safe with no call sites to update. The change aligns with boolean-arg best practices and prevents accidental misuse.
♻️ Proposed signature change
- def _from_proto( - cls, - response_or_responses: Union[ - mirror_proto.ConsensusTopicResponse, - List[mirror_proto.ConsensusTopicResponse], - ], - chunking_enabled: bool = False, - ) -> "TopicMessage": + def _from_proto( + cls, + response_or_responses: Union[ + mirror_proto.ConsensusTopicResponse, + List[mirror_proto.ConsensusTopicResponse], + ], + *, + chunking_enabled: bool = False, + ) -> "TopicMessage":src/hiero_sdk_python/tokens/fee_assessment_method.py (1)
33-34:⚠️ Potential issue | 🔴 CriticalCRITICAL: FeeAssessmentMethod enum values are semantically inverted relative to Hedera protobuf.
The enum currently defines
INCLUSIVE = 0andEXCLUSIVE = 1, but this is backwards. According to Hedera's protobuf definition, thenet_of_transfersfield is a boolean where:
true(1) means fee is assessed net of transfers → INCLUSIVEfalse(0) means fee is assessed not net of transfers → EXCLUSIVEThis inversion causes a critical semantic bug: developers setting
assessment_method=FeeAssessmentMethod.INCLUSIVEwill have their fees assessed asEXCLUSIVEon Hedera, and vice versa.Required fix: Swap the enum values:
INCLUSIVE = 1EXCLUSIVE = 0This must be treated as a breaking change requiring deprecation warnings and migration guidance, as existing code relying on the inverted semantics will need updates.
src/hiero_sdk_python/query/topic_info_query.py (1)
106-109:⚠️ Potential issue | 🟡 MinorRemove debug print statements from production code.
The
print()andtraceback.print_exc()calls introduce side effects that should not be in production code. These appear to be debug artifacts. Per coding guidelines, side effects like logging and prints should be flagged if they could affect execution flow or user experience.Proposed fix — remove debug statements
except Exception as e: - print(f"Exception in _make_request: {e}") - traceback.print_exc() raiseIf logging is needed, consider using the standard
loggingmodule with appropriate log levels instead.src/hiero_sdk_python/client/network.py (1)
19-71: 🧹 Nitpick | 🔵 TrivialConsider annotating mutable class attributes with
ClassVar.The static analysis correctly identifies that mutable class attributes (
MIRROR_ADDRESS_DEFAULT,MIRROR_NODE_URLS,DEFAULT_NODES,LEDGER_ID) should be annotated withtyping.ClassVarto indicate they are class-level and not instance-level attributes. This is a recommended practice but not blocking.♻️ Proposed fix
+from typing import ClassVar, Dict, List, Optional, Any, Tuple -from typing import Dict, List, Optional, Any, Tuple class Network: # Mirror node gRPC addresses (always use TLS, port 443 for HTTPS) - MIRROR_ADDRESS_DEFAULT: Dict[str, str] = { + MIRROR_ADDRESS_DEFAULT: ClassVar[Dict[str, str]] = { "mainnet": "mainnet.mirrornode.hedera.com:443", ... }src/hiero_sdk_python/tokens/token_create_transaction.py (1)
115-125:⚠️ Potential issue | 🔴 CriticalBug: Instance method called as static method.
The
_validate_token_freeze_statusmethod is defined without the@staticmethoddecorator (unlike_validate_token_paramsat line 104), making it an instance method. However, at line 517 it's called asTokenCreateValidator._validate_token_freeze_status(self._keys, self._token_params)which is a class-level call without instantiation.This will cause a
TypeErrorat runtime because theselfparameter won't be bound.🐛 Proposed fix - Add `@staticmethod` decorator
+ `@staticmethod` def _validate_token_freeze_status( - keys: TokenKeys, token_params: TokenParams + keys: TokenKeys, token_params: TokenParams ) -> None: """Ensure account is not frozen for this token."""src/hiero_sdk_python/query/transaction_get_receipt_query.py (1)
175-178:⚠️ Potential issue | 🟡 MinorRemove debug print statements from production code.
The
print()andtraceback.print_exc()calls are debug artifacts that should not be in production code. They bypass the SDK's logging infrastructure and clutter stdout. If error context is needed, use the SDK'sLoggeror let the exception propagate naturally with its traceback.🧹 Suggested fix
- except Exception as e: - print(f"Exception in _make_request: {e}") - traceback.print_exc() - raise + except Exception: + raiseOr remove the try/except entirely since it just re-raises:
- try: if not self.transaction_id: raise ValueError( "Transaction ID must be set before making the request." ) # ... rest of method ... return query - except Exception as e: - print(f"Exception in _make_request: {e}") - traceback.print_exc() - raisesrc/hiero_sdk_python/__init__.py (1)
215-215:⚠️ Potential issue | 🟡 MinorDuplicate
ResponseCodeentry in__all__.
ResponseCodeappears twice in the exports list — at line 215 (under "Response") and line 240 (under "HBAR"). This redundancy is harmless but should be cleaned up.🧹 Suggested fix
Remove the duplicate on line 240:
# HBAR "Hbar", "HbarUnit", - "ResponseCode", "Timestamp", "Duration", "StakingInfo",Also applies to: 240-240
src/hiero_sdk_python/tokens/token_nft_info.py (1)
1-14: 🧹 Nitpick | 🔵 TrivialModule docstring should precede all imports.
The module docstring (lines 3-9) is placed after the
dataclassimport on line 1. Per Python conventions, module-level docstrings should be the first statement in the file.🧹 Suggested fix
+""" +hiero_sdk_python.tokens.token_nft_info.py +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Defines TokenNftInfo, a class representing Non-Fungible Token details (ID, owner, +creation time, metadata, spender) on the Hedera network, with protobuf conversion. +""" from dataclasses import dataclass - -""" -hiero_sdk_python.tokens.token_nft_info.py -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -Defines TokenNftInfo, a class representing Non-Fungible Token details (ID, owner, -creation time, metadata, spender) on the Hedera network, with protobuf conversion. -""" from typing import Optionalsrc/hiero_sdk_python/tokens/token_id.py (1)
41-42:⚠️ Potential issue | 🔴 CriticalTokenId checksum must be excluded from equality comparison.
Adding
checksumto the dataclass withoutcompare=Falsecreates a Python hash/equality contract violation. TokenId defines a custom__hash__based only on (shard, realm, num), but the generated__eq__would include checksum. This means two TokenIds with identical shard/realm/num but different checksums would have the same hash yet not be equal—breaking dict/set behavior and violating Python's equality contract.Since checksums are validation metadata (not part of identity),
compare=Falseis the correct fix and aligns with the semantic intent of token identity.Proposed fix
- checksum: str | None = field(default=None, init=False) + checksum: str | None = field(default=None, init=False, compare=False)src/hiero_sdk_python/query/token_nft_info_query.py (1)
60-77:⚠️ Potential issue | 🟡 MinorRemove print/traceback side effects from
_make_request.Query construction shouldn’t emit console output or stack traces; it changes execution side effects and can leak details. Let the exception propagate naturally.
🔧 Suggested change
- try: - if not self.nft_id: - raise ValueError("NFT ID must be set before making the request.") - - query_header = self._make_request_header() - - nft_info_query = token_get_nft_info_pb2.TokenGetNftInfoQuery() - nft_info_query.header.CopyFrom(query_header) - nft_info_query.nftID.CopyFrom(self.nft_id._to_proto()) - - query = query_pb2.Query() - query.tokenGetNftInfo.CopyFrom(nft_info_query) - - return query - except Exception as e: - print(f"Exception in _make_request: {e}") - traceback.print_exc() - raise + if not self.nft_id: + raise ValueError("NFT ID must be set before making the request.") + + query_header = self._make_request_header() + + nft_info_query = token_get_nft_info_pb2.TokenGetNftInfoQuery() + nft_info_query.header.CopyFrom(query_header) + nft_info_query.nftID.CopyFrom(self.nft_id._to_proto()) + + query = query_pb2.Query() + query.tokenGetNftInfo.CopyFrom(nft_info_query) + + return query🧹 Follow-up cleanup
-import tracebacksrc/hiero_sdk_python/tokens/token_update_transaction.py (1)
492-515:⚠️ Potential issue | 🔴 CriticalRemove reference to non-existent
metadata_keyfield.The protobuf definition for
TokenUpdateTransactionBodydoes not include ametadata_keyfield. Attempting to accesstoken_update_body.metadata_keywill raise anAttributeErrorat runtime.The mixed casing (camelCase for
adminKey/kycKeyvs snake_case forpause_key/fee_schedule_key) is correct and matches the official hedera-protobufs definition. Remove themetadata_keyassignment or clarify if this field should map to a different protobuf field.
| staked_account_id: Optional[AccountId] = None, | ||
| staked_node_id: Optional[int] = None, | ||
| decline_staking_reward: Optional[bool] = False | ||
| decline_staking_reward: Optional[bool] = False, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor type hint inconsistency: Optional[bool] with non-None default.
The type Optional[bool] implies None is valid, but the default is False and the setter at line 318 rejects non-bool values (including None). Consider using bool = False for consistency, or if None is intentionally allowed in the constructor, update the setter accordingly.
💡 Suggested fix for type consistency
- decline_staking_reward: Optional[bool] = False,
+ decline_staking_reward: bool = False,📝 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.
| decline_staking_reward: Optional[bool] = False, | |
| decline_staking_reward: bool = False, |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 48-48: Boolean-typed positional argument in function definition
(FBT001)
[error] 48-48: Boolean default positional argument in function definition
(FBT002)
| if mirror_address.endswith(":50212") or mirror_address.endswith(":443"): | ||
| self.mirror_channel = grpc.secure_channel( | ||
| mirror_address, grpc.ssl_channel_credentials() | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Merge endswith calls into a single tuple check.
The static analysis correctly identifies that multiple endswith calls can be merged for cleaner code.
♻️ Proposed fix
- if mirror_address.endswith(":50212") or mirror_address.endswith(":443"):
+ if mirror_address.endswith((":50212", ":443")):
self.mirror_channel = grpc.secure_channel(
mirror_address, grpc.ssl_channel_credentials()
)🧰 Tools
🪛 Ruff (0.14.14)
[warning] 156-156: Call endswith once with a tuple
Merge into a single endswith call
(PIE810)
| # ← Extract the many ternaries here (this flattens control flow) | ||
| admin_key_proto = self.admin_key._to_proto() if self.admin_key else None | ||
| submit_key_proto = self.submit_key._to_proto() if self.submit_key else None | ||
| auto_renew_period = duration_pb2.Duration(seconds=self.auto_renew_period.seconds) if self.auto_renew_period else None | ||
| auto_renew_account = self.auto_renew_account._to_proto() if self.auto_renew_account else None | ||
| expiration_time = self.expiration_time._to_protobuf() if self.expiration_time else None | ||
| memo_value = _wrappers_pb2.StringValue(value=self.memo) if self.memo is not None else None | ||
| fee_schedule_key_proto = self.fee_schedule_key._to_proto() if self.fee_schedule_key else None | ||
|
|
There was a problem hiding this comment.
Remove stray code review comment.
Line 263 contains a leftover comment # ← Extract the many ternaries here that appears to be a code review artifact or development note. This should be removed before merging.
Proposed fix
- # ← Extract the many ternaries here
admin_key_proto = self.admin_key._to_proto() if self.admin_key else 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.
| # ← Extract the many ternaries here (this flattens control flow) | |
| admin_key_proto = self.admin_key._to_proto() if self.admin_key else None | |
| submit_key_proto = self.submit_key._to_proto() if self.submit_key else None | |
| auto_renew_period = duration_pb2.Duration(seconds=self.auto_renew_period.seconds) if self.auto_renew_period else None | |
| auto_renew_account = self.auto_renew_account._to_proto() if self.auto_renew_account else None | |
| expiration_time = self.expiration_time._to_protobuf() if self.expiration_time else None | |
| memo_value = _wrappers_pb2.StringValue(value=self.memo) if self.memo is not None else None | |
| fee_schedule_key_proto = self.fee_schedule_key._to_proto() if self.fee_schedule_key else None | |
| admin_key_proto = self.admin_key._to_proto() if self.admin_key else None | |
| submit_key_proto = self.submit_key._to_proto() if self.submit_key else None | |
| auto_renew_period = duration_pb2.Duration(seconds=self.auto_renew_period.seconds) if self.auto_renew_period else None | |
| auto_renew_account = self.auto_renew_account._to_proto() if self.auto_renew_account else None | |
| expiration_time = self.expiration_time._to_protobuf() if self.expiration_time else None | |
| memo_value = _wrappers_pb2.StringValue(value=self.memo) if self.memo is not None else None | |
| fee_schedule_key_proto = self.fee_schedule_key._to_proto() if self.fee_schedule_key else None |
| raise ValueError( | ||
| "to_string_with_checksum cannot be applied to ContractId with evm_address" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Address Ruff TRY003 (long message outside exception class).
If the rule is enforced, either suppress it or use a custom exception with a built-in message to comply.
🛠️ Minimal suppression option
if self.evm_address is not None:
raise ValueError(
"to_string_with_checksum cannot be applied to ContractId with evm_address"
- )
+ ) # noqa: TRY003🧰 Tools
🪛 Ruff (0.14.14)
[warning] 215-217: Avoid specifying long messages outside the exception class
(TRY003)
| def execute(self, client: "Client") -> Any: | ||
| """ | ||
| Executes the file append transaction. | ||
|
|
||
| For multi-chunk transactions, this method will execute all chunks sequentially. | ||
|
|
||
| Args: | ||
| client: The client to execute the transaction with. | ||
|
|
||
| Returns: | ||
| TransactionReceipt: The receipt from the first chunk execution. | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Inspect base Transaction.execute signature/return type
rg -nP 'class Transaction\b' src/hiero_sdk_python/transaction/transaction.py -C2
rg -nP 'def execute\(' src/hiero_sdk_python/transaction/transaction.py -C3Repository: hiero-ledger/hiero-sdk-python
Length of output: 349
🏁 Script executed:
# Get more context around Transaction.execute
sed -n '351,370p' src/hiero_sdk_python/transaction/transaction.py
# Check for type stub files
find . -name "*.pyi" | head -20
# Check ruff configuration
cat pyproject.toml | grep -A 20 "\[tool.ruff"Repository: hiero-ledger/hiero-sdk-python
Length of output: 1723
🏁 Script executed:
# Check FileAppendTransaction.execute() full implementation
sed -n '330,360p' src/hiero_sdk_python/file/file_append_transaction.py
# Check imports in FileAppendTransaction
head -50 src/hiero_sdk_python/file/file_append_transaction.py | grep -E "^import|^from"
# Verify ANN401 rule status in ruff
grep -i "ANN" pyproject.tomlRepository: hiero-ledger/hiero-sdk-python
Length of output: 1663
🏁 Script executed:
# Check if Client is in TYPE_CHECKING block in FileAppendTransaction
grep -n "TYPE_CHECKING" src/hiero_sdk_python/file/file_append_transaction.py -A 20
# Also check what the multi-chunk execute() actually returns at the end
sed -n '330,380p' src/hiero_sdk_python/file/file_append_transaction.py | tail -30Repository: hiero-ledger/hiero-sdk-python
Length of output: 2884
Correct the return type annotation to match the implementation.
The return type is annotated as Any, but the docstring and implementation indicate it returns TransactionReceipt. For single-chunk transactions it returns the base class result (TransactionReceipt), and for multi-chunk it returns responses[0] if responses else None. Update the signature to TransactionReceipt | None to accurately reflect the actual return type. The quoted Client type is necessary and should remain unchanged (it's correctly placed in the TYPE_CHECKING block to avoid circular imports).
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 330-330: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 330-330: Dynamically typed expressions (typing.Any) are disallowed in execute
(ANN401)
| if status == ResponseCode.OK: | ||
| if response.transactionGetRecord.header.responseType == query_header_pb2.ResponseType.COST_ANSWER: | ||
| if ( | ||
| response.transactionGetRecord.header.responseType | ||
| == query_header_pb2.ResponseType.COST_ANSWER | ||
| ): | ||
| return _ExecutionState.FINISHED | ||
| pass |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unnecessary pass statement.
The pass on line 161 serves no purpose and can be safely removed.
♻️ Proposed fix
if status == ResponseCode.OK:
if (
response.transactionGetRecord.header.responseType
== query_header_pb2.ResponseType.COST_ANSWER
):
return _ExecutionState.FINISHED
- pass
elif (📝 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.
| if status == ResponseCode.OK: | |
| if response.transactionGetRecord.header.responseType == query_header_pb2.ResponseType.COST_ANSWER: | |
| if ( | |
| response.transactionGetRecord.header.responseType | |
| == query_header_pb2.ResponseType.COST_ANSWER | |
| ): | |
| return _ExecutionState.FINISHED | |
| pass | |
| if status == ResponseCode.OK: | |
| if ( | |
| response.transactionGetRecord.header.responseType | |
| == query_header_pb2.ResponseType.COST_ANSWER | |
| ): | |
| return _ExecutionState.FINISHED |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 161-161: Unnecessary pass statement
Remove unnecessary pass
(PIE790)
| def set_denominating_token_id( | ||
| self, token_id: typing.Optional["TokenId"] | ||
| ) -> "CustomFixedFee": | ||
| """Sets the fungible token used to pay the fee. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read its contents
cat -n src/hiero_sdk_python/tokens/custom_fixed_fee.py | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 995
🏁 Script executed:
# Get file info and check lines 130-133 and 160-163
wc -l src/hiero_sdk_python/tokens/custom_fixed_fee.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 124
🏁 Script executed:
# Read the specific sections
sed -n '1,15p' src/hiero_sdk_python/tokens/custom_fixed_fee.py
sed -n '125,140p' src/hiero_sdk_python/tokens/custom_fixed_fee.py
sed -n '155,170p' src/hiero_sdk_python/tokens/custom_fixed_fee.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 1597
Address Ruff UP037: remove quoted annotations.
With from __future__ import annotations at line 1, annotations are already deferred and implicitly stringified. Remove unnecessary quotes from TokenId, CustomFixedFee, and custom_fees_pb2.FixedFee annotations at lines 130–133 and 160–163 to keep the codebase lint-clean.
Suggested changes
def set_denominating_token_id(
- self, token_id: typing.Optional["TokenId"]
- ) -> "CustomFixedFee":
+ self, token_id: typing.Optional[TokenId]
+ ) -> CustomFixedFee: `@staticmethod`
def _from_fixed_fee_proto(
- fixed_fee: "custom_fees_pb2.FixedFee",
- ) -> "CustomFixedFee":
+ fixed_fee: custom_fees_pb2.FixedFee,
+ ) -> CustomFixedFee:🧰 Tools
🪛 Ruff (0.14.14)
[warning] 131-131: Remove quotes from type annotation
Remove quotes
(UP037)
[warning] 132-132: Remove quotes from type annotation
Remove quotes
(UP037)
| raise ValueError("Pending airdrops list must contain mininum 1 and maximum 10 pendingAirdrop.") | ||
| if len(pending_airdrops_proto) < 1 or len(pending_airdrops_proto) > 10: | ||
| raise ValueError( | ||
| "Pending airdrops list must contain mininum 1 and maximum 10 pendingAirdrop." |
There was a problem hiding this comment.
Fix typo in error message.
The error message contains a typo: "mininum" should be "minimum".
📝 Proposed fix
- "Pending airdrops list must contain mininum 1 and maximum 10 pendingAirdrop."
+ "Pending airdrops list must contain minimum 1 and maximum 10 pendingAirdrop."📝 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.
| "Pending airdrops list must contain mininum 1 and maximum 10 pendingAirdrop." | |
| "Pending airdrops list must contain minimum 1 and maximum 10 pendingAirdrop." |
| • KYC_NOT_APPLICABLE – not applicable | ||
| • GRANTED – KYC granted | ||
| • REVOKED – KYC revoked |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace ambiguous EN DASH characters in docstring.
Ruff flags the – (EN DASH) characters as ambiguous (RUF002). Replace with standard hyphen-minus - to avoid encoding ambiguity.
🧹 Suggested cleanup
- • KYC_NOT_APPLICABLE – not applicable
- • GRANTED – KYC granted
+ • KYC_NOT_APPLICABLE - not applicable
+ • GRANTED - KYC granted
• REVOKED – KYC revoked
+ • REVOKED - KYC revoked📝 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.
| • KYC_NOT_APPLICABLE – not applicable | |
| • GRANTED – KYC granted | |
| • REVOKED – KYC revoked | |
| • KYC_NOT_APPLICABLE - not applicable | |
| • GRANTED - KYC granted | |
| • REVOKED - KYC revoked |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 18-18: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
[warning] 19-19: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
[warning] 20-20: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
| expected_decimals=( | ||
| {"value": self.expected_decimals} if self.expected_decimals else None | ||
| ), |
There was a problem hiding this comment.
Falsy check incorrectly excludes expected_decimals=0.
The condition if self.expected_decimals evaluates to False when expected_decimals is 0, which may be a valid decimal value. This would cause expected_decimals=0 to be silently omitted from the protobuf message.
🐛 Proposed fix
proto = basic_types_pb2.TokenTransferList(
token=self.token._to_proto(),
expected_decimals=(
- {"value": self.expected_decimals} if self.expected_decimals else None
+ {"value": self.expected_decimals} if self.expected_decimals is not None else 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.
| expected_decimals=( | |
| {"value": self.expected_decimals} if self.expected_decimals else None | |
| ), | |
| expected_decimals=( | |
| {"value": self.expected_decimals} if self.expected_decimals is not None else None | |
| ), |
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)
src/hiero_sdk_python/transaction/transaction_record.py (1)
62-76: 🧹 Nitpick | 🔵 TrivialDocument the new
duplicatesfield in the class docstring.
duplicatesis now a public field but isn’t listed in the Attributes section, which makes the public API docs incomplete.📝 Suggested doc update
prng_number (Optional[int]): A pseudo-random integer generated by the network (if applicable). prng_bytes (Optional[bytes]): A pseudo-random byte array generated by the network (if applicable). + duplicates (list[TransactionRecord]): Duplicate/child transaction records returned by include-duplicates queries.src/hiero_sdk_python/query/transaction_record_query.py (1)
177-212:⚠️ Potential issue | 🟠 MajorUse receipt status when constructing
ReceiptStatusError.
ReceiptStatusErrorshould reflect the receipt’s status; currently it uses the header precheck status (often OK), which can mask the actual failure code.🛠️ Proposed fix
- receipt = response.transactionGetRecord.transactionRecord.receipt - - return ReceiptStatusError( - status, - self.transaction_id, - TransactionReceipt._from_proto(receipt, self.transaction_id), - ) + receipt = response.transactionGetRecord.transactionRecord.receipt + receipt_status = receipt.status + + return ReceiptStatusError( + receipt_status, + self.transaction_id, + TransactionReceipt._from_proto(receipt, self.transaction_id), + )
|
Closing this PR to open a fresh one with all feedback incorporated. Thank you! |
|
New PR for the same issue #1671 |
5 participants
Description:
Add support for the
includeDuplicatesflagRelated issue(s):
Fixes #1635
Notes for reviewer:
Checklist