From b2ba19639144a90b135d38812a7f7a9228b51540 Mon Sep 17 00:00:00 2001 From: gorka Date: Mon, 15 Nov 2021 15:35:48 +0100 Subject: [PATCH 01/16] Start trying to separate fee from asset --- xtokens/src/lib.rs | 226 ++++++++++++++++++++++++++++++++++++++ xtokens/src/mock/relay.rs | 8 ++ xtokens/src/tests.rs | 36 ++++++ 3 files changed, 270 insertions(+) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index b75dbf8cf..b52622841 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -134,6 +134,8 @@ pub mod module { /// The version of the `Versioned` value used is not able to be /// interpreted. BadVersion, + /// The fee MultiAsset is of different type than the asset to transfer + DistincAssetAndFeeId, } #[pallet::hooks] @@ -170,6 +172,33 @@ pub mod module { Self::do_transfer(who, currency_id, amount, dest, dest_weight) } + /// Transfer native currencies specifying the fee. + /// + /// `dest_weight` is the weight for XCM execution on the dest chain, and + /// it would be charged from the transferred assets. If set below + /// requirements, the execution may fail and assets wouldn't be + /// received. + /// + /// It's a no-op if any error on local XCM execution or message sending. + /// Note sending assets out per se doesn't guarantee they would be + /// received. Receiving depends on if the XCM message could be delivered + /// by the network, and if the receiving chain would handle + /// messages correctly. + #[pallet::weight(Pallet::::weight_of_transfer(currency_id.clone(), *amount, dest))] + #[transactional] + pub fn transfer_with_fee( + origin: OriginFor, + currency_id: T::CurrencyId, + amount: T::Balance, + fee: T::Balance, + dest: Box, + dest_weight: Weight, + ) -> DispatchResult { + let who = ensure_signed(origin)?; + let dest: MultiLocation = (*dest).try_into().map_err(|()| Error::::BadVersion)?; + Self::do_transfer_with_fee(who, currency_id, amount, fee, dest, dest_weight) + } + /// Transfer `MultiAsset`. /// /// `dest_weight` is the weight for XCM execution on the dest chain, and @@ -195,6 +224,35 @@ pub mod module { let dest: MultiLocation = (*dest).try_into().map_err(|()| Error::::BadVersion)?; Self::do_transfer_multiasset(who, asset, dest, dest_weight, true) } + + /// Transfer `MultiAsset` specifying the fee. + /// + /// `dest_weight` is the weight for XCM execution on the dest chain, and + /// it would be charged from the transferred assets. If set below + /// requirements, the execution may fail and assets wouldn't be + /// received. + /// + /// It's a no-op if any error on local XCM execution or message sending. + /// Note sending assets out per se doesn't guarantee they would be + /// received. Receiving depends on if the XCM message could be delivered + /// by the network, and if the receiving chain would handle + /// messages correctly. + #[pallet::weight(Pallet::::weight_of_transfer_multiasset(asset, dest))] + #[transactional] + pub fn transfer_multiasset_with_fee( + origin: OriginFor, + asset: Box, + fee: Box, + dest: Box, + dest_weight: Weight, + ) -> DispatchResult { + let who = ensure_signed(origin)?; + 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)?; + + Self::do_transfer_multiasset_with_fee(who, asset, fee, dest, dest_weight, true) + } } impl Pallet { @@ -215,6 +273,25 @@ pub mod module { Ok(()) } + fn do_transfer_with_fee( + who: T::AccountId, + currency_id: T::CurrencyId, + amount: T::Balance, + fee: T::Balance, + dest: MultiLocation, + dest_weight: Weight, + ) -> DispatchResult { + let location: MultiLocation = T::CurrencyIdConvert::convert(currency_id.clone()) + .ok_or(Error::::NotCrossChainTransferableCurrency)?; + + let asset = (location.clone(), amount.into()).into(); + let fee: MultiAsset = (location, fee.into()).into(); + Self::do_transfer_multiasset_with_fee(who.clone(), asset, fee, dest.clone(), dest_weight, false)?; + + Self::deposit_event(Event::::Transferred(who, currency_id, amount, dest)); + Ok(()) + } + fn do_transfer_multiasset( who: T::AccountId, asset: MultiAsset, @@ -254,6 +331,50 @@ pub mod module { Ok(()) } + fn do_transfer_multiasset_with_fee( + who: T::AccountId, + asset: MultiAsset, + fee: MultiAsset, + dest: MultiLocation, + dest_weight: Weight, + deposit_event: bool, + ) -> DispatchResult { + if !asset.is_fungible(None) { + return Err(Error::::NotFungible.into()); + } + + if fungible_amount(&asset).is_zero() { + return Ok(()); + } + + // For now fee and asset id should be identical + // We can relax this assumption in the future + ensure!(fee.id == asset.id, Error::::DistincAssetAndFeeId); + + let (transfer_kind, dest, reserve, recipient) = Self::transfer_kind(&asset, &dest)?; + let mut msg = match transfer_kind { + SelfReserveAsset => { + Self::transfer_self_reserve_asset_with_fee(asset.clone(), fee.clone(), dest.clone(), recipient, dest_weight)? + } + ToReserve => Self::transfer_to_reserve_with_fee(asset.clone(), fee.clone(), dest.clone(), recipient, dest_weight)?, + ToNonReserve => { + Self::transfer_to_non_reserve_with_fee(asset.clone(), fee.clone(), reserve, dest.clone(), recipient, dest_weight)? + } + }; + + let origin_location = T::AccountIdToMultiLocation::convert(who.clone()); + let weight = T::Weigher::weight(&mut msg).map_err(|()| Error::::UnweighableMessage)?; + T::XcmExecutor::execute_xcm_in_credit(origin_location, msg, weight, weight) + .ensure_complete() + .map_err(|_| Error::::XcmExecutionFailed)?; + + if deposit_event { + Self::deposit_event(Event::::TransferredMultiAsset(who, asset, dest)); + } + + Ok(()) + } + fn transfer_self_reserve_asset( asset: MultiAsset, dest: MultiLocation, @@ -274,6 +395,31 @@ pub mod module { ])) } + fn transfer_self_reserve_asset_with_fee( + asset: MultiAsset, + fee: MultiAsset, + dest: MultiLocation, + recipient: MultiLocation, + dest_weight: Weight, + ) -> Result, DispatchError> { + Ok(Xcm(vec![ + WithdrawAsset(vec![ + asset.clone().into(), + fee.clone().into() + ].into() + ), + DepositReserveAsset { + assets: All.into(), + max_assets: 1, + dest: dest.clone(), + xcm: Xcm(vec![ + Self::buy_execution(fee, &dest, dest_weight)?, + Self::deposit_asset_specific(asset, recipient), + ]), + }, + ])) + } + fn transfer_to_reserve( asset: MultiAsset, reserve: MultiLocation, @@ -293,6 +439,31 @@ pub mod module { ])) } + fn transfer_to_reserve_with_fee( + asset: MultiAsset, + fee: MultiAsset, + reserve: MultiLocation, + recipient: MultiLocation, + dest_weight: Weight, + ) -> Result, DispatchError> { + println!("asset {:?}", asset); + Ok(Xcm(vec![ + WithdrawAsset(vec![ + asset.clone().into(), + fee.clone().into() + ].into() + ), + InitiateReserveWithdraw { + assets: All.into(), + reserve: reserve.clone(), + xcm: Xcm(vec![ + Self::buy_execution(fee, &reserve, dest_weight)?, + Self::deposit_asset_specific(asset, recipient), + ]), + }, + ])) + } + fn transfer_to_non_reserve( asset: MultiAsset, reserve: MultiLocation, @@ -334,6 +505,52 @@ pub mod module { ])) } + fn transfer_to_non_reserve_with_fee( + asset: MultiAsset, + fee: MultiAsset, + reserve: MultiLocation, + dest: MultiLocation, + recipient: MultiLocation, + dest_weight: Weight, + ) -> Result, DispatchError> { + let mut reanchored_dest = dest.clone(); + if reserve == MultiLocation::parent() { + match dest { + MultiLocation { + parents, + interior: X1(Parachain(id)), + } if parents == 1 => { + reanchored_dest = Parachain(id).into(); + } + _ => {} + } + } + + Ok(Xcm(vec![ + WithdrawAsset(vec![ + asset.clone().into(), + fee.clone().into() + ].into() + ), + InitiateReserveWithdraw { + assets: All.into(), + reserve: reserve.clone(), + xcm: Xcm(vec![ + Self::buy_execution(half(&fee), &reserve, dest_weight)?, + DepositReserveAsset { + assets: vec![half(&fee.clone()), asset.clone()].into(), + max_assets: 1, + dest: reanchored_dest, + xcm: Xcm(vec![ + Self::buy_execution(half(&fee), &dest, dest_weight)?, + Self::deposit_asset_specific(asset, recipient), + ]), + }, + ]), + }, + ])) + } + fn deposit_asset(recipient: MultiLocation) -> Instruction<()> { DepositAsset { assets: All.into(), @@ -342,6 +559,15 @@ pub mod module { } } + fn deposit_asset_specific(asset: MultiAsset, recipient: MultiLocation) -> Instruction<()> { + DepositAsset { + assets: vec![asset].into(), + //assets: All.into(), + max_assets: 1, + beneficiary: recipient, + } + } + fn buy_execution( asset: MultiAsset, at: &MultiLocation, diff --git a/xtokens/src/mock/relay.rs b/xtokens/src/mock/relay.rs index aa1959675..26d970f94 100644 --- a/xtokens/src/mock/relay.rs +++ b/xtokens/src/mock/relay.rs @@ -170,3 +170,11 @@ construct_runtime!( XcmPallet: pallet_xcm::{Pallet, Call, Storage, Event, Origin}, } ); + +pub(crate) fn relay_events() -> Vec { + System::events() + .into_iter() + .map(|r| r.event) + .filter_map(|e| Some(e)) + .collect::>() +} \ No newline at end of file diff --git a/xtokens/src/tests.rs b/xtokens/src/tests.rs index 74a927d92..ed9505f74 100644 --- a/xtokens/src/tests.rs +++ b/xtokens/src/tests.rs @@ -77,6 +77,42 @@ fn send_relay_chain_asset_to_relay_chain() { }); } +#[test] +fn send_relay_chain_asset_to_relay_chain_with_fee() { + TestNet::reset(); + + Relay::execute_with(|| { + let _ = RelayBalances::deposit_creating(¶_a_account(), 1_000); + }); + + ParaA::execute_with(|| { + assert_ok!(ParaXTokens::transfer_with_fee( + Some(ALICE).into(), + CurrencyId::R, + 460, + 40, + Box::new( + MultiLocation::new( + 1, + X1(Junction::AccountId32 { + network: NetworkId::Any, + id: BOB.into(), + }) + ) + .into() + ), + 40, + )); + assert_eq!(ParaTokens::free_balance(CurrencyId::R, &ALICE), 500); + }); + + Relay::execute_with(|| { + println!("Relay events {:?}", relay::relay_events()); + assert_eq!(RelayBalances::free_balance(¶_a_account()), 500); + assert_eq!(RelayBalances::free_balance(&BOB), 460); + }); +} + #[test] fn cannot_lost_fund_on_send_failed() { TestNet::reset(); From 002b0ce90445fea348e04565845410174e602bd2 Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 16 Nov 2021 14:54:26 +0100 Subject: [PATCH 02/16] add new funcs --- xtokens/src/lib.rs | 23 ++++++++++++++-------- xtokens/src/tests.rs | 45 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index b52622841..c5b301b14 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -414,7 +414,7 @@ pub mod module { dest: dest.clone(), xcm: Xcm(vec![ Self::buy_execution(fee, &dest, dest_weight)?, - Self::deposit_asset_specific(asset, recipient), + Self::deposit_asset_specific(asset, &dest, recipient)?, ]), }, ])) @@ -458,7 +458,7 @@ pub mod module { reserve: reserve.clone(), xcm: Xcm(vec![ Self::buy_execution(fee, &reserve, dest_weight)?, - Self::deposit_asset_specific(asset, recipient), + Self::deposit_asset_specific(asset, &reserve, recipient)?, ]), }, ])) @@ -526,6 +526,8 @@ pub mod module { } } + let inv_at = T::LocationInverter::invert_location(&reserve).map_err(|()| Error::::DestinationNotInvertible)?; + Ok(Xcm(vec![ WithdrawAsset(vec![ asset.clone().into(), @@ -538,12 +540,15 @@ pub mod module { xcm: Xcm(vec![ Self::buy_execution(half(&fee), &reserve, dest_weight)?, DepositReserveAsset { - assets: vec![half(&fee.clone()), asset.clone()].into(), + assets: vec![ + half(&fee.clone()).reanchored(&inv_at).map_err(|_| Error::::CannotReanchor)?, + asset.clone().reanchored(&inv_at).map_err(|_| Error::::CannotReanchor)? + ].into(), max_assets: 1, dest: reanchored_dest, xcm: Xcm(vec![ Self::buy_execution(half(&fee), &dest, dest_weight)?, - Self::deposit_asset_specific(asset, recipient), + Self::deposit_asset_specific(asset, &dest, recipient)?, ]), }, ]), @@ -559,13 +564,15 @@ pub mod module { } } - fn deposit_asset_specific(asset: MultiAsset, recipient: MultiLocation) -> Instruction<()> { - DepositAsset { - assets: vec![asset].into(), + fn deposit_asset_specific(asset: MultiAsset, at: &MultiLocation, recipient: MultiLocation) -> Result, DispatchError> { + let inv_at = T::LocationInverter::invert_location(at).map_err(|()| Error::::DestinationNotInvertible)?; + let asset = asset.reanchored(&inv_at).map_err(|_| Error::::CannotReanchor)?; + Ok(DepositAsset { + assets: asset.into(), //assets: All.into(), max_assets: 1, beneficiary: recipient, - } + }) } fn buy_execution( diff --git a/xtokens/src/tests.rs b/xtokens/src/tests.rs index ed9505f74..51cb2a574 100644 --- a/xtokens/src/tests.rs +++ b/xtokens/src/tests.rs @@ -97,7 +97,7 @@ fn send_relay_chain_asset_to_relay_chain_with_fee() { X1(Junction::AccountId32 { network: NetworkId::Any, id: BOB.into(), - }) + }) ) .into() ), @@ -107,7 +107,6 @@ fn send_relay_chain_asset_to_relay_chain_with_fee() { }); Relay::execute_with(|| { - println!("Relay events {:?}", relay::relay_events()); assert_eq!(RelayBalances::free_balance(¶_a_account()), 500); assert_eq!(RelayBalances::free_balance(&BOB), 460); }); @@ -185,6 +184,48 @@ fn send_relay_chain_asset_to_sibling() { }); } +#[test] +fn send_relay_chain_asset_to_sibling_with_fee() { + TestNet::reset(); + + Relay::execute_with(|| { + let _ = RelayBalances::deposit_creating(¶_a_account(), 1000); + }); + + ParaA::execute_with(|| { + assert_ok!(ParaXTokens::transfer_with_fee( + Some(ALICE).into(), + CurrencyId::R, + 420, + 80, + Box::new( + MultiLocation::new( + 1, + X2( + Parachain(2), + Junction::AccountId32 { + network: NetworkId::Any, + id: BOB.into(), + } + ) + ) + .into() + ), + 40, + )); + assert_eq!(ParaTokens::free_balance(CurrencyId::R, &ALICE), 500); + }); + + Relay::execute_with(|| { + assert_eq!(RelayBalances::free_balance(¶_a_account()), 500); + assert_eq!(RelayBalances::free_balance(¶_b_account()), 460); + }); + + ParaB::execute_with(|| { + assert_eq!(ParaTokens::free_balance(CurrencyId::R, &BOB), 420); + }); +} + #[test] fn send_sibling_asset_to_reserve_sibling() { TestNet::reset(); From 0be66482f4cdd2c2db9e5fa2392a58da4755b287 Mon Sep 17 00:00:00 2001 From: gorka Date: Wed, 17 Nov 2021 12:59:54 +0100 Subject: [PATCH 03/16] Remove relay events --- xtokens/src/mock/relay.rs | 10 +-- xtokens/src/tests.rs | 125 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 9 deletions(-) diff --git a/xtokens/src/mock/relay.rs b/xtokens/src/mock/relay.rs index 26d970f94..0a1cb5dc5 100644 --- a/xtokens/src/mock/relay.rs +++ b/xtokens/src/mock/relay.rs @@ -169,12 +169,4 @@ construct_runtime!( ParasUmp: ump::{Pallet, Call, Storage, Event}, XcmPallet: pallet_xcm::{Pallet, Call, Storage, Event, Origin}, } -); - -pub(crate) fn relay_events() -> Vec { - System::events() - .into_iter() - .map(|r| r.event) - .filter_map(|e| Some(e)) - .collect::>() -} \ No newline at end of file +); \ No newline at end of file diff --git a/xtokens/src/tests.rs b/xtokens/src/tests.rs index 51cb2a574..03d710c0f 100644 --- a/xtokens/src/tests.rs +++ b/xtokens/src/tests.rs @@ -266,6 +266,47 @@ fn send_sibling_asset_to_reserve_sibling() { }); } +#[test] +fn send_sibling_asset_to_reserve_sibling_with_fee() { + TestNet::reset(); + + ParaA::execute_with(|| { + assert_ok!(ParaTokens::deposit(CurrencyId::B, &ALICE, 1_000)); + }); + + ParaB::execute_with(|| { + assert_ok!(ParaTokens::deposit(CurrencyId::B, &sibling_a_account(), 1_000)); + }); + + ParaA::execute_with(|| { + assert_ok!(ParaXTokens::transfer_with_fee( + Some(ALICE).into(), + CurrencyId::B, + 460, + 40, + Box::new( + ( + Parent, + Parachain(2), + Junction::AccountId32 { + network: NetworkId::Any, + id: BOB.into(), + }, + ) + .into() + ), + 40, + )); + + assert_eq!(ParaTokens::free_balance(CurrencyId::B, &ALICE), 500); + }); + + ParaB::execute_with(|| { + assert_eq!(ParaTokens::free_balance(CurrencyId::B, &sibling_a_account()), 500); + assert_eq!(ParaTokens::free_balance(CurrencyId::B, &BOB), 460); + }); +} + #[test] fn send_sibling_asset_to_non_reserve_sibling() { TestNet::reset(); @@ -312,6 +353,53 @@ fn send_sibling_asset_to_non_reserve_sibling() { }); } +#[test] +fn send_sibling_asset_to_non_reserve_sibling_with_fee() { + TestNet::reset(); + + ParaA::execute_with(|| { + assert_ok!(ParaTokens::deposit(CurrencyId::B, &ALICE, 1_000)); + }); + + ParaB::execute_with(|| { + assert_ok!(ParaTokens::deposit(CurrencyId::B, &sibling_a_account(), 1_000)); + }); + + ParaA::execute_with(|| { + assert_ok!(ParaXTokens::transfer_with_fee( + Some(ALICE).into(), + CurrencyId::B, + 420, + 80, + Box::new( + MultiLocation::new( + 1, + X2( + Parachain(3), + Junction::AccountId32 { + network: NetworkId::Any, + id: BOB.into(), + } + ) + ) + .into() + ), + 40 + )); + assert_eq!(ParaTokens::free_balance(CurrencyId::B, &ALICE), 500); + }); + + // check reserve accounts + ParaB::execute_with(|| { + assert_eq!(ParaTokens::free_balance(CurrencyId::B, &sibling_a_account()), 500); + assert_eq!(ParaTokens::free_balance(CurrencyId::B, &sibling_c_account()), 460); + }); + + ParaC::execute_with(|| { + assert_eq!(ParaTokens::free_balance(CurrencyId::B, &BOB), 420); + }); +} + #[test] fn send_self_parachain_asset_to_sibling() { TestNet::reset(); @@ -348,6 +436,43 @@ fn send_self_parachain_asset_to_sibling() { }); } +#[test] +fn send_self_parachain_asset_to_sibling_with_feee() { + TestNet::reset(); + + ParaA::execute_with(|| { + assert_ok!(ParaTokens::deposit(CurrencyId::A, &ALICE, 1_000)); + + assert_ok!(ParaXTokens::transfer_with_fee( + Some(ALICE).into(), + CurrencyId::A, + 460, + 40, + Box::new( + MultiLocation::new( + 1, + X2( + Parachain(2), + Junction::AccountId32 { + network: NetworkId::Any, + id: BOB.into(), + } + ) + ) + .into() + ), + 40, + )); + + assert_eq!(ParaTokens::free_balance(CurrencyId::A, &ALICE), 500); + assert_eq!(ParaTokens::free_balance(CurrencyId::A, &sibling_b_account()), 500); + }); + + ParaB::execute_with(|| { + assert_eq!(ParaTokens::free_balance(CurrencyId::A, &BOB), 460); + }); +} + #[test] fn transfer_no_reserve_assets_fails() { TestNet::reset(); From 351f82c0cf22a674d0ba2f4f1b607d72ce88026c Mon Sep 17 00:00:00 2001 From: gorka Date: Wed, 17 Nov 2021 13:04:32 +0100 Subject: [PATCH 04/16] insert end of line --- xtokens/src/mock/relay.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xtokens/src/mock/relay.rs b/xtokens/src/mock/relay.rs index 0a1cb5dc5..aa1959675 100644 --- a/xtokens/src/mock/relay.rs +++ b/xtokens/src/mock/relay.rs @@ -169,4 +169,4 @@ construct_runtime!( ParasUmp: ump::{Pallet, Call, Storage, Event}, XcmPallet: pallet_xcm::{Pallet, Call, Storage, Event, Origin}, } -); \ No newline at end of file +); From ebc02a79e38a1269f02ec56d89d1dfe83d63393d Mon Sep 17 00:00:00 2001 From: gorka Date: Wed, 17 Nov 2021 13:22:47 +0100 Subject: [PATCH 05/16] Remove print --- xtokens/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index c5b301b14..9554691c1 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -446,7 +446,6 @@ pub mod module { recipient: MultiLocation, dest_weight: Weight, ) -> Result, DispatchError> { - println!("asset {:?}", asset); Ok(Xcm(vec![ WithdrawAsset(vec![ asset.clone().into(), From c3962247efeffecd70306e5aba565dcb0149561b Mon Sep 17 00:00:00 2001 From: gorka Date: Thu, 18 Nov 2021 10:49:25 +0100 Subject: [PATCH 06/16] Add new events --- xtokens/src/lib.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index 9554691c1..da769e719 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -104,8 +104,12 @@ pub mod module { pub enum Event { /// Transferred. \[sender, currency_id, amount, dest\] Transferred(T::AccountId, T::CurrencyId, T::Balance, MultiLocation), + /// Transferred. \[sender, currency_id, amount, fee, dest\] + TransferredWithFee(T::AccountId, T::CurrencyId, T::Balance, T::Balance, MultiLocation), /// Transferred `MultiAsset`. \[sender, asset, dest\] TransferredMultiAsset(T::AccountId, MultiAsset, MultiLocation), + /// Transferred `MultiAsset`. \[sender, asset, dest\] + TransferredMultiAssetWithFee(T::AccountId, MultiAsset, MultiAsset, MultiLocation), } #[pallet::error] @@ -285,10 +289,10 @@ pub mod module { .ok_or(Error::::NotCrossChainTransferableCurrency)?; let asset = (location.clone(), amount.into()).into(); - let fee: MultiAsset = (location, fee.into()).into(); - Self::do_transfer_multiasset_with_fee(who.clone(), asset, fee, dest.clone(), dest_weight, false)?; + let fee_asset: MultiAsset = (location, fee.into()).into(); + Self::do_transfer_multiasset_with_fee(who.clone(), asset, fee_asset, dest.clone(), dest_weight, false)?; - Self::deposit_event(Event::::Transferred(who, currency_id, amount, dest)); + Self::deposit_event(Event::::TransferredWithFee(who, currency_id, amount, fee, dest)); Ok(()) } @@ -369,7 +373,7 @@ pub mod module { .map_err(|_| Error::::XcmExecutionFailed)?; if deposit_event { - Self::deposit_event(Event::::TransferredMultiAsset(who, asset, dest)); + Self::deposit_event(Event::::TransferredMultiAssetWithFee(who, asset, fee, dest)); } Ok(()) From 870b95d744096c1a599a30b60c3290cd595b5877 Mon Sep 17 00:00:00 2001 From: gorka Date: Thu, 18 Nov 2021 18:01:14 +0100 Subject: [PATCH 07/16] refactor deposit asset and make transactional --- traits/src/xcm_transfer.rs | 19 ++++++ xtokens/src/lib.rs | 135 ++++++++++++++++++++++++------------- xtokens/src/tests.rs | 2 +- 3 files changed, 107 insertions(+), 49 deletions(-) diff --git a/traits/src/xcm_transfer.rs b/traits/src/xcm_transfer.rs index 18913cb00..f8bacf6f0 100644 --- a/traits/src/xcm_transfer.rs +++ b/traits/src/xcm_transfer.rs @@ -20,4 +20,23 @@ pub trait XcmTransfer { dest: MultiLocation, dest_weight: Weight, ) -> DispatchResult; + + /// Transfer native currencies. + fn transfer_with_fee( + who: AccountId, + currency_id: CurrencyId, + amount: Balance, + fee: Balance, + dest: MultiLocation, + dest_weight: Weight, + ) -> DispatchResult; + + /// Transfer `MultiAsset` + fn transfer_multi_asset_with_fee( + who: AccountId, + asset: MultiAsset, + fee: MultiAsset, + dest: MultiLocation, + dest_weight: Weight, + ) -> DispatchResult; } diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index da769e719..1f36ac487 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -176,13 +176,18 @@ pub mod module { Self::do_transfer(who, currency_id, amount, dest, dest_weight) } - /// Transfer native currencies specifying the fee. + /// Transfer native currencies specifying the fee and amount as + /// separate. /// /// `dest_weight` is the weight for XCM execution on the dest chain, and /// it would be charged from the transferred assets. If set below /// requirements, the execution may fail and assets wouldn't be /// received. /// + /// `fee` is the amount to be spent to pay for execution in destination + /// chain Both fee and amount will be substracted form the callers + /// balance + /// /// It's a no-op if any error on local XCM execution or message sending. /// Note sending assets out per se doesn't guarantee they would be /// received. Receiving depends on if the XCM message could be delivered @@ -229,13 +234,18 @@ pub mod module { Self::do_transfer_multiasset(who, asset, dest, dest_weight, true) } - /// Transfer `MultiAsset` specifying the fee. + /// Transfer `MultiAsset` specifying the fee and amount as separate. /// /// `dest_weight` is the weight for XCM execution on the dest chain, and /// it would be charged from the transferred assets. If set below /// requirements, the execution may fail and assets wouldn't be /// received. /// + /// `fee` is the multiasset to be spent to pay for execution in + /// destination chain Both fee and amount will be substracted form the + /// callers balance For now we only accept fee and asset having the same + /// multilocation id + /// /// It's a no-op if any error on local XCM execution or message sending. /// Note sending assets out per se doesn't guarantee they would be /// received. Receiving depends on if the XCM message could be delivered @@ -343,11 +353,11 @@ pub mod module { dest_weight: Weight, deposit_event: bool, ) -> DispatchResult { - if !asset.is_fungible(None) { + if !asset.is_fungible(None) || !fee.is_fungible(None) { return Err(Error::::NotFungible.into()); } - if fungible_amount(&asset).is_zero() { + if fungible_amount(&asset).is_zero() || fungible_amount(&fee).is_zero() { return Ok(()); } @@ -357,13 +367,28 @@ pub mod module { let (transfer_kind, dest, reserve, recipient) = Self::transfer_kind(&asset, &dest)?; let mut msg = match transfer_kind { - SelfReserveAsset => { - Self::transfer_self_reserve_asset_with_fee(asset.clone(), fee.clone(), dest.clone(), recipient, dest_weight)? - } - ToReserve => Self::transfer_to_reserve_with_fee(asset.clone(), fee.clone(), dest.clone(), recipient, dest_weight)?, - ToNonReserve => { - Self::transfer_to_non_reserve_with_fee(asset.clone(), fee.clone(), reserve, dest.clone(), recipient, dest_weight)? - } + SelfReserveAsset => Self::transfer_self_reserve_asset_with_fee( + asset.clone(), + fee.clone(), + dest.clone(), + recipient, + dest_weight, + )?, + ToReserve => Self::transfer_to_reserve_with_fee( + asset.clone(), + fee.clone(), + dest.clone(), + recipient, + dest_weight, + )?, + ToNonReserve => Self::transfer_to_non_reserve_with_fee( + asset.clone(), + fee.clone(), + reserve, + dest.clone(), + recipient, + dest_weight, + )?, }; let origin_location = T::AccountIdToMultiLocation::convert(who.clone()); @@ -393,7 +418,7 @@ pub mod module { dest: dest.clone(), xcm: Xcm(vec![ Self::buy_execution(asset, &dest, dest_weight)?, - Self::deposit_asset(recipient), + Self::deposit_asset(All.into(), &dest, recipient)?, ]), }, ])) @@ -407,18 +432,14 @@ pub mod module { dest_weight: Weight, ) -> Result, DispatchError> { Ok(Xcm(vec![ - WithdrawAsset(vec![ - asset.clone().into(), - fee.clone().into() - ].into() - ), + WithdrawAsset(vec![asset.clone().into(), fee.clone().into()].into()), DepositReserveAsset { assets: All.into(), max_assets: 1, dest: dest.clone(), xcm: Xcm(vec![ Self::buy_execution(fee, &dest, dest_weight)?, - Self::deposit_asset_specific(asset, &dest, recipient)?, + Self::deposit_asset(asset.into(), &dest, recipient)?, ]), }, ])) @@ -437,7 +458,7 @@ pub mod module { reserve: reserve.clone(), xcm: Xcm(vec![ Self::buy_execution(asset, &reserve, dest_weight)?, - Self::deposit_asset(recipient), + Self::deposit_asset(All.into(), &reserve, recipient)?, ]), }, ])) @@ -451,17 +472,13 @@ pub mod module { dest_weight: Weight, ) -> Result, DispatchError> { Ok(Xcm(vec![ - WithdrawAsset(vec![ - asset.clone().into(), - fee.clone().into() - ].into() - ), + WithdrawAsset(vec![asset.clone().into(), fee.clone().into()].into()), InitiateReserveWithdraw { assets: All.into(), reserve: reserve.clone(), xcm: Xcm(vec![ Self::buy_execution(fee, &reserve, dest_weight)?, - Self::deposit_asset_specific(asset, &reserve, recipient)?, + Self::deposit_asset(asset.into(), &reserve, recipient)?, ]), }, ])) @@ -500,7 +517,7 @@ pub mod module { dest: reanchored_dest, xcm: Xcm(vec![ Self::buy_execution(half(&asset), &dest, dest_weight)?, - Self::deposit_asset(recipient), + Self::deposit_asset(All.into(), &dest, recipient)?, ]), }, ]), @@ -529,14 +546,11 @@ pub mod module { } } - let inv_at = T::LocationInverter::invert_location(&reserve).map_err(|()| Error::::DestinationNotInvertible)?; + let reserve_inv_at = + T::LocationInverter::invert_location(&reserve).map_err(|()| Error::::DestinationNotInvertible)?; Ok(Xcm(vec![ - WithdrawAsset(vec![ - asset.clone().into(), - fee.clone().into() - ].into() - ), + WithdrawAsset(vec![asset.clone().into(), fee.clone().into()].into()), InitiateReserveWithdraw { assets: All.into(), reserve: reserve.clone(), @@ -544,14 +558,20 @@ pub mod module { Self::buy_execution(half(&fee), &reserve, dest_weight)?, DepositReserveAsset { assets: vec![ - half(&fee.clone()).reanchored(&inv_at).map_err(|_| Error::::CannotReanchor)?, - asset.clone().reanchored(&inv_at).map_err(|_| Error::::CannotReanchor)? - ].into(), + half(&fee.clone()) + .reanchored(&reserve_inv_at) + .map_err(|_| Error::::CannotReanchor)?, + asset + .clone() + .reanchored(&reserve_inv_at) + .map_err(|_| Error::::CannotReanchor)?, + ] + .into(), max_assets: 1, dest: reanchored_dest, xcm: Xcm(vec![ Self::buy_execution(half(&fee), &dest, dest_weight)?, - Self::deposit_asset_specific(asset, &dest, recipient)?, + Self::deposit_asset(asset.into(), &dest, recipient)?, ]), }, ]), @@ -559,20 +579,16 @@ pub mod module { ])) } - fn deposit_asset(recipient: MultiLocation) -> Instruction<()> { - DepositAsset { - assets: All.into(), - max_assets: 1, - beneficiary: recipient, - } - } - - fn deposit_asset_specific(asset: MultiAsset, at: &MultiLocation, recipient: MultiLocation) -> Result, DispatchError> { + fn deposit_asset( + mut asset: MultiAssetFilter, + at: &MultiLocation, + recipient: MultiLocation, + ) -> Result, DispatchError> { let inv_at = T::LocationInverter::invert_location(at).map_err(|()| Error::::DestinationNotInvertible)?; - let asset = asset.reanchored(&inv_at).map_err(|_| Error::::CannotReanchor)?; + asset.reanchor(&inv_at).map_err(|_| Error::::CannotReanchor)?; + Ok(DepositAsset { - assets: asset.into(), - //assets: All.into(), + assets: asset, max_assets: 1, beneficiary: recipient, }) @@ -696,6 +712,29 @@ pub mod module { ) -> DispatchResult { Self::do_transfer_multiasset(who, asset, dest, dest_weight, true) } + + #[require_transactional] + fn transfer_with_fee( + who: T::AccountId, + currency_id: T::CurrencyId, + amount: T::Balance, + fee: T::Balance, + dest: MultiLocation, + dest_weight: Weight, + ) -> DispatchResult { + Self::do_transfer_with_fee(who, currency_id, amount, fee, dest, dest_weight) + } + + #[require_transactional] + fn transfer_multi_asset_with_fee( + who: T::AccountId, + asset: MultiAsset, + fee: MultiAsset, + dest: MultiLocation, + dest_weight: Weight, + ) -> DispatchResult { + Self::do_transfer_multiasset_with_fee(who, asset, fee, dest, dest_weight, true) + } } } diff --git a/xtokens/src/tests.rs b/xtokens/src/tests.rs index 03d710c0f..6bfd24eb7 100644 --- a/xtokens/src/tests.rs +++ b/xtokens/src/tests.rs @@ -97,7 +97,7 @@ fn send_relay_chain_asset_to_relay_chain_with_fee() { X1(Junction::AccountId32 { network: NetworkId::Any, id: BOB.into(), - }) + }) ) .into() ), From 8fd3b848c6c630d7a1b54fa6fd59ee0762282d59 Mon Sep 17 00:00:00 2001 From: gorka Date: Mon, 22 Nov 2021 12:36:17 +0100 Subject: [PATCH 08/16] Remove from trait, as it is not necessary --- traits/src/xcm_transfer.rs | 19 ------------------- xtokens/src/lib.rs | 23 ----------------------- 2 files changed, 42 deletions(-) diff --git a/traits/src/xcm_transfer.rs b/traits/src/xcm_transfer.rs index f8bacf6f0..18913cb00 100644 --- a/traits/src/xcm_transfer.rs +++ b/traits/src/xcm_transfer.rs @@ -20,23 +20,4 @@ pub trait XcmTransfer { dest: MultiLocation, dest_weight: Weight, ) -> DispatchResult; - - /// Transfer native currencies. - fn transfer_with_fee( - who: AccountId, - currency_id: CurrencyId, - amount: Balance, - fee: Balance, - dest: MultiLocation, - dest_weight: Weight, - ) -> DispatchResult; - - /// Transfer `MultiAsset` - fn transfer_multi_asset_with_fee( - who: AccountId, - asset: MultiAsset, - fee: MultiAsset, - dest: MultiLocation, - dest_weight: Weight, - ) -> DispatchResult; } diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index 1f36ac487..c5d201fbd 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -712,29 +712,6 @@ pub mod module { ) -> DispatchResult { Self::do_transfer_multiasset(who, asset, dest, dest_weight, true) } - - #[require_transactional] - fn transfer_with_fee( - who: T::AccountId, - currency_id: T::CurrencyId, - amount: T::Balance, - fee: T::Balance, - dest: MultiLocation, - dest_weight: Weight, - ) -> DispatchResult { - Self::do_transfer_with_fee(who, currency_id, amount, fee, dest, dest_weight) - } - - #[require_transactional] - fn transfer_multi_asset_with_fee( - who: T::AccountId, - asset: MultiAsset, - fee: MultiAsset, - dest: MultiLocation, - dest_weight: Weight, - ) -> DispatchResult { - Self::do_transfer_multiasset_with_fee(who, asset, fee, dest, dest_weight, true) - } } } From 132c47f41d4bf5b262c667dcf853a14fe32d63d2 Mon Sep 17 00:00:00 2001 From: gorka Date: Mon, 22 Nov 2021 12:44:49 +0100 Subject: [PATCH 09/16] Better doc --- xtokens/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index c5d201fbd..2d9bf86b0 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -104,11 +104,11 @@ pub mod module { pub enum Event { /// Transferred. \[sender, currency_id, amount, dest\] Transferred(T::AccountId, T::CurrencyId, T::Balance, MultiLocation), - /// Transferred. \[sender, currency_id, amount, fee, dest\] + /// Transferred with fee. \[sender, currency_id, amount, fee, dest\] TransferredWithFee(T::AccountId, T::CurrencyId, T::Balance, T::Balance, MultiLocation), /// Transferred `MultiAsset`. \[sender, asset, dest\] TransferredMultiAsset(T::AccountId, MultiAsset, MultiLocation), - /// Transferred `MultiAsset`. \[sender, asset, dest\] + /// Transferred `MultiAsset` with fee. \[sender, asset, dest\] TransferredMultiAssetWithFee(T::AccountId, MultiAsset, MultiAsset, MultiLocation), } From b75e1cb690761de8917ad67fbb27f4cec2e5c3bd Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 23 Nov 2021 15:27:30 +0100 Subject: [PATCH 10/16] Return non-spent fee to deposit account, plus some fmt changes --- xtokens/src/lib.rs | 81 +++++++++++++++++--------------------------- xtokens/src/tests.rs | 29 ++++++++++------ 2 files changed, 49 insertions(+), 61 deletions(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index 2d9bf86b0..da5d9aac0 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -108,7 +108,7 @@ pub mod module { TransferredWithFee(T::AccountId, T::CurrencyId, T::Balance, T::Balance, MultiLocation), /// Transferred `MultiAsset`. \[sender, asset, dest\] TransferredMultiAsset(T::AccountId, MultiAsset, MultiLocation), - /// Transferred `MultiAsset` with fee. \[sender, asset, dest\] + /// Transferred `MultiAsset` with fee. \[sender, asset, fee, dest\] TransferredMultiAssetWithFee(T::AccountId, MultiAsset, MultiAsset, MultiLocation), } @@ -176,62 +176,62 @@ pub mod module { Self::do_transfer(who, currency_id, amount, dest, dest_weight) } - /// Transfer native currencies specifying the fee and amount as - /// separate. + /// Transfer `MultiAsset`. /// /// `dest_weight` is the weight for XCM execution on the dest chain, and /// it would be charged from the transferred assets. If set below /// requirements, the execution may fail and assets wouldn't be /// received. /// - /// `fee` is the amount to be spent to pay for execution in destination - /// chain Both fee and amount will be substracted form the callers - /// balance - /// /// It's a no-op if any error on local XCM execution or message sending. /// Note sending assets out per se doesn't guarantee they would be /// received. Receiving depends on if the XCM message could be delivered /// by the network, and if the receiving chain would handle /// messages correctly. - #[pallet::weight(Pallet::::weight_of_transfer(currency_id.clone(), *amount, dest))] + #[pallet::weight(Pallet::::weight_of_transfer_multiasset(asset, dest))] #[transactional] - pub fn transfer_with_fee( + pub fn transfer_multiasset( origin: OriginFor, - currency_id: T::CurrencyId, - amount: T::Balance, - fee: T::Balance, + asset: Box, dest: Box, dest_weight: Weight, ) -> DispatchResult { let who = ensure_signed(origin)?; + let asset: MultiAsset = (*asset).try_into().map_err(|()| Error::::BadVersion)?; let dest: MultiLocation = (*dest).try_into().map_err(|()| Error::::BadVersion)?; - Self::do_transfer_with_fee(who, currency_id, amount, fee, dest, dest_weight) + Self::do_transfer_multiasset(who, asset, dest, dest_weight, true) } - /// Transfer `MultiAsset`. + /// Transfer native currencies specifying the fee and amount as + /// separate. /// /// `dest_weight` is the weight for XCM execution on the dest chain, and /// it would be charged from the transferred assets. If set below /// requirements, the execution may fail and assets wouldn't be /// received. /// + /// `fee` is the amount to be spent to pay for execution in destination + /// chain. Both fee and amount will be subtracted form the callers + /// balance. + /// /// It's a no-op if any error on local XCM execution or message sending. /// Note sending assets out per se doesn't guarantee they would be /// received. Receiving depends on if the XCM message could be delivered /// by the network, and if the receiving chain would handle /// messages correctly. - #[pallet::weight(Pallet::::weight_of_transfer_multiasset(asset, dest))] + #[pallet::weight(Pallet::::weight_of_transfer(currency_id.clone(), *amount, dest))] #[transactional] - pub fn transfer_multiasset( + pub fn transfer_with_fee( origin: OriginFor, - asset: Box, + currency_id: T::CurrencyId, + amount: T::Balance, + fee: T::Balance, dest: Box, dest_weight: Weight, ) -> DispatchResult { let who = ensure_signed(origin)?; - let asset: MultiAsset = (*asset).try_into().map_err(|()| Error::::BadVersion)?; let dest: MultiLocation = (*dest).try_into().map_err(|()| Error::::BadVersion)?; - Self::do_transfer_multiasset(who, asset, dest, dest_weight, true) + Self::do_transfer_with_fee(who, currency_id, amount, fee, dest, dest_weight) } /// Transfer `MultiAsset` specifying the fee and amount as separate. @@ -357,7 +357,7 @@ pub mod module { return Err(Error::::NotFungible.into()); } - if fungible_amount(&asset).is_zero() || fungible_amount(&fee).is_zero() { + if fungible_amount(&asset).is_zero() { return Ok(()); } @@ -418,7 +418,7 @@ pub mod module { dest: dest.clone(), xcm: Xcm(vec![ Self::buy_execution(asset, &dest, dest_weight)?, - Self::deposit_asset(All.into(), &dest, recipient)?, + Self::deposit_asset(recipient), ]), }, ])) @@ -439,7 +439,7 @@ pub mod module { dest: dest.clone(), xcm: Xcm(vec![ Self::buy_execution(fee, &dest, dest_weight)?, - Self::deposit_asset(asset.into(), &dest, recipient)?, + Self::deposit_asset(recipient), ]), }, ])) @@ -458,7 +458,7 @@ pub mod module { reserve: reserve.clone(), xcm: Xcm(vec![ Self::buy_execution(asset, &reserve, dest_weight)?, - Self::deposit_asset(All.into(), &reserve, recipient)?, + Self::deposit_asset(recipient), ]), }, ])) @@ -478,7 +478,7 @@ pub mod module { reserve: reserve.clone(), xcm: Xcm(vec![ Self::buy_execution(fee, &reserve, dest_weight)?, - Self::deposit_asset(asset.into(), &reserve, recipient)?, + Self::deposit_asset(recipient), ]), }, ])) @@ -517,7 +517,7 @@ pub mod module { dest: reanchored_dest, xcm: Xcm(vec![ Self::buy_execution(half(&asset), &dest, dest_weight)?, - Self::deposit_asset(All.into(), &dest, recipient)?, + Self::deposit_asset(recipient), ]), }, ]), @@ -546,9 +546,6 @@ pub mod module { } } - let reserve_inv_at = - T::LocationInverter::invert_location(&reserve).map_err(|()| Error::::DestinationNotInvertible)?; - Ok(Xcm(vec![ WithdrawAsset(vec![asset.clone().into(), fee.clone().into()].into()), InitiateReserveWithdraw { @@ -557,21 +554,12 @@ pub mod module { xcm: Xcm(vec![ Self::buy_execution(half(&fee), &reserve, dest_weight)?, DepositReserveAsset { - assets: vec![ - half(&fee.clone()) - .reanchored(&reserve_inv_at) - .map_err(|_| Error::::CannotReanchor)?, - asset - .clone() - .reanchored(&reserve_inv_at) - .map_err(|_| Error::::CannotReanchor)?, - ] - .into(), + assets: All.into(), max_assets: 1, dest: reanchored_dest, xcm: Xcm(vec![ Self::buy_execution(half(&fee), &dest, dest_weight)?, - Self::deposit_asset(asset.into(), &dest, recipient)?, + Self::deposit_asset(recipient), ]), }, ]), @@ -579,19 +567,12 @@ pub mod module { ])) } - fn deposit_asset( - mut asset: MultiAssetFilter, - at: &MultiLocation, - recipient: MultiLocation, - ) -> Result, DispatchError> { - let inv_at = T::LocationInverter::invert_location(at).map_err(|()| Error::::DestinationNotInvertible)?; - asset.reanchor(&inv_at).map_err(|_| Error::::CannotReanchor)?; - - Ok(DepositAsset { - assets: asset, + fn deposit_asset(recipient: MultiLocation) -> Instruction<()> { + DepositAsset { + assets: All.into(), max_assets: 1, beneficiary: recipient, - }) + } } fn buy_execution( diff --git a/xtokens/src/tests.rs b/xtokens/src/tests.rs index 6bfd24eb7..c50ea8552 100644 --- a/xtokens/src/tests.rs +++ b/xtokens/src/tests.rs @@ -89,8 +89,8 @@ fn send_relay_chain_asset_to_relay_chain_with_fee() { assert_ok!(ParaXTokens::transfer_with_fee( Some(ALICE).into(), CurrencyId::R, - 460, - 40, + 450, + 50, Box::new( MultiLocation::new( 1, @@ -106,6 +106,7 @@ fn send_relay_chain_asset_to_relay_chain_with_fee() { assert_eq!(ParaTokens::free_balance(CurrencyId::R, &ALICE), 500); }); + // It should use 40 for weight, so 460 should reach destination Relay::execute_with(|| { assert_eq!(RelayBalances::free_balance(¶_a_account()), 500); assert_eq!(RelayBalances::free_balance(&BOB), 460); @@ -196,8 +197,8 @@ fn send_relay_chain_asset_to_sibling_with_fee() { assert_ok!(ParaXTokens::transfer_with_fee( Some(ALICE).into(), CurrencyId::R, - 420, - 80, + 410, + 90, Box::new( MultiLocation::new( 1, @@ -216,11 +217,13 @@ fn send_relay_chain_asset_to_sibling_with_fee() { assert_eq!(ParaTokens::free_balance(CurrencyId::R, &ALICE), 500); }); + // It should use 40 weight Relay::execute_with(|| { assert_eq!(RelayBalances::free_balance(¶_a_account()), 500); assert_eq!(RelayBalances::free_balance(¶_b_account()), 460); }); + // It should use another 40 weight in paraB ParaB::execute_with(|| { assert_eq!(ParaTokens::free_balance(CurrencyId::R, &BOB), 420); }); @@ -282,8 +285,8 @@ fn send_sibling_asset_to_reserve_sibling_with_fee() { assert_ok!(ParaXTokens::transfer_with_fee( Some(ALICE).into(), CurrencyId::B, - 460, - 40, + 450, + 50, Box::new( ( Parent, @@ -301,6 +304,7 @@ fn send_sibling_asset_to_reserve_sibling_with_fee() { assert_eq!(ParaTokens::free_balance(CurrencyId::B, &ALICE), 500); }); + // It should use 40 for weight, so 460 should reach destination ParaB::execute_with(|| { assert_eq!(ParaTokens::free_balance(CurrencyId::B, &sibling_a_account()), 500); assert_eq!(ParaTokens::free_balance(CurrencyId::B, &BOB), 460); @@ -369,8 +373,8 @@ fn send_sibling_asset_to_non_reserve_sibling_with_fee() { assert_ok!(ParaXTokens::transfer_with_fee( Some(ALICE).into(), CurrencyId::B, - 420, - 80, + 410, + 90, Box::new( MultiLocation::new( 1, @@ -389,12 +393,14 @@ fn send_sibling_asset_to_non_reserve_sibling_with_fee() { assert_eq!(ParaTokens::free_balance(CurrencyId::B, &ALICE), 500); }); + // Should use only 40 weight // check reserve accounts ParaB::execute_with(|| { assert_eq!(ParaTokens::free_balance(CurrencyId::B, &sibling_a_account()), 500); assert_eq!(ParaTokens::free_balance(CurrencyId::B, &sibling_c_account()), 460); }); + // Should use 40 additional weight ParaC::execute_with(|| { assert_eq!(ParaTokens::free_balance(CurrencyId::B, &BOB), 420); }); @@ -437,7 +443,7 @@ fn send_self_parachain_asset_to_sibling() { } #[test] -fn send_self_parachain_asset_to_sibling_with_feee() { +fn send_self_parachain_asset_to_sibling_with_fee() { TestNet::reset(); ParaA::execute_with(|| { @@ -446,8 +452,8 @@ fn send_self_parachain_asset_to_sibling_with_feee() { assert_ok!(ParaXTokens::transfer_with_fee( Some(ALICE).into(), CurrencyId::A, - 460, - 40, + 450, + 50, Box::new( MultiLocation::new( 1, @@ -468,6 +474,7 @@ fn send_self_parachain_asset_to_sibling_with_feee() { assert_eq!(ParaTokens::free_balance(CurrencyId::A, &sibling_b_account()), 500); }); + // It should use 40 for weight, so 460 should reach destination ParaB::execute_with(|| { assert_eq!(ParaTokens::free_balance(CurrencyId::A, &BOB), 460); }); From 34c79cad8c1a5c063a386fecfcd877b962374bed Mon Sep 17 00:00:00 2001 From: gorka Date: Tue, 23 Nov 2021 15:29:41 +0100 Subject: [PATCH 11/16] More comment fixes --- xtokens/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index da5d9aac0..44f1ac891 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -138,7 +138,7 @@ pub mod module { /// The version of the `Versioned` value used is not able to be /// interpreted. BadVersion, - /// The fee MultiAsset is of different type than the asset to transfer + /// The fee MultiAsset is of different type than the asset to transfer. DistincAssetAndFeeId, } @@ -242,9 +242,9 @@ pub mod module { /// received. /// /// `fee` is the multiasset to be spent to pay for execution in - /// destination chain Both fee and amount will be substracted form the + /// destination chain. Both fee and amount will be subtracted form the /// callers balance For now we only accept fee and asset having the same - /// multilocation id + /// `MultiLocation` id. /// /// It's a no-op if any error on local XCM execution or message sending. /// Note sending assets out per se doesn't guarantee they would be From 6eea618719849593875ea0c39be36383d86800c9 Mon Sep 17 00:00:00 2001 From: gorka Date: Wed, 24 Nov 2021 09:41:40 +0100 Subject: [PATCH 12/16] Zero fee should error --- xtokens/src/lib.rs | 16 ++++++++++++++++ xtokens/src/tests.rs | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index 44f1ac891..7a9bf97c8 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -140,6 +140,9 @@ pub mod module { BadVersion, /// The fee MultiAsset is of different type than the asset to transfer. DistincAssetAndFeeId, + /// The fee amount was zero when the fee specification extrinsic is + /// being used. + FeeCannotBeZero, } #[pallet::hooks] @@ -214,6 +217,10 @@ pub mod module { /// chain. Both fee and amount will be subtracted form the callers /// balance. /// + /// If `fee` is not high enough to cover for the execution costs in the + /// destination chain, then the assets will be trapped in the + /// destination chain + /// /// It's a no-op if any error on local XCM execution or message sending. /// Note sending assets out per se doesn't guarantee they would be /// received. Receiving depends on if the XCM message could be delivered @@ -246,6 +253,10 @@ pub mod module { /// callers balance For now we only accept fee and asset having the same /// `MultiLocation` id. /// + /// If `fee` is not high enough to cover for the execution costs in the + /// destination chain, then the assets will be trapped in the + /// destination chain + /// /// It's a no-op if any error on local XCM execution or message sending. /// Note sending assets out per se doesn't guarantee they would be /// received. Receiving depends on if the XCM message could be delivered @@ -357,6 +368,11 @@ pub mod module { return Err(Error::::NotFungible.into()); } + // Zero fee is an error + if fungible_amount(&fee).is_zero() { + return Err(Error::::FeeCannotBeZero.into()); + } + if fungible_amount(&asset).is_zero() { return Ok(()); } diff --git a/xtokens/src/tests.rs b/xtokens/src/tests.rs index c50ea8552..964c5df2a 100644 --- a/xtokens/src/tests.rs +++ b/xtokens/src/tests.rs @@ -657,3 +657,35 @@ fn call_size_limit() { If the limit is too strong, maybe consider increasing the limit", ); } + +#[test] +fn send_with_zero_fee_should_yield_an_error() { + TestNet::reset(); + + ParaA::execute_with(|| { + // Transferring with zero fee should fail + assert_noop!( + ParaXTokens::transfer_with_fee( + Some(ALICE).into(), + CurrencyId::A, + 450, + 0, + Box::new( + MultiLocation::new( + 1, + X2( + Parachain(2), + Junction::AccountId32 { + network: NetworkId::Any, + id: BOB.into(), + } + ) + ) + .into() + ), + 40, + ), + Error::::FeeCannotBeZero + ); + }); +} From f53195f7a6a68f842543a6dff684b58401c0eeec Mon Sep 17 00:00:00 2001 From: gorka Date: Wed, 24 Nov 2021 09:58:58 +0100 Subject: [PATCH 13/16] Add trap assets tests --- xtokens/src/mock/para.rs | 4 ++-- xtokens/src/tests.rs | 44 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/xtokens/src/mock/para.rs b/xtokens/src/mock/para.rs index 0c67e974a..087217078 100644 --- a/xtokens/src/mock/para.rs +++ b/xtokens/src/mock/para.rs @@ -198,8 +198,8 @@ impl Config for XcmConfig { type Weigher = FixedWeightBounds; type Trader = AllTokensAreCreatedEqualToWeight; type ResponseHandler = (); - type AssetTrap = (); - type AssetClaims = (); + type AssetTrap = PolkadotXcm; + type AssetClaims = PolkadotXcm; type SubscriptionService = PolkadotXcm; } diff --git a/xtokens/src/tests.rs b/xtokens/src/tests.rs index 964c5df2a..9bd661014 100644 --- a/xtokens/src/tests.rs +++ b/xtokens/src/tests.rs @@ -663,6 +663,8 @@ fn send_with_zero_fee_should_yield_an_error() { TestNet::reset(); ParaA::execute_with(|| { + assert_ok!(ParaTokens::deposit(CurrencyId::A, &ALICE, 1_000)); + // Transferring with zero fee should fail assert_noop!( ParaXTokens::transfer_with_fee( @@ -689,3 +691,45 @@ fn send_with_zero_fee_should_yield_an_error() { ); }); } + +#[test] +fn send_with_insufficient_fee_traps_assets() { + TestNet::reset(); + + ParaA::execute_with(|| { + assert_ok!(ParaTokens::deposit(CurrencyId::A, &ALICE, 1_000)); + + // ParaB charges 40, but we specify 30 as fee. Assets will be trapped + // Call succedes in paraA + assert_ok!(ParaXTokens::transfer_with_fee( + Some(ALICE).into(), + CurrencyId::A, + 450, + 30, + Box::new( + MultiLocation::new( + 1, + X2( + Parachain(2), + Junction::AccountId32 { + network: NetworkId::Any, + id: BOB.into(), + } + ) + ) + .into() + ), + 40, + )); + }); + + // In paraB, assets have been trapped due to he failed execution + ParaB::execute_with(|| { + assert!(para::System::events().iter().any(|r| { + matches!( + r.event, + para::Event::PolkadotXcm(pallet_xcm::Event::::AssetsTrapped(_, _, _)) + ) + })); + }) +} From 8826038f81594aaa2676ef0110e22d794971f5c4 Mon Sep 17 00:00:00 2001 From: gorka Date: Wed, 24 Nov 2021 10:03:36 +0100 Subject: [PATCH 14/16] Clippy --- xtokens/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index 7a9bf97c8..542bae29d 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -448,7 +448,7 @@ pub mod module { dest_weight: Weight, ) -> Result, DispatchError> { Ok(Xcm(vec![ - WithdrawAsset(vec![asset.clone().into(), fee.clone().into()].into()), + WithdrawAsset(vec![asset, fee.clone()].into()), DepositReserveAsset { assets: All.into(), max_assets: 1, @@ -488,7 +488,7 @@ pub mod module { dest_weight: Weight, ) -> Result, DispatchError> { Ok(Xcm(vec![ - WithdrawAsset(vec![asset.clone().into(), fee.clone().into()].into()), + WithdrawAsset(vec![asset, fee.clone()].into()), InitiateReserveWithdraw { assets: All.into(), reserve: reserve.clone(), @@ -563,7 +563,7 @@ pub mod module { } Ok(Xcm(vec![ - WithdrawAsset(vec![asset.clone().into(), fee.clone().into()].into()), + WithdrawAsset(vec![asset, fee.clone()].into()), InitiateReserveWithdraw { assets: All.into(), reserve: reserve.clone(), From 4820c13c5597ac4e50a982b5792dd3c5bc5a1573 Mon Sep 17 00:00:00 2001 From: gorka Date: Fri, 26 Nov 2021 11:07:31 +0100 Subject: [PATCH 15/16] Add first shortcut for fee in do_transfer_with_fee --- xtokens/src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index 542bae29d..e043c059d 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -309,6 +309,11 @@ pub mod module { let location: MultiLocation = T::CurrencyIdConvert::convert(currency_id.clone()) .ok_or(Error::::NotCrossChainTransferableCurrency)?; + // Zero fee is an error + if fee.is_zero() { + return Err(Error::::FeeCannotBeZero.into()); + } + let asset = (location.clone(), amount.into()).into(); let fee_asset: MultiAsset = (location, fee.into()).into(); Self::do_transfer_multiasset_with_fee(who.clone(), asset, fee_asset, dest.clone(), dest_weight, false)?; From 41634f8f83d79a34dfd22b935f2c4c0dd5accec3 Mon Sep 17 00:00:00 2001 From: gorka Date: Fri, 26 Nov 2021 11:41:20 +0100 Subject: [PATCH 16/16] Do the check before getting into do_transfer_with_fee and do_transfer_multiasset_with_Fee --- xtokens/src/lib.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/xtokens/src/lib.rs b/xtokens/src/lib.rs index e043c059d..f511be7e5 100644 --- a/xtokens/src/lib.rs +++ b/xtokens/src/lib.rs @@ -238,6 +238,11 @@ 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) } @@ -275,6 +280,10 @@ 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, true) } @@ -309,11 +318,6 @@ pub mod module { let location: MultiLocation = T::CurrencyIdConvert::convert(currency_id.clone()) .ok_or(Error::::NotCrossChainTransferableCurrency)?; - // Zero fee is an error - if fee.is_zero() { - return Err(Error::::FeeCannotBeZero.into()); - } - let asset = (location.clone(), amount.into()).into(); let fee_asset: MultiAsset = (location, fee.into()).into(); Self::do_transfer_multiasset_with_fee(who.clone(), asset, fee_asset, dest.clone(), dest_weight, false)?; @@ -373,11 +377,6 @@ pub mod module { return Err(Error::::NotFungible.into()); } - // Zero fee is an error - if fungible_amount(&fee).is_zero() { - return Err(Error::::FeeCannotBeZero.into()); - } - if fungible_amount(&asset).is_zero() { return Ok(()); }