-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Runtime: Treasury spends various asset kinds #7427
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of hardcoding 18
here, I propose that we fetch it programmatically using the PalletInfo
trait.
Polkadot/Kusams/Rococo Treasury can accept proposals for
spends
of variousasset kinds
by specifying the asset's location and ID.Treasury Pallet New Dispatchables:
in this context, the
AssetKind
parameter contains the asset'slocation
and it's correspondingasset_id
, for example:USDT on AssetHub,
location = MultiLocation(0, Parachain(1000))
asset_id = MultiLocation(0, (PalletInstance(50), GeneralIndex(1984)))
the
Beneficiary
parameter is aMultiLocation
in the context of the asset'slocation
, for exampleFellowshipSalaryPallet = MultiLocation(1, (Parachain(1001), PalletInstance(64)))
or Alice = MultiLocation(0, AccountId32(network: None, id: [1,...]))
the
AssetBalance
represents the amount of theAssetKind
to 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
spend
dispatchable allows for batching spends with differentValidFrom
arguments, 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_spend
dispatchable.Asset Rate Pallet provides the conversion rate from the
AssetKind
to the native balance.Asset Rate Pallet Dispatchables:
the pallet's dispatchables can be executed by the
Root
orTreasurer
origins.companion for paritytech/substrate#14434
cumulus companion: paritytech/cumulus#2902 // and xcm-emulator based tests