From 37951096e6d0d267e5b60e467b8f65bc8ed5d0af Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 17 Sep 2020 19:58:57 +0200 Subject: [PATCH 1/5] proposed fixes to bounties --- frame/treasury/src/lib.rs | 202 ++++++++++++++++++++++---------------- 1 file changed, 115 insertions(+), 87 deletions(-) diff --git a/frame/treasury/src/lib.rs b/frame/treasury/src/lib.rs index 11dd605c33fc0..23e45daaceca9 100644 --- a/frame/treasury/src/lib.rs +++ b/frame/treasury/src/lib.rs @@ -332,8 +332,8 @@ pub enum BountyStatus { Approved, /// The bounty is funded and waiting for curator assignment. Funded, - /// A curator is assigned. Waiting for acceptance from curator. - CuratorAssigned { + /// A curator has been proposed by the `ApproveOrigin`. Waiting for acceptance from the curator. + CuratorProposed { /// The assigned curator of this bounty. curator: AccountId, }, @@ -341,8 +341,8 @@ pub enum BountyStatus { Active { /// The curator of this bounty. curator: AccountId, - /// Expiry block - expires: BlockNumber, + /// An update from the curator is due by this block, else they are considered inactive. + update_due: BlockNumber, }, /// The bounty is awarded and waiting to released after a delay. PendingPayout { @@ -478,6 +478,9 @@ decl_error! { InvalidValue, /// Invalid bounty fee. InvalidFee, + /// A bounty payout is pending. + /// To cancel the bounty, you must unassign and slash the curator. + PendingPayout, } } @@ -793,7 +796,7 @@ decl_module! { Self::payout_tip(hash, tip); } - /// Propose for a new bounty. + /// Propose a new bounty. /// /// The dispatch origin for this call must be _Signed_. /// @@ -812,39 +815,9 @@ decl_module! { description: Vec, ) { let proposer = ensure_signed(origin)?; - Self::create_bounty(proposer, description, value)?; } - /// Reject a bounty proposal. The original deposit will be slashed. - /// - /// # - /// - O(1). - /// - Limited storage reads. - /// - Two DB clear. - /// # - #[weight = T::WeightInfo::reject_bounty()] - fn reject_bounty(origin, #[compact] bounty_id: BountyIndex) { - T::RejectOrigin::ensure_origin(origin)?; - - Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { - let bounty = maybe_bounty.as_ref().ok_or(Error::::InvalidIndex)?; - ensure!(bounty.status == BountyStatus::Proposed, Error::::UnexpectedStatus); - - BountyDescriptions::::remove(bounty_id); - - let value = bounty.bond; - let imbalance = T::Currency::slash_reserved(&bounty.proposer, value).0; - T::OnSlash::on_unbalanced(imbalance); - - Self::deposit_event(Event::::BountyRejected(bounty_id, value)); - - *maybe_bounty = None; - - Ok(()) - })?; - } - /// Approve a bounty proposal. At a later time, the bounty will be funded and become active /// and the original deposit will be returned. /// @@ -893,13 +866,13 @@ decl_module! { Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { let mut bounty = maybe_bounty.as_mut().ok_or(Error::::InvalidIndex)?; match bounty.status { - BountyStatus::Funded | BountyStatus::CuratorAssigned { .. } => {}, + BountyStatus::Funded | BountyStatus::CuratorProposed { .. } => {}, _ => return Err(Error::::UnexpectedStatus.into()), }; ensure!(fee < bounty.value, Error::::InvalidFee); - bounty.status = BountyStatus::CuratorAssigned { curator }; + bounty.status = BountyStatus::CuratorProposed { curator }; bounty.fee = fee; Ok(()) @@ -907,9 +880,19 @@ decl_module! { } /// Unassign curator from a bounty. - /// If the bounty is on active or pending payout status, the curator deposit will be slashed from curator. /// - /// May only be called from `T::ApproveOrigin` or the curator. + /// This function can only be called by the `RejectOrigin` a signed origin. + /// + /// If this function is called by the `RejectOrigin`, we assume that the curator is malicious + /// or inactive. As a result, we will slash the curator when possible. + /// + /// If the origin is the curator, we take this as a sign they are unable to do their job and + /// they willingly give up. We could slash them, but for now we allow them to recover their + /// deposit and exit without issue. (We may want to change this if it is abused.) + /// + /// Finally, the origin can be anyone if and only if the curator is "inactive". This allows + /// anyone in the community to call out that a curator is not doing their due diligence, and + /// we should pick a new curator. In this case the curator should also be slashed. /// /// # /// - O(1). @@ -921,23 +904,68 @@ decl_module! { origin, #[compact] bounty_id: ProposalIndex, ) { - let maybe_curator = ensure_signed(origin.clone()) + let maybe_sender = ensure_signed(origin.clone()) .map(Some) .or_else(|_| T::RejectOrigin::ensure_origin(origin).map(|_| None))?; Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { let mut bounty = maybe_bounty.as_mut().ok_or(Error::::InvalidIndex)?; - match bounty.status { - BountyStatus::CuratorAssigned { ref curator } => { - ensure!(maybe_curator.map_or(true, |c| c == *curator), BadOrigin); - }, - BountyStatus::Active { ref curator, .. } | BountyStatus::PendingPayout { ref curator, .. } => { - ensure!(maybe_curator.map_or(true, |c| c == *curator), BadOrigin); - let imbalance = T::Currency::slash_reserved(curator, bounty.curator_deposit).0; + + macro_rules! slash_curator { + ($curator:ident) => { + let imbalance = T::Currency::slash_reserved($curator, bounty.curator_deposit).0; T::OnSlash::on_unbalanced(imbalance); bounty.curator_deposit = Zero::zero(); + } + }; + + match bounty.status { + BountyStatus::Proposed | BountyStatus::Approved | BountyStatus::Funded => { + // No curator to unassign at this point. + return Err(Error::::UnexpectedStatus.into()) + } + BountyStatus::CuratorProposed { ref curator } => { + // A curator has been proposed, but not accepted yet. + // Either `RejectOrigin` or the proposed curator can unassign the curator. + ensure!(maybe_sender.map_or(true, |sender| sender == *curator), BadOrigin); }, - _ => return Err(Error::::UnexpectedStatus.into()), + BountyStatus::Active { ref curator, ref update_due } => { + // The bounty is active. + match maybe_sender { + // If the `RejectOrigin` is calling this function, slash the curator. + None => { + slash_curator!(curator); + // Continue to change bounty status below... + }, + Some(sender) => { + // If the sender is not the curator, and the curator is inactive, + // slash the curator. + if sender != *curator { + let block_number = system::Module::::block_number(); + if *update_due < block_number { + slash_curator!(curator); + // Continue to change bounty status below... + } else { + // Curator has more time to give an update. + return Err(Error::::Premature.into()) + } + } else { + // Else this is the curator, willingly giving up their role. + // Give back their deposit. + let _ = T::Currency::unreserve(&curator, bounty.curator_deposit); + // Continue to change bounty status below... + } + }, + } + }, + BountyStatus::PendingPayout { ref curator, .. } => { + // The bounty is pending payout, so only council can unassign a curator. + // By doing so, they are claiming the curator is acting maliciously, so + // we slash the curator. + ensure!(maybe_sender.is_none(), BadOrigin); + slash_curator!(curator); + // Continue to change bounty status below... + } }; bounty.status = BountyStatus::Funded; @@ -963,15 +991,15 @@ decl_module! { let mut bounty = maybe_bounty.as_mut().ok_or(Error::::InvalidIndex)?; match bounty.status { - BountyStatus::CuratorAssigned { ref curator } => { + BountyStatus::CuratorProposed { ref curator } => { ensure!(signer == *curator, Error::::RequireCurator); let deposit = T::BountyCuratorDeposit::get() * bounty.fee; T::Currency::reserve(curator, deposit)?; bounty.curator_deposit = deposit; - let expires = system::Module::::block_number() + T::BountyDuration::get(); - bounty.status = BountyStatus::Active { curator: curator.clone(), expires }; + let update_due = system::Module::::block_number() + T::BountyDuration::get(); + bounty.status = BountyStatus::Active { curator: curator.clone(), update_due }; Ok(()) }, @@ -1046,53 +1074,53 @@ decl_module! { })?; } - /// Cancel an active bounty. All the funds will be send to treasury. - /// If cancel is not initiated by `T::RejectOrigin`, curator deposit will be slashed. + /// Cancel a proposed or active bounty. All the funds will be sent to treasury and + /// the curator deposit will be unreserved if possible. /// - /// Only curator or `T::RejectOrigin` is able to cancel bounty. + /// Only `T::RejectOrigin` is able to cancel a bounty. /// /// - `bounty_id`: Bounty ID to cancel. #[weight = T::WeightInfo::cancel_bounty()] fn cancel_bounty(origin, #[compact] bounty_id: BountyIndex) { - let maybe_curator = ensure_signed(origin.clone()) - .map(Some) - .or_else(|_| T::RejectOrigin::ensure_origin(origin).map(|_| None))?; + T::RejectOrigin::ensure_origin(origin)?; Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { let bounty = maybe_bounty.as_ref().ok_or(Error::::InvalidIndex)?; match &bounty.status { - BountyStatus::Funded | - BountyStatus::CuratorAssigned { .. } => { - ensure!(maybe_curator.is_none(), BadOrigin); - }, - BountyStatus::PendingPayout { curator, .. } => { - if let Some(signer) = maybe_curator { - ensure!(signer == *curator, Error::::RequireCurator); + BountyStatus::Proposed => { + // The reject origin would like to cancel a proposed bounty. + BountyDescriptions::::remove(bounty_id); + let value = bounty.bond; + let imbalance = T::Currency::slash_reserved(&bounty.proposer, value).0; + T::OnSlash::on_unbalanced(imbalance); + *maybe_bounty = None; - let imbalance = T::Currency::slash_reserved(curator, bounty.curator_deposit).0; - T::OnSlash::on_unbalanced(imbalance); - } else { - // Cancelled by council, refund deposit - let _ = T::Currency::unreserve(&curator, bounty.curator_deposit); - } + Self::deposit_event(Event::::BountyRejected(bounty_id, value)); + // Return early, nothing else to do. + return Ok(()) }, - BountyStatus::Active { curator, expires } => { - if let Some(signer) = maybe_curator { - let now = system::Module::::block_number(); - if *expires > now { - // only curator can cancel unexpired bounty - ensure!(signer == *curator, Error::::RequireCurator); - } - - let imbalance = T::Currency::slash_reserved(curator, bounty.curator_deposit).0; - T::OnSlash::on_unbalanced(imbalance); - } else { - // Cancelled by council, refund deposit - let _ = T::Currency::unreserve(&curator, bounty.curator_deposit); - } + BountyStatus::Approved => { + // 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::::UnexpectedStatus.into()) }, - _ => return Err(Error::::UnexpectedStatus.into()), + BountyStatus::Funded | + BountyStatus::CuratorProposed { .. } => { + // Nothing extra to do besides the removal of the bounty below. + }, + BountyStatus::Active { curator, .. } => { + // Cancelled by council, refund deposit of the working curator. + let _ = T::Currency::unreserve(&curator, bounty.curator_deposit); + // Then execute removal of the bounty below. + }, + BountyStatus::PendingPayout { .. } => { + // Bounty is already pending payout. If council wants to cancel + // this bounty, it should mean the curator was acting maliciously. + // So the council should first unassign the curator, slashing their + // deposit. + return Err(Error::::PendingPayout.into()) + } } let bounty_account = Self::bounty_account_id(bounty_id); @@ -1123,9 +1151,9 @@ decl_module! { let bounty = maybe_bounty.as_mut().ok_or(Error::::InvalidIndex)?; match bounty.status { - BountyStatus::Active { ref curator, ref mut expires } => { + BountyStatus::Active { ref curator, ref mut update_due } => { ensure!(*curator == signer, Error::::RequireCurator); - *expires = (system::Module::::block_number() + T::BountyDuration::get()).max(*expires); + *update_due = (system::Module::::block_number() + T::BountyDuration::get()).max(*update_due); }, _ => return Err(Error::::UnexpectedStatus.into()), } From e833a53be9342aae40bd3afd604b6ddc5233b972 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 17 Sep 2020 20:39:42 +0200 Subject: [PATCH 2/5] fix tests --- frame/treasury/src/default_weights.rs | 9 +-- frame/treasury/src/lib.rs | 21 +++---- frame/treasury/src/tests.rs | 90 ++++++++++++++++----------- 3 files changed, 63 insertions(+), 57 deletions(-) diff --git a/frame/treasury/src/default_weights.rs b/frame/treasury/src/default_weights.rs index 7d115e51768a1..58a14ac2cd2e7 100644 --- a/frame/treasury/src/default_weights.rs +++ b/frame/treasury/src/default_weights.rs @@ -72,17 +72,12 @@ impl crate::WeightInfo for () { .saturating_add(DbWeight::get().reads(1 as Weight)) .saturating_add(DbWeight::get().writes(3 as Weight)) } - fn reject_bounty() -> Weight { - (47109000 as Weight) - .saturating_add(DbWeight::get().reads(1 as Weight)) - .saturating_add(DbWeight::get().writes(2 as Weight)) - } fn approve_bounty() -> Weight { (14829000 as Weight) .saturating_add(DbWeight::get().reads(2 as Weight)) .saturating_add(DbWeight::get().writes(2 as Weight)) } - fn assign_curator() -> Weight { + fn propose_curator() -> Weight { (11392000 as Weight) .saturating_add(DbWeight::get().reads(1 as Weight)) .saturating_add(DbWeight::get().writes(1 as Weight)) @@ -107,7 +102,7 @@ impl crate::WeightInfo for () { .saturating_add(DbWeight::get().reads(4 as Weight)) .saturating_add(DbWeight::get().writes(5 as Weight)) } - fn cancel_bounty() -> Weight { + fn close_bounty() -> Weight { (70335000 as Weight) .saturating_add(DbWeight::get().reads(2 as Weight)) .saturating_add(DbWeight::get().writes(3 as Weight)) diff --git a/frame/treasury/src/lib.rs b/frame/treasury/src/lib.rs index 23e45daaceca9..ae91072a4665c 100644 --- a/frame/treasury/src/lib.rs +++ b/frame/treasury/src/lib.rs @@ -118,15 +118,14 @@ //! Bounty protocol: //! - `propose_bounty` - Propose a specific treasury amount to be earmarked for a predefined set of //! tasks and stake the required deposit. -//! - `reject_bounty` - Reject a specific treasury amount to be earmarked for a predefined body of work. //! - `approve_bounty` - Accept a specific treasury amount to be earmarked for a predefined body of work. -//! - `assign_curator` - Assign an account to a bounty as candidate curator. +//! - `propose_curator` - Assign an account to a bounty as candidate curator. //! - `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. -//! - `cancel_bounty` - Cancel the earmark for a specific treasury amount and close the bounty. +//! - `close_bounty` - Cancel the earmark for a specific treasury amount and close the bounty. //! //! //! ## GenesisConfig @@ -173,13 +172,12 @@ pub trait WeightInfo { fn close_tip(t: u32, ) -> Weight; fn propose_bounty(r: u32, ) -> Weight; fn approve_bounty() -> Weight; - fn assign_curator() -> Weight; + fn propose_curator() -> Weight; fn unassign_curator() -> Weight; fn accept_curator() -> Weight; - fn reject_bounty() -> Weight; fn award_bounty() -> Weight; fn claim_bounty() -> Weight; - fn cancel_bounty() -> Weight; + fn close_bounty() -> Weight; fn extend_bounty_expiry() -> Weight; fn on_initialize_proposals(p: u32, ) -> Weight; fn on_initialize_bounties(b: u32, ) -> Weight; @@ -853,8 +851,8 @@ decl_module! { /// - Limited storage reads. /// - One DB change. /// # - #[weight = T::WeightInfo::assign_curator()] - fn assign_curator( + #[weight = T::WeightInfo::propose_curator()] + fn propose_curator( origin, #[compact] bounty_id: ProposalIndex, curator: ::Source, @@ -1080,8 +1078,8 @@ decl_module! { /// Only `T::RejectOrigin` is able to cancel a bounty. /// /// - `bounty_id`: Bounty ID to cancel. - #[weight = T::WeightInfo::cancel_bounty()] - fn cancel_bounty(origin, #[compact] bounty_id: BountyIndex) { + #[weight = T::WeightInfo::close_bounty()] + fn close_bounty(origin, #[compact] bounty_id: BountyIndex) { T::RejectOrigin::ensure_origin(origin)?; Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { @@ -1131,10 +1129,9 @@ decl_module! { let _ = T::Currency::transfer(&bounty_account, &Self::account_id(), balance, AllowDeath); // should not fail *maybe_bounty = None; + Self::deposit_event(Event::::BountyCanceled(bounty_id)); Ok(()) })?; - - Self::deposit_event(Event::::BountyCanceled(bounty_id)); } /// Extend the expiry time of an active bounty. diff --git a/frame/treasury/src/tests.rs b/frame/treasury/src/tests.rs index 5a383cebf4863..fcc719b20b0f9 100644 --- a/frame/treasury/src/tests.rs +++ b/frame/treasury/src/tests.rs @@ -615,15 +615,15 @@ fn propose_bounty_validation_works() { } #[test] -fn reject_bounty_works() { +fn close_bounty_works() { new_test_ext().execute_with(|| { System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); - assert_noop!(Treasury::reject_bounty(Origin::root(), 0), Error::::InvalidIndex); + assert_noop!(Treasury::close_bounty(Origin::root(), 0), Error::::InvalidIndex); assert_ok!(Treasury::propose_bounty(Origin::signed(0), 10, b"12345".to_vec())); - assert_ok!(Treasury::reject_bounty(Origin::root(), 0)); + assert_ok!(Treasury::close_bounty(Origin::root(), 0)); let deposit: u64 = 80 + 5; @@ -661,7 +661,7 @@ fn approve_bounty_works() { }); assert_eq!(Treasury::bounty_approvals(), vec![0]); - assert_noop!(Treasury::reject_bounty(Origin::root(), 0), Error::::UnexpectedStatus); + assert_noop!(Treasury::close_bounty(Origin::root(), 0), Error::::UnexpectedStatus); // deposit not returned yet assert_eq!(Balances::reserved_balance(0), deposit); @@ -692,7 +692,7 @@ fn assign_curator_works() { System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); - assert_noop!(Treasury::assign_curator(Origin::root(), 0, 4, 4), Error::::InvalidIndex); + assert_noop!(Treasury::propose_curator(Origin::root(), 0, 4, 4), Error::::InvalidIndex); assert_ok!(Treasury::propose_bounty(Origin::signed(0), 50, b"12345".to_vec())); @@ -701,9 +701,9 @@ fn assign_curator_works() { System::set_block_number(2); >::on_initialize(2); - assert_noop!(Treasury::assign_curator(Origin::root(), 0, 4, 50), Error::::InvalidFee); + assert_noop!(Treasury::propose_curator(Origin::root(), 0, 4, 50), Error::::InvalidFee); - assert_ok!(Treasury::assign_curator(Origin::root(), 0, 4, 4)); + assert_ok!(Treasury::propose_curator(Origin::root(), 0, 4, 4)); assert_eq!(Treasury::bounties(0).unwrap(), Bounty { proposer: 0, @@ -711,7 +711,7 @@ fn assign_curator_works() { curator_deposit: 0, value: 50, bond: 85, - status: BountyStatus::CuratorAssigned { + status: BountyStatus::CuratorProposed { curator: 4, }, }); @@ -731,7 +731,7 @@ fn assign_curator_works() { bond: 85, status: BountyStatus::Active { curator: 4, - expires: 22, + update_due: 22, }, }); @@ -752,7 +752,7 @@ fn unassign_curator_works() { System::set_block_number(2); >::on_initialize(2); - assert_ok!(Treasury::assign_curator(Origin::root(), 0, 4, 4)); + assert_ok!(Treasury::propose_curator(Origin::root(), 0, 4, 4)); assert_noop!(Treasury::unassign_curator(Origin::signed(1), 0), BadOrigin); @@ -767,7 +767,7 @@ fn unassign_curator_works() { status: BountyStatus::Funded, }); - assert_ok!(Treasury::assign_curator(Origin::root(), 0, 4, 4)); + assert_ok!(Treasury::propose_curator(Origin::root(), 0, 4, 4)); Balances::make_free_balance_be(&4, 10); @@ -802,7 +802,7 @@ fn award_and_claim_bounty_works() { System::set_block_number(2); >::on_initialize(2); - assert_ok!(Treasury::assign_curator(Origin::root(), 0, 4, 4)); + assert_ok!(Treasury::propose_curator(Origin::root(), 0, 4, 4)); assert_ok!(Treasury::accept_curator(Origin::signed(4), 0)); assert_eq!(Balances::free_balance(4), 8); // inital 10 - 2 deposit @@ -857,7 +857,7 @@ fn claim_handles_high_fee() { System::set_block_number(2); >::on_initialize(2); - assert_ok!(Treasury::assign_curator(Origin::root(), 0, 4, 49)); + assert_ok!(Treasury::propose_curator(Origin::root(), 0, 4, 49)); assert_ok!(Treasury::accept_curator(Origin::signed(4), 0)); assert_ok!(Treasury::award_bounty(Origin::signed(4), 0, 3)); @@ -888,8 +888,6 @@ fn cancel_and_refund() { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Treasury::propose_bounty(Origin::signed(0), 50, b"12345".to_vec())); - assert_noop!(Treasury::cancel_bounty(Origin::signed(1), 0), Error::::UnexpectedStatus); - assert_ok!(Treasury::approve_bounty(Origin::root(), 0)); System::set_block_number(2); @@ -908,9 +906,9 @@ fn cancel_and_refund() { assert_eq!(Balances::free_balance(Treasury::bounty_account_id(0)), 60); - assert_noop!(Treasury::cancel_bounty(Origin::signed(0), 0), BadOrigin); + assert_noop!(Treasury::close_bounty(Origin::signed(0), 0), BadOrigin); - assert_ok!(Treasury::cancel_bounty(Origin::root(), 0)); + assert_ok!(Treasury::close_bounty(Origin::root(), 0)); assert_eq!(Treasury::pot(), 85); // - 25 + 10 }); @@ -928,7 +926,7 @@ fn award_and_cancel() { System::set_block_number(2); >::on_initialize(2); - assert_ok!(Treasury::assign_curator(Origin::root(), 0, 0, 10)); + assert_ok!(Treasury::propose_curator(Origin::root(), 0, 0, 10)); assert_ok!(Treasury::accept_curator(Origin::signed(0), 0)); assert_eq!(Balances::free_balance(0), 95); @@ -936,12 +934,18 @@ fn award_and_cancel() { assert_ok!(Treasury::award_bounty(Origin::signed(0), 0, 3)); - assert_ok!(Treasury::cancel_bounty(Origin::root(), 0)); + // Cannot close bounty directly when payout is happening... + assert_noop!(Treasury::close_bounty(Origin::root(), 0), Error::::PendingPayout); + + // Instead unassign the curator to slash them and then close. + assert_ok!(Treasury::unassign_curator(Origin::root(), 0)); + assert_ok!(Treasury::close_bounty(Origin::root(), 0)); assert_eq!(last_event(), RawEvent::BountyCanceled(0)); assert_eq!(Balances::free_balance(Treasury::bounty_account_id(0)), 0); - assert_eq!(Balances::free_balance(0), 100); + // Slashed. + assert_eq!(Balances::free_balance(0), 95); assert_eq!(Balances::reserved_balance(0), 0); assert_eq!(Treasury::bounties(0), None); @@ -950,33 +954,45 @@ fn award_and_cancel() { } #[test] -fn expire_and_cancel() { +fn expire_and_unassign() { new_test_ext().execute_with(|| { System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Treasury::propose_bounty(Origin::signed(0), 50, b"12345".to_vec())); - assert_noop!(Treasury::cancel_bounty(Origin::signed(1), 0), Error::::UnexpectedStatus); - assert_ok!(Treasury::approve_bounty(Origin::root(), 0)); System::set_block_number(2); >::on_initialize(2); - assert_ok!(Treasury::assign_curator(Origin::root(), 0, 1, 10)); + assert_ok!(Treasury::propose_curator(Origin::root(), 0, 1, 10)); assert_ok!(Treasury::accept_curator(Origin::signed(1), 0)); - System::set_block_number(21); - >::on_initialize(21); - - assert_noop!(Treasury::cancel_bounty(Origin::signed(0), 0), Error::::RequireCurator); + assert_eq!(Balances::free_balance(1), 93); + assert_eq!(Balances::reserved_balance(1), 5); System::set_block_number(22); >::on_initialize(22); - assert_ok!(Treasury::cancel_bounty(Origin::signed(0), 0)); + assert_noop!(Treasury::unassign_curator(Origin::signed(0), 0), Error::::Premature); + + System::set_block_number(23); + >::on_initialize(23); + + assert_ok!(Treasury::unassign_curator(Origin::signed(0), 0)); + + assert_eq!(Treasury::bounties(0).unwrap(), Bounty { + proposer: 0, + fee: 10, + curator_deposit: 0, + value: 50, + bond: 85, + status: BountyStatus::Funded, + }); + + assert_eq!(Balances::free_balance(1), 93); + assert_eq!(Balances::reserved_balance(1), 0); // slashed - assert_eq!(Treasury::pot(), 63); // 100 - 25 - 12 }); } @@ -988,8 +1004,6 @@ fn extend_expiry() { Balances::make_free_balance_be(&4, 10); assert_ok!(Treasury::propose_bounty(Origin::signed(0), 50, b"12345".to_vec())); - assert_noop!(Treasury::cancel_bounty(Origin::signed(1), 0), Error::::UnexpectedStatus); - assert_ok!(Treasury::approve_bounty(Origin::root(), 0)); assert_noop!(Treasury::extend_bounty_expiry(Origin::signed(1), 0, Vec::new()), Error::::UnexpectedStatus); @@ -997,7 +1011,7 @@ fn extend_expiry() { System::set_block_number(2); >::on_initialize(2); - assert_ok!(Treasury::assign_curator(Origin::root(), 0, 4, 10)); + assert_ok!(Treasury::propose_curator(Origin::root(), 0, 4, 10)); assert_ok!(Treasury::accept_curator(Origin::signed(4), 0)); assert_eq!(Balances::free_balance(4), 5); @@ -1015,7 +1029,7 @@ fn extend_expiry() { curator_deposit: 5, value: 50, bond: 85, - status: BountyStatus::Active { curator: 4, expires: 30 }, + status: BountyStatus::Active { curator: 4, update_due: 30 }, }); assert_ok!(Treasury::extend_bounty_expiry(Origin::signed(4), 0, Vec::new())); @@ -1026,16 +1040,16 @@ fn extend_expiry() { curator_deposit: 5, value: 50, bond: 85, - status: BountyStatus::Active { curator: 4, expires: 30 }, // still the same + status: BountyStatus::Active { curator: 4, update_due: 30 }, // still the same }); System::set_block_number(25); >::on_initialize(25); - assert_noop!(Treasury::cancel_bounty(Origin::signed(0), 0), Error::::RequireCurator); - assert_ok!(Treasury::cancel_bounty(Origin::signed(4), 0)); + assert_noop!(Treasury::unassign_curator(Origin::signed(0), 0), Error::::Premature); + assert_ok!(Treasury::unassign_curator(Origin::signed(4), 0)); - assert_eq!(Balances::free_balance(4), 5); // slashed 5 + assert_eq!(Balances::free_balance(4), 10); // not slashed assert_eq!(Balances::reserved_balance(4), 0); }); } From e6f95c659554db30d81e3b3bc1bd10a4acd0c8e2 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 17 Sep 2020 21:08:58 +0200 Subject: [PATCH 3/5] fix benchmarks --- bin/node/runtime/src/lib.rs | 4 +-- frame/treasury/src/benchmarking.rs | 44 ++++++++++++++---------------- frame/treasury/src/lib.rs | 6 ++-- frame/treasury/src/tests.rs | 4 +-- 4 files changed, 28 insertions(+), 30 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index af8d53effcfce..cf5ff86e75e72 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -602,7 +602,7 @@ parameter_types! { pub const BountyDepositBase: Balance = 1 * DOLLARS; pub const BountyDepositPayoutDelay: BlockNumber = 1 * DAYS; pub const TreasuryModuleId: ModuleId = ModuleId(*b"py/trsry"); - pub const BountyDuration: BlockNumber = 14 * DAYS; + pub const BountyUpdatePeriod: BlockNumber = 14 * DAYS; pub const MaximumReasonLength: u32 = 16384; pub const BountyCuratorDeposit: Permill = Permill::from_percent(50); pub const BountyValueMinimum: Balance = 5 * DOLLARS; @@ -634,7 +634,7 @@ impl pallet_treasury::Trait for Runtime { type Burn = Burn; type BountyDepositBase = BountyDepositBase; type BountyDepositPayoutDelay = BountyDepositPayoutDelay; - type BountyDuration = BountyDuration; + type BountyUpdatePeriod = BountyUpdatePeriod; type BountyCuratorDeposit = BountyCuratorDeposit; type BountyValueMinimum = BountyValueMinimum; type MaximumReasonLength = MaximumReasonLength; diff --git a/frame/treasury/src/benchmarking.rs b/frame/treasury/src/benchmarking.rs index a8510962662d5..8d0a7ff073eb7 100644 --- a/frame/treasury/src/benchmarking.rs +++ b/frame/treasury/src/benchmarking.rs @@ -150,7 +150,7 @@ fn create_bounty, I: Instance>() -> Result<( let bounty_id = BountyCount::::get() - 1; Treasury::::approve_bounty(RawOrigin::Root.into(), bounty_id)?; Treasury::::on_initialize(T::BlockNumber::zero()); - Treasury::::assign_curator(RawOrigin::Root.into(), bounty_id, curator_lookup.clone(), fee)?; + Treasury::::propose_curator(RawOrigin::Root.into(), bounty_id, curator_lookup.clone(), fee)?; Treasury::::accept_curator(RawOrigin::Signed(curator).into(), bounty_id)?; Ok((curator_lookup, bounty_id)) } @@ -284,19 +284,13 @@ benchmarks_instance! { let (caller, curator, fee, value, description) = setup_bounty::(0, d); }: _(RawOrigin::Signed(caller), value, description) - reject_bounty { - let (caller, curator, fee, value, reason) = setup_bounty::(0, MAX_BYTES); - Treasury::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; - let bounty_id = BountyCount::::get() - 1; - }: _(RawOrigin::Root, bounty_id) - approve_bounty { let (caller, curator, fee, value, reason) = setup_bounty::(0, MAX_BYTES); Treasury::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; let bounty_id = BountyCount::::get() - 1; }: _(RawOrigin::Root, bounty_id) - assign_curator { + propose_curator { setup_pod_account::(); let (caller, curator, fee, value, reason) = setup_bounty::(0, MAX_BYTES); let curator_lookup = T::Lookup::unlookup(curator.clone()); @@ -306,16 +300,15 @@ benchmarks_instance! { Treasury::::on_initialize(T::BlockNumber::zero()); }: _(RawOrigin::Root, bounty_id, curator_lookup, fee) + // Worst case when curator is inactive and any sender unassigns the curator. unassign_curator { setup_pod_account::(); - let (caller, curator, fee, value, reason) = setup_bounty::(0, MAX_BYTES); - let curator_lookup = T::Lookup::unlookup(curator.clone()); - Treasury::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; - let bounty_id = BountyCount::::get() - 1; - Treasury::::approve_bounty(RawOrigin::Root.into(), bounty_id)?; + let (curator_lookup, bounty_id) = create_bounty::()?; Treasury::::on_initialize(T::BlockNumber::zero()); - Treasury::::assign_curator(RawOrigin::Root.into(), bounty_id, curator_lookup, fee)?; - }: _(RawOrigin::Root, bounty_id) + let bounty_id = BountyCount::::get() - 1; + frame_system::Module::::set_block_number(T::BountyUpdatePeriod::get() + 1.into()); + let caller = whitelisted_caller(); + }: _(RawOrigin::Signed(caller), bounty_id) accept_curator { setup_pod_account::(); @@ -325,7 +318,7 @@ benchmarks_instance! { let bounty_id = BountyCount::::get() - 1; Treasury::::approve_bounty(RawOrigin::Root.into(), bounty_id)?; Treasury::::on_initialize(T::BlockNumber::zero()); - Treasury::::assign_curator(RawOrigin::Root.into(), bounty_id, curator_lookup, fee)?; + Treasury::::propose_curator(RawOrigin::Root.into(), bounty_id, curator_lookup, fee)?; }: _(RawOrigin::Signed(curator), bounty_id) award_bounty { @@ -353,14 +346,19 @@ benchmarks_instance! { }: _(RawOrigin::Signed(curator), bounty_id) - cancel_bounty { + close_bounty_proposed { + setup_pod_account::(); + let (caller, curator, fee, value, reason) = setup_bounty::(0, 0); + Treasury::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; + let bounty_id = BountyCount::::get() - 1; + }: close_bounty(RawOrigin::Root, bounty_id) + + close_bounty_active { setup_pod_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; Treasury::::on_initialize(T::BlockNumber::zero()); - let bounty_id = BountyCount::::get() - 1; - let curator = T::Lookup::lookup(curator_lookup)?; - }: _(RawOrigin::Signed(curator), bounty_id) + }: close_bounty(RawOrigin::Root, bounty_id) extend_bounty_expiry { setup_pod_account::(); @@ -407,13 +405,13 @@ mod tests { assert_ok!(test_benchmark_close_tip::()); assert_ok!(test_benchmark_propose_bounty::()); assert_ok!(test_benchmark_approve_bounty::()); - assert_ok!(test_benchmark_reject_bounty::()); - assert_ok!(test_benchmark_assign_curator::()); + assert_ok!(test_benchmark_propose_curator::()); assert_ok!(test_benchmark_unassign_curator::()); assert_ok!(test_benchmark_accept_curator::()); assert_ok!(test_benchmark_award_bounty::()); assert_ok!(test_benchmark_claim_bounty::()); - assert_ok!(test_benchmark_cancel_bounty::()); + assert_ok!(test_benchmark_close_bounty_proposed::()); + assert_ok!(test_benchmark_close_bounty_active::()); assert_ok!(test_benchmark_extend_bounty_expiry::()); assert_ok!(test_benchmark_on_initialize_proposals::()); assert_ok!(test_benchmark_on_initialize_bounties::()); diff --git a/frame/treasury/src/lib.rs b/frame/treasury/src/lib.rs index ae91072a4665c..19031e4a85765 100644 --- a/frame/treasury/src/lib.rs +++ b/frame/treasury/src/lib.rs @@ -239,7 +239,7 @@ pub trait Trait: frame_system::Trait { type BountyDepositPayoutDelay: Get; /// Bounty duration in blocks. - type BountyDuration: Get; + type BountyUpdatePeriod: Get; /// Percentage of the curator fee that will be reserved upfront as deposit for bounty curator. type BountyCuratorDeposit: Get; @@ -996,7 +996,7 @@ decl_module! { T::Currency::reserve(curator, deposit)?; bounty.curator_deposit = deposit; - let update_due = system::Module::::block_number() + T::BountyDuration::get(); + let update_due = system::Module::::block_number() + T::BountyUpdatePeriod::get(); bounty.status = BountyStatus::Active { curator: curator.clone(), update_due }; Ok(()) @@ -1150,7 +1150,7 @@ decl_module! { match bounty.status { BountyStatus::Active { ref curator, ref mut update_due } => { ensure!(*curator == signer, Error::::RequireCurator); - *update_due = (system::Module::::block_number() + T::BountyDuration::get()).max(*update_due); + *update_due = (system::Module::::block_number() + T::BountyUpdatePeriod::get()).max(*update_due); }, _ => return Err(Error::::UnexpectedStatus.into()), } diff --git a/frame/treasury/src/tests.rs b/frame/treasury/src/tests.rs index fcc719b20b0f9..8f0e14f04d468 100644 --- a/frame/treasury/src/tests.rs +++ b/frame/treasury/src/tests.rs @@ -134,7 +134,7 @@ parameter_types! { pub const BountyDepositBase: u64 = 80; pub const BountyDepositPayoutDelay: u64 = 3; pub const TreasuryModuleId: ModuleId = ModuleId(*b"py/trsry"); - pub const BountyDuration: u32 = 20; + pub const BountyUpdatePeriod: u32 = 20; pub const MaximumReasonLength: u32 = 16384; pub const BountyCuratorDeposit: Permill = Permill::from_percent(50); pub const BountyValueMinimum: u64 = 1; @@ -157,7 +157,7 @@ impl Trait for Test { type Burn = Burn; type BountyDepositBase = BountyDepositBase; type BountyDepositPayoutDelay = BountyDepositPayoutDelay; - type BountyDuration = BountyDuration; + type BountyUpdatePeriod = BountyUpdatePeriod; type BountyCuratorDeposit = BountyCuratorDeposit; type BountyValueMinimum = BountyValueMinimum; type MaximumReasonLength = MaximumReasonLength; From 0982f46c0ab04d7459ae85ea36b9e5c938b53527 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 17 Sep 2020 21:18:11 +0200 Subject: [PATCH 4/5] update weightinfo --- frame/treasury/src/default_weights.rs | 7 ++++++- frame/treasury/src/lib.rs | 16 +++++++++------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/frame/treasury/src/default_weights.rs b/frame/treasury/src/default_weights.rs index 58a14ac2cd2e7..cadeaa30a38eb 100644 --- a/frame/treasury/src/default_weights.rs +++ b/frame/treasury/src/default_weights.rs @@ -102,7 +102,12 @@ impl crate::WeightInfo for () { .saturating_add(DbWeight::get().reads(4 as Weight)) .saturating_add(DbWeight::get().writes(5 as Weight)) } - fn close_bounty() -> Weight { + fn close_bounty_proposed() -> Weight { + (70335000 as Weight) + .saturating_add(DbWeight::get().reads(2 as Weight)) + .saturating_add(DbWeight::get().writes(3 as Weight)) + } + fn close_bounty_active() -> Weight { (70335000 as Weight) .saturating_add(DbWeight::get().reads(2 as Weight)) .saturating_add(DbWeight::get().writes(3 as Weight)) diff --git a/frame/treasury/src/lib.rs b/frame/treasury/src/lib.rs index 19031e4a85765..73435c65fdfbc 100644 --- a/frame/treasury/src/lib.rs +++ b/frame/treasury/src/lib.rs @@ -145,6 +145,7 @@ use frame_support::traits::{ use sp_runtime::{Permill, ModuleId, Percent, RuntimeDebug, DispatchResult, traits::{ Zero, StaticLookup, AccountIdConversion, Saturating, Hash, BadOrigin }}; +use frame_support::dispatch::DispatchResultWithPostInfo; use frame_support::weights::{Weight, DispatchClass}; use frame_support::traits::{Contains, ContainsLengthBound, EnsureOrigin}; use codec::{Encode, Decode}; @@ -177,7 +178,8 @@ pub trait WeightInfo { fn accept_curator() -> Weight; fn award_bounty() -> Weight; fn claim_bounty() -> Weight; - fn close_bounty() -> Weight; + fn close_bounty_proposed() -> Weight; + fn close_bounty_active() -> Weight; fn extend_bounty_expiry() -> Weight; fn on_initialize_proposals(p: u32, ) -> Weight; fn on_initialize_bounties(b: u32, ) -> Weight; @@ -1078,11 +1080,11 @@ decl_module! { /// Only `T::RejectOrigin` is able to cancel a bounty. /// /// - `bounty_id`: Bounty ID to cancel. - #[weight = T::WeightInfo::close_bounty()] - fn close_bounty(origin, #[compact] bounty_id: BountyIndex) { + #[weight = T::WeightInfo::close_bounty_proposed().max(T::WeightInfo::close_bounty_active())] + fn close_bounty(origin, #[compact] bounty_id: BountyIndex) -> DispatchResultWithPostInfo { T::RejectOrigin::ensure_origin(origin)?; - Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { + Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResultWithPostInfo { let bounty = maybe_bounty.as_ref().ok_or(Error::::InvalidIndex)?; match &bounty.status { @@ -1096,7 +1098,7 @@ decl_module! { Self::deposit_event(Event::::BountyRejected(bounty_id, value)); // Return early, nothing else to do. - return Ok(()) + return Ok(Some(T::WeightInfo::close_bounty_proposed()).into()) }, BountyStatus::Approved => { // For weight reasons, we don't allow a council to cancel in this phase. @@ -1130,8 +1132,8 @@ decl_module! { *maybe_bounty = None; Self::deposit_event(Event::::BountyCanceled(bounty_id)); - Ok(()) - })?; + Ok(Some(T::WeightInfo::close_bounty_active()).into()) + }) } /// Extend the expiry time of an active bounty. From 784a69a9b0d7a739759d4b8050c185fc129cc6e0 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 18 Sep 2020 00:59:24 +0200 Subject: [PATCH 5/5] use closure --- frame/treasury/src/lib.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/frame/treasury/src/lib.rs b/frame/treasury/src/lib.rs index 73435c65fdfbc..4d76f2f532151 100644 --- a/frame/treasury/src/lib.rs +++ b/frame/treasury/src/lib.rs @@ -911,12 +911,10 @@ decl_module! { Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { let mut bounty = maybe_bounty.as_mut().ok_or(Error::::InvalidIndex)?; - macro_rules! slash_curator { - ($curator:ident) => { - let imbalance = T::Currency::slash_reserved($curator, bounty.curator_deposit).0; - T::OnSlash::on_unbalanced(imbalance); - bounty.curator_deposit = Zero::zero(); - } + let slash_curator = |curator: &T::AccountId, curator_deposit: &mut BalanceOf| { + let imbalance = T::Currency::slash_reserved(curator, *curator_deposit).0; + T::OnSlash::on_unbalanced(imbalance); + *curator_deposit = Zero::zero(); }; match bounty.status { @@ -934,7 +932,7 @@ decl_module! { match maybe_sender { // If the `RejectOrigin` is calling this function, slash the curator. None => { - slash_curator!(curator); + slash_curator(curator, &mut bounty.curator_deposit); // Continue to change bounty status below... }, Some(sender) => { @@ -943,7 +941,7 @@ decl_module! { if sender != *curator { let block_number = system::Module::::block_number(); if *update_due < block_number { - slash_curator!(curator); + slash_curator(curator, &mut bounty.curator_deposit); // Continue to change bounty status below... } else { // Curator has more time to give an update. @@ -963,7 +961,7 @@ decl_module! { // By doing so, they are claiming the curator is acting maliciously, so // we slash the curator. ensure!(maybe_sender.is_none(), BadOrigin); - slash_curator!(curator); + slash_curator(curator, &mut bounty.curator_deposit); // Continue to change bounty status below... } };