test: proto pr test#209
Conversation
Signed-off-by: exploreriii <133720349+exploreriii@users.noreply.github.com>
WalkthroughConsolidates staking-related fields in AccountInfo into a single StakingInfo object, adds support for max_automatic_token_associations, updates protobuf serialization/deserialization logic, and adjusts test expectations for field persistence and date formatting in string representations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 (2)
src/hiero_sdk_python/account/account_info.py (1)
41-44:⚠️ Potential issue | 🟡 MinorDocstring references removed fields.
The docstring still documents
staked_account_id,staked_node_id, anddecline_staking_rewardas separate attributes, but these have been consolidated intostaking_info. Update the docstring to reflect the new structure.📝 Proposed fix
- staked_account_id (Optional[AccountId]): The account to which this account is staked. - staked_node_id (Optional[int]): The node to which this account is staked. - decline_staking_reward (bool): Whether this account declines receiving staking rewards. + staking_info (Optional[StakingInfo]): Staking information for this account, including + staked account/node ID and decline reward preference.tests/unit/account_info_test.py (1)
59-74: 🧹 Nitpick | 🔵 TrivialAdd
hasattrassertions to protect against breaking changes.Per coding guidelines, tests should assert public attributes exist to catch breaking changes early.
💡 Suggested addition
def test_account_info_initialization(account_info): """Test the initialization of the AccountInfo class""" + # Verify public attributes exist (protect against breaking changes) + assert hasattr(account_info, 'account_id') + assert hasattr(account_info, 'staking_info') + assert hasattr(account_info, 'max_automatic_token_associations') + assert account_info.account_id == AccountId(0, 0, 100)As per coding guidelines: "Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id'))."
| staking_info=( | ||
| self.staking_info._to_proto() | ||
| if self.staking_info is not None | ||
| else None | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent indentation in _to_proto.
Line 147 uses 6-space indentation which breaks consistency with the rest of the method.
🔧 Proposed fix
staking_info=(
self.staking_info._to_proto()
if self.staking_info is not None
else None
- ),
+ ),
)📝 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.
| staking_info=( | |
| self.staking_info._to_proto() | |
| if self.staking_info is not None | |
| else None | |
| ), | |
| ) | |
| staking_info=( | |
| self.staking_info._to_proto() | |
| if self.staking_info is not None | |
| else None | |
| ), | |
| ) |
| f"staked_info={self.staking_info!r}, " | ||
| f")" |
There was a problem hiding this comment.
Inconsistent field name in __repr__: staked_info should be staking_info.
The field is named staking_info (line 59), but __repr__ uses staked_info. This inconsistency could confuse developers during debugging.
🔧 Proposed fix
- f"staked_info={self.staking_info!r}, "
+ f"staking_info={self.staking_info!r}, "📝 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.
| f"staked_info={self.staking_info!r}, " | |
| f")" | |
| f"staking_info={self.staking_info!r}, " | |
| f")" |
| 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 |
There was a problem hiding this comment.
Test is broken: staking_info is never set on the proto.
The test creates a staking_info dict (lines 80-84) but never uses it. The proto (lines 86-92) doesn't have staking_info set, so AccountInfo._from_proto(proto) will return staking_info=None. The assertions on lines 96-98 will fail because they expect staking_info to be populated.
Static analysis also flagged the unused variable on line 80.
🐛 Proposed fix - set staking_info on the proto
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
- }
+ # Create StakingInfo and convert to proto
+ staking_info = StakingInfo(
+ 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,
-
-
+ staking_info=staking_info._to_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📝 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_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 | |
| 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() | |
| # Create StakingInfo and convert to proto | |
| staking_info = StakingInfo( | |
| 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, | |
| staking_info=staking_info._to_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)
[error] 80-80: Local variable staking_info is assigned to but never used
Remove assignment to unused variable staking_info
(F841)
| 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 |
There was a problem hiding this comment.
Use is None instead of == None for None comparison.
Per PEP 8, comparisons to singletons like None should use is or is not, not equality operators.
🔧 Proposed fix
- assert account_info.staking_info == None
+ assert account_info.staking_info is None📝 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.
| assert account_info.staking_info == None | |
| assert account_info.staking_info is None |
🧰 Tools
🪛 Ruff (0.14.14)
[error] 153-153: Comparison to None should be cond is None
Replace with cond is None
(E711)
1 participant
Description:
What will code rabbit do?
Fixes #1