Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor pallet recovery to fungibles #1807

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion polkadot/runtime/rococo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,7 @@ impl pallet_recovery::Config for Runtime {
type FriendDepositFactor = FriendDepositFactor;
type MaxFriends = MaxFriends;
type RecoveryDeposit = RecoveryDeposit;
type RuntimeHoldReason = RuntimeHoldReason;
}

parameter_types! {
Expand Down Expand Up @@ -1377,7 +1378,7 @@ construct_runtime! {
Society: pallet_society::{Pallet, Call, Storage, Event<T>} = 26,

// Social recovery module.
Recovery: pallet_recovery::{Pallet, Call, Storage, Event<T>} = 27,
Recovery: pallet_recovery::{Pallet, Call, Storage, Event<T>, HoldReason} = 27,

// Vesting. Usable initially, but removed once all vesting is finished.
Vesting: pallet_vesting::{Pallet, Call, Storage, Event<T>, Config<T>} = 28,
Expand Down
3 changes: 2 additions & 1 deletion polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -935,6 +935,7 @@ impl pallet_recovery::Config for Runtime {
type FriendDepositFactor = FriendDepositFactor;
type MaxFriends = MaxFriends;
type RecoveryDeposit = RecoveryDeposit;
type RuntimeHoldReason = RuntimeHoldReason;
}

parameter_types! {
Expand Down Expand Up @@ -1437,7 +1438,7 @@ construct_runtime! {
Identity: pallet_identity::{Pallet, Call, Storage, Event<T>} = 17,

// Social recovery module.
Recovery: pallet_recovery::{Pallet, Call, Storage, Event<T>} = 18,
Recovery: pallet_recovery::{Pallet, Call, Storage, Event<T>, HoldReason} = 18,

// Vesting. Usable initially, but removed once all vesting is finished.
Vesting: pallet_vesting::{Pallet, Call, Storage, Event<T>, Config<T>} = 19,
Expand Down
1 change: 1 addition & 0 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1514,6 +1514,7 @@ impl pallet_recovery::Config for Runtime {
type FriendDepositFactor = FriendDepositFactor;
type MaxFriends = MaxFriends;
type RecoveryDeposit = RecoveryDeposit;
type RuntimeHoldReason = RuntimeHoldReason;
}

parameter_types! {
Expand Down
46 changes: 22 additions & 24 deletions substrate/frame/recovery/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@ use super::*;

use crate::Pallet;
use frame_benchmarking::v1::{account, benchmarks, whitelisted_caller};
use frame_support::traits::{Currency, Get};
use frame_support::traits::Get;
use frame_system::RawOrigin;
use sp_runtime::traits::Bounded;

const SEED: u32 = 0;
const DEFAULT_DELAY: u32 = 0;
Expand All @@ -34,8 +33,7 @@ fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) {

fn get_total_deposit<T: Config>(
bounded_friends: &FriendsOf<T>,
) -> Option<<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance>
{
) -> Option<<<T as Config>::Currency as Inspect<<T as frame_system::Config>::AccountId>>::Balance> {
let friend_deposit = T::FriendDepositFactor::get()
.checked_mul(&bounded_friends.len().saturated_into())
.unwrap();
Expand All @@ -51,9 +49,9 @@ fn generate_friends<T: Config>(num: u32) -> Vec<<T as frame_system::Config>::Acc

for friend in 0..friends.len() {
// Top up accounts of friends
T::Currency::make_free_balance_be(
T::Currency::set_balance(
&friends.get(friend).unwrap(),
BalanceOf::<T>::max_value(),
T::Currency::minimum_balance() * 10000u32.into(),
);
}

Expand All @@ -67,7 +65,7 @@ fn add_caller_and_generate_friends<T: Config>(
// Create friends
let mut friends = generate_friends::<T>(num - 1);

T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
T::Currency::set_balance(&caller, T::Currency::minimum_balance() * 10000u32.into());

friends.push(caller);

Expand All @@ -78,7 +76,7 @@ fn add_caller_and_generate_friends<T: Config>(
}

fn insert_recovery_account<T: Config>(caller: &T::AccountId, account: &T::AccountId) {
T::Currency::make_free_balance_be(&account, BalanceOf::<T>::max_value());
T::Currency::set_balance(&account, T::Currency::minimum_balance() * 10000u32.into());

let n = T::MaxFriends::get();

Expand All @@ -96,8 +94,8 @@ fn insert_recovery_account<T: Config>(caller: &T::AccountId, account: &T::Accoun
threshold: n as u16,
};

// Reserve deposit for recovery
T::Currency::reserve(&caller, total_deposit).unwrap();
// Hold deposit for recovery
T::Currency::hold(&HoldReason::RecoveryProcessDeposit.into(), &caller, total_deposit).unwrap();

<Recoverable<T>>::insert(&account, recovery_config);
}
Expand Down Expand Up @@ -138,7 +136,7 @@ benchmarks! {
let n in 1 .. T::MaxFriends::get();

let caller: T::AccountId = whitelisted_caller();
T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
T::Currency::set_balance(&caller, T::Currency::minimum_balance() * 10000u32.into());

// Create friends
let friends = generate_friends::<T>(n);
Expand All @@ -153,7 +151,7 @@ benchmarks! {

initiate_recovery {
let caller: T::AccountId = whitelisted_caller();
T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
T::Currency::set_balance(&caller, T::Currency::minimum_balance() * 10000u32.into());

let lost_account: T::AccountId = account("lost_account", 0, SEED);
let lost_account_lookup = T::Lookup::unlookup(lost_account.clone());
Expand Down Expand Up @@ -198,8 +196,8 @@ benchmarks! {
// Create the recovery config storage item
<Recoverable<T>>::insert(&lost_account, recovery_config.clone());

// Reserve deposit for recovery
T::Currency::reserve(&caller, total_deposit).unwrap();
// Hold deposit for configuration
T::Currency::hold(&HoldReason::ConfigurationDeposit.into(), &caller, total_deposit).unwrap();

// Create an active recovery status
let recovery_status = ActiveRecovery {
Expand Down Expand Up @@ -232,7 +230,7 @@ benchmarks! {
let lost_account: T::AccountId = account("lost_account", 0, SEED);
let lost_account_lookup = T::Lookup::unlookup(lost_account.clone());

T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
T::Currency::set_balance(&caller, T::Currency::minimum_balance() * 10000u32.into());

// Create friends
let friends = generate_friends::<T>(n);
Expand All @@ -251,8 +249,8 @@ benchmarks! {
// Create the recovery config storage item
<Recoverable<T>>::insert(&lost_account, recovery_config.clone());

// Reserve deposit for recovery
T::Currency::reserve(&caller, total_deposit).unwrap();
// Hold deposit for configuration
T::Currency::hold(&HoldReason::ConfigurationDeposit.into(), &caller, total_deposit).unwrap();

// Create an active recovery status
let recovery_status = ActiveRecovery {
Expand Down Expand Up @@ -282,8 +280,8 @@ benchmarks! {

let n in 1 .. T::MaxFriends::get();

T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
T::Currency::make_free_balance_be(&rescuer_account, BalanceOf::<T>::max_value());
T::Currency::set_balance(&caller, T::Currency::minimum_balance() * 10000u32.into());
T::Currency::set_balance(&rescuer_account, T::Currency::minimum_balance() * 10000u32.into());

// Create friends
let friends = generate_friends::<T>(n);
Expand All @@ -302,8 +300,8 @@ benchmarks! {
// Create the recovery config storage item
<Recoverable<T>>::insert(&caller, recovery_config.clone());

// Reserve deposit for recovery
T::Currency::reserve(&caller, total_deposit).unwrap();
// Hold deposit for configuration
T::Currency::hold(&HoldReason::ConfigurationDeposit.into(), &caller, total_deposit).unwrap();

// Create an active recovery status
let recovery_status = ActiveRecovery {
Expand Down Expand Up @@ -331,7 +329,7 @@ benchmarks! {

let caller: T::AccountId = whitelisted_caller();

T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
T::Currency::set_balance(&caller, T::Currency::minimum_balance() * 10000u32.into());

// Create friends
let friends = generate_friends::<T>(n);
Expand All @@ -350,8 +348,8 @@ benchmarks! {
// Create the recovery config storage item
<Recoverable<T>>::insert(&caller, recovery_config);

// Reserve deposit for recovery
T::Currency::reserve(&caller, total_deposit).unwrap();
// Hold deposit for configuration
T::Currency::hold(&HoldReason::ConfigurationDeposit.into(), &caller, total_deposit).unwrap();
}: _(
RawOrigin::Signed(caller.clone())
) verify {
Expand Down
78 changes: 55 additions & 23 deletions substrate/frame/recovery/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@
//! configuration on the recovered account and reclaim the recovery configuration deposit they
//! placed.
//! 9. Using `as_recovered`, the account owner is able to call any other pallets to clean up their
//! state and reclaim any reserved or locked funds. They can then transfer all funds from the
//! state and reclaim any held or locked funds. They can then transfer all funds from the
//! recovered account to the new account.
//! 10. When the recovered account becomes reaped (i.e. its free and reserved balance drops to
//! zero), the final recovery link is removed.
//! 10. When the recovered account becomes reaped (i.e. its free and held balance drops to zero),
//! the final recovery link is removed.
//!
//! ### Malicious Recovery Attempts
//!
Expand Down Expand Up @@ -158,9 +158,14 @@ use sp_runtime::{
};
use sp_std::prelude::*;

#[cfg(feature = "runtime-benchmarks")]
use frame_support::traits::fungible::Mutate;
use frame_support::{
dispatch::{GetDispatchInfo, PostDispatchInfo},
traits::{BalanceStatus, Currency, ReservableCurrency},
traits::{
fungible::{Inspect, MutateHold},
tokens::{Fortitude, Precision, Restriction},
},
BoundedVec,
};

Expand All @@ -177,7 +182,7 @@ mod tests;
pub mod weights;

type BalanceOf<T> =
<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
<<T as Config>::Currency as Inspect<<T as frame_system::Config>::AccountId>>::Balance;

type FriendsOf<T> = BoundedVec<<T as frame_system::Config>::AccountId, <T as Config>::MaxFriends>;
type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;
Expand All @@ -187,7 +192,7 @@ type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup
pub struct ActiveRecovery<BlockNumber, Balance, Friends> {
/// The block number when the recovery process started.
created: BlockNumber,
/// The amount held in reserve of the `depositor`,
/// The amount held the `depositor`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The amount held the `depositor`,
/// The amount held by the `depositor`,

/// To be returned once this recovery process is closed.
deposit: Balance,
/// The friends which have vouched so far. Always sorted.
Expand All @@ -200,7 +205,7 @@ pub struct RecoveryConfig<BlockNumber, Balance, Friends> {
/// The minimum number of blocks since the start of the recovery process before the account
/// can be recovered.
delay_period: BlockNumber,
/// The amount held in reserve of the `depositor`,
/// The amount held of the `depositor`,
/// to be returned once this configuration is removed.
deposit: Balance,
/// The list of friends which can help recover an account. Always sorted.
Expand Down Expand Up @@ -235,9 +240,15 @@ pub mod pallet {
+ From<frame_system::Call<Self>>;

/// The currency mechanism.
type Currency: ReservableCurrency<Self::AccountId>;
#[cfg(feature = "runtime-benchmarks")]
type Currency: Mutate<Self::AccountId>
+ MutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason>;
Comment on lines +243 to +245
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to set_balance in benchmarking, so Mutate is needed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to set custom where bounds for benchmarks, like this.
So it should be possible to add a <T as Config>::Currency: Mutate there.


/// The currency mechanism.
#[cfg(not(feature = "runtime-benchmarks"))]
type Currency: MutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason>;

/// The base amount of currency needed to reserve for creating a recovery configuration.
/// The base amount of currency needed to hold for creating a recovery configuration.
///
/// This is held for an additional storage item whose value size is
/// `2 + sizeof(BlockNumber, Balance)` bytes.
Expand All @@ -254,14 +265,14 @@ pub mod pallet {

/// The maximum amount of friends allowed in a recovery configuration.
///
/// NOTE: The threshold programmed in this Pallet uses u16, so it does
/// not really make sense to have a limit here greater than u16::MAX.
/// NOTE: The threshold programmed in this Pallet uses u32, so it does
/// not really make sense to have a limit here greater than u32::MAX.
/// But also, that is a lot more than you should probably set this value
/// to anyway...
#[pallet::constant]
type MaxFriends: Get<u32>;

/// The base amount of currency needed to reserve for starting a recovery.
/// The base amount of currency needed to hold for starting a recovery.
///
/// This is primarily held for deterring malicious recovery attempts, and should
/// have a value large enough that a bad actor would choose not to place this
Expand All @@ -270,6 +281,9 @@ pub mod pallet {
/// threshold.
#[pallet::constant]
type RecoveryDeposit: Get<BalanceOf<Self>>;

/// The overarching hold reason.
type RuntimeHoldReason: From<HoldReason>;
}

/// Events type.
Expand Down Expand Up @@ -330,6 +344,16 @@ pub mod pallet {
BadState,
}

/// The reasons for the pallet recovery placing holds on funds.
#[pallet::composite_enum]
pub enum HoldReason {
/// The Pallet holds it as the initial Configuration Deposit, when the Recovery
/// Configuration is created.
ConfigurationDeposit,
/// The Pallet holds it as the Deposit for starting a Recovery Process.
RecoveryProcessDeposit,
}

/// The set of recoverable accounts and their recovery configuration.
#[pallet::storage]
#[pallet::getter(fn recovery_config)]
Expand Down Expand Up @@ -424,7 +448,7 @@ pub mod pallet {
/// Create a recovery configuration for your account. This makes your account recoverable.
///
/// Payment: `ConfigDepositBase` + `FriendDepositFactor` * #_of_friends balance
/// will be reserved for storing the recovery configuration. This deposit is returned
/// will be held for storing the recovery configuration. This deposit is returned
/// in full when the user calls `remove_recovery`.
///
/// The dispatch origin for this call must be _Signed_.
Expand Down Expand Up @@ -462,8 +486,8 @@ pub mod pallet {
let total_deposit = T::ConfigDepositBase::get()
.checked_add(&friend_deposit)
.ok_or(ArithmeticError::Overflow)?;
// Reserve the deposit
T::Currency::reserve(&who, total_deposit)?;
// Hold the deposit
T::Currency::hold(&HoldReason::ConfigurationDeposit.into(), &who, total_deposit)?;
// Create the recovery configuration
let recovery_config = RecoveryConfig {
delay_period,
Expand All @@ -480,7 +504,7 @@ pub mod pallet {

/// Initiate the process for recovering a recoverable account.
///
/// Payment: `RecoveryDeposit` balance will be reserved for initiating the
/// Payment: `RecoveryDeposit` balance will be held for initiating the
/// recovery process. This deposit will always be repatriated to the account
/// trying to be recovered. See `close_recovery`.
///
Expand All @@ -506,7 +530,7 @@ pub mod pallet {
);
// Take recovery deposit
let recovery_deposit = T::RecoveryDeposit::get();
T::Currency::reserve(&who, recovery_deposit)?;
T::Currency::hold(&HoldReason::RecoveryProcessDeposit.into(), &who, recovery_deposit)?;
// Create an active recovery status
let recovery_status = ActiveRecovery {
created: <frame_system::Pallet<T>>::block_number(),
Expand Down Expand Up @@ -637,13 +661,16 @@ pub mod pallet {
// Take the active recovery process started by the rescuer for this account.
let active_recovery =
<ActiveRecoveries<T>>::take(&who, &rescuer).ok_or(Error::<T>::NotStarted)?;
// Move the reserved funds from the rescuer to the rescued account.
// Move the held funds from the rescuer to the rescued account.
// Acts like a slashing mechanism for those who try to maliciously recover accounts.
let res = T::Currency::repatriate_reserved(
let res = T::Currency::transfer_on_hold(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking about how to do a lazy migration here. Maybe you can require the Currency config trait to be Currency + MutateHold so that it requires currency and fungibles.

Then in this function you can first try to use the new transfer_on_hold function, but if that does not work then fallback to the old repatriate_reserved. Do you think this could work?
Otherwise the MBMs are still in review and will need audit, so its a bit out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a big fan of lazy migrations here, cause that will generate some "garbage" code that will live there forever. It seems more like a patch to me than a real solution. It's been a while already since we wanted the traits to be migrated that I would rather wait for the MBM to be ready and apply a proper solution

&HoldReason::RecoveryProcessDeposit.into(),
&rescuer,
&who,
active_recovery.deposit,
BalanceStatus::Free,
Precision::BestEffort,
Restriction::Free,
Fortitude::Force,
);
debug_assert!(res.is_ok());
Self::deposit_event(Event::<T>::RecoveryClosed {
Expand All @@ -658,7 +685,7 @@ pub mod pallet {
/// NOTE: The user must make sure to call `close_recovery` on all active
/// recovery attempts before calling this function else it will fail.
///
/// Payment: By calling this function the recoverable account will unreserve
/// Payment: By calling this function the recoverable account will release
/// their recovery configuration deposit.
/// (`ConfigDepositBase` + `FriendDepositFactor` * #_of_friends)
///
Expand All @@ -674,8 +701,13 @@ pub mod pallet {
// Take the recovery configuration for this account.
let recovery_config = <Recoverable<T>>::take(&who).ok_or(Error::<T>::NotRecoverable)?;

// Unreserve the initial deposit for the recovery configuration.
T::Currency::unreserve(&who, recovery_config.deposit);
// Release the initial deposit for the recovery configuration.
let _ = T::Currency::release(
&HoldReason::ConfigurationDeposit.into(),
&who,
recovery_config.deposit,
Precision::BestEffort,
);
Self::deposit_event(Event::<T>::RecoveryRemoved { lost_account: who });
Ok(())
}
Expand Down
Loading
Loading