feat: add staking_info fields to ContractInfo#1744
feat: add staking_info fields to ContractInfo#1744Mounil2005 wants to merge 4 commits intohiero-ledger:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds staking-related metadata fields to ContractInfo so contract staking targets and reward preference can be round-tripped through protobuf serialization/deserialization.
Changes:
- Add
staked_account_id,staked_node_id, anddecline_staking_rewardfields toContractInfo. - Update
ContractInfo._from_proto()to deserializestaking_infointo the new fields. - Update
ContractInfo._to_proto()and add unit tests covering staking scenarios and round-trip conversions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/hiero_sdk_python/contract/contract_info.py |
Adds staking fields and (de)serialization logic for staking_info. |
tests/unit/contract_info_test.py |
Extends unit tests to validate staking field initialization and proto round-trips. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| staking_info=StakingInfo( | ||
| staked_account_id=self.staked_account_id._to_proto() if self.staked_account_id else None, | ||
| staked_node_id=self.staked_node_id if self.staked_node_id else None, | ||
| decline_reward=self.decline_staking_reward | ||
| ), |
There was a problem hiding this comment.
_to_proto() always constructs and populates staking_info, even when no staking fields are set. This makes staking_info appear present on the wire with default values, and the truthiness checks also drop valid values like staked_node_id=0. Build and set staking_info only when at least one of the staking fields is non-None, use is not None for staked_node_id, and avoid passing None for decline_reward (either omit it or coerce to bool when provided).
| assert contract_info.decline_staking_reward is True | ||
|
|
||
|
|
||
| def test_to_proto_with_staked_account_id(token_relationship): |
There was a problem hiding this comment.
token_relationship fixture parameter is unused in this test, which causes unnecessary fixture setup and can hide accidental dependencies. Remove the parameter (or use it) to keep the test focused.
| def test_to_proto_with_staked_account_id(token_relationship): | |
| def test_to_proto_with_staked_account_id(): |
| def test_to_proto_with_staked_node_id(): | ||
| """Test to_proto with staked_node_id""" | ||
| contract_info = ContractInfo( | ||
| contract_id=ContractId(0, 0, 200), | ||
| account_id=AccountId(0, 0, 300), | ||
| balance=5000000, | ||
| staked_node_id=5, | ||
| decline_staking_reward=True, | ||
| ) | ||
|
|
||
| proto = contract_info._to_proto() | ||
|
|
||
| assert proto.HasField('staking_info') | ||
| assert proto.staking_info.staked_node_id == 5 | ||
| assert proto.staking_info.decline_reward is True | ||
|
|
There was a problem hiding this comment.
The new staking serialization logic would benefit from a regression test that covers the "no staking fields set" case during _to_proto() (e.g., ensure staking_info is not set/serialized when staked_account_id, staked_node_id, and decline_staking_reward are all None). This helps prevent round-trip conversions from turning None into default values unintentionally.
|
|
||
| def test_from_proto_with_staked_node_id(): | ||
| """Test from_proto with staked_node_id (staked to node)""" | ||
| public_key = PrivateKey.generate_ed25519().public_key() |
There was a problem hiding this comment.
Variable public_key is not used.
| public_key = PrivateKey.generate_ed25519().public_key() |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1744 +/- ##
=======================================
Coverage 93.29% 93.30%
=======================================
Files 141 141
Lines 9118 9112 -6
=======================================
- Hits 8507 8502 -5
+ Misses 611 610 -1 🚀 New features to boost your workflow:
|
WalkthroughAdds three optional staking fields— Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
| staking_info=StakingInfo( | ||
| staked_account_id=self.staked_account_id._to_proto() if self.staked_account_id else None, | ||
| staked_node_id=self.staked_node_id if self.staked_node_id else None, | ||
| decline_reward=self.decline_staking_reward | ||
| ), |
There was a problem hiding this comment.
Bug: _to_proto always emits staking_info, breaking round-trips when no staking fields are set.
When all staking fields are None, this still creates a StakingInfo() message. Protobuf will then report HasField('staking_info') as true on deserialization, causing _from_proto to set decline_staking_reward = False (protobuf default) instead of None. This breaks the None → None round-trip invariant.
Additionally, on line 161, staked_node_id if self.staked_node_id else None uses a falsy check — node ID 0 is a valid value but would be silently dropped.
Proposed fix: conditionally build staking_info, use `is not None` checks
- staking_info=StakingInfo(
- staked_account_id=self.staked_account_id._to_proto() if self.staked_account_id else None,
- staked_node_id=self.staked_node_id if self.staked_node_id else None,
- decline_reward=self.decline_staking_reward
- ),
+ staking_info=self._build_staking_info_proto(),Then add a helper method:
def _build_staking_info_proto(self):
"""Build StakingInfo proto only if any staking field is set."""
if (self.staked_account_id is None
and self.staked_node_id is None
and self.decline_staking_reward is None):
return None
kwargs = {}
if self.staked_account_id is not None:
kwargs['staked_account_id'] = self.staked_account_id._to_proto()
if self.staked_node_id is not None:
kwargs['staked_node_id'] = self.staked_node_id
if self.decline_staking_reward is not None:
kwargs['decline_reward'] = self.decline_staking_reward
return StakingInfo(**kwargs)
tests/unit/contract_info_test.py
Outdated
| def test_from_proto_with_staked_node_id(): | ||
| """Test from_proto with staked_node_id (staked to node)""" | ||
| public_key = PrivateKey.generate_ed25519().public_key() | ||
| proto = ContractGetInfoResponse.ContractInfo( | ||
| contractID=ContractId(0, 0, 200)._to_proto(), | ||
| accountID=AccountId(0, 0, 300)._to_proto(), | ||
| storage=1024, | ||
| balance=5000000, | ||
| staking_info=StakingInfo( | ||
| staked_node_id=3, | ||
| decline_reward=True, | ||
| ), | ||
| ) | ||
|
|
||
| contract_info = ContractInfo._from_proto(proto) | ||
|
|
||
| assert contract_info.staked_account_id is None | ||
| assert contract_info.staked_node_id == 3 | ||
| assert contract_info.decline_staking_reward is True |
There was a problem hiding this comment.
Unused variable public_key (static analysis: Ruff F841).
public_key is assigned but never used in this test. Remove it to keep the test clean.
Proposed fix
def test_from_proto_with_staked_node_id():
"""Test from_proto with staked_node_id (staked to node)"""
- public_key = PrivateKey.generate_ed25519().public_key()
proto = ContractGetInfoResponse.ContractInfo(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_from_proto_with_staked_node_id(): | |
| """Test from_proto with staked_node_id (staked to node)""" | |
| public_key = PrivateKey.generate_ed25519().public_key() | |
| proto = ContractGetInfoResponse.ContractInfo( | |
| contractID=ContractId(0, 0, 200)._to_proto(), | |
| accountID=AccountId(0, 0, 300)._to_proto(), | |
| storage=1024, | |
| balance=5000000, | |
| staking_info=StakingInfo( | |
| staked_node_id=3, | |
| decline_reward=True, | |
| ), | |
| ) | |
| contract_info = ContractInfo._from_proto(proto) | |
| assert contract_info.staked_account_id is None | |
| assert contract_info.staked_node_id == 3 | |
| assert contract_info.decline_staking_reward is True | |
| def test_from_proto_with_staked_node_id(): | |
| """Test from_proto with staked_node_id (staked to node)""" | |
| proto = ContractGetInfoResponse.ContractInfo( | |
| contractID=ContractId(0, 0, 200)._to_proto(), | |
| accountID=AccountId(0, 0, 300)._to_proto(), | |
| storage=1024, | |
| balance=5000000, | |
| staking_info=StakingInfo( | |
| staked_node_id=3, | |
| decline_reward=True, | |
| ), | |
| ) | |
| contract_info = ContractInfo._from_proto(proto) | |
| assert contract_info.staked_account_id is None | |
| assert contract_info.staked_node_id == 3 | |
| assert contract_info.decline_staking_reward is True |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 353-353: Local variable public_key is assigned to but never used
Remove assignment to unused variable public_key
(F841)
| def test_to_proto_with_staked_account_id(token_relationship): | ||
| """Test to_proto with staked_account_id""" | ||
| contract_info = ContractInfo( | ||
| contract_id=ContractId(0, 0, 200), | ||
| account_id=AccountId(0, 0, 300), | ||
| balance=5000000, | ||
| staked_account_id=AccountId(0, 0, 500), | ||
| decline_staking_reward=False, | ||
| ) | ||
|
|
||
| proto = contract_info._to_proto() | ||
|
|
||
| assert proto.HasField('staking_info') | ||
| assert proto.staking_info.HasField('staked_account_id') | ||
| assert proto.staking_info.staked_account_id == AccountId(0, 0, 500)._to_proto() | ||
| assert proto.staking_info.decline_reward is False |
There was a problem hiding this comment.
Unused fixture argument token_relationship (static analysis: Ruff ARG001).
The token_relationship parameter is injected but never used. Remove it from the function signature.
Proposed fix
-def test_to_proto_with_staked_account_id(token_relationship):
+def test_to_proto_with_staked_account_id():📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_to_proto_with_staked_account_id(token_relationship): | |
| """Test to_proto with staked_account_id""" | |
| contract_info = ContractInfo( | |
| contract_id=ContractId(0, 0, 200), | |
| account_id=AccountId(0, 0, 300), | |
| balance=5000000, | |
| staked_account_id=AccountId(0, 0, 500), | |
| decline_staking_reward=False, | |
| ) | |
| proto = contract_info._to_proto() | |
| assert proto.HasField('staking_info') | |
| assert proto.staking_info.HasField('staked_account_id') | |
| assert proto.staking_info.staked_account_id == AccountId(0, 0, 500)._to_proto() | |
| assert proto.staking_info.decline_reward is False | |
| def test_to_proto_with_staked_account_id(): | |
| """Test to_proto with staked_account_id""" | |
| contract_info = ContractInfo( | |
| contract_id=ContractId(0, 0, 200), | |
| account_id=AccountId(0, 0, 300), | |
| balance=5000000, | |
| staked_account_id=AccountId(0, 0, 500), | |
| decline_staking_reward=False, | |
| ) | |
| proto = contract_info._to_proto() | |
| assert proto.HasField('staking_info') | |
| assert proto.staking_info.HasField('staked_account_id') | |
| assert proto.staking_info.staked_account_id == AccountId(0, 0, 500)._to_proto() | |
| assert proto.staking_info.decline_reward is False |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 372-372: Unused function argument: token_relationship
(ARG001)
|
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! |
| staked_account_id: Optional[AccountId] = None | ||
| staked_node_id: Optional[int] = None | ||
| decline_staking_reward: Optional[bool] = None |
There was a problem hiding this comment.
There is StakingInfo class in the python SDK. Why don't we use it instead?
| staked_account_id: Optional[AccountId] = None | |
| staked_node_id: Optional[int] = None | |
| decline_staking_reward: Optional[bool] = None | |
| staking_info: Optional[StakingInfo] = None |
There was a problem hiding this comment.
Thanks for your suggestion! I have done it.
There was a problem hiding this comment.
@Mounil2005,
I still don’t see the changes updated in the PR. could you please confirm whether you have pushed the latest updates?
|
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, |
Mounil2005
force-pushed
the
add-staking-info-to-contract-info
branch
from
bcbcd6f to
98115be
Compare
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/contract_info_test.py (1)
234-252: 🛠️ Refactor suggestion | 🟠 MajorAdd staking assertions to the
None-values_to_prototest.This test exercises
_to_protowith a default-constructedContractInfobut doesn't verify staking behavior. Given the_to_protobug (always emitting aStakingInfo), asserting here would document the expected behavior and catch regressions.assert proto.max_automatic_token_associations == 0 # 0 is default for protobuf int + # When all staking fields are None, staking_info should not be present + assert not proto.HasField('staking_info'), "staking_info should be absent when all staking fields are None"This assertion will currently fail, confirming the
_to_protobug. Once the production code is fixed, this test will pass and serve as a regression guard.
exploreriii
added
status: needs triage review
exploreriii
added
status: needs developer revision
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Mounil2005
force-pushed
the
add-staking-info-to-contract-info
branch
from
98115be to
8fe7319
Compare
Signed-off-by: Mounil <mounilkankhara@gmail.com>
exploreriii
removed
the
status: needs developer revision
exploreriii
added
status: needs triage review
| ### Added | ||
| - Add staking_info fields (`staked_account_id`, `staked_node_id`, `decline_staking_reward`) to ContractInfo class to expose staking metadata from protobuf. (#1365) |
There was a problem hiding this comment.
Missing blank line below heading (MD022).
The ### Added heading on line 14 needs a blank line below it before the list item, per Markdown lint rules (MD022).
Proposed fix
### Added
+
- Add staking_info fields (`staked_account_id`, `staked_node_id`, `decline_staking_reward`) to ContractInfo class to expose staking metadata from protobuf. (`#1365`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Added | |
| - Add staking_info fields (`staked_account_id`, `staked_node_id`, `decline_staking_reward`) to ContractInfo class to expose staking metadata from protobuf. (#1365) | |
| ### Added | |
| - Add staking_info fields (`staked_account_id`, `staked_node_id`, `decline_staking_reward`) to ContractInfo class to expose staking metadata from protobuf. (`#1365`) |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 14-14: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
tests/unit/contract_info_test.py
Outdated
| assert converted.staked_account_id == contract_info.staked_account_id | ||
| assert converted.staked_node_id == contract_info.staked_node_id | ||
| assert converted.decline_staking_reward == contract_info.decline_staking_reward |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good round-trip assertions for staking fields on fully populated object.
Consider adding assert isinstance(converted.staked_account_id, AccountId) for type safety.
tests/unit/contract_info_test.py
Outdated
| def test_proto_conversion_staking_node_round_trip(): | ||
| """Test proto conversion round trip with staked_node_id""" | ||
| contract_info = ContractInfo( | ||
| contract_id=ContractId(0, 0, 200), | ||
| account_id=AccountId(0, 0, 300), | ||
| balance=5000000, | ||
| staked_node_id=7, | ||
| decline_staking_reward=False, | ||
| ) | ||
|
|
||
| converted = ContractInfo._from_proto(contract_info._to_proto()) | ||
|
|
||
| assert converted.contract_id == contract_info.contract_id | ||
| assert converted.account_id == contract_info.account_id | ||
| assert converted.balance == contract_info.balance | ||
| assert converted.staked_account_id is None | ||
| assert converted.staked_node_id == 7 | ||
| assert converted.decline_staking_reward is False | ||
|
|
||
|
|
||
| def test_proto_conversion_staking_account_round_trip(): | ||
| """Test proto conversion round trip with staked_account_id""" | ||
| contract_info = ContractInfo( | ||
| contract_id=ContractId(0, 0, 200), | ||
| account_id=AccountId(0, 0, 300), | ||
| balance=5000000, | ||
| staked_account_id=AccountId(0, 0, 600), | ||
| decline_staking_reward=True, | ||
| ) | ||
|
|
||
| converted = ContractInfo._from_proto(contract_info._to_proto()) | ||
|
|
||
| assert converted.contract_id == contract_info.contract_id | ||
| assert converted.account_id == contract_info.account_id | ||
| assert converted.balance == contract_info.balance | ||
| assert converted.staked_account_id == AccountId(0, 0, 600) | ||
| assert converted.staked_node_id is None | ||
| assert converted.decline_staking_reward is True |
There was a problem hiding this comment.
Missing edge-case tests: staked_node_id=0 and round-trip with no staking info.
Two important gaps:
staked_node_id=0— Node 0 is a valid Hedera node. The current_to_protofalsy check will silently drop it. A test withstaked_node_id=0would catch this regression.- Round-trip with all staking fields
None— AContractInfo()with default staking fields should survive_to_proto() → _from_proto()with fields stillNone. This would surface the always-emittingstaking_infobug in_to_proto. isinstancecheck — Addassert isinstance(converted.staked_account_id, AccountId)in round-trip tests to protect against type regressions.
As per coding guidelines, "Cover happy paths AND unhappy paths/edge cases" and "Assert return types where relevant (e.g., assert isinstance(result, AccountId))".
|
|
||
| def test_from_proto_with_staked_node_id(): | ||
| """Test from_proto with staked_node_id (staked to node)""" | ||
| public_key = PrivateKey.generate_ed25519().public_key() |
|
|
||
| ### Examples | ||
|
|
||
| ### Added |
There was a problem hiding this comment.
move this under Src section
exploreriii
added
status: needs developer revision
Signed-off-by: Mounil <mounilkankhara@gmail.com>
| staked_account_id: Optional[AccountId] = None | ||
| staked_node_id: Optional[int] = None | ||
| decline_staking_reward: Optional[bool] = None | ||
| staking_info: Optional[StakingInfo] = None |
There was a problem hiding this comment.
the removal of staked_account_id and the other fields introduces breaking change. If someone (like in the integration tests) access the staked_account_id directly it will break for them. @exploreriii, what do you think?
|
You should also add checks for the |
4 participants
Description:
Add staking_info fields to ContractInfo class to support staking metadata from blockchain
Related issue(s):
Fixes #1365
Notes for reviewer:
Implementation follows the same pattern used in AccountInfo for consistency. All changes are isolated to ContractInfo and its unit tests. 17 unit tests pass including:
Checklist