feat: Add optional receipt waiting to Transaction.execute()#1769
feat: Add optional receipt waiting to Transaction.execute()#1769manishdait wants to merge 15 commits intohiero-ledger:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1769 +/- ##
==========================================
+ Coverage 93.29% 93.48% +0.18%
==========================================
Files 141 141
Lines 9118 9142 +24
==========================================
+ Hits 8507 8546 +39
+ Misses 611 596 -15 🚀 New features to boost your workflow:
|
|
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! |
manishdait
force-pushed
the
feat/remove-get-receipt
branch
2 times, most recently
from
cbebd59 to
7816d2e
Compare
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
chore: move changelog entry Signed-off-by: Manish Dait <daitmanish88@gmail.com>
manishdait
force-pushed
the
feat/remove-get-receipt
branch
from
7816d2e to
a84e840
Compare
chore: misssing return types Signed-off-by: Manish Dait <daitmanish88@gmail.com>
manishdait
force-pushed
the
feat/remove-get-receipt
branch
from
a84e840 to
0a32369
Compare
manishdait
added
help needed: committers
and removed
status: in initial development
labels
WalkthroughAdds an optional wait_for_receipt flag to Transaction.execute() (default True) to return either a TransactionReceipt or TransactionResponse, and extends TransactionResponse with methods to build/execute receipt and record queries and to fetch receipts/records on demand. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client
participant Transaction
participant TransactionResponse
participant Query
participant Network
rect rgba(100,150,200,0.5)
Note over User,Network: Execute with wait_for_receipt=True (default)
User->>Transaction: execute(client, wait_for_receipt=True)
Transaction->>Network: submit transaction
Network-->>Transaction: TransactionResponse
Transaction->>TransactionResponse: get_receipt(client)
TransactionResponse->>Query: build & execute receipt query
Query->>Network: fetch receipt
Network-->>Query: TransactionReceipt
Query-->>TransactionResponse: TransactionReceipt
TransactionResponse-->>Transaction: TransactionReceipt
Transaction-->>User: TransactionReceipt
end
rect rgba(150,200,100,0.5)
Note over User,Network: Execute with wait_for_receipt=False
User->>Transaction: execute(client, wait_for_receipt=False)
Transaction->>Network: submit transaction
Network-->>Transaction: TransactionResponse
Transaction-->>User: TransactionResponse (immediate)
Note over User: Later: retrieve receipt on-demand
User->>TransactionResponse: get_receipt(client)
TransactionResponse->>Query: build & execute receipt query
Query->>Network: fetch receipt
Network-->>Query: TransactionReceipt
Query-->>TransactionResponse: TransactionReceipt
TransactionResponse-->>User: TransactionReceipt
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 8
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.py (1)
333-382:⚠️ Potential issue | 🟡 Minor
Optional[bool]allowsNone, which silently behaves likeFalse.
wait_for_receipt: Optional[bool] = TrueacceptsNone, andif None:is falsy — soexecute(client, wait_for_receipt=None)would skip receipt retrieval without any warning, which may surprise callers. Usebool = Trueinstead.Proposed fix
def execute( self, client: "Client", timeout: Optional[Union[int, float]] = None, - wait_for_receipt: Optional[bool] = True + wait_for_receipt: bool = True ) -> Union["TransactionReceipt", "TransactionResponse"]:
manishdait
added
status: needs committer review
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
| def build_transaction(): | ||
| """ | ||
| Build a new AccountCreateTransaction with a generated private key | ||
| and a minimal initial balance. | ||
| """ | ||
| key = PrivateKey.generate() | ||
| return AccountCreateTransaction().set_key_without_alias(key).set_initial_balance(1) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Generated key is discarded — the created account becomes inaccessible.
The build_transaction() function generates a PrivateKey but doesn't return or store it. The user copying this example won't be able to use the created account afterward. For an example demonstrating the new API, consider printing or returning the key so users understand how to manage the created account.
Suggested improvement
def build_transaction():
"""
Build a new AccountCreateTransaction with a generated private key
and a minimal initial balance.
+
+ Returns:
+ tuple: (AccountCreateTransaction, PrivateKey)
"""
key = PrivateKey.generate()
- return AccountCreateTransaction().set_key_without_alias(key).set_initial_balance(1)
+ tx = AccountCreateTransaction().set_key_without_alias(key).set_initial_balance(1)
+ return tx, keyThen in main():
- tx = build_transaction()
+ tx, new_account_key = build_transaction()
+ print(f"Generated key: {new_account_key.to_string()}")| @pytest.mark.integration | ||
| def test_execute_without_wait_returns_transaction_response(env): | ||
| """Test execute return TransactionResponse when wait_for_receipt is False.""" | ||
| tx = create_transaction() | ||
| response = tx.execute(env.client, wait_for_receipt=False) | ||
|
|
||
| assert isinstance(response, TransactionResponse) | ||
| assert not isinstance(response, TransactionReceipt) | ||
|
|
||
| assert response.transaction is tx | ||
| assert response.transaction_id == tx.transaction_id | ||
| assert response.validate_status is True |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen assertions: verify response.transaction_id has valid content.
The test asserts response.transaction_id == tx.transaction_id but doesn't verify the transaction_id is actually populated (non-default). Adding assert response.transaction_id is not None or checking .account_id on the transaction_id would catch cases where both sides are empty/default and the equality passes vacuously. As per coding guidelines, "Tests should assert observable network behavior, not just SUCCESS."
| def test_get_record_executes_and_returns_record(transaction_response): | ||
| """Test get_record execute record query and return transaction record.""" | ||
| receipt = transaction_receipt_pb2.TransactionReceipt(status=ResponseCode.SUCCESS) | ||
| record = transaction_record_pb2.TransactionRecord( | ||
| receipt=receipt, memo="record", transactionFee=100 | ||
| ) | ||
|
|
||
| record_response = [ | ||
| [ | ||
| response_pb2.Response( | ||
| transactionGetRecord=transaction_get_record_pb2.TransactionGetRecordResponse( | ||
| header=response_header_pb2.ResponseHeader( | ||
| nodeTransactionPrecheckCode=ResponseCode.OK, | ||
| responseType=ResponseType.COST_ANSWER, | ||
| cost=2, | ||
| ) | ||
| ) | ||
| ), | ||
| response_pb2.Response( | ||
| transactionGetRecord=transaction_get_record_pb2.TransactionGetRecordResponse( | ||
| header=response_header_pb2.ResponseHeader( | ||
| nodeTransactionPrecheckCode=ResponseCode.OK, | ||
| responseType=ResponseType.ANSWER_ONLY, | ||
| cost=2, | ||
| ), | ||
| transactionRecord=record, | ||
| ) | ||
| ), | ||
| ] | ||
| ] | ||
|
|
||
| with mock_hedera_servers(record_response) as client: | ||
| record = transaction_response.get_record(client) | ||
|
|
||
| assert isinstance(record, TransactionRecord) | ||
| assert record.receipt.status == ResponseCode.SUCCESS |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding more record field assertions.
The test only verifies record.receipt.status. Consider also asserting record.transaction_fee or record.memo matches the values set in the mock proto (line 88: memo="record", transactionFee=100). This would strengthen the test against deserialization regressions. As per coding guidelines, "Cover happy paths AND unhappy paths/edge cases" and "Assert return types where relevant."
Suggested additions
assert isinstance(record, TransactionRecord)
assert record.receipt.status == ResponseCode.SUCCESS
+ assert record.transaction_fee == 100
+ assert record.transaction_memo == "record"
|
Nice! looks pretty good, will look at this in more detail over next few days |
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 13
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/consensus/topic_message_submit_transaction.py (1)
289-361: 🛠️ Refactor suggestion | 🟠 MajorSignificant code duplication with
FileAppendTransaction.execute/execute_all.The
execute→execute_alldelegation pattern, including the chunk loop (clear frozen state, re-freeze, re-sign, callsuper().execute(...), collect responses), is nearly identical betweenTopicMessageSubmitTransactionandFileAppendTransaction. Consider extracting the shared multi-chunk execution logic into the baseTransactionclass or a mixin to avoid divergence.
| def execute( | ||
| self, | ||
| client: "Client", | ||
| timeout: Optional[Union[int, float]] = None, | ||
| wait_for_receipt: Optional[bool] = True | ||
| ) -> Union["TransactionReceipt", "TransactionResponse"]: | ||
| """ | ||
| Executes the topic message submit transaction. | ||
|
|
||
| For multi-chunk transactions, this method will execute all chunks sequentially. | ||
| For multi-chunk transactions, this method will execute all chunks sequentially and return first response. | ||
|
|
||
| Args: | ||
| client: The client to execute the transaction with. | ||
| timeout (Optional[Union[int, float]): The total execution timeout (in seconds) for this execution. | ||
| wait_for_receipt (Optional[bool]): Whether to wait for consensus and return the receipt. | ||
| If False, the method returns a TransactionResponse immediately after submission. | ||
|
|
||
| Returns: | ||
| TransactionReceipt: The receipt from the first chunk execution. | ||
| TransactionReceipt: If wait_for_receipt is True (default) | ||
| TransactionResponse: If wait_for_receipt is False | ||
| """ | ||
| # Return the first response as the JS SDK does | ||
| responses = self.execute_all(client, timeout, wait_for_receipt) | ||
| return responses[0] if responses else None |
There was a problem hiding this comment.
Same None-return risk as FileAppendTransaction.execute().
Line 312: return responses[0] if responses else None — the return type annotation doesn't include None, and callers won't expect it. Same issue as in file_append_transaction.py.
Proposed fix
# Return the first response as the JS SDK does
responses = self.execute_all(client, timeout, wait_for_receipt)
- return responses[0] if responses else None
+ if not responses:
+ raise RuntimeError("execute_all returned no responses")
+ return responses[0]📝 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.
| def execute( | |
| self, | |
| client: "Client", | |
| timeout: Optional[Union[int, float]] = None, | |
| wait_for_receipt: Optional[bool] = True | |
| ) -> Union["TransactionReceipt", "TransactionResponse"]: | |
| """ | |
| Executes the topic message submit transaction. | |
| For multi-chunk transactions, this method will execute all chunks sequentially. | |
| For multi-chunk transactions, this method will execute all chunks sequentially and return first response. | |
| Args: | |
| client: The client to execute the transaction with. | |
| timeout (Optional[Union[int, float]): The total execution timeout (in seconds) for this execution. | |
| wait_for_receipt (Optional[bool]): Whether to wait for consensus and return the receipt. | |
| If False, the method returns a TransactionResponse immediately after submission. | |
| Returns: | |
| TransactionReceipt: The receipt from the first chunk execution. | |
| TransactionReceipt: If wait_for_receipt is True (default) | |
| TransactionResponse: If wait_for_receipt is False | |
| """ | |
| # Return the first response as the JS SDK does | |
| responses = self.execute_all(client, timeout, wait_for_receipt) | |
| return responses[0] if responses else None | |
| def execute( | |
| self, | |
| client: "Client", | |
| timeout: Optional[Union[int, float]] = None, | |
| wait_for_receipt: Optional[bool] = True | |
| ) -> Union["TransactionReceipt", "TransactionResponse"]: | |
| """ | |
| Executes the topic message submit transaction. | |
| For multi-chunk transactions, this method will execute all chunks sequentially and return first response. | |
| Args: | |
| client: The client to execute the transaction with. | |
| timeout (Optional[Union[int, float]): The total execution timeout (in seconds) for this execution. | |
| wait_for_receipt (Optional[bool]): Whether to wait for consensus and return the receipt. | |
| If False, the method returns a TransactionResponse immediately after submission. | |
| Returns: | |
| TransactionReceipt: If wait_for_receipt is True (default) | |
| TransactionResponse: If wait_for_receipt is False | |
| """ | |
| # Return the first response as the JS SDK does | |
| responses = self.execute_all(client, timeout, wait_for_receipt) | |
| if not responses: | |
| raise RuntimeError("execute_all returned no responses") | |
| return responses[0] |
| Args: | ||
| client: The client to execute the transaction with. | ||
| timeout (Optional[Union[int, float]): The total execution timeout (in seconds) for this execution. | ||
| wait_for_receipt (Optional[bool]): Whether to wait for consensus and return the receipt. | ||
| If False, the method returns a TransactionResponse immediately after submission. | ||
|
|
||
| Returns: | ||
| TransactionReceipt: The receipt from the first chunk execution. | ||
| TransactionReceipt: If wait_for_receipt is True (default) | ||
| TransactionResponse: If wait_for_receipt is False |
There was a problem hiding this comment.
Docstring bracket typo.
Line 302: Optional[Union[int, float]) — same mismatched bracket as in file_append_transaction.py.
Proposed fix
- timeout (Optional[Union[int, float]): The total execution timeout (in seconds) for this execution.
+ timeout (Optional[Union[int, float]]): The total execution timeout (in seconds) for this execution.Same fix needed at line 327.
📝 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.
| Args: | |
| client: The client to execute the transaction with. | |
| timeout (Optional[Union[int, float]): The total execution timeout (in seconds) for this execution. | |
| wait_for_receipt (Optional[bool]): Whether to wait for consensus and return the receipt. | |
| If False, the method returns a TransactionResponse immediately after submission. | |
| Returns: | |
| TransactionReceipt: The receipt from the first chunk execution. | |
| TransactionReceipt: If wait_for_receipt is True (default) | |
| TransactionResponse: If wait_for_receipt is False | |
| Args: | |
| client: The client to execute the transaction with. | |
| timeout (Optional[Union[int, float]]): The total execution timeout (in seconds) for this execution. | |
| wait_for_receipt (Optional[bool]): Whether to wait for consensus and return the receipt. | |
| If False, the method returns a TransactionResponse immediately after submission. | |
| Returns: | |
| TransactionReceipt: If wait_for_receipt is True (default) | |
| TransactionResponse: If wait_for_receipt is False |
| def execute( | ||
| self, | ||
| client: "Client", | ||
| timeout: Optional[Union[int, float]] = None, | ||
| wait_for_receipt: Optional[bool] = True | ||
| ) -> Union["TransactionReceipt", "TransactionResponse"]: | ||
| """ | ||
| Executes the file append transaction. | ||
|
|
||
| For multi-chunk transactions, this method will execute all chunks sequentially and return first response. | ||
|
|
||
| Args: | ||
| client: The client to execute the transaction with. | ||
| timeout (Optional[Union[int, float]): The total execution timeout (in seconds) for this execution. | ||
| wait_for_receipt (Optional[bool]): Whether to wait for consensus and return the receipt. | ||
| If False, the method returns a TransactionResponse immediately after submission. | ||
|
|
||
| Returns: | ||
| TransactionReceipt: If wait_for_receipt is True (default) | ||
| TransactionResponse: If wait_for_receipt is False | ||
| """ | ||
| # Return the first response (as per JavaScript implementation) | ||
| responses = self.execute_all(client, timeout, wait_for_receipt) | ||
| return responses[0] if responses else None |
There was a problem hiding this comment.
execute() can return None when responses is empty, but the type hint and callers don't expect it.
Line 347 returns None when execute_all returns an empty list. However, execute_all can only return an empty list if get_required_chunks() returns 0 and the loop doesn't execute — which currently doesn't happen because _validate_chunking and _calculate_total_chunks ensure at least 1 chunk. Still, the None fallback is misleading and fragile. Consider raising an exception instead to fail loudly.
Proposed fix
# Return the first response (as per JavaScript implementation)
responses = self.execute_all(client, timeout, wait_for_receipt)
- return responses[0] if responses else None
+ if not responses:
+ raise RuntimeError("execute_all returned no responses")
+ return responses[0]| def execute_all( | ||
| self, | ||
| client: "Client", | ||
| timeout: Optional[Union[int, float]] = None, | ||
| wait_for_receipt: Optional[bool] = True | ||
| ) -> Union["TransactionReceipt", "TransactionResponse"]: |
There was a problem hiding this comment.
execute_all return type annotation is incorrect — declares scalar but returns a list.
The type hint says Union["TransactionReceipt", "TransactionResponse"] but the method returns List[...]. This will mislead type checkers and IDE users.
Proposed fix
def execute_all(
self,
client: "Client",
timeout: Optional[Union[int, float]] = None,
wait_for_receipt: Optional[bool] = True
- ) -> Union["TransactionReceipt", "TransactionResponse"]:
+ ) -> Union[List["TransactionReceipt"], List["TransactionResponse"]]:| response = tx.execute(client, wait_for_receipt=False) | ||
|
|
||
| assert isinstance(response, TransactionResponse) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen assertions — verify TransactionResponse fields, not just type.
Per coding guidelines, tests should "provide useful error messages when they fail" and validate observable behavior beyond just isinstance. Consider also asserting response.transaction_id is populated and response.transaction is tx.
Proposed enhancement
response = tx.execute(client, wait_for_receipt=False)
assert isinstance(response, TransactionResponse)
+ assert response.transaction is tx, "Response should reference the originating transaction"
+ assert response.validate_status is TrueAs per coding guidelines, "Tests must provide useful error messages when they fail for future debugging."
| def test_file_append_tx_execute_all_without_wait_for_receipt(file_id): | ||
| """Test should return list of TransactionResponse when wait_for_receipt=False.""" | ||
| content = "Hello Hiero" | ||
|
|
||
| tx_response = transaction_response_pb2.TransactionResponse( | ||
| nodeTransactionPrecheckCode=ResponseCode.OK | ||
| ) | ||
|
|
||
| response_sequence = [tx_response] # No receipt | ||
|
|
||
| with mock_hedera_servers([response_sequence]) as client: | ||
| tx = ( | ||
| FileAppendTransaction() | ||
| .set_file_id(file_id) | ||
| .set_contents(content) | ||
| .freeze_with(client) | ||
| ) | ||
|
|
||
| responses = tx.execute_all(client, wait_for_receipt=False) | ||
|
|
||
| assert isinstance(responses, list) | ||
| assert isinstance(responses[0], TransactionResponse) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
execute_all tests should also assert list length.
The test verifies isinstance(responses, list) and type of the first element, but doesn't assert len(responses) == 1. This would catch regressions where the single-chunk path accidentally returns multiple or zero elements.
Proposed enhancement
responses = tx.execute_all(client, wait_for_receipt=False)
assert isinstance(responses, list)
+ assert len(responses) == 1, "Single-chunk transaction should return exactly one response"
assert isinstance(responses[0], TransactionResponse)Apply the same to test_file_append_tx_execute_all_with_wait_for_receipt.
As per coding guidelines, "Assert return types where relevant" and "Tests must provide useful error messages when they fail."
| def test_topic_message_submit_execute_all_multi_chunk_success(topic_id): | ||
| """Test multi-chunk transaction should return list of receipts for each chunk.""" | ||
| # Create a large message (just under the typical 4KB limit) | ||
| large_message = "A" * 4000 | ||
|
|
||
| # Create a single node response sequence for all chunks | ||
| tx_response = transaction_response_pb2.TransactionResponse( | ||
| nodeTransactionPrecheckCode=ResponseCode.OK | ||
| ) | ||
|
|
||
| receipt_response = response_pb2.Response( | ||
| transactionGetReceipt=transaction_get_receipt_pb2.TransactionGetReceiptResponse( | ||
| header=response_header_pb2.ResponseHeader( | ||
| nodeTransactionPrecheckCode=ResponseCode.OK | ||
| ), | ||
| receipt=transaction_receipt_pb2.TransactionReceipt( | ||
| status=ResponseCode.SUCCESS | ||
| ) | ||
| ) | ||
| ) | ||
|
|
||
| # For simplicity, assume 4 chunks are required | ||
| # All chunks go to the same node, so repeat the same responses for that node | ||
| response_sequence = [tx_response, receipt_response] * 4 # 4 chunks | ||
|
|
||
| with mock_hedera_servers([response_sequence]) as client: | ||
| tx = ( | ||
| TopicMessageSubmitTransaction() | ||
| .set_topic_id(topic_id) | ||
| .set_message(large_message) | ||
| .freeze_with(client) | ||
| ) | ||
|
|
||
| try: | ||
| receipts = tx.execute_all(client) | ||
| except Exception as e: | ||
| pytest.fail(f"Should not raise exception, but raised: {e}") | ||
|
|
||
| assert isinstance(receipts, list) | ||
| assert len(receipts) == 4 | ||
| for receipt in receipts: | ||
| assert receipt.status == ResponseCode.SUCCESS |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good multi-chunk execute_all coverage with receipt validation.
The test correctly verifies list length matches the expected chunk count and validates each receipt status. Well-structured.
One improvement: the try/except pytest.fail(...) pattern (lines 301–304) is unnecessary — pytest inherently fails on unhandled exceptions and produces a better traceback without it.
Simplify by removing try/except
- try:
- receipts = tx.execute_all(client)
- except Exception as e:
- pytest.fail(f"Should not raise exception, but raised: {e}")
+ receipts = tx.execute_all(client)| def test_topic_message_submit_execute_without_wait_for_receipt(topic_id): | ||
| """Test should return TransactionResponse when wait_for_receipt=False.""" | ||
| message = "Hello Hiero" | ||
|
|
||
| tx_response = transaction_response_pb2.TransactionResponse( | ||
| nodeTransactionPrecheckCode=ResponseCode.OK | ||
| ) | ||
|
|
||
| response_sequence = [tx_response] # No receipt | ||
|
|
||
| with mock_hedera_servers([response_sequence]) as client: | ||
| tx = ( | ||
| TopicMessageSubmitTransaction() | ||
| .set_topic_id(topic_id) | ||
| .set_message(message) | ||
| .freeze_with(client) | ||
| ) | ||
|
|
||
| response = tx.execute(client, wait_for_receipt=False) | ||
|
|
||
| assert isinstance(response, TransactionResponse) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen: assert response is not a TransactionReceipt and verify key fields.
Similar to the file append test, verifying that the response is not a receipt and checking response.transaction and response.validate_status would make the test more robust.
Proposed enhancement
response = tx.execute(client, wait_for_receipt=False)
assert isinstance(response, TransactionResponse)
+ assert not isinstance(response, TransactionReceipt), "Should not be a receipt"
+ assert response.transaction is tx
+ assert response.validate_status is TrueAs per coding guidelines, "Assert return types where relevant" and "Assert public attributes exist."
| def test_topic_message_submit_execute_all_without_wait_for_receipt(topic_id): | ||
| """Test should return list of TransactionResponse when wait_for_receipt=False.""" | ||
| message = "Hello Hiero" | ||
|
|
||
| tx_response = transaction_response_pb2.TransactionResponse( | ||
| nodeTransactionPrecheckCode=ResponseCode.OK | ||
| ) | ||
|
|
||
| response_sequence = [tx_response] # No receipt | ||
|
|
||
| with mock_hedera_servers([response_sequence]) as client: | ||
| tx = ( | ||
| TopicMessageSubmitTransaction() | ||
| .set_topic_id(topic_id) | ||
| .set_message(message) | ||
| .freeze_with(client) | ||
| ) | ||
|
|
||
| responses = tx.execute_all(client, wait_for_receipt=False) | ||
|
|
||
| assert isinstance(responses, list) | ||
| assert isinstance(responses[0], TransactionResponse) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assert list length for single-chunk execute_all without receipt.
Same pattern as file append tests — assert len(responses) == 1 to catch regressions.
Proposed enhancement
responses = tx.execute_all(client, wait_for_receipt=False)
assert isinstance(responses, list)
+ assert len(responses) == 1
assert isinstance(responses[0], TransactionResponse)As per coding guidelines, "Tests must provide useful error messages when they fail for future debugging."
2 participants
Description:
This PR updates the
Transaction.execute()method to include await_for_receiptparameter to removeget_receipt()from executing while being backward compatible.wait_for_receipt=True(default), the method returns the transaction receipt after execution.wait_for_receipt=False, the method returns theTransactionResponseimmediately without waiting for receipt.Related issue(s):
Fixes #398
Notes for reviewer:
Checklist