Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removes constraint in BlockNumberProvider from treasury #6522

Merged
merged 5 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions prdoc/pr_6522.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
title: Removes constraint in BlockNumberProvider from treasury

doc:
- audience: Runtime Dev
description: |-
https://github.com/paritytech/polkadot-sdk/pull/3970 updated the treasury pallet to support
relay chain block number provider. However, it added a constraint to the `BlockNumberProvider`
trait to have the same block number type as `frame_system`:

```rust
type BlockNumberProvider: BlockNumberProvider<BlockNumber = BlockNumberFor<Self>>;
```

This PR removes that constraint and allows the treasury pallet to use any block number type.

crates:
- name: pallet-treasury
bump: major
6 changes: 3 additions & 3 deletions substrate/frame/bounties/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ use alloc::{vec, vec::Vec};
use frame_benchmarking::v1::{
account, benchmarks_instance_pallet, whitelisted_caller, BenchmarkError,
};
use frame_system::{pallet_prelude::BlockNumberFor, RawOrigin};
use frame_system::{pallet_prelude::BlockNumberFor as SystemBlockNumberFor, RawOrigin};
use sp_runtime::traits::{BlockNumberProvider, Bounded};

use crate::Pallet as Bounties;
use pallet_treasury::Pallet as Treasury;

const SEED: u32 = 0;

fn set_block_number<T: Config<I>, I: 'static>(n: BlockNumberFor<T>) {
fn set_block_number<T: Config<I>, I: 'static>(n: BlockNumberFor<T, I>) {
<T as pallet_treasury::Config<I>>::BlockNumberProvider::set_block_number(n);
}

Expand Down Expand Up @@ -132,7 +132,7 @@ benchmarks_instance_pallet! {
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
Treasury::<T, I>::on_initialize(SystemBlockNumberFor::<T>::zero());
}: _<T::RuntimeOrigin>(approve_origin, bounty_id, curator_lookup, fee)
verify {
assert_last_event::<T, I>(
Expand Down
19 changes: 12 additions & 7 deletions substrate/frame/bounties/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ use sp_runtime::{
use frame_support::{dispatch::DispatchResultWithPostInfo, traits::EnsureOrigin};

use frame_support::pallet_prelude::*;
use frame_system::pallet_prelude::*;
use frame_system::pallet_prelude::{
ensure_signed, BlockNumberFor as SystemBlockNumberFor, OriginFor,
};
use scale_info::TypeInfo;
pub use weights::WeightInfo;

Expand All @@ -120,6 +122,9 @@ pub type BountyIndex = u32;

type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;

type BlockNumberFor<T, I = ()> =
<<T as pallet_treasury::Config<I>>::BlockNumberProvider as BlockNumberProvider>::BlockNumber;

/// A bounty proposal.
#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
pub struct Bounty<AccountId, Balance, BlockNumber> {
Expand Down Expand Up @@ -213,11 +218,11 @@ pub mod pallet {

/// The delay period for which a bounty beneficiary need to wait before claim the payout.
#[pallet::constant]
type BountyDepositPayoutDelay: Get<BlockNumberFor<Self>>;
type BountyDepositPayoutDelay: Get<BlockNumberFor<Self, I>>;

/// Bounty duration in blocks.
#[pallet::constant]
type BountyUpdatePeriod: Get<BlockNumberFor<Self>>;
type BountyUpdatePeriod: Get<BlockNumberFor<Self, I>>;

/// The curator deposit is calculated as a percentage of the curator fee.
///
Expand Down Expand Up @@ -326,7 +331,7 @@ pub mod pallet {
_,
Twox64Concat,
BountyIndex,
Bounty<T::AccountId, BalanceOf<T, I>, BlockNumberFor<T>>,
Bounty<T::AccountId, BalanceOf<T, I>, BlockNumberFor<T, I>>,
>;

/// The description of each bounty.
Expand Down Expand Up @@ -876,9 +881,9 @@ pub mod pallet {
}

#[pallet::hooks]
impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
impl<T: Config<I>, I: 'static> Hooks<SystemBlockNumberFor<T>> for Pallet<T, I> {
#[cfg(feature = "try-runtime")]
fn try_state(_n: BlockNumberFor<T>) -> Result<(), sp_runtime::TryRuntimeError> {
fn try_state(_n: SystemBlockNumberFor<T>) -> Result<(), sp_runtime::TryRuntimeError> {
Self::do_try_state()
}
}
Expand Down Expand Up @@ -928,7 +933,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// Get the block number used in the treasury pallet.
///
/// It may be configured to use the relay chain block number on a parachain.
pub fn treasury_block_number() -> BlockNumberFor<T> {
pub fn treasury_block_number() -> BlockNumberFor<T, I> {
<T as pallet_treasury::Config<I>>::BlockNumberProvider::current_block_number()
}

Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/child-bounties/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
use alloc::vec;
use frame_benchmarking::{v2::*, BenchmarkError};
use frame_support::ensure;
use frame_system::{pallet_prelude::BlockNumberFor, RawOrigin};
use frame_system::RawOrigin;
use pallet_bounties::Pallet as Bounties;
use pallet_treasury::Pallet as Treasury;
use sp_runtime::traits::BlockNumberProvider;
Expand Down
8 changes: 6 additions & 2 deletions substrate/frame/child-bounties/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ use sp_runtime::{
};

use frame_support::pallet_prelude::*;
use frame_system::pallet_prelude::*;
use frame_system::pallet_prelude::{
ensure_signed, BlockNumberFor as SystemBlockNumberFor, OriginFor,
};
use pallet_bounties::BountyStatus;
use scale_info::TypeInfo;
pub use weights::WeightInfo;
Expand All @@ -90,6 +92,8 @@ type BalanceOf<T> = pallet_treasury::BalanceOf<T>;
type BountiesError<T> = pallet_bounties::Error<T>;
type BountyIndex = pallet_bounties::BountyIndex;
type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;
type BlockNumberFor<T> =
<<T as pallet_treasury::Config>::BlockNumberProvider as BlockNumberProvider>::BlockNumber;

/// A child bounty proposal.
#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
Expand Down Expand Up @@ -810,7 +814,7 @@ pub mod pallet {
}

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
impl<T: Config> Hooks<SystemBlockNumberFor<T>> for Pallet<T> {
fn integrity_test() {
let parent_bounty_id: BountyIndex = 1;
let child_bounty_id: BountyIndex = 2;
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/treasury/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ mod benchmarks {
None,
);

let valid_from = frame_system::Pallet::<T>::block_number();
let valid_from = T::BlockNumberProvider::current_block_number();
let expire_at = valid_from.saturating_add(T::PayoutPeriod::get());
assert_last_event::<T, I>(
Event::AssetSpendApproved {
Expand Down
40 changes: 21 additions & 19 deletions substrate/frame/treasury/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ use frame_support::{
weights::Weight,
BoundedVec, PalletId,
};
use frame_system::pallet_prelude::BlockNumberFor;
use frame_system::pallet_prelude::BlockNumberFor as SystemBlockNumberFor;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to #6245, someday this type alias can be removed to avoid confusion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be local block number? What system really means here. Also BlockNumberFor reference to the block's header, not system pallet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be local block number? What system really means here. Also BlockNumberFor reference to the block's header, not system pallet

Since we are importing it from frame_system's prelude, I believe that SystemBlockNumberFor is reasonable.


pub use pallet::*;
pub use weights::WeightInfo;
Expand All @@ -122,6 +122,8 @@ pub type NegativeImbalanceOf<T, I = ()> = <<T as Config<I>>::Currency as Currenc
>>::NegativeImbalance;
type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;
type BeneficiaryLookupOf<T, I> = <<T as Config<I>>::BeneficiaryLookup as StaticLookup>::Source;
pub type BlockNumberFor<T, I = ()> =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather come up with new name, and not reuse the one which is used for local block number type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather come up with new name, and not reuse the one which is used for local block number type

I am actually doing it the other way around calling the local one as SystemBlockNumberFor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but this forces you to not import use frame_system::pallet_prelude::*;

I think having a different name that doesn't conflict with frame_system::BlockNumberFor would be a bit cleaner.

maybe like ProvidedBlockNumberFor? I agree the name is not good either.

anyway everything is good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but this forces you to not import use frame_system::pallet_prelude::*;

Yeah, my hope is to be able to provide a different type in the prelude once enough pallets have been migrated.

<<T as Config<I>>::BlockNumberProvider as BlockNumberProvider>::BlockNumber;

/// A trait to allow the Treasury Pallet to spend it's funds for other purposes.
/// There is an expectation that the implementer of this trait will correctly manage
Expand Down Expand Up @@ -202,7 +204,7 @@ pub mod pallet {
pallet_prelude::*,
traits::tokens::{ConversionFromAssetBalance, PaymentStatus},
};
use frame_system::pallet_prelude::*;
use frame_system::pallet_prelude::{ensure_signed, OriginFor};

#[pallet::pallet]
pub struct Pallet<T, I = ()>(PhantomData<(T, I)>);
Expand All @@ -221,7 +223,7 @@ pub mod pallet {

/// Period between successive spends.
#[pallet::constant]
type SpendPeriod: Get<BlockNumberFor<Self>>;
type SpendPeriod: Get<BlockNumberFor<Self, I>>;

/// Percentage of spare funds (if any) that are burnt per spend period.
#[pallet::constant]
Expand Down Expand Up @@ -277,14 +279,14 @@ pub mod pallet {

/// The period during which an approved treasury spend has to be claimed.
#[pallet::constant]
type PayoutPeriod: Get<BlockNumberFor<Self>>;
type PayoutPeriod: Get<BlockNumberFor<Self, I>>;

/// Helper type for benchmarks.
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkHelper: ArgumentsFactory<Self::AssetKind, Self::Beneficiary>;

/// Provider for the block number. Normally this is the `frame_system` pallet.
type BlockNumberProvider: BlockNumberProvider<BlockNumber = BlockNumberFor<Self>>;
type BlockNumberProvider: BlockNumberProvider;
}

/// DEPRECATED: associated with `spend_local` call and will be removed in May 2025.
Expand Down Expand Up @@ -335,15 +337,15 @@ pub mod pallet {
T::AssetKind,
AssetBalanceOf<T, I>,
T::Beneficiary,
BlockNumberFor<T>,
BlockNumberFor<T, I>,
<T::Paymaster as Pay>::Id,
>,
OptionQuery,
>;

/// The blocknumber for the last triggered spend period.
#[pallet::storage]
pub(crate) type LastSpendPeriod<T, I = ()> = StorageValue<_, BlockNumberFor<T>, OptionQuery>;
pub(crate) type LastSpendPeriod<T, I = ()> = StorageValue<_, BlockNumberFor<T, I>, OptionQuery>;

#[pallet::genesis_config]
#[derive(frame_support::DefaultNoBound)]
Expand Down Expand Up @@ -391,8 +393,8 @@ pub mod pallet {
asset_kind: T::AssetKind,
amount: AssetBalanceOf<T, I>,
beneficiary: T::Beneficiary,
valid_from: BlockNumberFor<T>,
expire_at: BlockNumberFor<T>,
valid_from: BlockNumberFor<T, I>,
expire_at: BlockNumberFor<T, I>,
},
/// An approved spend was voided.
AssetSpendVoided { index: SpendIndex },
Expand Down Expand Up @@ -434,10 +436,10 @@ pub mod pallet {
}

#[pallet::hooks]
impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
impl<T: Config<I>, I: 'static> Hooks<SystemBlockNumberFor<T>> for Pallet<T, I> {
/// ## Complexity
/// - `O(A)` where `A` is the number of approvals
fn on_initialize(_do_not_use_local_block_number: BlockNumberFor<T>) -> Weight {
fn on_initialize(_do_not_use_local_block_number: SystemBlockNumberFor<T>) -> Weight {
let block_number = T::BlockNumberProvider::current_block_number();
let pot = Self::pot();
let deactivated = Deactivated::<T, I>::get();
Expand All @@ -458,23 +460,23 @@ pub mod pallet {
// empty.
.unwrap_or_else(|| Self::update_last_spend_period());
let blocks_since_last_spend_period = block_number.saturating_sub(last_spend_period);
let safe_spend_period = T::SpendPeriod::get().max(BlockNumberFor::<T>::one());
let safe_spend_period = T::SpendPeriod::get().max(BlockNumberFor::<T, I>::one());

// Safe because of `max(1)` above.
let (spend_periods_passed, extra_blocks) = (
blocks_since_last_spend_period / safe_spend_period,
blocks_since_last_spend_period % safe_spend_period,
);
let new_last_spend_period = block_number.saturating_sub(extra_blocks);
if spend_periods_passed > BlockNumberFor::<T>::zero() {
if spend_periods_passed > BlockNumberFor::<T, I>::zero() {
Self::spend_funds(spend_periods_passed, new_last_spend_period)
} else {
Weight::zero()
}
}

#[cfg(feature = "try-runtime")]
fn try_state(_: BlockNumberFor<T>) -> Result<(), sp_runtime::TryRuntimeError> {
fn try_state(_: SystemBlockNumberFor<T>) -> Result<(), sp_runtime::TryRuntimeError> {
Self::do_try_state()?;
Ok(())
}
Expand Down Expand Up @@ -638,7 +640,7 @@ pub mod pallet {
asset_kind: Box<T::AssetKind>,
#[pallet::compact] amount: AssetBalanceOf<T, I>,
beneficiary: Box<BeneficiaryLookupOf<T, I>>,
valid_from: Option<BlockNumberFor<T>>,
valid_from: Option<BlockNumberFor<T, I>>,
) -> DispatchResult {
let max_amount = T::SpendOrigin::ensure_origin(origin)?;
let beneficiary = T::BeneficiaryLookup::lookup(*beneficiary)?;
Expand Down Expand Up @@ -844,9 +846,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
// Backfill the `LastSpendPeriod` storage, assuming that no configuration has changed
// since introducing this code. Used specifically for a migration-less switch to populate
// `LastSpendPeriod`.
fn update_last_spend_period() -> BlockNumberFor<T> {
fn update_last_spend_period() -> BlockNumberFor<T, I> {
let block_number = T::BlockNumberProvider::current_block_number();
let spend_period = T::SpendPeriod::get().max(BlockNumberFor::<T>::one());
let spend_period = T::SpendPeriod::get().max(BlockNumberFor::<T, I>::one());
let time_since_last_spend = block_number % spend_period;
// If it happens that this logic runs directly on a spend period block, we need to backdate
// to the last spend period so a spend still occurs this block.
Expand Down Expand Up @@ -889,8 +891,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {

/// Spend some money! returns number of approvals before spend.
pub fn spend_funds(
spend_periods_passed: BlockNumberFor<T>,
new_last_spend_period: BlockNumberFor<T>,
spend_periods_passed: BlockNumberFor<T, I>,
new_last_spend_period: BlockNumberFor<T, I>,
) -> Weight {
LastSpendPeriod::<T, I>::put(new_last_spend_period);
let mut total_weight = Weight::zero();
Expand Down
3 changes: 2 additions & 1 deletion substrate/primitives/runtime/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2349,7 +2349,8 @@ pub trait BlockNumberProvider {
+ TypeInfo
+ Debug
+ MaxEncodedLen
+ Copy;
+ Copy
+ EncodeLike;

/// Returns the current block number.
///
Expand Down
Loading