feat: add staking_info fields to ContractInfo#1744
feat: add staking_info fields to ContractInfo#1744Mounil2005 wants to merge 2 commits intohiero-ledger:mainfrom
Conversation
Signed-off-by: Mounil <mounilkankhara@gmail.com>
Signed-off-by: Mounil <mounilkankhara@gmail.com>
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).
| 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.
staked_account_id and staked_node_id represent a one-of choice in staking data. Consider enforcing mutual exclusivity (e.g., in __post_init__ or before serialization) so callers can’t set both at once and silently end up serializing only one of them.
| def __post_init__(self) -> None: | |
| if self.staked_account_id is not None and self.staked_node_id is not None: | |
| raise ValueError( | |
| "staked_account_id and staked_node_id are mutually exclusive; only one may be set." | |
| ) |
| 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.25% 93.26%
=======================================
Files 141 141
Lines 9061 9071 +10
=======================================
+ Hits 8450 8460 +10
Misses 611 611 🚀 New features to boost your workflow:
|
WalkthroughAdds staking-related fields ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
| from hiero_sdk_python.contract.contract_id import ContractId | ||
| from hiero_sdk_python.crypto.public_key import PublicKey | ||
| from hiero_sdk_python.Duration import Duration | ||
| from hiero_sdk_python.hapi.services.basic_types_pb2 import StakingInfo |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the structure and find the relevant files
find . -type f -name "*.py" | grep -E "(staking_info|contract_info|account_info)" | head -20Repository: hiero-ledger/hiero-sdk-python
Length of output: 717
🏁 Script executed:
# Check the import and surrounding context in contract_info.py
head -50 src/hiero_sdk_python/contract/contract_info.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 2606
🏁 Script executed:
# Look for the StakingInfo wrapper class
find . -type f -name "staking_info.py" | head -5Repository: hiero-ledger/hiero-sdk-python
Length of output: 111
🏁 Script executed:
# Search for class definitions of AccountInfo and StakingInfo
rg -n "^class (AccountInfo|StakingInfo)" --type pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 281
🏁 Script executed:
# Read the StakingInfo wrapper class
cat -n src/hiero_sdk_python/staking_info.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 6190
🏁 Script executed:
# Read the AccountInfo class to see how it handles staking
cat -n src/hiero_sdk_python/account/account_info.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 11449
🏁 Script executed:
# Read more of ContractInfo to see the full field definitions and how staking is handled
cat -n src/hiero_sdk_python/contract/contract_info.pyRepository: hiero-ledger/hiero-sdk-python
Length of output: 11996
Both AccountInfo and ContractInfo flatten staking fields instead of using the existing StakingInfo wrapper class.
ContractInfo (and AccountInfo) import the raw protobuf StakingInfo and manually extract only staked_account_id, staked_node_id, and decline_reward during deserialization. This approach loses:
- Mutual-exclusivity validation that the
StakingInfowrapper enforces (preventing bothstaked_account_idandstaked_node_idfrom being set) - Fields carried by the proto:
stake_period_start,pending_reward,staked_to_me
The SDK already has a StakingInfo wrapper class in src/hiero_sdk_python/staking_info.py with proper proto conversion and validation. Using it would align with issue #1365 and provide complete, validated staking information.
| 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)| 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)
| 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.
🛠️ Refactor suggestion | 🟠 Major
Missing edge-case and defensive tests for staking fields.
The round-trip tests look good for the happy path. However, per the coding guidelines, consider adding:
- Round-trip with no staking info — verifies that a
ContractInfo()with all staking fields asNonesurvives_to_proto()→_from_proto()with fields stillNone. (This will currently fail due to the_to_protobug flagged above.) staked_node_id=0— node 0 is valid; ensures it isn't dropped by falsy checks.isinstanceassertion — e.g.,assert isinstance(converted.staked_account_id, AccountId)to protect against type regressions.- Both
staked_account_idandstaked_node_idset — depending on whether mutual exclusivity should be enforced at theContractInfolevel.
As per coding guidelines, "Cover happy paths AND unhappy paths/edge cases" and "Assert return types where relevant (e.g., assert isinstance(result, AccountId))".
2 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