feat: Expose all missing protobuf fields in TransactionRecord #1781
Conversation
gangulysiddhartha22-cmyk
requested review from
AntonioCeppellini and
rbair23
|
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! |
|
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 a new public TokenAssociation dataclass and extends TransactionRecord with several protobuf-backed fields (timestamps, schedule ref, assessed custom fees, automatic token associations, parent timestamp, alias/ethereum/evm data, paid staking rewards, contract create result). Tests and an example script were added; proto (de)serialization and repr updated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 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/transaction/transaction_record.py (1)
38-64: 🧹 Nitpick | 🔵 TrivialClass docstring does not document the 10 new fields.
The
Attributessection in the docstring still only lists the original fields. Users relying on docstrings or IDE tooltips won't discoverconsensus_timestamp,schedule_ref,assessed_custom_fees, etc.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1781 +/- ##
==========================================
+ Coverage 93.56% 93.61% +0.04%
==========================================
Files 141 142 +1
Lines 9215 9297 +82
==========================================
+ Hits 8622 8703 +81
- Misses 593 594 +1 🚀 New features to boost your workflow:
|
|
@exploreriii Should I add some usage examples for these new TransactionRecord fields? |
|
@exploreriii @manishdait All review comments addressed: added docstring examples, improved coverage for new fields/repr/round-trips, fixed required fields. Ready for review |
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)
366-376: 🧹 Nitpick | 🔵 Trivial
call_resultandcontract_create_resultshare a proto oneof — last-write-wins in_to_proto.In the Hedera
TransactionRecordprotobuf,contractCallResultandcontractCreateResultare in aoneof body. The constructor at line 371 setscontractCallResult, and line 431 may overwrite it withcontractCreateResultviaCopyFrom. If both fields are non-Noneon the dataclass, the call result is silently lost.In practice the network never populates both, so this is low-risk, but adding a guard would prevent future confusion.
🛡️ Optional: add a defensive check
+ if self.call_result is not None and self.contract_create_result is not None: + raise ValueError( + "call_result and contract_create_result are mutually exclusive (proto oneof)" + ) + if self.contract_create_result is not None: record_proto.contractCreateResult.CopyFrom(self.contract_create_result._to_proto())Also applies to: 431-432
tests/unit/transaction_record_test.py (1)
100-111: 🧹 Nitpick | 🔵 TrivialConsider adding assertions for new
Optional[bytes]fields when parsing a minimal proto.
test_from_protovalidates the existing fields fromproto_transaction_recordbut doesn't assert that the new bytes fields (alias,ethereum_hash,evm_address) areNoneafter parsing a proto where they are unset. This would verify theproto.alias or Nonenormalization path end-to-end.Suggested additions
assert record.prng_number == 100 assert record.prng_bytes == b"" + assert record.alias is None, "Unset proto alias should normalize to None" + assert record.ethereum_hash is None, "Unset proto ethereum_hash should normalize to None" + assert record.evm_address is None, "Unset proto evm_address should normalize to None"As per coding guidelines, "Unit tests should be extensive - test even if we don't expect users to use it currently."
gangulysiddhartha22-cmyk
force-pushed
the
feature/add-missing-fields-to-transactionrecord
branch
from
7d610b2 to
af88ea9
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 (1)
src/hiero_sdk_python/transaction/transaction_record.py (1)
357-381:⚠️ Potential issue | 🟠 Major
call_resultis serialized twice — redundantCopyFromon Line 376-377.The constructor at Line 362-364 already sets
contractCallResultvia the keyword argument. TheCopyFromat Line 376-377 overwrites it with the same value. Remove one of the two to avoid calling_to_proto()twice.♻️ Proposed fix — remove redundant CopyFrom and move guard before constructor
+ # Defensive guard for protobuf oneof + if self.call_result is not None and self.contract_create_result is not None: + raise ValueError( + "call_result and contract_create_result are mutually exclusive " + "(protobuf oneof) — only one may be set at a time" + ) + record_proto = transaction_record_pb2.TransactionRecord( transactionHash=self.transaction_hash, memo=self.transaction_memo, transactionFee=self.transaction_fee, receipt=self.receipt._to_proto() if self.receipt else None, contractCallResult=( self.call_result._to_proto() if self.call_result else None ), prng_number=self.prng_number, prng_bytes=self.prng_bytes, ) - - # Defensive guard for protobuf oneof - if self.call_result is not None and self.contract_create_result is not None: - raise ValueError( - "call_result and contract_create_result are mutually exclusive " - "(protobuf oneof) — only one may be set at a time" - ) - - if self.call_result is not None: - record_proto.contractCallResult.CopyFrom(self.call_result._to_proto()) if self.contract_create_result is not None: record_proto.contractCreateResult.CopyFrom(self.contract_create_result._to_proto())
gangulysiddhartha22-cmyk
force-pushed
the
feature/add-missing-fields-to-transactionrecord
branch
from
af88ea9 to
2107434
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hiero_sdk_python/transaction/transaction_record.py (1)
357-377:⚠️ Potential issue | 🟠 Major
call_resultis serialized twice — once in the constructor (Line 362-364) and again viaCopyFrom(Line 376-377).The constructor already sets
contractCallResultfromself.call_result._to_proto(), then lines 376-377 overwrite it with an identicalCopyFrom. This calls_to_proto()on the same object twice for no benefit. Additionally, the mutual exclusivity check at lines 370-374 runs after the constructor already assignedcontractCallResult, which is logically out of order.🐛 Proposed fix — remove the duplicate and reorder the guard
+ # Defensive guard for protobuf oneof + if self.call_result is not None and self.contract_create_result is not None: + raise ValueError( + "call_result and contract_create_result are mutually exclusive " + "(protobuf oneof) — only one may be set at a time" + ) + record_proto = transaction_record_pb2.TransactionRecord( transactionHash=self.transaction_hash, memo=self.transaction_memo, transactionFee=self.transaction_fee, receipt=self.receipt._to_proto() if self.receipt else None, - contractCallResult=( - self.call_result._to_proto() if self.call_result else None - ), prng_number=self.prng_number, prng_bytes=self.prng_bytes, ) - - # Defensive guard for protobuf oneof - if self.call_result is not None and self.contract_create_result is not None: - raise ValueError( - "call_result and contract_create_result are mutually exclusive " - "(protobuf oneof) — only one may be set at a time" - ) if self.call_result is not None: record_proto.contractCallResult.CopyFrom(self.call_result._to_proto())
gangulysiddhartha22-cmyk
force-pushed
the
feature/add-missing-fields-to-transactionrecord
branch
from
2107434 to
ed7e895
Compare
AntonioCeppellini
left a comment
AntonioCeppellini
left a comment
There was a problem hiding this comment.
This is looking great! I agree with Manish about the docstrings🚀👀
aceppaluni
added
status: needs developer revision
and removed
status: needs committer review
labels
There was a problem hiding this comment.
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)
81-98: 🧹 Nitpick | 🔵 TrivialConsider adding
hasattrchecks for new fields in the default initialization test.The existing
test_transaction_record_default_initializationvalidates pre-existing fields but doesn't assert the presence of new fields. Whiletest_transaction_record_default_new_fieldscovers default values, addinghasattrchecks here would protect the public API surface against accidental removal, consistent with the pattern used forduplicatesat line 95.As per coding guidelines, "Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id'))."♻️ Proposed addition after line 98
assert record.duplicates == [] + # New field existence checks + assert hasattr(record, 'consensus_timestamp'), "Should have consensus_timestamp" + assert hasattr(record, 'parent_consensus_timestamp'), "Should have parent_consensus_timestamp" + assert hasattr(record, 'schedule_ref'), "Should have schedule_ref" + assert hasattr(record, 'assessed_custom_fees'), "Should have assessed_custom_fees" + assert hasattr(record, 'automatic_token_associations'), "Should have automatic_token_associations" + assert hasattr(record, 'alias'), "Should have alias" + assert hasattr(record, 'ethereum_hash'), "Should have ethereum_hash" + assert hasattr(record, 'evm_address'), "Should have evm_address" + assert hasattr(record, 'paid_staking_rewards'), "Should have paid_staking_rewards" + assert hasattr(record, 'contract_create_result'), "Should have contract_create_result"
Ready for another look! |
|
@exploreriii, |
gangulysiddhartha22-cmyk
force-pushed
the
feature/add-missing-fields-to-transactionrecord
branch
from
563bb6b to
c8274a4
Compare
|
Hi @exploreriii, First of all, really sorry for the delayed reply. In the latest commit I have addressed all these review comments:
Ready for another look! |
|
Hi @gangulysiddhartha22-cmyk note the outstanidng merge conflicts please! |
|
great please do click the ready to review button when it is :) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resume |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/hiero_sdk_python/transaction/transaction_record.py (2)
357-367:⚠️ Potential issue | 🟠 MajorAdd an explicit oneof guard for
contractCallResult/contractCreateResult.Line 362 and Line 425 can both be driven by object state without a pre-check. These are proto oneof
bodybranches (contractCallResultfield 7,contractCreateResultfield 8), so the current flow can silently override one branch during serialization.🐛 Suggested fix
+ if self.call_result is not None and self.contract_create_result is not None: + raise ValueError( + "contract_call_result and contract_create_result are mutually exclusive" + ) + record_proto = transaction_record_pb2.TransactionRecord( transactionHash=self.transaction_hash, memo=self.transaction_memo, transactionFee=self.transaction_fee, receipt=self.receipt._to_proto() if self.receipt else None, - contractCallResult=( - self.call_result._to_proto() if self.call_result else None - ), prng_number=self.prng_number, prng_bytes=self.prng_bytes, ) + + if self.call_result is not None: + record_proto.contractCallResult.CopyFrom(self.call_result._to_proto()) @@ if self.contract_create_result is not None: record_proto.contractCreateResult.CopyFrom(self.contract_create_result._to_proto())#!/bin/bash # Verify oneof branches and absence/presence of mutual-exclusion guard in serializer rg -n -C3 'contractCallResult|contractCreateResult|mutually exclusive|oneof' src/hiero_sdk_python/transaction/transaction_record.pyAs per coding guidelines, "Verify that
_to_protodoes NOT set more than one field from the sameoneofblock simultaneously. If it does, aValueErrorguard MUST be present before setting those fields."Also applies to: 424-425
21-22: 🛠️ Refactor suggestion | 🟠 MajorNew annotations should use
X | Noneinstead ofOptional[X].The newly added typing in this file uses
Optional[...](e.g., Line 99-Line 108), which conflicts with the repository typing rule for modified SDK code.#!/bin/bash # Verify Optional usage in modified annotations rg -n 'Optional\[(Timestamp|ScheduleId|bytes|ContractFunctionResult)\]' src/hiero_sdk_python/transaction/transaction_record.pyAs per coding guidelines, "The codebase uses
X | Noneunion syntax (Python 3.10+). Flag any use ofOptional[X]fromtypingin newly added or modified code, as it is inconsistent with the established style."Also applies to: 99-108
tests/unit/transaction_record_test.py (1)
281-286:⚠️ Potential issue | 🟡 MinorAdd explicit SDK type assertions for parsed new-field entries.
These tests validate values, but not concrete parsed types (
AssessedCustomFee,TokenAssociation). Type assertions would better protect API-shape compatibility.🧪 Suggested additions
record = TransactionRecord._from_proto(proto, transaction_id) assert len(record.assessed_custom_fees) == 1 f = record.assessed_custom_fees[0] + assert isinstance(f, AssessedCustomFee) assert f.amount == 500000000 assert f.fee_collector_account_id.num == 98 @@ record = TransactionRecord._from_proto(proto, transaction_id) assert len(record.automatic_token_associations) == 1 a = record.automatic_token_associations[0] + assert isinstance(a, TokenAssociation) assert a.token_id.num == 5678 assert a.account_id.num == 1234As per coding guidelines, "Assert return types where relevant (e.g.,
assert isinstance(result, AccountId))."Also applies to: 299-304
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a2ef771-236c-413f-bdf4-309fb2ec910b
📒 Files selected for processing (5)
examples/transaction/transaction_record.pysrc/hiero_sdk_python/tokens/token_association.pysrc/hiero_sdk_python/transaction/transaction_record.pytests/unit/token_association_test.pytests/unit/transaction_record_test.py
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hiero_sdk_python/transaction/transaction_record.py (1)
357-372:⚠️ Potential issue | 🟠 MajorAdd
entropyoneof guard to prevent simultaneous setting ofprng_numberandprng_bytes.The
entropyoneof (proto fields 19, 20) is not guarded in_to_proto(). Line 370-371 sets both fields directly in the constructor, violating protobuf oneof semantics. Add a guard before the constructor call and conditional field assignment afterward, mirroring thecontractCallResult/contractCreateResultpattern (lines 357–361, 429–430).In
_from_proto()(lines 245-246), useproto.WhichOneof("entropy")to detect which branch is populated instead of assigning both fields unconditionally.Suggested fix
return cls( + entropy_case = proto.WhichOneof("entropy") + + return cls( @@ - prng_number=proto.prng_number, - prng_bytes=proto.prng_bytes, + prng_number=proto.prng_number if entropy_case == "prng_number" else None, + prng_bytes=proto.prng_bytes if entropy_case == "prng_bytes" else None, @@ if self.call_result is not None and self.contract_create_result is not None: raise ValueError( "contractCallResult and contractCreateResult are mutually exclusive " "proto oneof fields and cannot both be set simultaneously." ) + if self.prng_number is not None and self.prng_bytes is not None: + raise ValueError( + "prng_number and prng_bytes are mutually exclusive " + "proto oneof fields and cannot both be set simultaneously." + ) record_proto = transaction_record_pb2.TransactionRecord( @@ - prng_number=self.prng_number, - prng_bytes=self.prng_bytes, ) + if self.prng_number is not None: + record_proto.prng_number = self.prng_number + if self.prng_bytes is not None: + record_proto.prng_bytes = self.prng_bytes
♻️ Duplicate comments (1)
src/hiero_sdk_python/tokens/token_association.py (1)
2-3:⚠️ Potential issue | 🟡 MinorUse
X | Nonesyntax for newly added type hints.Line 2 and Lines 23/26 use
Optional[...]in new code; project style requiresX | Nonefor modified SDK annotations.♻️ Suggested change
-from typing import Optional @@ - token_id: Optional[TokenId] = None + token_id: TokenId | None = None @@ - account_id: Optional[AccountId] = None + account_id: AccountId | None = None#!/bin/bash # Verify Optional usage in this newly added SDK file. rg -n 'Optional\[' src/hiero_sdk_python/tokens/token_association.pyAs per coding guidelines,
src/hiero_sdk_python/**/*.py: “The codebase usesX | Noneunion syntax (Python 3.10+). Flag any use ofOptional[X]in newly added or modified code.”Also applies to: 23-26
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65ddf81b-9c00-4f5b-bd0f-e3949afac415
📒 Files selected for processing (6)
CHANGELOG.mdexamples/transaction/transaction_record.pysrc/hiero_sdk_python/tokens/token_association.pysrc/hiero_sdk_python/transaction/transaction_record.pytests/unit/token_association_test.pytests/unit/transaction_record_test.py
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c974bb1-dc89-4f3a-91be-6b0d87524f0d
📒 Files selected for processing (6)
CHANGELOG.mdexamples/transaction/transaction_record.pysrc/hiero_sdk_python/tokens/token_association.pysrc/hiero_sdk_python/transaction/transaction_record.pytests/unit/token_association_test.pytests/unit/transaction_record_test.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/hiero_sdk_python/tokens/token_association.py (1)
2-3:⚠️ Potential issue | 🟡 MinorUse
X | Noneannotations in newly added SDK code.This new class uses
Optional[...]in changed lines, but the repository guideline for new/modified code expectsX | None.♻️ Proposed fix
-from typing import Optional @@ - token_id: Optional[TokenId] = None + token_id: TokenId | None = None @@ - account_id: Optional[AccountId] = None + account_id: AccountId | None = NoneAs per coding guidelines, "
X | Noneunion syntax (Python 3.10+) is required in newly added or modified code; flagOptional[X]."Also applies to: 23-26
src/hiero_sdk_python/transaction/transaction_record.py (1)
99-108:⚠️ Potential issue | 🟡 MinorUpdate type annotations to use
X | Nonestyle per codebase standards.The newly added fields at lines 99–108 use
Optional[...]instead of the establishedX | Noneunion syntax required for new/modified code. Update all eight field annotations to follow the codebase pattern.♻️ Proposed fix
- consensus_timestamp: Optional[Timestamp] = None - schedule_ref: Optional[ScheduleId] = None + consensus_timestamp: Timestamp | None = None + schedule_ref: ScheduleId | None = None @@ - parent_consensus_timestamp: Optional[Timestamp] = None - alias: Optional[bytes] = None - ethereum_hash: Optional[bytes] = None + parent_consensus_timestamp: Timestamp | None = None + alias: bytes | None = None + ethereum_hash: bytes | None = None @@ - evm_address: Optional[bytes] = None - contract_create_result: Optional[ContractFunctionResult] = None + evm_address: bytes | None = None + contract_create_result: ContractFunctionResult | None = None
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3a11cd79-2882-42a8-9678-132e7886eb38
📒 Files selected for processing (6)
CHANGELOG.mdexamples/transaction/transaction_record.pysrc/hiero_sdk_python/tokens/token_association.pysrc/hiero_sdk_python/transaction/transaction_record.pytests/unit/token_association_test.pytests/unit/transaction_record_test.py
There was a problem hiding this comment.
seems good to me, need seconds opinion @exploreriii, @AntonioCeppellini
|
Hi, this is WorkflowBot.
|
|
I think, based on the failing tests, it would be better to keep |
|
But this might, break the backward compatiblity like to hear your thoughts @exploreriii |
|
@manishdait Please could you run the workflows again? |
|
@manishdait ,just fixed the PRNG tests by matching real network behavior (prng_number vs prng_bytes None cases) + cleaned up alias config. |
AntonioCeppellini
left a comment
AntonioCeppellini
left a comment
There was a problem hiding this comment.
@gangulysiddhartha22-cmyk @manishdait seems good to me, nice job :D
|
@gangulysiddhartha22-cmyk could you add eg changelog addition being explicit on your changes to e.g. |
…h example Signed-off-by: Siddhartha Ganguly <gangulysiddhartha22@gmail.com> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: Siddhartha Ganguly <gangulysiddhartha22@gmail.com>
|
@gangulysiddhartha22-cmyk Thank you for the extensive effort you've put into this PR over the last few weeks. We recognize the importance of exposing these missing However after 140 plus comments multiple pushes, and shifting logic and inconsistencies, this PR has become too fragmented to maintain or review effectively. The current state contains several inconsistencies |
7 participants
Description:
Expose all previously dropped fields from the
TransactionRecordprotobuf message in the Python SDK.Related issue(s):
Fixes #1636
Notes for reviewer:
Restoration of dropped protobuf fields.
Checklist