Improve Bounties and Child Bounties Deposit Logic (paritytech#11014)

* 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>

bin/node/runtime/src/lib.rs

@@ -220,7 +220,7 @@ impl frame_system::Config for Runtime {
 	type SystemWeightInfo = frame_system::weights::SubstrateWeight<Runtime>;
 	type SS58Prefix = ConstU16<42>;
 	type OnSetCode = ();
-	type MaxConsumers = frame_support::traits::ConstU32<16>;
+	type MaxConsumers = ConstU32<16>;
 }
 
 impl pallet_randomness_collective_flip::Config for Runtime {}
@@ -827,7 +827,7 @@ impl pallet_democracy::Config for Runtime {
 	type Slash = Treasury;
 	type Scheduler = Scheduler;
 	type PalletsOrigin = OriginCaller;
-	type MaxVotes = frame_support::traits::ConstU32<100>;
+	type MaxVotes = ConstU32<100>;
 	type WeightInfo = pallet_democracy::weights::SubstrateWeight<Runtime>;
 	type MaxProposals = MaxProposals;
 }
@@ -929,17 +929,9 @@ parameter_types! {
 	pub const TipFindersFee: Percent = Percent::from_percent(20);
 	pub const TipReportDepositBase: Balance = 1 * DOLLARS;
 	pub const DataDepositPerByte: Balance = 1 * CENTS;
-	pub const BountyDepositBase: Balance = 1 * DOLLARS;
-	pub const BountyDepositPayoutDelay: BlockNumber = 1 * DAYS;
 	pub const TreasuryPalletId: PalletId = PalletId(*b"py/trsry");
-	pub const BountyUpdatePeriod: BlockNumber = 14 * DAYS;
 	pub const MaximumReasonLength: u32 = 300;
-	pub const BountyCuratorDeposit: Permill = Permill::from_percent(50);
-	pub const BountyValueMinimum: Balance = 5 * DOLLARS;
 	pub const MaxApprovals: u32 = 100;
-	pub const MaxActiveChildBountyCount: u32 = 5;
-	pub const ChildBountyValueMinimum: Balance = 1 * DOLLARS;
-	pub const ChildBountyCuratorDepositBase: Permill = Permill::from_percent(10);
 }
 
 impl pallet_treasury::Config for Runtime {
@@ -966,24 +958,40 @@ impl pallet_treasury::Config for Runtime {
 	type MaxApprovals = MaxApprovals;
 }
 
+parameter_types! {
+	pub const BountyCuratorDeposit: Permill = Permill::from_percent(50);
+	pub const BountyValueMinimum: Balance = 5 * DOLLARS;
+	pub const BountyDepositBase: Balance = 1 * DOLLARS;
+	pub const CuratorDepositMultiplier: Permill = Permill::from_percent(50);
+	pub const CuratorDepositMin: Balance = 1 * DOLLARS;
+	pub const CuratorDepositMax: Balance = 100 * DOLLARS;
+	pub const BountyDepositPayoutDelay: BlockNumber = 1 * DAYS;
+	pub const BountyUpdatePeriod: BlockNumber = 14 * DAYS;
+}
+
 impl pallet_bounties::Config for Runtime {
 	type Event = Event;
 	type BountyDepositBase = BountyDepositBase;
 	type BountyDepositPayoutDelay = BountyDepositPayoutDelay;
 	type BountyUpdatePeriod = BountyUpdatePeriod;
-	type BountyCuratorDeposit = BountyCuratorDeposit;
+	type CuratorDepositMultiplier = CuratorDepositMultiplier;
+	type CuratorDepositMin = CuratorDepositMin;
+	type CuratorDepositMax = CuratorDepositMax;
 	type BountyValueMinimum = BountyValueMinimum;
 	type DataDepositPerByte = DataDepositPerByte;
 	type MaximumReasonLength = MaximumReasonLength;
 	type WeightInfo = pallet_bounties::weights::SubstrateWeight<Runtime>;
 	type ChildBountyManager = ChildBounties;
 }
 
+parameter_types! {
+	pub const ChildBountyValueMinimum: Balance = 1 * DOLLARS;
+}
+
 impl pallet_child_bounties::Config for Runtime {
 	type Event = Event;
-	type MaxActiveChildBountyCount = MaxActiveChildBountyCount;
+	type MaxActiveChildBountyCount = ConstU32<5>;
 	type ChildBountyValueMinimum = ChildBountyValueMinimum;
-	type ChildBountyCuratorDepositBase = ChildBountyCuratorDepositBase;
 	type WeightInfo = pallet_child_bounties::weights::SubstrateWeight<Runtime>;
 }
 

frame/bounties/src/lib.rs

@@ -196,10 +196,20 @@ pub mod pallet {
 		#[pallet::constant]
 		type BountyUpdatePeriod: Get<Self::BlockNumber>;
 
-		/// Percentage of the curator fee that will be reserved upfront as deposit for bounty
-		/// curator.
+		/// The curator deposit is calculated as a percentage of the curator fee.
+		///
+		/// This deposit has optional upper and lower bounds with `CuratorDepositMax` and
+		/// `CuratorDepositMin`.
+		#[pallet::constant]
+		type CuratorDepositMultiplier: Get<Permill>;
+
+		/// Maximum amount of funds that should be placed in a deposit for making a proposal.
 		#[pallet::constant]
-		type BountyCuratorDeposit: Get<Permill>;
+		type CuratorDepositMax: Get<Option<BalanceOf<Self>>>;
+
+		/// Minimum amount of funds that should be placed in a deposit for making a proposal.
+		#[pallet::constant]
+		type CuratorDepositMin: Get<Option<BalanceOf<Self>>>;
 
 		/// Minimum value for a bounty.
 		#[pallet::constant]
@@ -502,7 +512,7 @@ pub mod pallet {
 					BountyStatus::CuratorProposed { ref curator } => {
 						ensure!(signer == *curator, Error::<T>::RequireCurator);
 
-						let deposit = T::BountyCuratorDeposit::get() * bounty.fee;
+						let deposit = Self::calculate_curator_deposit(&bounty.fee);
 						T::Currency::reserve(curator, deposit)?;
 						bounty.curator_deposit = deposit;
 
@@ -762,7 +772,19 @@ pub mod pallet {
 }
 
 impl<T: Config> Pallet<T> {
-	// Add public immutables and private mutables.
+	pub fn calculate_curator_deposit(fee: &BalanceOf<T>) -> BalanceOf<T> {
+		let mut deposit = T::CuratorDepositMultiplier::get() * *fee;
+
+		if let Some(max_deposit) = T::CuratorDepositMax::get() {
+			deposit = deposit.min(max_deposit)
+		}
+
+		if let Some(min_deposit) = T::CuratorDepositMin::get() {
+			deposit = deposit.max(min_deposit)
+		}
+
+		deposit
+	}
 
 	/// The account ID of the treasury pot.
 	///

frame/bounties/src/tests.rs

@@ -60,6 +60,8 @@ parameter_types! {
 	pub const AvailableBlockRatio: Perbill = Perbill::one();
 }
 
+type Balance = u64;
+
 impl frame_system::Config for Test {
 	type BaseCallFilter = frame_support::traits::Everything;
 	type BlockWeights = ();
@@ -91,7 +93,7 @@ impl pallet_balances::Config for Test {
 	type MaxLocks = ();
 	type MaxReserves = ();
 	type ReserveIdentifier = [u8; 8];
-	type Balance = u64;
+	type Balance = Balance;
 	type Event = Event;
 	type DustRemoval = ();
 	type ExistentialDeposit = ConstU64<1>;
@@ -125,15 +127,23 @@ impl pallet_treasury::Config for Test {
 	type SpendFunds = Bounties;
 	type MaxApprovals = ConstU32<100>;
 }
+
 parameter_types! {
-	pub const BountyCuratorDeposit: Permill = Permill::from_percent(50);
+	// This will be 50% of the bounty fee.
+	pub const CuratorDepositMultiplier: Permill = Permill::from_percent(50);
+	pub const CuratorDepositMax: Balance = 1_000;
+	pub const CuratorDepositMin: Balance = 3;
+
 }
+
 impl Config for Test {
 	type Event = Event;
 	type BountyDepositBase = ConstU64<80>;
 	type BountyDepositPayoutDelay = ConstU64<3>;
 	type BountyUpdatePeriod = ConstU64<20>;
-	type BountyCuratorDeposit = BountyCuratorDeposit;
+	type CuratorDepositMultiplier = CuratorDepositMultiplier;
+	type CuratorDepositMax = CuratorDepositMax;
+	type CuratorDepositMin = CuratorDepositMin;
 	type BountyValueMinimum = ConstU64<1>;
 	type DataDepositPerByte = ConstU64<1>;
 	type MaximumReasonLength = ConstU32<16384>;
@@ -543,13 +553,14 @@ fn assign_curator_works() {
 			Error::<Test>::InvalidFee
 		);
 
-		assert_ok!(Bounties::propose_curator(Origin::root(), 0, 4, 4));
+		let fee = 4;
+		assert_ok!(Bounties::propose_curator(Origin::root(), 0, 4, fee));
 
 		assert_eq!(
 			Bounties::bounties(0).unwrap(),
 			Bounty {
 				proposer: 0,
-				fee: 4,
+				fee,
 				curator_deposit: 0,
 				value: 50,
 				bond: 85,
@@ -567,20 +578,22 @@ fn assign_curator_works() {
 
 		assert_ok!(Bounties::accept_curator(Origin::signed(4), 0));
 
+		let expected_deposit = Bounties::calculate_curator_deposit(&fee);
+
 		assert_eq!(
 			Bounties::bounties(0).unwrap(),
 			Bounty {
 				proposer: 0,
-				fee: 4,
-				curator_deposit: 2,
+				fee,
+				curator_deposit: expected_deposit,
 				value: 50,
 				bond: 85,
 				status: BountyStatus::Active { curator: 4, update_due: 22 },
 			}
 		);
 
-		assert_eq!(Balances::free_balance(&4), 8);
-		assert_eq!(Balances::reserved_balance(&4), 2);
+		assert_eq!(Balances::free_balance(&4), 10 - expected_deposit);
+		assert_eq!(Balances::reserved_balance(&4), expected_deposit);
 	});
 }
 
@@ -596,46 +609,44 @@ fn unassign_curator_works() {
 		System::set_block_number(2);
 		<Treasury as OnInitialize<u64>>::on_initialize(2);
 
-		assert_ok!(Bounties::propose_curator(Origin::root(), 0, 4, 4));
+		let fee = 4;
 
+		assert_ok!(Bounties::propose_curator(Origin::root(), 0, 4, fee));
 		assert_noop!(Bounties::unassign_curator(Origin::signed(1), 0), BadOrigin);
-
 		assert_ok!(Bounties::unassign_curator(Origin::signed(4), 0));
 
 		assert_eq!(
 			Bounties::bounties(0).unwrap(),
 			Bounty {
 				proposer: 0,
-				fee: 4,
+				fee,
 				curator_deposit: 0,
 				value: 50,
 				bond: 85,
 				status: BountyStatus::Funded,
 			}
 		);
 
-		assert_ok!(Bounties::propose_curator(Origin::root(), 0, 4, 4));
-
+		assert_ok!(Bounties::propose_curator(Origin::root(), 0, 4, fee));
 		Balances::make_free_balance_be(&4, 10);
-
 		assert_ok!(Bounties::accept_curator(Origin::signed(4), 0));
-
+		let expected_deposit = Bounties::calculate_curator_deposit(&fee);
 		assert_ok!(Bounties::unassign_curator(Origin::root(), 0));
 
 		assert_eq!(
 			Bounties::bounties(0).unwrap(),
 			Bounty {
 				proposer: 0,
-				fee: 4,
+				fee,
 				curator_deposit: 0,
 				value: 50,
 				bond: 85,
 				status: BountyStatus::Funded,
 			}
 		);
 
-		assert_eq!(Balances::free_balance(&4), 8);
-		assert_eq!(Balances::reserved_balance(&4), 0); // slashed 2
+		assert_eq!(Balances::free_balance(&4), 10 - expected_deposit);
+		assert_eq!(Balances::reserved_balance(&4), 0); // slashed curator deposit
 	});
 }
 
@@ -652,10 +663,12 @@ fn award_and_claim_bounty_works() {
 		System::set_block_number(2);
 		<Treasury as OnInitialize<u64>>::on_initialize(2);
 
-		assert_ok!(Bounties::propose_curator(Origin::root(), 0, 4, 4));
+		let fee = 4;
+		assert_ok!(Bounties::propose_curator(Origin::root(), 0, 4, fee));
 		assert_ok!(Bounties::accept_curator(Origin::signed(4), 0));
 
-		assert_eq!(Balances::free_balance(4), 8); // inital 10 - 2 deposit
+		let expected_deposit = Bounties::calculate_curator_deposit(&fee);
+		assert_eq!(Balances::free_balance(4), 10 - expected_deposit);
 
 		assert_noop!(
 			Bounties::award_bounty(Origin::signed(1), 0, 3),
@@ -668,8 +681,8 @@ fn award_and_claim_bounty_works() {
 			Bounties::bounties(0).unwrap(),
 			Bounty {
 				proposer: 0,
-				fee: 4,
-				curator_deposit: 2,
+				fee,
+				curator_deposit: expected_deposit,
 				value: 50,
 				bond: 85,
 				status: BountyStatus::PendingPayout { curator: 4, beneficiary: 3, unlock_at: 5 },
@@ -1034,3 +1047,78 @@ fn unassign_curator_self() {
 		assert_eq!(Balances::reserved_balance(1), 0); // not slashed
 	});
 }
+
+#[test]
+fn accept_curator_handles_different_deposit_calculations() {
+	// This test will verify that a bounty with and without a fee results
+	// in a different curator deposit: one using the value, and one using the fee.
+	new_test_ext().execute_with(|| {
+		// Case 1: With a fee
+		let user = 1;
+		let bounty_index = 0;
+		let value = 88;
+		let fee = 42;
+
+		System::set_block_number(1);
+		Balances::make_free_balance_be(&Treasury::account_id(), 101);
+		Balances::make_free_balance_be(&user, 100);
+		assert_ok!(Bounties::propose_bounty(Origin::signed(0), value, b"12345".to_vec()));
+		assert_ok!(Bounties::approve_bounty(Origin::root(), bounty_index));
+
+		System::set_block_number(2);
+		<Treasury as OnInitialize<u64>>::on_initialize(2);
+
+		assert_ok!(Bounties::propose_curator(Origin::root(), bounty_index, user, fee));
+		assert_ok!(Bounties::accept_curator(Origin::signed(user), bounty_index));
+
+		let expected_deposit = CuratorDepositMultiplier::get() * fee;
+		assert_eq!(Balances::free_balance(&user), 100 - expected_deposit);
+		assert_eq!(Balances::reserved_balance(&user), expected_deposit);
+
+		// Case 2: Lower bound
+		let user = 2;
+		let bounty_index = 1;
+		let value = 35;
+		let fee = 0;
+
+		Balances::make_free_balance_be(&Treasury::account_id(), 101);
+		Balances::make_free_balance_be(&user, 100);
+
+		assert_ok!(Bounties::propose_bounty(Origin::signed(0), value, b"12345".to_vec()));
+		assert_ok!(Bounties::approve_bounty(Origin::root(), bounty_index));
+
+		System::set_block_number(3);
+		<Treasury as OnInitialize<u64>>::on_initialize(3);
+
+		assert_ok!(Bounties::propose_curator(Origin::root(), bounty_index, user, fee));
+		assert_ok!(Bounties::accept_curator(Origin::signed(user), bounty_index));
+
+		let expected_deposit = CuratorDepositMin::get();
+		assert_eq!(Balances::free_balance(&user), 100 - expected_deposit);
+		assert_eq!(Balances::reserved_balance(&user), expected_deposit);
+
+		// Case 3: Upper bound
+		let user = 3;
+		let bounty_index = 2;
+		let value = 1_000_000;
+		let fee = 50_000;
+		let starting_balance = fee * 2;
+
+		Balances::make_free_balance_be(&Treasury::account_id(), value * 2);
+		Balances::make_free_balance_be(&user, starting_balance);
+		Balances::make_free_balance_be(&0, starting_balance);
+
+		assert_ok!(Bounties::propose_bounty(Origin::signed(0), value, b"12345".to_vec()));
+		assert_ok!(Bounties::approve_bounty(Origin::root(), bounty_index));
+
+		System::set_block_number(3);
+		<Treasury as OnInitialize<u64>>::on_initialize(3);
+
+		assert_ok!(Bounties::propose_curator(Origin::root(), bounty_index, user, fee));
+		assert_ok!(Bounties::accept_curator(Origin::signed(user), bounty_index));
+
+		let expected_deposit = CuratorDepositMax::get();
+		assert_eq!(Balances::free_balance(&user), starting_balance - expected_deposit);
+		assert_eq!(Balances::reserved_balance(&user), expected_deposit);
+	});
+}