pallet-treasury: Ordered Payouts#11603
Conversation
|
please review @muharem |
dhirajs0
left a comment
There was a problem hiding this comment.
I would suggest reviewing the RFC again as it has been updated to address some issues.
| SpendCount::<T, I>::put(index + 1); | ||
|
|
||
| // Insert into payout queue for this asset kind | ||
| Self::insert_into_payout_queue(*asset_kind.clone(), index, valid_from)?; |
There was a problem hiding this comment.
Weights: spend , payout , check_status now do an extra PayoutQueue read + write (a BoundedVec up to MaxQueuedSpends) plus an O(n) scan/insert, but still use the existing WeightInfo.
| _, | ||
| Twox64Concat, | ||
| T::AssetKind, | ||
| BoundedVec<(SpendIndex, BlockNumberFor<T, I>), T::MaxQueuedSpends>, |
There was a problem hiding this comment.
Tied to my comments on the rotation/invariant: since this field isn't always valid_from (the RFC uses now for rotated entries), renaming it to order_key in the type doc and locals might make the rotation semantics clearer and help prevent ordering drift.
|
/cmd bench --pallet pallet_treasury |
|
Command "bench --pallet pallet_treasury" has started 🚀 See logs here |
|
Command "bench --pallet pallet_treasury" has finished ✅ See logs here DetailsSubweight results:
Command output:❌ Failed to build westend |
muharem
left a comment
There was a problem hiding this comment.
and bug fix that we in RFC initially: jessechejieh#2
| sp_runtime::TryRuntimeError::Other("Spend in queue must exist in Spends."), | ||
| )?; | ||
| ensure!( | ||
| matches!(spend.status, PaymentState::Pending | PaymentState::Failed), |
There was a problem hiding this comment.
why cannot this be Attempted?
| ); | ||
| } | ||
| }, | ||
| PaymentState::Attempted { .. } => { |
There was a problem hiding this comment.
why tho? if the fail they will be lost
| System::set_block_number(10); | ||
| assert_ok!(Treasury::payout(RuntimeOrigin::signed(1), 0)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
please add these tests
- Preemption: later-approved, earlier-maturing spend takes the head; the far-future spend is demoted — and is then payable immediately.
- Order-key clamp: a later spend with back-dated valid_from does not overtake an already-mature head (key clamped to max(now, valid_from)).
- Preemption among not-yet-mature spends: earlier valid_from becomes head, both unpayable before maturity.
- Preempt into a full queue → QueueFull: the demotion try_insert makes this newly reachable from spend (distinct from the existing queue_full_scenario, which fills via plain inserts).
- End-to-end stall: far-future head + later mature spend → mature spend is paid first; covers payout + ordering together.
Fix payout-order fairness in treasury FIFO queue
|
/cmd bench --pallet pallet_treasury |
|
Command "bench --pallet pallet_treasury" has started 🚀 See logs here |
|
Command "bench --pallet pallet_treasury" has finished ✅ See logs here DetailsSubweight results:
Command output:❌ Failed to build westend |
|
/cmd fmt |
|
/cmd prdoc |
|
Command "prdoc" has failed ❌! See logs here |
|
/cmd bench --runtime dev asset-hub-westend-runtime collectives-westend-runtime --pallet pallet_treasury |
|
Command "bench --runtime dev asset-hub-westend-runtime collectives-westend-runtime --pallet pallet_treasury" has started 🚀 See logs here |
|
Command "bench --runtime dev asset-hub-westend-runtime collectives-westend-runtime --pallet pallet_treasury" has failed ❌! See logs here |
|
/cmd bench --runtime asset-hub-westend --pallet pallet_treasury |
|
Command "bench --runtime asset-hub-westend --pallet pallet_treasury" has started 🚀 See logs here |
…t-hub-westend --pallet pallet_treasury'
|
Command "bench --runtime asset-hub-westend --pallet pallet_treasury" has finished ✅ See logs here DetailsSubweight results:
Command output:✅ Successful benchmarks of runtimes/pallets: |
| /// already expired, and keeping in-flight `Attempted` ones), groups them by asset kind, sorts each | ||
| /// group by order key `max(now, valid_from)` then by index, and initializes `NextPayout` and | ||
| /// `PayoutQueue` for each asset kind. | ||
| mod migrate_to_ordered_payouts { |
There was a problem hiding this comment.
maybe we can bump the storage version of pallet treasury, and use the storage version to against the re-execution of this migration. (use the #[pallet::storage_version] and use frame_support::migrations::VersionedMigration)
There was a problem hiding this comment.
Done! Plus unit testing.
Also verified against real state. try-runtime on live Westend Asset Hub (empty Spends there, so it checks the gate + idempotency), and a chopsticks fork of live Polkadot Asset Hub.
Vendored this version of the pallet into the fellowship runtime, the migration ordered 266 actual treasury spends.
try-runtime, live Westend Asset Hub:
[2026-07-10T12:34:35Z INFO frame_support::migrations] 🚚 Pallet "Treasury" VersionedMigration migrating storage version from 0 to 1.
[2026-07-10T12:34:35Z INFO runtime::treasury] Running migration to initialize ordered payouts
[2026-07-10T12:34:35Z INFO runtime::treasury] Migration complete: processed 0 spends across 0 asset kinds
[2026-07-10T12:34:35Z INFO try-runtime::cli] 🔬 Running TryRuntime_on_runtime_upgrade again to check idempotency: PreAndPost
[2026-07-10T12:34:37Z WARN frame_support::migrations] 🚚 Pallet "Treasury" VersionedMigration migration 0->1 can be removed; on-chain is already at StorageVersion(1).
[2026-07-10T12:34:37Z INFO try-runtime::cli] 🔬 TryRuntime_on_runtime_upgrade succeeded! Running it again for weight measurements.Chopsticks fork of live Polkadot Asset Hub (266 real spends):
frame_support::migrations INFO: [4] 🚚 Pallet "Treasury" VersionedMigration migrating storage version from 0 to 1.
runtime::treasury INFO: [4] Running migration to initialize ordered payouts
runtime::treasury DEBUG: [4] Skipping expired spend 98 (expire_at: 25236045, now: 32053717)
runtime::treasury DEBUG: [4] Skipping expired spend 79 (expire_at: 25432000, now: 32053717)
runtime::treasury INFO: [4] Migration complete: processed 264 spends across 7 asset kinds|
/cmd fmt |
|
/cmd label FRAME |
|
Command "label FRAME" has failed ❌! See logs here |
|
/cmd label T2-pallets |
Solves #11100
Adopting Solution 1.2: which sets up nicely for Category payouts #10381