From e51155d87bd105c3b1dc9b624ebf39233761d03e Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Sun, 27 Feb 2022 18:08:08 +1300 Subject: [PATCH 1/3] refactor --- xtokens/src/lib.rs | 176 +++++++++++-------------------------------- xtokens/src/tests.rs | 53 ++++++++++++- 2 files changed, 94 insertions(+), 135 deletions(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index 64f3e1977..8d13e7122 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, }, } @@ -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) } } @@ -407,6 +367,8 @@ pub mod module { let location: MultiLocation = T::CurrencyIdConvert::convert(currency_id.clone()) .ok_or(Error::::NotCrossChainTransferableCurrency)?; + ensure!(!amount.is_zero(), Error::::ZeroAmount); + let asset: MultiAsset = (location, amount.into()).into(); Self::do_transfer_multiassets( who.clone(), @@ -414,16 +376,7 @@ pub mod module { asset, dest.clone(), dest_weight, - false, - )?; - - Self::deposit_event(Event::::Transferred { - sender: who, - currency_id, - amount, - dest, - }); - Ok(()) + ) } fn do_transfer_with_fee( @@ -437,6 +390,9 @@ pub mod module { let location: MultiLocation = T::CurrencyIdConvert::convert(currency_id.clone()) .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 +401,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 +410,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.clone(), vec![asset.clone()].into(), asset, dest, dest_weight) } fn do_transfer_multiasset_with_fee( @@ -496,27 +420,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(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.clone(), assets, fee, dest, dest_weight)?; Ok(()) } @@ -538,6 +447,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 +459,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 +468,6 @@ pub mod module { fee: MultiAsset, dest: MultiLocation, dest_weight: Weight, - deposit_event: bool, ) -> DispatchResult { ensure!( assets.len() <= T::MaxAssetsForTransfer::get(), @@ -575,12 +477,8 @@ 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!(asset.is_fungible(None), Error::::NotFungible); + ensure!(!fungible_amount(asset).is_zero(), Error::::ZeroAmount); ensure!( fee.reserve() == asset.reserve(), Error::::DistinctReserveForAssetAndFee @@ -589,13 +487,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 +516,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 +} From b219134c5e2758e1a329016a2fece18fa957e9a3 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Sun, 27 Feb 2022 18:15:15 +1300 Subject: [PATCH 2/3] merge check --- xtokens/src/lib.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index 8d13e7122..adea721ab 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -136,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 @@ -477,8 +477,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)?; - ensure!(asset.is_fungible(None), Error::::NotFungible); - ensure!(!fungible_amount(asset).is_zero(), Error::::ZeroAmount); + ensure!( + matches!(asset.fun, Fungibility::Fungible(x) if !x.is_zero()), + Error::::InvalidAsset + ); ensure!( fee.reserve() == asset.reserve(), Error::::DistinctReserveForAssetAndFee From 7bb244c4b68b5e647290be72c787e2cad7280ac4 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Sun, 27 Feb 2022 18:23:28 +1300 Subject: [PATCH 3/3] clippy --- xtokens/src/lib.rs | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index adea721ab..5a6fd3810 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -364,19 +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, - ) + Self::do_transfer_multiassets(who, vec![asset.clone()].into(), asset, dest, dest_weight) } fn do_transfer_with_fee( @@ -387,8 +381,8 @@ 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); @@ -410,7 +404,7 @@ pub mod module { dest: MultiLocation, dest_weight: Weight, ) -> DispatchResult { - Self::do_transfer_multiassets(who.clone(), vec![asset.clone()].into(), asset, dest, dest_weight) + Self::do_transfer_multiassets(who, vec![asset.clone()].into(), asset, dest, dest_weight) } fn do_transfer_multiasset_with_fee( @@ -422,10 +416,10 @@ pub mod module { ) -> DispatchResult { // 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, dest, dest_weight)?; + Self::do_transfer_multiassets(who, assets, fee, dest, dest_weight)?; Ok(()) }