chore: Refactored AccountInfo class to use the staking_info#1595
chore: Refactored AccountInfo class to use the staking_info#1595mukundkumarjha wants to merge 4 commits intohiero-ledger:mainfrom
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:
WalkthroughAccountInfo was refactored to replace flattened staking fields with a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/hiero_sdk_python/account/account_info.py (5)
12-12: Wrong import: Using protobuf type instead of SDK class.The import pulls
StakingInfofrom the protobuf definitions, but based on the PR objectives and the SDK'sStakingInfoclass atsrc/hiero_sdk_python/staking_info.py, you should import the SDK wrapper class which has the_from_proto()and_to_proto()methods.🔧 Proposed fix
-from hiero_sdk_python.hapi.services.basic_types_pb2 import StakingInfo +from hiero_sdk_python.staking_info import StakingInfo
81-110: Critical bug:staking_infois parsed but never stored.The
staking_infovariable on lines 104-108 is assigned but never used (confirmed by Ruff F841). The parsed staking info is lost because it's not stored inaccount_info. Additionally, the SDK'sStakingInfoclass uses_from_proto(with underscore) per conventions.🐛 Proposed fix
Include
staking_infoin the constructor call or assign it after:account_info: "AccountInfo" = cls( account_id=AccountId._from_proto(proto.accountID) if proto.accountID else None, contract_account_id=proto.contractAccountID, is_deleted=proto.deleted, proxy_received=Hbar.from_tinybars(proto.proxyReceived), key=PublicKey._from_proto(proto.key) if proto.key else None, balance=Hbar.from_tinybars(proto.balance), receiver_signature_required=proto.receiverSigRequired, expiration_time=( Timestamp._from_protobuf(proto.expirationTime) if proto.expirationTime else None ), auto_renew_period=( Duration._from_proto(proto.autoRenewPeriod) if proto.autoRenewPeriod else None ), token_relationships=[ TokenRelationship._from_proto(relationship) for relationship in proto.tokenRelationships ], account_memo=proto.memo, owned_nfts=proto.ownedNfts, max_automatic_token_associations=proto.max_automatic_token_associations, + staking_info=( + StakingInfo._from_proto(proto.staking_info) + if proto.HasField("staking_info") + else None + ), ) - staking_info=( - StakingInfo.from_proto(proto.staking_info) - if proto.HasField("staking_info") - else None - ) - return account_info
161-179: Incorrect__str__implementation: Displaying full object instead of specific fields.Lines 161-162 both display the entire
staking_infoobject with different labels, and line 179 does the same for decline reward. The implementation should access specific fields from theStakingInfoobject.🔧 Proposed fix
simple_fields = [ (self.account_id, "Account ID"), (self.contract_account_id, "Contract Account ID"), (self.balance, "Balance"), (self.key, "Key"), (self.account_memo, "Memo"), (self.owned_nfts, "Owned NFTs"), (self.max_automatic_token_associations, "Max Automatic Token Associations"), - (self.staking_info, "Staked Account ID"), - (self.staking_info, "Staked Node ID"), + (self.staking_info.staked_account_id if self.staking_info else None, "Staked Account ID"), + (self.staking_info.staked_node_id if self.staking_info else None, "Staked Node ID"), (self.proxy_received, "Proxy Received"), (self.expiration_time, "Expiration Time"), (self.auto_renew_period, "Auto Renew Period"), ] ... if self.staking_info is not None: - lines.append(f"Decline Staking Reward: {self.staking_info}") + lines.append(f"Decline Staking Reward: {self.staking_info.decline_reward}")
186-199: Incorrect__repr__implementation: Displaying full object instead of specific fields.The
__repr__method labels fields asstaked_node_idandstaked_account_idbut displays the entirestaking_infoobject for both. It should either display the fullstaking_infoobject once, or access specific fields.🔧 Proposed fix (Option 1: Display staking_info as single field)
def __repr__(self) -> str: """Returns a string representation of the AccountInfo object for debugging.""" return ( f"AccountInfo(" f"account_id={self.account_id!r}, " f"contract_account_id={self.contract_account_id!r}, " f"is_deleted={self.is_deleted!r}, " f"balance={self.balance!r}, " f"receiver_signature_required={self.receiver_signature_required!r}, " f"owned_nfts={self.owned_nfts!r}, " f"account_memo={self.account_memo!r}, " - f"staked_node_id={self.staking_info!r}, " - f"staked_account_id={self.staking_info!r}" + f"staking_info={self.staking_info!r}" f")" )
41-44: Docstring references removed fields.The docstring still documents
staked_account_id,staked_node_id, anddecline_staking_rewardas separate attributes, but these were replaced by the unifiedstaking_infofield.📝 Proposed fix
- staked_account_id (Optional[AccountId]): The account to which this account is staked. - staked_node_id (Optional[int]): The node to which this account is staked. - decline_staking_reward (bool): Whether this account declines receiving staking rewards. + staking_info (Optional[StakingInfo]): Staking information for this account, including + the staked account/node ID, decline reward preference, and staking rewards.
There was a problem hiding this comment.
Actionable comments posted: 4
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/account/account_info.py (2)
12-12: Wrong import: imports protobuf type instead of SDK class.This imports
StakingInfofrom the protobuf module, but the code expects the SDK'sStakingInfoclass which has_from_proto()and_to_proto()methods. The protobuf type doesn't have these methods and will causeAttributeErrorat runtime.🔧 Proposed fix
-from hiero_sdk_python.hapi.services.basic_types_pb2 import StakingInfo +from hiero_sdk_python.staking_info import StakingInfo
41-44: Docstring references removed fields.The docstring still documents
staked_account_id,staked_node_id, anddecline_staking_rewardas attributes, but these have been replaced bystaking_info. Update the docstring to match the actual fields.📝 Proposed fix for docstring
- staked_account_id (Optional[AccountId]): The account to which this account is staked. - staked_node_id (Optional[int]): The node to which this account is staked. - decline_staking_reward (bool): Whether this account declines receiving staking rewards. + staking_info (Optional[StakingInfo]): Staking-related information for this account.Also applies to: 59-60
♻️ Duplicate comments (1)
src/hiero_sdk_python/account/account_info.py (1)
142-146: Method name still incorrect and indentation inconsistent.The past review flagged that
to_proto()should be_to_proto()per SDK conventions. This appears to still be present despite being marked as addressed. The closing parenthesis indentation is also inconsistent.🔧 Proposed fix
max_automatic_token_associations=self.max_automatic_token_associations, - staking_info=( - self.staking_info.to_proto() - if self.staking_info is not None - else None - ), + staking_info=( + self.staking_info._to_proto() + if self.staking_info is not None + else None + ), )
| if staking_info.HasField('staked_node_id') else None | ||
| ) | ||
| account_info.decline_staking_reward = staking_info.decline_reward | ||
| staking_info=( |
There was a problem hiding this comment.
@mukundkumarjha, this should be added inside the AccountInfo object itself, currenlty is is not included in the object, possibly move it after line 101
| (self.max_automatic_token_associations, "Max Automatic Token Associations"), | ||
| (self.staked_account_id, "Staked Account ID"), | ||
| (self.staked_node_id, "Staked Node ID"), | ||
| (self.staking_info, "Staked Account ID"), |
There was a problem hiding this comment.
better to display this as staking info only
| f"account_memo={self.account_memo!r}, " | ||
| f"staked_node_id={self.staked_node_id!r}, " | ||
| f"staked_account_id={self.staked_account_id!r}" | ||
| f"staked_node_id={self.staking_info!r}, " |
There was a problem hiding this comment.
same with these, better to include stating_info itself
There was a problem hiding this comment.
@mukundkumarjha Thanks for the PR. I’ve added a few required changes.
mukundkumarjha
force-pushed
the
accountinfo
branch
from
38cab37 to
f633b2d
Compare
|
@manishdait thanks for the review I have updated the same |
There was a problem hiding this comment.
Actionable comments posted: 0
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/account/account_info.py (2)
12-12: Critical: Wrong import - imports protobufStakingInfoinstead of SDK class.Line 12 imports from
hiero_sdk_python.hapi.services.basic_types_pb2, which is the protobuf-generated class. The SDK'sStakingInfoclass (with_from_proto()and_to_proto()methods) is located inhiero_sdk_python.staking_info. This causes runtime failures since protobuf classes don't havefrom_proto()/to_proto()methods.🔧 Proposed fix
-from hiero_sdk_python.hapi.services.basic_types_pb2 import StakingInfo +from hiero_sdk_python.staking_info import StakingInfo
41-44: Update docstring to reflect the refactored field.The docstring still documents the removed flattened fields (
staked_account_id,staked_node_id,decline_staking_reward). Update it to document the newstaking_infofield instead.🔧 Proposed fix
- staked_account_id (Optional[AccountId]): The account to which this account is staked. - staked_node_id (Optional[int]): The node to which this account is staked. - decline_staking_reward (bool): Whether this account declines receiving staking rewards. + staking_info (Optional[StakingInfo]): Staking-related information for this account.
♻️ Duplicate comments (5)
src/hiero_sdk_python/account/account_info.py (5)
143-147: Method name should follow SDK conventions with underscore prefix.The SDK's
StakingInfoclass uses_to_proto()(with underscore prefix) per the codebase conventions. The previous review comment flagged this issue but it appears to still be present. Also fix the inconsistent indentation.🔧 Proposed fix
staking_info=( - self.staking_info.to_proto() + self.staking_info._to_proto() if self.staking_info is not None else None - ), + ),
178-179: Incorrect value for "Decline Staking Reward".The label expects a boolean but the value is the entire
StakingInfoobject. This was flagged in a previous review. Access the specific field instead.🔧 Proposed fix
if self.staking_info is not None: - lines.append(f"Decline Staking Reward: {self.staking_info}") + lines.append(f"Decline Staking Reward: {self.staking_info.decline_reward}")
197-199: Incorrect field names in__repr__.The labels
staked_info=(typo) andstaked_account_id=reference removed/incorrect fields, and both incorrectly display the samestaking_infovalue. This was flagged in a previous review. Replace with a singlestaking_info=entry.🔧 Proposed fix
f"account_memo={self.account_memo!r}, " - f"staked_info={self.staking_info!r}, " - f"staked_account_id={self.staking_info!r}" + f"staking_info={self.staking_info!r}" f")"
162-162: Minor: Label inconsistency.The label "Staked Info" should be "Staking Info" for consistency with the field name
staking_info.🔧 Proposed fix
- (self.staking_info, "Staked Info"), + (self.staking_info, "Staking Info"),
102-106: Method name should follow SDK conventions with underscore prefix.The SDK's
StakingInfoclass uses_from_proto()(with underscore prefix) per the codebase conventions shown in the relevant code snippets. Also fix the inconsistent indentation.🔧 Proposed fix
staking_info=( - StakingInfo.from_proto(proto.staking_info) + StakingInfo._from_proto(proto.staking_info) if proto.HasField("staking_info") else None - ) + )
| decline_reward=self.decline_staking_reward | ||
| ), | ||
| staking_info=( | ||
| self.staking_info.to_proto() |
There was a problem hiding this comment.
this should be self.staking_info._to_proto()
| if self.decline_staking_reward is not None: | ||
| lines.append(f"Decline Staking Reward: {self.decline_staking_reward}") | ||
| if self.staking_info is not None: | ||
| lines.append(f"Decline Staking Reward: {self.staking_info}") |
| f"staked_node_id={self.staked_node_id!r}, " | ||
| f"staked_account_id={self.staked_account_id!r}" | ||
| f"staked_info={self.staking_info!r}, " | ||
| f"staked_account_id={self.staking_info!r}" |
There was a problem hiding this comment.
same with this, we can remove it
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@mukundkumarjha Thank you for your contribution!
I concur with @manishdait on the changes.
Additionally, please review your changelog entry.
If you have questions or need assistance, please reach out.
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/account_info_test.py (1)
180-210: Add a non-None staking_info round‑trip test to protect the new behavior.PR objectives require preserving full staking data, but tests only assert
None. Add a test that sets a realStakingInfo(with one of staked_account_id/staked_node_id and decline_reward), runs_to_proto()/_from_proto(), and asserts those fields survive the round trip.
mukundkumarjha
force-pushed
the
accountinfo
branch
from
2a5f281 to
fa034c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/account_info_test.py (1)
58-73: Addhasattrassertions for new public attributes to protect against breaking changes.Per coding guidelines (PRIORITY 1), tests should assert that public attributes exist. This helps catch accidental renames or removals early.
def test_account_info_initialization(account_info): """Test the initialization of the AccountInfo class""" + # Verify new public attributes exist (guards against breaking changes) + assert hasattr(account_info, 'max_automatic_token_associations') + assert hasattr(account_info, 'staking_info') + assert account_info.account_id == AccountId(0, 0, 100) # ... existing assertions ...
mukundkumarjha
force-pushed
the
accountinfo
branch
from
dfb7792 to
fa034c2
Compare
There was a problem hiding this comment.
this should be from hiero_sdk_python.staking_info import StakingInfo since we are now using the StakingInfo class
tests/unit/account_info_test.py
Outdated
| assert account_info.account_memo == "Test account memo" | ||
| assert account_info.owned_nfts == 5 | ||
| assert account_info.max_automatic_token_associations == 10 | ||
| #assert account_info.staking_info == None |
There was a problem hiding this comment.
same for this, test for staking_info not equals None
There was a problem hiding this comment.
Actionable comments posted: 2
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/account/account_info.py (1)
41-44: Docstring references removed fields and omits new ones.The docstring still documents
staked_account_id,staked_node_id, anddecline_staking_rewardwhich were removed by this refactor. It also omits the newstaking_infoandmax_automatic_token_associationsfields.📝 Suggested fix
account_memo (Optional[str]): The memo associated with this account. owned_nfts (Optional[int]): The number of NFTs owned by this account. - staked_account_id (Optional[AccountId]): The account to which this account is staked. - staked_node_id (Optional[int]): The node to which this account is staked. - decline_staking_reward (bool): Whether this account declines receiving staking rewards. + max_automatic_token_associations (Optional[int]): Maximum number of tokens that can be + automatically associated with this account. + staking_info (Optional[StakingInfo]): Staking information for this account, including + staked account/node, pending rewards, and decline reward preference.tests/unit/account_info_test.py (1)
252-267: Add assertions forstaking_infoin string representation tests.Per coding guidelines, tests should verify that new public attributes appear correctly in string outputs. The
test_str_and_reprtest should verifystaking_infois rendered correctly, especially since the PR modifies these methods.📝 Suggested addition
def test_str_and_repr_with_staking_info(): """Test __str__ and __repr__ methods include staking_info when populated""" account_info = AccountInfo( account_id=AccountId(0, 0, 100), staking_info=StakingInfo( decline_reward=True, staked_node_id=5, ) ) info_str = str(account_info) info_repr = repr(account_info) # Verify staking_info appears in string output assert "Staking Info" in info_str # or "Staked Info" per current impl assert "staking_info=" in info_repr # or "staked_info=" per current impl
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
prajeeta15
left a comment
prajeeta15
left a comment
There was a problem hiding this comment.
@mukundkumarjha please address the coderabbitai issues and merge conflicts.
and let us know if you need any assistance :)
CHANGELOG.md
Outdated
| - Added dry-run support and refactored `.github/workflows/bot-workflows.yml` to use dedicated script `.github/scripts/bot-workflows.js` for improved maintainability and testability. (`#1288`) | ||
|
|
||
| ### Changed | ||
| - Refactored AccountInfo class to use the staking_info |
There was a problem hiding this comment.
please write a more descriptive CHANGELOG.md entry with the issue number that is being addressed through this PR.
|
@mukundkumarjha Thank you for your contribution! I concur with @prajeeta15 on the suggestions. Do not hesitate to reach out if you need assistance. |
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Are there any breaking changes in this and what is the migration plan?
|
[commit-verification-bot]
View your commit verification status: Commits Tab. To achieve verified status, please read: Remember, you require a GPG key and each commit must be signed with: Thank you for contributing! From the Hiero Python SDK Team |
mukundkumarjha
force-pushed
the
accountinfo
branch
4 times, most recently
from
a381ba6 to
c1cf669
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/account/account_info.py (1)
41-43:⚠️ Potential issue | 🟡 MinorDocstring references removed fields.
The class docstring still documents
staked_account_id,staked_node_id, anddecline_staking_reward, which were replaced bystaking_info. Update to reflect the current API.Proposed fix
- staked_account_id (Optional[AccountId]): The account to which this account is staked. - staked_node_id (Optional[int]): The node to which this account is staked. - decline_staking_reward (bool): Whether this account declines receiving staking rewards. + max_automatic_token_associations (Optional[int]): Maximum number of token associations. + staking_info (Optional[StakingInfo]): Staking-related information for this account.
mukundkumarjha
force-pushed
the
accountinfo
branch
2 times, most recently
from
cd83eee to
7497dde
Compare
Signed-off-by: mukundkumarjha <mukundiiitg@gmail.com>
Signed-off-by: mukundkumarjha <mukundiiitg@gmail.com>
mukundkumarjha
force-pushed
the
accountinfo
branch
from
7497dde to
2d5dcd2
Compare
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
You have changed how the original API works, so you have introduced breaking changes
Users who use the old methods, will be unable to run their code, without updating it
import warnings
enable backwards compatibility by deprecating the old fields and emitting a warning
test the old code, and check it still works
@manishdait what do you think?
Agree with this. Some previously exposed fields To maintain backward compatibility, a possible solution is to reintroduce the removed fields as deprecated properties # Example
@property
def decline_staking_reward(self) -> bool:
warnings.warn(
"Use `staking_info.decline_reward` instead.",
FutureWarning,
)
return self.staking_info.decline_reward |
Signed-off-by: mukundkumarjha <mukundiiitg@gmail.com>
Hi @exploreriii @manishdait i have updated the accountinfo class for backward compatibility. |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (66.66%) is below the target coverage (92.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #1595 +/- ##
==========================================
- Coverage 93.28% 93.23% -0.05%
==========================================
Files 141 141
Lines 9100 9107 +7
==========================================
+ Hits 8489 8491 +2
- Misses 611 616 +5 🚀 New features to boost your workflow:
|
|
@mukundkumarjha This is looking great so far. After running the workflows, codecov seems to be fialing. This shows you what was missed. |
exploreriii
added
the
status: pending activity confirmation
exploreriii
removed
the
status: needs developer revision
|
Hi @mukundkumarjha, this is InactivityBot 👋 This pull request has had no new commits for 21 days, so I'm closing it and unassigning you from the linked issue to keep the backlog healthy. If you're no longer interested, no action is needed. Tip: You can comment If you'd like to continue working on this later, feel free to comment |
6 participants
Description:
Related issue(s):
Fixes #1366
Notes for reviewer:
Checklist