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.

Sign up for GitHub

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

Jump to bottom

Bounty Pallet: add approve_bounty_with_curator call to bounties pallet #5961

Merged
merged 15 commits into from
Nov 5, 2024
Merged

Bounty Pallet: add approve_bounty_with_curator call to bounties pallet #5961

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
83f249b
Add `approve_bounty_with_curator` call to `bounties` pallet
Oct 7, 2024
209aefb
Add dummy weights
Oct 8, 2024
a4aa9ea
Add prdoc
Oct 8, 2024
158c92a
prdoc fixes
Oct 8, 2024
f8397f3
Merge branch 'master' into davidk/batch-approve-bounty-with-curator
davidk-pt Oct 21, 2024
c1ea1bb
Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…
Oct 22, 2024
f09b5b1
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_bounties
Oct 22, 2024
727e8fb
".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_bounties
Oct 22, 2024
233576c
Add verify to benchmark and document the new call
Oct 24, 2024
904b7eb
Improve prdoc
Oct 24, 2024
adfd661
Change pallet-bounties semver
Oct 24, 2024
7573387
Remove redundant doc
Oct 25, 2024
bc3a06f
Merge branch 'master' into davidk/batch-approve-bounty-with-curator
davidk-pt Oct 25, 2024
b41a632
Merge branch 'master' into davidk/batch-approve-bounty-with-curator
davidk-pt Oct 28, 2024
7923a4e
Merge branch 'master' into davidk/batch-approve-bounty-with-curator
Ank4n Oct 29, 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
82 changes: 47 additions & 35 deletions polkadot/runtime/rococo/src/weights/pallet_bounties.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,25 +17,23 @@
//! Autogenerated weights for `pallet_bounties`
//!
//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 32.0.0
//! DATE: 2024-02-29, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
//! DATE: 2024-10-22, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]`
//! WORST CASE MAP SIZE: `1000000`
//! HOSTNAME: `runner-bn-ce5rx-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz`
//! HOSTNAME: `runner-augrssgt-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz`
//! WASM-EXECUTION: `Compiled`, CHAIN: `Some("rococo-dev")`, DB CACHE: 1024

// Executed Command:
// ./target/production/polkadot
// target/production/polkadot
// benchmark
// pallet
// --chain=rococo-dev
// --steps=50
// --repeat=20
// --no-storage-info
// --no-median-slopes
// --no-min-squares
// --pallet=pallet_bounties
// --extrinsic=*
// --execution=wasm
// --wasm-execution=compiled
// --heap-pages=4096
// --json-file=/builds/parity/mirrors/polkadot-sdk/.git/.artifacts/bench.json
// --pallet=pallet_bounties
// --chain=rococo-dev
// --header=./polkadot/file_header.txt
// --output=./polkadot/runtime/rococo/src/weights/

Expand Down Expand Up @@ -63,11 +61,11 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
// Proof Size summary in bytes:
// Measured: `210`
// Estimated: `3593`
// Minimum execution time: 21_772_000 picoseconds.
Weight::from_parts(22_861_341, 0)
// Minimum execution time: 26_614_000 picoseconds.
Weight::from_parts(28_274_660, 0)
.saturating_add(Weight::from_parts(0, 3593))
// Standard Error: 3
.saturating_add(Weight::from_parts(721, 0).saturating_mul(d.into()))
// Standard Error: 4
.saturating_add(Weight::from_parts(779, 0).saturating_mul(d.into()))
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(4))
}
Expand All @@ -79,8 +77,8 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
// Proof Size summary in bytes:
// Measured: `302`
// Estimated: `3642`
// Minimum execution time: 11_218_000 picoseconds.
Weight::from_parts(11_796_000, 0)
// Minimum execution time: 14_692_000 picoseconds.
Weight::from_parts(15_070_000, 0)
.saturating_add(Weight::from_parts(0, 3642))
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(2))
Expand All @@ -91,22 +89,36 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
// Proof Size summary in bytes:
// Measured: `322`
// Estimated: `3642`
// Minimum execution time: 10_959_000 picoseconds.
Weight::from_parts(11_658_000, 0)
// Minimum execution time: 13_695_000 picoseconds.
Weight::from_parts(14_220_000, 0)
.saturating_add(Weight::from_parts(0, 3642))
.saturating_add(T::DbWeight::get().reads(1))
.saturating_add(T::DbWeight::get().writes(1))
}
/// Storage: `Bounties::Bounties` (r:1 w:1)
/// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`)
/// Storage: `Bounties::BountyApprovals` (r:1 w:1)
/// Proof: `Bounties::BountyApprovals` (`max_values`: Some(1), `max_size`: Some(402), added: 897, mode: `MaxEncodedLen`)
fn approve_bounty_with_curator() -> Weight {
// Proof Size summary in bytes:
// Measured: `322`
// Estimated: `3642`
// Minimum execution time: 18_428_000 picoseconds.
Weight::from_parts(19_145_000, 0)
.saturating_add(Weight::from_parts(0, 3642))
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(2))
}
/// Storage: `Bounties::Bounties` (r:1 w:1)
/// Proof: `Bounties::Bounties` (`max_values`: None, `max_size`: Some(177), added: 2652, mode: `MaxEncodedLen`)
/// Storage: `System::Account` (r:1 w:1)
/// Proof: `System::Account` (`max_values`: None, `max_size`: Some(128), added: 2603, mode: `MaxEncodedLen`)
fn unassign_curator() -> Weight {
// Proof Size summary in bytes:
// Measured: `498`
// Estimated: `3642`
// Minimum execution time: 37_419_000 picoseconds.
Weight::from_parts(38_362_000, 0)
// Minimum execution time: 44_648_000 picoseconds.
Weight::from_parts(45_860_000, 0)
.saturating_add(Weight::from_parts(0, 3642))
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(2))
Expand All @@ -119,8 +131,8 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
// Proof Size summary in bytes:
// Measured: `494`
// Estimated: `3642`
// Minimum execution time: 27_328_000 picoseconds.
Weight::from_parts(27_661_000, 0)
// Minimum execution time: 33_973_000 picoseconds.
Weight::from_parts(34_979_000, 0)
.saturating_add(Weight::from_parts(0, 3642))
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(2))
Expand All @@ -133,8 +145,8 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
// Proof Size summary in bytes:
// Measured: `400`
// Estimated: `3642`
// Minimum execution time: 16_067_000 picoseconds.
Weight::from_parts(16_865_000, 0)
// Minimum execution time: 20_932_000 picoseconds.
Weight::from_parts(21_963_000, 0)
.saturating_add(Weight::from_parts(0, 3642))
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(1))
Expand All @@ -151,8 +163,8 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
// Proof Size summary in bytes:
// Measured: `764`
// Estimated: `8799`
// Minimum execution time: 101_153_000 picoseconds.
Weight::from_parts(102_480_000, 0)
// Minimum execution time: 114_942_000 picoseconds.
Weight::from_parts(117_653_000, 0)
.saturating_add(Weight::from_parts(0, 8799))
.saturating_add(T::DbWeight::get().reads(5))
.saturating_add(T::DbWeight::get().writes(6))
Expand All @@ -169,8 +181,8 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
// Proof Size summary in bytes:
// Measured: `444`
// Estimated: `3642`
// Minimum execution time: 38_838_000 picoseconds.
Weight::from_parts(39_549_000, 0)
// Minimum execution time: 47_649_000 picoseconds.
Weight::from_parts(49_016_000, 0)
.saturating_add(Weight::from_parts(0, 3642))
.saturating_add(T::DbWeight::get().reads(3))
.saturating_add(T::DbWeight::get().writes(3))
Expand All @@ -187,8 +199,8 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
// Proof Size summary in bytes:
// Measured: `680`
// Estimated: `6196`
// Minimum execution time: 68_592_000 picoseconds.
Weight::from_parts(70_727_000, 0)
// Minimum execution time: 80_298_000 picoseconds.
Weight::from_parts(82_306_000, 0)
.saturating_add(Weight::from_parts(0, 6196))
.saturating_add(T::DbWeight::get().reads(4))
.saturating_add(T::DbWeight::get().writes(4))
Expand All @@ -199,8 +211,8 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
// Proof Size summary in bytes:
// Measured: `358`
// Estimated: `3642`
// Minimum execution time: 11_272_000 picoseconds.
Weight::from_parts(11_592_000, 0)
// Minimum execution time: 14_237_000 picoseconds.
Weight::from_parts(14_969_000, 0)
.saturating_add(Weight::from_parts(0, 3642))
.saturating_add(T::DbWeight::get().reads(1))
.saturating_add(T::DbWeight::get().writes(1))
Expand All @@ -216,11 +228,11 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
// Proof Size summary in bytes:
// Measured: `0 + b * (297 ±0)`
// Estimated: `1887 + b * (5206 ±0)`
// Minimum execution time: 2_844_000 picoseconds.
Weight::from_parts(2_900_000, 0)
// Minimum execution time: 3_174_000 picoseconds.
Weight::from_parts(3_336_000, 0)
.saturating_add(Weight::from_parts(0, 1887))
// Standard Error: 9_467
.saturating_add(Weight::from_parts(32_326_595, 0).saturating_mul(b.into()))
// Standard Error: 10_408
.saturating_add(Weight::from_parts(37_811_366, 0).saturating_mul(b.into()))
.saturating_add(T::DbWeight::get().reads(1))
.saturating_add(T::DbWeight::get().reads((3_u64).saturating_mul(b.into())))
.saturating_add(T::DbWeight::get().writes(1))
Expand Down
15 changes: 15 additions & 0 deletions prdoc/pr_5961.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# 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: "Bounties Pallet: add `approve_bounty_with_curator` call"

doc:
- audience: [Runtime Dev, Runtime User]
description: |
Adds `approve_bounty_with_curator` call to the bounties pallet to combine `approve_bounty` and `propose_curator` into one call. If `unassign_curator` is called after `approve_bounty_with_curator` the process falls back to the previous flow of calling `propose_curator` separately.
davidk-pt marked this conversation as resolved.
Show resolved Hide resolved

crates:
- name: pallet-bounties
bump: minor
- name: rococo-runtime
bump: minor
10 changes: 10 additions & 0 deletions substrate/frame/bounties/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,16 @@ benchmarks_instance_pallet! {
Treasury::<T, I>::on_initialize(BlockNumberFor::<T>::zero());
}: _<T::RuntimeOrigin>(approve_origin, bounty_id, curator_lookup, fee)

approve_bounty_with_curator {
setup_pot_account::<T, I>();
let (caller, curator, fee, value, reason) = setup_bounty::<T, I>(0, T::MaximumReasonLength::get());
let curator_lookup = T::Lookup::unlookup(curator);
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());
}: _<T::RuntimeOrigin>(approve_origin, bounty_id, curator_lookup, fee)
davidk-pt marked this conversation as resolved.
Show resolved Hide resolved

// Worst case when curator is inactive and any sender unassigns the curator.
unassign_curator {
setup_pot_account::<T, I>();
Expand Down
71 changes: 69 additions & 2 deletions substrate/frame/bounties/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
//! - `claim_bounty` - Claim a specific bounty amount from the Payout Address.
//! - `unassign_curator` - Unassign an accepted curator from a specific earmark.
//! - `close_bounty` - Cancel the earmark for a specific treasury amount and close the bounty.
//! - `approve_bounty_with_curator` - Approve bounty and also propose curator for the bounty.

#![cfg_attr(not(feature = "std"), no_std)]

Expand Down Expand Up @@ -174,6 +175,11 @@ pub enum BountyStatus<AccountId, BlockNumber> {
/// When the bounty can be claimed.
unlock_at: BlockNumber,
},
/// The bounty is approved with curator assigned.
ApprovedWithCurator {
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need a new status? why cannot it be exactly the same behaviour as a batch of approve_bounty and propose_curator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@muharem Because there still needs some time to pass when bounty is funded because as I understand only at beginning of the block bounties are funded. There could be a time when bounty is approved but funds are not available for some time. So, if we were to jump to a Funded status we would be trying to fund the bounty right away taking from the treasury, and if we don't have funds we can't fund bounty and propose a curator yet. So we have status that bounty is ApprovedWithCurator to be able to approve the bounty without having funds yet.

Correct me if I'm wrong in this

Copy link
Contributor

Choose a reason for hiding this comment

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

you are right

Copy link
Contributor

@Ank4n Ank4n Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I’m not very familiar with the code, but having multiple statuses with similar meanings seems like a potential code smell.

How about modifying Approved with the field maybe_curator: Option<AccountId>? Probably we want to avoid any breaking change (which is totally understandable)? Though any indexer now needs to support the ApprovedWithCurator status as well, so I guess breaking change in this scenario is better.

I leave the judgement to you guys. Though it would be great to have some doc explaining expected lifecycle of a bounty (I couldn't find one).

Copy link
Contributor

@muharem muharem Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As a maintainer of this code, I wish to modify the existing status.
But my thought was - better non breaking change, all clients keep working. new feature available only to the clients adopting the new status.

@TarikGul @Tbaut @ERussel what you think?

Copy link
Member

Choose a reason for hiding this comment

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

But my thought was - better non breaking change, all clients keep working. new feature available only to the clients adopting the new status.

Generally speaking, breaking changes should be avoided if possible (when discussing downstream clients, and tools), and if they are necessary then they should be correctly communicated in a changelog - and/or runtime release docs.

It would be helpful to create a document (or perhaps even a talk, followed by a document) to give runtime developers a clearer understanding of existing client use cases, what constitutes a breaking change for a client (less and more painful)

Even though this would be a tall task, I totally agree. I would love to see more documentation on this front.

It could clarify what is generated dynamically from metadata and what isn’t

Atleast from the perspective of PJS - everything is dynamically generated from the metadata now. That being said the static typing (not to be confused with the dynamic generation at runtime) requires a few longer steps to get the types reflected in the PJS libraries - generate the static types - fix any required changes - release the api - then update all deps downstream.

If I am not mistaken the PAPI descriptors do a way better job at supplying the types (But I am no expert - yet :))

Copy link
Contributor

@kianenigma kianenigma Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we want to make this effort future-proof, and bring it up to date with #4722, we should be asking ourselves:

BountyStatus is the status enum that the internal logic of the pallet would need to care about. Is it even reasonable to expose this to the users?

For example, if we are to design a fn status_of(bounty) -> BountyStatus as a view function, what would the return type be?

Copy link

@josepot josepot Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How about modifying Approved with the field maybe_curator: Option<AccountId>?

I'm sorry that I'm late to the party. Modifying Approved to be an Option<AccountId> would have been a better way to go IMO.

The reason is simple: if a DApp has not been updated to take into account the new variant, then if they receive the variant "ApprovedWithCurator" the DApp will almost surely blow up, because they don't know how to deal with that variant.

On the other hand, modifying the Approved variant to support an optional value, may prevent some DApps from blowing up (sure, they may not take into account the value of the variant, but perhaps they don't need to). Also, at least with PAPI, it makes it much simpler for DApp developers to adapt to the change on their DApps.

Long story short: adding new variants on enums that are used as inputs is a good way to prevent breaking changes on DApps. However, adding new variants on enums that are used as outputs is a guaranteed way to trigger breaking changes on DApps.

New variants on read enums: bad.
New variants on write enums: good.

I'm sorry that I'm sharing this after the PR has been merged.

Copy link
Contributor

@muharem muharem Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This means the impact can be different for dapps and indexers. We need to learn more about it and document. It seems like we really can improve the status quo.

Copy link

Choose a reason for hiding this comment

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

I really fail to understand why this change is better for indexers.

/// The assigned curator of this bounty.
curator: AccountId,
},
}

/// The child bounty manager.
Expand Down Expand Up @@ -471,6 +477,15 @@ pub mod pallet {
// No curator to unassign at this point.
return Err(Error::<T, I>::UnexpectedStatus.into())
},
BountyStatus::ApprovedWithCurator { ref curator } => {
// Bounty not yet funded, but bounty was approved with curator.
// `RejectOrigin` or curator himself can unassign from this bounty.
ensure!(maybe_sender.map_or(true, |sender| sender == *curator), BadOrigin);
// This state can only be while the bounty is not yet funded so we return
// bounty to the `Approved` state without curator
bounty.status = BountyStatus::Approved;
return Ok(());
},
BountyStatus::CuratorProposed { ref curator } => {
// A curator has been proposed, but not accepted yet.
// Either `RejectOrigin` or the proposed curator can unassign the curator.
Expand Down Expand Up @@ -727,7 +742,7 @@ pub mod pallet {
Some(<T as Config<I>>::WeightInfo::close_bounty_proposed()).into()
)
},
BountyStatus::Approved => {
BountyStatus::Approved | BountyStatus::ApprovedWithCurator { .. } => {
// For weight reasons, we don't allow a council to cancel in this phase.
// We ask for them to wait until it is funded before they can cancel.
return Err(Error::<T, I>::UnexpectedStatus.into())
Expand Down Expand Up @@ -808,6 +823,52 @@ pub mod pallet {
Self::deposit_event(Event::<T, I>::BountyExtended { index: bounty_id });
Ok(())
}

/// Approve bountry and propose a curator simultaneously.
/// This call is a shortcut to calling `approve_bounty` and `propose_curator` separately.
Copy link
Contributor

Choose a reason for hiding this comment

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

This call seems like is an example of #5958.

Copy link
Contributor

Choose a reason for hiding this comment

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

But just looking at the diff, and knowing little about this pallet, I disagree with this statement.

If this call was purely an equivalent of batch(approve, propose), then:

  1. The implementation should be merely a approve().and(propose). Perhaps this would be less efficient, but it should be functionally the same.
  2. There would be no need to introduce a new BountyStatus variant.

So perhaps this PR is actually doing more than just introducing a shorthand for combining two existing atomic operations?

Copy link
Contributor Author

@davidk-pt davidk-pt Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The issue is ( #5961 (comment) ) that after bounty is approved some time must pass until bounty reaches Funded status because bounty fundings happen in the beginning of every block.

The only way for this to be atomic is if with approval we try to fund the bounty right away, from Funded then it could go to CuratorProposed state, but there may not be enough money in the treasury and such action will revert.

I'm open to simpler solutions which may keep the flow backwards compatible with previous version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining! The current solution is good, I was mainly interested in the relationship between this and #5958.

///
/// May only be called from `T::SpendOrigin`.
///
/// - `bounty_id`: Bounty ID to approve.
/// - `curator`: The curator account whom will manage this bounty.
/// - `fee`: The curator fee.
///
/// ## Complexity
/// - O(1).
#[pallet::call_index(9)]
#[pallet::weight(<T as Config<I>>::WeightInfo::approve_bounty_with_curator())]
pub fn approve_bounty_with_curator(
origin: OriginFor<T>,
#[pallet::compact] bounty_id: BountyIndex,
curator: AccountIdLookupOf<T>,
#[pallet::compact] fee: BalanceOf<T, I>,
) -> DispatchResult {
let max_amount = T::SpendOrigin::ensure_origin(origin)?;
let curator = T::Lookup::lookup(curator)?;
Bounties::<T, I>::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult {
// approve bounty
let bounty = maybe_bounty.as_mut().ok_or(Error::<T, I>::InvalidIndex)?;
ensure!(
bounty.value <= max_amount,
pallet_treasury::Error::<T, I>::InsufficientPermission
);
ensure!(bounty.status == BountyStatus::Proposed, Error::<T, I>::UnexpectedStatus);
ensure!(fee < bounty.value, Error::<T, I>::InvalidFee);

BountyApprovals::<T, I>::try_append(bounty_id)
.map_err(|()| Error::<T, I>::TooManyQueued)?;

bounty.status = BountyStatus::ApprovedWithCurator { curator: curator.clone() };
bounty.fee = fee;

Ok(())
})?;

Self::deposit_event(Event::<T, I>::BountyApproved { index: bounty_id });
Self::deposit_event(Event::<T, I>::CuratorProposed { bounty_id, curator });
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to emit both events? Is BountyApproved inferable from CuratorProposed?

Copy link
Contributor Author

@davidk-pt davidk-pt Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need to emit both, because this function does logically what approve_bounty and propose_curator does, and they emit these events separately.

Yes, BountyApproved could be inferred from CuratorProposed in the state machine, CuratorProposed can happen only after bounty is approved. But then I think people would need to start assuming implementation details of how this pallet works when indexing.

Copy link
Contributor Author

@davidk-pt davidk-pt Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I’m not very familiar with the code, but having multiple statuses with similar meanings seems like a potential code smell.

How about modifying Approved with the field maybe_curator: Option<AccountId>? Probably we want to avoid any breaking change (which is totally understandable)? Though any indexer now needs to support the ApprovedWithCurator status as well, so I guess breaking change in this scenario is better.

I leave the judgement to you guys. Though it would be great to have some doc explaining expected lifecycle of a bounty (I couldn't find one).

Wouldn't maybe_curator: Option<AccountId> break existing which used Approved enum? I think it would add at least 1 byte for the option of Approved whether it is None or Some, with these changes we leave the rest of the enums unmodified and people would only need to add support for the new one

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to emit both, because this function does logically what approve_bounty and propose_curator does, and they emit these events separately.

Only ask this since events are expensive and stored in state of each block. But of course if you have a good reason to emit both, go for it.

Wouldn't maybe_curator: Option break existing which used Approved enum?

Is breaking change always bad? Especially since old clients may have unexpected side effects, i.e. just miss the approved (ApprovedWithCurator) status altogether if they depend on the existing Approved status.

Also types are dynamically generated from substrate metadata and potentially for js clients this might not even be a breaking change.

P.S.: None of this is a huge deal tbh. I’m comfortable with either approach, as long as we’re mindful of the tradeoffs.


Ok(())
}
}

#[pallet::hooks]
Expand Down Expand Up @@ -942,7 +1003,13 @@ impl<T: Config<I>, I: 'static> pallet_treasury::SpendFunds<T, I> for Pallet<T, I
if bounty.value <= *budget_remaining {
*budget_remaining -= bounty.value;

bounty.status = BountyStatus::Funded;
// jump through the funded phase if we're already approved with curator
if let BountyStatus::ApprovedWithCurator { curator } = &bounty.status {
bounty.status =
BountyStatus::CuratorProposed { curator: curator.clone() };
} else {
bounty.status = BountyStatus::Funded;
}

// return their deposit.
let err_amount = T::Currency::unreserve(&bounty.proposer, bounty.bond);
Expand Down
Loading
Loading