-
Notifications
You must be signed in to change notification settings - Fork 186
chore: Refactored AccountInfo class to use the staking_info #1595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
353824a
f633b2d
d0079c8
fa034c2
44e60ed
2f13541
a2679e0
b704bf6
c0cbbb6
52a4ac1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,6 +124,7 @@ This changelog is based on [Keep a Changelog](https://keepachangelog.com/en/1.1. | |
| - 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 | ||
|
||
| - chore: format tests/unit/mock_server.py with black (#1542) | ||
| - Updated actions/checkout to v6.0.1 and actions/github-script v8.0.0 in bot-next-issue-recommendation workflow (#1586) | ||
| - Expanded inactivity bot messages to include `/unassign` command information for contributors (#1555) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |||||||||||||||||
| from hiero_sdk_python.account.account_id import AccountId | ||||||||||||||||||
| 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 | ||||||||||||||||||
| from hiero_sdk_python.staking_info import StakingInfo | ||||||||||||||||||
| from hiero_sdk_python.hapi.services.crypto_get_info_pb2 import CryptoGetInfoResponse | ||||||||||||||||||
| from hiero_sdk_python.hbar import Hbar | ||||||||||||||||||
| from hiero_sdk_python.timestamp import Timestamp | ||||||||||||||||||
|
|
@@ -56,9 +56,8 @@ class AccountInfo: | |||||||||||||||||
| account_memo: Optional[str] = None | ||||||||||||||||||
| owned_nfts: Optional[int] = None | ||||||||||||||||||
| max_automatic_token_associations: Optional[int] = None | ||||||||||||||||||
| staked_account_id: Optional[AccountId] = None | ||||||||||||||||||
| staked_node_id: Optional[int] = None | ||||||||||||||||||
| decline_staking_reward: Optional[bool] = None | ||||||||||||||||||
| staking_info: Optional[StakingInfo] = None | ||||||||||||||||||
|
|
||||||||||||||||||
|
Comment on lines
58
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Align max_automatic_token_associations default with tests and expected API behavior. Current default is 🔧 Proposed fix- max_automatic_token_associations: Optional[int] = None
+ max_automatic_token_associations: int = 10📝 Committable suggestion
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| @classmethod | ||||||||||||||||||
| def _from_proto(cls, proto: CryptoGetInfoResponse.AccountInfo) -> "AccountInfo": | ||||||||||||||||||
|
|
@@ -100,20 +99,14 @@ def _from_proto(cls, proto: CryptoGetInfoResponse.AccountInfo) -> "AccountInfo": | |||||||||||||||||
| 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 | ||||||||||||||||||
| ) | ||||||||||||||||||
| ) | ||||||||||||||||||
exploreriii marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
|
|
||||||||||||||||||
| staking_info = proto.staking_info if proto.HasField('staking_info') else None | ||||||||||||||||||
|
|
||||||||||||||||||
| if staking_info: | ||||||||||||||||||
| account_info.staked_account_id = ( | ||||||||||||||||||
| AccountId._from_proto(staking_info.staked_account_id) | ||||||||||||||||||
| if staking_info.HasField('staked_account_id') else None | ||||||||||||||||||
| ) | ||||||||||||||||||
| account_info.staked_node_id = ( | ||||||||||||||||||
| staking_info.staked_node_id | ||||||||||||||||||
| if staking_info.HasField('staked_node_id') else None | ||||||||||||||||||
| ) | ||||||||||||||||||
| account_info.decline_staking_reward = staking_info.decline_reward | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| return account_info | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -147,11 +140,11 @@ def _to_proto(self) -> CryptoGetInfoResponse.AccountInfo: | |||||||||||||||||
| memo=self.account_memo, | ||||||||||||||||||
| ownedNfts=self.owned_nfts, | ||||||||||||||||||
| max_automatic_token_associations=self.max_automatic_token_associations, | ||||||||||||||||||
| 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.staking_info._to_proto() | ||||||||||||||||||
| if self.staking_info is not None | ||||||||||||||||||
| else None | ||||||||||||||||||
| ), | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| def __str__(self) -> str: | ||||||||||||||||||
|
|
@@ -166,8 +159,7 @@ def __str__(self) -> str: | |||||||||||||||||
| (self.account_memo, "Memo"), | ||||||||||||||||||
| (self.owned_nfts, "Owned NFTs"), | ||||||||||||||||||
| (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 Info"), | ||||||||||||||||||
| (self.proxy_received, "Proxy Received"), | ||||||||||||||||||
|
Comment on lines
+162
to
163
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use consistent “staking_info” labeling in str / repr. The label “Staked Info” / “staked_info” is inconsistent with the field name and PR objectives, which can confuse users. 🔧 Proposed fix- (self.staking_info, "Staked Info"),
+ (self.staking_info, "Staking Info"),
...
- f"staked_info={self.staking_info!r}, "
+ f"staking_info={self.staking_info!r}, "Also applies to: 194-194 |
||||||||||||||||||
| (self.expiration_time, "Expiration Time"), | ||||||||||||||||||
| (self.auto_renew_period, "Auto Renew Period"), | ||||||||||||||||||
|
|
@@ -182,9 +174,6 @@ def __str__(self) -> str: | |||||||||||||||||
|
|
||||||||||||||||||
| if self.receiver_signature_required is not None: | ||||||||||||||||||
| lines.append(f"Receiver Signature Required: {self.receiver_signature_required}") | ||||||||||||||||||
|
|
||||||||||||||||||
| if self.decline_staking_reward is not None: | ||||||||||||||||||
| lines.append(f"Decline Staking Reward: {self.decline_staking_reward}") | ||||||||||||||||||
|
|
||||||||||||||||||
| if self.token_relationships: | ||||||||||||||||||
| lines.append(f"Token Relationships: {len(self.token_relationships)}") | ||||||||||||||||||
|
|
@@ -202,7 +191,6 @@ def __repr__(self) -> str: | |||||||||||||||||
| 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.staked_node_id!r}, " | ||||||||||||||||||
| f"staked_account_id={self.staked_account_id!r}" | ||||||||||||||||||
| f"staked_info={self.staking_info!r}, " | ||||||||||||||||||
| f")" | ||||||||||||||||||
| ) | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also please update the account_info unit tests to verify the changes. |
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| from hiero_sdk_python.tokens.token_relationship import TokenRelationship | ||
| from hiero_sdk_python.tokens.token_id import TokenId | ||
| from hiero_sdk_python.hapi.services.crypto_get_info_pb2 import CryptoGetInfoResponse | ||
| from hiero_sdk_python.staking_info import StakingInfo | ||
|
|
||
| pytestmark = pytest.mark.unit | ||
|
|
||
|
|
@@ -28,6 +29,8 @@ def account_info(): | |
| token_relationships=[], | ||
| account_memo="Test account memo", | ||
| owned_nfts=5, | ||
| max_automatic_token_associations=10, | ||
| staking_info=None | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -47,6 +50,8 @@ def proto_account_info(): | |
| tokenRelationships=[], | ||
| memo="Test account memo", | ||
| ownedNfts=5, | ||
| max_automatic_token_associations=10, | ||
| staking_info=None | ||
| ) | ||
| return proto | ||
|
|
||
|
|
@@ -65,6 +70,32 @@ def test_account_info_initialization(account_info): | |
| assert account_info.token_relationships == [] | ||
| 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 is None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can also add a test with staking_info present ( |
||
|
|
||
| def test_from_proto_with_staking_info(): | ||
| """Test the from_proto method of the AccountInfo class with staking info""" | ||
| public_key = PrivateKey.generate_ed25519().public_key() | ||
|
|
||
| staking_info={ | ||
| "decline_reward": True, | ||
| "staked_node_id": 3, | ||
| "staked_account_id": None | ||
| } | ||
|
|
||
| proto = CryptoGetInfoResponse.AccountInfo( | ||
| accountID=AccountId(0, 0, 100)._to_proto(), | ||
| key=public_key._to_proto(), | ||
| balance=5000000, | ||
|
|
||
|
|
||
| ) | ||
|
|
||
| account_info = AccountInfo._from_proto(proto) | ||
|
|
||
| assert account_info.staking_info is not None | ||
| assert account_info.staking_info.decline_reward is True | ||
| assert account_info.staking_info.staked_node_id == 3 | ||
|
Comment on lines
+76
to
+98
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test is broken: The Additionally, even if you intended to use it, passing a dict to a protobuf message field is fragile. Construct the proto message explicitly. 🔧 Proposed fix def test_from_proto_with_staking_info():
"""Test the from_proto method of the AccountInfo class with staking info"""
public_key = PrivateKey.generate_ed25519().public_key()
-
- staking_info={
- "decline_reward": True,
- "staked_node_id": 3,
- "staked_account_id": None
- }
-
+ from hiero_sdk_python.hapi.services.basic_types_pb2 import StakingInfo as StakingInfoProto
+
+ staking_info_proto = StakingInfoProto(
+ decline_reward=True,
+ staked_node_id=3
+ )
+
proto = CryptoGetInfoResponse.AccountInfo(
accountID=AccountId(0, 0, 100)._to_proto(),
key=public_key._to_proto(),
balance=5000000,
-
-
+ staking_info=staking_info_proto
)
account_info = AccountInfo._from_proto(proto)
-
+
assert account_info.staking_info is not None
assert account_info.staking_info.decline_reward is True
assert account_info.staking_info.staked_node_id == 3🧰 Tools🪛 Ruff (0.14.14)80-80: Local variable Remove assignment to unused variable (F841) |
||
|
|
||
|
|
||
| def test_account_info_default_initialization(): | ||
|
|
@@ -82,7 +113,25 @@ def test_account_info_default_initialization(): | |
| assert account_info.token_relationships == [] | ||
| assert account_info.account_memo is None | ||
| assert account_info.owned_nfts is None | ||
| assert account_info.max_automatic_token_associations is None | ||
| assert account_info.staking_info is None | ||
|
Comment on lines
+116
to
+117
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider adding a type validation test for Per coding guidelines for comprehensive coverage, consider adding a test that verifies passing an invalid type for |
||
|
|
||
| def test_staking_info_persistence(account_info): | ||
| """Ensure staking info is preserved through proto conversion""" | ||
|
|
||
| account_info.staking_info = StakingInfo( | ||
| decline_reward=True, | ||
| staked_node_id=5, | ||
| staked_account_id=None | ||
| ) | ||
|
|
||
| proto = account_info._to_proto() | ||
| converted_info = AccountInfo._from_proto(proto) | ||
|
|
||
| assert converted_info.staking_info is not None | ||
| assert converted_info.staking_info.decline_reward is True | ||
| assert converted_info.staking_info.staked_node_id == 5 | ||
| assert converted_info.staking_info.staked_account_id is None | ||
|
|
||
| def test_from_proto(proto_account_info): | ||
| """Test the from_proto method of the AccountInfo class""" | ||
|
|
@@ -100,6 +149,8 @@ def test_from_proto(proto_account_info): | |
| assert account_info.token_relationships == [] | ||
| 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 | ||
|
|
||
|
|
||
| def test_from_proto_with_token_relationships(): | ||
|
|
@@ -141,6 +192,11 @@ def test_to_proto(account_info): | |
| assert proto.tokenRelationships == [] | ||
| assert proto.memo == "Test account memo" | ||
| assert proto.ownedNfts == 5 | ||
| assert proto.max_automatic_token_associations == 10 | ||
| assert not proto.HasField("staking_info") | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| def test_to_proto_with_none_values(): | ||
|
|
@@ -192,6 +248,7 @@ def test_proto_conversion(account_info): | |
| ) | ||
| assert converted_account_info.account_memo == account_info.account_memo | ||
| assert converted_account_info.owned_nfts == account_info.owned_nfts | ||
| assert converted_account_info.staking_info == account_info.staking_info | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| def test_str_and_repr(account_info): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.