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

[manishdait]

Moving this to draft until we decide which approach to go with.

[mohityadav8]

Moving this to draft until we decide which approach to go with.

ahh i thik thhis is good i was thinking same
atleast now can move to next pr

[github-actions]

Hi @mohityadav8,

This pull request has had no commit activity for 10 days. Are you still working on it?
To keep the PR active, you can:

• Push a new commit.
• Comment /working on the linked issue (not this PR).

If you're no longer working on this, please comment /unassign on the linked issue to release it for others. Otherwise, this PR may be closed due to inactivity.

Reach out on discord or join our office hours if you need assistance.

• Discord
• Office Hours

From the Python SDK Team

[github-actions]

Requesting triage review from: @hiero-ledger/hiero-sdk-python-triage

[aceppaluni]

@exploreriii What is your opinion on this?

[mohityadav8]

@exploreriii @aceppaluni

Since this PR is currently in discussion and marked as draft pending a decision on the expected behavior, I noticed the inactivity bot still posts reminders asking for new commits after 10 days.

Would it make sense to update the workflow so inactivity reminders are skipped for PRs that have labels such as "status: discussion" or for draft PRs? In cases like this, the PR is intentionally waiting for feedback rather than code changes.

Just a thought that might help reduce unnecessary notifications.

[exploreriii]

yes! i think we tried to do something like that in the past, unsure if it worked/we resolved it

[github-actions]

Hi @mohityadav8, this is CronInactivityBot 👋

This pull request has had no new commits for 21 days, so I'm closing it and unassigning you from the linked issue to keep the backlog healthy.

If you're no longer interested, no action is needed.

Tip: You can comment /unassign on any issue to proactively step away before this bot kicks in.

If you'd like to continue working on this later, feel free to comment /assign on the issue to get re-assigned, and open a new PR when you're ready. 🚀

[mohityadav8]

@AntonioCeppellini @manishdait why this is closed

[AntonioCeppellini]

@mohityadav8:

@exploreriii @aceppaluni @exploreriii i think we do not have news about this one

@mohityadav8 sorry for this, we are in a stale situation on this right now i think

[exploreriii]

Hi sorry it was the bot, we have a PR to fix it -- need to get back to you

[exploreriii]

Great work! Issue was relevant, fix is correct and nearly merge ready

[exploreriii]

should add a comment here eg

        Transfers for the same (token_id, account_id) pair are merged into a single
        entry only when their approval status matches; a regular and an approved
        transfer for the same account are kept as separate entries, since spending
        an allowance is semantically different from spending the account's own
        balance. When merging, an explicitly provided expected_decimals updates the
        entry; passing None leaves the previously set value unchanged.

[exploreriii]

We should avoid this being overwritten by none eg

                if expected_decimals is not None:
                    transfer.expected_decimals = expected_decimals

[exploreriii]

can add a test for the new features

def test_merge_preserves_expected_decimals(mock_account_ids):
"""Merging a transfer without decimals must not clear a previously set expected_decimals."""

[github-actions]

Hi @mohityadav8, this is CronInactivityBot 👋

This pull request has had no new commits for 38 days, so I'm closing it and unassigning you from the linked issue to keep the backlog healthy.

If you're no longer interested, no action is needed.

Tip: You can comment /unassign on any issue to proactively step away before this bot kicks in.

If you'd like to continue working on this later, feel free to comment /assign on the issue to get re-assigned, and open a new PR when you're ready. 🚀