fix: only merge token transfers when approval status matches - #2655
fix: only merge token transfers when approval status matches#2655Yashraj-Jangra wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Fix mixed-order decimal serialization and rename the misleading unit test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates token transfer accumulation to distinguish approval status, preserve decimal metadata, and correct allowance-flow integration coverage.
Changes:
- Separates normal and approved transfers.
- Preserves decimal metadata when merging.
- Expands unit tests and corrects integration coverage.
File summaries
| File | Summary |
|---|---|
tests/unit/transfer_transaction_test.py |
Adds approval and decimal-preservation coverage; one test should be renamed for clarity. |
tests/integration/transfer_transaction_e2e_test.py |
Corrects the approved allowance transfer flow. |
src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py |
Updates merging logic; mixed-order decimal metadata still needs protobuf serialization coverage and correction. |
Review details
Suppressed comments (1)
tests/unit/transfer_transaction_test.py:458
test_approved_token_transfer_accumulationno longer tests accumulation; it now verifies separation, whiletest_same_approved_transfers_accumulatecovers accumulation below. Rename this test so its name matches the behavior it asserts and failures remain diagnosable.
"""Test that approved token transfers are stored as separate entries from normal ones."""
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if transfer.account_id == account_id and transfer.is_approved == is_approved: | ||
| transfer.amount += amount | ||
| transfer.expected_decimals = expected_decimals | ||
| if expected_decimals is not None: | ||
| transfer.expected_decimals = expected_decimals |
WalkthroughThe transfer merge logic now keeps regular and approved transfers separate, merges transfers only when approval status matches, and preserves ChangesToken transfer merge behavior
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Mixed regular and approved transfers can bypass requested token-decimal validation, while an incorrect allowance debit or credit would not be detected by the integration test. These should be addressed before merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/transfer_transaction_e2e_test.py (1)
385-385: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert resulting token balances.
The receipt assertion does not verify the changed allowance transfer result. Query both accounts after execution. Assert that
env.operator_idhas 500 units andaccount.idhas 500 units fortoken_id.Proposed test addition
+ operator_balance = CryptoGetAccountBalanceQuery(env.operator_id).execute(env.client) + account_balance = CryptoGetAccountBalanceQuery(account.id).execute(env.client) + + assert operator_balance.token_balances[token_id] == 500 + assert account_balance.token_balances[token_id] == 500As per path instructions, “Tests should assert observable network behavior, not just
SUCCESS.”Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: a0effce5-131d-4701-b968-f5b565494e1c
📒 Files selected for processing (3)
src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.pytests/integration/transfer_transaction_e2e_test.pytests/unit/transfer_transaction_test.py
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| if transfer.account_id == account_id and transfer.is_approved == is_approved: | ||
| transfer.amount += amount | ||
| transfer.expected_decimals = expected_decimals | ||
| if expected_decimals is not None: | ||
| transfer.expected_decimals = expected_decimals |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep expected_decimals token-scoped for approval-separated transfers.
Proto field: TokenTransferList.expected_decimals (field 4). Issue type: Asymmetric round-trip. The schema defines this value once per token transfer list, not once per account amount. (github.com)
If a regular transfer without decimals is added first and an approved transfer with decimals is added second, Line 164 prevents a merge. build_token_transfers() then serializes the first entry's None value and drops the caller's expected decimals. The transaction can skip the requested decimal validation.
src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py#L164-L167: propagate an explicit decimal value to the token's existing entries, and inherit the token decimal value for a new entry when the caller passesNone.tests/unit/transfer_transaction_test.py#L548-L583: build a transaction with a normal no-decimal transfer followed by an approved decimal transfer, then asserttokenTransfers[0].expected_decimals.value.
Proposed implementation direction
+ transfers = self.token_transfers[token_id]
+ if expected_decimals is not None:
+ for existing_transfer in transfers:
+ existing_transfer.expected_decimals = expected_decimals
+ elif transfers:
+ expected_decimals = transfers[0].expected_decimals
+
- for transfer in self.token_transfers[token_id]:
+ for transfer in transfers:
if transfer.account_id == account_id and transfer.is_approved == is_approved:
transfer.amount += amount
- if expected_decimals is not None:
- transfer.expected_decimals = expected_decimals
+ transfer.expected_decimals = expected_decimals
return
- self.token_transfers[token_id].append(
+ transfers.append(
TokenTransfer(token_id, account_id, amount, expected_decimals, is_approved)
)As per path instructions, protobuf fields must match their schema semantics and unit tests must cover edge cases.
📍 Affects 2 files
src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py#L164-L167(this comment)tests/unit/transfer_transaction_test.py#L548-L583
Source: Path instructions
|
@MonaaEid Please review this PR |
github-actions
Bot
added
open to community review
Signed-off-by: Yashraj Jangra <84060578+Yashraj-Jangra@users.noreply.github.com>
4448628 to
e4208bb
Compare
Description:
In
AbstractTokenTransferTransaction._add_token_transfer, token transfers for the same(token_id, account_id)pair were being merged without checkingis_approved. This caused approved transfers to get merged into standard transfers (or vice versa), silently clearing the approval flag depending on the order they were added.Since spending an allowance on behalf of an owner and transferring an account's own balance are two distinct actions, they shouldn't be combined into a single entry. This updates the accumulation logic so transfers only merge when both the account ID and approval status match.
_add_token_transferwhen bothaccount_idandis_approvedmatchexpected_decimalswhen merging if the incoming transfer passesexpected_decimals=Nonetest_approved_token_transfer_accumulationto assert that normal and approved transfers stay separatetest_integration_transfer_transaction_approved_token_transferto model the allowance flow correctly (approved debit from owner, credit to recipient) so consensus nodes don't reject repeated accounts in the same transfer listRelated issue(s):
Fixes #2253
Notes for reviewer:
All 55 tests in
transfer_transaction_test.pypass with full line coverage on the modified logic. Full unit test suite (3,025 tests) and pre-commit checks (ruff,bandit) pass clean.Checklist