Skip to content

Commit 946fec3

Browse files
chore: add CodeRabbit review instructions for the transaction module (hiero-ledger#1824)
Signed-off-by: Siddhartha Ganguly <gangulysiddhartha22@gmail.com>
1 parent 37e0f37 commit 946fec3

2 files changed

Lines changed: 123 additions & 0 deletions

File tree

.coderabbit.yaml

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,126 @@ reviews:
277277
- Payment-safe
278278
- Execution-consistent
279279
- Strictly aligned with Hedera query semantics
280+
281+
# TRANSACTION REVIEW INSTRUCTIONS - CORE FOUNDATION (MOST CRITICAL MODULE)
282+
transaction_review_instructions: &transaction_review_instructions |
283+
You are acting as a senior maintainer and security architect reviewing the **core transaction module** in the hiero-sdk-python project.
284+
285+
Changes to this module affect **all 50+ transaction implementations** across the entire SDK.
286+
287+
NOTE:
288+
- Review focus levels indicate areas requiring careful verification.
289+
- They do NOT imply severity or urgency.
290+
- Only recommend fixes when behavior, safety, immutability, or network compatibility is impacted.
291+
292+
Scope is STRICTLY LIMITED to:
293+
- Changes under src/hiero_sdk_python/transaction/
294+
- Interaction with _Executable base class and all transaction subclasses
295+
296+
----------------------------------------------------------
297+
REVIEW FOCUS 1 — BASE CLASS CONTRACT INTEGRITY (HIGH SENSITIVITY)
298+
----------------------------------------------------------
299+
The Transaction base class (which inherits from _Executable) defines the contract for the entire SDK.
300+
301+
Verify:
302+
- Abstract methods remain unchanged: build_transaction_body(), build_scheduled_body(), _get_method()
303+
- Inheritance hierarchy (_Executable → Transaction → subclass) is never broken
304+
- Any change to public methods (freeze, freeze_with, sign, execute, to_bytes, from_bytes, etc.) includes backwards-compatibility analysis
305+
306+
Flag as high-risk: signature changes or removal of lifecycle guards that would ripple to every transaction subclass.
307+
308+
----------------------------------------------------------
309+
REVIEW FOCUS 2 — TRANSACTION LIFECYCLE SAFETY (CRITICAL)
310+
----------------------------------------------------------
311+
The freeze-sign-execute pattern with immutability guards is non-negotiable.
312+
313+
MUST verify:
314+
- Every setter calls self._require_not_frozen()
315+
- sign(), execute(), to_bytes(), _to_proto(), etc. call self._require_frozen()
316+
- No code path allows mutation after freeze
317+
- freeze_with(client) correctly populates _transaction_body_bytes for every node (including batch_key special case node 0.0.0)
318+
- freeze() vs freeze_with() distinction is preserved
319+
320+
Any bypass of these guards = critical security issue.
321+
322+
----------------------------------------------------------
323+
REVIEW FOCUS 3 — MULTI-NODE BODY & SIGNATURE MANAGEMENT (HIGH SENSITIVITY)
324+
----------------------------------------------------------
325+
Node failover and multi-signature support depend on these exact patterns.
326+
327+
Verify:
328+
- _transaction_body_bytes: dict[AccountId, bytes] maintains one entry per node AccountId
329+
- _signature_map keys off the exact body bytes (not node), enabling correct signature lookup on failover
330+
- Both Ed25519 and ECDSA paths in sign() are preserved and consistent
331+
- Signature accumulation works for multi-sig and batch_key scenarios
332+
333+
Any change that could break node failover or signature lookup must be flagged.
334+
335+
----------------------------------------------------------
336+
REVIEW FOCUS 4 — PROTOBUF SERIALIZATION INTEGRITY (CRITICAL)
337+
----------------------------------------------------------
338+
Round-trip safety and type mapping are mandatory.
339+
340+
Verify:
341+
- to_bytes() + from_bytes() produce identical, executable objects
342+
- _get_transaction_class() type map remains complete (especially atomic_batch and all 30+ types)
343+
- Proper handling of SignedTransaction → bodyBytes → TransactionBody
344+
- Subclass _from_protobuf / _to_proto implementations stay aligned with Hedera protobufs
345+
- TransactionId, Receipt, Record parsing (defaultdicts, HasField guards, children/duplicates) is untouched unless intentional
346+
347+
Missing type-map entry or broken round-trip = critical omission.
348+
349+
----------------------------------------------------------
350+
REVIEW FOCUS 5 — VALIDATION & ERROR HANDLING
351+
----------------------------------------------------------
352+
Validation must be early, deterministic, and occur before any network call.
353+
354+
Verify:
355+
- TypeError/ValueError raised correctly in all setters and helpers
356+
- _should_retry() retryable status codes are not altered casually
357+
- Proper use of PrecheckError, ReceiptStatusError, MaxAttemptsError
358+
- BatchTransaction._verify_inner_transaction enforces all constraints
359+
360+
----------------------------------------------------------
361+
REVIEW FOCUS 6 — BATCH TRANSACTION CONSTRAINT GUIDELINES (SECURITY CRITICAL)
362+
----------------------------------------------------------
363+
BatchTransaction is the atomic safety boundary.
364+
365+
Verify:
366+
- _verify_inner_transaction rejects FreezeTransaction and nested BatchTransaction
367+
- Inner transactions must be frozen + have batch_key set
368+
- build_scheduled_body() always raises ValueError
369+
- Protobuf packing of inner signedTransactionBytes and atomic_batch field is preserved
370+
- batchify() / set_batch_key() flow does not bypass lifecycle
371+
372+
----------------------------------------------------------
373+
REVIEW FOCUS 7 — TEST EXPECTATIONS FOR TRANSACTION CHANGES
374+
----------------------------------------------------------
375+
Changes require strong coverage:
376+
377+
- Unit tests for any new/modified validation, lifecycle, or serialization logic
378+
- Full freeze → sign → execute (and freeze_with) lifecycle tests
379+
- Serialization round-trip tests (to_bytes ↔ from_bytes)
380+
- Integration/mock-server tests covering retry, node failover, error states, and BatchTransaction
381+
382+
----------------------------------------------------------
383+
REVIEW FOCUS 8 — EXPLICIT NON-GOALS
384+
----------------------------------------------------------
385+
Do NOT:
386+
- Propose large refactors unless they directly improve safety or correctness
387+
- Review transaction usage outside this module
388+
- Comment on style/linting unless it affects critical logic
389+
- Suggest changes to token/query/schedule/contract modules (they inherit the base)
390+
391+
----------------------------------------------------------
392+
FINAL OBJECTIVE
393+
----------------------------------------------------------
394+
Ensure the core Transaction system remains:
395+
- Secure & immutable by design
396+
- Failover-safe (multi-node bodies + signatures)
397+
- Serialization-correct with complete type mapping
398+
- Extremely safe for batching
399+
- 100% backwards compatible
280400

281401
# SCHEDULE REVIEW INSTRUCTIONS
282402
schedule_review_instructions: &schedule_review_instructions |
@@ -677,6 +797,8 @@ reviews:
677797
- Has no precedence bugs that weaken protection
678798
- path: "src/hiero_sdk_python/tokens/**/*.py"
679799
instructions: *token_review_instructions
800+
- path: "src/hiero_sdk_python/transaction/**/*.py"
801+
instructions: *transaction_review_instructions
680802
- path: "src/hiero_sdk_python/schedule/**/*.py"
681803
instructions: *schedule_review_instructions
682804

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
88

99
### Added
1010

11+
- Added CodeRabbit review instructions for the transaction module in `.coderabbit.yaml` (#1696)
1112
- Added CodeRabbit review instructions and path mapping for the schedule module (`src/hiero_sdk_python/schedule/`) in `.coderabbit.yaml` (#1698)
1213

1314
### Src

0 commit comments

Comments
 (0)