diff --git a/tokens/src/lib.rs b/tokens/src/lib.rs index 716cb6a4b..416fe5309 100644 --- a/tokens/src/lib.rs +++ b/tokens/src/lib.rs @@ -67,7 +67,7 @@ use orml_traits::{ arithmetic::{self, Signed}, currency::TransferAll, BalanceStatus, GetByKey, LockIdentifier, MultiCurrency, MultiCurrencyExtended, MultiLockableCurrency, - MultiReservableCurrency, OnDust, + MultiReservableCurrency, OnDust, Happened, }; mod imbalances; @@ -201,6 +201,12 @@ pub mod module { /// Handler to burn or transfer account's dust type OnDust: OnDust; + /// Handler for when an account was created + type OnNewTokenAccount: Happened<(Self::AccountId, Self::CurrencyId)>; + + /// Handler for when an account was created + type OnKilledTokenAccount: Happened<(Self::AccountId, Self::CurrencyId)>; + #[pallet::constant] type MaxLocks: Get; @@ -704,9 +710,11 @@ impl Pallet { // Ignore the result, because if it failed then there are remaining consumers, // and the account storage in frame_system shouldn't be reaped. let _ = frame_system::Pallet::::dec_providers(who); + T::OnKilledTokenAccount::happened(&(who.clone(), currency_id)); } else if !existed && exists { // if new, increase account provider frame_system::Pallet::::inc_providers(who); + T::OnNewTokenAccount::happened(&(who.clone(), currency_id)); } if let Some(endowed) = maybe_endowed { diff --git a/tokens/src/mock.rs b/tokens/src/mock.rs index 8935bd9e2..103dd1130 100644 --- a/tokens/src/mock.rs +++ b/tokens/src/mock.rs @@ -224,6 +224,43 @@ parameter_type_with_key! { }; } +thread_local! { + pub static CREATED: RefCell> = RefCell::new(vec![]); + pub static KILLED: RefCell> = RefCell::new(vec![]); +} + +pub struct TrackCreatedAccounts; +impl TrackCreatedAccounts { + pub fn accounts() -> Vec<(AccountId, CurrencyId)> { + CREATED.with(|accounts| accounts.borrow().clone()) + } + + pub fn reset() { + CREATED.with(|accounts| { accounts.replace(vec![]); }); + } +} +impl Happened<(AccountId, CurrencyId)> for TrackCreatedAccounts { + fn happened((who, currency): &(AccountId, CurrencyId)) { + CREATED.with(|accounts| { accounts.borrow_mut().push((who.clone(), *currency)); }); + } +} + +pub struct TrackKilledAccounts; +impl TrackKilledAccounts { + pub fn accounts() -> Vec<(AccountId, CurrencyId)> { + KILLED.with(|accounts| accounts.borrow().clone()) + } + + pub fn reset() { + KILLED.with(|accounts| { accounts.replace(vec![]); }); + } +} +impl Happened<(AccountId, CurrencyId)> for TrackKilledAccounts { + fn happened((who, currency): &(AccountId, CurrencyId)) { + KILLED.with(|accounts| { accounts.borrow_mut().push((who.clone(), *currency)); }); + } +} + parameter_types! { pub DustReceiver: AccountId = PalletId(*b"orml/dst").into_account(); pub MaxLocks: u32 = 2; @@ -238,6 +275,8 @@ impl Config for Runtime { type ExistentialDeposits = ExistentialDeposits; type OnDust = TransferDust; type MaxLocks = MaxLocks; + type OnNewTokenAccount = TrackCreatedAccounts; + type OnKilledTokenAccount = TrackKilledAccounts; type DustRemovalWhitelist = MockDustRemovalWhitelist; } pub type TreasuryCurrencyAdapter = ::Currency; @@ -299,6 +338,9 @@ impl ExtBuilder { .unwrap(); } + TrackCreatedAccounts::reset(); + TrackKilledAccounts::reset(); + let mut ext = sp_io::TestExternalities::new(t); ext.execute_with(|| System::set_block_number(1)); ext diff --git a/tokens/src/tests.rs b/tokens/src/tests.rs index 5fb3a37ef..4f14ab2d7 100644 --- a/tokens/src/tests.rs +++ b/tokens/src/tests.rs @@ -55,6 +55,7 @@ fn transfer_should_work() { Error::::ExistentialDeposit, ); assert_ok!(Tokens::transfer(Some(ALICE).into(), CHARLIE, DOT, 2)); + assert_eq!(TrackCreatedAccounts::accounts(), vec![(CHARLIE, DOT)]); // imply AllowDeath assert!(Accounts::::contains_key(ALICE, DOT)); @@ -131,6 +132,7 @@ fn transfer_all_allow_death_should_work() { assert!(Accounts::::contains_key(ALICE, DOT)); assert_eq!(Tokens::free_balance(DOT, &ALICE), 100); assert_ok!(Tokens::transfer_all(Some(ALICE).into(), CHARLIE, DOT, false)); + assert_eq!(TrackCreatedAccounts::accounts(), vec![(CHARLIE, DOT)]); System::assert_last_event(Event::Tokens(crate::Event::Transfer { currency_id: DOT, from: ALICE, @@ -139,6 +141,7 @@ fn transfer_all_allow_death_should_work() { })); assert!(!Accounts::::contains_key(ALICE, DOT)); assert_eq!(Tokens::free_balance(DOT, &ALICE), 0); + assert_eq!(TrackKilledAccounts::accounts(), vec![(ALICE, DOT)]); assert_ok!(Tokens::set_lock(ID_1, DOT, &BOB, 50)); assert_eq!(Tokens::accounts(&BOB, DOT).frozen, 50); @@ -176,6 +179,7 @@ fn force_transfer_should_work() { amount: 100, })); assert!(!Accounts::::contains_key(ALICE, DOT)); + assert_eq!(TrackKilledAccounts::accounts(), vec![(ALICE, DOT)]); assert_eq!(Tokens::free_balance(DOT, &ALICE), 0); assert_eq!(Tokens::free_balance(DOT, &BOB), 200); }); @@ -2419,3 +2423,22 @@ fn fungibles_mutate_convert_should_work() { ); }); } + +#[test] +fn lifecycle_callbacks_are_activated() { + ExtBuilder::default() + .build() + .execute_with(|| { + assert_ok!(Tokens::set_balance(RawOrigin::Root.into(), ALICE, DOT, 200, 0)); + assert_eq!(TrackCreatedAccounts::accounts(), vec![(ALICE, DOT)]); + + assert_ok!(Tokens::set_balance(RawOrigin::Root.into(), ALICE, BTC, 200, 0)); + assert_eq!(TrackCreatedAccounts::accounts(), vec![(ALICE, DOT), (ALICE, BTC)]); + + assert_ok!(Tokens::transfer_all(Some(ALICE).into(), CHARLIE, BTC, false)); + assert_eq!(TrackCreatedAccounts::accounts(), vec![ + (ALICE, DOT), (ALICE, BTC), (CHARLIE, BTC) + ]); + assert_eq!(TrackKilledAccounts::accounts(), vec![(ALICE, BTC)]); + }) +} diff --git a/traits/src/lib.rs b/traits/src/lib.rs index 9274c5a8e..bf3fe7b7b 100644 --- a/traits/src/lib.rs +++ b/traits/src/lib.rs @@ -14,7 +14,8 @@ use serde::{Deserialize, Serialize}; pub use auction::{Auction, AuctionHandler, AuctionInfo, OnNewBidResult}; pub use currency::{ BalanceStatus, BasicCurrency, BasicCurrencyExtended, BasicLockableCurrency, BasicReservableCurrency, - LockIdentifier, MultiCurrency, MultiCurrencyExtended, MultiLockableCurrency, MultiReservableCurrency, OnDust, + LockIdentifier, MultiCurrency, MultiCurrencyExtended, MultiLockableCurrency, MultiReservableCurrency, + OnDust, }; pub use data_provider::{DataFeeder, DataProvider, DataProviderExtended}; pub use get_by_key::GetByKey; diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index 64f3e1977..5a6fd3810 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -107,44 +107,11 @@ pub mod module { #[pallet::event] #[pallet::generate_deposit(fn deposit_event)] pub enum Event { - /// Transferred. - Transferred { - sender: T::AccountId, - currency_id: T::CurrencyId, - amount: T::Balance, - dest: MultiLocation, - }, - /// Transferred with fee. - TransferredWithFee { - sender: T::AccountId, - currency_id: T::CurrencyId, - amount: T::Balance, - fee: T::Balance, - dest: MultiLocation, - }, - /// Transferred `MultiAsset`. - TransferredMultiAsset { - sender: T::AccountId, - asset: MultiAsset, - dest: MultiLocation, - }, - /// Transferred `MultiAsset` with fee. - TransferredMultiAssetWithFee { - sender: T::AccountId, - asset: MultiAsset, - fee: MultiAsset, - dest: MultiLocation, - }, - /// Transferred `MultiAsset` with fee. - TransferredMultiCurrencies { - sender: T::AccountId, - currencies: Vec<(T::CurrencyId, T::Balance)>, - dest: MultiLocation, - }, /// Transferred `MultiAsset` with fee. TransferredMultiAssets { sender: T::AccountId, assets: MultiAssets, + fee: MultiAsset, dest: MultiLocation, }, } @@ -169,8 +136,8 @@ pub mod module { CannotReanchor, /// Could not get ancestry of asset reserve location. InvalidAncestry, - /// Not fungible asset. - NotFungible, + /// The MultiAsset is invalid. + InvalidAsset, /// The destination `MultiLocation` provided cannot be inverted. DestinationNotInvertible, /// The version of the `Versioned` value used is not able to be @@ -179,9 +146,10 @@ pub mod module { /// We tried sending distinct asset and fee but they have different /// reserve chains DistinctReserveForAssetAndFee, - /// The fee amount was zero when the fee specification extrinsic is - /// being used. - FeeCannotBeZero, + /// The fee is zero. + ZeroFee, + /// The transfering asset amount is zero. + ZeroAmount, /// The number of assets to be sent is over the maximum TooManyAssetsBeingSent, /// The specified index does not exist in a MultiAssets struct @@ -281,10 +249,6 @@ pub mod module { ) -> DispatchResult { let who = ensure_signed(origin)?; let dest: MultiLocation = (*dest).try_into().map_err(|()| Error::::BadVersion)?; - // Zero fee is an error - if fee.is_zero() { - return Err(Error::::FeeCannotBeZero.into()); - } Self::do_transfer_with_fee(who, currency_id, amount, fee, dest, dest_weight) } @@ -323,10 +287,6 @@ pub mod module { let asset: MultiAsset = (*asset).try_into().map_err(|()| Error::::BadVersion)?; let fee: MultiAsset = (*fee).try_into().map_err(|()| Error::::BadVersion)?; let dest: MultiLocation = (*dest).try_into().map_err(|()| Error::::BadVersion)?; - // Zero fee is an error - if fungible_amount(&fee).is_zero() { - return Err(Error::::FeeCannotBeZero.into()); - } Self::do_transfer_multiasset_with_fee(who, asset, fee, dest, dest_weight) } @@ -392,7 +352,7 @@ pub mod module { // We first grab the fee let fee: &MultiAsset = assets.get(fee_item as usize).ok_or(Error::::AssetIndexNonExistent)?; - Self::do_transfer_multiassets(who, assets.clone(), fee.clone(), dest, dest_weight, true) + Self::do_transfer_multiassets(who, assets.clone(), fee.clone(), dest, dest_weight) } } @@ -404,26 +364,13 @@ pub mod module { dest: MultiLocation, dest_weight: Weight, ) -> DispatchResult { - let location: MultiLocation = T::CurrencyIdConvert::convert(currency_id.clone()) - .ok_or(Error::::NotCrossChainTransferableCurrency)?; + let location: MultiLocation = + T::CurrencyIdConvert::convert(currency_id).ok_or(Error::::NotCrossChainTransferableCurrency)?; + + ensure!(!amount.is_zero(), Error::::ZeroAmount); let asset: MultiAsset = (location, amount.into()).into(); - Self::do_transfer_multiassets( - who.clone(), - vec![asset.clone()].into(), - asset, - dest.clone(), - dest_weight, - false, - )?; - - Self::deposit_event(Event::::Transferred { - sender: who, - currency_id, - amount, - dest, - }); - Ok(()) + Self::do_transfer_multiassets(who, vec![asset.clone()].into(), asset, dest, dest_weight) } fn do_transfer_with_fee( @@ -434,8 +381,11 @@ pub mod module { dest: MultiLocation, dest_weight: Weight, ) -> DispatchResult { - let location: MultiLocation = T::CurrencyIdConvert::convert(currency_id.clone()) - .ok_or(Error::::NotCrossChainTransferableCurrency)?; + let location: MultiLocation = + T::CurrencyIdConvert::convert(currency_id).ok_or(Error::::NotCrossChainTransferableCurrency)?; + + ensure!(!amount.is_zero(), Error::::ZeroAmount); + ensure!(!fee.is_zero(), Error::::ZeroFee); let asset = (location.clone(), amount.into()).into(); let fee_asset: MultiAsset = (location, fee.into()).into(); @@ -445,16 +395,7 @@ pub mod module { assets.push(asset); assets.push(fee_asset.clone()); - Self::do_transfer_multiassets(who.clone(), assets, fee_asset, dest.clone(), dest_weight, false)?; - - Self::deposit_event(Event::::TransferredWithFee { - sender: who, - currency_id, - fee, - amount, - dest, - }); - Ok(()) + Self::do_transfer_multiassets(who, assets, fee_asset, dest, dest_weight) } fn do_transfer_multiasset( @@ -463,30 +404,7 @@ pub mod module { dest: MultiLocation, dest_weight: Weight, ) -> DispatchResult { - if !asset.is_fungible(None) { - return Err(Error::::NotFungible.into()); - } - - if fungible_amount(&asset).is_zero() { - return Ok(()); - } - - Self::do_transfer_multiassets( - who.clone(), - vec![asset.clone()].into(), - asset.clone(), - dest.clone(), - dest_weight, - false, - )?; - - Self::deposit_event(Event::::TransferredMultiAsset { - sender: who, - asset, - dest, - }); - - Ok(()) + Self::do_transfer_multiassets(who, vec![asset.clone()].into(), asset, dest, dest_weight) } fn do_transfer_multiasset_with_fee( @@ -496,27 +414,12 @@ pub mod module { dest: MultiLocation, dest_weight: Weight, ) -> DispatchResult { - if !asset.is_fungible(None) || !fee.is_fungible(None) { - return Err(Error::::NotFungible.into()); - } - - if fungible_amount(&asset).is_zero() { - return Ok(()); - } - // Push contains saturated addition, so we should be able to use it safely let mut assets = MultiAssets::new(); - assets.push(asset.clone()); + assets.push(asset); assets.push(fee.clone()); - Self::do_transfer_multiassets(who.clone(), assets, fee.clone(), dest.clone(), dest_weight, false)?; - - Self::deposit_event(Event::::TransferredMultiAssetWithFee { - sender: who, - asset, - fee, - dest, - }); + Self::do_transfer_multiassets(who, assets, fee, dest, dest_weight)?; Ok(()) } @@ -538,6 +441,7 @@ pub mod module { for (currency_id, amount) in ¤cies { let location: MultiLocation = T::CurrencyIdConvert::convert(currency_id.clone()) .ok_or(Error::::NotCrossChainTransferableCurrency)?; + ensure!(!amount.is_zero(), Error::::ZeroAmount); // Push contains saturated addition, so we should be able to use it safely assets.push((location, (*amount).into()).into()) } @@ -549,14 +453,7 @@ pub mod module { let fee: MultiAsset = (fee_location, (*fee_amount).into()).into(); - Self::do_transfer_multiassets(who.clone(), assets, fee, dest.clone(), dest_weight, false)?; - - Self::deposit_event(Event::::TransferredMultiCurrencies { - sender: who, - currencies, - dest, - }); - Ok(()) + Self::do_transfer_multiassets(who, assets, fee, dest, dest_weight) } fn do_transfer_multiassets( @@ -565,7 +462,6 @@ pub mod module { fee: MultiAsset, dest: MultiLocation, dest_weight: Weight, - deposit_event: bool, ) -> DispatchResult { ensure!( assets.len() <= T::MaxAssetsForTransfer::get(), @@ -575,12 +471,10 @@ pub mod module { // We check that all assets are valid and share the same reserve for i in 0..assets.len() { let asset = assets.get(i).ok_or(Error::::AssetIndexNonExistent)?; - if !asset.is_fungible(None) { - return Err(Error::::NotFungible.into()); - } - if fungible_amount(asset).is_zero() { - return Ok(()); - } + ensure!( + matches!(asset.fun, Fungibility::Fungible(x) if !x.is_zero()), + Error::::InvalidAsset + ); ensure!( fee.reserve() == asset.reserve(), Error::::DistinctReserveForAssetAndFee @@ -589,13 +483,24 @@ pub mod module { let (transfer_kind, dest, reserve, recipient) = Self::transfer_kind(&fee, &dest)?; let mut msg = match transfer_kind { - SelfReserveAsset => { - Self::transfer_self_reserve_asset(assets.clone(), fee, dest.clone(), recipient, dest_weight)? - } - ToReserve => Self::transfer_to_reserve(assets.clone(), fee, dest.clone(), recipient, dest_weight)?, - ToNonReserve => { - Self::transfer_to_non_reserve(assets.clone(), fee, reserve, dest.clone(), recipient, dest_weight)? + SelfReserveAsset => Self::transfer_self_reserve_asset( + assets.clone(), + fee.clone(), + dest.clone(), + recipient, + dest_weight, + )?, + ToReserve => { + Self::transfer_to_reserve(assets.clone(), fee.clone(), dest.clone(), recipient, dest_weight)? } + ToNonReserve => Self::transfer_to_non_reserve( + assets.clone(), + fee.clone(), + reserve, + dest.clone(), + recipient, + dest_weight, + )?, }; let origin_location = T::AccountIdToMultiLocation::convert(who.clone()); @@ -607,13 +512,12 @@ pub mod module { Error::::XcmExecutionFailed })?; - if deposit_event { - Self::deposit_event(Event::::TransferredMultiAssets { - sender: who, - assets, - dest, - }); - } + Self::deposit_event(Event::::TransferredMultiAssets { + sender: who, + assets, + fee, + dest, + }); Ok(()) } diff --git a/xtokens/src/tests.rs b/xtokens/src/tests.rs index 4e835d225..df32eefc8 100644 --- a/xtokens/src/tests.rs +++ b/xtokens/src/tests.rs @@ -847,7 +847,7 @@ fn send_with_zero_fee_should_yield_an_error() { ), 40, ), - Error::::FeeCannotBeZero + Error::::ZeroFee ); }); } @@ -1048,3 +1048,54 @@ fn specifying_a_non_existent_asset_index_should_fail() { ); }); } + +#[test] +fn send_with_zero_amount() { + TestNet::reset(); + + ParaA::execute_with(|| { + assert_noop!( + ParaXTokens::transfer( + Some(ALICE).into(), + CurrencyId::B, + 0, + Box::new( + ( + Parent, + Parachain(2), + Junction::AccountId32 { + network: NetworkId::Any, + id: BOB.into(), + }, + ) + .into() + ), + 40, + ), + Error::::ZeroAmount + ); + + assert_noop!( + ParaXTokens::transfer_multicurrencies( + Some(ALICE).into(), + vec![(CurrencyId::B, 0), (CurrencyId::B1, 50)], + 1, + Box::new( + ( + Parent, + Parachain(2), + Junction::AccountId32 { + network: NetworkId::Any, + id: BOB.into(), + }, + ) + .into() + ), + 40, + ), + Error::::ZeroAmount + ); + }); + + // TODO: should have more tests after https://github.com/paritytech/polkadot/issues/4996 +}