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

Update Treasury to Support Relay Chain Block Number Provider #3970

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
d7678db
add block number provider
shawntabrizi Apr 3, 2024
a8b649d
logic for handling multiple spend periods passed
shawntabrizi Apr 3, 2024
757c7f4
update for new associated type
shawntabrizi Apr 3, 2024
0f74c0c
add and fix tests
shawntabrizi Apr 3, 2024
2b1bd39
Merge branch 'master' into shawntabrizi-treasury
shawntabrizi Apr 3, 2024
b0cc89d
fix bug!
shawntabrizi Apr 3, 2024
f01f0e6
introduce `update_last_spend_period`
shawntabrizi Apr 4, 2024
904dd96
remove test feature flag
shawntabrizi Apr 4, 2024
1add90c
Update substrate/frame/treasury/src/lib.rs
shawntabrizi Apr 10, 2024
7ee9a4b
Merge branch 'master' into shawntabrizi-treasury
shawntabrizi Apr 10, 2024
a45e506
Update mod.rs
shawntabrizi Apr 10, 2024
6a0becc
Merge branch 'master' into shawntabrizi-treasury
shawntabrizi Apr 11, 2024
ad5d05a
Merge branch 'master' into shawntabrizi-treasury
bkchr Apr 15, 2024
64ceef4
Create pr_3970.prdoc
shawntabrizi Apr 15, 2024
e3a06a8
Update substrate/frame/treasury/src/lib.rs
bkchr Apr 15, 2024
e61e0f2
Merge branch 'master' into shawntabrizi-treasury
shawntabrizi Apr 15, 2024
d0027d1
Update pr_3970.prdoc
shawntabrizi Apr 15, 2024
ea02217
Merge branch 'shawntabrizi-treasury' of https://github.com/shawntabri…
shawntabrizi Apr 15, 2024
874db2a
Update mod.rs
shawntabrizi Apr 16, 2024
e155ee6
fix bounties test
shawntabrizi Apr 16, 2024
53a1a2e
update child bounties tests for consistency
shawntabrizi Apr 16, 2024
b1f3407
Merge branch 'master' into shawntabrizi-treasury
shawntabrizi Apr 16, 2024
be1d7d0
Merge branch 'master' into shawntabrizi-treasury
bkchr Apr 20, 2024
aceda46
Merge remote-tracking branch 'upstream/master' into shawntabrizi-trea…
shawntabrizi Apr 28, 2024
e1f25b3
more fixes
shawntabrizi Apr 28, 2024
cc80522
fix `on_initialize(0)` which is invalid
shawntabrizi Apr 28, 2024
b383500
fix up child bounties for block number
shawntabrizi Apr 28, 2024
bf6b8d2
choose a more specific name
shawntabrizi Apr 28, 2024
ee8ce2b
Merge branch 'master' into shawntabrizi-treasury
shawntabrizi Apr 28, 2024
93418c5
Merge remote-tracking branch 'upstream/master' into shawntabrizi-trea…
shawntabrizi Apr 30, 2024
bac943d
Merge remote-tracking branch 'upstream/master' into shawntabrizi-trea…
shawntabrizi Apr 30, 2024
a4f2732
Merge branch 'master' into shawntabrizi-treasury
shawntabrizi May 3, 2024
554eac1
Merge branch 'master' into shawntabrizi-treasury
shawntabrizi May 6, 2024
2ef0ce3
Update prdoc/pr_3970.prdoc
shawntabrizi May 7, 2024
fa68d86
Merge branch 'master' into shawntabrizi-treasury
shawntabrizi May 13, 2024
b90bf4a
Merge branch 'master' into shawntabrizi-treasury
muharem Jun 24, 2024
471cb2b
Merge branch 'master' into shawntabrizi-treasury
shawntabrizi Jun 25, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -332,4 +332,5 @@ impl pallet_treasury::Config<FellowshipTreasuryInstance> for Runtime {
sp_core::ConstU8<1>,
ConstU32<1000>,
>;
type BlockNumberProvider = crate::System;
}
1 change: 1 addition & 0 deletions polkadot/runtime/common/src/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ mod tests {
type Paymaster = PayFromAccount<Balances, TreasuryAccount>;
type BalanceConverter = UnityAssetBalanceConversion;
type PayoutPeriod = ConstU64<0>;
type BlockNumberProvider = System;
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkHelper = ();
}
Expand Down
1 change: 1 addition & 0 deletions polkadot/runtime/rococo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,7 @@ impl pallet_treasury::Config for Runtime {
AssetRate,
>;
type PayoutPeriod = PayoutSpendPeriod;
type BlockNumberProvider = System;
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkHelper = polkadot_runtime_common::impls::benchmarks::TreasuryArguments;
}
Expand Down
1 change: 1 addition & 0 deletions polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,7 @@ impl pallet_treasury::Config for Runtime {
AssetRate,
>;
type PayoutPeriod = PayoutSpendPeriod;
type BlockNumberProvider = System;
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkHelper = polkadot_runtime_common::impls::benchmarks::TreasuryArguments;
}
Expand Down
17 changes: 17 additions & 0 deletions prdoc/pr_3970.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Update Treasury to Support Block Number Provider

doc:
- audience: Runtime Dev
description: |
The goal of this PR is to have the treasury pallet work on a parachain which does not produce blocks on a regular schedule, thus can use the relay chain as a block provider. Because blocks are not produced regularly, we cannot make the assumption that block number increases monotonically, and thus have new logic to handle multiple spend periods passing between blocks. To migrate existing treasury implementations, simply add `type BlockNumberProvider = System` to have the same behavior as before.

crates:
- name: pallet-treasury
bump: major
- name: pallet-bounties
bump: minor
- name: pallet-child-bounties
bump: minor
1 change: 1 addition & 0 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1247,6 +1247,7 @@ impl pallet_treasury::Config for Runtime {
type Paymaster = PayAssetFromAccount<Assets, TreasuryAccount>;
type BalanceConverter = AssetRate;
type PayoutPeriod = SpendPayoutPeriod;
type BlockNumberProvider = System;
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkHelper = ();
}
Expand Down
29 changes: 18 additions & 11 deletions substrate/frame/bounties/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,17 @@ use frame_benchmarking::v1::{
account, benchmarks_instance_pallet, whitelisted_caller, BenchmarkError,
};
use frame_system::{pallet_prelude::BlockNumberFor, RawOrigin};
use sp_runtime::traits::Bounded;
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>) {
<T as pallet_treasury::Config<I>>::BlockNumberProvider::set_block_number(n);
}

// Create bounties that are approved for use in `on_initialize`.
fn create_approved_bounties<T: Config<I>, I: 'static>(n: u32) -> Result<(), BenchmarkError> {
for i in 0..n {
Expand Down Expand Up @@ -77,7 +81,8 @@ fn create_bounty<T: Config<I>, I: 'static>(
let approve_origin =
T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Bounties::<T, I>::approve_bounty(approve_origin.clone(), bounty_id)?;
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
set_block_number::<T, I>(T::SpendPeriod::get());
Treasury::<T, I>::on_initialize(frame_system::Pallet::<T>::block_number());
Bounties::<T, I>::propose_curator(approve_origin, bounty_id, curator_lookup.clone(), fee)?;
Bounties::<T, I>::accept_curator(RawOrigin::Signed(curator).into(), bounty_id)?;
Ok((curator_lookup, bounty_id))
Expand Down Expand Up @@ -115,16 +120,17 @@ benchmarks_instance_pallet! {
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Bounties::<T, I>::approve_bounty(approve_origin.clone(), bounty_id)?;
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
set_block_number::<T, I>(T::SpendPeriod::get());
Treasury::<T, I>::on_initialize(frame_system::Pallet::<T>::block_number());
}: _<T::RuntimeOrigin>(approve_origin, bounty_id, curator_lookup, fee)

// Worst case when curator is inactive and any sender unassigns the curator.
unassign_curator {
setup_pot_account::<T, I>();
let (curator_lookup, bounty_id) = create_bounty::<T, I>()?;
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
Treasury::<T, I>::on_initialize(frame_system::Pallet::<T>::block_number());
let bounty_id = BountyCount::<T, I>::get() - 1;
frame_system::Pallet::<T>::set_block_number(T::BountyUpdatePeriod::get() + 2u32.into());
set_block_number::<T, I>(T::SpendPeriod::get() + T::BountyUpdatePeriod::get() + 2u32.into());
let caller = whitelisted_caller();
}: _(RawOrigin::Signed(caller), bounty_id)

Expand All @@ -136,14 +142,15 @@ benchmarks_instance_pallet! {
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Bounties::<T, I>::approve_bounty(approve_origin.clone(), bounty_id)?;
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
set_block_number::<T, I>(T::SpendPeriod::get());
Treasury::<T, I>::on_initialize(frame_system::Pallet::<T>::block_number());
Bounties::<T, I>::propose_curator(approve_origin, bounty_id, curator_lookup, fee)?;
}: _(RawOrigin::Signed(curator), bounty_id)

award_bounty {
setup_pot_account::<T, I>();
let (curator_lookup, bounty_id) = create_bounty::<T, I>()?;
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
Treasury::<T, I>::on_initialize(frame_system::Pallet::<T>::block_number());

let bounty_id = BountyCount::<T, I>::get() - 1;
let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?;
Expand All @@ -154,7 +161,7 @@ benchmarks_instance_pallet! {
claim_bounty {
setup_pot_account::<T, I>();
let (curator_lookup, bounty_id) = create_bounty::<T, I>()?;
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
Treasury::<T, I>::on_initialize(frame_system::Pallet::<T>::block_number());

let bounty_id = BountyCount::<T, I>::get() - 1;
let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?;
Expand All @@ -163,7 +170,7 @@ benchmarks_instance_pallet! {
let beneficiary = T::Lookup::unlookup(beneficiary_account.clone());
Bounties::<T, I>::award_bounty(RawOrigin::Signed(curator.clone()).into(), bounty_id, beneficiary)?;

frame_system::Pallet::<T>::set_block_number(T::BountyDepositPayoutDelay::get() + 1u32.into());
set_block_number::<T, I>(T::SpendPeriod::get() + T::BountyDepositPayoutDelay::get() + 1u32.into());
ensure!(T::Currency::free_balance(&beneficiary_account).is_zero(), "Beneficiary already has balance");

}: _(RawOrigin::Signed(curator), bounty_id)
Expand All @@ -183,7 +190,7 @@ benchmarks_instance_pallet! {
close_bounty_active {
setup_pot_account::<T, I>();
let (curator_lookup, bounty_id) = create_bounty::<T, I>()?;
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
Treasury::<T, I>::on_initialize(frame_system::Pallet::<T>::block_number());
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin =
T::RejectOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Expand All @@ -195,7 +202,7 @@ benchmarks_instance_pallet! {
extend_bounty_expiry {
setup_pot_account::<T, I>();
let (curator_lookup, bounty_id) = create_bounty::<T, I>()?;
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
Treasury::<T, I>::on_initialize(frame_system::Pallet::<T>::block_number());

let bounty_id = BountyCount::<T, I>::get() - 1;
let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?;
Expand Down
26 changes: 15 additions & 11 deletions substrate/frame/bounties/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ use frame_support::traits::{
};

use sp_runtime::{
traits::{AccountIdConversion, BadOrigin, Saturating, StaticLookup, Zero},
traits::{AccountIdConversion, BadOrigin, BlockNumberProvider, Saturating, StaticLookup, Zero},
DispatchResult, Permill, RuntimeDebug,
};

Expand Down Expand Up @@ -486,7 +486,7 @@ pub mod pallet {
// If the sender is not the curator, and the curator is inactive,
// slash the curator.
if sender != *curator {
let block_number = frame_system::Pallet::<T>::block_number();
let block_number = Self::treasury_block_number();
if *update_due < block_number {
slash_curator(curator, &mut bounty.curator_deposit);
// Continue to change bounty status below...
Expand Down Expand Up @@ -550,8 +550,8 @@ pub mod pallet {
T::Currency::reserve(curator, deposit)?;
bounty.curator_deposit = deposit;

let update_due = frame_system::Pallet::<T>::block_number() +
T::BountyUpdatePeriod::get();
let update_due =
Self::treasury_block_number() + T::BountyUpdatePeriod::get();
Copy link
Member

Choose a reason for hiding this comment

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

I imagine most of the logic breaks when a parachain migrates from System to the Relay BlockNumberProvider...
Not sure what can be done about it without re-writing the pallet to use timepoints. Maybe we can use this as first step and advise to still use System.

Copy link
Member Author

Choose a reason for hiding this comment

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

a transition from system to relay will always require a migration due to the nature of the code.

you think we should be transitioning to use timestamps for all of these various pallet logic? I have always suggested against this, but perhaps due to the nature of Polkadot, it might be the right thing to do.

Copy link
Member

Choose a reason for hiding this comment

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

I think in the long term we need some kind of block-number-abstraction like #3268, but its a bit stale now as low-prio. That is not necessarily timestamps, but could be implemented with Relay blocknumber or something else.

bounty.status =
BountyStatus::Active { curator: curator.clone(), update_due };

Expand Down Expand Up @@ -605,8 +605,7 @@ pub mod pallet {
bounty.status = BountyStatus::PendingPayout {
curator: signer,
beneficiary: beneficiary.clone(),
unlock_at: frame_system::Pallet::<T>::block_number() +
T::BountyDepositPayoutDelay::get(),
unlock_at: Self::treasury_block_number() + T::BountyDepositPayoutDelay::get(),
};

Ok(())
Expand Down Expand Up @@ -637,10 +636,7 @@ pub mod pallet {
if let BountyStatus::PendingPayout { curator, beneficiary, unlock_at } =
bounty.status
{
ensure!(
frame_system::Pallet::<T>::block_number() >= unlock_at,
Error::<T, I>::Premature
);
ensure!(Self::treasury_block_number() >= unlock_at, Error::<T, I>::Premature);
let bounty_account = Self::bounty_account_id(bounty_id);
let balance = T::Currency::free_balance(&bounty_account);
let fee = bounty.fee.min(balance); // just to be safe
Expand Down Expand Up @@ -793,7 +789,7 @@ pub mod pallet {
match bounty.status {
BountyStatus::Active { ref curator, ref mut update_due } => {
ensure!(*curator == signer, Error::<T, I>::RequireCurator);
*update_due = (frame_system::Pallet::<T>::block_number() +
*update_due = (Self::treasury_block_number() +
T::BountyUpdatePeriod::get())
.max(*update_due);
},
Expand All @@ -810,6 +806,14 @@ pub mod pallet {
}

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> {
<T as pallet_treasury::Config<I>>::BlockNumberProvider::current_block_number()
}

/// Calculate the deposit required for a curator.
pub fn calculate_curator_deposit(fee: &BalanceOf<T, I>) -> BalanceOf<T, I> {
let mut deposit = T::CuratorDepositMultiplier::get() * *fee;

Expand Down
Loading
Loading