feat: explicit reverser for reversible accounts (#8)

* Add explicit reverser for reversible transfers

* Make reverser get reserved funds, update benchmarks

* Add more tests

Author: dastansam

pallets/reversible-transfers/src/benchmarking.rs

@@ -40,8 +40,16 @@ fn setup_reversible_account<T: Config>(
     who: T::AccountId,
     delay: BlockNumberFor<T>,
     policy: DelayPolicy,
+    reverser: Option<T::AccountId>,
 ) {
-    ReversibleAccounts::<T>::insert(who, (delay, policy));
+    ReversibleAccounts::<T>::insert(
+        who,
+        ReversibleAccountData {
+            delay,
+            policy,
+            explicit_reverser: reverser,
+        },
+    );
 }
 
 // Helper to fund an account (requires Balances pallet in mock runtime)
@@ -77,6 +85,7 @@ mod benchmarks {
     #[benchmark]
     fn set_reversibility() -> Result<(), BenchmarkError> {
         let caller: T::AccountId = whitelisted_caller();
+        let explicit_reverser: T::AccountId = benchmark_account("explicit_reverser", 0, SEED);
         let delay: BlockNumberFor<T> = T::DefaultDelay::get();
         let policy = DelayPolicy::Explicit;
 
@@ -85,9 +94,17 @@ mod benchmarks {
             RawOrigin::Signed(caller.clone()),
             Some(delay),
             policy.clone(),
+            Some(explicit_reverser.clone()),
         );
 
-        assert_eq!(ReversibleAccounts::<T>::get(&caller), Some((delay, policy)));
+        assert_eq!(
+            ReversibleAccounts::<T>::get(&caller),
+            Some(ReversibleAccountData {
+                delay,
+                policy,
+                explicit_reverser: Some(explicit_reverser),
+            })
+        );
 
         Ok(())
     }
@@ -102,7 +119,7 @@ mod benchmarks {
 
         // Setup caller as reversible
         let delay = T::DefaultDelay::get();
-        setup_reversible_account::<T>(caller.clone(), delay, DelayPolicy::Explicit);
+        setup_reversible_account::<T>(caller.clone(), delay, DelayPolicy::Explicit, None);
 
         let call = make_transfer_call::<T>(recipient.clone(), transfer_amount)?;
         let tx_id = T::Hashing::hash_of(&(caller.clone(), call).encode());
@@ -129,13 +146,22 @@ mod benchmarks {
     #[benchmark]
     fn cancel() -> Result<(), BenchmarkError> {
         let caller: T::AccountId = whitelisted_caller();
+        let reverser: T::AccountId = benchmark_account("reverser", 1, SEED);
+
         fund_account::<T>(&caller, BalanceOf::<T>::from(1000u128));
+        fund_account::<T>(&reverser, BalanceOf::<T>::from(1000u128));
         let recipient: T::AccountId = benchmark_account("recipient", 0, SEED);
         let transfer_amount = 100u128;
 
         // Setup caller as reversible and schedule a task in setup
         let delay = T::DefaultDelay::get();
-        setup_reversible_account::<T>(caller.clone(), delay, DelayPolicy::Explicit);
+        // Worst case scenario: reverser is explicit
+        setup_reversible_account::<T>(
+            caller.clone(),
+            delay,
+            DelayPolicy::Explicit,
+            Some(reverser.clone()),
+        );
         let call = make_transfer_call::<T>(recipient.clone(), transfer_amount)?;
 
         // Use internal function directly in setup - requires RuntimeOrigin from Config
@@ -152,7 +178,7 @@ mod benchmarks {
 
         // Benchmark the cancel extrinsic
         #[extrinsic_call]
-        _(RawOrigin::Signed(caller.clone()), tx_id);
+        _(RawOrigin::Signed(reverser), tx_id);
 
         assert_eq!(AccountPendingIndex::<T>::get(&caller), 0);
         assert!(!PendingTransfers::<T>::contains_key(&tx_id));
@@ -173,7 +199,7 @@ mod benchmarks {
 
         // Setup owner as reversible and schedule a task in setup
         let delay = T::DefaultDelay::get();
-        setup_reversible_account::<T>(owner.clone(), delay, DelayPolicy::Explicit);
+        setup_reversible_account::<T>(owner.clone(), delay, DelayPolicy::Explicit, None);
         let call = make_transfer_call::<T>(recipient.clone(), transfer_amount)?;
 
         let owner_origin = RawOrigin::Signed(owner.clone()).into();

pallets/reversible-transfers/src/lib.rs

@@ -22,20 +22,21 @@ pub mod weights;
 pub use weights::WeightInfo;
 
 use alloc::vec::Vec;
+use frame_support::traits::tokens::{Fortitude, Restriction};
 use frame_support::{pallet_prelude::*, traits::schedule::DispatchTime};
 use frame_system::pallet_prelude::*;
 use sp_runtime::traits::StaticLookup;
 
 /// How to delay transactions
-/// - `Explicit`: Only delay transactions explicitly using `schedule_dispatch`.
+/// - `Explicit`: Only delay transactions explicitly using `schedule_transfer`.
 /// - `Intercept`: Intercept and delay transactions at the `TransactionExtension` level.
 ///
 /// For example, for a reversible account with `DelayPolicy::Intercept`, the transaction
-/// will be delayed even if the user doesn't explicitly call `schedule_dispatch`. And for `DelayPolicy::Explicit`,
-/// the transaction will be delayed only if the user explicitly calls this pallet's `schedule_dispatch` extrinsic.
+/// will be delayed even if the user doesn't explicitly call `schedule_transfer`. And for `DelayPolicy::Explicit`,
+/// the transaction will be delayed only if the user explicitly calls this pallet's `schedule_transfer` extrinsic.
 #[derive(Encode, Decode, MaxEncodedLen, Clone, Default, TypeInfo, Debug, PartialEq, Eq)]
 pub enum DelayPolicy {
-    /// Only explicitly delay transactions using `schedule_dispatch` call
+    /// Only explicitly delay transactions using `schedule_transfer` call
     #[default]
     Explicit,
     /// Intercept and delay transactions at `TransactionExtension` level. This is not UX friendly
@@ -46,6 +47,17 @@ pub enum DelayPolicy {
     Intercept,
 }
 
+/// Reversible account details
+#[derive(Encode, Decode, MaxEncodedLen, Clone, Default, TypeInfo, Debug, PartialEq, Eq)]
+pub struct ReversibleAccountData<AccountId, BlockNumber> {
+    /// The account that can reverse the transaction. `None` means the account itself.
+    pub explicit_reverser: Option<AccountId>,
+    /// The delay period for the account
+    pub delay: BlockNumber,
+    /// The policy for the account
+    pub policy: DelayPolicy,
+}
+
 /// Pending transfer details
 #[derive(Encode, Decode, MaxEncodedLen, Clone, Default, TypeInfo, Debug, PartialEq, Eq)]
 pub struct PendingTransfer<AccountId, Balance, Call> {
@@ -142,7 +154,7 @@ pub mod pallet {
         _,
         Blake2_128Concat,
         T::AccountId,
-        (BlockNumberFor<T>, DelayPolicy),
+        ReversibleAccountData<T::AccountId, BlockNumberFor<T>>,
         OptionQuery,
     >;
 
@@ -172,8 +184,7 @@ pub mod pallet {
         /// [who, maybe_delay: None means disabled]
         ReversibilitySet {
             who: T::AccountId,
-            delay: BlockNumberFor<T>,
-            policy: DelayPolicy,
+            data: ReversibleAccountData<T::AccountId, BlockNumberFor<T>>,
         },
         /// A transaction has been intercepted and scheduled for delayed execution.
         /// [who, tx_id, execute_at_moment]
@@ -199,6 +210,8 @@ pub mod pallet {
         AccountAlreadyReversible,
         /// The account attempting the action is not marked as reversible.
         AccountNotReversible,
+        /// Reverser can not be the account itself, because it is redundant.
+        ExplicitReverserCanNotBeSelf,
         /// The specified pending transaction ID was not found.
         PendingTxNotFound,
         /// The caller is not the original submitter of the transaction they are trying to cancel.
@@ -217,6 +230,8 @@ pub mod pallet {
         InvalidCall,
         /// Invalid scheduler origin
         InvalidSchedulerOrigin,
+        /// Reverser is invalid
+        InvalidReverser,
     }
 
     #[pallet::call]
@@ -235,19 +250,34 @@ pub mod pallet {
             origin: OriginFor<T>,
             delay: Option<BlockNumberFor<T>>,
             policy: DelayPolicy,
+            reverser: Option<T::AccountId>,
         ) -> DispatchResult {
             let who = ensure_signed(origin)?;
 
             ensure!(
                 !ReversibleAccounts::<T>::contains_key(&who),
                 Error::<T>::AccountAlreadyReversible
             );
+            ensure!(
+                reverser != Some(who.clone()),
+                Error::<T>::ExplicitReverserCanNotBeSelf
+            );
+
             let delay = delay.unwrap_or(T::DefaultDelay::get());
 
             ensure!(delay >= T::MinDelayPeriod::get(), Error::<T>::DelayTooShort);
 
-            ReversibleAccounts::<T>::insert(&who, (delay, policy.clone()));
-            Self::deposit_event(Event::ReversibilitySet { who, delay, policy });
+            let reversible_account_data = ReversibleAccountData {
+                explicit_reverser: reverser,
+                delay,
+                policy: policy.clone(),
+            };
+
+            ReversibleAccounts::<T>::insert(&who, reversible_account_data.clone());
+            Self::deposit_event(Event::ReversibilitySet {
+                who,
+                data: reversible_account_data,
+            });
 
             Ok(())
         }
@@ -320,7 +350,9 @@ pub mod pallet {
         T: pallet_balances::Config<RuntimeHoldReason = <T as Config>::RuntimeHoldReason>,
     {
         /// Check if an account has reversibility enabled and return its delay.
-        pub fn is_reversible(who: &T::AccountId) -> Option<(BlockNumberFor<T>, DelayPolicy)> {
+        pub fn is_reversible(
+            who: &T::AccountId,
+        ) -> Option<ReversibleAccountData<T::AccountId, BlockNumberFor<T>>> {
             ReversibleAccounts::<T>::get(who)
         }
 
@@ -398,8 +430,18 @@ pub mod pallet {
             amount: BalanceOf<T>,
         ) -> DispatchResult {
             let who = ensure_signed(origin)?;
-            let (delay, _) =
-                Self::reversible_accounts(&who).ok_or(Error::<T>::AccountNotReversible)?;
+            let ReversibleAccountData {
+                delay,
+                explicit_reverser,
+                policy: _,
+            } = Self::reversible_accounts(&who).ok_or(Error::<T>::AccountNotReversible)?;
+
+            match explicit_reverser {
+                Some(reverser) => {
+                    ensure!(who != reverser, Error::<T>::InvalidReverser);
+                }
+                None => {}
+            };
 
             let transfer_call: T::RuntimeCall = pallet_balances::Call::<T>::transfer_keep_alive {
                 dest: dest.clone(),
@@ -480,23 +522,33 @@ pub mod pallet {
         /// Cancels a previously scheduled transaction. Internal logic used by `cancel` extrinsic.
         fn cancel_transfer(who: &T::AccountId, tx_id: T::Hash) -> DispatchResult {
             // Retrieve owner from storage to verify ownership
-            let pending =
-                PendingTransfers::<T>::get(tx_id).ok_or(Error::<T>::PendingTxNotFound)?;
+            let pending = PendingTransfers::<T>::get(tx_id).ok_or(Error::<T>::PendingTxNotFound)?;
 
-            ensure!(&pending.who == who, Error::<T>::NotOwner);
+            let reversible_account_data = ReversibleAccounts::<T>::get(&pending.who)
+                .ok_or(Error::<T>::AccountNotReversible)?;
 
-            // Remove from main storage
-            PendingTransfers::<T>::mutate(tx_id, |pending_opt| {
-                if let Some(pending) = pending_opt {
-                    if pending.count > 1 {
-                        // If there are more than one identical transactions, decrement the count
-                        pending.count = pending.count.saturating_sub(1);
-                    } else {
-                        // Otherwise, remove the transaction from storage
-                        *pending_opt = None;
-                    }
-                }
-            });
+            if let Some(explicit_reverser) = &reversible_account_data.explicit_reverser {
+                // If the reverser is set, ensure the caller is the reverser
+                ensure!(who == explicit_reverser, Error::<T>::InvalidReverser);
+            } else {
+                ensure!(&pending.who == who, Error::<T>::NotOwner);
+            }
+
+            if pending.count > 1 {
+                // If there are more than one identical transactions, decrement the count
+                PendingTransfers::<T>::insert(
+                    &tx_id,
+                    PendingTransfer {
+                        who: pending.who.clone(),
+                        call: pending.call,
+                        amount: pending.amount,
+                        count: pending.count.saturating_sub(1),
+                    },
+                );
+            } else {
+                // Otherwise, remove the transaction from storage
+                PendingTransfers::<T>::remove(&tx_id);
+            }
 
             // Decrement account index
             AccountPendingIndex::<T>::mutate(&pending.who, |current_count| {
@@ -509,14 +561,25 @@ pub mod pallet {
             // Cancel the scheduled task
             T::Scheduler::cancel_named(schedule_id).map_err(|_| Error::<T>::CancellationFailed)?;
 
-            // Release the funds
-            pallet_balances::Pallet::<T>::release(
-                &HoldReason::ScheduledTransfer.into(),
-                &pending.who,
-                pending.amount,
-                Precision::Exact,
-            )?;
-
+            if let Some(reverser) = &reversible_account_data.explicit_reverser {
+                pallet_balances::Pallet::<T>::transfer_on_hold(
+                    &HoldReason::ScheduledTransfer.into(),
+                    &pending.who,
+                    reverser,
+                    pending.amount,
+                    Precision::Exact,
+                    Restriction::Free,
+                    Fortitude::Polite,
+                )?;
+            } else {
+                // Release the funds
+                pallet_balances::Pallet::<T>::release(
+                    &HoldReason::ScheduledTransfer.into(),
+                    &pending.who,
+                    pending.amount,
+                    Precision::Exact,
+                )?;
+            }
             Self::deposit_event(Event::TransactionCancelled {
                 who: who.clone(),
                 tx_id,
@@ -538,7 +601,14 @@ pub mod pallet {
             for (who, delay) in &self.initial_reversible_accounts {
                 // Basic validation, ensure delay is reasonable if needed
                 if *delay >= T::MinDelayPeriod::get() {
-                    ReversibleAccounts::<T>::insert(who, (delay, DelayPolicy::Explicit));
+                    ReversibleAccounts::<T>::insert(
+                        who,
+                        ReversibleAccountData {
+                            explicit_reverser: None,
+                            delay: *delay,
+                            policy: DelayPolicy::Explicit,
+                        },
+                    );
                 } else {
                     // Optionally log a warning during genesis build
                     log::warn!(

pallets/reversible-transfers/src/mock.rs

@@ -122,7 +122,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
         .unwrap();
 
     pallet_balances::GenesisConfig::<Test> {
-        balances: vec![(1, 1_000_000_000_000_000), (2, 2)],
+        balances: vec![(1, 1_000_000_000_000_000), (2, 2), (255, 100_000_000_000)],
     }
     .assimilate_storage(&mut t)
     .unwrap();