From 21b43c3285ebf66b05bc8073fdd683190703356a Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Mon, 9 Mar 2026 17:18:27 +0800 Subject: [PATCH 1/5] minor fix for delay >= instead of > added some ignored test for a hold issue --- pallets/reversible-transfers/src/lib.rs | 4 +- .../src/tests/test_high_security_account.rs | 89 +++++++++++++++++++ .../src/tests/test_reversible_transfers.rs | 32 +++++++ 3 files changed, 123 insertions(+), 2 deletions(-) diff --git a/pallets/reversible-transfers/src/lib.rs b/pallets/reversible-transfers/src/lib.rs index 09f894a3..9964d8c6 100644 --- a/pallets/reversible-transfers/src/lib.rs +++ b/pallets/reversible-transfers/src/lib.rs @@ -552,10 +552,10 @@ pub mod pallet { fn validate_delay(delay: &BlockNumberOrTimestampOf) -> DispatchResult { match delay { BlockNumberOrTimestamp::BlockNumber(x) => { - ensure!(*x > T::MinDelayPeriodBlocks::get(), Error::::DelayTooShort) + ensure!(*x >= T::MinDelayPeriodBlocks::get(), Error::::DelayTooShort) }, BlockNumberOrTimestamp::Timestamp(t) => { - ensure!(*t > T::MinDelayPeriodMoment::get(), Error::::DelayTooShort) + ensure!(*t >= T::MinDelayPeriodMoment::get(), Error::::DelayTooShort) }, } Ok(()) diff --git a/pallets/reversible-transfers/src/tests/test_high_security_account.rs b/pallets/reversible-transfers/src/tests/test_high_security_account.rs index c9c8aa73..046ae38c 100644 --- a/pallets/reversible-transfers/src/tests/test_high_security_account.rs +++ b/pallets/reversible-transfers/src/tests/test_high_security_account.rs @@ -7,6 +7,12 @@ use crate::{ }; use frame_support::{assert_err, assert_ok}; use pallet_balances::TotalIssuance; +use crate::{ + HoldReason, +}; +use frame_support::{ + traits::fungible::InspectHold, +}; // NOTE: Many of the high security / reversibility behaviors are enforced via SignedExtension or // external pallets (Proxy). They are covered by integration tests in runtime. @@ -109,3 +115,86 @@ fn guardian_can_cancel_reversible_transactions_for_hs_account() { ); }); } + +// --- Known issues tracked for future fix (require AccountId → PendingTransfers index) --- + +#[test] +#[ignore = "recover_funds does not yet cancel pending transfers or release holds"] +fn recover_funds_includes_held_pending_amounts() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + let hs_user = alice(); + let guardian = bob(); + let dest = charlie(); + let amount = 500; + + let initial_hs_balance = Balances::free_balance(&hs_user); + let initial_guardian_balance = Balances::free_balance(&guardian); + + assert_ok!(ReversibleTransfers::schedule_transfer( + RuntimeOrigin::signed(hs_user.clone()), + dest.clone(), + amount, + )); + + assert_eq!( + Balances::balance_on_hold( + &RuntimeHoldReason::ReversibleTransfers(HoldReason::ScheduledTransfer), + &hs_user + ), + amount + ); + + assert_ok!(ReversibleTransfers::recover_funds( + RuntimeOrigin::signed(guardian.clone()), + hs_user.clone() + )); + + assert_eq!( + Balances::free_balance(&guardian), + initial_guardian_balance + initial_hs_balance, + "Guardian should receive ALL funds including held amounts" + ); + + assert_eq!( + Balances::balance_on_hold( + &RuntimeHoldReason::ReversibleTransfers(HoldReason::ScheduledTransfer), + &hs_user + ), + 0, + "No holds should remain after recovery" + ); + }); +} + +#[test] +#[ignore = "recover_funds does not yet cancel pending transfers or release holds"] +fn recover_funds_cancels_pending_transfers() { + new_test_ext().execute_with(|| { + System::set_block_number(1); + let hs_user = alice(); + let guardian = bob(); + let dest = charlie(); + let amount = 500; + + let call = transfer_call(dest.clone(), amount); + let tx_id = calculate_tx_id::(hs_user.clone(), &call); + + assert_ok!(ReversibleTransfers::schedule_transfer( + RuntimeOrigin::signed(hs_user.clone()), + dest.clone(), + amount, + )); + assert!(ReversibleTransfers::pending_dispatches(tx_id).is_some()); + + assert_ok!(ReversibleTransfers::recover_funds( + RuntimeOrigin::signed(guardian.clone()), + hs_user.clone() + )); + + assert!( + ReversibleTransfers::pending_dispatches(tx_id).is_none(), + "Pending transfers should be cancelled after recovery" + ); + }); +} diff --git a/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs b/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs index 41334de1..e17df9f7 100644 --- a/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs +++ b/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs @@ -1992,3 +1992,35 @@ fn cancelled_reversible_transfer_does_not_record_proof() { ); }); } + + +#[test] +fn validate_delay_accepts_delay_equal_to_minimum() { + new_test_ext().execute_with(|| { + let user = charlie(); + let interceptor = dave(); + let delay = BlockNumberOrTimestamp::BlockNumber(MinDelayPeriodBlocks::get()); + + assert_ok!(ReversibleTransfers::set_high_security( + RuntimeOrigin::signed(user), + delay, + interceptor, + )); + }); +} + +#[test] +fn validate_delay_accepts_timestamp_equal_to_minimum() { + new_test_ext().execute_with(|| { + let user = charlie(); + let interceptor = dave(); + let delay = BlockNumberOrTimestamp::Timestamp(MinDelayPeriodMoment::get()); + + assert_ok!(ReversibleTransfers::set_high_security( + RuntimeOrigin::signed(user), + delay, + interceptor, + )); + }); +} + From b0146496c7d5a48fdfff66892b8e063322cf97b9 Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Mon, 9 Mar 2026 17:24:48 +0800 Subject: [PATCH 2/5] correctly consume preimage, same as execute does --- pallets/reversible-transfers/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/reversible-transfers/src/lib.rs b/pallets/reversible-transfers/src/lib.rs index 9964d8c6..d9104b27 100644 --- a/pallets/reversible-transfers/src/lib.rs +++ b/pallets/reversible-transfers/src/lib.rs @@ -788,7 +788,7 @@ pub mod pallet { }; // For assets, burn held funds (fee) and transfer remaining to interceptor // For native balances, burn held funds (fee) and transfer remaining to interceptor - if let Ok((call, _)) = T::Preimages::peek::>(&pending.call) { + if let Ok((call, _)) = T::Preimages::realize::>(&pending.call) { if let Ok(pallet_assets::Call::transfer_keep_alive { id, .. }) = call.clone().try_into() { From 2905ff512db936374f52acab0137a20253db36ae Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Mon, 9 Mar 2026 17:25:43 +0800 Subject: [PATCH 3/5] format --- .../src/tests/test_high_security_account.rs | 10 ++-------- .../src/tests/test_reversible_transfers.rs | 2 -- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/pallets/reversible-transfers/src/tests/test_high_security_account.rs b/pallets/reversible-transfers/src/tests/test_high_security_account.rs index 046ae38c..8688f4af 100644 --- a/pallets/reversible-transfers/src/tests/test_high_security_account.rs +++ b/pallets/reversible-transfers/src/tests/test_high_security_account.rs @@ -3,16 +3,10 @@ use crate::{ mock::*, test_reversible_transfers::{calculate_tx_id, transfer_call}, }, - Event, + Event, HoldReason, }; -use frame_support::{assert_err, assert_ok}; +use frame_support::{assert_err, assert_ok, traits::fungible::InspectHold}; use pallet_balances::TotalIssuance; -use crate::{ - HoldReason, -}; -use frame_support::{ - traits::fungible::InspectHold, -}; // NOTE: Many of the high security / reversibility behaviors are enforced via SignedExtension or // external pallets (Proxy). They are covered by integration tests in runtime. diff --git a/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs b/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs index e17df9f7..291dbe76 100644 --- a/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs +++ b/pallets/reversible-transfers/src/tests/test_reversible_transfers.rs @@ -1993,7 +1993,6 @@ fn cancelled_reversible_transfer_does_not_record_proof() { }); } - #[test] fn validate_delay_accepts_delay_equal_to_minimum() { new_test_ext().execute_with(|| { @@ -2023,4 +2022,3 @@ fn validate_delay_accepts_timestamp_equal_to_minimum() { )); }); } - From d29ec1d093f2dc26cecc445f5561e1d6a11177b3 Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Mon, 9 Mar 2026 17:28:25 +0800 Subject: [PATCH 4/5] remove unused tests --- .../src/tests/test_high_security_account.rs | 83 ------------------- 1 file changed, 83 deletions(-) diff --git a/pallets/reversible-transfers/src/tests/test_high_security_account.rs b/pallets/reversible-transfers/src/tests/test_high_security_account.rs index 8688f4af..f7642345 100644 --- a/pallets/reversible-transfers/src/tests/test_high_security_account.rs +++ b/pallets/reversible-transfers/src/tests/test_high_security_account.rs @@ -109,86 +109,3 @@ fn guardian_can_cancel_reversible_transactions_for_hs_account() { ); }); } - -// --- Known issues tracked for future fix (require AccountId → PendingTransfers index) --- - -#[test] -#[ignore = "recover_funds does not yet cancel pending transfers or release holds"] -fn recover_funds_includes_held_pending_amounts() { - new_test_ext().execute_with(|| { - System::set_block_number(1); - let hs_user = alice(); - let guardian = bob(); - let dest = charlie(); - let amount = 500; - - let initial_hs_balance = Balances::free_balance(&hs_user); - let initial_guardian_balance = Balances::free_balance(&guardian); - - assert_ok!(ReversibleTransfers::schedule_transfer( - RuntimeOrigin::signed(hs_user.clone()), - dest.clone(), - amount, - )); - - assert_eq!( - Balances::balance_on_hold( - &RuntimeHoldReason::ReversibleTransfers(HoldReason::ScheduledTransfer), - &hs_user - ), - amount - ); - - assert_ok!(ReversibleTransfers::recover_funds( - RuntimeOrigin::signed(guardian.clone()), - hs_user.clone() - )); - - assert_eq!( - Balances::free_balance(&guardian), - initial_guardian_balance + initial_hs_balance, - "Guardian should receive ALL funds including held amounts" - ); - - assert_eq!( - Balances::balance_on_hold( - &RuntimeHoldReason::ReversibleTransfers(HoldReason::ScheduledTransfer), - &hs_user - ), - 0, - "No holds should remain after recovery" - ); - }); -} - -#[test] -#[ignore = "recover_funds does not yet cancel pending transfers or release holds"] -fn recover_funds_cancels_pending_transfers() { - new_test_ext().execute_with(|| { - System::set_block_number(1); - let hs_user = alice(); - let guardian = bob(); - let dest = charlie(); - let amount = 500; - - let call = transfer_call(dest.clone(), amount); - let tx_id = calculate_tx_id::(hs_user.clone(), &call); - - assert_ok!(ReversibleTransfers::schedule_transfer( - RuntimeOrigin::signed(hs_user.clone()), - dest.clone(), - amount, - )); - assert!(ReversibleTransfers::pending_dispatches(tx_id).is_some()); - - assert_ok!(ReversibleTransfers::recover_funds( - RuntimeOrigin::signed(guardian.clone()), - hs_user.clone() - )); - - assert!( - ReversibleTransfers::pending_dispatches(tx_id).is_none(), - "Pending transfers should be cancelled after recovery" - ); - }); -} From 223befeb1c4f51aad7fcec25576e451e4009dc85 Mon Sep 17 00:00:00 2001 From: Nikolaus Heger Date: Mon, 9 Mar 2026 17:32:53 +0800 Subject: [PATCH 5/5] remove unused imports --- .../src/tests/test_high_security_account.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pallets/reversible-transfers/src/tests/test_high_security_account.rs b/pallets/reversible-transfers/src/tests/test_high_security_account.rs index f7642345..c9c8aa73 100644 --- a/pallets/reversible-transfers/src/tests/test_high_security_account.rs +++ b/pallets/reversible-transfers/src/tests/test_high_security_account.rs @@ -3,9 +3,9 @@ use crate::{ mock::*, test_reversible_transfers::{calculate_tx_id, transfer_call}, }, - Event, HoldReason, + Event, }; -use frame_support::{assert_err, assert_ok, traits::fungible::InspectHold}; +use frame_support::{assert_err, assert_ok}; use pallet_balances::TotalIssuance; // NOTE: Many of the high security / reversibility behaviors are enforced via SignedExtension or