diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 25c636a37d69d..433ca8e8b839e 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1860,7 +1860,6 @@ type Migrations = ( pallet_nomination_pools::migration::v2::MigrateToV2, pallet_alliance::migration::Migration, pallet_contracts::Migration, - pallet_assets::migration::v2::MigrateToV2, ); /// MMR helper types. diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 0252af1acf2f2..61791bf3ca4cf 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -66,11 +66,15 @@ impl, I: 'static> Pallet { pub(super) fn new_account( who: &T::AccountId, d: &mut AssetDetails>, - maybe_deposit: Option>, - ) -> Result>, DispatchError> { + maybe_deposit: Option<(&T::AccountId, DepositBalanceOf)>, + ) -> Result, DispatchError> { let accounts = d.accounts.checked_add(1).ok_or(ArithmeticError::Overflow)?; - let reason = if let Some(deposit) = maybe_deposit { - ExistenceReason::DepositHeld(deposit) + let reason = if let Some((depositor, deposit)) = maybe_deposit { + if depositor == who { + ExistenceReason::DepositHeld(deposit) + } else { + ExistenceReason::DepositFrom(depositor.clone(), deposit) + } } else if d.is_sufficient { frame_system::Pallet::::inc_sufficients(who); d.sufficients += 1; @@ -93,18 +97,19 @@ impl, I: 'static> Pallet { pub(super) fn dead_account( who: &T::AccountId, d: &mut AssetDetails>, - reason: &ExistenceReason>, + reason: &ExistenceReasonOf, force: bool, ) -> DeadConsequence { + use ExistenceReason::*; match *reason { - ExistenceReason::Consumer => frame_system::Pallet::::dec_consumers(who), - ExistenceReason::Sufficient => { + Consumer => frame_system::Pallet::::dec_consumers(who), + Sufficient => { d.sufficients = d.sufficients.saturating_sub(1); frame_system::Pallet::::dec_sufficients(who); }, - ExistenceReason::DepositRefunded => {}, - ExistenceReason::DepositHeld(_) if !force => return Keep, - ExistenceReason::DepositHeld(_) => {}, + DepositRefunded => {}, + DepositHeld(_) | DepositFrom(..) if !force => return Keep, + DepositHeld(_) | DepositFrom(..) => {}, } d.accounts = d.accounts.saturating_sub(1); Remove @@ -123,39 +128,30 @@ impl, I: 'static> Pallet { amount: T::Balance, increase_supply: bool, ) -> DepositConsequence { - use DepositConsequence::*; let details = match Asset::::get(id) { Some(details) => details, - None => return UnknownAsset, + None => return DepositConsequence::UnknownAsset, }; if increase_supply && details.supply.checked_add(&amount).is_none() { - return Overflow - } - // We don't allow freezing of accounts that don't already have an `(AssetId, AccountId)` - // entry. The account may have a zero balance provided by a deposit placed by `touch`ing the - // account. However, frozen accounts cannot receive more of the `AssetID`. - if let Some(a) = Account::::get(id, who) { - if a.is_frozen { - return Frozen - } + return DepositConsequence::Overflow } if let Some(balance) = Self::maybe_balance(id, who) { if balance.checked_add(&amount).is_none() { - return Overflow + return DepositConsequence::Overflow } } else { if amount < details.min_balance { - return BelowMinimum + return DepositConsequence::BelowMinimum } if !details.is_sufficient && !frame_system::Pallet::::can_inc_consumer(who) { - return CannotCreate + return DepositConsequence::CannotCreate } if details.is_sufficient && details.sufficients.checked_add(1).is_none() { - return Overflow + return DepositConsequence::Overflow } } - Success + DepositConsequence::Success } /// Return the consequence of a withdraw. @@ -321,7 +317,7 @@ impl, I: 'static> Pallet { let deposit = T::AssetAccountDeposit::get(); let mut details = Asset::::get(&id).ok_or(Error::::Unknown)?; ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); - let reason = Self::new_account(&who, &mut details, Some(deposit))?; + let reason = Self::new_account(&who, &mut details, Some((depositor, deposit)))?; T::Currency::reserve(&depositor, deposit)?; Asset::::insert(&id, details); Account::::insert( @@ -331,7 +327,6 @@ impl, I: 'static> Pallet { balance: Zero::zero(), is_frozen: false, reason, - depositor: Some(depositor.clone()), extra: T::Extra::default(), }, ); @@ -346,8 +341,8 @@ impl, I: 'static> Pallet { allow_burn: bool, ) -> DispatchResult { let mut account = Account::::get(id, &who).ok_or(Error::::NoDeposit)?; - let deposit = account.reason.take_deposit().ok_or(Error::::NoDeposit)?; - let depositor = account.depositor.take().unwrap_or(who.clone()); + let (depositor, deposit) = + account.reason.take_deposit(&who).ok_or(Error::::NoDeposit)?; let mut details = Asset::::get(&id).ok_or(Error::::Unknown)?; ensure!(details.status == AssetStatus::Live, Error::::AssetNotLive); ensure!(account.balance.is_zero() || allow_burn, Error::::WouldBurn); @@ -362,9 +357,9 @@ impl, I: 'static> Pallet { if let Remove = Self::dead_account(&who, &mut details, &account.reason, false) { Account::::remove(id, &who); } else { + debug_assert!(false, "refund did not result in dead account?!"); // deposit has been refunded, need to update `Account` Account::::insert(id, &who, account); - debug_assert!(false, "refund did not result in dead account?!"); } Asset::::insert(&id, details); // Executing a hook here is safe, since it is not in a `mutate`. @@ -434,7 +429,6 @@ impl, I: 'static> Pallet { *maybe_account = Some(AssetAccountOf:: { balance: amount, reason: Self::new_account(beneficiary, details, None)?, - depositor: None, is_frozen: false, extra: T::Extra::default(), }); @@ -631,7 +625,6 @@ impl, I: 'static> Pallet { balance: credit, is_frozen: false, reason: Self::new_account(dest, details, None)?, - depositor: None, extra: T::Extra::default(), }); }, @@ -969,7 +962,7 @@ impl, I: 'static> Pallet { /// Freeze an account `who`, preventing further reception or transfer of asset `id`. pub(super) fn do_freeze( - check_freezer: T::AccountId, + caller: T::AccountId, id: T::AssetId, who: T::AccountId, create: bool, @@ -979,11 +972,11 @@ impl, I: 'static> Pallet { d.status == AssetStatus::Live || d.status == AssetStatus::Frozen, Error::::AssetNotLive ); - ensure!(check_freezer == d.freezer, Error::::NoPermission); + ensure!(caller == d.freezer, Error::::NoPermission); if create && Account::::get(id, &who).is_none() { - // we create the account with zero balance, but the freezer must place a deposit - let _ = Self::do_touch(id, &who, &check_freezer)?; + // we create the account with zero balance, but the caller must place a deposit + let _ = Self::do_touch(id, &who, &caller)?; } Account::::try_mutate(id, &who, |maybe_account| -> DispatchResult { diff --git a/frame/assets/src/lib.rs b/frame/assets/src/lib.rs index d2cbd83211f2b..a19a7390af62b 100644 --- a/frame/assets/src/lib.rs +++ b/frame/assets/src/lib.rs @@ -1593,7 +1593,6 @@ pub mod pallet { /// /// - `id`: The identifier of the asset for the account holding a deposit. /// - `who`: The account to refund. - /// - `allow_burn`: If `true` then assets may be destroyed in order to complete the refund. /// /// Emits `Refunded` event when successful. #[pallet::call_index(30)] @@ -1602,11 +1601,10 @@ pub mod pallet { origin: OriginFor, id: T::AssetIdParameter, who: T::AccountId, - allow_burn: bool, ) -> DispatchResult { let origin = ensure_signed(origin)?; let id: T::AssetId = id.into(); - Self::do_refund(&origin, id, &who, allow_burn) + Self::do_refund(&origin, id, &who, false) } } } diff --git a/frame/assets/src/migration.rs b/frame/assets/src/migration.rs index 280a1a4322dd6..90400ef5bd1dc 100644 --- a/frame/assets/src/migration.rs +++ b/frame/assets/src/migration.rs @@ -131,129 +131,3 @@ pub mod v1 { } } } - -pub mod v2 { - use frame_support::{pallet_prelude::*, weights::Weight}; - - use super::*; - - #[derive(Decode)] - pub struct OldAssetAccount { - pub(super) balance: Balance, - pub(super) is_frozen: bool, - pub(super) reason: ExistenceReason, - pub(super) extra: Extra, - _phantom: sp_std::marker::PhantomData, - } - - impl - OldAssetAccount - { - fn migrate_to_v2( - self, - who: AccountId, - ) -> AssetAccount { - let depositor_account = match self.reason { - // any accounts that exist via deposit will have placed it themselves - ExistenceReason::DepositHeld(_) => Some(who), - _ => None, - }; - - AssetAccount { - balance: self.balance, - is_frozen: self.is_frozen, - reason: self.reason, - depositor: depositor_account, - extra: self.extra, - } - } - } - - // Migration to V2 possible from both V0 and V1. - pub struct MigrateToV2(sp_std::marker::PhantomData<(T, I)>); - impl, I: 'static> OnRuntimeUpgrade for MigrateToV2 { - fn on_runtime_upgrade() -> Weight { - // `current_version` is what we are migrating to - let current_version = Pallet::::current_storage_version(); - // `onchain_version` is what is already in place - let onchain_version = Pallet::::on_chain_storage_version(); - - // `Account`s are the same in v0 and v1, so we can migrate to v2 from either one. - // Of course the v1 migration should be applied for `Asset`s. - if (onchain_version == 0 || onchain_version == 1) && current_version == 2 { - let mut translated = 0u64; - - Account::::translate::< - OldAssetAccount, T::Extra, T::AccountId>, - _, - >(|_asset_id, account, old_value| { - translated.saturating_inc(); - Some(old_value.migrate_to_v2(account)) - }); - - // update the storage version of the pallet - StorageVersion::new(2).put::>(); - log::info!( - target: LOG_TARGET, - "Upgraded info for {} accounts, storage to version {:?}", - translated, - current_version - ); - T::DbWeight::get().reads_writes(translated + 1, translated + 1) - } else { - log::info!( - target: LOG_TARGET, - "Migration did not execute. This probably should be removed" - ); - T::DbWeight::get().reads(1) - } - } - - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, &'static str> { - frame_support::ensure!( - Pallet::::on_chain_storage_version() < 2, - "should not execute otherwise" - ); - let prev_count = Account::::iter().count(); - Ok((prev_count as u32).encode()) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(prev_count: Vec) -> Result<(), &'static str> { - let prev_count: u32 = Decode::decode(&mut prev_count.as_slice()).expect( - "the state parameter should be something that was generated by pre_upgrade", - ); - let post_count = Account::::iter().count() as u32; - assert_eq!( - prev_count, post_count, - "the number of asset accounts before and after the migration should be the same" - ); - - let current_version = Pallet::::current_storage_version(); - let onchain_version = Pallet::::on_chain_storage_version(); - - frame_support::ensure!(current_version == 2, "must_upgrade"); - assert_eq!( - current_version, onchain_version, - "after migration, the current_version and onchain_version should be the same" - ); - - Account::::iter().for_each(|(_asset, account, account_data)| match account_data - .reason - { - ExistenceReason::DepositHeld(_) => { - assert_eq!( - account_data.depositor, - Some(account), - "account depositor should be the account" - ); - }, - _ => { - assert_eq!(account_data.depositor, None, "account should have no depositor"); - }, - }); - Ok(()) - } - } -} diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index 88379ea01d1f3..0f5a2b16fec97 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -705,7 +705,7 @@ fn set_team_should_work() { } #[test] -fn transferring_to_frozen_account_should_not_work() { +fn transferring_from_frozen_account_should_not_work() { new_test_ext().execute_with(|| { assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1)); assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100)); @@ -713,9 +713,12 @@ fn transferring_to_frozen_account_should_not_work() { assert_eq!(Assets::balance(0, 1), 100); assert_eq!(Assets::balance(0, 2), 100); assert_ok!(Assets::freeze(RuntimeOrigin::signed(1), 0, 2)); - assert_noop!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 50), TokenError::Frozen); - assert_eq!(Assets::balance(0, 1), 100); - assert_eq!(Assets::balance(0, 2), 100); + // can transfer to `2` + assert_ok!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 50)); + // cannot transfer from `2` + assert_noop!(Assets::transfer(RuntimeOrigin::signed(2), 0, 1, 25), Error::::Frozen); + assert_eq!(Assets::balance(0, 1), 50); + assert_eq!(Assets::balance(0, 2), 150); }); } @@ -733,9 +736,12 @@ fn touching_and_freezing_account_with_zero_asset_balance_should_work() { assert_ok!(Assets::touch(RuntimeOrigin::signed(2), 0)); // now it can be frozen assert_ok!(Assets::freeze(RuntimeOrigin::signed(1), 0, 2)); - assert_noop!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 50), TokenError::Frozen); - assert_eq!(Assets::balance(0, 1), 100); - assert_eq!(Assets::balance(0, 2), 0); + // can transfer to `2` even though its frozen + assert_ok!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 50)); + // cannot transfer from `2` + assert_noop!(Assets::transfer(RuntimeOrigin::signed(2), 0, 1, 25), Error::::Frozen); + assert_eq!(Assets::balance(0, 1), 50); + assert_eq!(Assets::balance(0, 2), 50); }); } @@ -753,15 +759,20 @@ fn freeze_creating_works() { assert_ok!(Assets::freeze_creating(RuntimeOrigin::signed(1), 0, 2)); // `1` had to reserve `AssetAccountDeposit` to create `Account(id, 2)` assert_eq!(Balances::reserved_balance(&1), 10); - // cannot transfer to `2` - assert_noop!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 50), TokenError::Frozen); + // can transfer to `2` even though its frozen + assert_ok!(Assets::transfer(RuntimeOrigin::signed(1), 0, 2, 50),); + // cannot transfer from `2` + assert_noop!(Assets::transfer(RuntimeOrigin::signed(2), 0, 1, 25), Error::::Frozen); // asset balances unchanged - assert_eq!(Assets::balance(0, 1), 100); - assert_eq!(Assets::balance(0, 2), 0); + assert_eq!(Assets::balance(0, 1), 50); + assert_eq!(Assets::balance(0, 2), 50); // refund goes back to `1` assert_ok!(Assets::thaw(RuntimeOrigin::signed(1), 0, 2)); - assert_ok!(Assets::refund_other(RuntimeOrigin::signed(1), 0, 2, true)); - assert_eq!(Balances::reserved_balance(&1), 0); + assert_noop!( + Assets::refund_other(RuntimeOrigin::signed(1), 0, 2), + Error::::WouldBurn + ); + assert_eq!(Balances::reserved_balance(&1), 10); }); } @@ -788,21 +799,18 @@ fn cannot_refund_other_account_with_balance() { assert!(Account::::contains_key(0, &2)); // 4 is not the depositor, account holder, or admin assert_noop!( - Assets::refund_other(RuntimeOrigin::signed(4), 0, 2, true), + Assets::refund_other(RuntimeOrigin::signed(4), 0, 2), Error::::NoPermission ); // but 1 is the asset admin, ok. - assert_ok!(Assets::refund_other(RuntimeOrigin::signed(1), 0, 2, true)); + assert_ok!(Assets::refund_other(RuntimeOrigin::signed(1), 0, 2)); // ensure the account has actually died assert!(!Account::::contains_key(0, &2)); assert_ok!(Assets::freeze_creating(RuntimeOrigin::signed(1), 0, 3)); assert_eq!(Assets::balance(0, 3), 0); assert!(Account::::contains_key(0, &3)); - assert_noop!( - Assets::refund_other(RuntimeOrigin::signed(1), 0, 3, true), - Error::::Frozen - ); + assert_noop!(Assets::refund_other(RuntimeOrigin::signed(1), 0, 3), Error::::Frozen); }) } diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index 51372d088541d..4e18d389ca7c6 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -32,6 +32,8 @@ pub(super) type AssetAccountOf = AssetAccount< >::Extra, ::AccountId, >; +pub(super) type ExistenceReasonOf = + ExistenceReason, ::AccountId>; /// AssetStatus holds the current state of the asset. It could either be Live and available for use, /// or in a Destroying state. @@ -87,33 +89,41 @@ pub struct Approval { #[test] fn ensure_bool_decodes_to_consumer_or_sufficient() { - assert_eq!(false.encode(), ExistenceReason::<()>::Consumer.encode()); - assert_eq!(true.encode(), ExistenceReason::<()>::Sufficient.encode()); + assert_eq!(false.encode(), ExistenceReason::<(), ()>::Consumer.encode()); + assert_eq!(true.encode(), ExistenceReason::<(), ()>::Sufficient.encode()); } #[derive(Clone, Encode, Decode, Eq, PartialEq, RuntimeDebug, MaxEncodedLen, TypeInfo)] -pub enum ExistenceReason { +pub enum ExistenceReason { #[codec(index = 0)] Consumer, #[codec(index = 1)] Sufficient, + /// Deposit from the account owner. #[codec(index = 2)] DepositHeld(Balance), #[codec(index = 3)] DepositRefunded, + #[codec(index = 4)] + /// Deposit from some account id. + DepositFrom(AccountId, Balance), } -impl ExistenceReason { - pub(crate) fn take_deposit(&mut self) -> Option { - if !matches!(self, ExistenceReason::DepositHeld(_)) { +impl ExistenceReason +where + AccountId: Clone, +{ + /// Take the deposit if any. + /// `who` is the owner of the account and the deposit of `DepositHeld` variant. + pub(crate) fn take_deposit(&mut self, who: &AccountId) -> Option<(AccountId, Balance)> { + use ExistenceReason::*; + if !matches!(self, DepositHeld(_) | DepositFrom(..)) { return None } - if let ExistenceReason::DepositHeld(deposit) = - sp_std::mem::replace(self, ExistenceReason::DepositRefunded) - { - Some(deposit) - } else { - None + match sp_std::mem::replace(self, DepositRefunded) { + DepositHeld(deposit) => Some((who.clone(), deposit)), + DepositFrom(depositor, deposit) => Some((depositor, deposit)), + _ => None, } } } @@ -125,9 +135,7 @@ pub struct AssetAccount { /// Whether the account is frozen. pub(super) is_frozen: bool, /// The reason for the existence of the account. - pub(super) reason: ExistenceReason, - /// If the account exists because of `ExistenceReason::Deposit`, the depositor. - pub(super) depositor: Option, + pub(super) reason: ExistenceReason, /// Additional "sidecar" data, in case some other pallet wants to use this storage item. pub(super) extra: Extra, } diff --git a/frame/support/src/traits/tokens/misc.rs b/frame/support/src/traits/tokens/misc.rs index 9986a891e674a..75aef0e04ea65 100644 --- a/frame/support/src/traits/tokens/misc.rs +++ b/frame/support/src/traits/tokens/misc.rs @@ -139,8 +139,6 @@ pub enum DepositConsequence { /// with extremely small balance types or balances that approach the max value of the balance /// type. Overflow, - /// Account is frozen and cannot receive this asset. - Frozen, /// Account continued in existence. Success, } @@ -154,7 +152,6 @@ impl DepositConsequence { CannotCreate => TokenError::CannotCreate.into(), UnknownAsset => TokenError::UnknownAsset.into(), Overflow => ArithmeticError::Overflow.into(), - Frozen => TokenError::Frozen.into(), Success => return Ok(()), }) }