Skip to content

XTokens refactor #706

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 27, 2022
Merged

XTokens refactor #706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e51155d
refactor
xlc Feb 27, 2022
b219134
merge check
xlc Feb 27, 2022
7bb244c
clippy
xlc Feb 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
198 changes: 51 additions & 147 deletions xtokens/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,44 +107,11 @@ pub mod module {
#[pallet::event]
#[pallet::generate_deposit(fn deposit_event)]
pub enum Event<T: Config> {
/// 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,
},
}
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -281,10 +249,6 @@ pub mod module {
) -> DispatchResult {
let who = ensure_signed(origin)?;
let dest: MultiLocation = (*dest).try_into().map_err(|()| Error::<T>::BadVersion)?;
// Zero fee is an error
if fee.is_zero() {
return Err(Error::<T>::FeeCannotBeZero.into());
}

Self::do_transfer_with_fee(who, currency_id, amount, fee, dest, dest_weight)
}
Expand Down Expand Up @@ -323,10 +287,6 @@ pub mod module {
let asset: MultiAsset = (*asset).try_into().map_err(|()| Error::<T>::BadVersion)?;
let fee: MultiAsset = (*fee).try_into().map_err(|()| Error::<T>::BadVersion)?;
let dest: MultiLocation = (*dest).try_into().map_err(|()| Error::<T>::BadVersion)?;
// Zero fee is an error
if fungible_amount(&fee).is_zero() {
return Err(Error::<T>::FeeCannotBeZero.into());
}

Self::do_transfer_multiasset_with_fee(who, asset, fee, dest, dest_weight)
}
Expand Down Expand Up @@ -392,7 +352,7 @@ pub mod module {
// We first grab the fee
let fee: &MultiAsset = assets.get(fee_item as usize).ok_or(Error::<T>::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)
}
}

Expand All @@ -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::<T>::NotCrossChainTransferableCurrency)?;
let location: MultiLocation =
T::CurrencyIdConvert::convert(currency_id).ok_or(Error::<T>::NotCrossChainTransferableCurrency)?;

ensure!(!amount.is_zero(), Error::<T>::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::<T>::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(
Expand All @@ -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::<T>::NotCrossChainTransferableCurrency)?;
let location: MultiLocation =
T::CurrencyIdConvert::convert(currency_id).ok_or(Error::<T>::NotCrossChainTransferableCurrency)?;

ensure!(!amount.is_zero(), Error::<T>::ZeroAmount);
ensure!(!fee.is_zero(), Error::<T>::ZeroFee);

let asset = (location.clone(), amount.into()).into();
let fee_asset: MultiAsset = (location, fee.into()).into();
Expand All @@ -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::<T>::TransferredWithFee {
sender: who,
currency_id,
fee,
amount,
dest,
});
Ok(())
Self::do_transfer_multiassets(who, assets, fee_asset, dest, dest_weight)
}

fn do_transfer_multiasset(
Expand All @@ -463,30 +404,7 @@ pub mod module {
dest: MultiLocation,
dest_weight: Weight,
) -> DispatchResult {
if !asset.is_fungible(None) {
return Err(Error::<T>::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::<T>::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(
Expand All @@ -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::<T>::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::<T>::TransferredMultiAssetWithFee {
sender: who,
asset,
fee,
dest,
});
Self::do_transfer_multiassets(who, assets, fee, dest, dest_weight)?;

Ok(())
}
Expand All @@ -538,6 +441,7 @@ pub mod module {
for (currency_id, amount) in &currencies {
let location: MultiLocation = T::CurrencyIdConvert::convert(currency_id.clone())
.ok_or(Error::<T>::NotCrossChainTransferableCurrency)?;
ensure!(!amount.is_zero(), Error::<T>::ZeroAmount);
// Push contains saturated addition, so we should be able to use it safely
assets.push((location, (*amount).into()).into())
}
Expand All @@ -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::<T>::TransferredMultiCurrencies {
sender: who,
currencies,
dest,
});
Ok(())
Self::do_transfer_multiassets(who, assets, fee, dest, dest_weight)
}

fn do_transfer_multiassets(
Expand All @@ -565,7 +462,6 @@ pub mod module {
fee: MultiAsset,
dest: MultiLocation,
dest_weight: Weight,
deposit_event: bool,
) -> DispatchResult {
ensure!(
assets.len() <= T::MaxAssetsForTransfer::get(),
Expand All @@ -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::<T>::AssetIndexNonExistent)?;
if !asset.is_fungible(None) {
return Err(Error::<T>::NotFungible.into());
}
if fungible_amount(asset).is_zero() {
return Ok(());
}
ensure!(
matches!(asset.fun, Fungibility::Fungible(x) if !x.is_zero()),
Error::<T>::InvalidAsset
);
ensure!(
fee.reserve() == asset.reserve(),
Error::<T>::DistinctReserveForAssetAndFee
Expand All @@ -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());
Expand All @@ -607,13 +512,12 @@ pub mod module {
Error::<T>::XcmExecutionFailed
})?;

if deposit_event {
Self::deposit_event(Event::<T>::TransferredMultiAssets {
sender: who,
assets,
dest,
});
}
Self::deposit_event(Event::<T>::TransferredMultiAssets {
sender: who,
assets,
fee,
dest,
});

Ok(())
}
Expand Down
53 changes: 52 additions & 1 deletion xtokens/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@ fn send_with_zero_fee_should_yield_an_error() {
),
40,
),
Error::<para::Runtime>::FeeCannotBeZero
Error::<para::Runtime>::ZeroFee
);
});
}
Expand Down Expand Up @@ -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::<para::Runtime>::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::<para::Runtime>::ZeroAmount
);
});

// TODO: should have more tests after https://github.com/paritytech/polkadot/issues/4996
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should have more test to ensure MultiAsset is valid (fungible & non zero amount) after paritytech/polkadot#4996

}