-
Notifications
You must be signed in to change notification settings - Fork 185
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 3 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 |
|---|---|---|
|
|
@@ -123,6 +123,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 | ||
|
||
| - 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) | ||
| - Update the acceptance criteria wording in the issue templates to improve clarity and consistency for contributors (#1491) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||||||||||||||
| ) | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| 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 |
|---|---|---|
|
|
@@ -28,6 +28,8 @@ def account_info(): | |
| token_relationships=[], | ||
| account_memo="Test account memo", | ||
| owned_nfts=5, | ||
| max_automatic_token_associations=10, | ||
| staking_info=None | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -47,6 +49,8 @@ def proto_account_info(): | |
| tokenRelationships=[], | ||
| memo="Test account memo", | ||
| ownedNfts=5, | ||
| max_automatic_token_associations=10 | ||
| staking_info=None | ||
| ) | ||
|
||
| return proto | ||
|
|
||
|
|
@@ -65,6 +69,8 @@ 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_account_info_default_initialization(): | ||
|
|
@@ -82,6 +88,8 @@ 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 == 10 | ||
| assert account_info.staking_info is None | ||
|
|
||
|
|
||
| def test_from_proto(proto_account_info): | ||
|
|
@@ -100,6 +108,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 +151,10 @@ 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 proto.staking_info == None | ||
|
|
||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
|
|
||
| def test_to_proto_with_none_values(): | ||
|
|
@@ -192,6 +206,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.