fix: update is_approved in _add_token_transfer accumulation branch

[mohityadav8]

Description:
Fix _add_token_transfer silently ignoring is_approved when merging
transfers for the same (token_id, account_id) pair.

• Add transfer.is_approved = is_approved in the accumulation branch of
  _add_token_transfer to implement last-call-wins semantics
• Update existing test_approved_token_transfer_accumulation to assert
  the corrected behaviour
• Add test_is_approved_updated_normal_then_approved to cover normal → approved order
• Add test_is_approved_updated_approved_then_normal to cover approved → normal order
• Add test_add_approved_token_transfer_no_decimals for the non-decimal variant

Related issue(s):

Fixes #2253

Notes for reviewer:
The fix is a single line added to the accumulation branch in
_add_token_transfer(), consistent with how expected_decimals is
already handled. Semantics are last-call-wins for is_approved.

Checklist

• Documented (Code comments, README, etc.)
• Tested (unit, integration, etc.)

[mohityadav8]

cc @manishdait @AntonioCeppellini

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Aggregation now requires both account_id and is_approved to match before merging a fungible TokenTransfer; transfers with differing is_approved values remain separate. Tests were updated to assert separate entries for approved vs normal transfers and to verify accumulation when is_approved matches.

Changes

Token Transfer Aggregation

Layer / File(s)	Summary
Aggregation condition update src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py	Require both account_id and is_approved to match an existing fungible TokenTransfer before summing amount and updating expected_decimals; otherwise append a new entry.
Updated unit tests for approval behavior tests/unit/transfer_transaction_test.py	Tests changed to expect approved and normal transfers for the same account to be stored as separate list entries, to verify accumulation between matching approved transfers, and to check expected_decimals handling for approved transfers.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and accurately summarizes the main change: updating is_approved in the accumulation branch of _add_token_transfer.
Description check	✅ Passed	The description provides clear context about the bug fix, implementation approach, test coverage, and references the related issue #2253.
Linked Issues check	✅ Passed	All code requirements from issue #2253 are met: single-line fix adding transfer.is_approved = is_approved in accumulation branch [#2253], updated test assertions, and new test cases covering the corrected behavior [#2253].
Out of Scope Changes check	✅ Passed	All changes are scoped to the bug fix: core logic change in _add_token_transfer and corresponding test updates, with no unrelated modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

📋 Issue Planner

Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).

View plan for ticket: #2253

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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/unit/transfer_transaction_test.py (1)

481-481: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Incorrect assertion: last-call-wins should update is_approved to True.

After adding an approved transfer at line 475 (add_approved_token_transfer_with_decimals), the last-call-wins semantics should update transfer_1.is_approved to True. The assertion and comment "# unchanged" are both incorrect—the fix explicitly updates is_approved during accumulation.

🐛 Proposed fix

-    assert transfer_1.is_approved is False  # unchanged
+    assert transfer_1.is_approved is True  # updated by last-call-wins

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c9143b75-35f9-45e7-9328-5af95ab8de95

📥 Commits

Reviewing files that changed from the base of the PR and between 52c15a4 and 667c305.

📒 Files selected for processing (2)

• src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py
• tests/unit/transfer_transaction_test.py

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py (1)

154-161: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Critical: Double accumulation bug — remove the old condition.

Lines 155-157 (the old condition checking only account_id) were not removed, causing amount to be accumulated twice when both account_id and is_approved match:

1. Line 156 adds amount when account_id matches
2. Line 159 adds amount again when both account_id and is_approved match

This contradicts the PR objective ("accumulate only when account_id and is_approved both match") and the past review guidance. The tests in test_same_approved_transfers_accumulate expect 500 but would receive 700 with this implementation.

🐛 Proposed fix: remove lines 155-157

 for transfer in self.token_transfers[token_id]:
-    if transfer.account_id == account_id:
-        transfer.amount += amount
-        transfer.expected_decimals = expected_decimals
     if transfer.account_id == account_id and transfer.is_approved == is_approved:
         transfer.amount += amount
         transfer.expected_decimals = expected_decimals
         return

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7163efcf-87c4-4ef8-a59d-c3cb6cb0a3b2

📥 Commits

Reviewing files that changed from the base of the PR and between 667c305 and 1db7dc9.

📒 Files selected for processing (2)

• src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py
• tests/unit/transfer_transaction_test.py

[mohityadav8]

cc @manishdait

[AntonioCeppellini]

Seems good to me, thank you for your contribution :D 🧇

[AntonioCeppellini]

please update the branch @mohityadav8 :D and then is ready to go

[mohityadav8]

@AntonioCeppellini done🙂

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@           Coverage Diff           @@
##             main    #2290   +/-   ##
=======================================
  Coverage   93.97%   93.97%           
=======================================
  Files         163      163           
  Lines       10408    10408           
=======================================
  Hits         9781     9781           
  Misses        627      627           

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Hi, this is WorkflowBot.
Your pull request cannot be merged as it is not passing all our workflow checks.
Please click on each check to review the logs and resolve issues so all checks pass.
To help you:

• DCO signing guide
• Merge conflicts guide
• Rebase guide
• Testing guide
• Discord
• Community Calls
  Thank you for contributing!
  From the Hiero Python SDK Team

[aceppaluni]

@mohityadav8 This is looking great!

Can you update your branch when you have a moment?

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py (1)

154-161: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Critical: Double accumulation bug when both conditions match.

Lines 155-157 should have been removed when lines 158-161 were added. Currently, when both account_id and is_approved match an existing transfer, the amount is accumulated twice:

1. Line 156 executes: transfer.amount += amount
2. Line 159 executes again: transfer.amount += amount

This causes the amount to be doubled incorrectly.

🐛 Proposed fix: Remove the old accumulation logic

 for transfer in self.token_transfers[token_id]:
-    if transfer.account_id == account_id:
-        transfer.amount += amount
-        transfer.expected_decimals = expected_decimals
     if transfer.account_id == account_id and transfer.is_approved == is_approved:
         transfer.amount += amount
         transfer.expected_decimals = expected_decimals
         return

As per coding guidelines, this is a correctness issue that breaks the fundamental accumulation logic for approved token transfers, which is critical for allowance-based transfer flows.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 26996619-0519-4d82-a3df-25a064b8077e

📥 Commits

Reviewing files that changed from the base of the PR and between 1d58990 and fc23cbc.

📒 Files selected for processing (2)

• src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py
• tests/unit/transfer_transaction_test.py

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 584e9fd2-5bc5-48b1-a7eb-cebae61b0470

📥 Commits

Reviewing files that changed from the base of the PR and between fc23cbc and e6bc36d.

📒 Files selected for processing (1)

• src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py

[coderabbitai]

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Document the last-call-wins semantics for expected_decimals within aggregation groups.

The aggregation logic now requires both account_id and is_approved to match, but expected_decimals is still overwritten (line 157) rather than validated for consistency. This means multiple calls with the same (token_id, account_id, is_approved) will accumulate amounts but the last call's expected_decimals wins.

Consider adding a docstring note or inline comment explaining this behavior, since callers might expect either:

• Validation that all calls use the same expected_decimals, or
• Aggregation only when all three fields match

📝 Suggested inline comment

         for transfer in self.token_transfers[token_id]:
             if transfer.account_id == account_id and transfer.is_approved == is_approved:
+                # Accumulate amount; overwrite expected_decimals with latest value (last-call-wins)
                 transfer.amount += amount
                 transfer.expected_decimals = expected_decimals
                 return

[manishdait]

Previously, these changes caused the e2e test tests/integration/transfer_transaction_e2e_test.py::test_integration_transfer_transaction_approved_token_transfer to fail with ACCOUNT_REPEATED_IN_ACCOUNT_AMOUNTS.

This happens cause fro single token within one TransferTransaction you cannot have both a standard transfer entry and an approved transfer entry for the same (tokenId, accountId) pair they are treated as separate entries and are not merged.

One workaround is to keep the previous behavior, where both transfers are merged into a single entry (either fully standard or fully approved). Since the approval flag is only set once, the later value is effectively ignored.

The other option is to update the e2e test to completely use the standard transfer instead.

Would like to get an opinion from @exploreriii on which direction makes more sense.

[aceppaluni]

Left question in DM

[mohityadav8]

@aceppaluni @manishdait what change i have to do in this ??

[github-actions]

Hello, this is the OfficeHourBot.

This is a reminder that the Hiero Python SDK Office Hours will begin in approximately 3 hours (14:00 UTC).

This session provides an opportunity to ask questions regarding this Pull Request.

Details:

• Time: 14:00 UTC
• Join Link: Zoom Meeting

Disclaimer: This is an automated reminder. Please verify the schedule here for any changes.

From,
The Python SDK Team