Runtime: Treasury spends various asset kinds#7427
Conversation
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
|
bot rebase |
|
Rebased |
|
bot rebase |
|
Rebased |
|
The CI pipeline was cancelled due to failure one of the required jobs. |
| pub struct LocatableAssetIdConverter; | ||
| impl Convert<LocatableAssetId, xcm_builder::LocatableAssetId> for LocatableAssetIdConverter { | ||
| fn convert(a: LocatableAssetId) -> xcm_builder::LocatableAssetId { | ||
| xcm_builder::LocatableAssetId { asset_id: a.asset_id, location: a.location } |
There was a problem hiding this comment.
This looks silly, why can't we just directly reuse xcm_builder::LocatableAssetId instead of declaring a new LocatableAssetId here in this module?
There was a problem hiding this comment.
I do not like that a change in an external library can change the signature of the spend dispatchable.
Where xcm_builder::LocatableAssetId is not part of XCM protocol, or meant to be used as a dispatchable's type parameter.
There was a problem hiding this comment.
I don't see why you have to prepare for this? If upstream changes, then you can think of creating a new type retaining the old fields, but this now is a bit overoptimized.
There was a problem hiding this comment.
the change to the dispatchable signature will invalidate all spend proposals which has been submitted before and still active.
and such a change might be not noticed (this we can address with a test).
but this why I made it this way.
There was a problem hiding this comment.
Here's a better idea: why not have the spend dispatchable directly take in Box<VersionedMultiLocation> and Box<VersionedAssetId> as parameters? This should have been the proper way of accepting versioned XCM parameters anyway.
There was a problem hiding this comment.
I like the idea with versioned types, this is probably how we should do it.
We need to combine both type into one type, since it is meant for pallet_treasury::Config::AssetKind type parameter. This AssetKind can be just u32 for example.
It can be same as LocatableAssetId but with versioned versions of MultiLocation and AssetId,
pub struct LocatableAssetId {
pub location: VersionedMultiLocation,
pub asset_id: VersionedAssetId,
}
or i would better do something like
enum VersionedLocatableAsset {
#[codec(index = 3)]
V3{location: v3::MultiLocation, asset_id: v3::AssetId},
}
this way we can control which versions relevant right now specifically for treasury proposals.
There was a problem hiding this comment.
Hmm, so what you seem to be saying is the parameters to the spend extrinsic is dependent on what the concrete configured type is for pallet_treasury::Config::AssetKind, so if it ever changes, then the signature of the spend extrinsic must also necessarily change. Given that's the case, why would we then allow AssetKind to be configurable? It would seem to me that we should instead make it not configurable, so as to preserve API compatibility across versions.
| pub const PayoutSpendPeriod: BlockNumber = 30 * DAYS; | ||
| // The asset's interior location for the paying account. This is the Treasury | ||
| // pallet instance (which sits at index 18). | ||
| pub TreasuryInteriorLocation: InteriorMultiLocation = PalletInstance(18).into(); |
There was a problem hiding this comment.
Instead of hardcoding 18 here, I propose that we fetch it programmatically using the PalletInfo trait.
Polkadot/Kusams/Rococo Treasury can accept proposals for
spendsof variousasset kindsby specifying the asset's location and ID.Treasury Pallet New Dispatchables:
in this context, the
AssetKindparameter contains the asset'slocationand it's correspondingasset_id, for example:USDT on AssetHub,
location = MultiLocation(0, Parachain(1000))
asset_id = MultiLocation(0, (PalletInstance(50), GeneralIndex(1984)))
the
Beneficiaryparameter is aMultiLocationin the context of the asset'slocation, for exampleFellowshipSalaryPallet = MultiLocation(1, (Parachain(1001), PalletInstance(64)))
or Alice = MultiLocation(0, AccountId32(network: None, id: [1,...]))
the
AssetBalancerepresents the amount of theAssetKindto be transferred to theBeneficiary. For permission checks, the asset amount is converted to the native amount and compared against the maximum spendable amount determined by the commanding spend origin.the
spenddispatchable allows for batching spends with differentValidFromarguments, enabling milestone-based spending. If the expectations tied to an approved spend are not met, it is possible to void the spend later using thevoid_spenddispatchable.Asset Rate Pallet provides the conversion rate from the
AssetKindto the native balance.Asset Rate Pallet Dispatchables:
the pallet's dispatchables can be executed by the
RootorTreasurerorigins.companion for paritytech/substrate#14434
cumulus companion: paritytech/cumulus#2902 // and xcm-emulator based tests