recover_funds should cancel pending first

[illuzen]

Summary

Add PendingTransfersBySender index to track pending transfers per account, enabling recover_funds to cancel all pending transfers in a single call.

Changes

Core Features

• PendingTransfersBySender storage: Maps sender accounts to their pending transfer IDs (bounded to MaxPendingPerAccount = 16)
• recover_funds enhancement: Now cancels all pending transfers before transferring remaining balance to guardian
• TooManyPendingTransactions error: Prevents scheduling more than 16 pending transfers per account

Refactoring

• Extracted shared do_cancel_transfer function used by both cancel_transfer and recover_funds
• release_held_funds_with_fee helper handles volume fee calculation and fund release
• Volume fee (1% burn) applies to each cancelled transfer during recovery

Test Additions

• recover_funds_cancels_all_pending_transfers - verifies all pending transfers are cancelled with fees applied
• too_many_pending_transactions_error - verifies the 16-transfer limit is enforced
• chained_guardian_high_security_account_flow - runtime integration test for guardian accounts that are also high-security

Bug Fixes

• Fixed MockHighSecurity::is_whitelisted in multisig tests to use exact match instead of contains (was matching "unsafe" as whitelisted because it contains "safe")

Behavior

• recover_funds uses worst-case weight (always charges for 16 cancellations)
• Scheduler errors are ignored during recovery (strict_scheduler=false) to ensure recovery completes even if some scheduled tasks are already executed
• TransactionCancelled event emitted for each cancelled pending transfer

[n13]

Here's my review of PR #408:

Overall

The approach is sound -- PendingTransfersBySender bounded index is exactly what we discussed as Option A. The refactoring into do_cancel_transfer and release_held_funds_with_fee is clean. A few issues though:

Issues

1. Weight not updated (real problem)

recover_funds still uses the old benchmark weight:

#[pallet::weight(<T as Config>::WeightInfo::recover_funds())]

The old benchmark only covers a single transfer_all. The new logic does up to 16 cancellations, each involving storage reads/writes, scheduler cancellation, preimage realize, burn_held, and transfer_on_hold. The actual weight is many times higher than what's charged. Underweight extrinsics are a DoS vector -- an attacker can fill blocks cheaply.

The benchmark needs to be re-run, or at minimum the weight function needs a parameter for the number of pending transfers.

2. Recovery fails entirely if any single cancellation errors

for tx_id in pending_tx_ids.iter() {
    if let Some(pending) = PendingTransfers::<T>::get(tx_id) {
        Self::do_cancel_transfer(&who, *tx_id, &pending, &who, true, false)?;
    }
}

The ? means if release_held_funds_with_fee fails for any transfer (e.g., a hold is somehow inconsistent), the entire recover_funds rolls back and recovery is impossible. The strict_scheduler=false flag shows awareness of this for scheduler errors, but the same leniency isn't applied to fund-release errors. Consider wrapping the inner call in a let _ = or logging and continuing, since this is an emergency path.

3. Redundant PendingTransfersBySender writes during recovery

let pending_tx_ids = PendingTransfersBySender::<T>::take(&account);  // removes from storage
for tx_id in pending_tx_ids.iter() {
    // ...
    Self::do_cancel_transfer(...)?;  // internally does PendingTransfersBySender::mutate(retain)
}

take already emptied the map. Then do_cancel_transfer calls PendingTransfersBySender::mutate on a now-empty (default) list for each iteration -- each one writes an empty vec back to storage. Not a bug, just 16 unnecessary storage writes. Could be avoided by either skipping the retain in do_cancel_transfer when called from recovery, or by not using take and letting do_cancel_transfer handle removal naturally.

4. Volume fee during emergency recovery (design question)

The PR applies 1% volume fee to each cancelled transfer during recovery. In an emergency where the account is compromised, this burns funds unnecessarily. If this is intentional, fine -- but worth a conscious decision.

What looks good

• Bounded MaxPendingPerAccount = 16 keeps weight deterministic
• strict_scheduler=false for recovery is smart -- tasks may already have fired
• if let Some(pending) defensively skips orphaned tx_ids
• Removed let _ = on asset burn_held/transfer_on_hold (errors now propagate correctly)
• Test coverage for the happy path, limit enforcement, and fee calculations

Not addressed (out of scope, just noting)

• Free asset balances (non-held) are still not recovered -- transfer_all only handles native balance
• The validate_delay fix (we already have that on our branch)

[n13]

Hi there! 🌟 Thanks for working on this PR!

I've reviewed the code and noticed a couple of things we can improve to align perfectly with our project rules:

1. DRY Principle: It looks like the cancellation logic in duplicates some of the logic in (like removing from storage, cancelling the scheduler, releasing funds, and depositing the event). We should abstract this shared logic into a single helper function to avoid any duplicate code! 🚀
2. Comments: There are quite a few inline comments added (e.g., , , ). Let's keep the code as concise as possible by removing these extra comments. The code is already very readable! ✨

Otherwise, the approach with looks great! Let me know if you'd like me to help make these adjustments. Happy coding! 😊

[n13]

Hi there! 🌟 Thanks for working on this PR!

I've reviewed the code and noticed a couple of things we can improve to align perfectly with our project rules:

1. DRY Principle: It looks like the cancellation logic in recover_funds duplicates some of the logic in cancel_transfer (like removing from storage, cancelling the scheduler, releasing funds, and depositing the event). We should abstract this shared logic into a single helper function to avoid any duplicate code! 🚀
2. Comments: There are quite a few inline comments added (e.g., // Cancel all pending transfers..., // Ignore scheduler errors..., // Log and continue...). Let's keep the code as concise as possible by removing these extra comments. The code is already very readable! ✨

Otherwise, the approach with PendingTransfersBySender looks great! Let me know if you'd like me to help make these adjustments. Happy coding! 😊

[n13]

Here's my review of PR #408: recover_funds should cancel pending first.

---

Overview

Clean, well-structured PR. The PendingTransfersBySender index, the release_held_funds_with_fee extraction, and the recovery loop are all solid. Tests are thorough. A few issues worth addressing:

---

Issues

1. Two independent if let branches in release_held_funds_with_fee - could double-execute

			if let Ok((call, _)) = T::Preimages::realize::<RuntimeCallOf<T>>(&pending.call) {
				if let Ok(pallet_assets::Call::transfer_keep_alive { id, .. }) =
					call.clone().try_into()
				{
					// ... handles asset path
				}
				if let Ok(pallet_balances::Call::transfer_keep_alive { .. }) =
					call.clone().try_into()
				{
					// ... handles balance path
				}
			}

The second if should be else if. These are mutually exclusive call types, but as written, if a call could somehow satisfy both try_into() conversions, both branches would execute - burning and transferring twice. Defensively, the second should be else if.

2. recover_funds charges worst-case weight but never refunds

		#[pallet::weight(
			<T as Config>::WeightInfo::recover_funds()
				.saturating_add(<T as Config>::WeightInfo::cancel().saturating_mul(T::MaxPendingPerAccount::get().into()))
		)]

An account with 0 pending transfers pays the same as one with 16. Since the function already returns DispatchResultWithPostInfo, you could track the actual number of cancellations and return consumed weight for a refund:

let num_cancelled: u64 = /* count from loop */;
let actual_weight = <T as Config>::WeightInfo::recover_funds()
    .saturating_add(
        <T as Config>::WeightInfo::cancel()
            .saturating_mul(num_cancelled)
    );
Ok(Some(actual_weight).into())

Not critical for an emergency function, but it's free weight back for the guardian.

3. recover_funds benchmark doesn't include pending transfers

	fn recover_funds() -> Result<(), BenchmarkError> {
		let account: T::AccountId = whitelisted_caller();
		let guardian: T::AccountId = benchmark_account("guardian", 0, SEED);
		fund_account::<T>(&account, BalanceOf::<T>::from(10000u128));
		fund_account::<T>(&guardian, BalanceOf::<T>::from(10000u128));
		let delay = T::DefaultDelay::get();
		setup_high_security_account::<T>(account.clone(), delay, guardian.clone());
		#[extrinsic_call]
		_(RawOrigin::Signed(guardian.clone()), account.clone());
		Ok(())
	}

The benchmark sets up a HS account with zero pending transfers, so it only benchmarks the transfer_all path. The composite weight annotation adds cancel * 16 to compensate, but that's an approximation - the inline recovery loop (uses take, swallows scheduler errors, calls release_held_funds_with_fee directly) has different overhead than the full cancel extrinsic. Ideally the benchmark would schedule MaxPendingPerAccount transfers first to measure the actual worst case.

4. Stale data if scheduling fails mid-way (pre-existing, minor)

			PendingTransfers::<T>::insert(tx_id, new_pending);

			// Add to sender's pending list
			PendingTransfersBySender::<T>::try_mutate(&from, |list| {
				list.try_push(tx_id).map_err(|_| Error::<T>::TooManyPendingTransactions)
			})?;

			let bounded_call = T::Preimages::bound(Call::<T>::execute_transfer { tx_id }.into())?;

			// Schedule the `do_execute` call
			T::Scheduler::schedule_named(
				// ...
			)
			.map_err(|e| {
				// ...
				Error::<T>::SchedulingFailed
			})?;

			// hold funds...

If Preimages::bound, schedule_named, or hold fails, PendingTransfers and PendingTransfersBySender already have entries. This is safe because FRAME extrinsics are transactional by default, so all storage mutations roll back on error. Just noting for awareness - if this inner function is ever called from a non-transactional context, it would need guarding.

---

Positives

• DRY refactoring: release_held_funds_with_fee nicely deduplicates the fee calculation + fund release logic between cancel_transfer and recover_funds.
• take() in the recovery loop is clean - atomically reads and clears the sender index in one operation.
• Error handling improvement: burn_held and transfer_on_hold results are now properly propagated with ? instead of silently discarded with let _ =.
• Test coverage is solid - covers the happy path (multiple cancellations + fee math + events), the 16-transfer limit, and balance/issuance accounting.
• Bug fix in the multisig mock (is_whitelisted exact match vs contains) is a nice catch.

---

Verdict

Issues 1-3 are worth addressing before merge. #1 is a correctness/safety concern (use else if), #2 is a nice-to-have UX improvement, and #3 would make the weight accounting more accurate. Otherwise this is a solid PR.

[n13]

1 - 3 addressed

[n13]

Fixes #406

[n13]

GTG