Bounty Pallet: add `approve_bounty_with_curator` call to `bounties` pallet #5961

davidk-pt · 2024-10-08T06:20:53Z

Resolves issue #5928

Adds approve_bounty_with_curator call to the bounties pallet to combine functions of approve_bounty and propose_curator into one call. Also adds a new status ApprovedWithCurator required to distinguish if bounty was approved with curator when skipping through Funded status and moving to CuratorProposed status.

If unassign_curator is called after approve_bounty_with_curator the process will fall back to the old flow of calling propose_curator separately.

davidk-pt · 2024-10-08T12:31:07Z

bot bench polkadot-pallet --pallet=pallet_bounties --runtime=westend

command-bot · 2024-10-08T12:31:12Z

@davidk-pt https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7533738 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=pallet_bounties. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 26-8a484dfe-7917-4dc2-9bdb-e5882314ac4d to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-10-08T13:11:55Z

@davidk-pt Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=pallet_bounties has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7533738 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7533738/artifacts/download.

muharem · 2024-10-21T09:18:49Z

substrate/frame/bounties/src/lib.rs

@@ -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 {


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

@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

you are right

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).

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?

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 :))

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?

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.

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.

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

muharem · 2024-10-21T09:20:08Z

polkadot/runtime/rococo/src/weights/pallet_bounties.rs

+		//  Estimated: `0`
+		// Minimum execution time: 0_000 picoseconds.
+		Weight::from_parts(0, 0)
+			.saturating_add(Weight::from_parts(0, 0))


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

davidk-pt · 2024-10-21T09:32:53Z

bot help

command-bot · 2024-10-21T09:32:57Z

Here's a link to docs

davidk-pt · 2024-10-21T09:34:13Z

bot bench-all pallet --pallet=bounties

command-bot · 2024-10-21T09:34:18Z

@davidk-pt https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7607483 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=bounties. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-b44e26ce-db77-4284-86c7-f80e94c8885f to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-10-21T09:36:40Z

@davidk-pt Command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=bounties has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7607483 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7607483/artifacts/download.

davidk-pt · 2024-10-21T11:24:25Z

bot bench-all pallet --pallet=bounties

command-bot · 2024-10-21T11:24:31Z

@davidk-pt https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7608309 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=bounties. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 3-4191a828-183b-424a-ae30-9c30ac92ed0f to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-10-21T12:02:06Z

@davidk-pt Command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=bounties has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7608309 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7608309/artifacts/download.

davidk-pt · 2024-10-22T11:25:53Z

bot bench-all pallet --pallet=pallet-bounties

command-bot · 2024-10-22T11:25:55Z

@davidk-pt option '--pallet ' argument 'pallet-bounties' is invalid. argument pallet is not matching rule ^([a-z_]+)([:]{2}[a-z_]+)?$

davidk-pt · 2024-10-22T11:34:37Z

bot bench-all pallet --pallet=pallet_bounties

command-bot · 2024-10-22T11:34:43Z

@davidk-pt https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7617747 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=pallet_bounties. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 18-5827282b-326f-430a-bc6e-adb757b0e9dc to cancel this command or bot cancel to cancel all commands in this pull request.

…nto HEAD

command-bot · 2024-10-22T12:42:51Z

@davidk-pt Command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=pallet_bounties has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7617747 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7617747/artifacts/download.

muharem

Looks good. Two minor comments

prdoc/pr_5961.prdoc

substrate/frame/bounties/src/benchmarking.rs

muharem · 2024-10-23T10:23:17Z

substrate/frame/bounties/src/lib.rs

@@ -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 {


you are right

kianenigma · 2024-10-28T11:11:41Z

substrate/frame/bounties/src/lib.rs

@@ -808,6 +824,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.


This call seems like is an example of #5958.

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:

The implementation should be merely a approve().and(propose). Perhaps this would be less efficient, but it should be functionally the same.

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?

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.

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

Ank4n

Couple of questions but in general PR looks fine to me.

Ank4n · 2024-10-29T11:15:11Z

substrate/frame/bounties/src/lib.rs

@@ -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 {


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).

Ank4n · 2024-10-29T11:19:30Z

substrate/frame/bounties/src/lib.rs

+			})?;
+
+			Self::deposit_event(Event::<T, I>::BountyApproved { index: bounty_id });
+			Self::deposit_event(Event::<T, I>::CuratorProposed { bounty_id, curator });


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

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.

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

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.

* master: (129 commits) pallet-revive: Use `RUSTUP_TOOLCHAIN` if set (#6365) [eth-rpc] proxy /health (#6360) [Release|CI/CD] adjust release pipelines (#6366) Bump the known_good_semver group across 1 directory with 3 updates (#6339) Run check semver in MQ (#6287) [Deprecation] deprecate treasury `spend_local` call and related items (#6169) refactor and harden check_core_index (#6217) litep2p: Update litep2p to v0.8.0 (#6353) [pallet-staking] Additional check for virtual stakers (#5985) migrate pallet-remarks to v2 bench syntax (#6291) Remove leftover references of Wococo (#6361) snowbridge: allow account conversion for Ethereum accounts (#6221) authority-discovery: Populate DHT records with public listen addresses (#6298) Bounty Pallet: add `approve_bounty_with_curator` call to `bounties` pallet (#5961) Silent annoying log (#6351) [pallet-revive] rework balance transfers (#6187) `statement-distribution`: RFC103 implementation (#5883) Disable flaky tests reported in #6343 / #6345 (#6346) migrate pallet-recovery to benchmark V2 syntax (#6299) inclusion emulator: correctly handle UMP signals (#6178) ...

Add approve_bounty_with_curator call to bounties pallet

83f249b

davidk-pt added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 8, 2024

davidk-pt requested a review from muharem October 8, 2024 06:20

davidk-pt changed the title ~~Add approve_bounty_with_curator call to bounties pallet~~ Bounty Pallet: add approve_bounty_with_curator call to bounties pallet Oct 8, 2024

DavidK added 3 commits October 8, 2024 09:25

Add dummy weights

209aefb

Add prdoc

a4aa9ea

prdoc fixes

158c92a

davidk-pt marked this pull request as ready for review October 8, 2024 07:44

davidk-pt requested a review from a team as a code owner October 8, 2024 07:44

muharem reviewed Oct 21, 2024

View reviewed changes

Merge branch 'master' into davidk/batch-approve-bounty-with-curator

f8397f3

paritytech-review-bot bot requested a review from a team October 21, 2024 11:25

command-bot added 3 commits October 22, 2024 11:37

Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…

c1ea1bb

…nto HEAD

".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_bounties

f09b5b1

".git/.scripts/commands/bench-all/bench-all.sh" --pallet=pallet_bounties

727e8fb

muharem reviewed Oct 23, 2024

View reviewed changes

Add verify to benchmark and document the new call

233576c

paritytech-review-bot bot requested a review from a team October 24, 2024 07:30

Improve prdoc

904b7eb

davidk-pt requested a review from muharem October 24, 2024 07:39

Change pallet-bounties semver

adfd661

muharem approved these changes Oct 24, 2024

View reviewed changes

muharem requested a review from joepetrowski October 24, 2024 14:05

DavidK and others added 3 commits October 25, 2024 08:54

Remove redundant doc

7573387

Merge branch 'master' into davidk/batch-approve-bounty-with-curator

bc3a06f

Merge branch 'master' into davidk/batch-approve-bounty-with-curator

b41a632

kianenigma reviewed Oct 28, 2024

View reviewed changes

Ank4n approved these changes Oct 29, 2024

View reviewed changes

Merge branch 'master' into davidk/batch-approve-bounty-with-curator

7923a4e

muharem mentioned this pull request Nov 4, 2024

pallet-child-bounties index child bounty by parent bounty #6255

Merged

seadanda mentioned this pull request Nov 4, 2024

[pallet-broker] Add sale id in the event emitted when a coretime sale starts #6354

Open

davidk-pt added this pull request to the merge queue Nov 5, 2024

Merged via the queue into master with commit 2ae79be Nov 5, 2024
210 checks passed

davidk-pt deleted the davidk/batch-approve-bounty-with-curator branch November 5, 2024 08:22

Bounty Pallet: add approve_bounty_with_curator call to bounties pallet #5961

Bounty Pallet: add approve_bounty_with_curator call to bounties pallet #5961

Conversation

davidk-pt commented Oct 8, 2024 • edited Loading

davidk-pt commented Oct 8, 2024

command-bot bot commented Oct 8, 2024 • edited Loading

command-bot bot commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ank4n Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

muharem Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

josepot Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

muharem Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidk-pt commented Oct 21, 2024

command-bot bot commented Oct 21, 2024

davidk-pt commented Oct 21, 2024

command-bot bot commented Oct 21, 2024 • edited Loading

command-bot bot commented Oct 21, 2024

davidk-pt commented Oct 21, 2024

command-bot bot commented Oct 21, 2024 • edited Loading

command-bot bot commented Oct 21, 2024

davidk-pt commented Oct 22, 2024

command-bot bot commented Oct 22, 2024

davidk-pt commented Oct 22, 2024

command-bot bot commented Oct 22, 2024 • edited Loading

command-bot bot commented Oct 22, 2024

muharem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidk-pt Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ank4n left a comment

Choose a reason for hiding this comment

Ank4n Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidk-pt Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

davidk-pt Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bounty Pallet: add `approve_bounty_with_curator` call to `bounties` pallet #5961

Bounty Pallet: add `approve_bounty_with_curator` call to `bounties` pallet #5961

davidk-pt commented Oct 8, 2024 •

edited

Loading

command-bot bot commented Oct 8, 2024 •

edited

Loading

Ank4n Oct 29, 2024 •

edited

Loading

muharem Oct 29, 2024 •

edited

Loading

kianenigma Nov 4, 2024 •

edited

Loading

josepot Nov 5, 2024 •

edited

Loading

muharem Nov 5, 2024 •

edited

Loading

command-bot bot commented Oct 21, 2024 •

edited

Loading

command-bot bot commented Oct 21, 2024 •

edited

Loading

command-bot bot commented Oct 22, 2024 •

edited

Loading

davidk-pt Oct 28, 2024 •

edited

Loading

Ank4n Oct 29, 2024 •

edited

Loading

davidk-pt Oct 30, 2024 •

edited

Loading

davidk-pt Oct 30, 2024 •

edited

Loading