feat: Treasury config pallet

[czareko]

No description provided.

[illuzen]

I would prefer not to have a 0 address fallback if decoding fails but it shouldn't fail if we have it set so it it's probably fine

[n13]

postmortem PR review

---

PR #372 Review: Treasury Config Pallet

Overview

This PR replaces Substrate's full pallet-treasury (with spend proposals, payouts, custom origins, etc.) with a minimal custom config pallet that stores only the treasury account address and mining-reward portion. It also removes the pallet_custom_origins module, 4 treasury governance tracks, and all associated spend tests.

---

Critical Issues

1. Naming collision: pallet-treasury

The new crate is named pallet-treasury in Cargo.toml, which is the same name as Substrate's pallet-treasury. This completely shadows the upstream crate and will cause confusion for anyone reading the code. Consider naming it pallet-treasury-config or pallet-treasury-params to clearly distinguish it from the well-known Substrate pallet.

2. Pallet index renumbering is a breaking change

All pallet indices from 6 onward are renumbered (e.g., MiningRewards 7->6, Preimage 9->7, etc.). Pallet indices are baked into storage key prefixes. If this testnet has any existing state, this will break all storage reads for the affected pallets. If this is intentional (chain reset), it should be documented. If not, the old indices should be preserved.

3. Overly complex account_id() fallback

pub fn account_id() -> T::AccountId {
    TreasuryAccount::<T>::get().unwrap_or_else(|| {
        T::AccountId::decode(&mut sp_runtime::traits::TrailingZeroInput::zeroes())
            .unwrap_or_else(|_| {
                T::AccountId::decode(&mut &[0u8; 32][..])
                    .unwrap_or_else(|_| panic!("Cannot create fallback AccountId"))
            })
    })
}

This has nested fallbacks with a panic! in the innermost path. Since the genesis config always sets the account, this complexity is unnecessary and the panic path is a liability. Consider either making the storage ValueQuery with a sensible default, or simply panicking immediately if the storage is empty (which indicates a misconfiguration).

---

Design Concerns

4. u8 portion loses precision vs Permill

Previously TreasuryPortion was a Permill (parts-per-million precision). Now it's a u8 (0-100), which limits precision to whole percentages. The conversion Permill::from_percent(u32::from(treasury_portion)) is then repeated 13+ times across mining-rewards tests — a clear DRY violation. At minimum, the TreasuryProvider trait should return Permill directly, or a helper function should encapsulate the conversion.

5. Massive scope removal without explanation

The PR removes:

• All treasury spend/payout functionality
• RuntimeNativePaymaster and RuntimeNativeBalanceConverter
• pallet_custom_origins (SmallSpender, MediumSpender, BigSpender, Treasurer)
• 4 treasury governance tracks (tracks 2-5 from community referenda)
• ~800+ lines of treasury spend tests

The PR description is empty. This is a significant governance capability reduction that deserves a clear rationale — is treasury spending being reimplemented differently? Deferred to a future PR? Permanently removed?

6. No storage migration

Switching from Substrate's pallet-treasury to this custom one changes the storage layout entirely. Existing chains would need a migration to clean up old storage and populate the new TreasuryAccount/TreasuryPortion values.

---

Minor Issues

7. Unused getter structs

TreasuryAccountGetter<T> and TreasuryPortionGetter<T> are defined but never used anywhere in the PR. If they're for future use, they should be removed until needed (YAGNI).

8. Double #[cfg(test)] gating

In pallets/treasury/src/tests.rs, the entire file is wrapped in #[cfg(test)] mod tests { ... }, but the file is already conditionally compiled via #[cfg(test)] mod tests; in lib.rs. The inner #[cfg(test)] is redundant.

9. set_treasury_account accepts any account

There's no validation that the new treasury account is non-zero or otherwise valid. While root-only access is a reasonable guard, a zero/default account would silently break reward distribution.

10. Bare use pallet_treasury; import

In pallets/mining-rewards/src/mock.rs, the bare use pallet_treasury; may produce an unused-import warning depending on how the compiler resolves it vs the construct_runtime! macro.

---

What Looks Good

• The TreasuryProvider trait is a clean abstraction for mining-rewards integration
• Benchmarks are properly structured with v2 macros
• Weight file includes actual benchmark data
• Genesis config with validation (portion <= 100) is solid
• Root-only access for both setters is appropriate
• The pallet tests cover the core paths (genesis, setters, authorization, validation)

---

Summary

The core idea — a lightweight config pallet for treasury account/portion — is sound. However, the PR needs attention on:

1. Rename the crate to avoid shadowing Substrate's pallet-treasury
2. Preserve pallet indices or document the chain reset
3. Simplify the account_id() fallback
4. Extract the Permill::from_percent(u32::from(...)) pattern into a helper (or return Permill from the trait)
5. Add PR description explaining why full treasury-spend functionality is being removed
6. Remove unused TreasuryAccountGetter/TreasuryPortionGetter (or use them)