Conversation
Added knowledge base and web search configuration to .coderabbit.yaml Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
Add instructions for reviewing consensus-related Python files. Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
This module defines the TokenAssociation class, which provides an immutable representation of token-account associations as reported by the Hedera network. It includes methods for converting to and from protobuf representations. Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
Updated the TransactionRecord class to include new attributes and improve documentation. Enhanced type hints and refined the __repr__ method for better readability. Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
|
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. WalkthroughAdds a frozen TokenAssociation dataclass with protobuf conversions and substantially refactors TransactionRecord to add many strongly-typed fields, new public attributes, and extended protobuf (de)serialization; also adds Protobuf Schema Alignment review blocks and a knowledge_base flag in .coderabbit.yaml. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
Hi @MonaaEid, this is **LinkBot** 👋
Linking pull requests to issues helps us significantly with reviewing pull requests and keeping the repository healthy. 🚨 This pull request does not have an issue linked. Please link an issue using the following format: 📖 Guide: If no issue exists yet, please create one: Thanks! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/transaction/transaction_record.py (1)
365-382:⚠️ Potential issue | 🔴 CriticalMissing mutual-exclusion guard for
oneof entropy(prng_number/prng_bytes) in_to_proto.There is a correct guard for the
bodyoneof (lines 366-370), but theentropyoneof fieldsprng_numberandprng_bytesare both passed directly in the constructor (lines 380-381). If both are non-None, protobuf silently keeps only the last one written, causing silent data loss. Additionally, setting these in the constructor arguments simultaneously is problematic — they should be set conditionally likecontractCreateResult.Proposed fix
+ # Defensive guard for protobuf oneof (entropy) + if self.prng_number is not None and self.prng_bytes is not None: + raise ValueError( + "prng_number and prng_bytes are mutually exclusive " + "(protobuf oneof entropy) — only one may be set at a time" + ) + record_proto = transaction_record_pb2.TransactionRecord( transactionHash=self.transaction_hash, memo=self.transaction_memo, transactionFee=self.transaction_fee, receipt=self.receipt._to_proto() if self.receipt else None, contractCallResult=( self.call_result._to_proto() if self.call_result else None ), - prng_number=self.prng_number, - prng_bytes=self.prng_bytes, ) + if self.prng_number is not None: + record_proto.prng_number = self.prng_number + elif self.prng_bytes is not None: + record_proto.prng_bytes = self.prng_bytesAs per coding guidelines, "Verify that
_to_protodoes NOT set more than one field from the sameoneofblock simultaneously. If it does, aValueErrorguard MUST be present before setting those fields."
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/hiero_sdk_python/tokens/token_association.pysrc/hiero_sdk_python/transaction/transaction_record.py
| """ | ||
|
|
||
| from dataclasses import dataclass | ||
| from typing import Optional |
There was a problem hiding this comment.
Style inconsistency: use X | None instead of Optional[X].
The codebase convention uses Python 3.10+ union syntax (X | None). The Optional import and its usages should be replaced for consistency — especially since the sibling file transaction_record.py already uses X | None for all its fields.
Proposed fix
-from typing import Optional
-
from hiero_sdk_python.account.account_id import AccountId
from hiero_sdk_python.tokens.token_id import TokenId
from hiero_sdk_python.hapi.services.basic_types_pb2 import TokenAssociation as TokenAssociationProto- token_id: Optional[TokenId] = None
+ token_id: TokenId | None = None
"""The identifier of the token being associated."""
- account_id: Optional[AccountId] = None
+ account_id: AccountId | None = None
"""The identifier of the account receiving the association."""As per coding guidelines, "The codebase uses X | None union syntax (Python 3.10+). Flag any use of Optional[X] from typing in newly added or modified code, as it is inconsistent with the established style."
Also applies to: 50-50, 53-53
| transaction_id: The unique identifier of the transaction. | ||
| transaction_hash: The raw SHA-384 hash of the signed transaction bytes. | ||
| memo: Optional text memo attached to the transaction. | ||
| transaction_fee: Total network fee charged (in tinybars). | ||
| receipt: Summary of transaction outcome (status, account & file IDs created, etc.). | ||
| contract_call_result: Result of a ContractCall transaction (if applicable). | ||
| contract_create_result: Result of a ContractCreate transaction (if applicable). | ||
| token_transfers: Fungible token movements (token → account → amount). | ||
| nft_transfers: Non-fungible token (NFT) movements (token → list of serial transfers). | ||
| transfers: HBAR / account balance changes (account → net amount in tinybars). | ||
| new_pending_airdrops: Newly created pending airdrop records from this transaction. | ||
| prng_number: 32-bit pseudo-random number (if PRNG service was used). | ||
| prng_bytes: Variable-length pseudo-random bytes (if PRNG service was used). | ||
| consensus_timestamp: A consensus timestamp. (This SHALL be null if the transaction did not reach consensus yet.) | ||
| parent_consensus_timestamp: A consensus timestamp for a child record. (This SHALL be the consensus timestamp of a user transaction that spawned an internal child transaction.) | ||
| schedule_ref: A schedule reference. (The reference to a schedule ID for the schedule that initiated this transaction, if this transaction record represents a scheduled transaction.) | ||
| assessed_custom_fees: A list of all custom fees that were assessed during a CryptoTransfer. (These SHALL be paid if the transaction status resolved to SUCCESS.) | ||
| automatic_token_associations: Token associations automatically created. | ||
| alias: EVM-style alias bytes (usually for account creation via ECDSA key). | ||
| ethereum_hash: 32-byte hash used in Ethereum-compatible transaction format. | ||
| evm_address: 20-byte EVM-compatible address (if created or referenced). | ||
| paid_staking_rewards: A list of staking rewards paid. (This SHALL be a list of accounts with the corresponding staking rewards paid as a result of this transaction.) | ||
| """ |
There was a problem hiding this comment.
Docstring attribute names do not match actual field names.
The docstring documents memo (line 48) but the attribute is transaction_memo (line 71). Similarly, it documents contract_call_result (line 51) but the attribute is call_result (line 74). Users relying on the docstring will reference non-existent attributes.
Proposed fix (update docstring to match actual attribute names)
- memo: Optional text memo attached to the transaction.
+ transaction_memo: Optional text memo attached to the transaction.
...
- contract_call_result: Result of a ContractCall transaction (if applicable).
+ call_result: Result of a ContractCall transaction (if applicable).Also applies to: 69-74
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/transaction/transaction_record.py (1)
373-390:⚠️ Potential issue | 🟠 MajorAdd a guard for
oneof entropybefore serializing PRNG fields.Proto field:
prng_bytes(19),prng_number(20)
Issue type: oneof violation
Both fields are currently set in the constructor without validation. If bothprng_numberandprng_bytesare non-None, protobuf's oneof semantics will silently overwrite one field with the other.🛡️ Proposed fix
# Defensive guard for protobuf oneof if self.call_result is not None and self.contract_create_result is not None: raise ValueError( "call_result and contract_create_result are mutually exclusive " "(protobuf oneof) — only one may be set at a time" ) + + if self.prng_number is not None and self.prng_bytes is not None: + raise ValueError( + "prng_number and prng_bytes are mutually exclusive " + "(protobuf oneof) — only one may be set at a time" + ) record_proto = transaction_record_pb2.TransactionRecord( transactionHash=self.transaction_hash, memo=self.transaction_memo, transactionFee=self.transaction_fee, receipt=self.receipt._to_proto() if self.receipt else None, contractCallResult=( self.call_result._to_proto() if self.call_result else None ), - prng_number=self.prng_number, - prng_bytes=self.prng_bytes, ) + + if self.prng_number is not None: + record_proto.prng_number = self.prng_number + elif self.prng_bytes is not None: + record_proto.prng_bytes = self.prng_bytes
♻️ Duplicate comments (1)
src/hiero_sdk_python/transaction/transaction_record.py (1)
45-53:⚠️ Potential issue | 🟡 MinorDocstring still references stale attribute names.
memo/contract_call_resultin the docstring do not match actual dataclass fieldstransaction_memo/call_result, which can mislead users of the public API docs.📝 Proposed fix
- memo: Optional text memo attached to the transaction. + transaction_memo: Optional text memo attached to the transaction. ... - contract_call_result: Result of a ContractCall transaction (if applicable). + call_result: Result of a ContractCall transaction (if applicable).Also applies to: 69-75
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/hiero_sdk_python/tokens/token_association.pysrc/hiero_sdk_python/transaction/transaction_record.py
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. Quick Fix for CHANGELOG.md ConflictsIf your conflict is only in CHANGELOG.md, you can resolve it easily using the GitHub web editor:
For all other merge conflicts, please read: Thank you for contributing! |
|
Hi there! I'm the LinkedIssueBot.
Thank you, |
Added instructions for reviewing Python files in the account directory, including review priorities and architectural invariants. Signed-off-by: MontyPokemon <59332150+MonaaEid@users.noreply.github.com>
| knowledge_base: # Enable knowledge base access for up-to-date information | ||
| web_search: # Enable web search for up-to-date information | ||
| enabled: true |
There was a problem hiding this comment.
knowledge_base is incorrectly nested under reviews: — move to root level.
According to the CodeRabbit configuration schema, knowledge_base is a top-level property (sibling to reviews), not a child of reviews. The reviews object has additionalProperties: false, so this nested placement will either be ignored or cause validation errors.
🐛 Proposed fix — move to root level
reviews:
profile: "assertive"
high_level_summary: false
# ... other review settings ...
enable_prompt_for_ai_agents: false
- knowledge_base:
- web_search:
- enabled: true
+knowledge_base:
+ web_search:
+ enabled: true
# TOKEN REVIEW INSTRUCTIONS
token_review_instructions: &token_review_instructions |Place knowledge_base: at the same indentation level as reviews: and chat:.
|
Hi, this is WorkflowBot.
|
|
Hi there! I'm the LinkedIssueBot.
Thank you, |
1 participant
No description provided.