Pallets: Treasury `spend()` should use `Pay` trait #13607

tonyalaribe · 2023-03-15T13:56:22Z

Motivation

To support a more flexible treasury pallet, we need to prevent strongly coupling the treasury implementation to the Currency, fungible or fungibles traits.
Instead, this PR begins an attempt at a treasury pallet which is implemented over a more generic trait, which could then be implemented over fungibles, fungible, or even over pallet_xcm. Opening more possibilities including the ability for the treasury to control and spend assets which are available on remote parachains.

Approach

This PR leverages the new pay trait, as the means of actually implementing spend. The long term goal will be to deprecate the other non-spend extrinsics. Eg propose_spend(), reject_proposal, etc, so that the spend() extrinsic becomes the de-factor way of using spend().

A user story could be that all the proposal management logic happens through a referendum from the respective pallet, and that referendum is able to trigger the spend() action in the treasury pallet, which gets paid out at the next SpendPeriod. By doing this, the treasury becomes much simpler in scope, and becomes a pallet which can be used to control spending or paying out assets from accounts controlled by an origin.

So in practical terms, it could be possible to have many treasury instances for every account which is controlled by a collective. Eg the Governance could control a treasury over funds in statemint and control a treasury over funds on even a remote parachain or on the local chain, via the respective implementations of pay: PayFromAccount, PayFungibles, PayOverXcm or even a hypothetical PayNonFungible

polkadot companion: paritytech/polkadot#7058

TODO

Avoid depending on Assets and Balances pallet.

frame/treasury/src/lib.rs

…et_salary

frame/treasury/src/lib.rs

…s an identity function or transformation

…ytech/substrate into aa/multi-asset-support-in-treasury

frame/treasury/src/lib.rs

muharem · 2023-05-08T15:44:24Z

frame/treasury/src/lib.rs

+							});
+						},
+					},
+					Some(payment_id) => match T::Paymaster::check_payment(payment_id) {


we should be timing out after some timeout

pallet_xcm does not timeout now. but I think it should be on both sides

what do you propose?
With the current implementation, we would simply keep retrying the transaction each spend period until it succeeds, and that seems sufficient to me.

Also, the treasury pallet knows nothing about the XCM timeouts nor does it even know anything about XCM.
Eg lots of parachains will implement their treasuries using the PayFromAccount or PayFungibles implementations which have no timeouts. So i'm not convinced about adding any timeout logic into the substrate treasury implementation, as that is too specific to XCM

the treasury pallet does not need to know about XCM to have a timeout for some async status checks it does.
if for some reason it never gets some of the fail/success statuses, by this design it keeps checking the status forever.

I added a concept of retries, so we retry a payment a given number of times.
About the timeouts, It would only be useful in the XCM context, and in the case of XCM, when it expires, the check_payment method would return a status unknown, which we now treat as a failure, so it would fail immediately at that point. I think with that workflow, there won't be any extra utility for the timeouts, and if that use-case comes up, then we can include it in a subsequent PR.

frame/treasury/src/lib.rs

muharem · 2023-05-09T09:50:51Z

frame/treasury/src/lib.rs

+								"Paymaster::pay failed for PendingPayment with index: {:?}",
+								key
+							);
+							// try again in the next `T::SpendPeriod`.


I think the payment statuses should be check with a different interval, not SpendPeriod. if we separate the pay job and the check status one, we will be able to have more accurate weights for these jobs.

That makes sense. let me explore this more

muharem · 2023-05-09T09:52:27Z

frame/treasury/src/lib.rs

+							});
+							// Force the payment to none, so a fresh payment is sent during the next
+							// T::SpendPeriod.
+							p.payment_id = None;


which means we retry endlessly, looks wrong to me.

I think there's no harm. This gives time for governance to solve the reason for the failure. But what would you propose?

what if its not recoverable fail, how governance will solve it?

muharem · 2023-05-09T09:57:42Z

frame/treasury/src/lib.rs

+			}
+		}
+
+		total_weight += T::WeightInfo::on_initialize_pending_payments(proposals_len);


I think we should be checking if there is enough capacity left before executing pay, scheduler is a good example

By scheduler, what do you mean? Maybe point me to a file or code line of what you mean

scheduler pallet, jobs on_initialize

muharem · 2023-05-09T10:03:53Z

frame/treasury/src/lib.rs

 			// Check to see if we should spend some funds!
 			if (n % T::SpendPeriod::get()).is_zero() {
-				Self::spend_funds()
+				Self::spend_funds().saturating_add(Self::spend_funds_local())


there might be not enough weight capacity left for spend_funds_local .
and spend period might be quite long, for polkadot it is 24 days.

muharem · 2023-05-09T10:08:00Z

frame/treasury/src/benchmarking.rs

+		create_pending_payments::<T,_>(p)?;
+	}: {
+		Treasury::<T, _>::on_initialize(T::BlockNumber::zero());
+	}


we need some check here that pay was executed

muharem · 2023-05-09T10:10:05Z

frame/treasury/src/benchmarking.rs

@@ -138,5 +183,12 @@ benchmarks_instance_pallet! {
 		Treasury::<T, _>::on_initialize(T::BlockNumber::zero());
 	}

+	on_initialize_pending_payments {
+		let p in 0 .. T::MaxApprovals::get();


MaxApprovals limit is not enforced for pending payments

muharem · 2023-05-09T10:20:54Z

frame/treasury/src/tests.rs

+		<Treasury as OnInitialize<u64>>::on_initialize(2);
+		assert_eq!(Assets::reducible_balance(0, &Treasury::account_id(), Expendable, Polite), 0);
+		assert_eq!(Assets::reducible_balance(0, &6, Expendable, Polite), 100);
+	});


looks like many cases of the new functionality is not covered, would be good to have more tests

tonyalaribe · 2023-05-26T09:22:48Z

I made some larger changes to the spend method, such that it now accepts two main arguments.

A vector of assets
A beneficiary

The vector of assets can be anything that encodes a way to retrieve a balance from it, but in the kusama/runtimes context, it also allows us to use a type that the PayOverXcm can support. Here's a video demo of the new workflow:

Polkadot_Substrate.Portal.-.26.May.2023.mp4

gavofyork · 2023-05-29T11:35:11Z

Why a vec of assets? This introduces problematic edge cases - e.g. what happens if one asset can be transferred, but another later fails? A pallet/user can always make a batch of spend calls.

gavofyork · 2023-05-29T11:41:37Z

Can you make a separate PR for the alterations to trait Pay since it's non-controversial and depended upon by other downstream code (e.g. paritytech/polkadot#6900) and it would be nice to merge soon.

EDIT: Done in #14258

joepetrowski · 2023-05-30T06:53:00Z

Why a vec of assets? This introduces problematic edge cases - e.g. what happens if one asset can be transferred, but another later fails? A pallet/user can always make a batch of spend calls.

This came up on a call where we were discussing the API, and IIRC the idea was that for now we ensure!(assets.len() == 1) until we are confident with such edge case handling, but if there were a use case in the future that spend proposals contained many assets, we wouldn't have to break the API. A batch of many spends wouldn't account for budget and put it on the right track, while a single spend with a vec of assets should properly account for the total value and enforce the origin correctly.

tonyalaribe · 2023-05-30T07:27:38Z

Why a vec of assets? This introduces problematic edge cases - e.g. what happens if one asset can be transferred, but another later fails? A pallet/user can always make a batch of spend calls.

Just to add to what Joe said, another reason was to be consistent with some other APIs, eg how the initiate_teleport method accepts a vector of assets and a destination. So this could be more flexible long term and we won't have to break the API contract in a few months to support spending multiple asset classes in a single transaction.

…ytech/substrate into aa/multi-asset-support-in-treasury

gavofyork · 2023-05-30T14:50:49Z

Why not introduce spend_many if/when we decide we need to support multi-asset spends? I'm still not sure what the advantage is over batch([spend, spend, ...])...

tonyalaribe · 2023-05-30T17:51:15Z

Why not introduce spend_many if/when we decide we need to support multi-asset spends? I'm still not sure what the advantage is over batch([spend, spend, ...])...

Yeah, I agree with this. I would revert to spends on a single asset for now, except someone has a counterargument.

paritytech-cicd-pr · 2023-06-05T16:29:49Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2946290

muharem · 2023-06-26T13:53:10Z

closing in favour of #14434