Skip to content

Commit d2f17ed

Browse files
MonaaEidMonaaEid
authored
feat: add consensus and general protobuf alignment review instructions (hiero-ledger#1865)
Signed-off-by: MonaaEid <monaa_eid@hotmail.com>
1 parent 3cdc8a5 commit d2f17ed

File tree

2 files changed

+256
-1
lines changed

2 files changed

+256
-1
lines changed

‎.coderabbit.yaml‎

Lines changed: 253 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@ language: "en-US" # USA English
44
# It should not state additional information like: related issues, PRs, suggest reviewers
55
# It should not continue a casual conversation with users that reply to it
66

7+
knowledge_base: # Enable knowledge base access for up-to-date information
8+
web_search: # Enable web search for up-to-date information
9+
enabled: true
10+
711
# Only documents non-default options:
812
reviews:
913
profile: "assertive" # Assertive profile yields more feedback, that may be considered nitpicky.
@@ -1055,6 +1059,114 @@ reviews:
10551059
- Correctly matches repository paths
10561060
- Enforces intended review governance
10571061
- Has no precedence bugs that weaken protection
1062+
# PROTOBUF INSTRUCTIONS
1063+
- path: "src/hiero_sdk_python/**/*.py"
1064+
instructions: |
1065+
## Protobuf Schema Alignment Review
1066+
1067+
### Step 1 — Detect protobuf imports
1068+
1069+
Scan the file for all imports matching this pattern:
1070+
`from hiero_sdk_python.hapi.<path> import <module>_pb2`
1071+
1072+
For each import found, skip `_grpc` stubs (those end in `_pb2_grpc`) — only
1073+
process `_pb2` message modules.
1074+
1075+
### Step 2 — Construct the canonical schema URL
1076+
1077+
For each `_pb2` import:
1078+
1. Extract the path segment after `hapi`
1079+
(e.g. `services` from `hiero_sdk_python.hapi.services`)
1080+
2. Extract the module name before `_pb2`
1081+
(e.g. `transaction_record` from `transaction_record_pb2`)
1082+
3. Build the canonical URL:
1083+
`https://github.com/hashgraph/hedera-protobufs/blob/v0.66.0/<path>/<module>.proto`
1084+
1085+
**Example:**
1086+
```
1087+
Import: from hiero_sdk_python.hapi.services import transaction_record_pb2
1088+
Schema: https://github.com/hashgraph/hedera-protobufs/blob/v0.66.0/services/transaction_record.proto
1089+
```
1090+
1091+
### Step 3 — Compare the SDK class against the proto schema
1092+
1093+
For each SDK class that calls `_from_proto` or `_to_proto` using messages from
1094+
the detected proto module, verify the following:
1095+
1096+
#### 3a. Field coverage
1097+
- Every non-deprecated proto field SHOULD have a corresponding SDK attribute.
1098+
- Flag any proto field that is present in the schema but missing from the SDK
1099+
class (either as a dataclass field or handled in `_from_proto`).
1100+
- Flag any SDK attribute that references a proto field name that does not exist
1101+
in the schema.
1102+
1103+
#### 3b. Field name mapping
1104+
- Proto field names use `snake_case` (e.g. `ethereum_hash`) or `camelCase`
1105+
(e.g. `transactionHash`, `consensusTimestamp`). Verify that `_from_proto`
1106+
accesses the correct proto field name, not a renamed/misspelled version.
1107+
- Verify `_to_proto` sets the correct proto field name.
1108+
1109+
#### 3c. Field type alignment
1110+
Check that each SDK field type correctly represents the proto type:
1111+
1112+
| Proto type | Expected SDK type |
1113+
|------------------------|----------------------------------------------|
1114+
| `string` | `str \| None` or `str` |
1115+
| `bytes` | `bytes \| None` or `bytes` |
1116+
| `bool` | `bool` |
1117+
| `int32`, `uint32` | `int` |
1118+
| `int64`, `uint64`, `sint64` | `int` |
1119+
| `MessageType` | corresponding SDK wrapper class or `None` |
1120+
| `repeated MessageType` | `list[SdkType]` |
1121+
| `repeated scalar` | `list[scalar]` |
1122+
1123+
#### 3d. `oneof` field handling
1124+
- Identify all `oneof` blocks in the proto schema for the message.
1125+
- Verify that `_to_proto` does NOT set more than one field from the same
1126+
`oneof` block simultaneously. If it does, a `ValueError` guard MUST be
1127+
present before setting those fields.
1128+
- Verify that `_from_proto` uses `proto.HasField(...)` (not attribute access)
1129+
to detect which branch of a `oneof` is populated.
1130+
- Common `oneof` groups to watch for in `TransactionRecord`:
1131+
- `oneof body { contractCallResult, contractCreateResult }` (fields 7, 8)
1132+
- `oneof entropy { prng_bytes, prng_number }` (fields 19, 20)
1133+
1134+
#### 3e. Bytes field normalization
1135+
- Proto `bytes` fields default to `b""` (empty bytes) when unset, not `None`.
1136+
- If the SDK models the field as `bytes | None`, verify that `_from_proto`
1137+
uses `proto.<field> or None` to normalize empty bytes to `None`.
1138+
- Conversely, if `_to_proto` sets a bytes field, verify it guards against
1139+
setting `None` (which would cause a protobuf type error).
1140+
1141+
#### 3f. `_from_proto` / `_to_proto` symmetry
1142+
- Every field parsed in `_from_proto` SHOULD also be serialized in `_to_proto`,
1143+
and vice versa, unless the field is intentionally read-only (e.g. server-set
1144+
fields that clients never send).
1145+
- Flag any asymmetry that is not accompanied by a code comment explaining why.
1146+
1147+
#### 3g. Repeated field default values
1148+
- SDK attributes representing proto `repeated` fields MUST default to an empty
1149+
list (`field(default_factory=list)`), never `None`.
1150+
- Flag any `repeated` field that defaults to `None`.
1151+
1152+
#### 3h. Type annotation consistency
1153+
- The codebase uses `X | None` union syntax (Python 3.10+). Flag any use of
1154+
`Optional[X]` from `typing` in newly added or modified code, as it is
1155+
inconsistent with the established style.
1156+
1157+
### Step 4 — Report format
1158+
1159+
For each issue found, report:
1160+
- **File and line**: where the issue occurs
1161+
- **Proto field**: the canonical proto field name and number from the schema
1162+
- **Issue type**: one of — Missing field | Wrong field name | Wrong type |
1163+
oneof violation | Bytes not normalized | Asymmetric round-trip |
1164+
Wrong default | Style inconsistency
1165+
- **Description**: concise explanation of the discrepancy
1166+
- **Suggested fix**: the corrected code snippet
1167+
1168+
If no issues are found for a file, state:
1169+
All mapped proto fields align with the schema at `<URL>`.
10581170
- path: "src/hiero_sdk_python/tokens/**/*.py"
10591171
instructions: *token_review_instructions
10601172
- path: "src/hiero_sdk_python/transaction/**/*.py"
@@ -1424,6 +1536,147 @@ reviews:
14241536
instructions: *node_review_instructions
14251537
- path: "src/hiero_sdk_python/file/**/*.py"
14261538
instructions: *file_review_instructions
1539+
- path: "src/hiero_sdk_python/consensus/**/*.py"
1540+
instructions: |
1541+
You are reviewing Python files in the `src/hiero_sdk_python/consensus/` directory.
1542+
These files implement consensus-related transactions and queries for the Hiero SDK.
1543+
1544+
> Note: Protobuf schema alignment (field names, types, oneof, bytes normalization,
1545+
> repeated defaults, and canonical `.proto` URL derivation) is already enforced by
1546+
> the global `src/hiero_sdk_python/**/*.py` instruction. Do NOT re-run those checks
1547+
> here — focus exclusively on the consensus-domain concerns below.
1548+
1549+
---
1550+
1551+
## Review priorities (highest → lowest)
1552+
1553+
### 1) Protobuf correctness (must-not-break)
1554+
- Validate field names, field types, and (if visible/known) field numbers +
1555+
repeated/optional semantics.
1556+
- Flag any divergence as **BLOCKER**.
1557+
1558+
### 2) Serialization / wire-format integrity
1559+
- Ensure `to_proto` / `from_proto` (or equivalent) set the correct oneof
1560+
branches and do not drop fields.
1561+
- Confirm byte/string encoding, timestamps/durations, and `TransactionID` /
1562+
`TopicID` mapping are correct.
1563+
- Treat silent coercions, defaulting, or ignored exceptions as **BLOCKER** if
1564+
they could change what is signed or sent.
1565+
- **Memo-overwrite trap**: any field whose Python default is `""` or `0` instead
1566+
of `None` and is later guarded by `if field is not None` will *always* serialize
1567+
— even when the caller never set it — silently overwriting the on-chain value.
1568+
Verify every optional update field defaults to `None`, not a falsy sentinel.
1569+
Flag as **BLOCKER** if the unintended overwrite affects keys, expiry, or memo.
1570+
1571+
### 3) Signing / transaction body construction
1572+
- Verify the class builds the transaction/query body exactly once and uses the
1573+
correct protobuf message type.
1574+
- Ensure required fields are enforced before freezing/signing (if the SDK has a
1575+
freeze step).
1576+
- Missing required fields or signing the wrong body type is **BLOCKER**.
1577+
1578+
#### 3a) Multi-chunk TransactionID safety (`TopicMessageSubmitTransaction`)
1579+
When reviewing chunked message submission, verify all of the following:
1580+
1581+
**Nanos-overflow guard**
1582+
- In `freeze_with()`, the expression `base_timestamp.nanos + i` MUST be checked
1583+
for overflow. Protobuf `Timestamp.nanos` is bounded to `[0, 999_999_999]`.
1584+
If `nanos + num_chunks > 999_999_999` the value wraps or is rejected by the
1585+
node. An overflow guard MUST carry seconds over:
1586+
```python
1587+
total_nanos = base_timestamp.nanos + i
1588+
chunk_valid_start = timestamp_pb2.Timestamp(
1589+
seconds=base_timestamp.seconds + total_nanos // 1_000_000_000,
1590+
nanos=total_nanos % 1_000_000_000
1591+
)
1592+
```
1593+
Flag the absence of this guard as **BLOCKER**.
1594+
1595+
**`_initial_transaction_id` null-safety**
1596+
- `_build_proto_body()` calls `self._initial_transaction_id._to_proto()` when
1597+
`_total_chunks > 1`. If `freeze_with()` was never called (e.g. the caller
1598+
invokes `build_transaction_body()` directly on a multi-chunk message),
1599+
`_initial_transaction_id` is `None` and this raises `AttributeError` at
1600+
serialization time. An explicit `if self._initial_transaction_id is None`
1601+
guard with a descriptive `ValueError` MUST be present.
1602+
Flag as **BLOCKER**.
1603+
1604+
**Chunk signing consistency**
1605+
- In `execute()`, each chunk clears `_transaction_body_bytes` and
1606+
`_signature_map` then re-signs with every key in `_signing_keys`. Verify
1607+
that the re-sign loop uses the *stored* signing keys and not the operator
1608+
key only, so submit-key-protected topics are signed correctly for every chunk.
1609+
Flag any chunk that may be sent unsigned or with an incomplete signature set
1610+
as **BLOCKER**.
1611+
1612+
### 4) Topic ID handling & validation
1613+
- Validate `TopicId` parsing, checksum/network validation (if applicable), and
1614+
conversion to protobuf.
1615+
- Check `from_string` for the bare `except Exception` catch: it loses the
1616+
original exception type; the re-raised `ValueError` should chain with `from e`
1617+
(already present) — verify the chaining is not accidentally removed.
1618+
- Incorrect network scoping/checksum behavior is **MAJOR** (or **BLOCKER** if
1619+
it can misroute funds/requests).
1620+
- **Frozen-dataclass checksum mutation**: `topic_id.py` uses
1621+
`object.__setattr__(topic_id, "checksum", checksum)` to bypass `frozen=True`.
1622+
Verify this is the only site where the frozen contract is broken, and that no
1623+
other attribute is mutated this way.
1624+
1625+
### 5) Fees, limits, and defaults
1626+
1627+
#### 5a) Mutable default arguments in `__init__`
1628+
- Flag any mutable object used directly as a default parameter value in a method
1629+
signature. This is a Python anti-pattern because the object is shared across
1630+
all instances constructed without that argument.
1631+
- **Known instance**: `TopicUpdateTransaction.__init__` declares
1632+
`auto_renew_period: Optional[Duration] = Duration(7890000)`.
1633+
The fix is to default to `None` and assign in the body:
1634+
```python
1635+
def __init__(self, ..., auto_renew_period: Optional[Duration] = None, ...):
1636+
...
1637+
self.auto_renew_period = auto_renew_period if auto_renew_period is not None \
1638+
else Duration(7890000)
1639+
```
1640+
Flag as **MAJOR** (shared mutable state across instances; mutating one
1641+
instance's default would silently affect others).
1642+
- Apply the same check to any `list` or `dict` default arguments.
1643+
1644+
#### 5b) Cross-class `transaction_fee` consistency
1645+
- Verify that the hardcoded `transaction_fee` defaults are intentional and
1646+
consistent across all sibling classes in this directory.
1647+
- Flag the **missing override** in `TopicMessageSubmitTransaction` as **MAJOR**
1648+
if the base-class default is not appropriate for message submission, or add a
1649+
comment confirming the base-class value is intentional.
1650+
- Flag any value that deviates from the table above without a comment as **MAJOR**.
1651+
1652+
#### 5c) General fee/limit logic
1653+
- Confirm any fee/max payment logic is consistent and does not allow unintended
1654+
"free" execution.
1655+
- Suspicious defaults or overflow/underflow issues are **MAJOR**.
1656+
1657+
### 6) Errors & API ergonomics
1658+
- Errors should be actionable and consistent; avoid swallowing RPC/precheck errors.
1659+
- Ambiguous exceptions or inconsistent return types are **MINOR** / **MAJOR**
1660+
depending on impact.
1661+
- **`execute()` return-type consistency**: `TopicMessageSubmitTransaction.execute()`
1662+
returns `responses[0] if responses else None`. The `None` branch is unreachable
1663+
in practice (the loop always runs at least once for `num_chunks >= 2`) but
1664+
it makes the declared return type `TransactionReceipt | None`, which is
1665+
inconsistent with the single-chunk path. Flag as **MINOR**.
1666+
1667+
### 7) Tests / examples (regression protection)
1668+
- Prefer tests that assert on produced protobufs (fields set, oneofs, ids).
1669+
- Cover: valid submit, missing topic id, empty message, large message boundary (if enforced), invalid checksum/network.
1670+
- Missing critical tests is **MAJOR** (or **MINOR** if coverage exists elsewhere).
1671+
1672+
---
1673+
1674+
## Severity labels
1675+
Use exactly these labels in all findings:
1676+
- **BLOCKER** — must be fixed before merge; wire-format, signing, or data-loss risk
1677+
- **MAJOR** — should be fixed; correctness or significant usability issue
1678+
- **MINOR** — should be fixed; low-risk correctness or ergonomics issue
1679+
- **NIT** — optional; style, naming, or documentation only
14271680
14281681
chat:
14291682
art: false # Don't draw ASCII art (false)

‎CHANGELOG.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# Changelog
1+
# Changelog
22

33
All notable changes to this project will be documented in this file.
44
This project adheres to [Semantic Versioning](https://semver.org).
@@ -12,6 +12,8 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.
1212
- Added CodeRabbit review instructions for the transaction module in `.coderabbit.yaml` (#1696)
1313
- Added CodeRabbit review instructions and path mapping for the schedule module (`src/hiero_sdk_python/schedule/`) in `.coderabbit.yaml` (#1698)
1414
- Added advanced code review prompts for the `src/hiero_sdk_python/file` module in `.coderabbit.yaml` to guide reviewers in verifying proper `FileAppendTransaction` chunking constraints and nuances in memo handling for `FileUpdateTransaction` according to Hiero SDK best practices. (#1697)
15+
- Added CodeRabbit review instructions for consensus module `src/hiero_sdk_python/consensus/` with critical focus on protobuf alignment `.coderabbit.yaml`.
16+
1517

1618
### Fixed
1719

0 commit comments

Comments
 (0)