Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
52698ec
pallet-bounties: extends update_due infinitly if BountiesUpdatePeriod…
BigTava Feb 26, 2025
fa19b53
pallet-bounties: fmt
BigTava Feb 26, 2025
4e49830
pallet-bounties: changes docs for config parameter and adds prdoc
BigTava Feb 26, 2025
08c637d
pallet-bounties: fix benchmarking
BigTava Feb 26, 2025
2478bfc
Merge branch 'master' into tiago-remove-bounties-expiry
BigTava Feb 26, 2025
c8cb79e
pallet-child-bounties: fix benchamarking
BigTava Feb 27, 2025
4654163
pallet-child-bounties: move Bounded from lib to benchmarking
BigTava Feb 27, 2025
2e22a58
pallet-bounties: improves prdoc and parameter docs
BigTava Feb 27, 2025
3890ab0
pallet-bounties: refactors prdoc and tests BountyUpdatePeriod set to …
BigTava Feb 27, 2025
91eb76d
docs: provides migration path
BigTava Feb 27, 2025
2ef5617
pallet-bounties: remove debug statements
BigTava Feb 28, 2025
1ee5b56
docs: improves migration path and adds function to reuse update_due c…
BigTava Feb 28, 2025
a927617
pallet-bounties: fmt
BigTava Feb 28, 2025
cda4673
pallet-bounties: adds docs to calculate_update_due
BigTava Feb 28, 2025
5c18edd
docs: adds API change in migration
BigTava Feb 28, 2025
e0600e3
docs: adds sentence on how to migrate to new change and removes migra…
BigTava Mar 4, 2025
133ca5b
pallet-bounties: improves code readability, docs, and add dynamic par…
BigTava Mar 4, 2025
6e42b1e
pallet-child-bounties: improves code readability
BigTava Mar 4, 2025
5ce7249
docs: refactor to describe only the use of `saturating_add` to preven…
BigTava Mar 7, 2025
c34e68e
pallet-bounties: refactor to only use `saturating_add` instead of cha…
BigTava Mar 7, 2025
a143787
pallet-child-bounties: refactors since BountyUpdatePeriod is no longe…
BigTava Mar 7, 2025
7a9f641
pallet-bounties: handles benchmarking call fail when `BountyUpdatePe…
BigTava Mar 8, 2025
e254cec
docs: refactor prdoc
BigTava Mar 17, 2025
1f6b1bb
pallet-bounties: refactor unassign_curator benchmark to always perfor…
BigTava Mar 17, 2025
3bde188
pallet-bounties: defaults to some signed caller if RejectOrigin is un…
BigTava Mar 17, 2025
e754423
pallet-bounties: defaults to curator if RejectOrigin is unavailable
BigTava Mar 17, 2025
0b8c2a5
pallet-child-bounties: minor
BigTava Mar 17, 2025
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
7 changes: 7 additions & 0 deletions prdoc/pr_7723.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
title: '[pallet-bounties] Make BountyUpdatePeriod optional'
doc:
- audience: Runtime Dev
description: Refactored `BountyUpdatePeriod` to be optional and adjusted expiration handling to default to `BlockNumber::MAX` when `None`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this will be included in the release notes. I would explain what this means. this is more like a description of the implementation, it can be read from the code.

also please add PR description

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thanks!

crates:
- name: pallet-bounties
bump: patch
2 changes: 1 addition & 1 deletion substrate/frame/bounties/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ benchmarks_instance_pallet! {
let (curator_lookup, bounty_id) = create_bounty::<T, I>()?;
Treasury::<T, I>::on_initialize(frame_system::Pallet::<T>::block_number());
let bounty_id = BountyCount::<T, I>::get() - 1;
set_block_number::<T, I>(T::SpendPeriod::get() + T::BountyUpdatePeriod::get() + 2u32.into());
set_block_number::<T, I>(T::SpendPeriod::get() + T::BountyUpdatePeriod::get().unwrap_or(BlockNumberFor::<T, I>::max_value()) + 2u32.into());
let caller = whitelisted_caller();
}: _(RawOrigin::Signed(caller), bounty_id)

Expand Down
20 changes: 13 additions & 7 deletions substrate/frame/bounties/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,10 @@ use frame_support::traits::{
};

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

Expand Down Expand Up @@ -221,9 +224,9 @@ pub mod pallet {
#[pallet::constant]
type BountyDepositPayoutDelay: Get<BlockNumberFor<Self, I>>;

/// Bounty duration in blocks.
/// Optional bounty duration in blocks. If `None`, it is considered `BlockNumber::MAX`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it was a poor doc for this parameter, lets give more context. explain behaviour.
".. it is considered BlockNumber::MAX" is description of the implementation. what it means for a user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thanks!

#[pallet::constant]
type BountyUpdatePeriod: Get<BlockNumberFor<Self, I>>;
type BountyUpdatePeriod: Get<Option<BlockNumberFor<Self, I>>>;
Copy link
Copy Markdown
Contributor

@Ank4n Ank4n Mar 4, 2025

Choose a reason for hiding this comment

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

DQ: Do we really require this runtime change? It seems equivalent to configuring the BountyUpdatePeriod in runtime with u32::MAX.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. This PR explicitly signals to runtime developers that bounties can safely be configured to never expire, making it a deliberate choice rather than an implicit behavior. Additionally, as-is u32::MAX could lead to overflow, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahh just have seen this comment by @Ank4n and I see it the same way. We only need the saturating_add change.

Additionally, as-is u32::MAX could lead to overflow, right?

If you use saturating_add, this will now overflow.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR explicitly signals to runtime developers that bounties can safely be configured to never expire, making it a deliberate choice rather than an implicit behavior

We achieve exactly the same as using Option. By setting it to max_value is also a deliberate choice :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I managed to draw the attention of 4 reviewers, including Basti, to what is going to be a one-liner fix 😅. I apologize! It was a great learning experience for me and thanks everyone for your feedback. Refactored to only use saturating_add instead of changing BountyUpdatePeriod to an optional parameter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's please merge this big boy! 🤞


/// The curator deposit is calculated as a percentage of the curator fee.
///
Expand Down Expand Up @@ -578,8 +581,9 @@ pub mod pallet {
T::Currency::reserve(curator, deposit)?;
bounty.curator_deposit = deposit;

let update_due =
Self::treasury_block_number() + T::BountyUpdatePeriod::get();
let update_due = Self::treasury_block_number() +
T::BountyUpdatePeriod::get()
.unwrap_or(BlockNumberFor::<T, I>::max_value());
bounty.status =
BountyStatus::Active { curator: curator.clone(), update_due };

Expand Down Expand Up @@ -820,8 +824,10 @@ pub mod pallet {
match bounty.status {
BountyStatus::Active { ref curator, ref mut update_due } => {
ensure!(*curator == signer, Error::<T, I>::RequireCurator);
*update_due = (Self::treasury_block_number() +
T::BountyUpdatePeriod::get())
*update_due = (Self::treasury_block_number().saturating_add(
T::BountyUpdatePeriod::get()
.unwrap_or(BlockNumberFor::<T, I>::max_value()),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this part repeated few time, you could abstract it into function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion.

))
.max(*update_due);
},
_ => return Err(Error::<T, I>::UnexpectedStatus.into()),
Expand Down
8 changes: 6 additions & 2 deletions substrate/frame/child-bounties/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use frame_support::ensure;
use frame_system::RawOrigin;
use pallet_bounties::Pallet as Bounties;
use pallet_treasury::Pallet as Treasury;
use sp_runtime::traits::BlockNumberProvider;
use sp_runtime::traits::{BlockNumberProvider, Bounded};

use crate::*;

Expand Down Expand Up @@ -273,7 +273,11 @@ mod benchmarks {
setup_pot_account::<T>();
let bounty_setup = activate_child_bounty::<T>(0, T::MaximumReasonLength::get())?;
Treasury::<T>::on_initialize(frame_system::Pallet::<T>::block_number());
set_block_number::<T>(T::SpendPeriod::get() + T::BountyUpdatePeriod::get() + 1u32.into());
set_block_number::<T>(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here we need to do something similar.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pallet-child-bounties uses benchmarking::v2. I used #[block] similar to treasury pallet benchmarking.

T::SpendPeriod::get() +
T::BountyUpdatePeriod::get().unwrap_or(BlockNumberFor::<T>::max_value()) +
1u32.into(),
);
let caller = whitelisted_caller();

#[extrinsic_call]
Expand Down
Loading