refactor: Use VersionedLocation in RestrictedTransferLocation
#1861
Conversation
wischli
added
D2-notify
| @@ -29,105 +32,282 @@ use super::*; | |||
|
|
|||
| const BENCHMARK_CURRENCY_ID: FilterCurrency = FilterCurrency::Specific(CurrencyId::ForeignAsset(1)); | |||
|
|
|||
| benchmarks! { | |||
There was a problem hiding this comment.
NOTE: Apart from updating to v2 and adjusting the where clause a bit as well as providing the default_location wrapper, I did not change anything here apart from removing redundant clones.
…n' into wf/polkadot-v1.7.2-restricted-transfer-location
| chain="development-local" | ||
| chain="development" |
There was a problem hiding this comment.
Feature creep: Remainder from removing the redundant development-local in #1844.
| let destination: Location = (*dest.clone()) | ||
| .try_into() | ||
| .map_err(|()| Error::<T>::BadVersion)?; |
There was a problem hiding this comment.
Cool! It seems like the it wanted to be so 👍🏻
| Deserialize, | ||
| Serialize, | ||
| )] | ||
| pub enum Location { |
There was a problem hiding this comment.
Personally, I liked this in order to not import centrifuge runtime types in pallets. Did you find some issues with the current Location used here?
|
|
||
| add_transfer_allowance_no_existing_metadata { | ||
| #[benchmark] |
There was a problem hiding this comment.
Thanks for organizing this!
libs/types/src/locations.rs
Outdated
|
|
||
| impl From<AccountId32> for RestrictedTransferLocation { | ||
| fn from(value: AccountId32) -> Self { | ||
| Self::Local(value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Did you have an error with this? I remember runtime benchmarks not working without this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Unfortunately, it was not possible to implement From/TryFrom for AccountId and RestrictedTransferLocation
The above compiled in main IIRC 🤔
There was a problem hiding this comment.
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 🤷
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I am happy to hear even you don't understand it. It really frustrated me yesterday..
|
Click the button when you want! |
* migration for restricted transfer location * add correct location for centrifuge * minor changes * fix benchmarks * fix IT compilation * chore: extend v1.7.2 transfer location migration (#1858) * feat: add pre + post-upgrade * feat: add nuking migration for dev and demo * fix: remove account list for demo * refactor: Use `VersionedLocation` in `RestrictedTransferLocation` (#1861) * feat: add pre + post-upgrade * feat: add nuking migration for dev and demo * fix: remove account list for demo * refactor: use VersionedLocation * fix: transfer allowlist benchmarks + tests * fix: clippy * fix: benchmark cli * fix: revert tight coupling * fmt * fix IT compilation --------- Co-authored-by: William Freudenberger <[email protected]>
Description
v4::LocationofRestrictedTransferLocation::XCMwithVersionedLocationWhy so many benchmark and test changes?
This change broke transfer allowlist benchmarks which required
T::AccountId <-> T::Locationconversion. Unfortunately, it was not possible to implementFrom/TryFromforAccountIdandRestrictedTransferLocationbecause of the the blanket RustFrom<T> for Timplementation which conflicts with any custom implementation involving the same type for bothFromandTryFrom.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
u64asAccountIdand a custom enum forT::Locationwhich conflicted with my new benchmark restriction ofConfig<AccountId = AccountId, Location = RestrictedTransferLocation.Due to some weird error logs, I also converted the benchmarks from v1 to v2.
Checklist: