From 43e6eac229867715ac1ab6319d049a21b33137ea Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sat, 4 Jan 2020 20:02:25 +0100 Subject: [PATCH 01/21] Initial sketch of social recovery pallet --- frame/recover/Cargo.toml | 31 ++++ frame/recover/src/lib.rs | 322 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 353 insertions(+) create mode 100644 frame/recover/Cargo.toml create mode 100644 frame/recover/src/lib.rs diff --git a/frame/recover/Cargo.toml b/frame/recover/Cargo.toml new file mode 100644 index 0000000000000..925a464b87acc --- /dev/null +++ b/frame/recover/Cargo.toml @@ -0,0 +1,31 @@ +[package] +name = "pallet-recover" +version = "2.0.0" +authors = ["Parity Technologies "] +edition = "2018" + +[dependencies] +serde = { version = "1.0.101", optional = true } +codec = { package = "parity-scale-codec", version = "1.0.0", default-features = false, features = ["derive"] } +enumflags2 = { version = "0.6.2" } +sp-std = { version = "2.0.0", default-features = false, path = "../../primitives/std" } +sp-io = { version = "2.0.0", default-features = false, path = "../../primitives/io" } +sp-runtime = { version = "2.0.0", default-features = false, path = "../../primitives/runtime" } +frame-support = { version = "2.0.0", default-features = false, path = "../support" } +frame-system = { version = "2.0.0", default-features = false, path = "../system" } + +[dev-dependencies] +sp-core = { version = "2.0.0", path = "../../primitives/core" } +pallet-balances = { version = "2.0.0", path = "../balances" } + +[features] +default = ["std"] +std = [ + "serde", + "codec/std", + "sp-std/std", + "sp-io/std", + "sp-runtime/std", + "frame-support/std", + "frame-system/std", +] diff --git a/frame/recover/src/lib.rs b/frame/recover/src/lib.rs new file mode 100644 index 0000000000000..fce39dd8638c6 --- /dev/null +++ b/frame/recover/src/lib.rs @@ -0,0 +1,322 @@ +// Ensure we're `no_std` when compiling for Wasm. +#![cfg_attr(not(feature = "std"), no_std)] + +/// Configuration trait. +pub trait Trait: frame_system::Trait { + /// The overarching event type. + type Event: From> + Into<::Event>; + + /// The overarching call type. + type Call: Parameter + Dispatchable + GetDispatchInfo; + + /// The currency mechanism. + type Currency: ReservableCurrency; + + /// The base amount of currency needed to reserve for creating a recovery setup. + /// + /// This is held for an additional storage item whose value size is + /// TODO bytes. + type SetupDepositBase: Get>; + + /// The amount of currency needed per additional user when creating a recovery setup. + /// + /// This is held for adding TODO bytes more into a pre-existing storage value. + type SetupDepositFactor: Get>; + + /// The maximum amount of friends allowed in a recovery setup. + type MaxFriends: Get; + + /// The base amount of currency needed to reserve for starting a recovery. + /// + /// This is held for an additional storage item whose value size is + /// TODO bytes. + type RecoveryDeposit: Get>; +} + +#[derive(Clone, Eq, PartialEq, Encode, Decode, Default, RuntimeDebug)] +pub enum RecoveryStep { + Active, + Succeeded, + Failed, +} + +/// An open multisig operation. +#[derive(Clone, Eq, PartialEq, Encode, Decode, Default, RuntimeDebug)] +pub struct RecoveryStatus { + /// Denotes if the recovery process is active. + step: RecoveryStep, + /// The last block when someone has tried to recover the account + last: BlockNumber, + /// The amount held in reserve of the `depositor`, to be returned once this setup is closed. + deposit: Balance, + /// The approvals achieved so far, including the depositor. Always sorted. + friends: Vec, +} + +/// An open multisig operation. +#[derive(Clone, Eq, PartialEq, Encode, Decode, Default, RuntimeDebug)] +pub struct RecoverySetup { + /// The minimum amount of time between friend approvals. + delay_period: BlockNumber + /// The amount held in reserve of the `depositor`, to be returned once this setup is closed. + deposit: Balance, + /// The list of friends which can help recover an account. Always sorted. + friends: Vec, + /// The number of approving friends needed to recover an account. + threshold: u16, +} + +decl_storage! { + trait Store for Module as Utility { + /// The set of recovery setups. + pub RecoverySetups get(fn recovery_setup): + map T::AccountId => Option, T::AccountId>>; + /// Active recovery attempts. + /// + /// First account is the account to be recovered, and the second account is the user trying to + /// recover the account. + pub ActiveRecoveries get(fn active_recovery): + double_map hasher(twox_64_concat) T::AccountId, twox_64_concat(T::AccountId) => + Option, T::AccountId>>; + /// The final list of recovered accounts. + /// + /// Map from the recovered account to the user who can access it. + pub Recovered get(fn recovered_account): T::AccountId => Option; + } +} + +decl_event! { + /// Events type. + pub enum Event where + AccountId = ::AccountId, + { + /// A recovery process has been set up for an account + RecoveryCreated(AccountId), + /// A recovery process has been initiated by account_1 for account_2 + RecoveryInitiated(AccountId, AccountId), + /// A recovery process by account_1 for account_2 has been vouched for by account_3 + RecoveryVouched(AccountId, AccountId, AccountId), + /// Account_1 has recovered account_2 + AccountRecovered(AccountId, AccountId), + /// A recovery process has been removed for an account + RecoveryRemoved(AccountId), + } +} + +decl_error! { + pub enum Error { + /// User is not allowed to make a call on behalf of this account + NotAllowed, + /// Threshold must be greater than zero + ZeroThreshold, + /// Friends list must be greater than zero + ZeroFriends, + /// Friends list must be less than max friends + MaxFriends, + /// Friends list must be sorted + NotSorted, + /// This account is not set up for recovery + NotSetup, + /// This account is already set up for recovery + AlreadySetup, + /// A recovery process has already started for this account + AlreadyStarted, + /// A recovery process has not started for this rescuer + NotStarted, + /// This account is not a friend who can vouch + NotFriend, + /// The friend must wait until the delay period to vouch for this recovery + DelayPeriod, + /// This user has already vouched for this recovery + AlreadyVouched, + /// The threshold for recovering this account has not been met + Threshold, + /// There are still active recovery attempts that need to be closed + StillActive, + } +} + +decl_module! { + pub struct Module for enum Call where origin: T::Origin { + /// Deposit one of this module's events by using the default implementation. + fn deposit_event() = default; + + /// Send a call through a recovered account. + /// + /// The dispatch origin for this call must be _Signed_. + /// + /// # + /// - The weight of the `call`. + /// # + #[weight = ::Call>>::new()] + fn as_recovered(origin, account: AccountId, call: Box<::Call>) -> DispatchResult { + let who = ensure_signed(origin)?; + // Check `who` is allowed to make a call on behalf of `account` + ensure!(Self::recovered_account(&account) == Some(who), Error::::NotAllowed); + call.dispatch(frame_system::RawOrigin::Signed(account).into()) + } + + /// Allow Sudo to bypass the recovery process and set an alias account. + fn set_recovered_account(origin, rescuee: AccountId, rescuer: AccountId) { + ensure_root(origin)?; + + // Create the recovery storage item. + >::insert(rescuee, rescuer); + + Self::deposit_event(RawEvent::AccountRecovered(rescuer, rescuee)); + } + + /// Create a recovery process for your account. + fn create_recovery(origin, friends: Vec, threshold: u16, delay_period: T::BlockNumber) { + let who = ensure_signed(origin)?; + // Check account is not already set up for recovery + ensure!(>::exists(&account), Error::::AlreadySetup); + // Check user input is valid + ensure!(threshold >= 1, Error::::ZeroThreshold); + ensure!(!friends.is_empty(), Error::::ZeroFriends); + ensure!(friends.len() < max_sigs, Error::::MaxFriends); + ensure!(Self::is_sorted(friends), Error::::NotSorted); + + // Total deposit is base fee + number of friends * factor fee + let total_deposit = T::SetupDepositBase::get() + T::SetupDepositFactor::get() * friends.len(); + // Reserve the deposit + T::Currency::reserve(&who, total_deposit)?; + + // Create the recovery setup + let recovery_setup = RecoverySetup { + delay_period + deposit: total_deposit, + friends, + threshold, + }; + + >::insert(who, recovery_setup); + + Self::deposit_event(RawEvent::RecoveryCreated(who)); + } + + fn initiate_recovery(origin, account: AccountId) { + let who = ensure_signed(origin)?; + // Check that the account has a recovery setup + ensure!(>::exists(&account), Error::::NotSetup); + // Check that the recovery process has not already been started + ensure!(!>::exists(&account, &who), Error::::AlreadyStarted); + // Take recovery deposit + let recovery_deposit = T::RecoveryDeposit::get(); + T::Currency::reserve(&who, recovery_deposit)?; + // Create an active recovery status + let recovery_status = RecoveryStatus { + step: RecoveryStep::Active, + last: T::BlockNumber::zero(), + deposit: recovery_deposit, + friends: vec![], + }; + + >::insert(&account, &who, recovery_status); + + Self::deposit_event(RawEvent::RecoveryInitiated(who, account)); + } + + fn vouch_recovery(origin, rescuee: AccountId, rescuer: AccountId) { + let who = ensure_signed(origin)?; + + // Check that there is a recovery setup for this rescuee + if let Some(recovery_setup) = Self::recovery_setup(&rescuee) { + // Check that the recovery process has been initiated for this rescuer + if let Some(mut active_recovery) = Self::active_recovery(&rescuee, &rescuer) { + // Make sure the voter is a friend + ensure!(Self::is_friend(recovery_setup.friends, &who), Error::::NotFriend); + // Make sure the delay period has passed + let current_block_number = >::block_number(); + ensure!( + active_recovery.last + recovery_setup.delay_period >= current_block_number), + Error::::DelayPeriod + ); + // Either insert the vouch, or return an error that the user already vouched. + match active_recovery.friends.binary_search(&who) { + Ok(pos) => Err(Error::AlreadyVouched)?, + Err(pos) => active_recovery.friends.insert(pos, who), + } + // Update the last vote time + active_recovery.last = current_block_number; + // Update storage with the latest details + >::insert(&rescuee, &rescuer, active_recovery); + + Self::deposit_event(RawEvent::RecoveryVouched(rescuer, rescuee, who)); + } else { + Err(Error::::NotStarted)? + } + } else { + Err(Error::::NotSetup)? + } + } + + /// Allow a rescuer to claim their recovered account. + fn claim_recovery(origin, account: T::AccountId) { + let who = ensure_signed(origin)?; + // Check that there is a recovery setup for this rescuee + if let Some(recovery_setup) = Self::recovery_setup(&rescuee) { + // Check that the recovery process has been initiated for this rescuer + if let Some(active_recovery) = Self::active_recovery(&rescuee, &rescuer) { + // Make sure the threshold is met + ensure!( + recovery_setup.threshold <= active_recovery.friends.len(), + Error::::Threshold + ); + + // Create the recovery storage item + >::insert(&account, &who) + + Self::deposit_event(RawEvent::AccountRecovered(who, rescuee)); + } else { + Err(Error::::NotStarted)? + } + } else { + Err(Error::::NotSetup)? + } + } + + /// Close an active recovery process. + /// + /// Can only be called by the account trying to be recovered. + fn close_recovery(origin, rescuer) { + let who = ensure_signed(origin)?; + if let Some(active_recovery) = >::take(&who, &rescuer) { + // Move the reserved funds from the rescuer to the rescuee account. + // Acts like a slashing mechanism for those who try to maliciously recover accounts. + T::Currency::repatriate_reserved(&rescuer, &who, active_recovery.deposit); + } else { + Err(Error::::NotStarted)? + } + } + + /// Remove the recovery process for your account. + /// + /// The user must make sure to call `close_recovery` on all active recovery attempts + /// before calling this function. + fn remove_recovery(origin) { + let who = ensure_signed(origin)?; + // Check there are no active recoveries + let active_recoveries = >::iter_prefix(&who); + ensure!(active_recoveries.next().is_none(), Error::::StillActive); + // Check account has recovery setup + if let Some(recovery_setup) = >::take(&account) { + T::Currency::unreserve(&who, recovery_setup.deposit); + + Self::deposit_event(RawEvent::RecoveryRemoved); + } + } + } +} + +impl Module { + + /// Check that friends list is sorted. + fn is_sorted(friends: Vec) -> bool { + friends.windows(2).all(|w| w[0] <= w[1]) + } + + fn is_friend(friends: Vec, friend: T::AccountId) -> bool { + friends.binary_search(&friend).is_ok() + } +} \ No newline at end of file From ccda36c7875ebbafeb96357cd685029138875a66 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sat, 4 Jan 2020 21:01:56 +0100 Subject: [PATCH 02/21] Fix compilation issues --- Cargo.lock | 16 +++++ Cargo.toml | 1 + frame/recover/src/lib.rs | 129 ++++++++++++++++++++++----------------- 3 files changed, 90 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5113c6a5ada76..e7dca880536e3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3870,6 +3870,22 @@ dependencies = [ "sp-std 2.0.0", ] +[[package]] +name = "pallet-recover" +version = "2.0.0" +dependencies = [ + "enumflags2 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)", + "frame-support 2.0.0", + "frame-system 2.0.0", + "pallet-balances 2.0.0", + "parity-scale-codec 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 1.0.103 (registry+https://github.com/rust-lang/crates.io-index)", + "sp-core 2.0.0", + "sp-io 2.0.0", + "sp-runtime 2.0.0", + "sp-std 2.0.0", +] + [[package]] name = "pallet-scored-pool" version = "2.0.0" diff --git a/Cargo.toml b/Cargo.toml index 505cd299d9c71..707cfa75b3589 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -78,6 +78,7 @@ members = [ "frame/nicks", "frame/offences", "frame/randomness-collective-flip", + "frame/recover", "frame/scored-pool", "frame/session", "frame/staking", diff --git a/frame/recover/src/lib.rs b/frame/recover/src/lib.rs index fce39dd8638c6..f1a103d2b6c37 100644 --- a/frame/recover/src/lib.rs +++ b/frame/recover/src/lib.rs @@ -1,6 +1,39 @@ +// Copyright 2017-2019 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] +use sp_std::prelude::*; +use sp_runtime::{ + traits::{StaticLookup, Dispatchable, SaturatedConversion, Zero}, + DispatchError, DispatchResult +}; +use codec::{Encode, Decode}; + +use frame_support::{ + decl_module, decl_event, decl_storage, decl_error, ensure, + Parameter, RuntimeDebug, + weights::{SimpleDispatchInfo, GetDispatchInfo, PaysFee, WeighData, Weight, ClassifyDispatch, DispatchClass}, + traits::{Currency, ReservableCurrency, Get}, +}; +use frame_system::{self as system, ensure_signed, ensure_root}; + +type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; + /// Configuration trait. pub trait Trait: frame_system::Trait { /// The overarching event type. @@ -33,18 +66,9 @@ pub trait Trait: frame_system::Trait { type RecoveryDeposit: Get>; } -#[derive(Clone, Eq, PartialEq, Encode, Decode, Default, RuntimeDebug)] -pub enum RecoveryStep { - Active, - Succeeded, - Failed, -} - /// An open multisig operation. #[derive(Clone, Eq, PartialEq, Encode, Decode, Default, RuntimeDebug)] pub struct RecoveryStatus { - /// Denotes if the recovery process is active. - step: RecoveryStep, /// The last block when someone has tried to recover the account last: BlockNumber, /// The amount held in reserve of the `depositor`, to be returned once this setup is closed. @@ -57,7 +81,7 @@ pub struct RecoveryStatus { #[derive(Clone, Eq, PartialEq, Encode, Decode, Default, RuntimeDebug)] pub struct RecoverySetup { /// The minimum amount of time between friend approvals. - delay_period: BlockNumber + delay_period: BlockNumber, /// The amount held in reserve of the `depositor`, to be returned once this setup is closed. deposit: Balance, /// The list of friends which can help recover an account. Always sorted. @@ -81,7 +105,7 @@ decl_storage! { /// The final list of recovered accounts. /// /// Map from the recovered account to the user who can access it. - pub Recovered get(fn recovered_account): T::AccountId => Option; + pub Recovered get(fn recovered_account): map T::AccountId => Option; } } @@ -104,7 +128,7 @@ decl_event! { } decl_error! { - pub enum Error { + pub enum Error for Module { /// User is not allowed to make a call on behalf of this account NotAllowed, /// Threshold must be greater than zero @@ -140,16 +164,9 @@ decl_module! { pub struct Module for enum Call where origin: T::Origin { /// Deposit one of this module's events by using the default implementation. fn deposit_event() = default; - + /// Send a call through a recovered account. - /// - /// The dispatch origin for this call must be _Signed_. - /// - /// # - /// - The weight of the `call`. - /// # - #[weight = ::Call>>::new()] - fn as_recovered(origin, account: AccountId, call: Box<::Call>) -> DispatchResult { + fn as_recovered(origin, account: T::AccountId, call: Box<::Call>) -> DispatchResult { let who = ensure_signed(origin)?; // Check `who` is allowed to make a call on behalf of `account` ensure!(Self::recovered_account(&account) == Some(who), Error::::NotAllowed); @@ -157,45 +174,46 @@ decl_module! { } /// Allow Sudo to bypass the recovery process and set an alias account. - fn set_recovered_account(origin, rescuee: AccountId, rescuer: AccountId) { + fn set_recovered_account(origin, rescuee: T::AccountId, rescuer: T::AccountId) { ensure_root(origin)?; // Create the recovery storage item. - >::insert(rescuee, rescuer); + >::insert(&rescuee, &rescuer); Self::deposit_event(RawEvent::AccountRecovered(rescuer, rescuee)); } - + /// Create a recovery process for your account. fn create_recovery(origin, friends: Vec, threshold: u16, delay_period: T::BlockNumber) { let who = ensure_signed(origin)?; // Check account is not already set up for recovery - ensure!(>::exists(&account), Error::::AlreadySetup); + ensure!(>::exists(&who), Error::::AlreadySetup); // Check user input is valid ensure!(threshold >= 1, Error::::ZeroThreshold); ensure!(!friends.is_empty(), Error::::ZeroFriends); - ensure!(friends.len() < max_sigs, Error::::MaxFriends); - ensure!(Self::is_sorted(friends), Error::::NotSorted); + let max_friends = T::MaxFriends::get() as usize; + ensure!(friends.len() < max_friends, Error::::MaxFriends); + ensure!(Self::is_sorted(&friends), Error::::NotSorted); // Total deposit is base fee + number of friends * factor fee - let total_deposit = T::SetupDepositBase::get() + T::SetupDepositFactor::get() * friends.len(); + let total_deposit = T::SetupDepositBase::get() + T::SetupDepositFactor::get() * friends.len().saturated_into(); // Reserve the deposit T::Currency::reserve(&who, total_deposit)?; // Create the recovery setup let recovery_setup = RecoverySetup { - delay_period + delay_period, deposit: total_deposit, friends, threshold, }; - >::insert(who, recovery_setup); + >::insert(&who, recovery_setup); Self::deposit_event(RawEvent::RecoveryCreated(who)); } - - fn initiate_recovery(origin, account: AccountId) { + + fn initiate_recovery(origin, account: T::AccountId) { let who = ensure_signed(origin)?; // Check that the account has a recovery setup ensure!(>::exists(&account), Error::::NotSetup); @@ -206,7 +224,6 @@ decl_module! { T::Currency::reserve(&who, recovery_deposit)?; // Create an active recovery status let recovery_status = RecoveryStatus { - step: RecoveryStep::Active, last: T::BlockNumber::zero(), deposit: recovery_deposit, friends: vec![], @@ -216,8 +233,8 @@ decl_module! { Self::deposit_event(RawEvent::RecoveryInitiated(who, account)); } - - fn vouch_recovery(origin, rescuee: AccountId, rescuer: AccountId) { + + fn vouch_recovery(origin, rescuee: T::AccountId, rescuer: T::AccountId) { let who = ensure_signed(origin)?; // Check that there is a recovery setup for this rescuee @@ -225,17 +242,17 @@ decl_module! { // Check that the recovery process has been initiated for this rescuer if let Some(mut active_recovery) = Self::active_recovery(&rescuee, &rescuer) { // Make sure the voter is a friend - ensure!(Self::is_friend(recovery_setup.friends, &who), Error::::NotFriend); + ensure!(Self::is_friend(&recovery_setup.friends, &who), Error::::NotFriend); // Make sure the delay period has passed let current_block_number = >::block_number(); ensure!( - active_recovery.last + recovery_setup.delay_period >= current_block_number), + active_recovery.last + recovery_setup.delay_period >= current_block_number, Error::::DelayPeriod ); // Either insert the vouch, or return an error that the user already vouched. match active_recovery.friends.binary_search(&who) { - Ok(pos) => Err(Error::AlreadyVouched)?, - Err(pos) => active_recovery.friends.insert(pos, who), + Ok(_pos) => Err(Error::::AlreadyVouched)?, + Err(pos) => active_recovery.friends.insert(pos, who.clone()), } // Update the last vote time active_recovery.last = current_block_number; @@ -250,24 +267,24 @@ decl_module! { Err(Error::::NotSetup)? } } - + /// Allow a rescuer to claim their recovered account. fn claim_recovery(origin, account: T::AccountId) { let who = ensure_signed(origin)?; // Check that there is a recovery setup for this rescuee - if let Some(recovery_setup) = Self::recovery_setup(&rescuee) { + if let Some(recovery_setup) = Self::recovery_setup(&account) { // Check that the recovery process has been initiated for this rescuer - if let Some(active_recovery) = Self::active_recovery(&rescuee, &rescuer) { + if let Some(active_recovery) = Self::active_recovery(&account, &who) { // Make sure the threshold is met ensure!( - recovery_setup.threshold <= active_recovery.friends.len(), + recovery_setup.threshold as usize <= active_recovery.friends.len(), Error::::Threshold ); // Create the recovery storage item - >::insert(&account, &who) + >::insert(&account, &who); - Self::deposit_event(RawEvent::AccountRecovered(who, rescuee)); + Self::deposit_event(RawEvent::AccountRecovered(who, account)); } else { Err(Error::::NotStarted)? } @@ -275,21 +292,21 @@ decl_module! { Err(Error::::NotSetup)? } } - + /// Close an active recovery process. /// /// Can only be called by the account trying to be recovered. - fn close_recovery(origin, rescuer) { + fn close_recovery(origin, rescuer: T::AccountId) { let who = ensure_signed(origin)?; if let Some(active_recovery) = >::take(&who, &rescuer) { // Move the reserved funds from the rescuer to the rescuee account. // Acts like a slashing mechanism for those who try to maliciously recover accounts. - T::Currency::repatriate_reserved(&rescuer, &who, active_recovery.deposit); + let _ = T::Currency::repatriate_reserved(&rescuer, &who, active_recovery.deposit); } else { Err(Error::::NotStarted)? } } - + /// Remove the recovery process for your account. /// /// The user must make sure to call `close_recovery` on all active recovery attempts @@ -297,26 +314,26 @@ decl_module! { fn remove_recovery(origin) { let who = ensure_signed(origin)?; // Check there are no active recoveries - let active_recoveries = >::iter_prefix(&who); + let mut active_recoveries = >::iter_prefix(&who); ensure!(active_recoveries.next().is_none(), Error::::StillActive); // Check account has recovery setup - if let Some(recovery_setup) = >::take(&account) { + if let Some(recovery_setup) = >::take(&who) { T::Currency::unreserve(&who, recovery_setup.deposit); - Self::deposit_event(RawEvent::RecoveryRemoved); + Self::deposit_event(RawEvent::RecoveryRemoved(who)); } } } } impl Module { - /// Check that friends list is sorted. - fn is_sorted(friends: Vec) -> bool { + fn is_sorted(friends: &Vec) -> bool { friends.windows(2).all(|w| w[0] <= w[1]) } - fn is_friend(friends: Vec, friend: T::AccountId) -> bool { + /// Check that a user is a friend in the friends list. + fn is_friend(friends: &Vec, friend: &T::AccountId) -> bool { friends.binary_search(&friend).is_ok() } -} \ No newline at end of file +} From a57fb0ed2ffa0ccb280023e11799c40b244c558b Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 5 Jan 2020 07:10:32 +0100 Subject: [PATCH 03/21] Use a single total delay, rename stuff --- frame/recover/src/lib.rs | 143 ++++++++++++++++++++++----------------- 1 file changed, 80 insertions(+), 63 deletions(-) diff --git a/frame/recover/src/lib.rs b/frame/recover/src/lib.rs index f1a103d2b6c37..9b6a74c24f0ce 100644 --- a/frame/recover/src/lib.rs +++ b/frame/recover/src/lib.rs @@ -19,7 +19,7 @@ use sp_std::prelude::*; use sp_runtime::{ - traits::{StaticLookup, Dispatchable, SaturatedConversion, Zero}, + traits::{StaticLookup, Dispatchable, SaturatedConversion, Zero, CheckedAdd, CheckedMul}, DispatchError, DispatchResult }; use codec::{Encode, Decode}; @@ -27,12 +27,16 @@ use codec::{Encode, Decode}; use frame_support::{ decl_module, decl_event, decl_storage, decl_error, ensure, Parameter, RuntimeDebug, - weights::{SimpleDispatchInfo, GetDispatchInfo, PaysFee, WeighData, Weight, ClassifyDispatch, DispatchClass}, + weights::{ + SimpleDispatchInfo, GetDispatchInfo, PaysFee, WeighData, Weight, + ClassifyDispatch, DispatchClass + }, traits::{Currency, ReservableCurrency, Get}, }; use frame_system::{self as system, ensure_signed, ensure_root}; -type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; +type BalanceOf = + <::Currency as Currency<::AccountId>>::Balance; /// Configuration trait. pub trait Trait: frame_system::Trait { @@ -45,18 +49,18 @@ pub trait Trait: frame_system::Trait { /// The currency mechanism. type Currency: ReservableCurrency; - /// The base amount of currency needed to reserve for creating a recovery setup. + /// The base amount of currency needed to reserve for creating a recovery configuration. /// /// This is held for an additional storage item whose value size is /// TODO bytes. - type SetupDepositBase: Get>; + type ConfigDepositBase: Get>; - /// The amount of currency needed per additional user when creating a recovery setup. + /// The amount of currency needed per additional user when creating a recovery configuration. /// /// This is held for adding TODO bytes more into a pre-existing storage value. - type SetupDepositFactor: Get>; + type FriendDepositFactor: Get>; - /// The maximum amount of friends allowed in a recovery setup. + /// The maximum amount of friends allowed in a recovery configuration. type MaxFriends: Get; /// The base amount of currency needed to reserve for starting a recovery. @@ -66,23 +70,26 @@ pub trait Trait: frame_system::Trait { type RecoveryDeposit: Get>; } -/// An open multisig operation. +/// An active recovery process. #[derive(Clone, Eq, PartialEq, Encode, Decode, Default, RuntimeDebug)] -pub struct RecoveryStatus { - /// The last block when someone has tried to recover the account - last: BlockNumber, - /// The amount held in reserve of the `depositor`, to be returned once this setup is closed. +pub struct ActiveRecovery { + /// The block number when the recovery process started. + created: BlockNumber, + /// The amount held in reserve of the `depositor`, + /// To be returned once this recovery process is closed. deposit: Balance, - /// The approvals achieved so far, including the depositor. Always sorted. + /// The friends which have vouched so far. Always sorted. friends: Vec, } -/// An open multisig operation. +/// Configuration for recovering an account. #[derive(Clone, Eq, PartialEq, Encode, Decode, Default, RuntimeDebug)] -pub struct RecoverySetup { - /// The minimum amount of time between friend approvals. +pub struct RecoveryConfig { + /// 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`, to be returned once this setup is closed. + /// The amount held in reserve 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. friends: Vec, @@ -92,16 +99,16 @@ pub struct RecoverySetup { decl_storage! { trait Store for Module as Utility { - /// The set of recovery setups. - pub RecoverySetups get(fn recovery_setup): - map T::AccountId => Option, T::AccountId>>; + /// The set of recoverable accounts and their recovery configuration. + pub Recoverable get(fn recovery_config): + map T::AccountId => Option, T::AccountId>>; /// Active recovery attempts. /// - /// First account is the account to be recovered, and the second account is the user trying to - /// recover the account. + /// First account is the account to be recovered, and the second account + /// is the user trying to recover the account. pub ActiveRecoveries get(fn active_recovery): double_map hasher(twox_64_concat) T::AccountId, twox_64_concat(T::AccountId) => - Option, T::AccountId>>; + Option, T::AccountId>>; /// The final list of recovered accounts. /// /// Map from the recovered account to the user who can access it. @@ -140,9 +147,9 @@ decl_error! { /// Friends list must be sorted NotSorted, /// This account is not set up for recovery - NotSetup, + NotRecoverable, /// This account is already set up for recovery - AlreadySetup, + AlreadyRecoverable, /// A recovery process has already started for this account AlreadyStarted, /// A recovery process has not started for this rescuer @@ -157,6 +164,8 @@ decl_error! { Threshold, /// There are still active recovery attempts that need to be closed StillActive, + /// There was an overflow in a calculation + Overflow, } } @@ -174,20 +183,24 @@ decl_module! { } /// Allow Sudo to bypass the recovery process and set an alias account. - fn set_recovered_account(origin, rescuee: T::AccountId, rescuer: T::AccountId) { + fn set_recovered_account(origin, lost: T::AccountId, rescuer: T::AccountId) { ensure_root(origin)?; // Create the recovery storage item. - >::insert(&rescuee, &rescuer); + >::insert(&lost, &rescuer); - Self::deposit_event(RawEvent::AccountRecovered(rescuer, rescuee)); + Self::deposit_event(RawEvent::AccountRecovered(rescuer, lost)); } /// Create a recovery process for your account. - fn create_recovery(origin, friends: Vec, threshold: u16, delay_period: T::BlockNumber) { + fn create_recovery(origin, + friends: Vec, + threshold: u16, + delay_period: T::BlockNumber + ) { let who = ensure_signed(origin)?; // Check account is not already set up for recovery - ensure!(>::exists(&who), Error::::AlreadySetup); + ensure!(>::exists(&who), Error::::AlreadyRecoverable); // Check user input is valid ensure!(threshold >= 1, Error::::ZeroThreshold); ensure!(!friends.is_empty(), Error::::ZeroFriends); @@ -196,35 +209,40 @@ decl_module! { ensure!(Self::is_sorted(&friends), Error::::NotSorted); // Total deposit is base fee + number of friends * factor fee - let total_deposit = T::SetupDepositBase::get() + T::SetupDepositFactor::get() * friends.len().saturated_into(); + let friend_deposit = T::FriendDepositFactor::get() + .checked_mul(&friends.len().saturated_into()) + .ok_or(Error::::Overflow)?; + let total_deposit = T::ConfigDepositBase::get() + .checked_add(&friend_deposit) + .ok_or(Error::::Overflow)?; // Reserve the deposit T::Currency::reserve(&who, total_deposit)?; - // Create the recovery setup - let recovery_setup = RecoverySetup { + // Create the recovery configuration + let recovery_config = RecoveryConfig { delay_period, deposit: total_deposit, friends, threshold, }; - >::insert(&who, recovery_setup); + >::insert(&who, recovery_config); Self::deposit_event(RawEvent::RecoveryCreated(who)); } fn initiate_recovery(origin, account: T::AccountId) { let who = ensure_signed(origin)?; - // Check that the account has a recovery setup - ensure!(>::exists(&account), Error::::NotSetup); + // Check that the account is recoverable + ensure!(>::exists(&account), Error::::NotRecoverable); // Check that the recovery process has not already been started ensure!(!>::exists(&account, &who), Error::::AlreadyStarted); // Take recovery deposit let recovery_deposit = T::RecoveryDeposit::get(); T::Currency::reserve(&who, recovery_deposit)?; // Create an active recovery status - let recovery_status = RecoveryStatus { - last: T::BlockNumber::zero(), + let recovery_status = ActiveRecovery { + created: >::block_number(), deposit: recovery_deposit, friends: vec![], }; @@ -234,50 +252,49 @@ decl_module! { Self::deposit_event(RawEvent::RecoveryInitiated(who, account)); } - fn vouch_recovery(origin, rescuee: T::AccountId, rescuer: T::AccountId) { + fn vouch_recovery(origin, lost: T::AccountId, rescuer: T::AccountId) { let who = ensure_signed(origin)?; - // Check that there is a recovery setup for this rescuee - if let Some(recovery_setup) = Self::recovery_setup(&rescuee) { + // Check that the lost account is recoverable + if let Some(recovery_config) = Self::recovery_config(&lost) { // Check that the recovery process has been initiated for this rescuer - if let Some(mut active_recovery) = Self::active_recovery(&rescuee, &rescuer) { + if let Some(mut active_recovery) = Self::active_recovery(&lost, &rescuer) { // Make sure the voter is a friend - ensure!(Self::is_friend(&recovery_setup.friends, &who), Error::::NotFriend); - // Make sure the delay period has passed - let current_block_number = >::block_number(); - ensure!( - active_recovery.last + recovery_setup.delay_period >= current_block_number, - Error::::DelayPeriod - ); + ensure!(Self::is_friend(&recovery_config.friends, &who), Error::::NotFriend); // Either insert the vouch, or return an error that the user already vouched. match active_recovery.friends.binary_search(&who) { Ok(_pos) => Err(Error::::AlreadyVouched)?, Err(pos) => active_recovery.friends.insert(pos, who.clone()), } - // Update the last vote time - active_recovery.last = current_block_number; + // Update storage with the latest details - >::insert(&rescuee, &rescuer, active_recovery); + >::insert(&lost, &rescuer, active_recovery); - Self::deposit_event(RawEvent::RecoveryVouched(rescuer, rescuee, who)); + Self::deposit_event(RawEvent::RecoveryVouched(rescuer, lost, who)); } else { Err(Error::::NotStarted)? } } else { - Err(Error::::NotSetup)? + Err(Error::::NotRecoverable)? } } /// Allow a rescuer to claim their recovered account. fn claim_recovery(origin, account: T::AccountId) { let who = ensure_signed(origin)?; - // Check that there is a recovery setup for this rescuee - if let Some(recovery_setup) = Self::recovery_setup(&account) { + // Check that the lost account is recoverable + if let Some(recovery_config) = Self::recovery_config(&account) { // Check that the recovery process has been initiated for this rescuer if let Some(active_recovery) = Self::active_recovery(&account, &who) { + // Make sure the delay period has passed + let current_block_number = >::block_number(); + ensure!( + active_recovery.created + recovery_config.delay_period >= current_block_number, + Error::::DelayPeriod + ); // Make sure the threshold is met ensure!( - recovery_setup.threshold as usize <= active_recovery.friends.len(), + recovery_config.threshold as usize <= active_recovery.friends.len(), Error::::Threshold ); @@ -289,7 +306,7 @@ decl_module! { Err(Error::::NotStarted)? } } else { - Err(Error::::NotSetup)? + Err(Error::::NotRecoverable)? } } @@ -299,7 +316,7 @@ decl_module! { fn close_recovery(origin, rescuer: T::AccountId) { let who = ensure_signed(origin)?; if let Some(active_recovery) = >::take(&who, &rescuer) { - // Move the reserved funds from the rescuer to the rescuee account. + // Move the reserved funds from the rescuer to the rescued account. // Acts like a slashing mechanism for those who try to maliciously recover accounts. let _ = T::Currency::repatriate_reserved(&rescuer, &who, active_recovery.deposit); } else { @@ -316,9 +333,9 @@ decl_module! { // Check there are no active recoveries let mut active_recoveries = >::iter_prefix(&who); ensure!(active_recoveries.next().is_none(), Error::::StillActive); - // Check account has recovery setup - if let Some(recovery_setup) = >::take(&who) { - T::Currency::unreserve(&who, recovery_setup.deposit); + // Check account is recoverable + if let Some(recovery_config) = >::take(&who) { + T::Currency::unreserve(&who, recovery_config.deposit); Self::deposit_event(RawEvent::RecoveryRemoved(who)); } From 95b0c3a8eeef48206f6e8790abac21404ae2b778 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 5 Jan 2020 07:22:57 +0100 Subject: [PATCH 04/21] Check possible overflow --- frame/recover/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/recover/src/lib.rs b/frame/recover/src/lib.rs index 9b6a74c24f0ce..ed312865fbb71 100644 --- a/frame/recover/src/lib.rs +++ b/frame/recover/src/lib.rs @@ -288,10 +288,10 @@ decl_module! { if let Some(active_recovery) = Self::active_recovery(&account, &who) { // Make sure the delay period has passed let current_block_number = >::block_number(); - ensure!( - active_recovery.created + recovery_config.delay_period >= current_block_number, - Error::::DelayPeriod - ); + let recoverable_block_number = active_recovery.created + .checked_add(&recovery_config.delay_period) + .ok_or(Error::::Overflow)?; + ensure!(recoverable_block_number <= current_block_number, Error::::DelayPeriod); // Make sure the threshold is met ensure!( recovery_config.threshold as usize <= active_recovery.friends.len(), From f41abcf89aea3c935438c914afc2931c01d5f46d Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 5 Jan 2020 13:45:10 +0100 Subject: [PATCH 05/21] Copyright bump --- frame/recover/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/recover/src/lib.rs b/frame/recover/src/lib.rs index ed312865fbb71..c0cefe210e721 100644 --- a/frame/recover/src/lib.rs +++ b/frame/recover/src/lib.rs @@ -1,4 +1,4 @@ -// Copyright 2017-2019 Parity Technologies (UK) Ltd. +// Copyright 2017-2020 Parity Technologies (UK) Ltd. // This file is part of Substrate. // Substrate is free software: you can redistribute it and/or modify From 27e10340a9e2b496125be3e92bbee12f681b6965 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 6 Jan 2020 06:07:33 +0100 Subject: [PATCH 06/21] Add mock for tests --- Cargo.lock | 2 +- Cargo.toml | 2 +- frame/{recover => recovery}/Cargo.toml | 2 +- frame/{recover => recovery}/src/lib.rs | 9 +- frame/recovery/src/mock.rs | 140 +++++++++++++++++++++++++ frame/recovery/src/tests.rs | 35 +++++++ 6 files changed, 185 insertions(+), 5 deletions(-) rename frame/{recover => recovery}/Cargo.toml (97%) rename frame/{recover => recovery}/src/lib.rs (98%) create mode 100644 frame/recovery/src/mock.rs create mode 100644 frame/recovery/src/tests.rs diff --git a/Cargo.lock b/Cargo.lock index c0e656cf83650..c6584bfa2fee2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3855,7 +3855,7 @@ dependencies = [ ] [[package]] -name = "pallet-recover" +name = "pallet-recovery" version = "2.0.0" dependencies = [ "enumflags2 0.6.2 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/Cargo.toml b/Cargo.toml index 707cfa75b3589..07c86c5e9cbb2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -78,7 +78,7 @@ members = [ "frame/nicks", "frame/offences", "frame/randomness-collective-flip", - "frame/recover", + "frame/recovery", "frame/scored-pool", "frame/session", "frame/staking", diff --git a/frame/recover/Cargo.toml b/frame/recovery/Cargo.toml similarity index 97% rename from frame/recover/Cargo.toml rename to frame/recovery/Cargo.toml index 925a464b87acc..9f2f50ab06f84 100644 --- a/frame/recover/Cargo.toml +++ b/frame/recovery/Cargo.toml @@ -1,5 +1,5 @@ [package] -name = "pallet-recover" +name = "pallet-recovery" version = "2.0.0" authors = ["Parity Technologies "] edition = "2018" diff --git a/frame/recover/src/lib.rs b/frame/recovery/src/lib.rs similarity index 98% rename from frame/recover/src/lib.rs rename to frame/recovery/src/lib.rs index c0cefe210e721..590a4ae622d3f 100644 --- a/frame/recover/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -1,4 +1,4 @@ -// Copyright 2017-2020 Parity Technologies (UK) Ltd. +// Copyright 2020 Parity Technologies (UK) Ltd. // This file is part of Substrate. // Substrate is free software: you can redistribute it and/or modify @@ -35,6 +35,11 @@ use frame_support::{ }; use frame_system::{self as system, ensure_signed, ensure_root}; +#[cfg(test)] +mod mock; +#[cfg(test)] +mod tests; + type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; @@ -98,7 +103,7 @@ pub struct RecoveryConfig { } decl_storage! { - trait Store for Module as Utility { + trait Store for Module as Recovery { /// The set of recoverable accounts and their recovery configuration. pub Recoverable get(fn recovery_config): map T::AccountId => Option, T::AccountId>>; diff --git a/frame/recovery/src/mock.rs b/frame/recovery/src/mock.rs new file mode 100644 index 0000000000000..0dc9bbd33afc2 --- /dev/null +++ b/frame/recovery/src/mock.rs @@ -0,0 +1,140 @@ +// Copyright 2020 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +//! Test utilities + +use super::*; + +use frame_support::{impl_outer_origin, impl_outer_dispatch, impl_outer_event, parameter_types}; +use sp_core::H256; +// The testing primitives are very useful for avoiding having to work with signatures +// or public keys. `u64` is used as the `AccountId` and no `Signature`s are required. +use sp_runtime::{ + Perbill, traits::{BlakeTwo256, IdentityLookup, OnInitialize, OnFinalize}, testing::Header, +}; +use frame_system::EnsureSignedBy; +use crate as recovery; + +impl_outer_origin! { + pub enum Origin for Test where system = frame_system {} +} + +impl_outer_event! { + pub enum TestEvent for Test { + pallet_balances, + recovery, + } +} +impl_outer_dispatch! { + pub enum Call for Test where origin: Origin { + pallet_balances::Balances, + recovery::Recovery, + } +} + +// For testing the module, we construct most of a mock runtime. This means +// first constructing a configuration type (`Test`) which `impl`s each of the +// configuration traits of modules we want to use. +#[derive(Clone, Eq, PartialEq)] +pub struct Test; + +parameter_types! { + pub const BlockHashCount: u64 = 250; + pub const MaximumBlockWeight: Weight = 1024; + pub const MaximumBlockLength: u32 = 2 * 1024; + pub const AvailableBlockRatio: Perbill = Perbill::one(); +} + +impl frame_system::Trait for Test { + type Origin = Origin; + type Index = u64; + type BlockNumber = u64; + type Hash = H256; + type Call = Call; + type Hashing = BlakeTwo256; + type AccountId = u64; + type Lookup = IdentityLookup; + type Header = Header; + type Event = TestEvent; + type BlockHashCount = BlockHashCount; + type MaximumBlockWeight = MaximumBlockWeight; + type MaximumBlockLength = MaximumBlockLength; + type AvailableBlockRatio = AvailableBlockRatio; + type Version = (); + type ModuleToIndex = (); +} + +parameter_types! { + pub const ExistentialDeposit: u64 = 0; + pub const TransferFee: u64 = 0; + pub const CreationFee: u64 = 0; +} + +impl pallet_balances::Trait for Test { + type Balance = u128; + type OnFreeBalanceZero = (); + type OnNewAccount = (); + type Event = TestEvent; + type TransferPayment = (); + type DustRemoval = (); + type ExistentialDeposit = ExistentialDeposit; + type TransferFee = TransferFee; + type CreationFee = CreationFee; +} + +parameter_types! { + pub const ConfigDepositBase: u64 = 10; + pub const FriendDepositFactor: u64 = 1; + pub const MaxFriends: u16 = 3; + pub const RecoveryDeposit: u64 = 10; +} + +impl Trait for Test { + type Event = TestEvent; + type Call = Call; + type Currency = Balances; + type ConfigDepositBase = ConfigDepositBase; + type FriendDepositFactor = FriendDepositFactor; + type MaxFriends = MaxFriends; + type RecoveryDeposit = RecoveryDeposit; +} + +use pallet_balances::Call as BalancesCall; +use pallet_balances::Error as BalancesError; + +pub type Recovery = Module; +pub type System = frame_system::Module; +pub type Balances = pallet_balances::Module; + +pub fn new_test_ext() -> sp_io::TestExternalities { + let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); + pallet_balances::GenesisConfig:: { + balances: vec![(1, 100), (2, 100), (3, 100), (4, 100), (5, 100)], + vesting: vec![], + }.assimilate_storage(&mut t).unwrap(); + t.into() +} + +/// Run until a particular block. +pub fn run_to_block(n: u64) { + while System::block_number() < n { + if System::block_number() > 1 { + System::on_finalize(System::block_number()); + } + System::set_block_number(System::block_number() + 1); + System::on_initialize(System::block_number()); + } +} diff --git a/frame/recovery/src/tests.rs b/frame/recovery/src/tests.rs new file mode 100644 index 0000000000000..5879ed9a2cd47 --- /dev/null +++ b/frame/recovery/src/tests.rs @@ -0,0 +1,35 @@ +// Copyright 2020 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +//! Tests for the module. + +use super::*; +use mock::{Recovery, Balances, Test, System, new_test_ext, run_to_block}; +use sp_runtime::traits::{SignedExtension, BadOrigin}; +use frame_support::{ + assert_noop, assert_ok, assert_err, + traits::{LockableCurrency, LockIdentifier, WithdrawReason, WithdrawReasons, + Currency, ReservableCurrency, ExistenceRequirement::AllowDeath} +}; + +#[test] +fn basic_setup_works() { + new_test_ext().execute_with(|| { + assert_eq!(Recovery::recovered_account(&1), None); + assert_eq!(Recovery::active_recovery(&1, &2), None); + assert_eq!(Recovery::recovery_config(&1), None); + }); +} \ No newline at end of file From 58aff4049eaafa8a5189e5d2abf7a58540e0b39b Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 6 Jan 2020 06:54:59 +0100 Subject: [PATCH 07/21] Add basic end to end test --- frame/recovery/src/lib.rs | 4 +-- frame/recovery/src/mock.rs | 8 +++-- frame/recovery/src/tests.rs | 64 +++++++++++++++++++++++++++++++++++-- 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 590a4ae622d3f..6833c1e4bd859 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -205,12 +205,12 @@ decl_module! { ) { let who = ensure_signed(origin)?; // Check account is not already set up for recovery - ensure!(>::exists(&who), Error::::AlreadyRecoverable); + ensure!(!>::exists(&who), Error::::AlreadyRecoverable); // Check user input is valid ensure!(threshold >= 1, Error::::ZeroThreshold); ensure!(!friends.is_empty(), Error::::ZeroFriends); let max_friends = T::MaxFriends::get() as usize; - ensure!(friends.len() < max_friends, Error::::MaxFriends); + ensure!(friends.len() <= max_friends, Error::::MaxFriends); ensure!(Self::is_sorted(&friends), Error::::NotSorted); // Total deposit is base fee + number of friends * factor fee diff --git a/frame/recovery/src/mock.rs b/frame/recovery/src/mock.rs index 0dc9bbd33afc2..801e70feee425 100644 --- a/frame/recovery/src/mock.rs +++ b/frame/recovery/src/mock.rs @@ -112,13 +112,15 @@ impl Trait for Test { type RecoveryDeposit = RecoveryDeposit; } -use pallet_balances::Call as BalancesCall; -use pallet_balances::Error as BalancesError; - pub type Recovery = Module; pub type System = frame_system::Module; pub type Balances = pallet_balances::Module; +pub type BalancesCall = pallet_balances::Call; +pub type BalancesError = pallet_balances::Error; + +pub type RecoveryCall = super::Call; + pub fn new_test_ext() -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); pallet_balances::GenesisConfig:: { diff --git a/frame/recovery/src/tests.rs b/frame/recovery/src/tests.rs index 5879ed9a2cd47..bba240b2ecd64 100644 --- a/frame/recovery/src/tests.rs +++ b/frame/recovery/src/tests.rs @@ -17,7 +17,10 @@ //! Tests for the module. use super::*; -use mock::{Recovery, Balances, Test, System, new_test_ext, run_to_block}; +use mock::{ + Recovery, Balances, Test, System, Origin, Call, BalancesCall, RecoveryCall, + new_test_ext, run_to_block +}; use sp_runtime::traits::{SignedExtension, BadOrigin}; use frame_support::{ assert_noop, assert_ok, assert_err, @@ -28,8 +31,65 @@ use frame_support::{ #[test] fn basic_setup_works() { new_test_ext().execute_with(|| { + // Nothing in storage to start assert_eq!(Recovery::recovered_account(&1), None); assert_eq!(Recovery::active_recovery(&1, &2), None); assert_eq!(Recovery::recovery_config(&1), None); + // Everyone should have starting balance of 100 + assert_eq!(Balances::free_balance(&1), 100); + }); +} + +#[test] +fn set_recovered_account_works() { + new_test_ext().execute_with(|| { + // Not accessible by a normal user + assert_noop!(Recovery::set_recovered_account(Origin::signed(1), 5, 1), DispatchError::BadOrigin); + // Root can set a recovered account though + assert_ok!(Recovery::set_recovered_account(Origin::ROOT, 5, 1)); + // Account 1 should now be able to make a call through account 5 + let call = Box::new(Call::Balances(BalancesCall::transfer(1, 100))); + assert_ok!(Recovery::as_recovered(Origin::signed(1), 5, call)); + // Account 1 has successfully drained the funds from account 5 + assert_eq!(Balances::free_balance(1), 200); + assert_eq!(Balances::free_balance(5), 0); + }); +} + +#[test] +fn recovery_lifecycle_works() { + new_test_ext().execute_with(|| { + let friends = vec![2, 3, 4]; + let threshold = 2; + let delay_period = 10; + // Account 5 sets up a recovery configuration on their account + assert_ok!(Recovery::create_recovery(Origin::signed(5), friends, threshold, delay_period)); + // Some time has passed, and the user lost their keys! + run_to_block(10); + // Using account 1, the user begins the recovery process to recover the lost account + assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); + // Off chain, the user contacts their friends and asks them to vouch for the recovery attempt + assert_ok!(Recovery::vouch_recovery(Origin::signed(2), 5, 1)); + assert_ok!(Recovery::vouch_recovery(Origin::signed(3), 5, 1)); + // We met the threshold, lets try to recover the account...? + assert_noop!(Recovery::claim_recovery(Origin::signed(1), 5), Error::::DelayPeriod); + // We need to wait at least the delay_period number of blocks before we can recover + run_to_block(20); + assert_ok!(Recovery::claim_recovery(Origin::signed(1), 5)); + // Account 1 can use account 5 to close the active recovery process, claiming the deposited + // funds used to initiate the recovery process into account 5. + let call = Box::new(Call::Recovery(RecoveryCall::close_recovery(1))); + assert_ok!(Recovery::as_recovered(Origin::signed(1), 5, call)); + // Account 1 can then use account 5 to close the recovery configuration, claiming the + // deposited funds used to create the recovery configuration into account 5. + let call = Box::new(Call::Recovery(RecoveryCall::remove_recovery())); + assert_ok!(Recovery::as_recovered(Origin::signed(1), 5, call)); + // Account 1 should now be able to make a call through account 5 to get all of their funds + assert_eq!(Balances::free_balance(5), 110); + let call = Box::new(Call::Balances(BalancesCall::transfer(1, 110))); + assert_ok!(Recovery::as_recovered(Origin::signed(1), 5, call)); + // All funds have been fully recovered! + assert_eq!(Balances::free_balance(1), 200); + assert_eq!(Balances::free_balance(5), 0); }); -} \ No newline at end of file +} From 8e7b7f235892bc74a38ede3708b787253d2e989c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 6 Jan 2020 07:23:35 +0100 Subject: [PATCH 08/21] Add `create_recovery` tests --- frame/recovery/src/lib.rs | 11 +++++----- frame/recovery/src/tests.rs | 43 ++++++++++++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 6833c1e4bd859..ccd56db0ec824 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -145,11 +145,11 @@ decl_error! { NotAllowed, /// Threshold must be greater than zero ZeroThreshold, - /// Friends list must be greater than zero - ZeroFriends, + /// Friends list must be greater than zero and threshold + NotEnoughFriends, /// Friends list must be less than max friends MaxFriends, - /// Friends list must be sorted + /// Friends list must be sorted and free of duplicates NotSorted, /// This account is not set up for recovery NotRecoverable, @@ -208,7 +208,8 @@ decl_module! { ensure!(!>::exists(&who), Error::::AlreadyRecoverable); // Check user input is valid ensure!(threshold >= 1, Error::::ZeroThreshold); - ensure!(!friends.is_empty(), Error::::ZeroFriends); + ensure!(!friends.is_empty(), Error::::NotEnoughFriends); + ensure!(threshold as usize <= friends.len(), Error::::NotEnoughFriends); let max_friends = T::MaxFriends::get() as usize; ensure!(friends.len() <= max_friends, Error::::MaxFriends); ensure!(Self::is_sorted(&friends), Error::::NotSorted); @@ -351,7 +352,7 @@ decl_module! { impl Module { /// Check that friends list is sorted. fn is_sorted(friends: &Vec) -> bool { - friends.windows(2).all(|w| w[0] <= w[1]) + friends.windows(2).all(|w| w[0] < w[1]) } /// Check that a user is a friend in the friends list. diff --git a/frame/recovery/src/tests.rs b/frame/recovery/src/tests.rs index bba240b2ecd64..f53ad51bb693b 100644 --- a/frame/recovery/src/tests.rs +++ b/frame/recovery/src/tests.rs @@ -60,7 +60,7 @@ fn set_recovered_account_works() { fn recovery_lifecycle_works() { new_test_ext().execute_with(|| { let friends = vec![2, 3, 4]; - let threshold = 2; + let threshold = 3; let delay_period = 10; // Account 5 sets up a recovery configuration on their account assert_ok!(Recovery::create_recovery(Origin::signed(5), friends, threshold, delay_period)); @@ -71,6 +71,7 @@ fn recovery_lifecycle_works() { // Off chain, the user contacts their friends and asks them to vouch for the recovery attempt assert_ok!(Recovery::vouch_recovery(Origin::signed(2), 5, 1)); assert_ok!(Recovery::vouch_recovery(Origin::signed(3), 5, 1)); + assert_ok!(Recovery::vouch_recovery(Origin::signed(4), 5, 1)); // We met the threshold, lets try to recover the account...? assert_noop!(Recovery::claim_recovery(Origin::signed(1), 5), Error::::DelayPeriod); // We need to wait at least the delay_period number of blocks before we can recover @@ -93,3 +94,43 @@ fn recovery_lifecycle_works() { assert_eq!(Balances::free_balance(5), 0); }); } + +#[test] +fn create_recovery_handles_basic_errors() { + new_test_ext().execute_with(|| { + // No friends + assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![], 1, 0), Error::::NotEnoughFriends); + // Zero threshold + assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![2], 0, 0), Error::::ZeroThreshold); + // Threshold greater than friends length + assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![2, 3, 4], 4, 0), Error::::NotEnoughFriends); + // Too many friends + assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![1, 2, 3, 4], 4, 0), Error::::MaxFriends); + // Unsorted friends + assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![3, 2, 4], 3, 0), Error::::NotSorted); + // No duplicate friends + assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![2, 2, 4], 3, 0), Error::::NotSorted); + }); +} + +#[test] +fn create_recovery_works() { + new_test_ext().execute_with(|| { + let friends = vec![2, 3, 4]; + let threshold = 3; + let delay_period = 10; + // Account 5 sets up a recovery configuration on their account + assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); + // Deposit is taken, and scales with the number of friends they pick + // Base 10 + 1 per friends = 13 total reserved + assert_eq!(Balances::reserved_balance(5), 13); + // Recovery configuration is correctly stored + let recovery_config = RecoveryConfig { + delay_period, + deposit: 13, + friends: friends.clone(), + threshold, + }; + assert_eq!(Recovery::recovery_config(5), Some(recovery_config)); + }); +} From 49fb230671dec93526026441e2968ddbe2205166 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 6 Jan 2020 07:44:27 +0100 Subject: [PATCH 09/21] Add malicious recovery lifecycle test --- frame/recovery/src/lib.rs | 2 + frame/recovery/src/tests.rs | 78 +++++++++++++++++++++++++++++++++---- 2 files changed, 73 insertions(+), 7 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index ccd56db0ec824..319abc23f64a8 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -237,6 +237,7 @@ decl_module! { Self::deposit_event(RawEvent::RecoveryCreated(who)); } + /// Allow a user to start the process for recovering an account. fn initiate_recovery(origin, account: T::AccountId) { let who = ensure_signed(origin)?; // Check that the account is recoverable @@ -258,6 +259,7 @@ decl_module! { Self::deposit_event(RawEvent::RecoveryInitiated(who, account)); } + /// Allow a friend to vouch for an active recovery process. fn vouch_recovery(origin, lost: T::AccountId, rescuer: T::AccountId) { let who = ensure_signed(origin)?; diff --git a/frame/recovery/src/tests.rs b/frame/recovery/src/tests.rs index f53ad51bb693b..a1c303df496c2 100644 --- a/frame/recovery/src/tests.rs +++ b/frame/recovery/src/tests.rs @@ -95,21 +95,85 @@ fn recovery_lifecycle_works() { }); } +#[test] +fn malicious_recovery_fails() { + new_test_ext().execute_with(|| { + let friends = vec![2, 3, 4]; + let threshold = 3; + let delay_period = 10; + // Account 5 sets up a recovery configuration on their account + assert_ok!(Recovery::create_recovery(Origin::signed(5), friends, threshold, delay_period)); + // Some time has passed, and account 1 wants to try and attack this account! + run_to_block(10); + // Using account 1, the malicious user begins the recovery process on account 5 + assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); + // Off chain, the user **tricks** their friends and asks them to vouch for the recovery + assert_ok!(Recovery::vouch_recovery(Origin::signed(2), 5, 1)); // shame on you + assert_ok!(Recovery::vouch_recovery(Origin::signed(3), 5, 1)); // shame on you + assert_ok!(Recovery::vouch_recovery(Origin::signed(4), 5, 1)); // shame on you + // We met the threshold, lets try to recover the account...? + assert_noop!(Recovery::claim_recovery(Origin::signed(1), 5), Error::::DelayPeriod); + // Account 1 needs to wait... + run_to_block(19); + // One more block to wait! + assert_noop!(Recovery::claim_recovery(Origin::signed(1), 5), Error::::DelayPeriod); + // Account 5 checks their account every `delay_period` and notices the malicious attack! + // Account 5 can close the recovery process before account 1 can claim it + assert_ok!(Recovery::close_recovery(Origin::signed(5), 1)); + // By doing so, account 5 has now claimed the deposit originally reserved by account 1 + assert_eq!(Balances::total_balance(&1), 90); + // Thanks for the free money! + assert_eq!(Balances::total_balance(&5), 110); + // The recovery process has been closed, so account 1 can't make the claim + run_to_block(20); + assert_noop!(Recovery::claim_recovery(Origin::signed(1), 5), Error::::NotStarted); + // Account 5 can remove their recovery config and pick some better friends + assert_ok!(Recovery::remove_recovery(Origin::signed(5))); + assert_ok!(Recovery::create_recovery(Origin::signed(5), vec![22, 33, 44], threshold, delay_period)); + }); +} + #[test] fn create_recovery_handles_basic_errors() { new_test_ext().execute_with(|| { // No friends - assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![], 1, 0), Error::::NotEnoughFriends); + assert_noop!( + Recovery::create_recovery(Origin::signed(5), vec![], 1, 0), + Error::::NotEnoughFriends + ); // Zero threshold - assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![2], 0, 0), Error::::ZeroThreshold); + assert_noop!( + Recovery::create_recovery(Origin::signed(5), vec![2], 0, 0), + Error::::ZeroThreshold + ); // Threshold greater than friends length - assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![2, 3, 4], 4, 0), Error::::NotEnoughFriends); + assert_noop!( + Recovery::create_recovery(Origin::signed(5), vec![2, 3, 4], 4, 0), + Error::::NotEnoughFriends + ); // Too many friends - assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![1, 2, 3, 4], 4, 0), Error::::MaxFriends); + assert_noop!( + Recovery::create_recovery(Origin::signed(5), vec![1, 2, 3, 4], 4, 0), + Error::::MaxFriends + ); // Unsorted friends - assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![3, 2, 4], 3, 0), Error::::NotSorted); - // No duplicate friends - assert_noop!(Recovery::create_recovery(Origin::signed(5), vec![2, 2, 4], 3, 0), Error::::NotSorted); + assert_noop!( + Recovery::create_recovery(Origin::signed(5), vec![3, 2, 4], 3, 0), + Error::::NotSorted + ); + // Duplicate friends + assert_noop!( + Recovery::create_recovery(Origin::signed(5), vec![2, 2, 4], 3, 0), + Error::::NotSorted + ); + // Already configured + assert_ok!( + Recovery::create_recovery(Origin::signed(5), vec![2, 3, 4], 3, 10) + ); + assert_noop!( + Recovery::create_recovery(Origin::signed(5), vec![2, 3, 4], 3, 10), + Error::::AlreadyRecoverable + ); }); } From b68f1fdcbfb08c1a98867f120e4e84cd13c49ee1 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 6 Jan 2020 07:56:16 +0100 Subject: [PATCH 10/21] Make clear we check for sorted and unique friends --- frame/recovery/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 319abc23f64a8..62be24ac4a4cb 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -212,7 +212,7 @@ decl_module! { ensure!(threshold as usize <= friends.len(), Error::::NotEnoughFriends); let max_friends = T::MaxFriends::get() as usize; ensure!(friends.len() <= max_friends, Error::::MaxFriends); - ensure!(Self::is_sorted(&friends), Error::::NotSorted); + ensure!(Self::is_sorted_and_unique(&friends), Error::::NotSorted); // Total deposit is base fee + number of friends * factor fee let friend_deposit = T::FriendDepositFactor::get() @@ -352,8 +352,8 @@ decl_module! { } impl Module { - /// Check that friends list is sorted. - fn is_sorted(friends: &Vec) -> bool { + /// Check that friends list is sorted and has no duplicates. + fn is_sorted_and_unique(friends: &Vec) -> bool { friends.windows(2).all(|w| w[0] < w[1]) } From 087bc6c49f7aa943c350bd5ffa5bb3d7fe1b2cd0 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 9 Jan 2020 16:23:58 +0100 Subject: [PATCH 11/21] Work on some tests, clean up imports --- frame/recovery/src/lib.rs | 9 ++--- frame/recovery/src/mock.rs | 8 ++-- frame/recovery/src/tests.rs | 80 +++++++++++++++++++++++++++++++++---- 3 files changed, 80 insertions(+), 17 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 62be24ac4a4cb..a75af82ef5dbf 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -19,8 +19,8 @@ use sp_std::prelude::*; use sp_runtime::{ - traits::{StaticLookup, Dispatchable, SaturatedConversion, Zero, CheckedAdd, CheckedMul}, - DispatchError, DispatchResult + traits::{Dispatchable, SaturatedConversion, CheckedAdd, CheckedMul}, + DispatchResult }; use codec::{Encode, Decode}; @@ -28,8 +28,7 @@ use frame_support::{ decl_module, decl_event, decl_storage, decl_error, ensure, Parameter, RuntimeDebug, weights::{ - SimpleDispatchInfo, GetDispatchInfo, PaysFee, WeighData, Weight, - ClassifyDispatch, DispatchClass + GetDispatchInfo, }, traits::{Currency, ReservableCurrency, Get}, }; @@ -188,7 +187,7 @@ decl_module! { } /// Allow Sudo to bypass the recovery process and set an alias account. - fn set_recovered_account(origin, lost: T::AccountId, rescuer: T::AccountId) { + fn set_recovered(origin, lost: T::AccountId, rescuer: T::AccountId) { ensure_root(origin)?; // Create the recovery storage item. diff --git a/frame/recovery/src/mock.rs b/frame/recovery/src/mock.rs index 801e70feee425..6607be442ce48 100644 --- a/frame/recovery/src/mock.rs +++ b/frame/recovery/src/mock.rs @@ -18,14 +18,16 @@ use super::*; -use frame_support::{impl_outer_origin, impl_outer_dispatch, impl_outer_event, parameter_types}; +use frame_support::{ + impl_outer_origin, impl_outer_dispatch, impl_outer_event, parameter_types, + weights::Weight, +}; use sp_core::H256; // The testing primitives are very useful for avoiding having to work with signatures // or public keys. `u64` is used as the `AccountId` and no `Signature`s are required. use sp_runtime::{ Perbill, traits::{BlakeTwo256, IdentityLookup, OnInitialize, OnFinalize}, testing::Header, }; -use frame_system::EnsureSignedBy; use crate as recovery; impl_outer_origin! { @@ -117,8 +119,6 @@ pub type System = frame_system::Module; pub type Balances = pallet_balances::Module; pub type BalancesCall = pallet_balances::Call; -pub type BalancesError = pallet_balances::Error; - pub type RecoveryCall = super::Call; pub fn new_test_ext() -> sp_io::TestExternalities { diff --git a/frame/recovery/src/tests.rs b/frame/recovery/src/tests.rs index a1c303df496c2..dc1624be30dd8 100644 --- a/frame/recovery/src/tests.rs +++ b/frame/recovery/src/tests.rs @@ -18,14 +18,13 @@ use super::*; use mock::{ - Recovery, Balances, Test, System, Origin, Call, BalancesCall, RecoveryCall, + Recovery, Balances, Test, Origin, Call, BalancesCall, RecoveryCall, new_test_ext, run_to_block }; -use sp_runtime::traits::{SignedExtension, BadOrigin}; +use sp_runtime::traits::{BadOrigin}; use frame_support::{ - assert_noop, assert_ok, assert_err, - traits::{LockableCurrency, LockIdentifier, WithdrawReason, WithdrawReasons, - Currency, ReservableCurrency, ExistenceRequirement::AllowDeath} + assert_noop, assert_ok, + traits::{Currency}, }; #[test] @@ -41,12 +40,12 @@ fn basic_setup_works() { } #[test] -fn set_recovered_account_works() { +fn set_recovered_works() { new_test_ext().execute_with(|| { // Not accessible by a normal user - assert_noop!(Recovery::set_recovered_account(Origin::signed(1), 5, 1), DispatchError::BadOrigin); + assert_noop!(Recovery::set_recovered(Origin::signed(1), 5, 1), BadOrigin); // Root can set a recovered account though - assert_ok!(Recovery::set_recovered_account(Origin::ROOT, 5, 1)); + assert_ok!(Recovery::set_recovered(Origin::ROOT, 5, 1)); // Account 1 should now be able to make a call through account 5 let call = Box::new(Call::Balances(BalancesCall::transfer(1, 100))); assert_ok!(Recovery::as_recovered(Origin::signed(1), 5, call)); @@ -198,3 +197,68 @@ fn create_recovery_works() { assert_eq!(Recovery::recovery_config(5), Some(recovery_config)); }); } + +#[test] +fn initiate_recovery_handles_basic_errors() { + new_test_ext().execute_with(|| { + // No recovery process set up for the account + assert_noop!( + Recovery::initiate_recovery(Origin::signed(1), 5), + Error::::NotRecoverable + ); + // Create a recovery process for next test + let friends = vec![2, 3, 4]; + let threshold = 3; + let delay_period = 10; + assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); + + // Same user cannot recover same account twice + assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); + assert_noop!(Recovery::initiate_recovery(Origin::signed(1), 5), Error::::AlreadyStarted); + + // No double deposit + assert_eq!(Balances::reserved_balance(&1), 10); + }); +} + +#[test] +fn initiate_recovery_works() { + new_test_ext().execute_with(|| { + // Create a recovery process for the test + let friends = vec![2, 3, 4]; + let threshold = 3; + let delay_period = 10; + assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); + + // Recovery can be initiated + assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); + // Deposit is reserved + assert_eq!(Balances::reserved_balance(&1), 10); + // Recovery status object is created correctly + let recovery_status = ActiveRecovery { + created: 1, + deposit: 10, + friends: vec![], + }; + assert_eq!(>::get(&5, &1), Some(recovery_status)); + + // Multiple users can attempt to recover the same account + assert_ok!(Recovery::initiate_recovery(Origin::signed(2), 5)); + }); +} + +#[test] +fn vouch_recovery_handles_basic_errors() { + new_test_ext().execute_with(|| { + // Cannot vouch for non-recoverable account + + + // Create and initiate a recovery process for next tests + let friends = vec![2, 3, 4]; + let threshold = 3; + let delay_period = 10; + assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); + assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); + + }); +} From 8bb69ee384d8f1f4287c74070551cbefbf4ea28c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 9 Jan 2020 16:50:06 +0100 Subject: [PATCH 12/21] Change `if let Some(_)` to `ok_or()` --- frame/recovery/src/lib.rs | 119 +++++++++++++++++--------------------- 1 file changed, 54 insertions(+), 65 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index a75af82ef5dbf..4f906b7aad708 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -127,11 +127,13 @@ decl_event! { { /// A recovery process has been set up for an account RecoveryCreated(AccountId), - /// A recovery process has been initiated by account_1 for account_2 + /// A recovery process has been initiated for account_1 by account_2 RecoveryInitiated(AccountId, AccountId), - /// A recovery process by account_1 for account_2 has been vouched for by account_3 + /// A recovery process for account_1 by account_2 has been vouched for by account_3 RecoveryVouched(AccountId, AccountId, AccountId), - /// Account_1 has recovered account_2 + /// A recovery process for account_1 by account_2 has been closed + RecoveryClosed(AccountId, AccountId), + /// Account_1 has been recovered by account_2 AccountRecovered(AccountId, AccountId), /// A recovery process has been removed for an account RecoveryRemoved(AccountId), @@ -193,7 +195,7 @@ decl_module! { // Create the recovery storage item. >::insert(&lost, &rescuer); - Self::deposit_event(RawEvent::AccountRecovered(rescuer, lost)); + Self::deposit_event(RawEvent::AccountRecovered(lost, rescuer)); } /// Create a recovery process for your account. @@ -255,66 +257,55 @@ decl_module! { >::insert(&account, &who, recovery_status); - Self::deposit_event(RawEvent::RecoveryInitiated(who, account)); + Self::deposit_event(RawEvent::RecoveryInitiated(account, who)); } /// Allow a friend to vouch for an active recovery process. fn vouch_recovery(origin, lost: T::AccountId, rescuer: T::AccountId) { let who = ensure_signed(origin)?; - // Check that the lost account is recoverable - if let Some(recovery_config) = Self::recovery_config(&lost) { - // Check that the recovery process has been initiated for this rescuer - if let Some(mut active_recovery) = Self::active_recovery(&lost, &rescuer) { - // Make sure the voter is a friend - ensure!(Self::is_friend(&recovery_config.friends, &who), Error::::NotFriend); - // Either insert the vouch, or return an error that the user already vouched. - match active_recovery.friends.binary_search(&who) { - Ok(_pos) => Err(Error::::AlreadyVouched)?, - Err(pos) => active_recovery.friends.insert(pos, who.clone()), - } - - // Update storage with the latest details - >::insert(&lost, &rescuer, active_recovery); - - Self::deposit_event(RawEvent::RecoveryVouched(rescuer, lost, who)); - } else { - Err(Error::::NotStarted)? - } - } else { - Err(Error::::NotRecoverable)? + // Get the recovery configuration for the lost account. + let recovery_config = Self::recovery_config(&lost).ok_or(Error::::NotRecoverable)?; + // Get the active recovery process for the rescuer. + let mut active_recovery = Self::active_recovery(&lost, &rescuer).ok_or(Error::::NotStarted)?; + // Make sure the voter is a friend + ensure!(Self::is_friend(&recovery_config.friends, &who), Error::::NotFriend); + + // Either insert the vouch, or return an error that the user already vouched. + match active_recovery.friends.binary_search(&who) { + Ok(_pos) => Err(Error::::AlreadyVouched)?, + Err(pos) => active_recovery.friends.insert(pos, who.clone()), } + + // Update storage with the latest details + >::insert(&lost, &rescuer, active_recovery); + + Self::deposit_event(RawEvent::RecoveryVouched(lost, rescuer, who)); } /// Allow a rescuer to claim their recovered account. fn claim_recovery(origin, account: T::AccountId) { let who = ensure_signed(origin)?; - // Check that the lost account is recoverable - if let Some(recovery_config) = Self::recovery_config(&account) { - // Check that the recovery process has been initiated for this rescuer - if let Some(active_recovery) = Self::active_recovery(&account, &who) { - // Make sure the delay period has passed - let current_block_number = >::block_number(); - let recoverable_block_number = active_recovery.created - .checked_add(&recovery_config.delay_period) - .ok_or(Error::::Overflow)?; - ensure!(recoverable_block_number <= current_block_number, Error::::DelayPeriod); - // Make sure the threshold is met - ensure!( - recovery_config.threshold as usize <= active_recovery.friends.len(), - Error::::Threshold - ); - - // Create the recovery storage item - >::insert(&account, &who); - - Self::deposit_event(RawEvent::AccountRecovered(who, account)); - } else { - Err(Error::::NotStarted)? - } - } else { - Err(Error::::NotRecoverable)? - } + // Get the recovery configuration for the lost account + let recovery_config = Self::recovery_config(&account).ok_or(Error::::NotRecoverable)?; + // Get the active recovery process for the rescuer + let active_recovery = Self::active_recovery(&account, &who).ok_or(Error::::NotStarted)?; + // Make sure the delay period has passed + let current_block_number = >::block_number(); + let recoverable_block_number = active_recovery.created + .checked_add(&recovery_config.delay_period) + .ok_or(Error::::Overflow)?; + ensure!(recoverable_block_number <= current_block_number, Error::::DelayPeriod); + // Make sure the threshold is met + ensure!( + recovery_config.threshold as usize <= active_recovery.friends.len(), + Error::::Threshold + ); + + // Create the recovery storage item + >::insert(&account, &who); + + Self::deposit_event(RawEvent::AccountRecovered(account, who)); } /// Close an active recovery process. @@ -322,13 +313,12 @@ decl_module! { /// Can only be called by the account trying to be recovered. fn close_recovery(origin, rescuer: T::AccountId) { let who = ensure_signed(origin)?; - if let Some(active_recovery) = >::take(&who, &rescuer) { - // Move the reserved funds from the rescuer to the rescued account. - // Acts like a slashing mechanism for those who try to maliciously recover accounts. - let _ = T::Currency::repatriate_reserved(&rescuer, &who, active_recovery.deposit); - } else { - Err(Error::::NotStarted)? - } + // Take the active recovery process by the rescuer for this account. + let active_recovery = >::take(&who, &rescuer).ok_or(Error::::NotStarted)?; + // Move the reserved funds from the rescuer to the rescued account. + // Acts like a slashing mechanism for those who try to maliciously recover accounts. + let _ = T::Currency::repatriate_reserved(&rescuer, &who, active_recovery.deposit); + Self::deposit_event(RawEvent::RecoveryClosed(who, rescuer)); } /// Remove the recovery process for your account. @@ -340,12 +330,11 @@ decl_module! { // Check there are no active recoveries let mut active_recoveries = >::iter_prefix(&who); ensure!(active_recoveries.next().is_none(), Error::::StillActive); - // Check account is recoverable - if let Some(recovery_config) = >::take(&who) { - T::Currency::unreserve(&who, recovery_config.deposit); - - Self::deposit_event(RawEvent::RecoveryRemoved(who)); - } + // Take the recovery configuration for this account. + let recovery_config = >::take(&who).ok_or(Error::::NotRecoverable)?; + // Unreserve the initial deposit for the recovery configuration. + T::Currency::unreserve(&who, recovery_config.deposit); + Self::deposit_event(RawEvent::RecoveryRemoved(who)); } } } From 48291828c1ee86cde822e8ccac05b9c9c2eb05d8 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 9 Jan 2020 18:40:16 +0100 Subject: [PATCH 13/21] More tests --- frame/recovery/src/lib.rs | 2 +- frame/recovery/src/tests.rs | 103 +++++++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 4f906b7aad708..28910d20c1011 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -133,7 +133,7 @@ decl_event! { RecoveryVouched(AccountId, AccountId, AccountId), /// A recovery process for account_1 by account_2 has been closed RecoveryClosed(AccountId, AccountId), - /// Account_1 has been recovered by account_2 + /// Account_1 has been successfully recovered by account_2 AccountRecovered(AccountId, AccountId), /// A recovery process has been removed for an account RecoveryRemoved(AccountId), diff --git a/frame/recovery/src/tests.rs b/frame/recovery/src/tests.rs index dc1624be30dd8..2ea6956e32b3e 100644 --- a/frame/recovery/src/tests.rs +++ b/frame/recovery/src/tests.rs @@ -251,14 +251,115 @@ fn initiate_recovery_works() { fn vouch_recovery_handles_basic_errors() { new_test_ext().execute_with(|| { // Cannot vouch for non-recoverable account + assert_noop!(Recovery::vouch_recovery(Origin::signed(2), 5, 1), Error::::NotRecoverable); + // Create a recovery process for next tests + let friends = vec![2, 3, 4]; + let threshold = 3; + let delay_period = 10; + assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); + // Cannot vouch a recovery process that has not started + assert_noop!(Recovery::vouch_recovery(Origin::signed(2), 5, 1), Error::::NotStarted); + + // Initiate a recovery process + assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); + // Cannot vouch if you are not a friend + assert_noop!(Recovery::vouch_recovery(Origin::signed(22), 5, 1), Error::::NotFriend); + // Cannot vouch twice + assert_ok!(Recovery::vouch_recovery(Origin::signed(2), 5, 1)); + assert_noop!(Recovery::vouch_recovery(Origin::signed(2), 5, 1), Error::::AlreadyVouched); + }); +} - // Create and initiate a recovery process for next tests +#[test] +fn vouch_recovery_works() { + new_test_ext().execute_with(|| { + // Create and initiate a recovery process for the test let friends = vec![2, 3, 4]; let threshold = 3; let delay_period = 10; assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); + // Vouching works + assert_ok!(Recovery::vouch_recovery(Origin::signed(2), 5, 1)); + // Handles out of order vouches + assert_ok!(Recovery::vouch_recovery(Origin::signed(4), 5, 1)); + assert_ok!(Recovery::vouch_recovery(Origin::signed(3), 5, 1)); + // Final recovery status object is updated correctly + let recovery_status = ActiveRecovery { + created: 1, + deposit: 10, + friends: vec![2, 3, 4], + }; + assert_eq!(>::get(&5, &1), Some(recovery_status)); }); } + +#[test] +fn claim_recovery_handles_basic_errors() { + new_test_ext().execute_with(|| { + // Cannot claim a non-recoverable account + assert_noop!(Recovery::claim_recovery(Origin::signed(1), 5), Error::::NotRecoverable); + + // Create a recovery process for the test + let friends = vec![2, 3, 4]; + let threshold = 3; + let delay_period = 10; + assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); + // Cannot claim an account which has not started the recovery process + assert_noop!(Recovery::claim_recovery(Origin::signed(1), 5), Error::::NotStarted); + assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); + // Cannot claim an account which has not passed the delay period + assert_noop!(Recovery::claim_recovery(Origin::signed(1), 5), Error::::DelayPeriod); + run_to_block(11); + // Cannot claim an account which has not passed the threshold number of votes + assert_ok!(Recovery::vouch_recovery(Origin::signed(2), 5, 1)); + assert_ok!(Recovery::vouch_recovery(Origin::signed(3), 5, 1)); + // Only 2/3 is not good enough + assert_noop!(Recovery::claim_recovery(Origin::signed(1), 5), Error::::Threshold); + }); +} + +#[test] +fn claim_recovery_works() { + new_test_ext().execute_with(|| { + // Create, initiate, and vouch recovery process for the test + let friends = vec![2, 3, 4]; + let threshold = 3; + let delay_period = 10; + assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); + assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); + assert_ok!(Recovery::vouch_recovery(Origin::signed(2), 5, 1)); + assert_ok!(Recovery::vouch_recovery(Origin::signed(3), 5, 1)); + assert_ok!(Recovery::vouch_recovery(Origin::signed(4), 5, 1)); + + run_to_block(11); + + // Account can be recovered. + assert_ok!(Recovery::claim_recovery(Origin::signed(1), 5)); + // Recovered storage item is correctly created + assert_eq!(>::get(&5), Some(1)); + + // Account could be re-recovered in the case that the recoverer account also gets lost. + assert_ok!(Recovery::initiate_recovery(Origin::signed(4), 5)); + assert_ok!(Recovery::vouch_recovery(Origin::signed(2), 5, 4)); + assert_ok!(Recovery::vouch_recovery(Origin::signed(3), 5, 4)); + assert_ok!(Recovery::vouch_recovery(Origin::signed(4), 5, 4)); + + run_to_block(21); + + // Account is re-recovered. + assert_ok!(Recovery::claim_recovery(Origin::signed(4), 5)); + // Recovered storage item is correctly updated + assert_eq!(>::get(&5), Some(4)); + }); +} + +#[test] +fn close_recovery_works() { + new_test_ext().execute_with(|| { + // Cannot close a non-active recovery + + }); +} \ No newline at end of file From 1f2f53a630b0dd7b74fdb87e8b510c11317d3e91 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 9 Jan 2020 19:42:04 +0100 Subject: [PATCH 14/21] Finish tests, except issue with `on_free_balance_zero` --- frame/recovery/src/lib.rs | 12 +++++++++-- frame/recovery/src/mock.rs | 2 +- frame/recovery/src/tests.rs | 40 +++++++++++++++++++++++++++++++++---- 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 28910d20c1011..0b8b18fdca38d 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -30,7 +30,7 @@ use frame_support::{ weights::{ GetDispatchInfo, }, - traits::{Currency, ReservableCurrency, Get}, + traits::{Currency, ReservableCurrency, Get, OnFreeBalanceZero}, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -313,7 +313,7 @@ decl_module! { /// Can only be called by the account trying to be recovered. fn close_recovery(origin, rescuer: T::AccountId) { let who = ensure_signed(origin)?; - // Take the active recovery process by the rescuer for this account. + // Take the active recovery process started by the rescuer for this account. let active_recovery = >::take(&who, &rescuer).ok_or(Error::::NotStarted)?; // Move the reserved funds from the rescuer to the rescued account. // Acts like a slashing mechanism for those who try to maliciously recover accounts. @@ -350,3 +350,11 @@ impl Module { friends.binary_search(&friend).is_ok() } } + +impl OnFreeBalanceZero for Module { + /// Remove any existing access another account might have when the account's free balance becomes zero. + /// This removes the final storage item managed by this module for any given account. + fn on_free_balance_zero(who: &T::AccountId) { + >::remove(who); + } +} diff --git a/frame/recovery/src/mock.rs b/frame/recovery/src/mock.rs index 6607be442ce48..3afef554e6ef2 100644 --- a/frame/recovery/src/mock.rs +++ b/frame/recovery/src/mock.rs @@ -87,7 +87,7 @@ parameter_types! { impl pallet_balances::Trait for Test { type Balance = u128; - type OnFreeBalanceZero = (); + type OnFreeBalanceZero = (Recovery); type OnNewAccount = (); type Event = TestEvent; type TransferPayment = (); diff --git a/frame/recovery/src/tests.rs b/frame/recovery/src/tests.rs index 2ea6956e32b3e..1ec8b92c05a68 100644 --- a/frame/recovery/src/tests.rs +++ b/frame/recovery/src/tests.rs @@ -80,7 +80,7 @@ fn recovery_lifecycle_works() { // funds used to initiate the recovery process into account 5. let call = Box::new(Call::Recovery(RecoveryCall::close_recovery(1))); assert_ok!(Recovery::as_recovered(Origin::signed(1), 5, call)); - // Account 1 can then use account 5 to close the recovery configuration, claiming the + // Account 1 can then use account 5 to remove the recovery configuration, claiming the // deposited funds used to create the recovery configuration into account 5. let call = Box::new(Call::Recovery(RecoveryCall::remove_recovery())); assert_ok!(Recovery::as_recovered(Origin::signed(1), 5, call)); @@ -91,6 +91,10 @@ fn recovery_lifecycle_works() { // All funds have been fully recovered! assert_eq!(Balances::free_balance(1), 200); assert_eq!(Balances::free_balance(5), 0); + // All storage items are removed from the module + assert!(!>::exists(&5, &1)); + assert!(!>::exists(&5)); + //assert!(!>::exists(&5)); }); } @@ -357,9 +361,37 @@ fn claim_recovery_works() { } #[test] -fn close_recovery_works() { +fn close_recovery_handles_basic_errors() { new_test_ext().execute_with(|| { // Cannot close a non-active recovery - + assert_noop!(Recovery::close_recovery(Origin::signed(5), 1), Error::::NotStarted); + }); +} + +#[test] +fn remove_recovery_works() { + new_test_ext().execute_with(|| { + // Cannot remove an unrecoverable account + assert_noop!(Recovery::remove_recovery(Origin::signed(5)), Error::::NotRecoverable); + + // Create and initiate a recovery process for the test + let friends = vec![2, 3, 4]; + let threshold = 3; + let delay_period = 10; + assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); + assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); + assert_ok!(Recovery::initiate_recovery(Origin::signed(2), 5)); + // Cannot remove a recovery when there are active recoveries. + assert_noop!(Recovery::remove_recovery(Origin::signed(5)), Error::::StillActive); + assert_ok!(Recovery::close_recovery(Origin::signed(5), 1)); + // Still need to remove one more! + assert_noop!(Recovery::remove_recovery(Origin::signed(5)), Error::::StillActive); + assert_ok!(Recovery::close_recovery(Origin::signed(5), 2)); + // Finally removed + assert_ok!(Recovery::remove_recovery(Origin::signed(5))); + + // Storage items are cleaned up at the end of this process + assert!(!>::exists(&5, &1)); + assert!(!>::exists(&5)); }); -} \ No newline at end of file +} From 08a648d4f8bc3e1853b595d8e7f96c6c09c282d7 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 9 Jan 2020 23:14:54 +0100 Subject: [PATCH 15/21] Fix `on_free_balance_zero` --- frame/recovery/src/mock.rs | 4 ++-- frame/recovery/src/tests.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/recovery/src/mock.rs b/frame/recovery/src/mock.rs index 3afef554e6ef2..a97cb0f9a0bce 100644 --- a/frame/recovery/src/mock.rs +++ b/frame/recovery/src/mock.rs @@ -80,14 +80,14 @@ impl frame_system::Trait for Test { } parameter_types! { - pub const ExistentialDeposit: u64 = 0; + pub const ExistentialDeposit: u64 = 1; pub const TransferFee: u64 = 0; pub const CreationFee: u64 = 0; } impl pallet_balances::Trait for Test { type Balance = u128; - type OnFreeBalanceZero = (Recovery); + type OnFreeBalanceZero = Recovery; type OnNewAccount = (); type Event = TestEvent; type TransferPayment = (); diff --git a/frame/recovery/src/tests.rs b/frame/recovery/src/tests.rs index 1ec8b92c05a68..bd4b93cfa9f27 100644 --- a/frame/recovery/src/tests.rs +++ b/frame/recovery/src/tests.rs @@ -94,7 +94,7 @@ fn recovery_lifecycle_works() { // All storage items are removed from the module assert!(!>::exists(&5, &1)); assert!(!>::exists(&5)); - //assert!(!>::exists(&5)); + assert!(!>::exists(&5)); }); } From 28db6a7a9074df37a60fd1691bcee947f97cab75 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 10 Jan 2020 11:43:23 +0100 Subject: [PATCH 16/21] Pallet docs --- frame/recovery/src/lib.rs | 157 +++++++++++++++++++++++++++++++----- frame/recovery/src/mock.rs | 3 +- frame/recovery/src/tests.rs | 14 ---- 3 files changed, 141 insertions(+), 33 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 0b8b18fdca38d..5e23f3c1e5a93 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -14,6 +14,139 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . +//! # Recovery Pallet +//! +//! - [`recovery::Trait`](./trait.Trait.html) +//! - [`Call`](./enum.Call.html) +//! +//! ## Overview +//! +//! The Recovery pallet is an M-of-N social recovery tool for users to gain +//! access to their accounts if the private key or other authentication mechanism +//! is lost. Through this pallet, a user is able to make calls on-behalf-of another +//! account which they have recovered. The recovery process is protected by trusted +//! "friends" whom the original account owner chooses. A threshold (M) out of N +//! friends are needed to give another account access to the recoverable account. +//! +//! ### Recovery Configuration +//! +//! The recovery process for each recoverable account can be configured by the account owner. +//! They are able to choose: +//! * `friends` - The list of friends that the account owner trusts to protect the +//! recovery process for their account. +//! * `threshold` - The number of friends that need to approve a recovery process for +//! the account to be successfully recovered. +//! * `delay_period` - The minimum number of blocks after the beginning of the recovery +//! process that need to pass before the account can be successfully recovered. +//! +//! There is a configurable deposit that all users need to pay to create a recovery +//! configuration. This deposit is composed of a base deposit plus a multiplier for +//! the number of friends chosen. This deposit is returned in full when the account +//! owner removes their recovery configuration. +//! +//! ### Recovery Lifecycle +//! +//! The intended lifecycle of a successful recovery takes the following steps: +//! 1. The account owner calls `create_recovery` to set up a recovery configuration +//! for their account. +//! 2. At some later time, the account owner loses access to their account and wants +//! to recover it. Likely, they will need to create a new account and fund it with +//! enough balance to support the transaction fees and the deposit for the +//! recovery process. +//! 3. Using this new account, they call `initiate_recovery`. +//! 4. Then the account owner would contact their configured friends to vouch for +//! the recovery attempt. The account owner would provide their old account id +//! and the new account id, and friends would call `vouch_recovery` with those +//! parameters. +//! 5. Once a threshold number of friends have vouched for the recovery attempt, +//! the account owner needs to wait until the delay period has passed, starting +//! when they initiated the recovery process. +//! 6. Now the account owner is able to call `claim_recovery`, which subsequently +//! allows them to call `as_recovered` and directly make calls on-behalf-of the lost +//! account. +//! 7. Using the now recovered account, the account owner can call `close_recovery` +//! on the recovery process they opened, reclaiming the recovery deposit they +//! placed. +//! 8. Then the account owner should then call `remove_recovery` to remove the recovery +//! 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 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. +//! +//! ### Malicious Recovery Attempts +//! +//! Initializing a the recovery process for a recoverable account is open and +//! permissionless. However, the recovery deposit is an economic deterrent that +//! should disincentivize would-be attackers from trying to maliciously recover +//! accounts. +//! +//! The recovery deposit can always be claimed by the account which is trying to +//! to be recovered. In the case of a malicious recovery attempt, the account +//! owner who still has access to their account can claim the deposit and +//! essentially punish the malicious user. +//! +//! Furthermore, the malicious recovery attempt can only be successful if the +//! attacker is also able to get enough friends to vouch for the recovery attempt. +//! In the case where the account owner prevents a malicious recovery process, +//! this pallet makes it near-zero cost to re-configure the recovery settings and +//! remove/replace friends who are acting inappropriately. +//! +//! ### Safety Considerations +//! +//! It is important to note that this is a powerful pallet that can compromise the +//! security of an account if used incorrectly. Some recommended practices for users +//! of this pallet are: +//! +//! * Configure a significant `delay_period` for your recovery process: As long as you +//! have access to your recoverable account, you need only check the blockchain once +//! every `delay_period` blocks to ensure that no recovery attempt is successful +//! against your account. Using off-chain notification systems can help with this, +//! but ultimately, setting a large `delay_period` means that even the most skilled +//! attacker will need to wait this long before they can access your account. +//! * Use a high threshold of approvals: Setting a value of 1 for the threshold means +//! that any of your friends would be able to recover your account. They would +//! simply need to start a recovery process and approve their own process. Similarly, +//! a threshold of 2 would mean that any 2 friends could work together to gain +//! access to your account. The only way to prevent against these kinds of attacks +//! is to choose a high threshold of approvals and select from a diverse friend +//! group that would not be able to reasonably coordinate with one another. +//! * Reset your configuration over time: Since the entire deposit of creating a +//! recovery configuration is returned to the user, the only cost of updating +//! your recovery configuration is the transaction fees for the calls. Thus, +//! it is strongly encouraged to regularly update your recovery configuration +//! as your life changes and your relationship with new and existing friends +//! change as well. +//! +//! ## Interface +//! +//! ### Dispatchable Functions +//! +//! #### For General Users +//! +//! * `create_recovery` - Create a recovery configuration for your account and make it recoverable. +//! * `initiate_recovery` - Start the recovery process for a recoverable account. +//! +//! #### For Friends of a Recoverable Account +//! * `vouch_recovery` - As a `friend` of a recoverable account, vouch for a recovery attempt on the account. +//! +//! #### For a User Who Successfully Recovered an Account +//! +//! * `claim_recovery` - Claim access to the account that you have successfully completed the recovery process for. +//! * `as_recovered` - Send a transaction as an account that you have recovered. See other functions below. +//! +//! #### For the Recoverable Account +//! +//! * `close_recovery` - Close an active recovery process for your account and reclaim the recovery deposit. +//! * `remove_recovery` - Remove the recovery configuration from the account, making it un-recoverable. +//! +//! #### For Super Users +//! +//! * `set_recovered` - The ROOT origin is able to skip the recovery process and directly allow +//! one account to access another. + // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] @@ -30,7 +163,7 @@ use frame_support::{ weights::{ GetDispatchInfo, }, - traits::{Currency, ReservableCurrency, Get, OnFreeBalanceZero}, + traits::{Currency, ReservableCurrency, Get, OnReapAccount}, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -191,10 +324,8 @@ decl_module! { /// Allow Sudo to bypass the recovery process and set an alias account. fn set_recovered(origin, lost: T::AccountId, rescuer: T::AccountId) { ensure_root(origin)?; - // Create the recovery storage item. >::insert(&lost, &rescuer); - Self::deposit_event(RawEvent::AccountRecovered(lost, rescuer)); } @@ -214,7 +345,6 @@ decl_module! { let max_friends = T::MaxFriends::get() as usize; ensure!(friends.len() <= max_friends, Error::::MaxFriends); ensure!(Self::is_sorted_and_unique(&friends), Error::::NotSorted); - // Total deposit is base fee + number of friends * factor fee let friend_deposit = T::FriendDepositFactor::get() .checked_mul(&friends.len().saturated_into()) @@ -224,7 +354,6 @@ decl_module! { .ok_or(Error::::Overflow)?; // Reserve the deposit T::Currency::reserve(&who, total_deposit)?; - // Create the recovery configuration let recovery_config = RecoveryConfig { delay_period, @@ -232,9 +361,8 @@ decl_module! { friends, threshold, }; - + // Create the recovery configuration storage item >::insert(&who, recovery_config); - Self::deposit_event(RawEvent::RecoveryCreated(who)); } @@ -254,32 +382,27 @@ decl_module! { deposit: recovery_deposit, friends: vec![], }; - + // Create the active recovery storage item >::insert(&account, &who, recovery_status); - Self::deposit_event(RawEvent::RecoveryInitiated(account, who)); } /// Allow a friend to vouch for an active recovery process. fn vouch_recovery(origin, lost: T::AccountId, rescuer: T::AccountId) { let who = ensure_signed(origin)?; - // Get the recovery configuration for the lost account. let recovery_config = Self::recovery_config(&lost).ok_or(Error::::NotRecoverable)?; // Get the active recovery process for the rescuer. let mut active_recovery = Self::active_recovery(&lost, &rescuer).ok_or(Error::::NotStarted)?; // Make sure the voter is a friend ensure!(Self::is_friend(&recovery_config.friends, &who), Error::::NotFriend); - // Either insert the vouch, or return an error that the user already vouched. match active_recovery.friends.binary_search(&who) { Ok(_pos) => Err(Error::::AlreadyVouched)?, Err(pos) => active_recovery.friends.insert(pos, who.clone()), } - // Update storage with the latest details >::insert(&lost, &rescuer, active_recovery); - Self::deposit_event(RawEvent::RecoveryVouched(lost, rescuer, who)); } @@ -301,10 +424,8 @@ decl_module! { recovery_config.threshold as usize <= active_recovery.friends.len(), Error::::Threshold ); - // Create the recovery storage item >::insert(&account, &who); - Self::deposit_event(RawEvent::AccountRecovered(account, who)); } @@ -351,10 +472,10 @@ impl Module { } } -impl OnFreeBalanceZero for Module { - /// Remove any existing access another account might have when the account's free balance becomes zero. +impl OnReapAccount for Module { + /// Remove any existing access another account might have when the account is reaped. /// This removes the final storage item managed by this module for any given account. - fn on_free_balance_zero(who: &T::AccountId) { + fn on_reap_account(who: &T::AccountId) { >::remove(who); } } diff --git a/frame/recovery/src/mock.rs b/frame/recovery/src/mock.rs index a97cb0f9a0bce..8d9f531965873 100644 --- a/frame/recovery/src/mock.rs +++ b/frame/recovery/src/mock.rs @@ -87,7 +87,8 @@ parameter_types! { impl pallet_balances::Trait for Test { type Balance = u128; - type OnFreeBalanceZero = Recovery; + type OnFreeBalanceZero = (); + type OnReapAccount = Recovery; type OnNewAccount = (); type Event = TestEvent; type TransferPayment = (); diff --git a/frame/recovery/src/tests.rs b/frame/recovery/src/tests.rs index bd4b93cfa9f27..76f05156ef5c6 100644 --- a/frame/recovery/src/tests.rs +++ b/frame/recovery/src/tests.rs @@ -215,11 +215,9 @@ fn initiate_recovery_handles_basic_errors() { let threshold = 3; let delay_period = 10; assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); - // Same user cannot recover same account twice assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); assert_noop!(Recovery::initiate_recovery(Origin::signed(1), 5), Error::::AlreadyStarted); - // No double deposit assert_eq!(Balances::reserved_balance(&1), 10); }); @@ -233,7 +231,6 @@ fn initiate_recovery_works() { let threshold = 3; let delay_period = 10; assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); - // Recovery can be initiated assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); // Deposit is reserved @@ -245,7 +242,6 @@ fn initiate_recovery_works() { friends: vec![], }; assert_eq!(>::get(&5, &1), Some(recovery_status)); - // Multiple users can attempt to recover the same account assert_ok!(Recovery::initiate_recovery(Origin::signed(2), 5)); }); @@ -256,7 +252,6 @@ fn vouch_recovery_handles_basic_errors() { new_test_ext().execute_with(|| { // Cannot vouch for non-recoverable account assert_noop!(Recovery::vouch_recovery(Origin::signed(2), 5, 1), Error::::NotRecoverable); - // Create a recovery process for next tests let friends = vec![2, 3, 4]; let threshold = 3; @@ -264,7 +259,6 @@ fn vouch_recovery_handles_basic_errors() { assert_ok!(Recovery::create_recovery(Origin::signed(5), friends.clone(), threshold, delay_period)); // Cannot vouch a recovery process that has not started assert_noop!(Recovery::vouch_recovery(Origin::signed(2), 5, 1), Error::::NotStarted); - // Initiate a recovery process assert_ok!(Recovery::initiate_recovery(Origin::signed(1), 5)); // Cannot vouch if you are not a friend @@ -289,7 +283,6 @@ fn vouch_recovery_works() { // Handles out of order vouches assert_ok!(Recovery::vouch_recovery(Origin::signed(4), 5, 1)); assert_ok!(Recovery::vouch_recovery(Origin::signed(3), 5, 1)); - // Final recovery status object is updated correctly let recovery_status = ActiveRecovery { created: 1, @@ -305,7 +298,6 @@ fn claim_recovery_handles_basic_errors() { new_test_ext().execute_with(|| { // Cannot claim a non-recoverable account assert_noop!(Recovery::claim_recovery(Origin::signed(1), 5), Error::::NotRecoverable); - // Create a recovery process for the test let friends = vec![2, 3, 4]; let threshold = 3; @@ -344,7 +336,6 @@ fn claim_recovery_works() { assert_ok!(Recovery::claim_recovery(Origin::signed(1), 5)); // Recovered storage item is correctly created assert_eq!(>::get(&5), Some(1)); - // Account could be re-recovered in the case that the recoverer account also gets lost. assert_ok!(Recovery::initiate_recovery(Origin::signed(4), 5)); assert_ok!(Recovery::vouch_recovery(Origin::signed(2), 5, 4)); @@ -373,7 +364,6 @@ fn remove_recovery_works() { new_test_ext().execute_with(|| { // Cannot remove an unrecoverable account assert_noop!(Recovery::remove_recovery(Origin::signed(5)), Error::::NotRecoverable); - // Create and initiate a recovery process for the test let friends = vec![2, 3, 4]; let threshold = 3; @@ -389,9 +379,5 @@ fn remove_recovery_works() { assert_ok!(Recovery::close_recovery(Origin::signed(5), 2)); // Finally removed assert_ok!(Recovery::remove_recovery(Origin::signed(5))); - - // Storage items are cleaned up at the end of this process - assert!(!>::exists(&5, &1)); - assert!(!>::exists(&5)); }); } From 29986ecfe60385e367a67a707b88ebe132392637 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 10 Jan 2020 15:29:53 +0100 Subject: [PATCH 17/21] Add function/weight docs --- frame/recovery/src/lib.rs | 213 +++++++++++++++++++++++++++++++++++--- 1 file changed, 198 insertions(+), 15 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 5e23f3c1e5a93..cdc25d7751ff3 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -161,7 +161,7 @@ use frame_support::{ decl_module, decl_event, decl_storage, decl_error, ensure, Parameter, RuntimeDebug, weights::{ - GetDispatchInfo, + GetDispatchInfo, PaysFee, DispatchClass, ClassifyDispatch, Weight, WeighData, }, traits::{Currency, ReservableCurrency, Get, OnReapAccount}, }; @@ -189,12 +189,12 @@ pub trait Trait: frame_system::Trait { /// The base amount of currency needed to reserve for creating a recovery configuration. /// /// This is held for an additional storage item whose value size is - /// TODO bytes. + /// `2 + sizeof(BlockNumber, Balance)` bytes. type ConfigDepositBase: Get>; /// The amount of currency needed per additional user when creating a recovery configuration. /// - /// This is held for adding TODO bytes more into a pre-existing storage value. + /// This is held for adding `sizeof(AccountId)` bytes more into a pre-existing storage value. type FriendDepositFactor: Get>; /// The maximum amount of friends allowed in a recovery configuration. @@ -202,8 +202,11 @@ pub trait Trait: frame_system::Trait { /// The base amount of currency needed to reserve for starting a recovery. /// - /// This is held for an additional storage item whose value size is - /// TODO bytes. + /// 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 + /// deposit. It also acts to fund additional storage item whose value size is + /// `sizeof(BlockNumber, Balance + T * AccountId)` bytes. Where T is a configurable + /// threshold. type RecoveryDeposit: Get>; } @@ -314,14 +317,42 @@ decl_module! { fn deposit_event() = default; /// Send a call through a recovered account. - fn as_recovered(origin, account: T::AccountId, call: Box<::Call>) -> DispatchResult { + /// + /// The dispatch origin for this call must be _Signed_ and registered to + /// be able to make calls on behalf of the recovered account. + /// + /// Parameters: + /// - `account`: The recovered account you want to make a call on-behalf-of. + /// - `call`: The call you want to make with the recovered account. + /// + /// # + /// - The weight of the `call`. + /// # + #[weight = ::Call>>::new()] + fn as_recovered(origin, + account: T::AccountId, + call: Box<::Call> + ) -> DispatchResult { let who = ensure_signed(origin)?; // Check `who` is allowed to make a call on behalf of `account` ensure!(Self::recovered_account(&account) == Some(who), Error::::NotAllowed); call.dispatch(frame_system::RawOrigin::Signed(account).into()) } - /// Allow Sudo to bypass the recovery process and set an alias account. + /// Allow ROOT to bypass the recovery process and set an a rescuer account + /// for a lost account directly. + /// + /// The dispatch origin for this call must be _ROOT_. + /// + /// Parameters: + /// - `lost`: The "lost account" to be recovered. + /// - `rescuer`: The "rescuer account" which can call as the lost account. + /// + /// # + /// - One storage write O(1) + /// - One event + /// # + #[weight = SimpleDispatchInfo::FixedNormal(10_000)] fn set_recovered(origin, lost: T::AccountId, rescuer: T::AccountId) { ensure_root(origin)?; // Create the recovery storage item. @@ -329,7 +360,34 @@ decl_module! { Self::deposit_event(RawEvent::AccountRecovered(lost, rescuer)); } - /// Create a recovery process for your account. + /// 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 + /// in full when the user calls `remove_recovery`. + /// + /// The dispatch origin for this call must be _Signed_. + /// + /// Parameters: + /// - `friends`: A list of friends you trust to vouch for recovery attempts. + /// Should be ordered and contain no duplicate values. + /// - `threshold`: The number of friends that must vouch for a recovery attempt + /// before the account can be recovered. Should be less than or equal to + /// the length of the list of friends. + /// - `delay_period`: The number of blocks after a recovery attempt is initialized + /// that needs to pass before the account can be recovered. + /// + /// # + /// - Key: F (len of friends) + /// - One storage read to check that account is not already recoverable. O(1). + /// - A check that the friends list is sorted and unique. O(F) + /// - One currency reserve operation. O(X) + /// - One storage write. O(1). Codec O(F). + /// - One event. + /// + /// Total Complexity: O(F + X) + /// # + #[weight = SimpleDispatchInfo::FixedNormal(100_000)] fn create_recovery(origin, friends: Vec, threshold: u16, @@ -366,7 +424,29 @@ decl_module! { Self::deposit_event(RawEvent::RecoveryCreated(who)); } - /// Allow a user to start the process for recovering an account. + /// Initiate the process for recovering a recoverable account. + /// + /// Payment: `RecoveryDeposit` balance will be reserved for initiating the + /// recovery process. This deposit will always be repatriated to the account + /// trying to be recovered. See `close_recovery`. + /// + /// The dispatch origin for this call must be _Signed_. + /// + /// Parameters: + /// - `account`: The lost account that you want to recover. This account + /// needs to be recoverable (i.e. have a recovery configuration). + /// + /// # + /// - One storage read to check that account is recoverable. O(F) + /// - One storage read to check that this recovery process hasn't already started. O(1) + /// - One currency reserve operation. O(X) + /// - One storage read to get the current block number. O(1) + /// - One storage write. O(1). + /// - One event. + /// + /// Total Complexity: O(F + X) + /// # + #[weight = SimpleDispatchInfo::FixedNormal(100_000)] fn initiate_recovery(origin, account: T::AccountId) { let who = ensure_signed(origin)?; // Check that the account is recoverable @@ -387,7 +467,32 @@ decl_module! { Self::deposit_event(RawEvent::RecoveryInitiated(account, who)); } - /// Allow a friend to vouch for an active recovery process. + /// Allow a "friend" of a recoverable account to vouch for an active recovery + /// process for that account. + /// + /// The dispatch origin for this call must be _Signed_ and must be a "friend" + /// for the recoverable account. + /// + /// Parameters: + /// - `lost`: The lost account that you want to recover. + /// - `rescuer`: The account trying to rescue the lost account that you + /// want to vouch for. + /// + /// The combination of these two parameters must point to an active recovery + /// process. + /// + /// # + /// Key: F (len of friends in config), V (len of vouching friends) + /// - One storage read to get the recovery configuration. O(1), Codec O(F) + /// - One storage read to get the active recovery process. O(1), Codec O(V) + /// - One binary search to confirm caller is a friend. O(logF) + /// - One binary search to confirm caller has not already vouched. O(logV) + /// - One storage write. O(1), Codec O(V). + /// - One event. + /// + /// Total Complexity: O(F + logF + V + logV) + /// # + #[weight = SimpleDispatchInfo::FixedNormal(100_000)] fn vouch_recovery(origin, lost: T::AccountId, rescuer: T::AccountId) { let who = ensure_signed(origin)?; // Get the recovery configuration for the lost account. @@ -406,7 +511,27 @@ decl_module! { Self::deposit_event(RawEvent::RecoveryVouched(lost, rescuer, who)); } - /// Allow a rescuer to claim their recovered account. + /// Allow a successful rescuer to claim their recovered account. + /// + /// The dispatch origin for this call must be _Signed_ and must be a "rescuer" + /// who has successfully completed the account recovery process: collected + /// `threshold` or more vouches, waited `delay_period` blocks since initiation. + /// + /// Parameters: + /// - `account`: The lost account that you want to claim has been successfully + /// recovered by you. + /// + /// # + /// Key: F (len of friends in config), V (len of vouching friends) + /// - One storage read to get the recovery configuration. O(1), Codec O(F) + /// - One storage read to get the active recovery process. O(1), Codec O(V) + /// - One storage read to get the current block number. O(1) + /// - One storage write. O(1), Codec O(V). + /// - One event. + /// + /// Total Complexity: O(F + V) + /// # + #[weight = SimpleDispatchInfo::FixedNormal(100_000)] fn claim_recovery(origin, account: T::AccountId) { let who = ensure_signed(origin)?; // Get the recovery configuration for the lost account @@ -429,9 +554,27 @@ decl_module! { Self::deposit_event(RawEvent::AccountRecovered(account, who)); } - /// Close an active recovery process. + /// As the controller of a recoverable account, close an active recovery + /// process for your account. + /// + /// Payment: By calling this function, the recoverable account will receive + /// the recovery deposit `RecoveryDeposit` placed by the rescuer. + /// + /// The dispatch origin for this call must be _Signed_ and must be a + /// recoverable account with an active recovery process for it. /// - /// Can only be called by the account trying to be recovered. + /// Parameters: + /// - `rescuer`: The account trying to rescue this recoverable account. + /// + /// # + /// Key: V (len of vouching friends) + /// - One storage read/remove to get the active recovery process. O(1), Codec O(V) + /// - One balance call to repatriate reserved. O(X) + /// - One event. + /// + /// Total Complexity: O(V + X) + /// # + #[weight = SimpleDispatchInfo::FixedNormal(30_000)] fn close_recovery(origin, rescuer: T::AccountId) { let who = ensure_signed(origin)?; // Take the active recovery process started by the rescuer for this account. @@ -444,8 +587,26 @@ decl_module! { /// Remove the recovery process for your account. /// - /// The user must make sure to call `close_recovery` on all active recovery attempts - /// before calling this function. + /// 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 + /// their recovery configuration deposit. + /// (`ConfigDepositBase` + `FriendDepositFactor` * #_of_friends) + /// + /// The dispatch origin for this call must be _Signed_ and must be a + /// recoverable account (i.e. has a recovery configuration). + /// + /// # + /// Key: F (len of friends) + /// - One storage read to get the prefix iterator for active recoveries. O(1) + /// - One storage read/remove to get the recovery configuration. O(1), Codec O(F) + /// - One balance call to unreserved. O(X) + /// - One event. + /// + /// Total Complexity: O(F + X) + /// # + #[weight = SimpleDispatchInfo::FixedNormal(30_000)] fn remove_recovery(origin) { let who = ensure_signed(origin)?; // Check there are no active recoveries @@ -479,3 +640,25 @@ impl OnReapAccount for Module { >::remove(who); } } + +/// Simple pass through for the weight functions. +struct Passthrough(sp_std::marker::PhantomData<(AccountId, Call)>); + +impl Passthrough { + fn new() -> Self { Self(Default::default()) } +} +impl WeighData<(&AccountId, &Box)> for Passthrough { + fn weigh_data(&self, (_, call): (&AccountId, &Box)) -> Weight { + call.get_dispatch_info().weight + 10_000 + } +} +impl ClassifyDispatch<(&AccountId, &Box)> for Passthrough { + fn classify_dispatch(&self, (_, call): (&AccountId, &Box)) -> DispatchClass { + call.get_dispatch_info().class + } +} +impl PaysFee for Passthrough { + fn pays_fee(&self) -> bool { + true + } +} From 95c4513f5c93f48f2064ec2395446c04f3272c7e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 10 Jan 2020 15:33:11 +0100 Subject: [PATCH 18/21] Fix merge master --- frame/recovery/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index cdc25d7751ff3..7f117d7bd65f0 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -162,6 +162,7 @@ use frame_support::{ Parameter, RuntimeDebug, weights::{ GetDispatchInfo, PaysFee, DispatchClass, ClassifyDispatch, Weight, WeighData, + SimpleDispatchInfo, }, traits::{Currency, ReservableCurrency, Get, OnReapAccount}, }; @@ -247,7 +248,7 @@ decl_storage! { /// First account is the account to be recovered, and the second account /// is the user trying to recover the account. pub ActiveRecoveries get(fn active_recovery): - double_map hasher(twox_64_concat) T::AccountId, twox_64_concat(T::AccountId) => + double_map hasher(twox_64_concat) T::AccountId, hasher(twox_64_concat) T::AccountId => Option, T::AccountId>>; /// The final list of recovered accounts. /// From 38048db968828aa95f9a3bc33616185c0bb655bf Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 10 Jan 2020 15:37:28 +0100 Subject: [PATCH 19/21] OnReapAccount for System too --- frame/recovery/src/mock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/recovery/src/mock.rs b/frame/recovery/src/mock.rs index 8d9f531965873..be9ed5c97e598 100644 --- a/frame/recovery/src/mock.rs +++ b/frame/recovery/src/mock.rs @@ -88,7 +88,7 @@ parameter_types! { impl pallet_balances::Trait for Test { type Balance = u128; type OnFreeBalanceZero = (); - type OnReapAccount = Recovery; + type OnReapAccount = (System, Recovery); type OnNewAccount = (); type Event = TestEvent; type TransferPayment = (); From 8f1f02f0ff27f28912112cbe1fb1f4a5a78b7437 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 13 Jan 2020 16:19:44 +0100 Subject: [PATCH 20/21] Update weight docs --- frame/recovery/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 7f117d7bd65f0..778136aae1818 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -328,6 +328,7 @@ decl_module! { /// /// # /// - The weight of the `call`. + /// - One storage lookup to check account is recovered by `who`. O(1) /// # #[weight = ::Call>>::new()] fn as_recovered(origin, From 29bef125659f599a5a7d9de1a1408e13453eca67 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 13 Jan 2020 17:00:02 +0100 Subject: [PATCH 21/21] Allow passthrough to support fee-less extrinsics --- frame/recovery/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 778136aae1818..d0b5783587251 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -659,8 +659,8 @@ impl ClassifyDispatch<(&AccountId, &Box) call.get_dispatch_info().class } } -impl PaysFee for Passthrough { - fn pays_fee(&self) -> bool { - true +impl PaysFee<(&AccountId, &Box)> for Passthrough { + fn pays_fee(&self, (_, call): (&AccountId, &Box)) -> bool { + call.get_dispatch_info().pays_fee } }