feat: Improve to_bytes/from_bytes serialization and deserialization#2390
feat: Improve to_bytes/from_bytes serialization and deserialization#2390manishdait wants to merge 17 commits into
Conversation
github-actions
Bot
added
the
skill: advanced
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2390 +/- ##
==========================================
+ Coverage 95.01% 95.07% +0.06%
==========================================
Files 164 164
Lines 10464 10547 +83
==========================================
+ Hits 9942 10028 +86
+ Misses 522 519 -3 🚀 New features to boost your workflow:
|
manishdait
added
the
notes: extra care
03e8ca7 to
a4d0127
Compare
|
Hi, this is WorkflowBot.
|
a4d0127 to
2b2fc79
Compare
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
Left some questions:
Should we consider restoring all temporary mutable state (such as _current_chunk_index) after helper methods that iterate through chunks?
| return | ||
|
|
||
| if signed_transaction.sigMap.sigPair: | ||
| restored_transaction._signature_map[signed_transaction.bodyBytes] = signed_transaction.sigMap |
There was a problem hiding this comment.
It seems like the assumption here is bodyBytes uniquely identifies a transaction body. However, is it possible to have duplicate body bytes across different nodes?
There was a problem hiding this comment.
possibly no cause the node_id will be diffrent
|
|
||
| if transaction_body.HasField("transactionID"): | ||
| transaction.transaction_id = TransactionId._from_proto(transaction_body.transactionID) | ||
| transaction._transaction_ids = [TransactionId._from_proto(transaction_body.transactionID)] |
There was a problem hiding this comment.
For chunked transactions, is the ordering guaranteed?
aceppaluni
added
the
status: Needs Developer Revision
d32d076 to
eae34cb
Compare
WalkthroughTransactions now support multi-node and chunked serialization through nested transaction-ID/node mappings and ChangesTransaction serialization and execution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Transaction
participant TransactionList
participant Network
Client->>Transaction: freeze and generate transaction IDs
Transaction->>Transaction: build body bytes per transaction ID and node
Client->>Transaction: sign available body bytes
Transaction->>TransactionList: serialize bodies and signature maps
TransactionList->>Transaction: deserialize and restore transaction state
Transaction->>Network: execute reconstructed transaction chunks
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ 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 |
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>
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>
eae34cb to
9dd5458
Compare
d9ed7bf to
9dd5458
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/unit/transaction_freeze_and_bytes_test.py (1)
579-593: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNode-id dimension of
_transaction_body_bytesis left untested (# TODO check fro node ids).These tests assert the outer
TransactionIdkeys but the multi-node body-byte mapping is the central feature of this PR. The inner{node_account_id: bytes}layer is never asserted, so a regression that drops or miskeys per-node bodies would pass. Consider assertingset(tx._transaction_body_bytes[tx._transaction_ids[0]].keys()) == set(node_account_ids). (Note:fro→forin the comments.)Want me to draft the per-node assertions for these cases?
Source: Path instructions
src/hiero_sdk_python/executable.py (1)
448-456: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winError message references the wrong variable.
self._node_account_idis only assigned on Line 456 (after this check), so on a failed lookup it prints its stale value (Noneon the first attempt) rather than thenode_idthat could not be resolved. Referencenode_idfor a useful diagnostic.Proposed fix
if node is None: - raise RuntimeError(f"No node found for node_account_id: {self._node_account_id}") + raise RuntimeError(f"No node found for node_account_id: {node_id}")src/hiero_sdk_python/transaction/transfer_transaction.py (1)
165-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate
_from_protobufdocstrings to match the new single-argument signature. Both overrides droppedbody_bytesandsig_map, but their docstrings still document those parameters, which is now misleading.
src/hiero_sdk_python/transaction/transfer_transaction.py#L165-L177: remove thebody_bytes (bytes)andsig_maplines from the Args section.src/hiero_sdk_python/transaction/batch_transaction.py#L97-L110: remove thebody_bytes (bytes)andsig_maplines from the Args section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c96605d6-4bcb-4da6-ab80-aa6d49d427fa
📒 Files selected for processing (58)
src/hiero_sdk_python/account/account_create_transaction.pysrc/hiero_sdk_python/consensus/topic_message_submit_transaction.pysrc/hiero_sdk_python/executable.pysrc/hiero_sdk_python/file/file_append_transaction.pysrc/hiero_sdk_python/nodes/node_create_transaction.pysrc/hiero_sdk_python/nodes/node_update_transaction.pysrc/hiero_sdk_python/nodes/registered_node_create_transaction.pysrc/hiero_sdk_python/nodes/registered_node_delete_transaction.pysrc/hiero_sdk_python/nodes/registered_node_update_transaction.pysrc/hiero_sdk_python/query/fee_estimate_query.pysrc/hiero_sdk_python/query/query.pysrc/hiero_sdk_python/transaction/batch_transaction.pysrc/hiero_sdk_python/transaction/chunked_transaction.pysrc/hiero_sdk_python/transaction/transaction.pysrc/hiero_sdk_python/transaction/transfer_transaction.pytests/integration/file_append_transaction_e2e_test.pytests/integration/topic_message_submit_transaction_e2e_test.pytests/integration/transaction_freeze_e2e_test.pytests/unit/account_create_transaction_test.pytests/unit/account_delete_transaction_test.pytests/unit/account_update_transaction_test.pytests/unit/batch_transaction_test.pytests/unit/chunked_transaction_test.pytests/unit/contract_delete_transaction_test.pytests/unit/contract_execute_transaction_test.pytests/unit/ethereum_transaction_test.pytests/unit/executable_test.pytests/unit/fee_estimate_query_test.pytests/unit/file_append_transaction_test.pytests/unit/file_delete_transaction_test.pytests/unit/file_update_transaction_test.pytests/unit/freeze_transaction_test.pytests/unit/node_create_transaction_test.pytests/unit/node_delete_transaction_test.pytests/unit/node_update_transaction_test.pytests/unit/prng_transaction_test.pytests/unit/query_test.pytests/unit/schedule_create_transaction_test.pytests/unit/schedule_delete_transaction_test.pytests/unit/schedule_sign_transaction_test.pytests/unit/token_airdrop_transaction_cancel_test.pytests/unit/token_airdrop_transaction_test.pytests/unit/token_associate_transaction_test.pytests/unit/token_create_transaction_test.pytests/unit/token_delete_transaction_test.pytests/unit/token_dissociate_transaction_test.pytests/unit/token_freeze_transaction_test.pytests/unit/token_mint_transaction_test.pytests/unit/token_pause_transaction_test.pytests/unit/token_unfreeze_transaction_test.pytests/unit/token_unpause_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.pytests/unit/transaction_freeze_and_bytes_test.pytests/unit/transaction_test.pytests/unit/transfer_transaction_test.py
| transaction.auto_renew_period = None | ||
|
|
||
| if body.HasField("autoRenewPeriod"): | ||
| transaction.auto_renew_period = Duration._from_proto(body.autoRenewPeriod) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
auto_renew_period = None can make the round-tripped transaction unserializable.
_build_proto_body() (line 322) does duration_pb2.Duration(seconds=self.auto_renew_period.seconds), which raises AttributeError when auto_renew_period is None. If a deserialized proto lacks autoRenewPeriod, this reconstruction sets it to None, so a subsequent build_transaction_body()/execute() on the restored object crashes. Prefer leaving the constructor default in place when the field is absent.
🛡️ Proposed change
- transaction.initial_balance = body.initialBalance
- transaction.receiver_signature_required = body.receiverSigRequired
- transaction.auto_renew_period = None
-
- if body.HasField("autoRenewPeriod"):
- transaction.auto_renew_period = Duration._from_proto(body.autoRenewPeriod)
+ transaction.initial_balance = body.initialBalance
+ transaction.receiver_signature_required = body.receiverSigRequired
+
+ if body.HasField("autoRenewPeriod"):
+ transaction.auto_renew_period = Duration._from_proto(body.autoRenewPeriod)📝 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.
| transaction.auto_renew_period = None | |
| if body.HasField("autoRenewPeriod"): | |
| transaction.auto_renew_period = Duration._from_proto(body.autoRenewPeriod) | |
| transaction.initial_balance = body.initialBalance | |
| transaction.receiver_signature_required = body.receiverSigRequired | |
| if body.HasField("autoRenewPeriod"): | |
| transaction.auto_renew_period = Duration._from_proto(body.autoRenewPeriod) |
| return self | ||
| if body.HasField("topicID"): | ||
| transaction.topic_id = TopicId._from_proto(body.topicID) | ||
| transaction.message = body.message.decode("utf-8") if body.message else None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
decode("utf-8") breaks round-trip for binary messages.
set_message() accepts bytes | str, so a message can be arbitrary binary. Unconditionally decoding as UTF-8 raises UnicodeDecodeError for non-UTF-8 payloads, causing Transaction.from_bytes() to fail entirely on a previously valid serialized transaction. Guard the decode and fall back to the raw bytes so both text and binary messages round-trip.
🛡️ Proposed change
- transaction.message = body.message.decode("utf-8") if body.message else None
+ if not body.message:
+ transaction.message = None
+ else:
+ try:
+ transaction.message = body.message.decode("utf-8")
+ except UnicodeDecodeError:
+ transaction.message = body.message📝 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.
| transaction.message = body.message.decode("utf-8") if body.message else None | |
| if not body.message: | |
| transaction.message = None | |
| else: | |
| try: | |
| transaction.message = body.message.decode("utf-8") | |
| except UnicodeDecodeError: | |
| transaction.message = body.message |
Source: Path instructions
| if not self.node_account_ids: | ||
| self.node_account_ids = [node._account_id for node in client.network._healthy_nodes] | ||
| self.node_account_ids = [node._account_id for node in client.network.nodes] | ||
|
|
||
| if not self.node_account_ids: | ||
| raise RuntimeError("No healthy nodes available for execution") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Node source switched to all nodes, but the error text still says "healthy".
Selection now seeds node_account_ids from client.network.nodes instead of _healthy_nodes, so health is deferred to is_healthy()/_handle_unhealthy_node() during execution. Confirm this behavioral change is intended (bodies are pre-built for every node to support failover), and correct the misleading message at Line 375 — it now only fires when the node list is empty, not when all nodes are unhealthy.
Proposed message fix
if not self.node_account_ids:
- raise RuntimeError("No healthy nodes available for execution")
+ raise RuntimeError("No nodes available for execution")📝 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.
| if not self.node_account_ids: | |
| self.node_account_ids = [node._account_id for node in client.network._healthy_nodes] | |
| self.node_account_ids = [node._account_id for node in client.network.nodes] | |
| if not self.node_account_ids: | |
| raise RuntimeError("No healthy nodes available for execution") | |
| if not self.node_account_ids: | |
| self.node_account_ids = [node._account_id for node in client.network.nodes] | |
| if not self.node_account_ids: | |
| raise RuntimeError("No nodes available for execution") |
| def _generate_transaction_ids(self, initial_transaction: TransactionId, count: int): | ||
| """Generate all transaction_id for require chunks.""" | ||
| self._transaction_ids = [] | ||
|
|
||
| if count == 1: | ||
| self._transaction_ids = [initial_transaction] | ||
| return | ||
|
|
||
| transaction_id = initial_transaction | ||
| for i in range(count): | ||
| self._transaction_ids.append(transaction_id) | ||
|
|
||
| next_nanos = initial_transaction.valid_start.nanos + (i + 1) | ||
| next_valid_start = timestamp_pb2.Timestamp( | ||
| seconds=initial_transaction.valid_start.seconds + next_nanos // 1_000_000_000, | ||
| nanos=next_nanos % 1_000_000_000, | ||
| ) | ||
|
|
||
| transaction_id = TransactionId(account_id=self.transaction_id.account_id, valid_start=next_valid_start) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use initial_transaction.account_id instead of self.transaction_id.account_id.
Inside the loop, self.transaction_id resolves to _transaction_ids[self._current_transaction_id_index]. This only returns the initial id because the index happens to be 0 at freeze time; it silently couples id generation to _current_transaction_id_index (which is mutated during chunked execution / fee estimation). Reference the passed-in initial_transaction directly to remove the hidden dependency.
♻️ Proposed change
- transaction_id = TransactionId(account_id=self.transaction_id.account_id, valid_start=next_valid_start)
+ transaction_id = TransactionId(account_id=initial_transaction.account_id, valid_start=next_valid_start)📝 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 _generate_transaction_ids(self, initial_transaction: TransactionId, count: int): | |
| """Generate all transaction_id for require chunks.""" | |
| self._transaction_ids = [] | |
| if count == 1: | |
| self._transaction_ids = [initial_transaction] | |
| return | |
| transaction_id = initial_transaction | |
| for i in range(count): | |
| self._transaction_ids.append(transaction_id) | |
| next_nanos = initial_transaction.valid_start.nanos + (i + 1) | |
| next_valid_start = timestamp_pb2.Timestamp( | |
| seconds=initial_transaction.valid_start.seconds + next_nanos // 1_000_000_000, | |
| nanos=next_nanos % 1_000_000_000, | |
| ) | |
| transaction_id = TransactionId(account_id=self.transaction_id.account_id, valid_start=next_valid_start) | |
| def _generate_transaction_ids(self, initial_transaction: TransactionId, count: int): | |
| """Generate all transaction_id for require chunks.""" | |
| self._transaction_ids = [] | |
| if count == 1: | |
| self._transaction_ids = [initial_transaction] | |
| return | |
| transaction_id = initial_transaction | |
| for i in range(count): | |
| self._transaction_ids.append(transaction_id) | |
| next_nanos = initial_transaction.valid_start.nanos + (i + 1) | |
| next_valid_start = timestamp_pb2.Timestamp( | |
| seconds=initial_transaction.valid_start.seconds + next_nanos // 1_000_000_000, | |
| nanos=next_nanos % 1_000_000_000, | |
| ) | |
| transaction_id = TransactionId(account_id=initial_transaction.account_id, valid_start=next_valid_start) |
| if transaction._node_account_id is not None: | ||
| transaction.set_node_account_id(transaction._node_account_id) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Avoid calling the deprecated set_node_account_id() from library internals.
_node_account_id is already populated by _from_protobuf; this call only mirrors it into node_account_ids, but set_node_account_id() emits a DeprecationWarning (see executable.py) that callers of from_bytes() cannot suppress. Set node_account_ids directly instead.
♻️ Proposed change
- if transaction._node_account_id is not None:
- transaction.set_node_account_id(transaction._node_account_id)
+ if transaction._node_account_id is not None:
+ transaction.node_account_ids = [transaction._node_account_id]📝 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.
| if transaction._node_account_id is not None: | |
| transaction.set_node_account_id(transaction._node_account_id) | |
| if transaction._node_account_id is not None: | |
| transaction.node_account_ids = [transaction._node_account_id] |
| ValueError, | ||
| match="Cannot execute ChunkedTransaction with more than 5 chunks. Required: 100 Increase limit with set_max_chunks().", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape regex metacharacters in match=.
The match string contains unescaped (, ), and ., which pytest.raises treats as a regex (via re.search). This happens to still match here only by accident (the empty group () plus wildcard . absorb the literal parentheses), but it doesn't assert the literal wording precisely and would silently tolerate unrelated message changes. Wrap with re.escape(...) for a deterministic check.
🐛 Proposed fix
+ import re
+
with pytest.raises(
ValueError,
- match="Cannot execute ChunkedTransaction with more than 5 chunks. Required: 100 Increase limit with set_max_chunks().",
+ match=re.escape(
+ "Cannot execute ChunkedTransaction with more than 5 chunks. Required: 100 Increase limit with set_max_chunks()."
+ ),
):📝 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.
| ValueError, | |
| match="Cannot execute ChunkedTransaction with more than 5 chunks. Required: 100 Increase limit with set_max_chunks().", | |
| ValueError, | |
| match=re.escape( | |
| "Cannot execute ChunkedTransaction with more than 5 chunks. Required: 100 Increase limit with set_max_chunks()." | |
| ), |
| assert len(tx._transaction_body_bytes) == len(tx._transaction_ids) | ||
| assert set(tx._transaction_body_bytes.keys()) == set(tx._transaction_ids) | ||
|
|
||
| # TODO: check for node_ids |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Node-id verification dropped from all three freeze e2e tests, only a TODO left behind.
Each test replaced a stronger assertion (that _transaction_body_bytes inner keys matched the expected node AccountId set) with a check against _transaction_ids only. Since freeze_with unconditionally creates one _transaction_body_bytes entry per transaction id, this new check is nearly tautological and no longer verifies which nodes the bodies were built for — the actual behavior these three tests are meant to cover for the multi-node feature this PR introduces.
tests/integration/transaction_freeze_e2e_test.py#L25-L28: add an assertion that the inner keys oftx._transaction_body_bytes[tx_id]equal the set of node AccountIds fromenv.client.network.nodesfor the default (no explicit node_account_ids) case.tests/integration/transaction_freeze_e2e_test.py#L46-L49: add an assertion that the inner keys equalset(node_account_ids)(the two explicitly provided nodes).tests/integration/transaction_freeze_e2e_test.py#L68-L71: add an assertion that the inner keys equal{node_account_id}(the single explicitly provided node).
📍 Affects 1 file
tests/integration/transaction_freeze_e2e_test.py#L25-L28(this comment)tests/integration/transaction_freeze_e2e_test.py#L46-L49tests/integration/transaction_freeze_e2e_test.py#L68-L71
| def test_sign_topic_create_transaction(mock_account_ids, private_key): | ||
| """ | ||
| Test signing the TopicCreateTransaction with a private key. | ||
| """ | ||
| _, _, node_account_id, _, _ = mock_account_ids | ||
| opertor_id, _, node_account_id, _, _ = mock_account_ids | ||
| transaction_id = TransactionId.generate(opertor_id) | ||
|
|
||
| tx = TopicCreateTransaction(memo="Signing test") | ||
| tx.operator_account_id = AccountId(0, 0, 2) | ||
| tx.node_account_id = node_account_id | ||
| tx.transaction_id = transaction_id | ||
|
|
||
| body_bytes = tx.build_transaction_body().SerializeToString() | ||
| tx._transaction_body_bytes.setdefault(node_account_id, body_bytes) | ||
| tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
dict(node_account_id=body_bytes) keys by the literal string "node_account_id", not the actual node AccountId. Both sites use Python keyword-argument dict construction, which treats node_account_id as a string key rather than the in-scope node_account_id variable, producing {"node_account_id": body_bytes} instead of {node_account_id: body_bytes}. This mismatches the real _transaction_body_bytes: dict[TransactionId, dict[AccountId, bytes]] contract; the tests still pass only because sign() iterates .values(), but they no longer validate the true key shape.
tests/unit/topic_create_transaction_test.py#L224-L237: replacedict(node_account_id=body_bytes)with{node_account_id: body_bytes}.tests/unit/topic_delete_transaction_test.py#L75-L85: replacedict(node_account_id=body_bytes)with{node_account_id: body_bytes}.
📍 Affects 2 files
tests/unit/topic_create_transaction_test.py#L224-L237(this comment)tests/unit/topic_delete_transaction_test.py#L75-L85
| def test_frezee_with_client_generate_all_transaction_body_bytes(topic_id, mock_client): | ||
| """Test freeze_with() generate all required transaction body bytes.""" | ||
| message = "A" * 40 # will create 4 chunks | ||
|
|
||
| transaction = TopicMessageSubmitTransaction().set_topic_id(topic_id).set_chunk_size(10).set_message(message) | ||
|
|
||
| assert len(transaction._transaction_ids) == 0 | ||
| assert not transaction._transaction_body_bytes | ||
|
|
||
| transaction.freeze_with(mock_client) | ||
|
|
||
| # generated transaction ids == number of chunks i.e 4 (40/10) | ||
| assert len(transaction._transaction_ids) == 4 | ||
|
|
||
| transaction_ids = transaction._transaction_ids | ||
| initial_transaction = transaction_ids[0] | ||
|
|
||
| for i, _ in enumerate(transaction_ids): | ||
| assert transaction_ids[i].account_id == initial_transaction.account_id | ||
| assert transaction_ids[i].valid_start.nanos == initial_transaction.valid_start.nanos + i | ||
|
|
||
| # create body bytes for each transaction_id and every node_account_ids | ||
| assert len(transaction._transaction_body_bytes) == 4 | ||
| for transaction_id in transaction._transaction_ids: | ||
| assert len(transaction._transaction_body_bytes[transaction_id]) == len(mock_client.network.nodes) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider a boundary test for nanos overflow in chunked freeze_with().
These tests only exercise small chunk counts (+i for i in 0-3), never a case where base_timestamp.nanos + i would exceed 999_999_999. This is the specific overflow risk called out for freeze_with()'s chunk TransactionId generation in this feature area; a test seeding valid_start.nanos near the boundary would protect against a wraparound/invalid-timestamp regression.
| body_bytes = tx.build_transaction_body().SerializeToString() | ||
| tx._transaction_body_bytes.setdefault(node_account_id, body_bytes) | ||
| tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix incorrect dictionary key initialization.
Using dict(node_account_id=body_bytes) creates a dictionary with the literal string key "node_account_id" instead of using the local variable node_account_id. This violates the internal mapping type (dict[AccountId, bytes]).
Replace it with standard dictionary syntax to use the variable as the key.
💡 Proposed fix
- tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes))
+ tx._transaction_body_bytes.setdefault(transaction_id, {node_account_id: body_bytes})📝 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.
| body_bytes = tx.build_transaction_body().SerializeToString() | |
| tx._transaction_body_bytes.setdefault(node_account_id, body_bytes) | |
| tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes)) | |
| body_bytes = tx.build_transaction_body().SerializeToString() | |
| tx._transaction_body_bytes.setdefault(transaction_id, {node_account_id: body_bytes}) |
github-actions
Bot
added
open to community review
4 participants
Description:
This PR improve the
to_bytes/from_bytesserialization and deserialization, to support parsing multiplenode_account_idsin transaction.Changes Made:
build_base_transaction()to create the transaction body without thetransaction_idandnode_account_idfreeze_with()method to handle thenode_account_idslistnode_account_idandset_node_account_id()to prefer theset_node_account_idsinstead to match other sdksRelated issue(s):
Fixes #2302
Notes for reviewer:
_from_protobufmethod only toAccountCreateTransactionfor now to test changes. Since we have another PR that is currently working on adding the protobuf method to other transactionto_bytesthat previously depends on therequire_frozenChecklist