Skip to content
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

Jump to bottom

refactor: Use VersionedLocation in RestrictedTransferLocation #1861

Merged
merged 10 commits into from
Jun 6, 2024
Merged

refactor: Use VersionedLocation in RestrictedTransferLocation #1861

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
66886ca
feat: add pre + post-upgrade
wischli Jun 4, 2024
8242a08
feat: add nuking migration for dev and demo
wischli Jun 4, 2024
995887e
fix: remove account list for demo
wischli Jun 4, 2024
8e5c8d5
refactor: use VersionedLocation
wischli Jun 5, 2024
1f4c2d7
fix: transfer allowlist benchmarks + tests
wischli Jun 5, 2024
0c0d53d
fix: clippy
wischli Jun 5, 2024
6b97978
fix: benchmark cli
wischli Jun 5, 2024
3e0c342
Merge remote-tracking branch 'origin/polkadot-v1.7.2-transfer-locatio…
wischli Jun 5, 2024
a7cf00f
fix: revert tight coupling
wischli Jun 6, 2024
15a3f2a
fmt
wischli Jun 6, 2024
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
15 changes: 3 additions & 12 deletions libs/types/src/locations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,17 @@ use cfg_primitives::AccountId;
use frame_support::RuntimeDebugNoBound;
use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
use scale_info::TypeInfo;
use sp_core::crypto::AccountId32;
// Please note that if this version change,
// a migration could be required in those places where
// RestrictedTransferLocation is stored
use staging_xcm::v4::Location;
use staging_xcm::VersionedLocation;

use crate::domain_address::DomainAddress;

/// Location types for destinations that can receive restricted transfers
#[derive(Clone, RuntimeDebugNoBound, Encode, Decode, Eq, PartialEq, MaxEncodedLen, TypeInfo)]
pub enum RestrictedTransferLocation {
/// Local chain account sending destination.
Local(AccountId),
/// XCM Location sending destinations.
Xcm(Location),
Xcm(Box<VersionedLocation>),
lemunozm marked this conversation as resolved.
Show resolved Hide resolved
/// DomainAddress sending location from a liquidity pools' instance
Address(DomainAddress),
}

impl From<AccountId32> for RestrictedTransferLocation {
fn from(value: AccountId32) -> Self {
Self::Local(value)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you have an error with this? I remember runtime benchmarks not working without this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check again with this enabled. I don't know why but recently I am often fooled by my cargo cache which I can only fix by cleaning completely.

This change broke transfer allowlist benchmarks which required T::AccountId <-> T::Location conversion. Unfortunately, it was not possible to implement From/TryFrom for AccountId and RestrictedTransferLocation because of the the blanket Rust From<T> for T implementation which conflicts with any custom implementation involving the same type for both From and TryFrom.

AFAICT, I could have either added a custom conversion trait or gone with my current proposal. Unfortunately, my proposal required refactoring unit tests which used u64 as AccountId and a custom enum for T::Location which conflicted with my new benchmark restriction of Config<AccountId = AccountId, Location = RestrictedTransferLocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, it was not possible to implement From/TryFrom for AccountId and RestrictedTransferLocation

The above compiled in main IIRC 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently, it works for AccountId32 as part of our current codebase. However, it does not work for AccountId even though that's the same type under the hood 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted in a7cf00f

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that is something I previously battled with. I do not understand why TBH. Even I was not able to find the impl T for T implementation running cargo doc. There is something weird in the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to hear even you don't understand it. It really frustrated me yesterday..

42 changes: 12 additions & 30 deletions pallets/restricted-xtokens/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,37 +51,37 @@ use staging_xcm::{v4::prelude::*, VersionedAsset, VersionedAssets, VersionedLoca
pub enum TransferEffects<AccountId, CurrencyId, Balance> {
Transfer {
sender: AccountId,
destination: Location,
destination: VersionedLocation,
currency_id: CurrencyId,
amount: Balance,
},
TransferMultiAsset {
sender: AccountId,
destination: Location,
destination: VersionedLocation,
asset: Asset,
},
TransferWithFee {
sender: AccountId,
destination: Location,
destination: VersionedLocation,
currency_id: CurrencyId,
amount: Balance,
fee: Balance,
},
TransferMultiAssetWithFee {
sender: AccountId,
destination: Location,
destination: VersionedLocation,
asset: Asset,
fee_asset: Asset,
},
TransferMultiCurrencies {
sender: AccountId,
destination: Location,
destination: VersionedLocation,
currencies: Vec<(CurrencyId, Balance)>,
fee: (CurrencyId, Balance),
},
TransferMultiAssets {
sender: AccountId,
destination: Location,
destination: VersionedLocation,
assets: Assets,
fee_asset: Asset,
},
Expand Down Expand Up @@ -130,14 +130,11 @@ pub mod pallet {
dest: Box<VersionedLocation>,
dest_weight_limit: WeightLimit,
) -> DispatchResult {
let destination: Location = (*dest.clone())
.try_into()
.map_err(|()| Error::<T>::BadVersion)?;
let sender = ensure_signed(origin.clone())?;

T::PreTransfer::check(TransferEffects::Transfer {
sender,
destination,
destination: (*dest.clone()),
currency_id: currency_id.clone(),
amount,
})?;
Expand Down Expand Up @@ -175,13 +172,10 @@ pub mod pallet {
let multi_asset: Asset = (*asset.clone())
.try_into()
.map_err(|()| Error::<T>::BadVersion)?;
let destination: Location = (*dest.clone())
.try_into()
.map_err(|()| Error::<T>::BadVersion)?;

T::PreTransfer::check(TransferEffects::TransferMultiAsset {
sender,
destination,
destination: (*dest.clone()),
asset: multi_asset,
})?;

Expand Down Expand Up @@ -220,13 +214,10 @@ pub mod pallet {
dest_weight_limit: WeightLimit,
) -> DispatchResult {
let sender = ensure_signed(origin.clone())?;
let destination: Location = (*dest.clone())
.try_into()
.map_err(|()| Error::<T>::BadVersion)?;

T::PreTransfer::check(TransferEffects::TransferWithFee {
sender,
destination,
destination: (*dest.clone()),
currency_id: currency_id.clone(),
amount,
fee,
Expand Down Expand Up @@ -279,13 +270,10 @@ pub mod pallet {
let fee_asset: Asset = (*fee.clone())
.try_into()
.map_err(|()| Error::<T>::BadVersion)?;
let destination: Location = (*dest.clone())
.try_into()
.map_err(|()| Error::<T>::BadVersion)?;
Comment on lines -282 to -284
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! It seems like the it wanted to be so 👍🏻


T::PreTransfer::check(TransferEffects::TransferMultiAssetWithFee {
sender,
destination,
destination: (*dest.clone()),
asset: multi_asset,
fee_asset,
})?;
Expand Down Expand Up @@ -324,16 +312,13 @@ pub mod pallet {
dest_weight_limit: WeightLimit,
) -> DispatchResult {
let sender = ensure_signed(origin.clone())?;
let destination: Location = (*dest.clone())
.try_into()
.map_err(|()| Error::<T>::BadVersion)?;
let fee = currencies
.get(fee_item as usize)
.ok_or(orml_xtokens::Error::<T>::AssetIndexNonExistent)?;

T::PreTransfer::check(TransferEffects::TransferMultiCurrencies {
sender,
destination,
destination: (*dest.clone()),
currencies: currencies.clone(),
fee: fee.clone(),
})?;
Expand Down Expand Up @@ -375,16 +360,13 @@ pub mod pallet {
let multi_assets: Assets = (*assets.clone())
.try_into()
.map_err(|()| Error::<T>::BadVersion)?;
let destination: Location = (*dest.clone())
.try_into()
.map_err(|()| Error::<T>::BadVersion)?;
let fee_asset: &Asset = multi_assets
.get(fee_item as usize)
.ok_or(orml_xtokens::Error::<T>::AssetIndexNonExistent)?;

T::PreTransfer::check(TransferEffects::TransferMultiAssets {
sender,
destination,
destination: (*dest.clone()),
assets: multi_assets.clone(),
fee_asset: fee_asset.clone(),
})?;
Expand Down
Loading
Loading