feat: implement _from_protobuf deserialization for all transaction types#2183
feat: implement _from_protobuf deserialization for all transaction types#2183yuval99g wants to merge 36 commits into
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 2 high |
| Complexity | 5 critical 5 medium |
🟢 Metrics 48 complexity
Metric Results Complexity 48
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
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:
WalkthroughImplemented many transaction-specific Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Transaction.from_bytes
participant Router as Transaction._get_transaction_class
participant Proto as TransactionBody (protobuf)
participant Subclass as TransactionSubclass._from_protobuf
participant Instance as Transaction instance
Note over Caller,Router: decode bytes → protobuf transaction body
Caller->>Router: WhichOneof("data") -> subclass mapping
Router->>Subclass: call _from_protobuf(transaction_body, body_bytes, sig_map)
Subclass->>Proto: inspect HasField / fields
Subclass->>Instance: set IDs, keys, amounts, lists, metadata
Subclass-->>Caller: return populated Instance
rect rgba(0,128,255,0.5)
Note right of Instance: Fully hydrated transaction object
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. ✨ 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 |
WhichOneof("data") returns snake_case for newer proto fields, so fix
camelCase keys for token_pause, token_unpause, token_fee_schedule_update,
and token_update_nfts. Add missing entries for tokenClaimAirdrop,
token_fee_schedule_update, and freeze. Fix incorrect module paths.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Add _from_protobuf classmethod to AccountCreateTransaction, AccountUpdateTransaction, AccountDeleteTransaction, AccountAllowanceApproveTransaction, and AccountAllowanceDeleteTransaction so that Transaction.from_bytes() correctly restores all fields. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Add _from_protobuf classmethod to all token transaction types: TokenCreateTransaction, TokenUpdateTransaction, TokenMintTransaction, TokenBurnTransaction, TokenDeleteTransaction, TokenFreezeTransaction, TokenUnfreezeTransaction, TokenGrantKycTransaction, TokenRevokeKycTransaction, TokenWipeTransaction, TokenPauseTransaction, TokenUnpauseTransaction, TokenAssociateTransaction, TokenDissociateTransaction, TokenFeeScheduleUpdateTransaction, TokenAirdropTransaction, TokenClaimAirdropTransaction, TokenCancelAirdropTransaction, TokenRejectTransaction, and TokenUpdateNftsTransaction. Also add HasField guards on all inner message-type fields to avoid silent default-value population. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
…tem transactions Add _from_protobuf classmethod to TopicCreateTransaction, TopicUpdateTransaction, TopicDeleteTransaction, TopicMessageSubmitTransaction, FileCreateTransaction, FileAppendTransaction, FileUpdateTransaction, FileDeleteTransaction, ScheduleCreateTransaction, ScheduleSignTransaction, ScheduleDeleteTransaction, and FreezeTransaction. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Add test_from_protobuf to every transaction test file that was missing deserialization coverage. Each test builds a transaction, serializes it via build_transaction_body(), reconstructs via _from_protobuf(), and asserts all fields are correctly restored. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
yuval99g
force-pushed
the
feat/implement-from-protobuf-deserialization
branch
from
2cf6d6c to
9ed28ba
Compare
…pdate_transaction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Wide proto deserialization methods necessarily have many HasField branches. Suppress PLR0912 rather than splitting them into less-readable helpers. Signed-off-by: Yuval Goihberg <yuval@blockaid.co> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
…tion._from_protobuf Missed in previous commit; wide proto deserialization necessarily has many branches. Signed-off-by: Yuval Goihberg <yuval@blockaid.co> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Local imports inside _from_protobuf were unnecessary; no circular import exists. Moved to top-level for consistency with the rest of the codebase. Signed-off-by: Yuval Goihberg <yuval@blockaid.co> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
|
The |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Reopening and linking to the issue assigned #2179 |
token_airdrop_transaction Signed-off-by: Yuval Goihberg <yuval@blockaid.co> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 37
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/hiero_sdk_python/file/file_create_transaction.py (1)
189-204:⚠️ Potential issue | 🟡 MinorNormalize
contentsbytes and useHasField()for message field presence.Lines 203–204 in
_from_proto():
contents(bytes field):proto.contentsdefaults tob""when unset; SDK models it asbytes | None, requiringproto.contents or Nonenormalization per guidelines.expirationTime(message field): Useproto.HasField("expirationTime")instead of truthiness check to explicitly detect presence.Proposed fix
- self.contents = proto.contents - self.expiration_time = Timestamp._from_protobuf(proto.expirationTime) if proto.expirationTime else None + self.contents = proto.contents or None + self.expiration_time = ( + Timestamp._from_protobuf(proto.expirationTime) + if proto.HasField("expirationTime") + else None + )src/hiero_sdk_python/consensus/topic_message_submit_transaction.py (2)
203-211:⚠️ Potential issue | 🔴 CriticalBLOCKER: Raise a clear error before dereferencing
_initial_transaction_id.For a multi-chunk message, calling
build_transaction_body()beforefreeze_with()leaves_initial_transaction_idasNone, so Line 207 raisesAttributeErrorinstead of a deterministic SDK error.🛡️ Proposed guard
# Multi-chunk metadata if self._total_chunks > 1: + if self._initial_transaction_id is None: + raise ValueError( + "Missing initial transaction ID; call freeze_with() before building a multi-chunk message" + ) body.chunkInfo.CopyFrom( consensus_submit_message_pb2.ConsensusMessageChunkInfo( initialTransactionID=self._initial_transaction_id._to_proto(),As per coding guidelines, “An explicit
if self._initial_transaction_id is Noneguard with a descriptiveValueErrorMUST be present.”
280-282:⚠️ Potential issue | 🔴 CriticalBLOCKER: Carry nanosecond overflow for chunk transaction IDs.
base_timestamp.nanos + ican exceed the valid protobufTimestamp.nanosrange ([0, 999_999_999]) for later chunks. Carry intosecondsbefore constructing the next chunk'svalid_start.⏱️ Proposed fix
else: + total_nanos = base_timestamp.nanos + i chunk_valid_start = timestamp_pb2.Timestamp( - seconds=base_timestamp.seconds, nanos=base_timestamp.nanos + i + seconds=base_timestamp.seconds + total_nanos // 1_000_000_000, + nanos=total_nanos % 1_000_000_000, )src/hiero_sdk_python/consensus/topic_update_transaction.py (1)
35-59:⚠️ Potential issue | 🔴 CriticalBLOCKER: Optional update fields must remain absent when omitted to prevent unintended on-chain mutations.
Proto fields:
memo(field 2,google.protobuf.StringValue) andautoRenewPeriod(field 8,Duration) in the canonicalconsensus_update_topic.protoschema state: "Fields not set on this transaction SHALL NOT be updated."The issue: Constructor defaults cause silent mutation on deserialization:
Line 35:
auto_renew_period: Duration | None = Duration(7890000)is a shared mutable default — all instances constructed without an explicit value share the sameDurationobject.Line 56:
self.memo: str = memo or ""coercesNoneto empty string; the type annotation isstr(neverNone).Deserialization sequence (lines 313–337):
super()._from_protobuf()constructs the transaction, applying__init__defaults:memo="",auto_renew_period=Duration(7890000).HasField(...)guards skip override branches if the proto fields are absent.- Defaults remain in place.
Re-serialization (lines 266, 270):
if self.auto_renew_periodevaluates true (default Duration is truthy) → field is serialized.if self.memo is not Noneevaluates true (empty string is not None) → field is serialized.- Result: fields omitted from the input proto are re-serialized with defaults, unintentionally mutating the topic on-chain.
Proposed fix:
- auto_renew_period: Duration | None = Duration(7890000), + auto_renew_period: Duration | None = None,- self.memo: str = memo or "" + self.memo: str | None = memoIf a user-created transaction requires the 7890000-second default, set it explicitly after construction or provide a factory method. Deserialization must preserve proto absence (not overwrite with defaults).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2bcf120e-cae7-42dc-9d28-aa207d4f7bb4
📒 Files selected for processing (71)
src/hiero_sdk_python/account/account_allowance_approve_transaction.pysrc/hiero_sdk_python/account/account_allowance_delete_transaction.pysrc/hiero_sdk_python/account/account_create_transaction.pysrc/hiero_sdk_python/account/account_delete_transaction.pysrc/hiero_sdk_python/account/account_update_transaction.pysrc/hiero_sdk_python/consensus/topic_create_transaction.pysrc/hiero_sdk_python/consensus/topic_delete_transaction.pysrc/hiero_sdk_python/consensus/topic_message_submit_transaction.pysrc/hiero_sdk_python/consensus/topic_update_transaction.pysrc/hiero_sdk_python/file/file_append_transaction.pysrc/hiero_sdk_python/file/file_create_transaction.pysrc/hiero_sdk_python/file/file_delete_transaction.pysrc/hiero_sdk_python/file/file_update_transaction.pysrc/hiero_sdk_python/schedule/schedule_create_transaction.pysrc/hiero_sdk_python/schedule/schedule_delete_transaction.pysrc/hiero_sdk_python/schedule/schedule_sign_transaction.pysrc/hiero_sdk_python/system/freeze_transaction.pysrc/hiero_sdk_python/tokens/token_airdrop_claim.pysrc/hiero_sdk_python/tokens/token_airdrop_transaction.pysrc/hiero_sdk_python/tokens/token_airdrop_transaction_cancel.pysrc/hiero_sdk_python/tokens/token_associate_transaction.pysrc/hiero_sdk_python/tokens/token_burn_transaction.pysrc/hiero_sdk_python/tokens/token_create_transaction.pysrc/hiero_sdk_python/tokens/token_delete_transaction.pysrc/hiero_sdk_python/tokens/token_dissociate_transaction.pysrc/hiero_sdk_python/tokens/token_fee_schedule_update_transaction.pysrc/hiero_sdk_python/tokens/token_freeze_transaction.pysrc/hiero_sdk_python/tokens/token_grant_kyc_transaction.pysrc/hiero_sdk_python/tokens/token_mint_transaction.pysrc/hiero_sdk_python/tokens/token_pause_transaction.pysrc/hiero_sdk_python/tokens/token_reject_transaction.pysrc/hiero_sdk_python/tokens/token_revoke_kyc_transaction.pysrc/hiero_sdk_python/tokens/token_unfreeze_transaction.pysrc/hiero_sdk_python/tokens/token_unpause_transaction.pysrc/hiero_sdk_python/tokens/token_update_nfts_transaction.pysrc/hiero_sdk_python/tokens/token_update_transaction.pysrc/hiero_sdk_python/tokens/token_wipe_transaction.pysrc/hiero_sdk_python/transaction/transaction.pytests/unit/account_allowance_approve_transaction_test.pytests/unit/account_allowance_delete_transaction_test.pytests/unit/account_create_transaction_test.pytests/unit/account_delete_transaction_test.pytests/unit/account_update_transaction_test.pytests/unit/file_append_transaction_test.pytests/unit/file_create_transaction_test.pytests/unit/file_delete_transaction_test.pytests/unit/file_update_transaction_test.pytests/unit/freeze_transaction_test.pytests/unit/schedule_create_transaction_test.pytests/unit/schedule_delete_transaction_test.pytests/unit/schedule_sign_transaction_test.pytests/unit/token_airdrop_transaction_test.pytests/unit/token_associate_transaction_test.pytests/unit/token_burn_transaction_test.pytests/unit/token_delete_transaction_test.pytests/unit/token_dissociate_transaction_test.pytests/unit/token_fee_schedule_update_transaction_test.pytests/unit/token_freeze_transaction_test.pytests/unit/token_grant_kyc_transaction_test.pytests/unit/token_mint_transaction_test.pytests/unit/token_pause_transaction_test.pytests/unit/token_revoke_kyc_transaction_test.pytests/unit/token_unfreeze_transaction_test.pytests/unit/token_unpause_transaction_test.pytests/unit/token_update_nfts_transaction_test.pytests/unit/token_update_transaction_test.pytests/unit/token_wipe_transaction_test.pytests/unit/topic_create_transaction_test.pytests/unit/topic_delete_transaction_test.pytests/unit/topic_message_submit_transaction_test.pytests/unit/topic_update_transaction_test.py
- Route round-trip tests through the public Transaction.from_bytes() API
to cover the dispatch map, not just the protected _from_protobuf hook
- Rename test_from_protobuf -> test_from_bytes to reflect the tested API
- Expand field coverage: inactive branches, staking fields, fee fields,
allowance types, scheduled body, and key deserialization
- Use freeze() + to_bytes() for proper serialization round-trip
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
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)
tests/unit/token_fee_schedule_update_transaction_test.py (1)
5-9:⚠️ Potential issue | 🟠 MajorExercise
Transaction.from_bytesinstead of calling_from_protobufdirectly.This test bypasses the dispatch map entry for
token_fee_schedule_update, which is one of the fixes this PR needs to protect. Use the public round-trip path so a brokenTransaction._get_transaction_classmapping fails loudly.Suggested change
from hiero_sdk_python.tokens.custom_fixed_fee import CustomFixedFee +from hiero_sdk_python.transaction.transaction import Transaction from hiero_sdk_python.tokens.token_fee_schedule_update_transaction import ( TokenFeeScheduleUpdateTransaction, ) @@ - body = tx.build_transaction_body() - reconstructed = TokenFeeScheduleUpdateTransaction._from_protobuf(body, body.SerializeToString(), None) + reconstructed = Transaction.from_bytes(tx.to_bytes()) + assert isinstance(reconstructed, TokenFeeScheduleUpdateTransaction) assert reconstructed.token_id == token_id_1As per coding guidelines, “PRIORITY 1 - Protect Against Breaking Changes”.
Also applies to: 121-124
♻️ Duplicate comments (5)
tests/unit/token_freeze_transaction_test.py (1)
141-156:⚠️ Potential issue | 🟡 MinorExercise the public round-trip and keep payer/target accounts distinct.
This currently bypasses the
Transaction.from_bytesdispatch path fixed by the PR, so theisinstanceassertion is tautological. It also uses the payer as the freeze target, which can hide account-source mixups. Use the fixture’s distinct freeze account and deserialize through the public round-trip path.Suggested test strengthening
+from hiero_sdk_python.transaction.transaction import Transaction + def test_from_protobuf(mock_account_ids): """Test round-trip via _from_protobuf for TokenFreezeTransaction.""" - account_id_sender, _, node_account_id, token_id_1, _ = mock_account_ids + account_id_sender, freeze_id, node_account_id, token_id_1, _ = mock_account_ids tx = TokenFreezeTransaction() tx.set_token_id(token_id_1) - tx.set_account_id(account_id_sender) + tx.set_account_id(freeze_id) tx.transaction_id = generate_transaction_id(account_id_sender) tx.node_account_id = node_account_id - body = tx.build_transaction_body() - reconstructed = TokenFreezeTransaction._from_protobuf(body, body.SerializeToString(), None) + reconstructed = Transaction.from_bytes(tx.to_bytes()) assert isinstance(reconstructed, TokenFreezeTransaction) assert reconstructed.token_id == token_id_1 - assert reconstructed.account_id == account_id_sender + assert reconstructed.account_id == freeze_idAs per coding guidelines, tests should “Protect Against Breaking Changes” and “Unit tests protect our future selves - be defensive and forward-looking.”
tests/unit/topic_update_transaction_test.py (1)
143-170:⚠️ Potential issue | 🟠 MajorUse the public round-trip path and cover the remaining restored fields.
Line 163 still bypasses
Transaction.from_bytes(...), so a brokenconsensusUpdateTopicdispatch entry would not fail this test. This also still omits restored fields such asfee_schedule_key,custom_fees,fee_exempt_keys, andexpiration_time.Suggested direction
+from hiero_sdk_python.transaction.transaction import Transaction +from hiero_sdk_python.transaction.transaction_id import TransactionIddef test_from_protobuf(mock_account_ids, topic_id): @@ tx.set_auto_renew_account(auto_renew_account) + tx.set_fee_schedule_key(admin_key) + tx.set_fee_exempt_keys([admin_key]) + tx.set_custom_fees([CustomFixedFee(1000, fee_collector_account_id=auto_renew_account)]) tx.operator_account_id = AccountId(0, 0, 2) tx.node_account_id = node_account_id + tx.transaction_id = TransactionId.generate(tx.operator_account_id) + tx.freeze() - body = tx.build_transaction_body() - reconstructed = TopicUpdateTransaction._from_protobuf(body, body.SerializeToString(), None) + reconstructed = Transaction.from_bytes(tx.to_bytes()) + assert isinstance(reconstructed, TopicUpdateTransaction) assert reconstructed.topic_id == topic_id @@ assert reconstructed.auto_renew_period == auto_renew_period assert reconstructed.auto_renew_account == auto_renew_account + assert reconstructed.fee_schedule_key == admin_key + assert reconstructed.fee_exempt_keys == [admin_key] + assert len(reconstructed.custom_fees) == 1As per coding guidelines, “Unit tests should be extensive - test even if we don't expect users to use it currently” and “Assert return types where relevant.”
tests/unit/topic_create_transaction_test.py (1)
580-604:⚠️ Potential issue | 🟠 MajorRound-trip through
Transaction.from_bytes(...)and assert the fee/key fields too.Line 598 still calls the concrete helper directly, so the test does not protect the public dispatch map. It also leaves
fee_schedule_key,custom_fees, andfee_exempt_keysunasserted even though_from_protobufrestores them.Suggested direction
+from hiero_sdk_python.transaction.transaction import Transaction +from hiero_sdk_python.transaction.transaction_id import TransactionId-def test_from_protobuf(mock_account_ids): +def test_from_protobuf(mock_account_ids, custom_fixed_fee): @@ admin_key = PrivateKey.generate_ed25519().public_key() submit_key = PrivateKey.generate_ed25519().public_key() + fee_schedule_key = PrivateKey.generate_ed25519().public_key() auto_renew_period = Duration(8000000) @@ tx.set_submit_key(submit_key) tx.set_auto_renew_period(auto_renew_period) + tx.set_fee_schedule_key(fee_schedule_key) + tx.set_custom_fees([custom_fixed_fee]) + tx.set_fee_exempt_keys([admin_key]) tx.operator_account_id = account_id_sender tx.node_account_id = node_account_id + tx.transaction_id = TransactionId.generate(account_id_sender) + tx.freeze() - body = tx.build_transaction_body() - reconstructed = TopicCreateTransaction._from_protobuf(body, body.SerializeToString(), None) + reconstructed = Transaction.from_bytes(tx.to_bytes()) + assert isinstance(reconstructed, TopicCreateTransaction) assert reconstructed.memo == "hello" @@ assert reconstructed.submit_key == submit_key assert reconstructed.auto_renew_period == auto_renew_period + assert reconstructed.fee_schedule_key == fee_schedule_key + assert reconstructed.fee_exempt_keys == [admin_key] + assert len(reconstructed.custom_fees) == 1As per coding guidelines, “Unit tests should be extensive - test even if we don't expect users to use it currently” and “Assert return types where relevant.”
tests/unit/token_airdrop_transaction_test.py (1)
409-427:⚠️ Potential issue | 🟠 MajorAdd NFT, approval, and expected-decimal coverage to this round-trip.
This now uses the public path, but it only proves basic fungible transfers. A regression dropping
expected_decimals,is_approved, or NFT transfers would still pass.Suggested expansion
def test_from_bytes(mock_account_ids): """Test round-trip via _from_protobuf for TokenAirdropTransaction.""" - sender, receiver, node_account_id, token_id_1, _ = mock_account_ids + sender, receiver, node_account_id, token_id_1, token_id_2 = mock_account_ids + nft_id = NftId(token_id_2, 1) tx = TokenAirdropTransaction() - tx.add_token_transfer(token_id=token_id_1, account_id=sender, amount=-1) - tx.add_token_transfer(token_id=token_id_1, account_id=receiver, amount=1) + tx.add_approved_token_transfer_with_decimals(token_id=token_id_1, account_id=sender, amount=-1, decimals=2) + tx.add_approved_token_transfer_with_decimals(token_id=token_id_1, account_id=receiver, amount=1, decimals=2) + tx.add_approved_nft_transfer(nft_id=nft_id, sender_id=sender, receiver_id=receiver) @@ assert len(reconstructed.token_transfers[token_id_1]) == 2 assert reconstructed.token_transfers[token_id_1][0].account_id == sender assert reconstructed.token_transfers[token_id_1][0].amount == -1 + assert reconstructed.token_transfers[token_id_1][0].expected_decimals == 2 + assert reconstructed.token_transfers[token_id_1][0].is_approved is True assert reconstructed.token_transfers[token_id_1][1].account_id == receiver assert reconstructed.token_transfers[token_id_1][1].amount == 1 + assert reconstructed.token_transfers[token_id_1][1].expected_decimals == 2 + assert reconstructed.token_transfers[token_id_1][1].is_approved is True + assert len(reconstructed.nft_transfers[token_id_2]) == 1 + assert reconstructed.nft_transfers[token_id_2][0].sender_id == sender + assert reconstructed.nft_transfers[token_id_2][0].receiver_id == receiver + assert reconstructed.nft_transfers[token_id_2][0].serial_number == 1 + assert reconstructed.nft_transfers[token_id_2][0].is_approved is TrueAs per coding guidelines, “Unit tests should be extensive - test even if we don't expect users to use it currently.”
tests/unit/account_update_transaction_test.py (1)
820-844: 🧹 Nitpick | 🔵 TrivialExtend round-trip coverage to
key,auto_renew_period,expiration_time, and thestaked_account_idoneof branch.Good to see this now exercises the public
Transaction.from_bytesdispatch and theFalse-valued wrappers. However, the round-trip still omits several fields that_from_protobufexplicitly restores (key,auto_renew_period,expiration_time, and thestaked_account_idarm of thestaked_idoneof). A regression in any of those — e.g., thestaked_idWhichOneofbranch mis-handlingstaked_account_id, orHasField("key")being dropped — would not be caught here.Consider either extending this test or adding a second parametrized case for the
staked_account_idbranch.🧪 Suggested additional coverage
def test_from_bytes(mock_account_ids): """Test round-trip via _from_protobuf for AccountUpdateTransaction.""" operator_id, _, node_account_id, _, _ = mock_account_ids account_id_sender = AccountId(0, 0, 1) + public_key = PrivateKey.generate().public_key() tx = AccountUpdateTransaction() tx.set_account_id(account_id_sender) + tx.set_key(public_key) + tx.set_auto_renew_period(TEST_AUTO_RENEW_PERIOD) + tx.set_expiration_time(TEST_EXPIRATION_TIME) tx.set_account_memo("updated") tx.set_staked_node_id(5) tx.set_decline_staking_reward(False) tx.set_receiver_signature_required(False) tx.set_max_automatic_token_associations(10) tx.transaction_id = TransactionId.generate(operator_id) tx.node_account_id = node_account_id tx.freeze() reconstructed = Transaction.from_bytes(tx.to_bytes()) assert isinstance(reconstructed, AccountUpdateTransaction) assert reconstructed.account_id == account_id_sender + assert reconstructed.key == public_key + assert reconstructed.auto_renew_period == TEST_AUTO_RENEW_PERIOD + assert reconstructed.expiration_time == TEST_EXPIRATION_TIME assert reconstructed.account_memo == "updated" assert reconstructed.staked_node_id == 5 + assert reconstructed.staked_account_id is None assert reconstructed.decline_staking_reward == False assert reconstructed.receiver_signature_required == False assert reconstructed.max_automatic_token_associations == 10A second test setting
staked_account_id(and assertingstaked_node_id is None) would also lock down the opposite oneof arm.As per coding guidelines, unit tests should be "extensive" and "protect against breaking changes" by covering the full deserialization surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a16f008a-b052-431b-9be6-a28b35d92f83
📒 Files selected for processing (21)
tests/unit/account_allowance_approve_transaction_test.pytests/unit/account_allowance_delete_transaction_test.pytests/unit/account_create_transaction_test.pytests/unit/account_update_transaction_test.pytests/unit/file_append_transaction_test.pytests/unit/file_update_transaction_test.pytests/unit/freeze_transaction_test.pytests/unit/schedule_create_transaction_test.pytests/unit/token_airdrop_transaction_test.pytests/unit/token_associate_transaction_test.pytests/unit/token_burn_transaction_test.pytests/unit/token_delete_transaction_test.pytests/unit/token_fee_schedule_update_transaction_test.pytests/unit/token_freeze_transaction_test.pytests/unit/token_revoke_kyc_transaction_test.pytests/unit/token_update_nfts_transaction_test.pytests/unit/token_update_transaction_test.pytests/unit/token_wipe_transaction_test.pytests/unit/topic_create_transaction_test.pytests/unit/topic_message_submit_transaction_test.pytests/unit/topic_update_transaction_test.py
Transaction.from_bytes()
- Route round-trip tests through the public Transaction.from_bytes() API
to cover the dispatch map, not just the protected _from_protobuf hook
- Rename test_from_protobuf -> test_from_bytes to reflect the tested API
- Expand field coverage: inactive branches, staking fields, fee fields,
allowance types, scheduled body, and key deserialization
- Use freeze() + to_bytes() for proper serialization round-trip
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
|
|
@aceppaluni fixed and resolved all |
There was a problem hiding this comment.
Actionable comments posted: 1
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/account/account_create_transaction.py (1)
317-327:⚠️ Potential issue | 🔴 CriticalBLOCKER: Preserve
hook_creation_detailsduring account-create round-trips.Proto field:
CryptoCreateTransactionBody.hook_creation_details(#19) inservices/crypto_create.proto.
Issue type: Missing field / Asymmetric round-trip._from_protobuf()drops this non-deprecated repeated field, and_build_proto_body()cannot serialize it back, soTransaction.from_bytes(...).to_bytes()changes the signed body for account-create transactions that include hooks.Suggested fix direction
class AccountCreateTransaction(Transaction): @@ self.decline_staking_reward = decline_staking_reward + self.hook_creation_details = [] @@ decline_reward=self.decline_staking_reward, ) + proto_body.hook_creation_details.extend(self.hook_creation_details) @@ elif staked_id == "staked_node_id": transaction.staked_node_id = body.staked_node_id + transaction.hook_creation_details = list(body.hook_creation_details) return transactionIf raw proto messages are not an intended public API, add an SDK wrapper first, but the repeated field should still default to
[]and round-trip without loss.Also applies to: 375-397
♻️ Duplicate comments (1)
tests/unit/account_allowance_approve_transaction_test.py (1)
384-388:⚠️ Potential issue | 🟡 MinorTighten NFT allowance assertions.
inonly checks presence; a future bug appending duplicate serials or changingapproved_for_alldefault would not be caught. Prefer equality onserial_numbersand an explicitapproved_for_allcheck.🧪 Suggested tightening
- assert nft_id.serial_number in reconstructed.nft_allowances[0].serial_numbers + assert reconstructed.nft_allowances[0].serial_numbers == [nft_id.serial_number] + assert reconstructed.nft_allowances[0].approved_for_all is FalseAs per coding guidelines: "Unit tests should be extensive - test even if we don't expect users to use it currently" and "Avoid brittle ordering assertions unless order is part of the contract" — serial number order is part of the wire contract here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c71e9da-4598-47fc-9df1-5bae91d63fff
📒 Files selected for processing (18)
src/hiero_sdk_python/account/account_create_transaction.pysrc/hiero_sdk_python/account/account_update_transaction.pysrc/hiero_sdk_python/consensus/topic_create_transaction.pysrc/hiero_sdk_python/consensus/topic_update_transaction.pysrc/hiero_sdk_python/file/file_append_transaction.pysrc/hiero_sdk_python/file/file_update_transaction.pysrc/hiero_sdk_python/schedule/schedule_create_transaction.pysrc/hiero_sdk_python/tokens/custom_fixed_fee.pysrc/hiero_sdk_python/tokens/token_airdrop_claim.pysrc/hiero_sdk_python/tokens/token_update_transaction.pysrc/hiero_sdk_python/tokens/token_wipe_transaction.pysrc/hiero_sdk_python/transaction/transaction.pytests/unit/account_allowance_approve_transaction_test.pytests/unit/file_create_transaction_test.pytests/unit/file_update_transaction_test.pytests/unit/token_revoke_kyc_transaction_test.pytests/unit/token_update_transaction_test.pytests/unit/token_wipe_transaction_test.py
… transactions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
yuval99g
force-pushed
the
feat/implement-from-protobuf-deserialization
branch
from
89a61a4 to
0f9b3e3
Compare
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
|
Hi @yuval99g, 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 |
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
| Complexity | 5 critical 5 medium |
🟢 Metrics 5 complexity
Metric Results Complexity 5
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
github-actions
Bot
added
the
queue:junior-committer
exploreriii
removed
step: 1st
github-actions
Bot
added
the
open to community review
|
Hii @yuval99g Codacy is Failing logs https://github.com/hiero-ledger/hiero-sdk-python/pull/2183/checks?check_run_id=75217567004 cc @exploreriii Please move to draft (still in progress) |
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
|
@tech0priyanshu can you dismiss all the cyclomatic complexity issues? The _from_protobuf deserialization methods necessarily have high cyclomatic complexity — each HasField guard is a required branch to avoid overwriting unset fields with empty defaults. Splitting these methods would hurt readability without any real benefit, since all the branches operate on the same object and belong together conceptually. Suppressed with # noqa: PLR0912 rather than refactoring into artificial helpers. |
|
@manishdait Can I get your input on this? |
Agree with this, for this PR we can ignore the the cyclomatic complexity. |
Signed-off-by: Yuval Goihberg <yuval@blockaid.co>
|
@manishdait Are we able to dismiss codacy somehow? |
|
you can still merge despite codacy failures if you really want @aceppaluni , or you can log in to codacy and change its settings on how strict it is. You can also tell it to ignore certain things in the code |
manishdait
left a comment
manishdait
left a comment
There was a problem hiding this comment.
@yuval99g, Sorry for late review, just added some review changes.
| transaction.token_id = TokenId._from_proto(body.token) | ||
| if body.HasField("account"): | ||
| transaction.account_id = AccountId._from_proto(body.account) | ||
| transaction.amount = body.amount |
There was a problem hiding this comment.
we can add a none check for the amount as well
| def _from_protobuf(cls, transaction_body, body_bytes: bytes, sig_map): | ||
| transaction = super()._from_protobuf(transaction_body, body_bytes, sig_map) | ||
| if transaction_body.HasField("fileCreate"): | ||
| transaction._from_proto(transaction_body.fileCreate) |
There was a problem hiding this comment.
Non-Blocking:
Not sure about using _from_proto method here cause In some places it is implemented as a @classmethod eg.TokenDissociationTransaction, while in others it's an instance method like this class.
I think _from_proto might make more sense as a @classmethod, if I am not wrong Any thoughts on this @Dosik13?
There was a problem hiding this comment.
but here its classmethod and not instance method
There was a problem hiding this comment.
@manishdait Would it be a class method if the values are updated and not pre-filled?
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@yuval99g I apologize for the delay. This is looking really good.
Happy to review again when suggestions are added.
Regarding Codacy:
Codacy has been removed. Once you push the changes you shouldn't see that check anymore.
aceppaluni
added
the
status: Needs Developer Revision
7 participants
Fixes #2179
Summary
Transaction.from_bytes() silently returned an incomplete object for almost all transaction types. The deserialization chain (from_bytes() → _get_transaction_class() →
_from_protobuf()) worked at the base class level, but concrete subclasses never implemented _from_protobuf, so the result was a correctly-typed instance with all transaction-specific
fields at their default None/empty values — no error, no warning.
This PR fixes the issue by:
token_unpause, token_fee_schedule_update, token_update_nfts), missing entries for tokenClaimAirdrop, token_fee_schedule_update, and freeze, and incorrect module paths
Test plan