fix: precision loss in redeem_basket by applying fee before split (#326)

[aadviksinghdebug]

closes #326

Burning redeem_basket fee applied after-split causes precision loss.

Root cause: The redeem_basket function in acbu_burning/src/lib.rs had severe code corruption (duplicate imports, two overlapping function bodies, fragmented logic). Additionally, the fee allocation approach had a mixed implementation that could cause precision loss when splitting the gross amount first and then deducting fee per-slice.

Fix:

1. Fee-first-then-split approach: Fee is now applied to the total acbu_amount first (total_fee = calculate_fee(acbu_amount, fee_rate)), then the net remainder is split proportionally among recipients. This avoids per-slice rounding precision loss from splitting then applying fee per-leg.
2. Clean up code corruption: Removed all duplicate imports, duplicate function definitions, and fragmented code. The file is now clean, compilable, and maintains all existing functionality.
3. Added missing storage helpers: get_fee_rate, set_fee_rate, get_fee_single_redeem, set_fee_single_redeem, pause, unpause, is_paused.
4. Added missing ContractError variants: NoPendingAdmin, AdminTimelockNotElapsed, NoPendingAdminToCancel to shared.
5. Fixed corrupted test files: test.rs and fuzz_test.rs had duplicate code, undefined variables, and incorrect mock usage. Rewritten to use the common module properly.
6. Added edge divisibility tests: 4 new tests covering prime amounts, small amounts, extremely uneven weights, and exact fee divisibility.

Testing: All 25+ burning contract tests pass. Build is clean (0 warnings).

Summary by CodeRabbit

• New Features

  • Added pause/unpause functionality for contract control
  • Added public fee configuration methods: get and set fee rates for single and basket redemptions

• Bug Fixes

  • Improved fee calculation and rounding accuracy during redemptions
  • Enhanced reserve validation checks during redemption operations

• Tests

  • Expanded test coverage for edge-case scenarios in fee calculations and rounding behavior

[drips-wave]

@aadviksinghdebug Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors redeem_basket to apply the fee to the total gross amount before splitting across basket legs, using remainder-last allocation to prevent dust loss (fixing issue #326). Updates redeem_single and redeem_basket to use shared reentrancy guards and timestamped oracle constants. Adds public fee/pause read-write APIs, pins three ContractError discriminants, and expands test coverage with edge divisibility, fuzz, and pause tests.

Changes

BurningContract Redemption Refactor and API Additions

Layer / File(s)	Summary
Shared ContractError discriminants and oracle constants shared/src/lib.rs, acbu_burning/src/lib.rs	Pins NoPendingAdmin, AdminTimelockNotElapsed, and NoPendingAdminToCancel to explicit = 13/14/15 discriminants. Updates shared import block to use ORACLE_GET_ACBU_RATE_WITH_TS, ORACLE_GET_RATE_WITH_TS, and adds ADMIN_TIMELOCK_SECONDS.
redeem_single: shared reentrancy guard and timestamped oracle acbu_burning/src/lib.rs	Reentrancy guard acquisition/release moved to shared::reentrancy_guard. Oracle calls now use ORACLE_GET_ACBU_RATE_WITH_TS/ORACLE_GET_RATE_WITH_TS. Self::check_reserves called before burn. BurnEvent includes currency. check_paused simplified to a direct flag read-and-panic.
redeem_basket: fee-first split and weight-based per-leg allocation acbu_burning/src/lib.rs	Fee applied to gross acbu_amount before leg splitting. Per-currency weights and total_weight are precomputed; the last positive-weight bucket absorbs the allocation remainder. transfer_from is skipped when native_i == 0. BurnEvent emitted per leg with gross/net/fee and currency. Reserve-tracker pre-burn call replaced by Self::check_reserves.
Public fee/pause/version APIs and admin query cleanup acbu_burning/src/lib.rs	Adds get_fee_rate, get_fee_single_redeem, set_fee_rate, set_fee_single_redeem, pause, unpause, is_paused. Repositions version. Admin query methods reformatted; migration and validate_recipient comment blocks removed.
Fuzz test wiring and redeem_basket edge divisibility tests acbu_burning/tests/fuzz_test.rs, acbu_burning/tests/redeem_basket.rs	Fuzz test adds MockReserveTracker/MockToken stubs, timestamped oracle, vault-minted stoken, and switches to try_redeem_basket. Four new tests cover prime-amount rounding, small-amount dust bounds, uneven-weight rounding, and exact single-currency fee divisibility.
Integration tests migrated to ctx harness and warning fixes acbu_burning/tests/test.rs, acbu_burning/tests/redeem_single.rs, acbu_burning/tests/common/mod.rs	All test.rs tests (pause, paused-redeem, fee rates, empty/duplicate recipients, version) rewritten using the setup_test/ctx harness. Unused-variable and dead-code suppressions added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• Pi-Defi-world/acbu-smart-contract#300: Directly overlaps with this PR's redeem_basket fee-first/net-split, weighted-remainder allocation, and dust/rounding test additions.
• Pi-Defi-world/acbu-smart-contract#298: Both PRs update redeem_single/redeem_basket to use *_WITH_TS oracle symbol constants in the same oracle-invocation code paths.
• Pi-Defi-world/acbu-smart-contract#299: Directly touches acbu_burning/tests/fuzz_test.rs and exercises redeem_basket recipient/allocation edge cases with mock oracle logic, overlapping with the fuzz test changes here.

Poem

🐇 Hop, hop — the fee goes first,
Before the basket splits its thirst.
No dust remains for rounding's bite,
The last-weight bucket holds it tight.
Shared guards and timestamped oracles gleam—
A cleaner burn than it may seem! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: applying fee before split in redeem_basket to fix precision loss, directly matching the core objective and primary code modification.
Linked Issues check	✅ Passed	The PR successfully addresses issue #326 by implementing fee-first-then-split accounting, adding comprehensive edge divisibility tests for rounding behavior, and ensuring the approach is correct and well-tested.
Out of Scope Changes check	✅ Passed	All changes align with addressing the linked issue: fee-first-then-split refactoring, code cleanup from corruption, test improvements, and necessary storage/error variants for functionality.

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

✨ 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.}

[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)

acbu_burning/src/lib.rs (1)

223-234: 🔒 Security & Privacy | 🟠 Major

redeem_basket skips validate_recipient, allowing the contract address as a recipient.

redeem_single calls Self::validate_recipient(&env, &recipient) (line 114) to reject env.current_contract_address() as a recipient, but redeem_basket omits this check. Only empty and duplicate recipients are validated (lines 223-229). A caller can include the contract's own address as a basket recipient, causing S-tokens to be transferred to the contract itself.

Add the guard to each recipient in the validation loop:

Proposed fix

         let rlen = recipients.len();
         for i in 0..rlen {
+            Self::validate_recipient(&env, &recipients.get(i).unwrap());
             for j in (i + 1)..rlen {
                 if recipients.get(i).unwrap() == recipients.get(j).unwrap() {
                     env.panic_with_error(ContractError::InvalidRecipient);
                 }
             }
         }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@acbu_burning/src/lib.rs` around lines 223 - 234, The redeem_basket function
is missing a critical validation that exists in redeem_single. The validation
loop for recipients (lines 223-229) currently only checks for empty and
duplicate recipients but does not reject the contract address itself as a
recipient. Add a call to Self::validate_recipient(&env, &recipient) for each
recipient in the loop to ensure that env.current_contract_address() is rejected
as a recipient, preventing S-tokens from being transferred to the contract
itself.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@shared/src/lib.rs`:
- Around line 227-229: Update the ContractError table in docs/ERROR_CODES.md to
include the three new error code variants. Add entries for NoPendingAdmin (code
13), AdminTimelockNotElapsed (code 14), and NoPendingAdminToCancel (code 15) to
the shared — ContractError documentation section, which currently ends at code
12. Ensure each new variant entry includes a clear human-readable description
that explains when and why that error would be raised.

---

Outside diff comments:
In `@acbu_burning/src/lib.rs`:
- Around line 223-234: The redeem_basket function is missing a critical
validation that exists in redeem_single. The validation loop for recipients
(lines 223-229) currently only checks for empty and duplicate recipients but
does not reject the contract address itself as a recipient. Add a call to
Self::validate_recipient(&env, &recipient) for each recipient in the loop to
ensure that env.current_contract_address() is rejected as a recipient,
preventing S-tokens from being transferred to the contract itself.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1881796c-e05f-4764-85ff-7b8abb0069fe

📥 Commits

Reviewing files that changed from the base of the PR and between dd486ea and b2bbfbe.

📒 Files selected for processing (7)

• acbu_burning/src/lib.rs
• acbu_burning/tests/common/mod.rs
• acbu_burning/tests/fuzz_test.rs
• acbu_burning/tests/redeem_basket.rs
• acbu_burning/tests/redeem_single.rs
• acbu_burning/tests/test.rs
• shared/src/lib.rs

[Junman140]

@aadviksinghdebug clear conflicts