Bounty Pallet: add approve_bounty_with_curator call to bounties pallet
#5961
There are no files selected for viewing
| 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. Introduces a new `ApprovedWithCurator` bounty status when bounty is approved with curator. | ||
|
|
||
| crates: | ||
| - name: pallet-bounties | ||
| bump: major | ||
| - name: rococo-runtime | ||
| bump: minor |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,12 +73,15 @@ | |
| //! - `approve_bounty` - Accept a specific treasury amount to be earmarked for a predefined body of | ||
| //! work. | ||
| //! - `propose_curator` - Assign an account to a bounty as candidate curator. | ||
| //! - `approve_bounty_with_curator` - Accept a specific treasury amount for a predefined body of | ||
| //! work with assigned candidate curator account. | ||
| //! - `accept_curator` - Accept a bounty assignment from the Council, setting a curator deposit. | ||
| //! - `extend_bounty_expiry` - Extend the expiry block number of the bounty and stay active. | ||
| //! - `award_bounty` - Close and pay out the specified amount for the completed work. | ||
| //! - `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)] | ||
|
|
||
|
|
@@ -174,6 +177,11 @@ pub enum BountyStatus<AccountId, BlockNumber> { | |
| /// When the bounty can be claimed. | ||
| unlock_at: BlockNumber, | ||
| }, | ||
| /// The bounty is approved with curator assigned. | ||
| ApprovedWithCurator { | ||
|
Contributor
There was a problem hiding this comment. why we need a new status? why cannot it be exactly the same behaviour as a batch of
Contributor
Author
There was a problem hiding this comment. @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
Contributor
There was a problem hiding this comment. I’m not very familiar with the code, but having multiple statuses with similar meanings seems like a potential code smell. How about modifying 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).
Contributor
Member
There was a problem hiding this comment.
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.
Even though this would be a tall task, I totally agree. I would love to see more documentation on this front.
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 :))
Contributor
There was a problem hiding this comment. If we want to make this effort future-proof, and bring it up to date with #4722, we should be asking ourselves:
For example, if we are to design a
Contributor
There was a problem hiding this comment.
I'm sorry that I'm late to the party. Modifying The reason is simple: if a DApp has not been updated to take into account the new variant, then if they receive the variant " On the other hand, modifying the 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. I'm sorry that I'm sharing this after the PR has been merged.
Contributor
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. I really fail to understand why this change is better for indexers. |
||
| /// The assigned curator of this bounty. | ||
| curator: AccountId, | ||
| }, | ||
| } | ||
|
|
||
| /// The child bounty manager. | ||
|
|
@@ -471,6 +479,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. | ||
|
|
@@ -727,7 +744,7 @@ pub mod pallet { | |
| Some(<T as Config<I>>::WeightInfo::close_bounty_proposed()).into() | ||
| ) | ||
| }, | ||
| 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()) | ||
|
|
@@ -808,6 +825,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. | ||
|
Contributor
There was a problem hiding this comment. 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
So perhaps this PR is actually doing more than just introducing a shorthand for combining two existing atomic operations?
Contributor
Author
There was a problem hiding this comment. The issue is ( #5961 (comment) ) that after bounty is approved some time must pass until bounty reaches The only way for this to be atomic is if with approval we try to fund the bounty right away, from I'm open to simpler solutions which may keep the flow backwards compatible with previous version.
Contributor
There was a problem hiding this comment. 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 }); | ||
|
Contributor
There was a problem hiding this comment. Do we need to emit both events? Is
Contributor
Author
There was a problem hiding this comment. I think we need to emit both, because this function does logically what Yes,
Contributor
Author
There was a problem hiding this comment. Wouldn't
Contributor
There was a problem hiding this comment.
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.
Is breaking change always bad? Especially since old clients may have unexpected side effects, i.e. just miss the approved ( 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] | ||
|
|
@@ -942,7 +1005,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; | ||
|
|
||
| // 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); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.