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

Bounty Pallet: add approve_bounty_with_curator call to bounties pallet #5961

Merged
merged 15 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
8 changes: 8 additions & 0 deletions polkadot/runtime/rococo/src/weights/pallet_bounties.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,14 @@ impl<T: frame_system::Config> pallet_bounties::WeightInfo for WeightInfo<T> {
Weight::from_parts(0, 0)
.saturating_add(Weight::from_parts(0, 0))
}
fn approve_bounty_with_curator() -> Weight {
// Proof Size summary in bytes:
// Measured: `0`
// Estimated: `0`
// Minimum execution time: 0_000 picoseconds.
Weight::from_parts(0, 0)
.saturating_add(Weight::from_parts(0, 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to run the benchmarks to have an actual weights here and test your benchmark code. you can do it here in PR with a bot. try to comment bot help

}
fn propose_curator() -> Weight {
// Proof Size summary in bytes:
// Measured: `0`
Expand Down
15 changes: 15 additions & 0 deletions prdoc/pr_5961.prdoc
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
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
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

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

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

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

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

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

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

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

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