Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improve Bounties and Child Bounties Deposit Logic #11014

Merged
merged 20 commits into from Mar 25, 2022

Conversation

shawntabrizi
Copy link
Contributor

@shawntabrizi shawntabrizi commented Mar 11, 2022

It was suggested that the deposit logic for Bounties and Child Bounties was not flexible enough.

Specifically:

  • If a Bounty paid no fee, it would have no deposit requirements.
  • If a Child Bounty had a huge value, it could end up for a huge deposit for the end user.
  • If a Child Bounty curator was also the parent curator, they were being asked to put an additional deposit down.

To solve this, we refactored the logic around calculating the curator deposit into its own function, and put a little bit of simple logic in there to handle different situations.

For Bounties:

  • The curator deposit is always a multiplier of the curator fee.
  • The curator deposit has an optional upper and lower bound to ensure the value is never zero, and never too high.

For Child Bounties:

  • If the parent curator is the same as the child curator, then no deposit is taken. They already have a deposit on the main bounty.
  • Otherwise, the logic is exactly the same, and actually uses the bounties function directly to calculate the deposit.

The multiplier and limits are all configurable in the runtime.

New tests ensure each of these different scenarios are handled correctly.

polkadot companion: paritytech/polkadot#5183

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Mar 11, 2022
@shawntabrizi shawntabrizi added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Mar 11, 2022
@shawntabrizi shawntabrizi added this to In progress in Runtime via automation Mar 11, 2022
NukeManDan and others added 3 commits March 12, 2022 03:03
* use uniform notion of parent and child, no "master" or "general" entity
* README updated to match comments
This reverts commit 1ab729e.
@xlc
Copy link
Contributor

xlc commented Mar 14, 2022

I think this should be voluntary curator friendly that when curator is doing voluntary work, they shouldn't need to lock deposit from their own pocket?

Otherwise we simply require a minimal curator fee so that is ok to ask curator to lock deposit which will be compensated later.

@shawntabrizi
Copy link
Contributor Author

shawntabrizi commented Mar 14, 2022

Can you make a specific suggestion?

For example, the NoFee multiplier can be set to zero to support actually 0 fee from the curator.

@xlc
Copy link
Contributor

xlc commented Mar 14, 2022

I just don't find this useful. If we are not going to set NoFee to a non zero value, this code will be redundant and I don't think we want to set it to a non zero value.

What value are you going to pick anyway? The Polkadot Pioneers Price is 993286 DOT, so if you make it 0.1%, the deposit will be 993 DOT, which is still a big number for curator.

But for 10 DOT (the current minimal bounty value for Polkadot), 0.1% will be 0.01 DOT, which is less than ED.

@xlc
Copy link
Contributor

xlc commented Mar 14, 2022

What I want to see, is a maximum value for curator deposit. Similar to the treasury proposal deposit. That ensures the deposit won't be too large for some large curator payout. e.g. for the ORML security bounty, the curator fee will be 10% of the payout, which can be large if the payout is big.

@michalisFr
Copy link

I agree with @xlc that there should be a cap on the curator deposit of a couple hundred DOT. For cases when the fee is zero, the percentage I proposed for the deposit was 1%, which for a 20000 DOT bounty would be 200 DOT, which I think is reasonable to be the cap as well.

IMO the fee should be zero only when the curator is part of an organisation and they're doing it as part of their job. In those cases the organisation can front the deposit. Otherwise community members should get rewarded for their efforts.

But we do need a deposit. Otherwise a malicious curator could set a zero fee in order not to place a deposit and then take advantage of the fact the have nothing to lose.

@NukeManDan
Copy link
Contributor

NukeManDan commented Mar 19, 2022

Related: paritytech/polkadot-sdk#1141 #11053

@xlc
Copy link
Contributor

xlc commented Mar 23, 2022

The min/max bounds implementation looks good to me.

@shawntabrizi
Copy link
Contributor Author

Thanks for the feedback @xlc. Things should be up to date now, and pretty straightforward.

The only other thing I did here was remove the redundant configurations from child bounties, since I don't think they should really ever be different from the parent bounties itself.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Looks sane to me, I think we should wait for approval from @xlc as well.

Runtime automation moved this from Please Review to Needs Audit Mar 23, 2022
@shawntabrizi
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: Approval criteria was not satisfied in paritytech/polkadot#5183.

The following errors might have affected the outcome of this attempt:

Merge failed. Check out the criteria for merge. If you're not meeting the approval count, check if the approvers are team members of substrateteamleads or core-devs.

@shawntabrizi
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 26ea757 into master Mar 25, 2022
@paritytech-processbot paritytech-processbot bot deleted the shawntabrizi-bounty-deposit-update branch March 25, 2022 13:42
Runtime automation moved this from Needs Audit to Done Mar 25, 2022
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* basic idea

* make tests better

* update bounties pallet to also have similar logic

* new test verifies logic for bounty pallet

* add test for new child logic

* better name

* make `node` compile with bounties changes

* * formatting
* use uniform notion of parent and child, no "master" or "general" entity
* README updated to match comments

* Revert "* formatting"

This reverts commit 1ab729e.

* update bounties logic to use bounds

* fix child

* bounties test for max

* update tests

* check min bound

* update node

* remove stale comment

* Update frame/bounties/src/lib.rs

Co-authored-by: Dan Shields <nukemandan@protonmail.com>
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* basic idea

* make tests better

* update bounties pallet to also have similar logic

* new test verifies logic for bounty pallet

* add test for new child logic

* better name

* make `node` compile with bounties changes

* * formatting
* use uniform notion of parent and child, no "master" or "general" entity
* README updated to match comments

* Revert "* formatting"

This reverts commit 1ab729e.

* update bounties logic to use bounds

* fix child

* bounties test for max

* update tests

* check min bound

* update node

* remove stale comment

* Update frame/bounties/src/lib.rs

Co-authored-by: Dan Shields <nukemandan@protonmail.com>
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* basic idea

* make tests better

* update bounties pallet to also have similar logic

* new test verifies logic for bounty pallet

* add test for new child logic

* better name

* make `node` compile with bounties changes

* * formatting
* use uniform notion of parent and child, no "master" or "general" entity
* README updated to match comments

* Revert "* formatting"

This reverts commit 1ab729e.

* update bounties logic to use bounds

* fix child

* bounties test for max

* update tests

* check min bound

* update node

* remove stale comment

* Update frame/bounties/src/lib.rs

Co-authored-by: Dan Shields <nukemandan@protonmail.com>
@shawntabrizi shawntabrizi moved this from Done to Archive in Runtime May 18, 2022
@louismerlin louismerlin added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jun 27, 2022
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* basic idea

* make tests better

* update bounties pallet to also have similar logic

* new test verifies logic for bounty pallet

* add test for new child logic

* better name

* make `node` compile with bounties changes

* * formatting
* use uniform notion of parent and child, no "master" or "general" entity
* README updated to match comments

* Revert "* formatting"

This reverts commit 1ab729e.

* update bounties logic to use bounds

* fix child

* bounties test for max

* update tests

* check min bound

* update node

* remove stale comment

* Update frame/bounties/src/lib.rs

Co-authored-by: Dan Shields <nukemandan@protonmail.com>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* basic idea

* make tests better

* update bounties pallet to also have similar logic

* new test verifies logic for bounty pallet

* add test for new child logic

* better name

* make `node` compile with bounties changes

* * formatting
* use uniform notion of parent and child, no "master" or "general" entity
* README updated to match comments

* Revert "* formatting"

This reverts commit 1ab729e.

* update bounties logic to use bounds

* fix child

* bounties test for max

* update tests

* check min bound

* update node

* remove stale comment

* Update frame/bounties/src/lib.rs

Co-authored-by: Dan Shields <nukemandan@protonmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants