Conversation
|
Hi, this is WorkflowBot.
|
There was a problem hiding this comment.
Pull request overview
Fixes an issue where child transaction receipts returned by TransactionGetReceiptQuery do not expose expected receipt protobuf fields (e.g., accountID / account_id) when include_children is enabled.
Changes:
- Map child/duplicate receipt protos via
_map_receipt_list()usingTransactionReceipt._from_proto(..., None)rather than passing the parenttransaction_id. - Update
TransactionReceipt._from_proto()type hint to acceptOptional[TransactionId]. - Add a unit test asserting child receipt
account_idaccess andtransaction_idbehavior; add a changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/hiero_sdk_python/query/transaction_get_receipt_query.py |
Changes receipt mapping for children/duplicates to pass None as transaction_id. |
src/hiero_sdk_python/transaction/transaction_receipt.py |
Updates _from_proto() signature/docstring to allow transaction_id=None. |
tests/unit/get_receipt_query_test.py |
Adds a unit test for child receipt account_id accessibility. |
CHANGELOG.md |
Documents the intended fix under [Unreleased]. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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:
WalkthroughChild and duplicate transaction receipt mapping was changed so child receipts are converted from protobuf with a None parent Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89bf9397-57f5-405e-a6c3-9250dde31e41
📒 Files selected for processing (4)
CHANGELOG.mdsrc/hiero_sdk_python/query/transaction_get_receipt_query.pysrc/hiero_sdk_python/transaction/transaction_receipt.pytests/unit/get_receipt_query_test.py
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #2013 +/- ##
==========================================
+ Coverage 93.68% 93.71% +0.02%
==========================================
Files 144 144
Lines 9350 9351 +1
==========================================
+ Hits 8760 8763 +3
+ Misses 590 588 -2 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/unit/get_receipt_query_test.py (1)
517-517: 🧹 Nitpick | 🔵 TrivialAdd a matching duplicate-receipt regression test for the same mapping path.
_map_receipt_list()is used for both children and duplicates, but the new tests only harden children. Add the sameaccount_idassertion forinclude_duplicates=Trueto prevent branch-specific regressions and ensure duplicate receipts retain the parent'stransaction_id.✅ Suggested additional test
+ + +def test_duplicate_receipts_with_account_id(transaction_id): + """Test that duplicate receipts have accountID populated and retain parent transaction_id.""" + duplicate_account_id = basic_types_pb2.AccountID(shardNum=0, realmNum=0, accountNum=888) + + 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), + duplicateTransactionReceipts=[ + transaction_receipt_pb2.TransactionReceipt( + status=ResponseCode.SUCCESS, + accountID=duplicate_account_id, + ), + ], + ) + ) + + response_sequences = [[response]] + + with mock_hedera_servers(response_sequences) as client: + result = ( + TransactionGetReceiptQuery() + .set_transaction_id(transaction_id) + .set_include_duplicates(True) + .execute(client) + ) + + assert len(result.duplicates) == 1 + duplicate_receipt = result.duplicates[0] + assert duplicate_receipt.account_id is not None + assert duplicate_receipt.account_id.num == 888 + # Duplicates retain parent transaction_id (different from child receipts) + assert duplicate_receipt.transaction_id == transaction_id
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1e3c61d-2c88-4d4b-bad7-c79998414ef6
📒 Files selected for processing (3)
src/hiero_sdk_python/query/transaction_get_receipt_query.pysrc/hiero_sdk_python/transaction/transaction_receipt.pytests/unit/get_receipt_query_test.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/get_receipt_query_test.py (1)
466-471:⚠️ Potential issue | 🟡 MinorStrengthen child-receipt regression by asserting
transaction_id is None.Both child-receipt tests validate
account_id, but neither verifies the mapping behavior that prevents parent ID inheritance. Add an explicittransaction_id is Noneassertion (and optionallyhasattr) in both tests.Suggested test hardening
assert len(result.children) == 1 child_receipt = result.children[0] # Verify: child receipt has account_id accessible + assert hasattr(child_receipt, "account_id") assert child_receipt.account_id is not None assert child_receipt.account_id.num == 999 + assert child_receipt.transaction_id is None @@ assert len(result.children) == 1 child_receipt = result.children[0] # Verify: account_id is accessible even with accountNum=0 + assert hasattr(child_receipt, "account_id") assert child_receipt.account_id is not None assert child_receipt.account_id.num == 0 assert child_receipt.account_id.shard == 0 assert child_receipt.account_id.realm == 0 + assert child_receipt.transaction_id is NoneAs per coding guidelines "Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id'))." and "Unit tests should be extensive - test even if we don't expect users to use it currently."Also applies to: 510-517
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c03cbac2-49b3-49a4-b723-d62b2e85dcac
📒 Files selected for processing (1)
tests/unit/get_receipt_query_test.py
132b7f5 to
87703e2
Compare
AntonioCeppellini
left a comment
There was a problem hiding this comment.
Hi @Mounil2005 great work so far!
Do you think we should update also integration tests about it?
01649c3 to
cbcb40a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b1788cd-9ca2-46ee-9743-3bff614b0954
📒 Files selected for processing (2)
test_collection.txttests/integration/transaction_get_receipt_query_e2e_test.py
c300294 to
7fff39c
Compare
cabf5d7 to
32135db
Compare
There was a problem hiding this comment.
The fix looks good , child receipts getting None instead of the parent's transaction_id makes sense, and the tests cover it well.
One thing that stood out: the account_id property behavior changed. It now returns an AccountId(shard=0, realm=0, num=0) instead of None when accountNum == 0. The other ID properties in the same file still check for != 0, so this feels inconsistent. Is that intentional for the EVM use case?
Also, might be worth adding a quick test for the case where accountID isn't set in the protobuf at all, just to document that it returns None.
Otherwise LGTM.
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
32135db to
5eda8b0
Compare
f2df543 to
cbf5d3e
Compare
|
Great catch on the inconsistency @cheese-cakee ! You're absolutely right that account_id differs from token_id, file_id, contract_id, etc. by returning even when num=0. The semantic difference matters:
|
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
… fixes Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
812d9c7 to
aeb33d4
Compare
Adityarya11
left a comment
There was a problem hiding this comment.
A minor suggestion, i have provided though they are completely fine.
This LGTM @exploreriii
Approved!!
|
|
||
| @classmethod | ||
| def _from_proto(cls, proto: transaction_receipt_pb2.TransactionReceipt, transaction_id: TransactionId) -> "TransactionReceipt": | ||
| def _from_proto(cls, proto: transaction_receipt_pb2.TransactionReceipt, transaction_id: TransactionId | None) -> "TransactionReceipt": |
There was a problem hiding this comment.
**non-blocking Issue:** a minor note, the PR description mentions using Optional[TransactionId] but the code uses TransactionId | None. Since we run on Python 3.10+ this is perfectly valid and safe, just pointing it out as a small stylistic difference compared to older files.
Description:
Fix child transaction receipts not exposing account_id field.
Child receipts were incorrectly initialized with the parent's transaction_id instead of None, preventing proper access to protobuf fields like accountID.
Related issue(s):
Fixes #1849
Notes for reviewer:
Root cause: Child receipts created with parent transaction_id instead of None
Fix: TransactionReceipt._from_proto(receipt_proto, None)
Testing:
14/14 unit tests pass, including new verification test for child receipt account_id
Checklist
[] Documented (Type hints updated)
[] Tested (14/14 unit tests pass)