Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Runtime: Treasury spends various asset kinds #7427

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Jun 26, 2023

Polkadot/Kusams/Rococo Treasury can accept proposals for spends of various asset kinds by specifying the asset's location and ID.

Treasury Pallet New Dispatchables:

  • spend(AssetKind, AssetBalance, Beneficiary, Option<ValidFrom>) - propose and approve a spend;
  • payout(SpendIndex) - payout an approved spend or retry a failed payout
  • check_payment(SpendIndex) - check the status of a payout;
  • void_spend(SpendIndex) - void previously approved spend;

existing spend dispatchable renamed to spend_local

in this context, the AssetKind parameter contains the asset's location and it's corresponding asset_id, for example:
USDT on AssetHub,
location = MultiLocation(0, Parachain(1000))
asset_id = MultiLocation(0, (PalletInstance(50), GeneralIndex(1984)))

the Beneficiary parameter is a MultiLocation in the context of the asset's location, for example
FellowshipSalaryPallet = MultiLocation(1, (Parachain(1001), PalletInstance(64)))
or Alice = MultiLocation(0, AccountId32(network: None, id: [1,...]))

the AssetBalance represents the amount of the AssetKind to be transferred to the Beneficiary. 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 different ValidFrom 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 the void_spend dispatchable.

Asset Rate Pallet provides the conversion rate from the AssetKind to the native balance.

Asset Rate Pallet Dispatchables:

  • create(AssetKind, Rate) - initialize a conversion rate to the native balance for the given asset
  • update(AssetKind, Rate) - update the conversion rate to the native balance for the given asset
  • remove(AssetKind) - remove an existing conversion rate to the native balance for the given asset

the pallet's dispatchables can be executed by the Root or Treasurer origins.

companion for paritytech/substrate#14434

cumulus companion: paritytech/cumulus#2902 // and xcm-emulator based tests

@muharem muharem added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. T1-runtime This PR/Issue is related to the topic “runtime”. B1-note_worthy Changes should be noted in the release notes labels Jul 4, 2023
@muharem muharem marked this pull request as ready for review July 4, 2023 17:57
@paritytech-ci paritytech-ci requested review from a team July 4, 2023 17:58
@muharem muharem requested a review from a team July 13, 2023 15:48
@muharem
Copy link
Contributor Author

muharem commented Jul 30, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@muharem
Copy link
Contributor Author

muharem commented Aug 1, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3317528

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 }
Copy link
Contributor

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?

Copy link
Contributor Author

@muharem muharem Aug 16, 2023

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@muharem muharem Aug 23, 2023

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.

Copy link
Contributor

@KiChjang KiChjang Aug 24, 2023

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();
Copy link
Contributor

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Development

Successfully merging this pull request may close these issues.

5 participants