Create ChunkedTransaction class#73
Conversation
|
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:
Walkthrough
ChangesChunkedTransaction base class and subclass migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.py (1)
179-182: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winBLOCKER: Guard
_initial_transaction_idbefore writingchunkInfo.File and line:
src/hiero_sdk_python/consensus/topic_message_submit_transaction.py, Line 182
Proto field:ConsensusSubmitMessageTransactionBody.chunkInfo = 3,ConsensusMessageChunkInfo.initialTransactionID = 1
Issue type: Missing guard
Description:build_transaction_body()can reach_build_proto_body()for a multi-chunk message beforefreeze_with()initializes_initial_transaction_id, causing anAttributeErrorinstead of a deterministic serialization error.
Suggested fix: raise a descriptiveValueErrorbefore dereferencing.Proposed fix
# Multi-chunk metadata if self._total_chunks > 1: + if self._initial_transaction_id is None: + raise ValueError( + "Cannot build chunkInfo before freeze_with() initializes " + "the initial transaction ID for a multi-chunk message." + ) body.chunkInfo.CopyFrom( consensus_submit_message_pb2.ConsensusMessageChunkInfo( initialTransactionID=self._initial_transaction_id._to_proto(),As per path instructions,
_build_proto_body()must explicitly guard_initial_transaction_id is Nonebefore calling_to_proto()for multi-chunk topic messages.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ada005e-13ec-4a18-b13a-49b9ad17b7d3
📒 Files selected for processing (3)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.pysrc/hiero_sdk_python/file/file_append_transaction.pysrc/hiero_sdk_python/transaction/chunked_transaction.py
| self.contents: bytes | None = self._encode_contents(contents) | ||
| self.max_chunks: int = max_chunks if max_chunks is not None else 20 | ||
| self.chunk_size: int = chunk_size if chunk_size is not None else 4096 | ||
| self._default_transaction_fee = Hbar(5).to_tinybars() | ||
|
|
||
| # Internal tracking for chunking | ||
| self._current_chunk_index: int = 0 | ||
| self._total_chunks: int = self._calculate_total_chunks() | ||
| self._transaction_ids: list[TransactionId] = [] | ||
| self._signing_keys: list[PrivateKey] = [] # Use string annotation to avoid import issues | ||
| self._total_chunks = self._calculate_total_chunks() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cd src/hiero_sdk_python/file && find . -name "file_append_transaction.py" -type fRepository: MonaaEid/hiero-sdk-python
Length of output: 192
🏁 Script executed:
cd src/hiero_sdk_python && head -250 file/file_append_transaction.py | cat -nRepository: MonaaEid/hiero-sdk-python
Length of output: 9522
🏁 Script executed:
cd src/hiero_sdk_python && wc -l file/file_append_transaction.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 199
🏁 Script executed:
cd src/hiero_sdk_python && sed -n '1,80p' file/file_append_transaction.py | cat -nRepository: MonaaEid/hiero-sdk-python
Length of output: 3414
🏁 Script executed:
cd src/hiero_sdk_python && sed -n '130,160p' file/file_append_transaction.py | cat -nRepository: MonaaEid/hiero-sdk-python
Length of output: 1511
🏁 Script executed:
cd src/hiero_sdk_python && sed -n '200,220p' file/file_append_transaction.py | cat -nRepository: MonaaEid/hiero-sdk-python
Length of output: 1130
🏁 Script executed:
cd src/hiero_sdk_python && grep -n "_from_proto\|_to_proto\|_calculate_total_chunks" file/file_append_transaction.pyRepository: MonaaEid/hiero-sdk-python
Length of output: 806
Validate and enforce non-empty file append contents to prevent zero-chunk transactions.
Proto field: contents — field 4 in FileAppendTransactionBody. Issue type: Bytes not normalized / Chunking contract violation. The protobuf schema requires non-empty contents. However, empty bytes (b"") are not normalized to None, causing _calculate_total_chunks() to return 0 when contents is b"" instead of the expected minimum of 1. This can occur when:
- Calling
_from_proto()with proto.contents =b""(protobuf default for bytes) - Passing empty string or bytes to constructor or
set_contents()
The _build_proto_body() method then silently serializes an empty body instead of rejecting it, violating the chunking contract. Per coding guidelines, required proto fields must be validated client-side, and bytes fields modeled as bytes | None must normalize empty bytes to None.
Proposed fixes
- Line 207 (
_from_proto()): Normalize empty bytes toNone:
self.contents = proto.contents or None- Lines 51-60 (
_calculate_total_chunks()): Treat bothNoneand empty bytes as requiring 1 chunk:
if not self.contents:
return 1
return math.ceil(len(self.contents) / self.chunk_size)- Lines 144-149 (
_build_proto_body()): Reject empty contents instead of silent fallback:
if not self.contents:
raise ValueError("Missing required non-empty contents")
start_index = self._current_chunk_index * self.chunk_size
end_index = min(start_index + self.chunk_size, len(self.contents))
chunk_contents = self.contents[start_index:end_index]Also applies to: 51-60, 140-149, 206-208
🧰 Tools
🪛 GitHub Actions: Secondary PR Check - Hiero Solo Integration & Unit Tests / Unit Tests (Python 3.14)
[error] 31-31: NameError: name 'Hbar' is not defined. In FileAppendTransaction.init, _default_transaction_fee is set with Hbar(5).to_tinybars() causing all related tests to fail.
Source: Path instructions
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 (2)
src/hiero_sdk_python/file/file_append_transaction.py (1)
216-227: 🎯 Functional Correctness | 🟡 Minor
_validate_chunking()override breaks the base-class return contract.The base
ChunkedTransaction._validate_chunking()is declared to returnintand is assigned torequired_chunksinfreeze_with()(line 153). TheFileAppendTransactionoverride is annotated-> Noneand returns nothing, causing a type contract violation. Although the current code path doesn't use the assigned variable (it callsself.get_required_chunks()directly in the loop), this is a latent bug that breaks the override contract. Addreturn self.get_required_chunks()to the override.♻️ Add return statement to align with base contract
def _validate_chunking(self) -> None: + def _validate_chunking(self) -> int: """ Validates that the transaction doesn't exceed the maximum number of chunks. Raises: ValueError: If the transaction exceeds the maximum number of chunks. """ if self.max_chunks and self.get_required_chunks() > self.max_chunks: raise ValueError( f"Cannot execute FileAppendTransaction with more than {self.max_chunks} chunks. " f"Required: {self.get_required_chunks()}" ) + return self.get_required_chunks()Source: Path instructions
src/hiero_sdk_python/consensus/topic_message_submit_transaction.py (1)
174-195: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winBLOCKER: Guard
_initial_transaction_idbefore serializing a multi-chunk body.When
_total_chunks > 1, line 189 dereferencesself._initial_transaction_id._to_proto().ChunkedTransaction.freeze_with()is what populates_initial_transaction_id; if a caller invokesbuild_transaction_body()(orbuild_scheduled_body()) directly without freezing,_initial_transaction_idisNoneand this raisesAttributeErrorat serialization time. Add an explicitNonecheck with a descriptiveValueError.🛡️ Proposed guard
# Multi-chunk metadata if self._total_chunks > 1: + if self._initial_transaction_id is None: + raise ValueError( + "initial_transaction_id is not set; call freeze_with() " + "before building a multi-chunk transaction body." + ) body.chunkInfo.CopyFrom( consensus_submit_message_pb2.ConsensusMessageChunkInfo( initialTransactionID=self._initial_transaction_id._to_proto(), total=self._total_chunks, number=self._current_chunk_index + 1, ) )As per path instructions: "An explicit
if self._initial_transaction_id is Noneguard with a descriptiveValueErrorMUST be present. Flag as BLOCKER."Source: Path instructions
♻️ Duplicate comments (2)
src/hiero_sdk_python/file/file_append_transaction.py (2)
34-38: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate positive
max_chunks/chunk_sizein the constructor.
chunk_size=0reaches_calculate_total_chunks()(Line 38) and raisesZeroDivisionError, whilemax_chunks=0later disables the_validate_chunking()guard (if self.max_chunks). This was flagged on a prior commit and is still unaddressed.Source: Path instructions
149-154: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEmpty
contentsstill serializes a zero-chunk body.When
self.contentsisNone/b"",_build_proto_body()silently emits emptycontentsand_calculate_total_chunks()returns0, violating the chunking contract. Previously flagged and still unaddressed; normalize empty bytes toNoneand reject empty contents.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63467aa1-bf58-4800-bf64-1da24e98af51
📒 Files selected for processing (3)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.pysrc/hiero_sdk_python/file/file_append_transaction.pysrc/hiero_sdk_python/transaction/chunked_transaction.py
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/consensus/topic_message_submit_transaction.py (1)
197-200: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winBLOCKER: Guard
_initial_transaction_idbefore serializing chunk metadata.File and line: Line 200
Proto field:ConsensusSubmitMessageTransactionBody.chunkInfo = 3,ConsensusMessageChunkInfo.initialTransactionID = 1
Issue type: Missing field
Description: For multi-chunk submissions, the schema requires chunk metadata, including the initial transaction ID. Ifbuild_transaction_body()is called beforefreeze_with(),_initial_transaction_idcan still beNone, causing serialization to fail withAttributeErrorinstead of an actionable error. (raw.githubusercontent.com)
Suggested fix:Proposed fix
# Multi-chunk metadata if self._total_chunks > 1: + if self._initial_transaction_id is None: + raise ValueError( + "Cannot build a chunked topic message body before freeze_with() " + "initializes the initial transaction ID." + ) body.chunkInfo.CopyFrom( consensus_submit_message_pb2.ConsensusMessageChunkInfo( initialTransactionID=self._initial_transaction_id._to_proto(),As per path instructions,
_build_proto_body()must explicitly guard_initial_transaction_id is Nonefor multi-chunk messages.Source: Path instructions
♻️ Duplicate comments (1)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.py (1)
48-48: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winBLOCKER: Normalize the stored HCS message to
bytes.File and line: Line 48
Proto field:ConsensusSubmitMessageTransactionBody.message = 2
Issue type: Wrong type
Description: The schema definesmessageasbytes, but the SDK attribute still allows storingstr. Acceptingstrat the API boundary is fine, but the model field should bebytes | Noneto align with the proto field. (raw.githubusercontent.com)
Suggested fix:Proposed fix
- self.message: bytes | str | None = message + self.message: bytes | None = ( + message.encode("utf-8") if isinstance(message, str) else message + ) @@ - self.message = message + self.message = message.encode("utf-8") if isinstance(message, str) else messageAs per path instructions, proto
bytesfields must map tobytes | Noneorbytes, and consensus protobuf correctness is must-not-break.Also applies to: 100-112
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cea53e29-97ab-40ca-8f30-b284a7378be9
📒 Files selected for processing (1)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.py
9f06310 to
dd22252
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/file/file_append_transaction.py (1)
218-229: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove this override so shared chunk validation runs.
File and line: Line 218. Proto field:
contents— field4. Issue type: Bytes not normalized / chunking contract violation.FileAppendTransactionnow inheritsChunkedTransaction, but this override shadows the base guard that rejects< 1chunks and maintains_total_chunks;freeze_with()andexecute_all()dispatch here, so empty append payloads can still bypass the centralized validation even though the schema requires non-emptycontents. (raw.githubusercontent.com)Proposed fix
- def _validate_chunking(self) -> None: - """ - Validates that the transaction doesn't exceed the maximum number of chunks. - - Raises: - ValueError: If the transaction exceeds the maximum number of chunks. - """ - if self.max_chunks and self.get_required_chunks() > self.max_chunks: - raise ValueError( - f"Cannot execute FileAppendTransaction with more than {self.max_chunks} chunks. " - f"Required: {self.get_required_chunks()}" - )As per path instructions, “Validation bounds for max_chunks and chunk_size are strictly enforced” and serialization must map directly to Hedera protobufs.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d29f3ff-fffc-4531-acae-b4bdf23dea03
📒 Files selected for processing (1)
src/hiero_sdk_python/file/file_append_transaction.py
dd22252 to
26e5372
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.py (1)
197-201: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winBLOCKER: Guard
initialTransactionIDbefore serializing chunk metadata.File and line:
src/hiero_sdk_python/consensus/topic_message_submit_transaction.py, Lines 197-201
Proto field:ConsensusMessageChunkInfo.initialTransactionID = 1
Issue type: Missing field
Description: Callingbuild_transaction_body()directly for a multi-chunk message can dereferenceNonebeforefreeze_with()initializes_initial_transaction_id, producing anAttributeErrorinstead of a deterministic validation error. (raw.githubusercontent.com)
Suggested fix:Proposed fix
# Multi-chunk metadata if self._total_chunks > 1: + if self._initial_transaction_id is None: + raise ValueError( + "Cannot build a multi-chunk TopicMessageSubmitTransaction before freeze_with(); " + "initial transaction ID is missing." + ) body.chunkInfo.CopyFrom( consensus_submit_message_pb2.ConsensusMessageChunkInfo( initialTransactionID=self._initial_transaction_id._to_proto(),As per path instructions,
_initial_transaction_idnull-safety requires an explicit guard with a descriptiveValueError.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c769826-af58-4220-ba1a-8cafb11c6703
📒 Files selected for processing (5)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.pysrc/hiero_sdk_python/file/file_append_transaction.pysrc/hiero_sdk_python/transaction/chunked_transaction.pytests/integration/topic_message_submit_transaction_e2e_test.pytests/unit/chunked_transaction_test.py
| def test_freeze_with_builds_chunk_transaction_ids(mock_client): | ||
| tx = DummyChunkedTransaction(required_chunks=3) | ||
| base_timestamp = timestamp_pb2.Timestamp(seconds=123, nanos=456) | ||
| tx.transaction_id = TransactionId(account_id=AccountId(0, 0, 1234), valid_start=base_timestamp) | ||
|
|
||
| tx.freeze_with(mock_client) | ||
|
|
||
| assert tx._total_chunks == 3 | ||
| assert len(tx._transaction_ids) == 3 | ||
| assert tx._initial_transaction_id == tx.transaction_id | ||
| assert tx._transaction_ids[0] == tx.transaction_id | ||
| assert tx._transaction_ids[1].valid_start.seconds == 123 | ||
| assert tx._transaction_ids[1].valid_start.nanos == 457 | ||
| assert tx._transaction_ids[2].valid_start.seconds == 123 | ||
| assert tx._transaction_ids[2].valid_start.nanos == 458 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a rollover case for valid_start.nanos.
This only exercises the easy path (456 -> 457/458). The new chunking base also has a carry path when nanos crosses 1_000_000_000, and that boundary is exactly what can break chunk ordering on the wire. Please add a case starting near 999_999_999 and assert the later chunk increments seconds and wraps nanos. As per path instructions, unit tests should cover boundary conditions, and chunked message submission must carry seconds when nanos + num_chunks > 999_999_999.
Source: Path instructions
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
4d30b9c to
16e3540
Compare
Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
02d3111 to
f8bd8cf
Compare
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
f0b39c6 to
34149b5
Compare
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
cac35b2 to
c6d2eba
Compare
1 participant
No description provided.