diff --git a/bin/node-template/pallets/template/src/mock.rs b/bin/node-template/pallets/template/src/mock.rs index 7a23610e4b0a3..2ea81ffb45626 100644 --- a/bin/node-template/pallets/template/src/mock.rs +++ b/bin/node-template/pallets/template/src/mock.rs @@ -41,7 +41,7 @@ impl system::Trait for Test { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl Trait for Test { type Event = (); diff --git a/bin/node-template/runtime/src/lib.rs b/bin/node-template/runtime/src/lib.rs index a1bcd157ad63a..afc18177ce078 100644 --- a/bin/node-template/runtime/src/lib.rs +++ b/bin/node-template/runtime/src/lib.rs @@ -164,7 +164,7 @@ impl system::Trait for Runtime { /// What to do if a new account is created. type OnNewAccount = (); /// What to do if an account is fully reaped from the system. - type OnReapAccount = Balances; + type OnKilledAccount = (); /// The data to be stored in an account. type AccountData = balances::AccountData; } diff --git a/bin/node/cli/src/chain_spec.rs b/bin/node/cli/src/chain_spec.rs index 5cdfb5cab86f5..d163cdf3918a2 100644 --- a/bin/node/cli/src/chain_spec.rs +++ b/bin/node/cli/src/chain_spec.rs @@ -243,7 +243,7 @@ pub fn testnet_genesis( }), pallet_session: Some(SessionConfig { keys: initial_authorities.iter().map(|x| { - (x.0.clone(), session_keys(x.2.clone(), x.3.clone(), x.4.clone(), x.5.clone())) + (x.0.clone(), x.0.clone(), session_keys(x.2.clone(), x.3.clone(), x.4.clone(), x.5.clone())) }).collect::>(), }), pallet_staking: Some(StakingConfig { diff --git a/bin/node/cli/tests/common.rs b/bin/node/cli/tests/common.rs index 93a4a3e4e5789..682a30bcc0178 100644 --- a/bin/node/cli/tests/common.rs +++ b/bin/node/cli/tests/common.rs @@ -60,5 +60,5 @@ pub fn run_command_for_a_while(base_path: &Path, dev: bool) { // Stop the process kill(Pid::from_raw(cmd.id().try_into().unwrap()), SIGINT).unwrap(); - assert!(wait_for(&mut cmd, 20).map(|x| x.success()).unwrap_or_default()); + assert!(wait_for(&mut cmd, 40).map(|x| x.success()).unwrap_or_default()); } diff --git a/bin/node/executor/tests/basic.rs b/bin/node/executor/tests/basic.rs index 972c561a2fb66..eb629029c6dd1 100644 --- a/bin/node/executor/tests/basic.rs +++ b/bin/node/executor/tests/basic.rs @@ -164,7 +164,7 @@ fn panic_execution_with_foreign_code_gives_error() { let mut t = TestExternalities::::new_with_code(BLOATY_CODE, Storage { top: map![ >::hashed_key_for(alice()) => { - (69u128, 0u128, 0u128, 0u128).encode() + (69u128, 0u8, 0u128, 0u128, 0u128).encode() }, >::hashed_key().to_vec() => { 69_u128.encode() @@ -200,7 +200,7 @@ fn bad_extrinsic_with_native_equivalent_code_gives_error() { let mut t = TestExternalities::::new_with_code(COMPACT_CODE, Storage { top: map![ >::hashed_key_for(alice()) => { - (0u32, 69u128, 0u128, 0u128, 0u128).encode() + (0u32, 0u8, 69u128, 0u128, 0u128, 0u128).encode() }, >::hashed_key().to_vec() => { 69_u128.encode() @@ -236,7 +236,7 @@ fn successful_execution_with_native_equivalent_code_gives_ok() { let mut t = TestExternalities::::new_with_code(COMPACT_CODE, Storage { top: map![ >::hashed_key_for(alice()) => { - (0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() + (0u32, 0u8, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() }, >::hashed_key().to_vec() => { (111 * DOLLARS).encode() @@ -278,7 +278,7 @@ fn successful_execution_with_foreign_code_gives_ok() { let mut t = TestExternalities::::new_with_code(BLOATY_CODE, Storage { top: map![ >::hashed_key_for(alice()) => { - (0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() + (0u32, 0u8, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() }, >::hashed_key().to_vec() => { (111 * DOLLARS).encode() @@ -734,7 +734,7 @@ fn successful_execution_gives_ok() { let mut t = TestExternalities::::new_with_code(COMPACT_CODE, Storage { top: map![ >::hashed_key_for(alice()) => { - (0u32, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() + (0u32, 0u8, 111 * DOLLARS, 0u128, 0u128, 0u128).encode() }, >::hashed_key().to_vec() => { (111 * DOLLARS).encode() diff --git a/bin/node/executor/tests/fees.rs b/bin/node/executor/tests/fees.rs index 374dee354f123..024c02bd6aaf9 100644 --- a/bin/node/executor/tests/fees.rs +++ b/bin/node/executor/tests/fees.rs @@ -139,10 +139,10 @@ fn transaction_fee_is_correct_ultimate() { let mut t = TestExternalities::::new_with_code(COMPACT_CODE, Storage { top: map![ >::hashed_key_for(alice()) => { - (0u32, 100 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS).encode() + (0u32, 0u8, 100 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS).encode() }, >::hashed_key_for(bob()) => { - (0u32, 10 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS).encode() + (0u32, 0u8, 10 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS, 0 * DOLLARS).encode() }, >::hashed_key().to_vec() => { (110 * DOLLARS).encode() diff --git a/bin/node/executor/tests/submit_transaction.rs b/bin/node/executor/tests/submit_transaction.rs index b870cf40370f1..1a92aeca6ba77 100644 --- a/bin/node/executor/tests/submit_transaction.rs +++ b/bin/node/executor/tests/submit_transaction.rs @@ -167,8 +167,9 @@ fn submitted_transaction_should_be_valid() { // add balance to the account let author = extrinsic.signature.clone().unwrap().0; let address = Indices::lookup(author).unwrap(); - let account = pallet_balances::AccountData { free: 5_000_000_000_000, ..Default::default() }; - >::insert(&address, (0u32, account)); + let data = pallet_balances::AccountData { free: 5_000_000_000_000, ..Default::default() }; + let account = frame_system::AccountInfo { nonce: 0u32, refcount: 0u8, data }; + >::insert(&address, account); // check validity let res = Executive::validate_transaction(extrinsic); diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index b6be9335ca94c..e91eca4e400cd 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -81,8 +81,8 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // and set impl_version to 0. If only runtime // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. - spec_version: 224, - impl_version: 2, + spec_version: 225, + impl_version: 0, apis: RUNTIME_API_VERSIONS, }; @@ -131,7 +131,7 @@ impl frame_system::Trait for Runtime { type ModuleToIndex = ModuleToIndex; type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = (Balances, Staking, Contracts, Session, Recovery); + type OnKilledAccount = (); } parameter_types! { diff --git a/bin/node/testing/src/genesis.rs b/bin/node/testing/src/genesis.rs index 9c163ea6153b2..6aa7290a641bf 100644 --- a/bin/node/testing/src/genesis.rs +++ b/bin/node/testing/src/genesis.rs @@ -69,15 +69,15 @@ pub fn config_endowed( }), pallet_session: Some(SessionConfig { keys: vec![ - (alice(), to_session_keys( + (dave(), alice(), to_session_keys( &Ed25519Keyring::Alice, &Sr25519Keyring::Alice, )), - (bob(), to_session_keys( + (eve(), bob(), to_session_keys( &Ed25519Keyring::Bob, &Sr25519Keyring::Bob, )), - (charlie(), to_session_keys( + (ferdie(), charlie(), to_session_keys( &Ed25519Keyring::Charlie, &Sr25519Keyring::Charlie, )), diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index ebd1d17bcffe4..042ff89913417 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -295,7 +295,7 @@ mod tests { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl Trait for Test { type Event = (); diff --git a/frame/aura/src/mock.rs b/frame/aura/src/mock.rs index 5b3ccd7745f8c..05a161ee49c3d 100644 --- a/frame/aura/src/mock.rs +++ b/frame/aura/src/mock.rs @@ -63,7 +63,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl pallet_timestamp::Trait for Test { diff --git a/frame/authority-discovery/src/lib.rs b/frame/authority-discovery/src/lib.rs index 77906e1bfe34f..8ee4931e488c3 100644 --- a/frame/authority-discovery/src/lib.rs +++ b/frame/authority-discovery/src/lib.rs @@ -159,7 +159,7 @@ mod tests { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl_outer_origin! { diff --git a/frame/authorship/src/lib.rs b/frame/authorship/src/lib.rs index 335e13a7fd71b..d3c1bf752aeb8 100644 --- a/frame/authorship/src/lib.rs +++ b/frame/authorship/src/lib.rs @@ -433,7 +433,7 @@ mod tests { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } parameter_types! { diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index aef0b4b386037..2ec083728e82c 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -66,7 +66,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl_opaque_keys! { diff --git a/frame/balances/Cargo.toml b/frame/balances/Cargo.toml index 728adf9de7612..be3fa14c7a057 100644 --- a/frame/balances/Cargo.toml +++ b/frame/balances/Cargo.toml @@ -18,7 +18,6 @@ frame-support = { version = "2.0.0-dev", default-features = false, path = "../su frame-system = { version = "2.0.0-dev", default-features = false, path = "../system" } [dev-dependencies] -sp-io = { version = "2.0.0-dev", path = "../../primitives/io" } sp-core = { version = "2.0.0-dev", path = "../../primitives/core" } pallet-transaction-payment = { version = "2.0.0-dev", path = "../transaction-payment" } diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 0f5e8b40d5efa..4dbaf4b8a886d 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -159,11 +159,12 @@ mod benchmarking; use sp_std::prelude::*; use sp_std::{cmp, result, mem, fmt::Debug, ops::BitOr, convert::Infallible}; +use sp_io::hashing::twox_64; use codec::{Codec, Encode, Decode}; use frame_support::{ StorageValue, Parameter, decl_event, decl_storage, decl_module, decl_error, ensure, weights::SimpleDispatchInfo, traits::{ - Currency, OnReapAccount, OnUnbalanced, TryDrop, StoredMap, + Currency, OnKilledAccount, OnUnbalanced, TryDrop, StoredMap, WithdrawReason, WithdrawReasons, LockIdentifier, LockableCurrency, ExistenceRequirement, Imbalance, SignedImbalance, ReservableCurrency, Get, ExistenceRequirement::KeepAlive, ExistenceRequirement::AllowDeath, IsDeadAccount, BalanceStatus as Status @@ -178,7 +179,7 @@ use sp_runtime::{ }; use frame_system::{self as system, ensure_signed, ensure_root}; use frame_support::storage::migration::{ - get_storage_value, take_storage_value, put_storage_value, StorageIterator + get_storage_value, take_storage_value, put_storage_value, StorageIterator, have_storage_value }; pub use self::imbalances::{PositiveImbalance, NegativeImbalance}; @@ -609,7 +610,16 @@ impl, I: Instance> Module { for (hash, balances) in StorageIterator::>::new(b"Balances", b"Account").drain() { let nonce = take_storage_value::(b"System", b"AccountNonce", &hash).unwrap_or_default(); - put_storage_value(b"System", b"Account", &hash, (nonce, balances)); + let mut refs: system::RefCount = 0; + // The items in Kusama that would result in a ref count being incremented. + if have_storage_value(b"Democracy", b"Proxy", &hash) { refs += 1 } + // We skip Recovered since it's being replaced anyway. + let mut prefixed_hash = twox_64(&b":session:keys"[..]).to_vec(); + prefixed_hash.extend(&b":session:keys"[..]); + prefixed_hash.extend(&hash[..]); + if have_storage_value(b"Session", b"NextKeys", &prefixed_hash) { refs += 1 } + if have_storage_value(b"Staking", b"Bonded", &hash) { refs += 1 } + put_storage_value(b"System", b"Account", &hash, (nonce, refs, &balances)); } } @@ -721,14 +731,21 @@ impl, I: Instance> Module { } } }); - Locks::::insert(who, locks); - } -} -impl, I: Instance> OnReapAccount for Module { - fn on_reap_account(who: &T::AccountId) { - Locks::::remove(who); - Account::::remove(who); + let existed = Locks::::contains_key(who); + if locks.is_empty() { + Locks::::remove(who); + if existed { + // TODO: use Locks::::hashed_key + // https://github.com/paritytech/substrate/issues/4969 + system::Module::::dec_ref(who); + } + } else { + Locks::::insert(who, locks); + if !existed { + system::Module::::inc_ref(who); + } + } } } @@ -923,7 +940,7 @@ impl, I: Instance> frame_system::Trait for ElevatedTrait { type Version = T::Version; type ModuleToIndex = T::ModuleToIndex; type OnNewAccount = T::OnNewAccount; - type OnReapAccount = T::OnReapAccount; + type OnKilledAccount = T::OnKilledAccount; type AccountData = T::AccountData; } impl, I: Instance> Trait for ElevatedTrait { @@ -1040,6 +1057,7 @@ impl, I: Instance> Currency for Module where )?; let allow_death = existence_requirement == ExistenceRequirement::AllowDeath; + let allow_death = allow_death && system::Module::::allow_death(transactor); ensure!(allow_death || from_account.free >= ed, Error::::KeepAlive); Ok(()) @@ -1283,6 +1301,17 @@ impl, I: Instance> ReservableCurrency for Module } } +/// Implement `OnKilledAccount` to remove the local account, if using local account storage. +/// +/// NOTE: You probably won't need to use this! This only needs to be "wired in" to System module +/// if you're using the local balance storage. **If you're using the composite system account +/// storage (which is the default in most examples and tests) then there's no need.** +impl, I: Instance> OnKilledAccount for Module { + fn on_killed_account(who: &T::AccountId) { + Account::::remove(who); + } +} + impl, I: Instance> LockableCurrency for Module where T::Balance: MaybeSerializeDeserialize + Debug diff --git a/frame/balances/src/tests.rs b/frame/balances/src/tests.rs index 3d0c9e9207f5d..98c7c856bc88a 100644 --- a/frame/balances/src/tests.rs +++ b/frame/balances/src/tests.rs @@ -24,8 +24,10 @@ macro_rules! decl_tests { use sp_runtime::{Fixed64, traits::{SignedExtension, BadOrigin}}; use frame_support::{ assert_noop, assert_ok, assert_err, - traits::{LockableCurrency, LockIdentifier, WithdrawReason, WithdrawReasons, - Currency, ReservableCurrency, ExistenceRequirement::AllowDeath} + traits::{ + LockableCurrency, LockIdentifier, WithdrawReason, WithdrawReasons, + Currency, ReservableCurrency, ExistenceRequirement::AllowDeath, StoredMap + } }; use pallet_transaction_payment::ChargeTransactionPayment; use frame_system::RawOrigin; @@ -55,6 +57,15 @@ macro_rules! decl_tests { }); } + #[test] + fn account_should_be_reaped() { + <$ext_builder>::default().existential_deposit(1).monied(true).build().execute_with(|| { + assert_eq!(Balances::free_balance(1), 10); + assert_ok!(>::transfer(&1, &2, 10, AllowDeath)); + assert!(!<::AccountStore as StoredMap>>::is_explicit(&1)); + }); + } + #[test] fn partial_locking_should_work() { <$ext_builder>::default().existential_deposit(1).monied(true).build().execute_with(|| { diff --git a/frame/balances/src/tests_composite.rs b/frame/balances/src/tests_composite.rs index c566c9a9d009d..3a5c2178f88cd 100644 --- a/frame/balances/src/tests_composite.rs +++ b/frame/balances/src/tests_composite.rs @@ -67,7 +67,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = super::AccountData; type OnNewAccount = (); - type OnReapAccount = Module; + type OnKilledAccount = (); } parameter_types! { pub const TransactionBaseFee: u64 = 0; diff --git a/frame/balances/src/tests_local.rs b/frame/balances/src/tests_local.rs index a63046e901d49..861c1972127a0 100644 --- a/frame/balances/src/tests_local.rs +++ b/frame/balances/src/tests_local.rs @@ -67,7 +67,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = super::AccountData; type OnNewAccount = (); - type OnReapAccount = Module; + type OnKilledAccount = Module; } parameter_types! { pub const TransactionBaseFee: u64 = 0; diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index a3ea901a6fd24..605cda3cb77e2 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -435,7 +435,7 @@ mod tests { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl Trait for Test { type Origin = Origin; diff --git a/frame/contracts/src/account_db.rs b/frame/contracts/src/account_db.rs index 5204f1003a6c5..374c55c374d7b 100644 --- a/frame/contracts/src/account_db.rs +++ b/frame/contracts/src/account_db.rs @@ -151,7 +151,7 @@ impl AccountDb for DirectAccountDb { let exists = !T::Currency::total_balance(&address).is_zero(); total_imbalance = total_imbalance.merge(imbalance); if existed && !exists { - // Account killed. This will ultimately lead to calling `OnReapAccount` callback + // Account killed. This will ultimately lead to calling `OnKilledAccount` callback // which will make removal of CodeHashOf and AccountStorage for this account. // In order to avoid writing over the deleted properties we `continue` here. continue; diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 42cbaa3a7c2af..571ae9700cf94 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -125,7 +125,7 @@ use frame_support::{ parameter_types, IsSubType, weights::DispatchInfo, }; -use frame_support::traits::{OnReapAccount, OnUnbalanced, Currency, Get, Time, Randomness}; +use frame_support::traits::{OnKilledAccount, OnUnbalanced, Currency, Get, Time, Randomness}; use frame_system::{self as system, ensure_signed, RawOrigin, ensure_root}; use sp_core::storage::well_known_keys::CHILD_STORAGE_KEY_PREFIX; use pallet_contracts_primitives::{RentProjection, ContractAccessError}; @@ -941,8 +941,12 @@ decl_storage! { } } -impl OnReapAccount for Module { - fn on_reap_account(who: &T::AccountId) { +// TODO: this should be removed in favour of a self-destruct contract host function allowing the +// contract to delete all storage and the `ContractInfoOf` key and transfer remaining balance to +// some other account. As it stands, it's an economic insecurity on any smart-contract chain. +// https://github.com/paritytech/substrate/issues/4952 +impl OnKilledAccount for Module { + fn on_killed_account(who: &T::AccountId) { if let Some(ContractInfo::Alive(info)) = >::take(who) { child::kill_storage(&info.trie_id, info.child_trie_unique_id()); } diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index ddd532334c158..e775998a3a553 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -117,7 +117,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = (Balances, Contracts); + type OnKilledAccount = Contracts; } impl pallet_balances::Trait for Test { type Balance = u64; @@ -1606,7 +1606,7 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage: assert_eq!(System::events(), vec![ EventRecord { phase: Phase::ApplyExtrinsic(0), - event: MetaEvent::system(system::RawEvent::ReapedAccount(DJANGO)), + event: MetaEvent::system(system::RawEvent::KilledAccount(DJANGO)), topics: vec![], }, EventRecord { diff --git a/frame/democracy/src/lib.rs b/frame/democracy/src/lib.rs index c2c44918b036c..6755fabd2039f 100644 --- a/frame/democracy/src/lib.rs +++ b/frame/democracy/src/lib.rs @@ -30,7 +30,7 @@ use frame_support::{ weights::SimpleDispatchInfo, traits::{ Currency, ReservableCurrency, LockableCurrency, WithdrawReason, LockIdentifier, Get, - OnReapAccount, OnUnbalanced, BalanceStatus + OnUnbalanced, BalanceStatus } }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -260,6 +260,24 @@ impl ReferendumInfo } } +/// State of a proxy voting account. +#[derive(Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug)] +pub enum ProxyState { + /// Account is open to becoming a proxy but is not yet assigned. + Open(AccountId), + /// Account is actively being a proxy. + Active(AccountId), +} + +impl ProxyState { + fn as_active(self) -> Option { + match self { + ProxyState::Active(a) => Some(a), + ProxyState::Open(_) => None, + } + } +} + decl_storage! { trait Store for Module as Democracy { /// The number of (public) proposals that have been made so far. @@ -299,7 +317,7 @@ decl_storage! { /// Who is able to vote for whom. Value is the fund-holding account, key is the /// vote-transaction-sending account. - pub Proxy get(fn proxy): map hasher(blake2_256) T::AccountId => Option; + pub Proxy get(fn proxy): map hasher(blake2_256) T::AccountId => Option>; /// Get the account (and lock periods) to which another account is delegating vote. pub Delegations get(fn delegations): @@ -423,6 +441,12 @@ decl_error! { NotLocked, /// The lock on the account to be unlocked has not yet expired. NotExpired, + /// A proxy-pairing was attempted to an account that was not open. + NotOpen, + /// A proxy-pairing was attempted to an account that was open to another account. + WrongOpen, + /// A proxy-de-pairing was attempted to an account that was not active. + NotActive } } @@ -525,8 +549,9 @@ decl_module! { #[compact] ref_index: ReferendumIndex, vote: Vote ) -> DispatchResult { - let who = Self::proxy(ensure_signed(origin)?).ok_or(Error::::NotProxy)?; - Self::do_vote(who, ref_index, vote) + let who = ensure_signed(origin)?; + let voter = Self::proxy(who).and_then(|a| a.as_active()).ok_or(Error::::NotProxy)?; + Self::do_vote(voter, ref_index, vote) } /// Schedule an emergency cancellation of a referendum. Cannot happen twice to the same @@ -659,42 +684,65 @@ decl_module! { } } - /// Specify a proxy. Called by the stash. + /// Specify a proxy that is already open to us. Called by the stash. + /// + /// NOTE: Used to be called `set_proxy`. /// /// # /// - One extra DB entry. /// # #[weight = SimpleDispatchInfo::FixedNormal(100_000)] - fn set_proxy(origin, proxy: T::AccountId) { + fn activate_proxy(origin, proxy: T::AccountId) { let who = ensure_signed(origin)?; - ensure!(!>::contains_key(&proxy), Error::::AlreadyProxy); - >::insert(proxy, who) + Proxy::::try_mutate(&proxy, |a| match a.take() { + None => Err(Error::::NotOpen), + Some(ProxyState::Active(_)) => Err(Error::::AlreadyProxy), + Some(ProxyState::Open(x)) if &x == &who => { + *a = Some(ProxyState::Active(who)); + Ok(()) + } + Some(ProxyState::Open(_)) => Err(Error::::WrongOpen), + })?; } /// Clear the proxy. Called by the proxy. /// + /// NOTE: Used to be called `resign_proxy`. + /// /// # /// - One DB clear. /// # #[weight = SimpleDispatchInfo::FixedNormal(100_000)] - fn resign_proxy(origin) { + fn close_proxy(origin) { let who = ensure_signed(origin)?; - >::remove(who); + Proxy::::mutate(&who, |a| { + if a.is_some() { + system::Module::::dec_ref(&who); + } + *a = None; + }); } - /// Clear the proxy. Called by the stash. + /// Deactivate the proxy, but leave open to this account. Called by the stash. + /// + /// The proxy must already be active. + /// + /// NOTE: Used to be called `remove_proxy`. /// /// # /// - One DB clear. /// # #[weight = SimpleDispatchInfo::FixedNormal(100_000)] - fn remove_proxy(origin, proxy: T::AccountId) { + fn deactivate_proxy(origin, proxy: T::AccountId) { let who = ensure_signed(origin)?; - ensure!( - &Self::proxy(&proxy).ok_or(Error::::NotProxy)? == &who, - Error::::WrongProxy, - ); - >::remove(proxy); + Proxy::::try_mutate(&proxy, |a| match a.take() { + None | Some(ProxyState::Open(_)) => Err(Error::::NotActive), + Some(ProxyState::Active(x)) if &x == &who => { + *a = Some(ProxyState::Open(who)); + Ok(()) + } + Some(ProxyState::Active(_)) => Err(Error::::WrongProxy), + })?; } /// Delegate vote. @@ -818,6 +866,30 @@ decl_module! { Locks::::remove(&target); Self::deposit_event(RawEvent::Unlocked(target)); } + + /// Become a proxy. + /// + /// This must be called prior to a later `activate_proxy`. + /// + /// Origin must be a Signed. + /// + /// - `target`: The account whose votes will later be proxied. + /// + /// `close_proxy` must be called before the account can be destroyed. + /// + /// # + /// - One extra DB entry. + /// # + #[weight = SimpleDispatchInfo::FixedNormal(100_000)] + fn open_proxy(origin, target: T::AccountId) { + let who = ensure_signed(origin)?; + Proxy::::mutate(&who, |a| { + if a.is_none() { + system::Module::::inc_ref(&who); + } + *a = Some(ProxyState::Open(target)); + }); + } } } @@ -935,7 +1007,12 @@ impl Module { #[cfg(feature = "std")] pub fn force_proxy(stash: T::AccountId, proxy: T::AccountId) { - >::insert(proxy, stash) + Proxy::::mutate(&proxy, |o| { + if o.is_none() { + system::Module::::inc_ref(&proxy); + } + *o = Some(ProxyState::Active(stash)) + }) } /// Start a referendum. @@ -1162,12 +1239,6 @@ impl Module { } } -impl OnReapAccount for Module { - fn on_reap_account(who: &T::AccountId) { - >::remove(who) - } -} - #[cfg(test)] mod tests { use super::*; @@ -1229,7 +1300,7 @@ mod tests { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } parameter_types! { pub const ExistentialDeposit: u64 = 1; @@ -1968,29 +2039,45 @@ mod tests { fn proxy_should_work() { new_test_ext().execute_with(|| { assert_eq!(Democracy::proxy(10), None); - assert_ok!(Democracy::set_proxy(Origin::signed(1), 10)); - assert_eq!(Democracy::proxy(10), Some(1)); + assert!(System::allow_death(&10)); + + assert_noop!(Democracy::activate_proxy(Origin::signed(1), 10), Error::::NotOpen); + + assert_ok!(Democracy::open_proxy(Origin::signed(10), 1)); + assert!(!System::allow_death(&10)); + assert_eq!(Democracy::proxy(10), Some(ProxyState::Open(1))); + + assert_noop!(Democracy::activate_proxy(Origin::signed(2), 10), Error::::WrongOpen); + assert_ok!(Democracy::activate_proxy(Origin::signed(1), 10)); + assert_eq!(Democracy::proxy(10), Some(ProxyState::Active(1))); // Can't set when already set. - assert_noop!(Democracy::set_proxy(Origin::signed(2), 10), Error::::AlreadyProxy); + assert_noop!(Democracy::activate_proxy(Origin::signed(2), 10), Error::::AlreadyProxy); // But this works because 11 isn't proxying. - assert_ok!(Democracy::set_proxy(Origin::signed(2), 11)); - assert_eq!(Democracy::proxy(10), Some(1)); - assert_eq!(Democracy::proxy(11), Some(2)); + assert_ok!(Democracy::open_proxy(Origin::signed(11), 2)); + assert_ok!(Democracy::activate_proxy(Origin::signed(2), 11)); + assert_eq!(Democracy::proxy(10), Some(ProxyState::Active(1))); + assert_eq!(Democracy::proxy(11), Some(ProxyState::Active(2))); // 2 cannot fire 1's proxy: - assert_noop!(Democracy::remove_proxy(Origin::signed(2), 10), Error::::WrongProxy); + assert_noop!(Democracy::deactivate_proxy(Origin::signed(2), 10), Error::::WrongProxy); - // 1 fires his proxy: - assert_ok!(Democracy::remove_proxy(Origin::signed(1), 10)); - assert_eq!(Democracy::proxy(10), None); - assert_eq!(Democracy::proxy(11), Some(2)); + // 1 deactivates their proxy: + assert_ok!(Democracy::deactivate_proxy(Origin::signed(1), 10)); + assert_eq!(Democracy::proxy(10), Some(ProxyState::Open(1))); + // but the proxy account cannot be killed until the proxy is closed. + assert!(!System::allow_death(&10)); - // 11 resigns: - assert_ok!(Democracy::resign_proxy(Origin::signed(11))); + // and then 10 closes it completely: + assert_ok!(Democracy::close_proxy(Origin::signed(10))); assert_eq!(Democracy::proxy(10), None); + assert!(System::allow_death(&10)); + + // 11 just closes without 2's "permission". + assert_ok!(Democracy::close_proxy(Origin::signed(11))); assert_eq!(Democracy::proxy(11), None); + assert!(System::allow_death(&11)); }); } @@ -2002,7 +2089,8 @@ mod tests { fast_forward_to(2); let r = 0; - assert_ok!(Democracy::set_proxy(Origin::signed(1), 10)); + assert_ok!(Democracy::open_proxy(Origin::signed(10), 1)); + assert_ok!(Democracy::activate_proxy(Origin::signed(1), 10)); assert_ok!(Democracy::proxy_vote(Origin::signed(10), r, AYE)); assert_eq!(Democracy::voters_for(r), vec![1]); diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index f250e771216e6..a9474ae844141 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -816,7 +816,7 @@ mod tests { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } parameter_types! { diff --git a/frame/elections/src/mock.rs b/frame/elections/src/mock.rs index c5af6ba0456b9..b82e73d512aa6 100644 --- a/frame/elections/src/mock.rs +++ b/frame/elections/src/mock.rs @@ -56,7 +56,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } parameter_types! { diff --git a/frame/example-offchain-worker/src/tests.rs b/frame/example-offchain-worker/src/tests.rs index 6d6a82d8c9952..9b6a567a17840 100644 --- a/frame/example-offchain-worker/src/tests.rs +++ b/frame/example-offchain-worker/src/tests.rs @@ -50,10 +50,10 @@ parameter_types! { } impl frame_system::Trait for Test { type Origin = Origin; + type Call = (); type Index = u64; type BlockNumber = u64; type Hash = H256; - type Call = (); type Hashing = BlakeTwo256; type AccountId = sp_core::sr25519::Public; type Lookup = IdentityLookup; @@ -65,9 +65,9 @@ impl frame_system::Trait for Test { type AvailableBlockRatio = AvailableBlockRatio; type Version = (); type ModuleToIndex = (); - type OnReapAccount = (); - type OnNewAccount = (); type AccountData = (); + type OnNewAccount = (); + type OnKilledAccount = (); } type Extrinsic = TestXt, ()>; diff --git a/frame/example/src/lib.rs b/frame/example/src/lib.rs index 39f0d25d3232a..f4139a310a67a 100644 --- a/frame/example/src/lib.rs +++ b/frame/example/src/lib.rs @@ -690,7 +690,7 @@ mod tests { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = pallet_balances::Module; + type OnKilledAccount = (); } parameter_types! { pub const ExistentialDeposit: u64 = 1; diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index f58833440c2ff..d7fecf45909c8 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -469,7 +469,7 @@ mod tests { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } parameter_types! { pub const ExistentialDeposit: u64 = 1; @@ -573,7 +573,7 @@ mod tests { header: Header { parent_hash: [69u8; 32].into(), number: 1, - state_root: hex!("96797237079b6d6ffab7a47f90ee257a439a0e8268bdab3fe2f1e52572b101de").into(), + state_root: hex!("17caebd966d10cc6dc9659edf7fa3196511593f6c39f80f9b97cdbc3b0855cf3").into(), extrinsics_root: hex!("03170a2e7597b7b7e3d84c05391d139a62b157e78786d8c082f29dcf4c111314").into(), digest: Digest { logs: vec![], }, }, diff --git a/frame/finality-tracker/src/lib.rs b/frame/finality-tracker/src/lib.rs index ef6e0d9a4bbbe..08056a34ab028 100644 --- a/frame/finality-tracker/src/lib.rs +++ b/frame/finality-tracker/src/lib.rs @@ -263,7 +263,7 @@ mod tests { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } parameter_types! { pub const WindowSize: u64 = 11; diff --git a/frame/generic-asset/src/lib.rs b/frame/generic-asset/src/lib.rs index e98fc32ecbe9a..f1713dd586a93 100644 --- a/frame/generic-asset/src/lib.rs +++ b/frame/generic-asset/src/lib.rs @@ -1124,7 +1124,7 @@ impl frame_system::Trait for ElevatedTrait { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl Trait for ElevatedTrait { type Balance = T::Balance; diff --git a/frame/generic-asset/src/mock.rs b/frame/generic-asset/src/mock.rs index 7a61a61c430ac..8db140d90c666 100644 --- a/frame/generic-asset/src/mock.rs +++ b/frame/generic-asset/src/mock.rs @@ -64,7 +64,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl Trait for Test { diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 77a19c7626594..8b94becd5aa6a 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -67,7 +67,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } mod grandpa { diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 23a0620edff5f..68b7905961da8 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -936,7 +936,7 @@ mod tests { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } parameter_types! { pub const ExistentialDeposit: u64 = 1; diff --git a/frame/im-online/src/mock.rs b/frame/im-online/src/mock.rs index 7ee4c89ab46d3..a703b24629c0f 100644 --- a/frame/im-online/src/mock.rs +++ b/frame/im-online/src/mock.rs @@ -117,7 +117,7 @@ impl frame_system::Trait for Runtime { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } parameter_types! { diff --git a/frame/indices/src/mock.rs b/frame/indices/src/mock.rs index fe01b680bcc01..355b3cc792c94 100644 --- a/frame/indices/src/mock.rs +++ b/frame/indices/src/mock.rs @@ -67,7 +67,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } parameter_types! { diff --git a/frame/membership/src/lib.rs b/frame/membership/src/lib.rs index 1ad187b2ee35e..c39055c1bc928 100644 --- a/frame/membership/src/lib.rs +++ b/frame/membership/src/lib.rs @@ -271,7 +271,7 @@ mod tests { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } ord_parameter_types! { pub const One: u64 = 1; diff --git a/frame/nicks/src/lib.rs b/frame/nicks/src/lib.rs index 2b2d59014d8b2..caed6e40accb0 100644 --- a/frame/nicks/src/lib.rs +++ b/frame/nicks/src/lib.rs @@ -287,7 +287,7 @@ mod tests { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } parameter_types! { pub const ExistentialDeposit: u64 = 1; diff --git a/frame/offences/src/mock.rs b/frame/offences/src/mock.rs index f2e19b63f5a47..a003ad69157fc 100644 --- a/frame/offences/src/mock.rs +++ b/frame/offences/src/mock.rs @@ -91,7 +91,7 @@ impl frame_system::Trait for Runtime { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl Trait for Runtime { diff --git a/frame/randomness-collective-flip/src/lib.rs b/frame/randomness-collective-flip/src/lib.rs index 53d640688ef7d..0ded7dd6b0c64 100644 --- a/frame/randomness-collective-flip/src/lib.rs +++ b/frame/randomness-collective-flip/src/lib.rs @@ -193,7 +193,7 @@ mod tests { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } type System = frame_system::Module; diff --git a/frame/recovery/src/lib.rs b/frame/recovery/src/lib.rs index 6fa00af751e67..1b3c9416388f9 100644 --- a/frame/recovery/src/lib.rs +++ b/frame/recovery/src/lib.rs @@ -159,9 +159,8 @@ use codec::{Encode, Decode}; use frame_support::{ decl_module, decl_event, decl_storage, decl_error, ensure, - Parameter, RuntimeDebug, - weights::{GetDispatchInfo, SimpleDispatchInfo, FunctionOf}, - traits::{Currency, ReservableCurrency, Get, OnReapAccount, BalanceStatus}, + Parameter, RuntimeDebug, weights::{GetDispatchInfo, SimpleDispatchInfo, FunctionOf}, + traits::{Currency, ReservableCurrency, Get, BalanceStatus}, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -241,6 +240,7 @@ decl_storage! { pub Recoverable get(fn recovery_config): map hasher(blake2_256) T::AccountId => Option, T::AccountId>>; + /// Active recovery attempts. /// /// First account is the account to be recovered, and the second account @@ -248,10 +248,11 @@ decl_storage! { pub ActiveRecoveries get(fn active_recovery): double_map hasher(twox_64_concat) T::AccountId, hasher(twox_64_concat) T::AccountId => Option, T::AccountId>>; - /// The final list of recovered accounts. + + /// The list of allowed proxy accounts. /// - /// Map from the recovered account to the user who can access it. - pub Recovered get(fn recovered_account): + /// Map from the user who can access it to the recovered account. + pub Proxy get(fn proxy): map hasher(blake2_256) T::AccountId => Option; } } @@ -308,6 +309,8 @@ decl_error! { StillActive, /// There was an overflow in a calculation Overflow, + /// This account is already set up for recovery + AlreadyProxy, } } @@ -332,7 +335,7 @@ decl_module! { /// - One storage lookup to check account is recovered by `who`. O(1) /// # #[weight = FunctionOf( - |args: (&T::AccountId, &Box<::Call>)| args.1.get_dispatch_info().weight + 10_000, + |args: (&T::AccountId, &Box<::Call>)| args.1.get_dispatch_info().weight + 10_000, |args: (&T::AccountId, &Box<::Call>)| args.1.get_dispatch_info().class, true )] @@ -342,7 +345,8 @@ decl_module! { ) -> 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); + let target = Self::proxy(&who).ok_or(Error::::NotAllowed)?; + ensure!(&target == &account, Error::::NotAllowed); call.dispatch(frame_system::RawOrigin::Signed(account).into()) } @@ -363,7 +367,7 @@ decl_module! { fn set_recovered(origin, lost: T::AccountId, rescuer: T::AccountId) { ensure_root(origin)?; // Create the recovery storage item. - >::insert(&lost, &rescuer); + >::insert(&rescuer, &lost); Self::deposit_event(RawEvent::AccountRecovered(lost, rescuer)); } @@ -428,6 +432,7 @@ decl_module! { }; // Create the recovery configuration storage item >::insert(&who, recovery_config); + Self::deposit_event(RawEvent::RecoveryCreated(who)); } @@ -545,6 +550,7 @@ decl_module! { 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)?; + ensure!(!Proxy::::contains_key(&who), Error::::AlreadyProxy); // Make sure the delay period has passed let current_block_number = >::block_number(); let recoverable_block_number = active_recovery.created @@ -557,7 +563,8 @@ decl_module! { Error::::Threshold ); // Create the recovery storage item - >::insert(&account, &who); + Proxy::::insert(&who, &account); + system::Module::::inc_ref(&who); Self::deposit_event(RawEvent::AccountRecovered(account, who)); } @@ -592,7 +599,7 @@ decl_module! { Self::deposit_event(RawEvent::RecoveryClosed(who, rescuer)); } - /// Remove the recovery process for your account. + /// Remove the recovery process for your account. Recovered accounts are still accessible. /// /// NOTE: The user must make sure to call `close_recovery` on all active /// recovery attempts before calling this function else it will fail. @@ -621,10 +628,30 @@ decl_module! { ensure!(active_recoveries.next().is_none(), Error::::StillActive); // 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)); } + + /// Cancel the ability to use `as_recovered` for `account`. + /// + /// 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 are able to call on-behalf-of. + /// + /// # + /// - One storage mutation to check account is recovered by `who`. O(1) + /// # + fn cancel_recovered(origin, account: T::AccountId) { + let who = ensure_signed(origin)?; + // Check `who` is allowed to make a call on behalf of `account` + ensure!(Self::proxy(&who) == Some(account), Error::::NotAllowed); + Proxy::::remove(&who); + system::Module::::dec_ref(&who); + } } } @@ -639,11 +666,3 @@ impl Module { friends.binary_search(&friend).is_ok() } } - -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_reap_account(who: &T::AccountId) { - >::remove(who); - } -} diff --git a/frame/recovery/src/mock.rs b/frame/recovery/src/mock.rs index 97ee07bc6a2e9..a5b7731c2286a 100644 --- a/frame/recovery/src/mock.rs +++ b/frame/recovery/src/mock.rs @@ -80,7 +80,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = (Balances, Recovery); + type OnKilledAccount = (); } parameter_types! { diff --git a/frame/recovery/src/tests.rs b/frame/recovery/src/tests.rs index 97d4791cce508..9c644291c906d 100644 --- a/frame/recovery/src/tests.rs +++ b/frame/recovery/src/tests.rs @@ -31,7 +31,7 @@ use frame_support::{ fn basic_setup_works() { new_test_ext().execute_with(|| { // Nothing in storage to start - assert_eq!(Recovery::recovered_account(&1), None); + assert_eq!(Recovery::proxy(&2), None); assert_eq!(Recovery::active_recovery(&1, &2), None); assert_eq!(Recovery::recovery_config(&1), None); // Everyone should have starting balance of 100 @@ -91,10 +91,13 @@ fn recovery_life_cycle_works() { // All funds have been fully recovered! assert_eq!(Balances::free_balance(1), 200); assert_eq!(Balances::free_balance(5), 0); + // Remove the proxy link. + assert_ok!(Recovery::cancel_recovered(Origin::signed(1), 5)); + // All storage items are removed from the module assert!(!>::contains_key(&5, &1)); assert!(!>::contains_key(&5)); - assert!(!>::contains_key(&5)); + assert!(!>::contains_key(&1)); }); } @@ -335,7 +338,7 @@ fn claim_recovery_works() { // 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)); + assert_eq!(>::get(&1), Some(5)); // 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)); @@ -347,7 +350,7 @@ fn claim_recovery_works() { // 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)); + assert_eq!(>::get(&4), Some(5)); }); } diff --git a/frame/scored-pool/src/mock.rs b/frame/scored-pool/src/mock.rs index 90006da91e99a..a28b7891370ea 100644 --- a/frame/scored-pool/src/mock.rs +++ b/frame/scored-pool/src/mock.rs @@ -72,7 +72,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } impl pallet_balances::Trait for Test { diff --git a/frame/session/src/historical.rs b/frame/session/src/historical.rs index cf20f31360cb6..ced630e5fd8ae 100644 --- a/frame/session/src/historical.rs +++ b/frame/session/src/historical.rs @@ -318,7 +318,7 @@ mod tests { let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); crate::GenesisConfig:: { keys: NEXT_VALIDATORS.with(|l| - l.borrow().iter().cloned().map(|i| (i, UintAuthorityId(i).into())).collect() + l.borrow().iter().cloned().map(|i| (i, i, UintAuthorityId(i).into())).collect() ), }.assimilate_storage(&mut t).unwrap(); sp_io::TestExternalities::new(t) diff --git a/frame/session/src/lib.rs b/frame/session/src/lib.rs index 0abe06527b391..6340b79f0f579 100644 --- a/frame/session/src/lib.rs +++ b/frame/session/src/lib.rs @@ -105,8 +105,9 @@ use sp_runtime::{KeyTypeId, Perbill, RuntimeAppPublic, BoundToRuntimeAppPublic}; use frame_support::weights::SimpleDispatchInfo; use sp_runtime::traits::{Convert, Zero, Member, OpaqueKeys}; use sp_staking::SessionIndex; -use frame_support::{dispatch, ConsensusEngineId, decl_module, decl_event, decl_storage, decl_error}; -use frame_support::{ensure, traits::{OnReapAccount, Get, FindAuthor, ValidatorRegistration}, Parameter}; +use frame_support::{ensure, decl_module, decl_event, decl_storage, decl_error, ConsensusEngineId}; +use frame_support::{traits::{Get, FindAuthor, ValidatorRegistration}, Parameter}; +use frame_support::dispatch::{self, DispatchResult, DispatchError}; use frame_system::{self as system, ensure_signed}; #[cfg(test)] @@ -361,6 +362,7 @@ decl_storage! { /// /// The first key is always `DEDUP_KEY_PREFIX` to have all the data in the same branch of /// the trie. Having all data in the same branch should prevent slowing down other queries. + // TODO: Migrate to a normal map now https://github.com/paritytech/substrate/issues/4917 NextKeys: double_map hasher(twox_64_concat) Vec, hasher(blake2_256) T::ValidatorId => Option; @@ -368,11 +370,12 @@ decl_storage! { /// /// The first key is always `DEDUP_KEY_PREFIX` to have all the data in the same branch of /// the trie. Having all data in the same branch should prevent slowing down other queries. + // TODO: Migrate to a normal map now https://github.com/paritytech/substrate/issues/4917 KeyOwner: double_map hasher(twox_64_concat) Vec, hasher(blake2_256) (KeyTypeId, Vec) => Option; } add_extra_genesis { - config(keys): Vec<(T::ValidatorId, T::Keys)>; + config(keys): Vec<(T::AccountId, T::ValidatorId, T::Keys)>; build(|config: &GenesisConfig| { if T::SessionHandler::KEY_TYPE_IDS.len() != T::Keys::key_ids().len() { panic!("Number of keys in session handler and session keys does not match"); @@ -388,21 +391,17 @@ decl_storage! { } }); - for (who, keys) in config.keys.iter().cloned() { - assert!( - >::load_keys(&who).is_none(), - "genesis config contained duplicate validator {:?}", who, - ); - - >::do_set_keys(&who, keys) + for (account, val, keys) in config.keys.iter().cloned() { + >::inner_set_keys(&val, keys) .expect("genesis config must not contain duplicates; qed"); + system::Module::::inc_ref(&account); } let initial_validators_0 = T::SessionManager::new_session(0) .unwrap_or_else(|| { frame_support::print("No initial validator provided by `SessionManager`, use \ session config keys to generate initial validator set."); - config.keys.iter().map(|(ref v, _)| v.clone()).collect() + config.keys.iter().map(|x| x.1.clone()).collect() }); assert!(!initial_validators_0.is_empty(), "Empty validator set for session 0 in genesis block!"); @@ -445,6 +444,8 @@ decl_error! { NoAssociatedValidatorId, /// Registered duplicate key. DuplicatedKey, + /// No keys are associated with this account. + NoKeys, } } @@ -467,6 +468,8 @@ decl_module! { /// # /// - O(log n) in number of accounts. /// - One extra DB entry. + /// - Increases system account refs by one on success iff there were previously no keys set. + /// In this case, purge_keys will need to be called before the account can be removed. /// # #[weight = SimpleDispatchInfo::FixedNormal(150_000)] fn set_keys(origin, keys: T::Keys, proof: Vec) -> dispatch::DispatchResult { @@ -474,13 +477,27 @@ decl_module! { ensure!(keys.ownership_proof_is_valid(&proof), Error::::InvalidProof); - let who = T::ValidatorIdOf::convert(who).ok_or(Error::::NoAssociatedValidatorId)?; - Self::do_set_keys(&who, keys)?; Ok(()) } + /// Removes any session key(s) of the function caller. + /// This doesn't take effect until the next session. + /// + /// The dispatch origin of this function must be signed. + /// + /// # + /// - O(N) in number of key types. + /// - Removes N + 1 DB entries. + /// - Reduces system account refs by one on success. + /// # + #[weight = SimpleDispatchInfo::FixedNormal(150_000)] + fn purge_keys(origin) { + let who = ensure_signed(origin)?; + Self::do_purge_keys(&who)?; + } + /// Called when a block is initialized. Will rotate session if it is the last /// block of the current session. fn on_initialize(n: T::BlockNumber) { @@ -612,10 +629,30 @@ impl Module { Self::validators().iter().position(|i| i == c).map(Self::disable_index).ok_or(()) } - // perform the set_key operation, checking for duplicates. - // does not set `Changed`. - fn do_set_keys(who: &T::ValidatorId, keys: T::Keys) -> dispatch::DispatchResult { - let old_keys = Self::load_keys(&who); + /// Perform the set_key operation, checking for duplicates. Does not set `Changed`. + /// + /// This ensures that the reference counter in system is incremented appropriately and as such + /// must accept an account ID, rather than a validator ID. + fn do_set_keys(account: &T::AccountId, keys: T::Keys) -> dispatch::DispatchResult { + let who = T::ValidatorIdOf::convert(account.clone()) + .ok_or(Error::::NoAssociatedValidatorId)?; + + let old_keys = Self::inner_set_keys(&who, keys)?; + if old_keys.is_none() { + system::Module::::inc_ref(&account); + } + + Ok(()) + } + + /// Perform the set_key operation, checking for duplicates. Does not set `Changed`. + /// + /// The old keys for this validator are returned, or `None` if there were none. + /// + /// This does not ensure that the reference counter in system is incremented appropriately, it + /// must be done by the caller or the keys will be leaked in storage. + fn inner_set_keys(who: &T::ValidatorId, keys: T::Keys) -> Result, DispatchError> { + let old_keys = Self::load_keys(who); for id in T::Keys::key_ids() { let key = keys.get_raw(*id); @@ -634,21 +671,25 @@ impl Module { Self::clear_key_owner(*id, old); } - Self::put_key_owner(*id, key, &who); + Self::put_key_owner(*id, key, who); } - Self::put_keys(&who, &keys); - - Ok(()) + Self::put_keys(who, &keys); + Ok(old_keys) } - fn prune_dead_keys(who: &T::ValidatorId) { - if let Some(old_keys) = Self::take_keys(who) { - for id in T::Keys::key_ids() { - let key_data = old_keys.get_raw(*id); - Self::clear_key_owner(*id, key_data); - } + fn do_purge_keys(account: &T::AccountId) -> DispatchResult { + let who = T::ValidatorIdOf::convert(account.clone()) + .ok_or(Error::::NoAssociatedValidatorId)?; + + let old_keys = Self::take_keys(&who).ok_or(Error::::NoKeys)?; + for id in T::Keys::key_ids() { + let key_data = old_keys.get_raw(*id); + Self::clear_key_owner(*id, key_data); } + system::Module::::dec_ref(&account); + + Ok(()) } fn load_keys(v: &T::ValidatorId) -> Option { @@ -676,12 +717,6 @@ impl Module { } } -impl OnReapAccount for Module { - fn on_reap_account(who: &T::ValidatorId) { - Self::prune_dead_keys(who); - } -} - /// Wraps the author-scraping logic for consensus engines that can recover /// the canonical index of an author. This then transforms it into the /// registering account-ID of that session key index. @@ -716,7 +751,7 @@ mod tests { let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap(); GenesisConfig:: { keys: NEXT_VALIDATORS.with(|l| - l.borrow().iter().cloned().map(|i| (i, UintAuthorityId(i).into())).collect() + l.borrow().iter().cloned().map(|i| (i, i, UintAuthorityId(i).into())).collect() ), }.assimilate_storage(&mut t).unwrap(); sp_io::TestExternalities::new(t) @@ -754,7 +789,10 @@ mod tests { let id = DUMMY; assert_eq!(Session::key_owner(id, UintAuthorityId(1).get_raw(id)), Some(1)); - Session::on_reap_account(&1); + assert!(!System::allow_death(&1)); + assert_ok!(Session::purge_keys(Origin::signed(1))); + assert!(System::allow_death(&1)); + assert_eq!(Session::load_keys(&1), None); assert_eq!(Session::key_owner(id, UintAuthorityId(1).get_raw(id)), None); }) diff --git a/frame/session/src/mock.rs b/frame/session/src/mock.rs index 0c922670697aa..df858c8ed57ac 100644 --- a/frame/session/src/mock.rs +++ b/frame/session/src/mock.rs @@ -178,7 +178,7 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = Session; + type OnKilledAccount = (); } impl pallet_timestamp::Trait for Test { diff --git a/frame/society/src/mock.rs b/frame/society/src/mock.rs index 62be1aa9f1da3..158f139df5673 100644 --- a/frame/society/src/mock.rs +++ b/frame/society/src/mock.rs @@ -78,7 +78,7 @@ impl frame_system::Trait for Test { type Version = (); type ModuleToIndex = (); type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); type AccountData = pallet_balances::AccountData; } diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 3f84597912f53..ee708dabd3c6e 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -259,6 +259,7 @@ use codec::{HasCompact, Encode, Decode}; use frame_support::{ decl_module, decl_event, decl_storage, ensure, decl_error, weights::SimpleDispatchInfo, + dispatch::DispatchResult, traits::{ Currency, LockIdentifier, LockableCurrency, WithdrawReasons, OnUnbalanced, Imbalance, Get, Time @@ -282,7 +283,6 @@ use sp_runtime::{Serialize, Deserialize}; use frame_system::{self as system, ensure_signed, ensure_root}; use sp_phragmen::ExtendedBalance; -use frame_support::traits::OnReapAccount; const DEFAULT_MINIMUM_VALIDATOR_COUNT: u32 = 4; const MAX_NOMINATIONS: usize = 16; @@ -831,6 +831,8 @@ decl_error! { NoMoreChunks, /// Can not rebond without unlocking chunks. NoUnlockChunk, + /// Attempting to target a stash that still has funds. + FundedTarget, } } @@ -900,6 +902,8 @@ decl_module! { >::insert(&stash, &controller); >::insert(&stash, payee); + system::Module::::inc_ref(&stash); + let stash_balance = T::Currency::free_balance(&stash); let value = value.min(stash_balance); let item = StakingLedger { stash, total: value, active: value, unlocking: vec![] }; @@ -1013,10 +1017,10 @@ decl_module! { // portion to fall below existential deposit + will have no more unlocking chunks // left. We can now safely remove this. let stash = ledger.stash; + // remove all staking-related information. + Self::kill_stash(&stash)?; // remove the lock. T::Currency::remove_lock(STAKING_ID, &stash); - // remove all staking-related information. - Self::kill_stash(&stash); } else { // This was the consequence of a partial unbond. just update the ledger and move on. Self::update_ledger(&controller, &ledger); @@ -1187,10 +1191,11 @@ decl_module! { fn force_unstake(origin, stash: T::AccountId) { ensure_root(origin)?; + // remove all staking-related information. + Self::kill_stash(&stash)?; + // remove the lock. T::Currency::remove_lock(STAKING_ID, &stash); - // remove all staking-related information. - Self::kill_stash(&stash); } /// Force there to be a new era at the end of sessions indefinitely. @@ -1254,9 +1259,22 @@ decl_module! { ); let ledger = ledger.rebond(value); - Self::update_ledger(&controller, &ledger); } + + /// Remove all data structure concerning a staker/stash once its balance is zero. + /// This is essentially equivalent to `withdraw_unbonded` except it can be called by anyone + /// and the target `stash` must have no funds left. + /// + /// This can be called from any origin. + /// + /// - `stash`: The stash account to reap. Its balance must be zero. + fn reap_stash(_origin, stash: T::AccountId) { + Self::ensure_storage_upgraded(); + ensure!(T::Currency::total_balance(&stash).is_zero(), Error::::FundedTarget); + Self::kill_stash(&stash)?; + T::Currency::remove_lock(STAKING_ID, &stash); + } } } @@ -1585,18 +1603,22 @@ impl Module { /// /// Assumes storage is upgraded before calling. /// - /// This is called : - /// - Immediately when an account's balance falls below existential deposit. + /// This is called: /// - after a `withdraw_unbond()` call that frees all of a stash's bonded balance. - fn kill_stash(stash: &T::AccountId) { - if let Some(controller) = >::take(stash) { - >::remove(&controller); - } + /// - through `reap_stash()` if the balance has fallen to zero (through slashing). + fn kill_stash(stash: &T::AccountId) -> DispatchResult { + let controller = Bonded::::take(stash).ok_or(Error::::NotStash)?; + >::remove(&controller); + >::remove(stash); >::remove(stash); >::remove(stash); slashing::clear_stash_metadata::(stash); + + system::Module::::dec_ref(stash); + + Ok(()) } /// Add reward points to validators using their stash account ID. @@ -1676,13 +1698,6 @@ impl SessionManager> } } -impl OnReapAccount for Module { - fn on_reap_account(stash: &T::AccountId) { - Self::ensure_storage_upgraded(); - Self::kill_stash(stash); - } -} - /// Add reward points to block authors: /// * 20 points to the block producer for producing a (non-uncle) block in the relay chain, /// * 2 points to the block producer for each reference to a previously unreferenced uncle, and diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index c5f3ef250803a..22d662846356e 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -140,12 +140,12 @@ impl frame_system::Trait for Test { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = (Balances, Staking, Session); + type OnKilledAccount = (); } impl pallet_balances::Trait for Test { type Balance = Balance; - type Event = (); type DustRemoval = (); + type Event = (); type ExistentialDeposit = ExistentialDeposit; type AccountStore = System; } @@ -156,13 +156,13 @@ parameter_types! { pub const DisabledValidatorsThreshold: Perbill = Perbill::from_percent(25); } impl pallet_session::Trait for Test { - type SessionManager = pallet_session::historical::NoteHistoricalRoot; - type Keys = UintAuthorityId; - type ShouldEndSession = pallet_session::PeriodicSessions; - type SessionHandler = TestSessionHandler; type Event = (); type ValidatorId = AccountId; type ValidatorIdOf = crate::StashOf; + type ShouldEndSession = pallet_session::PeriodicSessions; + type SessionManager = pallet_session::historical::NoteHistoricalRoot; + type SessionHandler = TestSessionHandler; + type Keys = UintAuthorityId; type DisabledValidatorsThreshold = DisabledValidatorsThreshold; } @@ -300,22 +300,22 @@ impl ExtBuilder { let _ = pallet_balances::GenesisConfig::{ balances: vec![ - (1, 10 * balance_factor), - (2, 20 * balance_factor), - (3, 300 * balance_factor), - (4, 400 * balance_factor), - (10, balance_factor), - (11, balance_factor * 1000), - (20, balance_factor), - (21, balance_factor * 2000), - (30, balance_factor), - (31, balance_factor * 2000), - (40, balance_factor), - (41, balance_factor * 2000), - (100, 2000 * balance_factor), - (101, 2000 * balance_factor), - // This allow us to have a total_payout different from 0. - (999, 1_000_000_000_000), + (1, 10 * balance_factor), + (2, 20 * balance_factor), + (3, 300 * balance_factor), + (4, 400 * balance_factor), + (10, balance_factor), + (11, balance_factor * 1000), + (20, balance_factor), + (21, balance_factor * 2000), + (30, balance_factor), + (31, balance_factor * 2000), + (40, balance_factor), + (41, balance_factor * 2000), + (100, 2000 * balance_factor), + (101, 2000 * balance_factor), + // This allow us to have a total_payout different from 0. + (999, 1_000_000_000_000), ], }.assimilate_storage(&mut storage); @@ -346,7 +346,7 @@ impl ExtBuilder { }.assimilate_storage(&mut storage); let _ = pallet_session::GenesisConfig:: { - keys: validators.iter().map(|x| (*x, UintAuthorityId(*x))).collect(), + keys: validators.iter().map(|x| (*x, *x, UintAuthorityId(*x))).collect(), }.assimilate_storage(&mut storage); let mut ext = sp_io::TestExternalities::from(storage); diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 73215d6117776..d950c171ad18d 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -1456,6 +1456,9 @@ fn on_free_balance_zero_stash_removes_validator() { // Check total balance of stash assert_eq!(Balances::total_balance(&11), 0); + // Reap the stash + assert_ok!(Staking::reap_stash(Origin::NONE, 11)); + // Check storage items do not exist assert!(!>::contains_key(&10)); assert!(!>::contains_key(&11)); @@ -1509,6 +1512,9 @@ fn on_free_balance_zero_stash_removes_nominator() { // Check total balance of stash assert_eq!(Balances::total_balance(&11), 0); + // Reap the stash + assert_ok!(Staking::reap_stash(Origin::NONE, 11)); + // Check storage items do not exist assert!(!>::contains_key(&10)); assert!(!>::contains_key(&11)); @@ -2303,6 +2309,10 @@ fn garbage_collection_after_slashing() { // so we don't test those here. assert_eq!(Balances::free_balance(11), 0); + assert_eq!(Balances::total_balance(&11), 0); + + assert_ok!(Staking::reap_stash(Origin::NONE, 11)); + assert!(::SlashingSpans::get(&11).is_none()); assert_eq!(::SpanSlash::get(&(11, 0)).amount_slashed(), &0); }) diff --git a/frame/support/src/storage/migration.rs b/frame/support/src/storage/migration.rs index e8d58b46d4c4f..291dceb4ea111 100644 --- a/frame/support/src/storage/migration.rs +++ b/frame/support/src/storage/migration.rs @@ -70,6 +70,11 @@ impl Iterator for StorageIterator { } } +/// Get a particular value in storage by the `module`, the map's `item` name and the key `hash`. +pub fn have_storage_value(module: &[u8], item: &[u8], hash: &[u8]) -> bool { + get_storage_value::<()>(module, item, hash).is_some() +} + /// Get a particular value in storage by the `module`, the map's `item` name and the key `hash`. pub fn get_storage_value(module: &[u8], item: &[u8], hash: &[u8]) -> Option { let mut key = vec![0u8; 32 + hash.len()]; diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 4730e1955e77a..bd6895bda5bcd 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -195,9 +195,9 @@ pub trait OnNewAccount { /// The account with the given id was reaped. #[impl_trait_for_tuples::impl_for_tuples(30)] -pub trait OnReapAccount { +pub trait OnKilledAccount { /// The account with the given id was reaped. - fn on_reap_account(who: &AccountId); + fn on_killed_account(who: &AccountId); } /// A trait for finding the author of a block header based on the `PreRuntime` digests contained diff --git a/frame/system/benches/bench.rs b/frame/system/benches/bench.rs index 1e24cb2c05974..cfcaa6f64ac59 100644 --- a/frame/system/benches/bench.rs +++ b/frame/system/benches/bench.rs @@ -78,7 +78,7 @@ impl system::Trait for Runtime { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl module::Trait for Runtime { diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs index ec35fe3b60321..f89c9efbd2962 100644 --- a/frame/system/src/lib.rs +++ b/frame/system/src/lib.rs @@ -93,6 +93,7 @@ use serde::Serialize; use sp_std::prelude::*; #[cfg(any(feature = "std", test))] use sp_std::map; +use sp_std::convert::Infallible; use sp_std::marker::PhantomData; use sp_std::fmt::Debug; use sp_version::RuntimeVersion; @@ -112,9 +113,9 @@ use sp_runtime::{ use sp_core::{ChangesTrieConfiguration, storage::well_known_keys}; use frame_support::{ - decl_module, decl_event, decl_storage, decl_error, storage, Parameter, + decl_module, decl_event, decl_storage, decl_error, storage, Parameter, ensure, debug, traits::{ - Contains, Get, ModuleToIndex, OnNewAccount, OnReapAccount, IsDeadAccount, Happened, + Contains, Get, ModuleToIndex, OnNewAccount, OnKilledAccount, IsDeadAccount, Happened, StoredMap }, weights::{Weight, DispatchInfo, DispatchClass, SimpleDispatchInfo, FunctionOf}, @@ -219,7 +220,7 @@ pub trait Trait: 'static + Eq + Clone { /// A function that is invoked when an account has been determined to be dead. /// /// All resources should be cleaned up associated with the given account. - type OnReapAccount: OnReapAccount; + type OnKilledAccount: OnKilledAccount; } pub type DigestOf = generic::Digest<::Hash>; @@ -290,13 +291,29 @@ fn hash69 + Default>() -> T { /// which can't contain more than `u32::max_value()` items. type EventIndex = u32; +/// Type used to encode the number of references an account has. +pub type RefCount = u8; + +/// Information of an account. +#[derive(Clone, Eq, PartialEq, Default, RuntimeDebug, Encode, Decode)] +pub struct AccountInfo { + /// The number of transactions this account has sent. + pub nonce: Index, + /// The number of other modules that currently depend on this account's existence. The account + /// cannot be reaped until this is zero. + pub refcount: RefCount, + /// The additional data that belongs to this account. Used to store the balance(s) in a lot of + /// chains. + pub data: AccountData, +} + decl_storage! { trait Store for Module as System { /// The full account information for a particular account ID. // TODO: should be hasher(twox64_concat) - will need staged migration - // TODO: should not including T::Index (the nonce) // https://github.com/paritytech/substrate/issues/4917 - pub Account get(fn account): map hasher(blake2_256) T::AccountId => (T::Index, T::AccountData); + pub Account get(fn account): + map hasher(blake2_256) T::AccountId => AccountInfo; /// Total extrinsics count for the current block. ExtrinsicCount: Option; @@ -384,7 +401,7 @@ decl_event!( /// A new account was created. NewAccount(AccountId), /// An account was reaped. - ReapedAccount(AccountId), + KilledAccount(AccountId), } ); @@ -407,6 +424,11 @@ decl_error! { /// /// Either calling `Core_version` or decoding `RuntimeVersion` failed. FailedToExtractRuntimeVersion, + + /// Suicide called when the account has non-default composite data. + NonDefaultComposite, + /// There is a non-zero reference count preventing the account from being purged. + NonZeroRefCount } } @@ -517,6 +539,17 @@ decl_module! { ensure_root(origin)?; storage::unhashed::kill_prefix(&prefix); } + + /// Kill the sending account, assuming there are no references outstanding and the composite + /// data is equal to its default value. + #[weight = SimpleDispatchInfo::FixedOperational(25_000)] + fn suicide(origin) { + let who = ensure_signed(origin)?; + let account = Account::::get(&who); + ensure!(account.refcount == 0, Error::::NonZeroRefCount); + ensure!(account.data == T::AccountData::default(), Error::::NonDefaultComposite); + Account::::remove(who); + } } } @@ -637,13 +670,40 @@ impl Default for InitKind { } } +/// Reference status; can be either referenced or unreferenced. +pub enum RefStatus { + Referenced, + Unreferenced, +} + impl Module { /// Deposits an event into this block's event record. pub fn deposit_event(event: impl Into) { Self::deposit_event_indexed(&[], event.into()); } - /// Deposits an event into this block's event record adding this event + /// Increment the reference counter on an account. + pub fn inc_ref(who: &T::AccountId) { + Account::::mutate(who, |a| a.refcount = a.refcount.saturating_add(1)); + } + + /// Decrement the reference counter on an account. This *MUST* only be done once for every time + /// you called `inc_ref` on `who`. + pub fn dec_ref(who: &T::AccountId) { + Account::::mutate(who, |a| a.refcount = a.refcount.saturating_sub(1)); + } + + /// The number of outstanding references for the account `who`. + pub fn refs(who: &T::AccountId) -> RefCount { + Account::::get(who).refcount + } + + /// True if the account has no outstanding references. + pub fn allow_death(who: &T::AccountId) -> bool { + Account::::get(who).refcount == 0 + } + + /// Deposits an event into this block's event record adding this event /// to the corresponding topic indexes. /// /// This will update storage entries that correspond to the specified topics. @@ -855,12 +915,12 @@ impl Module { /// Retrieve the account transaction counter from storage. pub fn account_nonce(who: impl EncodeLike) -> T::Index { - Account::::get(who).0 + Account::::get(who).nonce } /// Increment a particular account's nonce by 1. pub fn inc_account_nonce(who: impl EncodeLike) { - Account::::mutate(who, |a| a.0 += T::Index::one()); + Account::::mutate(who, |a| a.nonce += T::Index::one()); } /// Note what the extrinsic data of the current extrinsic index is. If this @@ -912,18 +972,28 @@ impl Module { Self::deposit_event(RawEvent::NewAccount(who)); } - /// Kill the account and reap any related information. - pub fn kill_account(who: T::AccountId) { - if Account::::contains_key(&who) { - Account::::remove(&who); - Self::on_killed_account(who); - } - } - /// Do anything that needs to be done after an account has been killed. fn on_killed_account(who: T::AccountId) { - T::OnReapAccount::on_reap_account(&who); - Self::deposit_event(RawEvent::ReapedAccount(who)); + T::OnKilledAccount::on_killed_account(&who); + Self::deposit_event(RawEvent::KilledAccount(who)); + } + + /// Remove an account from storage. This should only be done when its refs are zero or you'll + /// get storage leaks in other modules. Nonetheless we assume that the calling logic knows best. + /// + /// This is a no-op if the account doesn't already exist. If it does then it will ensure + /// cleanups (those in `on_killed_account`) take place. + fn kill_account(who: &T::AccountId) { + if Account::::contains_key(who) { + let account = Account::::take(who); + if account.refcount > 0 { + debug::debug!( + target: "system", + "WARNING: Referenced account deleted. This is probably a bug." + ); + } + Module::::on_killed_account(who.clone()); + } } } @@ -939,7 +1009,7 @@ impl Happened for CallOnCreatedAccount { pub struct CallKillAccount(PhantomData); impl Happened for CallKillAccount { fn happened(who: &T::AccountId) { - Module::::kill_account(who.clone()); + Module::::kill_account(who) } } @@ -948,53 +1018,45 @@ impl Happened for CallKillAccount { // Anything more complex will need more sophisticated logic. impl StoredMap for Module { fn get(k: &T::AccountId) -> T::AccountData { - Account::::get(k).1 + Account::::get(k).data } fn is_explicit(k: &T::AccountId) -> bool { Account::::contains_key(k) } - fn insert(k: &T::AccountId, t: T::AccountData) { + fn insert(k: &T::AccountId, data: T::AccountData) { let existed = Account::::contains_key(k); - Account::::insert(k, (T::Index::default(), t)); + Account::::mutate(k, |a| a.data = data); if !existed { Self::on_created_account(k.clone()); } } fn remove(k: &T::AccountId) { - if Account::::contains_key(&k) { - Self::kill_account(k.clone()); - } + Self::kill_account(k) } fn mutate(k: &T::AccountId, f: impl FnOnce(&mut T::AccountData) -> R) -> R { let existed = Account::::contains_key(k); - let r = Account::::mutate(k, |a| f(&mut a.1)); + let r = Account::::mutate(k, |a| f(&mut a.data)); if !existed { Self::on_created_account(k.clone()); } r } fn mutate_exists(k: &T::AccountId, f: impl FnOnce(&mut Option) -> R) -> R { - let (existed, exists, r) = Account::::mutate_exists(k, |maybe_value| { - let existed = maybe_value.is_some(); - let (maybe_nonce, mut maybe_extra) = split_inner(maybe_value.take(), |v| v); - let r = f(&mut maybe_extra); - *maybe_value = maybe_extra.map(|extra| (maybe_nonce.unwrap_or_default(), extra)); - (existed, maybe_value.is_some(), r) - }); - if !existed && exists { - Self::on_created_account(k.clone()); - } else if existed && !exists { - Self::on_killed_account(k.clone()); - } - r + Self::try_mutate_exists(k, |x| -> Result { Ok(f(x)) }).expect("Infallible; qed") } fn try_mutate_exists(k: &T::AccountId, f: impl FnOnce(&mut Option) -> Result) -> Result { Account::::try_mutate_exists(k, |maybe_value| { let existed = maybe_value.is_some(); - let (maybe_nonce, mut maybe_extra) = split_inner(maybe_value.take(), |v| v); - f(&mut maybe_extra).map(|v| { - *maybe_value = maybe_extra.map(|extra| (maybe_nonce.unwrap_or_default(), extra)); - (existed, maybe_value.is_some(), v) + let (maybe_prefix, mut maybe_data) = split_inner( + maybe_value.take(), + |account| ((account.nonce, account.refcount), account.data) + ); + f(&mut maybe_data).map(|result| { + *maybe_value = maybe_data.map(|data| { + let (nonce, refcount) = maybe_prefix.unwrap_or_default(); + AccountInfo { nonce, refcount, data } + }); + (existed, maybe_value.is_some(), result) }) }).map(|(existed, exists, v)| { if !existed && exists { @@ -1213,17 +1275,18 @@ impl SignedExtension for CheckNonce { _info: Self::DispatchInfo, _len: usize, ) -> Result<(), TransactionValidityError> { - let (expected, extra) = Account::::get(who); - if self.0 != expected { + let mut account = Account::::get(who); + if self.0 != account.nonce { return Err( - if self.0 < expected { + if self.0 < account.nonce { InvalidTransaction::Stale } else { InvalidTransaction::Future }.into() ) } - Account::::insert(who, (expected + T::Index::one(), extra)); + account.nonce += T::Index::one(); + Account::::insert(who, account); Ok(()) } @@ -1235,13 +1298,13 @@ impl SignedExtension for CheckNonce { _len: usize, ) -> TransactionValidity { // check index - let (expected, _extra) = Account::::get(who); - if self.0 < expected { + let account = Account::::get(who); + if self.0 < account.nonce { return InvalidTransaction::Stale.into() } let provides = vec![Encode::encode(&(who, self.0))]; - let requires = if expected < self.0 { + let requires = if account.nonce < self.0 { vec![Encode::encode(&(who, self.0 - One::one()))] } else { vec![] @@ -1411,6 +1474,7 @@ impl Lookup for ChainContext { #[cfg(test)] mod tests { use super::*; + use sp_std::cell::RefCell; use sp_core::H256; use sp_runtime::{traits::{BlakeTwo256, IdentityLookup}, testing::Header, DispatchError}; use frame_support::{impl_outer_origin, parameter_types}; @@ -1437,6 +1501,15 @@ mod tests { }; } + thread_local!{ + pub static KILLED: RefCell> = RefCell::new(vec![]); + } + + pub struct RecordKilled; + impl OnKilledAccount for RecordKilled { + fn on_killed_account(who: &u64) { KILLED.with(|r| r.borrow_mut().push(*who)) } + } + impl Trait for Test { type Origin = Origin; type Call = (); @@ -1454,9 +1527,9 @@ mod tests { type MaximumBlockLength = MaximumBlockLength; type Version = Version; type ModuleToIndex = (); - type AccountData = (); + type AccountData = u32; type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = RecordKilled; } impl From> for u16 { @@ -1493,6 +1566,27 @@ mod tests { assert_eq!(x, Ok(RawOrigin::::Signed(1u64))); } + #[test] + fn stored_map_works() { + new_test_ext().execute_with(|| { + System::insert(&0, 42); + assert!(System::allow_death(&0)); + + System::inc_ref(&0); + assert!(!System::allow_death(&0)); + + System::insert(&0, 69); + assert!(!System::allow_death(&0)); + + System::dec_ref(&0); + assert!(System::allow_death(&0)); + + assert!(KILLED.with(|r| r.borrow().is_empty())); + System::kill_account(&0); + assert_eq!(KILLED.with(|r| r.borrow().clone()), vec![0u64]); + }); + } + #[test] fn deposit_event_should_work() { new_test_ext().execute_with(|| { @@ -1645,7 +1739,7 @@ mod tests { #[test] fn signed_ext_check_nonce_works() { new_test_ext().execute_with(|| { - Account::::insert(1, (1, ())); + Account::::insert(1, AccountInfo { nonce: 1, refcount: 0, data: 0 }); let info = DispatchInfo::default(); let len = 0_usize; // stale diff --git a/frame/system/src/offchain.rs b/frame/system/src/offchain.rs index 507092f626dcf..5abafe7655172 100644 --- a/frame/system/src/offchain.rs +++ b/frame/system/src/offchain.rs @@ -128,19 +128,20 @@ pub trait SignAndSubmitTransaction { fn sign_and_submit(call: impl Into, public: PublicOf) -> Result<(), ()> { let call = call.into(); let id = public.clone().into_account(); - let (expected_nonce, extra) = super::Account::::get(&id); + let mut account = super::Account::::get(&id); debug::native::debug!( target: "offchain", "Creating signed transaction from account: {:?} (nonce: {:?})", id, - expected_nonce, + account.nonce, ); let (call, signature_data) = Self::CreateTransaction - ::create_transaction::(call, public, id.clone(), expected_nonce) + ::create_transaction::(call, public, id.clone(), account.nonce) .ok_or(())?; // increment the nonce. This is fine, since the code should always // be running in off-chain context, so we NEVER persists data. - super::Account::::insert(&id, (expected_nonce + One::one(), extra)); + account.nonce += One::one(); + super::Account::::insert(&id, account); let xt = Self::Extrinsic::new(call, Some(signature_data)).ok_or(())?; sp_io::offchain::submit_transaction(xt.encode()) diff --git a/frame/timestamp/src/lib.rs b/frame/timestamp/src/lib.rs index de36b65851be3..1c3fec2112a08 100644 --- a/frame/timestamp/src/lib.rs +++ b/frame/timestamp/src/lib.rs @@ -278,7 +278,7 @@ mod tests { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } parameter_types! { pub const MinimumPeriod: u64 = 5; diff --git a/frame/transaction-payment/src/lib.rs b/frame/transaction-payment/src/lib.rs index 0d9cfc0b77683..67b3fa63ac07f 100644 --- a/frame/transaction-payment/src/lib.rs +++ b/frame/transaction-payment/src/lib.rs @@ -304,7 +304,7 @@ mod tests { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } parameter_types! { diff --git a/frame/treasury/src/lib.rs b/frame/treasury/src/lib.rs index 192dd1aff6116..bbf31cc599da0 100644 --- a/frame/treasury/src/lib.rs +++ b/frame/treasury/src/lib.rs @@ -760,7 +760,7 @@ mod tests { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } parameter_types! { pub const ExistentialDeposit: u64 = 1; diff --git a/frame/utility/src/lib.rs b/frame/utility/src/lib.rs index da099fcb4ad00..76f3ca6bf6261 100644 --- a/frame/utility/src/lib.rs +++ b/frame/utility/src/lib.rs @@ -258,7 +258,7 @@ decl_module! { /// - The weight of the `call` + 10,000. /// # #[weight = FunctionOf( - |args: (&u16, &Box<::Call>)| args.1.get_dispatch_info().weight + 10_000, + |args: (&u16, &Box<::Call>)| args.1.get_dispatch_info().weight + 10_000, |args: (&u16, &Box<::Call>)| args.1.get_dispatch_info().class, true )] @@ -410,7 +410,7 @@ decl_module! { #[weight = FunctionOf( |args: (&u16, &Vec, &Option>, &[u8; 32])| { 10_000 * (args.1.len() as u32 + 1) - }, + }, DispatchClass::Normal, true )] @@ -485,7 +485,7 @@ decl_module! { #[weight = FunctionOf( |args: (&u16, &Vec, &Timepoint, &[u8; 32])| { 10_000 * (args.1.len() as u32 + 1) - }, + }, DispatchClass::Normal, true )] @@ -624,7 +624,7 @@ mod tests { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } parameter_types! { pub const ExistentialDeposit: u64 = 1; diff --git a/frame/vesting/src/lib.rs b/frame/vesting/src/lib.rs index dd5125984280a..fdc480ddd0417 100644 --- a/frame/vesting/src/lib.rs +++ b/frame/vesting/src/lib.rs @@ -339,7 +339,7 @@ mod tests { type ModuleToIndex = (); type AccountData = pallet_balances::AccountData; type OnNewAccount = (); - type OnReapAccount = Balances; + type OnKilledAccount = (); } impl pallet_balances::Trait for Test { type Balance = u64; diff --git a/test-utils/runtime/src/lib.rs b/test-utils/runtime/src/lib.rs index 138e79cdd5c59..55e153103d905 100644 --- a/test-utils/runtime/src/lib.rs +++ b/test-utils/runtime/src/lib.rs @@ -374,7 +374,7 @@ impl frame_system::Trait for Runtime { type ModuleToIndex = (); type AccountData = (); type OnNewAccount = (); - type OnReapAccount = (); + type OnKilledAccount = (); } impl pallet_timestamp::Trait for Runtime {