chore: wrap staking fields in StakingInfo wrapper class#1914
chore: wrap staking fields in StakingInfo wrapper class#1914Mounil2005 wants to merge 13 commits intohiero-ledger:mainfrom
Conversation
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1914 +/- ##
=======================================
Coverage 93.53% 93.54%
=======================================
Files 141 141
Lines 9146 9190 +44
=======================================
+ Hits 8555 8597 +42
- Misses 591 593 +2 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Refactors AccountInfo to encapsulate staking-related data inside a dedicated staking_info object, aligning the Python SDK with the protobuf StakingInfo model and updating unit/integration tests to use nested field access.
Changes:
- Add
src/hiero_sdk_python/account/staking_info.pywith_from_proto()/_to_proto()plus__str__/__repr__. - Refactor
AccountInfoto replace flattened staking fields withstaking_info: Optional[StakingInfo]. - Update unit + integration tests to access staking fields via
info.staking_info.*.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/hiero_sdk_python/account/staking_info.py |
Introduces a new Account-scoped StakingInfo wrapper with proto conversion + formatting. |
src/hiero_sdk_python/account/account_info.py |
Switches AccountInfo to a single staking_info field and updates proto conversions + formatting. |
tests/unit/account_info_test.py |
Extends proto fixtures + assertions to cover nested staking info conversion. |
tests/integration/account_update_transaction_e2e_test.py |
Updates staking assertions to use nested staking_info access. |
tests/integration/account_create_transaction_e2e_test.py |
Updates staking assertions to use nested staking_info access. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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:
WalkthroughReplaces three flattened staking fields on AccountInfo with a single optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ 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 |
![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)
tests/unit/account_info_test.py (1)
179-196: 🧹 Nitpick | 🔵 TrivialConsider adding explicit assertion for absent staking_info.
The
test_to_proto_with_none_valuestest doesn't assert thatstaking_infois absent whenAccountInfohas no staking_info set. Adding this would strengthen the test.💡 Suggested addition
assert proto.ownedNfts == 0 # 0 is default for protobuf int + assert not proto.HasField('staking_info') # staking_info should not be set when None
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3fd4e52-2887-4c9a-b73b-39f48c243132
📒 Files selected for processing (5)
src/hiero_sdk_python/account/account_info.pysrc/hiero_sdk_python/account/staking_info.pytests/integration/account_create_transaction_e2e_test.pytests/integration/account_update_transaction_e2e_test.pytests/unit/account_info_test.py
|
Note @Mounil2005 watch out for any breaking changes please, and if you must, ensure there is backwards compatibility |
Yes ma'am! Still working on it! |
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)
tests/unit/account_info_test.py (1)
179-196: 🧹 Nitpick | 🔵 TrivialConsider adding assertion for
staking_infoabsence.This test verifies
_to_protobehavior withNonevalues but doesn't assert thatproto.staking_infois unset whenAccountInfo.staking_infoisNone.Suggested addition
assert proto.ownedNfts == 0 # 0 is default for protobuf int + # staking_info should not be set when AccountInfo.staking_info is None + assert not proto.HasField('staking_info')
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e85bcc4c-4b03-4a31-9c12-c29fe698439f
📒 Files selected for processing (4)
CHANGELOG.mdsrc/hiero_sdk_python/account/account_info.pysrc/hiero_sdk_python/account/staking_info.pytests/unit/account_info_test.py
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
@Mounil2005
have you seen src/hiero_sdk_python/staking_info.py?
They are both to do with account staking information, i assume you'll be able to use that one - perhaps you can add sections to it if you see anything missing in there.
We do have some content duplicted in the source code as we have missed possible instances like this :D
exploreriii
added
the
status: needs developer revision
Noted, looking into it. |
Mounil2005
force-pushed
the
chore/Refactor-AccountInfo-class-to-use-the-staking_info
branch
from
56c9719 to
1094ae5
Compare
exploreriii
added
status: needs triage review
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95d48dea-b1e4-460d-a0b0-f3d995f21eec
📒 Files selected for processing (5)
CHANGELOG.mdsrc/hiero_sdk_python/account/account_info.pytests/integration/account_create_transaction_e2e_test.pytests/integration/account_update_transaction_e2e_test.pytests/unit/account_info_test.py
Mounil2005
force-pushed
the
chore/Refactor-AccountInfo-class-to-use-the-staking_info
branch
from
1094ae5 to
d3321db
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/hiero_sdk_python/account/account_info.py (2)
8-8: 🧹 Nitpick | 🔵 TrivialUse
X | Noneunion syntax instead ofOptional[X].The new code at line 58 uses
Optional[StakingInfo]which is inconsistent with the codebase style that prefersX | None. While the existing fields in this file also useOptional[X], new code should follow the modern style.As per coding guidelines: "The codebase uses
X | Noneunion syntax (Python 3.10+). Flag any use ofOptional[X]fromtypingin newly added or modified code."
191-222:⚠️ Potential issue | 🟡 MinorDeprecation shims do not support legacy constructor kwargs.
The deprecated properties provide backward-compatible attribute access (
info.staked_account_id), but constructor usage likeAccountInfo(staked_account_id=...)will raiseTypeErrorbecause these are no longer dataclass fields.If full backward compatibility is required, consider adding a custom
__init__that accepts legacy kwargs and translates them tostaking_info. Otherwise, document this constructor breaking change explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8b133902-282b-4e05-a242-7750c4f39010
📒 Files selected for processing (5)
CHANGELOG.mdsrc/hiero_sdk_python/account/account_info.pytests/integration/account_create_transaction_e2e_test.pytests/integration/account_update_transaction_e2e_test.pytests/unit/account_info_test.py
Mounil2005
force-pushed
the
chore/Refactor-AccountInfo-class-to-use-the-staking_info
branch
from
9d14e9d to
6e988b3
Compare
Mounil2005
force-pushed
the
chore/Refactor-AccountInfo-class-to-use-the-staking_info
branch
from
f66aa3d to
9bf4e81
Compare
Dosik13
left a comment
Dosik13
left a comment
There was a problem hiding this comment.
good work, left some comments
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
…on AccountInfo Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
…mpat Signed-off-by: Mounil <mounilkankhara@gmail.com>
…roperties Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Add @property.setter implementations for staked_account_id, staked_node_id, and decline_staking_reward. This ensures full backwards compatibility for code that both reads AND writes these deprecated fields. When setting a field, if the oneof constraint would be violated (both staked_account_id and staked_node_id set), the conflicting field is cleared automatically. Add 4 regression tests for the setter behavior. Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Mounil2005
force-pushed
the
chore/Refactor-AccountInfo-class-to-use-the-staking_info
branch
from
5dd9449 to
0729769
Compare
|
Hi, this is WorkflowBot.
|
|
|
||
| # --------------------------------------------------------------------------- | ||
| # StakingInfo (staking_info) unit tests | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| class TestStakingInfo: | ||
| """Unit tests for the staking_info.StakingInfo wrapper.""" | ||
|
|
||
| def test_from_proto_none_raises(self): | ||
| """_from_proto(None) should raise ValueError.""" | ||
| with pytest.raises(ValueError, match="Staking info proto is None"): | ||
| StakingInfo._from_proto(None) | ||
|
|
||
| def test_from_proto_staked_node_id(self): | ||
| """_from_proto should populate staked_node_id from the oneof field.""" | ||
| from hiero_sdk_python.hapi.services.basic_types_pb2 import StakingInfo as StakingInfoProto | ||
| proto = StakingInfoProto(decline_reward=False, pending_reward=0, staked_to_me=0) | ||
| proto.staked_node_id = 5 | ||
| si = StakingInfo._from_proto(proto) | ||
| assert si.staked_node_id == 5 | ||
| assert si.staked_account_id is None | ||
|
|
||
| def test_from_proto_staked_node_id_zero(self): | ||
| """staked_node_id=0 should be preserved (valid node ID).""" | ||
| from hiero_sdk_python.hapi.services.basic_types_pb2 import StakingInfo as StakingInfoProto | ||
| proto = StakingInfoProto(decline_reward=False) | ||
| proto.staked_node_id = 0 # 0 is a valid node ID | ||
| si = StakingInfo._from_proto(proto) | ||
| assert si.staked_node_id == 0 | ||
| assert si.staked_account_id is None | ||
|
|
||
| def test_to_proto_staked_node_id(self): | ||
| """_to_proto should set staked_node_id on the proto.""" | ||
| si = StakingInfo(staked_node_id=7, decline_reward=False) | ||
| proto = si._to_proto() | ||
| assert proto.staked_node_id == 7 | ||
| assert not proto.HasField("staked_account_id") | ||
|
|
||
| def test_to_proto_oneof_raises(self): | ||
| """StakingInfo construction should raise ValueError if both staked fields are set.""" | ||
| with pytest.raises(ValueError, match="Only one of"): | ||
| StakingInfo( | ||
| staked_account_id=AccountId(0, 0, 1), | ||
| staked_node_id=2, | ||
| ) | ||
|
|
||
| def test_to_proto_all_fields(self): | ||
| """_to_proto should serialize all optional fields correctly.""" | ||
| si = StakingInfo( | ||
| pending_reward=Hbar.from_tinybars(100), | ||
| staked_to_me=Hbar.from_tinybars(200), | ||
| stake_period_start=Timestamp(1625097600, 0), | ||
| staked_account_id=AccountId(0, 0, 50), | ||
| decline_reward=True, | ||
| ) | ||
| proto = si._to_proto() | ||
| assert proto.pending_reward == 100 | ||
| assert proto.staked_to_me == 200 | ||
| assert proto.decline_reward is True | ||
| assert proto.HasField("stake_period_start") | ||
| assert proto.stake_period_start.seconds == 1625097600 | ||
| assert proto.HasField("staked_account_id") | ||
|
|
||
| def test_to_proto_none_fields(self): | ||
| """_to_proto with all-None fields should return empty proto.""" | ||
| si = StakingInfo() | ||
| proto = si._to_proto() | ||
| assert proto.pending_reward == 0 | ||
| assert proto.staked_to_me == 0 | ||
| assert proto.decline_reward is False | ||
|
|
||
| def test_str_contains_set_fields(self): | ||
| """__str__ should include staked fields.""" | ||
| si = StakingInfo(staked_node_id=3, decline_reward=True) | ||
| s = str(si) | ||
| assert "staked_node_id=3" in s | ||
| assert "decline_reward=True" in s | ||
|
|
||
| def test_str_with_account_id(self): | ||
| """__str__ should include staked_account_id when set.""" | ||
| si = StakingInfo(staked_account_id=AccountId(0, 0, 99)) | ||
| s = str(si) | ||
| assert "staked_account_id=" in s | ||
|
|
||
| def test_repr(self): | ||
| """__repr__ should include all field names.""" | ||
| si = StakingInfo(staked_node_id=1) | ||
| r = repr(si) | ||
| assert r.startswith("StakingInfo(") | ||
| assert "staked_node_id=1" in r | ||
| assert "pending_reward=" in r | ||
| assert "decline_reward=" in r | ||
|
|
There was a problem hiding this comment.
I think this tests shouldn't be in this PR as they belong to the staking_info_test.py. Also, I believe most of the tests are in the staking info test file but if you want you can add more in future PR
|
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, |
Description:
Extract staking information fields from AccountInfo into a dedicated StakingInfo wrapper class following SDK patterns. This improves code organization and maintainability by encapsulating staking-related logic.
_from_proto()and_to_proto()methods__str__()and__repr__()for staking info representationstaking_infofield instead of three flattened fieldsRelated issue(s):
Fixes #1366
Notes for reviewer:
All 2224 unit tests passing. Integration tests updated to use nested field access pattern (
info.staking_info.staked_account_idinstead ofinfo.staked_account_id). No breaking changes to public API beyond the AccountInfo field structure refactor.Checklist