feat: Add optional receipt waiting to Transaction.execute() - #1769
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1769 +/- ##
==========================================
+ Coverage 93.35% 93.53% +0.18%
==========================================
Files 141 141
Lines 9121 9143 +22
==========================================
+ Hits 8515 8552 +37
+ Misses 606 591 -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! |
a84e840 to
0a32369
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an optional wait_for_receipt flag to Transaction.execute() (default True) to return either a TransactionReceipt or TransactionResponse; extends TransactionResponse with query-building and fetch methods for receipts/records; and propagates wait_for_receipt through chunked multi-part submissions, examples, and tests. 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
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"]:
|
Nice! looks pretty good, will look at this in more detail over next few days |
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.
AntonioCeppellini
left a comment
There was a problem hiding this comment.
LGTM! Nice work @manishdait :D
|
Hi @manishdait, This pull request has had no commit activity for 10 days. Are you still working on it?
If you're no longer working on this, please comment Reach out on discord or join our office hours if you need assistance. From the Python SDK Team |
Dosik13
left a comment
There was a problem hiding this comment.
i left some typos to fix :D
34575e4 to
f75e7ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
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/file/file_append_transaction.py (1)
379-399:⚠️ Potential issue | 🟠 MajorChunk transaction IDs are regenerated inside the chunk loop, causing ID drift across chunks.
execute_all()callsfreeze_with(client)every iteration after clearing state. Sincefreeze_with()rebuilds_transaction_idsfrom the currenttransaction_id, later chunks no longer follow the originally generated sequence.Proposed fix
- # Generate transaction IDs for all chunks - self._transaction_ids = [] - base_timestamp = self.transaction_id.valid_start - - for i in range(self.get_required_chunks()): - if i == 0: - # First chunk uses the original transaction ID - chunk_transaction_id = self.transaction_id - else: - # Subsequent chunks get incremented timestamps - # Add i nanoseconds to space out chunks - chunk_valid_start = timestamp_pb2.Timestamp( - 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 - ) - self._transaction_ids.append(chunk_transaction_id) + if not self._transaction_ids: + self._transaction_ids = [] + base_timestamp = self.transaction_id.valid_start + for i in range(self.get_required_chunks()): + if i == 0: + chunk_transaction_id = self.transaction_id + else: + chunk_valid_start = timestamp_pb2.Timestamp( + 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 + ) + self._transaction_ids.append(chunk_transaction_id)As per coding guidelines, “freeze_with() correctly generates sequential TransactionIds for each chunk.”
♻️ Duplicate comments (8)
examples/transaction/transaction_without_wait_for_receipt.py (1)
19-25:⚠️ Potential issue | 🟡 MinorThe generated account key is discarded, making the created account hard to use afterward.
build_transaction()creates aPrivateKeybut never returns or displays it. Users copying this example lose access to the new account credentials.Proposed fix
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) + tx = AccountCreateTransaction().set_key_without_alias(key).set_initial_balance(1) + return tx, key @@ - tx = build_transaction() + tx, new_account_key = build_transaction() + print(f"Generated new account private key: {new_account_key.to_string()}")As per coding guidelines, “examples work verbatim for users who copy-paste them.”
src/hiero_sdk_python/file/file_append_transaction.py (1)
345-347:⚠️ Potential issue | 🟠 Major
execute()can returnNonefor empty payloads, violating its declared return contract.With
set_contents(b""),get_required_chunks()becomes 0,execute_all()returns[], and Line 347 returnsNone. That breaks the annotated/public API contract (TransactionReceipt | TransactionResponse).Proposed fix
responses = self.execute_all(client, timeout, wait_for_receipt) - return responses[0] if responses else None + if not responses: + raise RuntimeError("No chunk responses were produced.") + return responses[0]- return math.ceil(len(self.contents) / self.chunk_size) + return max(1, math.ceil(len(self.contents) / self.chunk_size))As per coding guidelines, “Public API contracts … are user-facing.”
src/hiero_sdk_python/consensus/topic_message_submit_transaction.py (1)
310-313:⚠️ Potential issue | 🟡 MinorAvoid the
Nonefallback inexecute()to keep the runtime contract strict.Line 312 returns
Noneon emptyresponses, but the method contract isTransactionReceipt | TransactionResponse. Prefer failing loudly instead of returning an undocumented type.Proposed fix
responses = self.execute_all(client, timeout, wait_for_receipt) - return responses[0] if responses else None + if not responses: + raise RuntimeError("No chunk responses were produced.") + return responses[0]tests/unit/topic_message_submit_transaction_test.py (3)
301-304: 🧹 Nitpick | 🔵 TrivialRemove
try/except pytest.fail(...)wrappers aroundexecute_all.These wrappers reduce traceback quality and make failures harder to diagnose; let pytest surface the original exception directly.
Proposed simplification
- 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) ... - 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)As per coding guidelines, "Tests must provide useful error messages when they fail for future debugging."
Also applies to: 342-345
372-375:⚠️ Potential issue | 🟡 MinorStrengthen non-wait execute assertions for compatibility guarantees.
Line 374 currently checks only type; add negative-type and key public-attribute assertions to guard behavior drift.
Proposed assertion strengthening
response = tx.execute(client, wait_for_receipt=False) assert isinstance(response, TransactionResponse) + assert not isinstance(response, TransactionReceipt), "Expected TransactionResponse, not TransactionReceipt" + assert response.transaction is tx, "Response should reference the submitted transaction" + assert response.validate_status is True, "Response should preserve validate_status default"As per coding guidelines, "Assert return types where relevant" and "Assert public attributes exist."
431-433:⚠️ Potential issue | 🟡 MinorAssert expected list length before indexing responses.
Line 432 indexes
responses[0]without asserting size, which produces less actionable failures.Proposed fix
responses = tx.execute_all(client, wait_for_receipt=False) assert isinstance(responses, list) + assert len(responses) == 1, "Single-chunk message should produce exactly one response" assert isinstance(responses[0], TransactionResponse)As per coding guidelines, "Tests must provide useful error messages when they fail for future debugging."
tests/integration/transaction_e2e_test.py (2)
19-25:⚠️ Potential issue | 🟠 Major
create_transaction()prevents account cleanup by discarding the private key.Without returning/storing the generated key, teardown cannot reliably delete created accounts, causing long-term testnet state leakage.
Suggested refactor direction
def create_transaction(): """Create a minimal valid AccountCreateTransaction for integration tests.""" - return ( - AccountCreateTransaction() - .set_key_without_alias(PrivateKey.generate()) - .set_initial_balance(1) - ) + key = PrivateKey.generate() + tx = ( + AccountCreateTransaction() + .set_key_without_alias(key) + .set_initial_balance(1) + ) + return tx, keyAs per coding guidelines, "Are accounts, tokens, and allowances properly cleaned up to avoid state leakage?"
48-49:⚠️ Potential issue | 🟡 MinorAssert
transaction_idis populated before equality comparison.Line 48 can pass even if both IDs are unset/default; assert non-null (and ideally non-default fields) first.
Proposed assertion hardening
assert response.transaction is tx + assert response.transaction_id is not None, "Expected a populated transaction_id from execute()" assert response.transaction_id == tx.transaction_id assert response.validate_status is TrueAs per coding guidelines, "Tests should assert observable network behavior, not just
SUCCESS."
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.mdexamples/transaction/transaction_without_wait_for_receipt.pysrc/hiero_sdk_python/consensus/topic_message_submit_transaction.pysrc/hiero_sdk_python/file/file_append_transaction.pysrc/hiero_sdk_python/transaction/transaction.pysrc/hiero_sdk_python/transaction/transaction_response.pytests/integration/transaction_e2e_test.pytests/unit/file_append_transaction_test.pytests/unit/topic_message_submit_transaction_test.pytests/unit/transaction_response_test.pytests/unit/transaction_test.py
exploreriii
left a comment
There was a problem hiding this comment.
Hi @manishdait please rebase and LGTM
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>
chore: misssing return types 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>
1ad23e6 to
0c6c063
Compare
|
Thank you, @manishdait ! |
…dger#1769) Signed-off-by: Manish Dait <daitmanish88@gmail.com>
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