-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[wip] XCM executor keeps track and resolves all imbalances created by XCM operations #10384
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.
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
base: master
Are you sure you want to change the base?
Conversation
_Context_ Asset Hub currently supports registering following types of assets: 1. Local Assets (also called trust-based assets), with `u32` asset ID, asset creatable by Signed accounts and managed by `PalletInstance(50)`. 2. Foreign Assets, `Location` as asset ID, creatable only by external locations over XCM. For "foreign assets", the current code assumes and enforces that the asset is teleportable with its original location/creator. Some ecosystem parachains would want to register foreign assets on Asset Hub, but not also have them teleportable, not have Asset Hub act as a reserve for the asset. Multiple reserve locations adds reserve management overhead for teams, and some don't want to do it. E.g. for HOLLAR to be registered to Asset Hub, Hydration wants Asset Hub to allow reserve-based foreign assets. _Solution_ This PR adds a new storage item and new call to `pallet-assets` that can be used to configure the trusted reserves (as seen by the local chain) on a per-asset basis. It also adds runtime machinery to use the assets reserves information when reasoning about XCM teleports and XCM reserve-based transfers. For each asset, the base chain described in its asset id location is considered a default reserve chain without it having to be explicitly configured. Any other chain that should be assumed as a trusted reserve needs to be marked as such in `pallet-assets-reserves` by the asset's Owner or Admin. When the local chain is also marked as a reserve (`Here` is added to the list of reserves) for a particular asset, then that asset can be teleported between the local chain and any other reserve location. By default, on asset creation, no explicit reserve chain is configured, this results in foreign assets to be reserve-based assets by default and not teleportable. To make them teleportable with their origin chain, `Here` needs to be added as a reserve (local chain to also be a reserve). _AH considerations_ Since there are already a number of Foreign Assets already registered, and have been teleportable since registration, this PR also adds a migration that adds `Here` as a reserve for existing foreign assets, so as not to change any existing behaviors. Newly registered foreign assets will not be teleportable by default, which is the desired result of this PR. They can be configured to be teleportable by the asset's Owner or Admin, post-creation. Signed-off-by: Adrian Catangiu <[email protected]> move reserves config to pallet-assets
…nce-pallet-assets-with-reserves
…nce-pallet-assets-with-reserves
…nce-pallet-assets-with-reserves
…nce-pallet-assets-with-reserves
…nce-pallet-assets-with-reserves
Co-authored-by: Francisco Aguirre <[email protected]>
Co-authored-by: Francisco Aguirre <[email protected]>
…ime asset-hub-westend'" This reverts commit d6838f3. The benchmark changes pollute the PR too much. We'll add them in a dedicated PR.
Co-authored-by: Branislav Kontur <[email protected]>
…executor-annoyance
muharem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, this is a big effort. looks good to me. It does make things even more complex, though, but I don’t have a fundamentally better idea. I left a few suggestions that might make things a bit safer.
this definitely requires us to write new tests for all affected types to verify the correct total issuance.
| Err(XcmError::AssetNotFound) | Err(XcmError::Unimplemented) => (), | ||
| // Err((unspent, error)) if error == XcmError::AssetNotFound || error == XcmError::Unimplemented => (), | ||
| Err((unspent, XcmError::AssetNotFound)) | Err((unspent, XcmError::Unimplemented)) => { | ||
| what = unspent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should what be just the last? or sum?
same comment applies to deposit_asset_with_surplus
| /// The general asset trap - handler for when assets are left in the Holding Register at the | ||
| /// end of execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and the handler for when there is an instruction to claim assets.
| maybe_failed_bin.as_mut().map(|f| f.non_fungible.insert((class, inst))); | ||
| None | ||
| }, | ||
| assets.sort(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we start sorting? what if I indexed the fee asset, index might change.
|
|
||
| /// Return all inner assets, but interpreted from the perspective of a `target` chain. The local | ||
| /// chain's `context` is provided. | ||
| pub fn reanchored_assets(&self, target: &Location, context: &InteriorLocation) -> Assets { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this basically same as reanchor_and_burn_local but does not burn and at the same time still returns Assets. may be also warn comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this unfortunately means that Assets might represent amounts and imbalances too
| (*self_amount == 0, amount) | ||
| // Ok to use `saturating_take()` because we checked with | ||
| // `self.ensure_contains()` above against `saturate` flag | ||
| let balance = self_amount.saturating_take(amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if amount < self_amount, this wont fail and will remove the self_amount. not sure if this an issue.
| holding.drop_without_accounting(); | ||
| let versioned = VersionedAssets::from(Assets::from(assets)); | ||
| let hash = BlakeTwo256::hash_of(&(&origin, &versioned)); | ||
| AssetTraps::<T>::mutate(hash, |n| *n += 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, this is also by itself is a credit, may be worth documenting
| } | ||
| let mut claimed = AssetsInHolding::new(); | ||
| for asset in assets.inner() { | ||
| match <T::XcmExecutor as XcmAssetTransfers>::AssetTransactor::mint_asset(asset, context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be minted? on asset trap above we drop_without_accounting
| self.amount = self.amount.saturating_add(other.amount); | ||
| core::mem::forget(other); | ||
| } | ||
| fn subsume_amount(&mut self, amount: B) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be pub (crate) subsume_amount like forget above?
otherwise we make safe trait unsafe.
|
|
||
| /// Helper trait to be used for generic Imbalance, helpful for tracking multiple concrete types of | ||
| /// `Imbalance` using dynamic dispatch of this trait. | ||
| pub trait ImbalanceAccounting<Balance> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be ImbalanceAccounting: Unsafe
where we decouple subsume_amount, unsafe_clone and forget_amount to Unsafe
| use core::mem; | ||
| use sp_runtime::{traits::Saturating, RuntimeDebug}; | ||
| use core::{fmt::Formatter, mem}; | ||
| use frame_support::traits::tokens::imbalance::ImbalanceAccounting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we break the rule of xcm-executor not depending on frame. to not do it we could have an adapter in the builder. not blocker for me, just heads-up.
This reverts commit e9e5946. This doesn't work because we are actually changing types across the stack, we use UnionOf combinations to combine Fungible and Fungibles types into new "union types" so downcasting to specific types becomes very complex...
3f3726e to
f1311eb
Compare
TODO