diff --git a/Cargo.lock b/Cargo.lock index 37773a4ae3a69..839c68b71fbf1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5121,6 +5121,7 @@ dependencies = [ "frame-benchmarking", "frame-support", "frame-system", + "log 0.4.14", "pallet-balances", "pallet-treasury", "parity-scale-codec", diff --git a/frame/bounties/Cargo.toml b/frame/bounties/Cargo.toml index 3bb184d5b3393..93a7ababb2ebd 100644 --- a/frame/bounties/Cargo.toml +++ b/frame/bounties/Cargo.toml @@ -22,24 +22,27 @@ sp-runtime = { version = "4.0.0-dev", default-features = false, path = "../../pr frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } pallet-treasury = { version = "4.0.0-dev", default-features = false, path = "../treasury" } - +sp-io = { version = "4.0.0-dev", path = "../../primitives/io", default-features = false } +sp-core = { version = "4.0.0-dev", path = "../../primitives/core", default-features = false } frame-benchmarking = { version = "4.0.0-dev", default-features = false, path = "../benchmarking", optional = true } +log = { version = "0.4.14", default-features = false } [dev-dependencies] -sp-io = { version = "4.0.0-dev", path = "../../primitives/io" } -sp-core = { version = "4.0.0-dev", path = "../../primitives/core" } pallet-balances = { version = "4.0.0-dev", path = "../balances" } [features] default = ["std"] std = [ "codec/std", + "sp-core/std", + "sp-io/std", "scale-info/std", "sp-std/std", "sp-runtime/std", "frame-support/std", "frame-system/std", "pallet-treasury/std", + "log/std", ] runtime-benchmarks = [ "frame-benchmarking", diff --git a/frame/bounties/src/benchmarking.rs b/frame/bounties/src/benchmarking.rs index 798d929d241f7..1aa1eabdb5177 100644 --- a/frame/bounties/src/benchmarking.rs +++ b/frame/bounties/src/benchmarking.rs @@ -22,11 +22,10 @@ use super::*; use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller}; -use frame_support::traits::OnInitialize; use frame_system::RawOrigin; use sp_runtime::traits::Bounded; -use crate::Module as Bounties; +use crate::Pallet as Bounties; use pallet_treasury::Pallet as Treasury; const SEED: u32 = 0; @@ -36,10 +35,10 @@ fn create_approved_bounties(n: u32) -> Result<(), &'static str> { for i in 0..n { let (caller, _curator, _fee, value, reason) = setup_bounty::(i, MAX_BYTES); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; Bounties::::approve_bounty(RawOrigin::Root.into(), bounty_id)?; } - ensure!(BountyApprovals::get().len() == n as usize, "Not all bounty approved"); + ensure!(BountyApprovals::::get().len() == n as usize, "Not all bounty approved"); Ok(()) } @@ -64,7 +63,7 @@ fn create_bounty( let (caller, curator, fee, value, reason) = setup_bounty::(0, MAX_BYTES); let curator_lookup = T::Lookup::unlookup(curator.clone()); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; Bounties::::approve_bounty(RawOrigin::Root.into(), bounty_id)?; Treasury::::on_initialize(T::BlockNumber::zero()); Bounties::::propose_curator(RawOrigin::Root.into(), bounty_id, curator_lookup.clone(), fee)?; @@ -94,7 +93,7 @@ benchmarks! { approve_bounty { let (caller, curator, fee, value, reason) = setup_bounty::(0, MAX_BYTES); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; }: _(RawOrigin::Root, bounty_id) propose_curator { @@ -102,7 +101,7 @@ benchmarks! { let (caller, curator, fee, value, reason) = setup_bounty::(0, MAX_BYTES); let curator_lookup = T::Lookup::unlookup(curator.clone()); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; Bounties::::approve_bounty(RawOrigin::Root.into(), bounty_id)?; Bounties::::on_initialize(T::BlockNumber::zero()); }: _(RawOrigin::Root, bounty_id, curator_lookup, fee) @@ -112,7 +111,7 @@ benchmarks! { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; Bounties::::on_initialize(T::BlockNumber::zero()); - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; frame_system::Pallet::::set_block_number(T::BountyUpdatePeriod::get() + 1u32.into()); let caller = whitelisted_caller(); }: _(RawOrigin::Signed(caller), bounty_id) @@ -122,7 +121,7 @@ benchmarks! { let (caller, curator, fee, value, reason) = setup_bounty::(0, MAX_BYTES); let curator_lookup = T::Lookup::unlookup(curator.clone()); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; Bounties::::approve_bounty(RawOrigin::Root.into(), bounty_id)?; Bounties::::on_initialize(T::BlockNumber::zero()); Bounties::::propose_curator(RawOrigin::Root.into(), bounty_id, curator_lookup, fee)?; @@ -133,7 +132,7 @@ benchmarks! { let (curator_lookup, bounty_id) = create_bounty::()?; Bounties::::on_initialize(T::BlockNumber::zero()); - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?; let beneficiary = T::Lookup::unlookup(account("beneficiary", 0, SEED)); @@ -144,10 +143,9 @@ benchmarks! { let (curator_lookup, bounty_id) = create_bounty::()?; Bounties::::on_initialize(T::BlockNumber::zero()); - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?; - let beneficiary_account: T::AccountId = account("beneficiary", 0, SEED); let beneficiary = T::Lookup::unlookup(beneficiary_account.clone()); Bounties::::award_bounty(RawOrigin::Signed(curator.clone()).into(), bounty_id, beneficiary)?; @@ -164,17 +162,17 @@ benchmarks! { setup_pot_account::(); let (caller, curator, fee, value, reason) = setup_bounty::(0, 0); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; }: close_bounty(RawOrigin::Root, bounty_id) close_bounty_active { setup_pot_account::(); let (curator_lookup, bounty_id) = create_bounty::()?; Bounties::::on_initialize(T::BlockNumber::zero()); - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; }: close_bounty(RawOrigin::Root, bounty_id) verify { - assert_last_event::(RawEvent::BountyCanceled(bounty_id).into()) + assert_last_event::(Event::BountyCanceled(bounty_id).into()) } extend_bounty_expiry { @@ -182,11 +180,11 @@ benchmarks! { let (curator_lookup, bounty_id) = create_bounty::()?; Bounties::::on_initialize(T::BlockNumber::zero()); - let bounty_id = BountyCount::get() - 1; + let bounty_id = BountyCount::::get() - 1; let curator = T::Lookup::lookup(curator_lookup).map_err(<&str>::from)?; }: _(RawOrigin::Signed(curator), bounty_id, Vec::new()) verify { - assert_last_event::(RawEvent::BountyExtended(bounty_id).into()) + assert_last_event::(Event::BountyExtended(bounty_id).into()) } spend_funds { @@ -209,7 +207,7 @@ benchmarks! { verify { ensure!(budget_remaining < BalanceOf::::max_value(), "Budget not used"); ensure!(missed_any == false, "Missed some"); - assert_last_event::(RawEvent::BountyBecameActive(b - 1).into()) + assert_last_event::(Event::BountyBecameActive(b - 1).into()) } } diff --git a/frame/bounties/src/lib.rs b/frame/bounties/src/lib.rs index 77a8e47174019..69380502bad3f 100644 --- a/frame/bounties/src/lib.rs +++ b/frame/bounties/src/lib.rs @@ -75,13 +75,12 @@ #![cfg_attr(not(feature = "std"), no_std)] mod benchmarking; +pub mod migrations; mod tests; pub mod weights; use sp_std::prelude::*; -use frame_support::{decl_error, decl_event, decl_module, decl_storage, ensure}; - use frame_support::traits::{ Currency, ExistenceRequirement::AllowDeath, Get, Imbalance, OnUnbalanced, ReservableCurrency, }; @@ -93,46 +92,17 @@ use sp_runtime::{ use frame_support::{dispatch::DispatchResultWithPostInfo, traits::EnsureOrigin}; -use frame_support::weights::Weight; - -use codec::{Decode, Encode}; -use frame_system::{self as system, ensure_signed}; +use frame_support::pallet_prelude::*; +use frame_system::pallet_prelude::*; use scale_info::TypeInfo; pub use weights::WeightInfo; +pub use pallet::*; + type BalanceOf = pallet_treasury::BalanceOf; type PositiveImbalanceOf = pallet_treasury::PositiveImbalanceOf; -pub trait Config: frame_system::Config + pallet_treasury::Config { - /// The amount held on deposit for placing a bounty proposal. - type BountyDepositBase: Get>; - - /// The delay period for which a bounty beneficiary need to wait before claim the payout. - type BountyDepositPayoutDelay: Get; - - /// Bounty duration in blocks. - type BountyUpdatePeriod: Get; - - /// Percentage of the curator fee that will be reserved upfront as deposit for bounty curator. - type BountyCuratorDeposit: Get; - - /// Minimum value for a bounty. - type BountyValueMinimum: Get>; - - /// The amount held on deposit per byte within the tip report reason or bounty description. - type DataDepositPerByte: Get>; - - /// The overarching event type. - type Event: From> + Into<::Event>; - - /// Maximum acceptable reason length. - type MaximumReasonLength: Get; - - /// Weight information for extrinsics in this pallet. - type WeightInfo: WeightInfo; -} - /// An index of a bounty. Just a `u32`. pub type BountyIndex = u32; @@ -186,55 +156,54 @@ pub enum BountyStatus { }, } -// Note :: For backward compatibility reasons, -// pallet-bounties uses Treasury for storage. -// This is temporary solution, soon will get replaced with -// Own storage identifier. -decl_storage! { - trait Store for Module as Treasury { +#[frame_support::pallet] +pub mod pallet { + use super::*; - /// Number of bounty proposals that have been made. - pub BountyCount get(fn bounty_count): BountyIndex; + #[pallet::pallet] + #[pallet::generate_store(pub(super) trait Store)] + pub struct Pallet(_); - /// Bounties that have been made. - pub Bounties get(fn bounties): - map hasher(twox_64_concat) BountyIndex - => Option, T::BlockNumber>>; + #[pallet::config] + pub trait Config: frame_system::Config + pallet_treasury::Config { + /// The amount held on deposit for placing a bounty proposal. + #[pallet::constant] + type BountyDepositBase: Get>; - /// The description of each bounty. - pub BountyDescriptions get(fn bounty_descriptions): map hasher(twox_64_concat) BountyIndex => Option>; + /// The delay period for which a bounty beneficiary need to wait before claim the payout. + #[pallet::constant] + type BountyDepositPayoutDelay: Get; - /// Bounty indices that have been approved but not yet funded. - pub BountyApprovals get(fn bounty_approvals): Vec; - } -} + /// Bounty duration in blocks. + #[pallet::constant] + type BountyUpdatePeriod: Get; -decl_event!( - pub enum Event - where - Balance = BalanceOf, - ::AccountId, - { - /// New bounty proposal. \[index\] - BountyProposed(BountyIndex), - /// A bounty proposal was rejected; funds were slashed. \[index, bond\] - BountyRejected(BountyIndex, Balance), - /// A bounty proposal is funded and became active. \[index\] - BountyBecameActive(BountyIndex), - /// A bounty is awarded to a beneficiary. \[index, beneficiary\] - BountyAwarded(BountyIndex, AccountId), - /// A bounty is claimed by beneficiary. \[index, payout, beneficiary\] - BountyClaimed(BountyIndex, Balance, AccountId), - /// A bounty is cancelled. \[index\] - BountyCanceled(BountyIndex), - /// A bounty expiry is extended. \[index\] - BountyExtended(BountyIndex), + /// Percentage of the curator fee that will be reserved upfront as deposit for bounty + /// curator. + #[pallet::constant] + type BountyCuratorDeposit: Get; + + /// Minimum value for a bounty. + #[pallet::constant] + type BountyValueMinimum: Get>; + + /// The amount held on deposit per byte within the tip report reason or bounty description. + #[pallet::constant] + type DataDepositPerByte: Get>; + + /// The overarching event type. + type Event: From> + IsType<::Event>; + + /// Maximum acceptable reason length. + #[pallet::constant] + type MaximumReasonLength: Get; + + /// Weight information for extrinsics in this pallet. + type WeightInfo: WeightInfo; } -); -decl_error! { - /// Error for the treasury module. - pub enum Error for Module { + #[pallet::error] + pub enum Error { /// Proposer's balance is too low. InsufficientProposersBalance, /// No proposal or bounty at that index. @@ -255,38 +224,53 @@ decl_error! { /// The bounties cannot be claimed/closed because it's still in the countdown period. Premature, } -} - -decl_module! { - pub struct Module - for enum Call - where origin: T::Origin - { - /// The amount held on deposit per byte within bounty description. - const DataDepositPerByte: BalanceOf = T::DataDepositPerByte::get(); - - /// The amount held on deposit for placing a bounty proposal. - const BountyDepositBase: BalanceOf = T::BountyDepositBase::get(); - - /// The delay period for which a bounty beneficiary need to wait before claim the payout. - const BountyDepositPayoutDelay: T::BlockNumber = T::BountyDepositPayoutDelay::get(); - /// Bounty duration in blocks. - const BountyUpdatePeriod: T::BlockNumber = T::BountyUpdatePeriod::get(); - - /// Percentage of the curator fee that will be reserved upfront as deposit for bounty curator. - const BountyCuratorDeposit: Permill = T::BountyCuratorDeposit::get(); - - /// Minimum value for a bounty. - const BountyValueMinimum: BalanceOf = T::BountyValueMinimum::get(); - - /// Maximum acceptable reason length. - const MaximumReasonLength: u32 = T::MaximumReasonLength::get(); - - type Error = Error; - - fn deposit_event() = default; + #[pallet::event] + #[pallet::generate_deposit(pub(super) fn deposit_event)] + pub enum Event { + /// New bounty proposal. \[index\] + BountyProposed(BountyIndex), + /// A bounty proposal was rejected; funds were slashed. \[index, bond\] + BountyRejected(BountyIndex, BalanceOf), + /// A bounty proposal is funded and became active. \[index\] + BountyBecameActive(BountyIndex), + /// A bounty is awarded to a beneficiary. \[index, beneficiary\] + BountyAwarded(BountyIndex, T::AccountId), + /// A bounty is claimed by beneficiary. \[index, payout, beneficiary\] + BountyClaimed(BountyIndex, BalanceOf, T::AccountId), + /// A bounty is cancelled. \[index\] + BountyCanceled(BountyIndex), + /// A bounty expiry is extended. \[index\] + BountyExtended(BountyIndex), + } + /// Number of bounty proposals that have been made. + #[pallet::storage] + #[pallet::getter(fn bounty_count)] + pub type BountyCount = StorageValue<_, BountyIndex, ValueQuery>; + + /// Bounties that have been made. + #[pallet::storage] + #[pallet::getter(fn bounties)] + pub type Bounties = StorageMap< + _, + Twox64Concat, + BountyIndex, + Bounty, T::BlockNumber>, + >; + + /// The description of each bounty. + #[pallet::storage] + #[pallet::getter(fn bounty_descriptions)] + pub type BountyDescriptions = StorageMap<_, Twox64Concat, BountyIndex, Vec>; + + /// Bounty indices that have been approved but not yet funded. + #[pallet::storage] + #[pallet::getter(fn bounty_approvals)] + pub type BountyApprovals = StorageValue<_, Vec, ValueQuery>; + + #[pallet::call] + impl Pallet { /// Propose a new bounty. /// /// The dispatch origin for this call must be _Signed_. @@ -299,14 +283,15 @@ decl_module! { /// - `fee`: The curator fee. /// - `value`: The total payment amount of this bounty, curator fee included. /// - `description`: The description of this bounty. - #[weight = ::WeightInfo::propose_bounty(description.len() as u32)] - fn propose_bounty( - origin, - #[compact] value: BalanceOf, + #[pallet::weight(::WeightInfo::propose_bounty(description.len() as u32))] + pub fn propose_bounty( + origin: OriginFor, + #[pallet::compact] value: BalanceOf, description: Vec, - ) { + ) -> DispatchResult { let proposer = ensure_signed(origin)?; Self::create_bounty(proposer, description, value)?; + Ok(()) } /// Approve a bounty proposal. At a later time, the bounty will be funded and become active @@ -317,8 +302,11 @@ decl_module! { /// # /// - O(1). /// # - #[weight = ::WeightInfo::approve_bounty()] - fn approve_bounty(origin, #[compact] bounty_id: BountyIndex) { + #[pallet::weight(::WeightInfo::approve_bounty())] + pub fn approve_bounty( + origin: OriginFor, + #[pallet::compact] bounty_id: BountyIndex, + ) -> DispatchResult { T::ApproveOrigin::ensure_origin(origin)?; Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { @@ -327,10 +315,11 @@ decl_module! { bounty.status = BountyStatus::Approved; - BountyApprovals::append(bounty_id); + BountyApprovals::::append(bounty_id); Ok(()) })?; + Ok(()) } /// Assign a curator to a funded bounty. @@ -340,18 +329,17 @@ decl_module! { /// # /// - O(1). /// # - #[weight = ::WeightInfo::propose_curator()] - fn propose_curator( - origin, - #[compact] bounty_id: BountyIndex, + #[pallet::weight(::WeightInfo::propose_curator())] + pub fn propose_curator( + origin: OriginFor, + #[pallet::compact] bounty_id: BountyIndex, curator: ::Source, - #[compact] fee: BalanceOf, - ) { + #[pallet::compact] fee: BalanceOf, + ) -> DispatchResult { T::ApproveOrigin::ensure_origin(origin)?; let curator = T::Lookup::lookup(curator)?; Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { - let mut bounty = maybe_bounty.as_mut().ok_or(Error::::InvalidIndex)?; match bounty.status { BountyStatus::Proposed | BountyStatus::Approved | BountyStatus::Funded => {}, @@ -365,14 +353,15 @@ decl_module! { Ok(()) })?; + Ok(()) } /// Unassign curator from a bounty. /// /// 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 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 @@ -385,11 +374,11 @@ decl_module! { /// # /// - O(1). /// # - #[weight = ::WeightInfo::unassign_curator()] - fn unassign_curator( - origin, - #[compact] bounty_id: BountyIndex, - ) { + #[pallet::weight(::WeightInfo::unassign_curator())] + pub fn unassign_curator( + origin: OriginFor, + #[pallet::compact] bounty_id: BountyIndex, + ) -> DispatchResult { let maybe_sender = ensure_signed(origin.clone()) .map(Some) .or_else(|_| T::RejectOrigin::ensure_origin(origin).map(|_| None))?; @@ -407,7 +396,7 @@ decl_module! { 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. @@ -425,10 +414,10 @@ decl_module! { // If the sender is not the curator, and the curator is inactive, // slash the curator. if sender != *curator { - let block_number = system::Pallet::::block_number(); + let block_number = frame_system::Pallet::::block_number(); if *update_due < block_number { slash_curator(curator, &mut bounty.curator_deposit); - // Continue to change bounty status below... + // Continue to change bounty status below... } else { // Curator has more time to give an update. return Err(Error::::Premature.into()) @@ -436,7 +425,8 @@ decl_module! { } else { // Else this is the curator, willingly giving up their role. // Give back their deposit. - let err_amount = T::Currency::unreserve(&curator, bounty.curator_deposit); + let err_amount = + T::Currency::unreserve(&curator, bounty.curator_deposit); debug_assert!(err_amount.is_zero()); // Continue to change bounty status below... } @@ -450,12 +440,13 @@ decl_module! { ensure!(maybe_sender.is_none(), BadOrigin); slash_curator(curator, &mut bounty.curator_deposit); // Continue to change bounty status below... - } + }, }; bounty.status = BountyStatus::Funded; Ok(()) })?; + Ok(()) } /// Accept the curator role for a bounty. @@ -466,8 +457,11 @@ decl_module! { /// # /// - O(1). /// # - #[weight = ::WeightInfo::accept_curator()] - fn accept_curator(origin, #[compact] bounty_id: BountyIndex) { + #[pallet::weight(::WeightInfo::accept_curator())] + pub fn accept_curator( + origin: OriginFor, + #[pallet::compact] bounty_id: BountyIndex, + ) -> DispatchResult { let signer = ensure_signed(origin)?; Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { @@ -481,17 +475,21 @@ decl_module! { T::Currency::reserve(curator, deposit)?; bounty.curator_deposit = deposit; - let update_due = system::Pallet::::block_number() + T::BountyUpdatePeriod::get(); - bounty.status = BountyStatus::Active { curator: curator.clone(), update_due }; + let update_due = frame_system::Pallet::::block_number() + + T::BountyUpdatePeriod::get(); + bounty.status = + BountyStatus::Active { curator: curator.clone(), update_due }; Ok(()) }, _ => Err(Error::::UnexpectedStatus.into()), } })?; + Ok(()) } - /// Award bounty to a beneficiary account. The beneficiary will be able to claim the funds after a delay. + /// Award bounty to a beneficiary account. The beneficiary will be able to claim the funds + /// after a delay. /// /// The dispatch origin for this call must be the curator of this bounty. /// @@ -501,18 +499,19 @@ decl_module! { /// # /// - O(1). /// # - #[weight = ::WeightInfo::award_bounty()] - fn award_bounty(origin, #[compact] bounty_id: BountyIndex, beneficiary: ::Source) { + #[pallet::weight(::WeightInfo::award_bounty())] + pub fn award_bounty( + origin: OriginFor, + #[pallet::compact] bounty_id: BountyIndex, + beneficiary: ::Source, + ) -> DispatchResult { let signer = ensure_signed(origin)?; let beneficiary = T::Lookup::lookup(beneficiary)?; Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { let mut bounty = maybe_bounty.as_mut().ok_or(Error::::InvalidIndex)?; match &bounty.status { - BountyStatus::Active { - curator, - .. - } => { + BountyStatus::Active { curator, .. } => { ensure!(signer == *curator, Error::::RequireCurator); }, _ => return Err(Error::::UnexpectedStatus.into()), @@ -520,13 +519,15 @@ decl_module! { bounty.status = BountyStatus::PendingPayout { curator: signer, beneficiary: beneficiary.clone(), - unlock_at: system::Pallet::::block_number() + T::BountyDepositPayoutDelay::get(), + unlock_at: frame_system::Pallet::::block_number() + + T::BountyDepositPayoutDelay::get(), }; Ok(()) })?; Self::deposit_event(Event::::BountyAwarded(bounty_id, beneficiary)); + Ok(()) } /// Claim the payout from an awarded bounty after payout delay. @@ -538,14 +539,22 @@ decl_module! { /// # /// - O(1). /// # - #[weight = ::WeightInfo::claim_bounty()] - fn claim_bounty(origin, #[compact] bounty_id: BountyIndex) { + #[pallet::weight(::WeightInfo::claim_bounty())] + pub fn claim_bounty( + origin: OriginFor, + #[pallet::compact] bounty_id: BountyIndex, + ) -> DispatchResult { let _ = ensure_signed(origin)?; // anyone can trigger claim Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { let bounty = maybe_bounty.take().ok_or(Error::::InvalidIndex)?; - if let BountyStatus::PendingPayout { curator, beneficiary, unlock_at } = bounty.status { - ensure!(system::Pallet::::block_number() >= unlock_at, Error::::Premature); + if let BountyStatus::PendingPayout { curator, beneficiary, unlock_at } = + bounty.status + { + ensure!( + frame_system::Pallet::::block_number() >= unlock_at, + Error::::Premature + ); let bounty_account = Self::bounty_account_id(bounty_id); let balance = T::Currency::free_balance(&bounty_account); let fee = bounty.fee.min(balance); // just to be safe @@ -554,12 +563,13 @@ decl_module! { debug_assert!(err_amount.is_zero()); let res = T::Currency::transfer(&bounty_account, &curator, fee, AllowDeath); // should not fail debug_assert!(res.is_ok()); - let res = T::Currency::transfer(&bounty_account, &beneficiary, payout, AllowDeath); // should not fail + let res = + T::Currency::transfer(&bounty_account, &beneficiary, payout, AllowDeath); // should not fail debug_assert!(res.is_ok()); *maybe_bounty = None; - BountyDescriptions::remove(bounty_id); + BountyDescriptions::::remove(bounty_id); Self::deposit_event(Event::::BountyClaimed(bounty_id, payout, beneficiary)); Ok(()) @@ -567,6 +577,7 @@ decl_module! { Err(Error::::UnexpectedStatus.into()) } })?; + Ok(()) } /// Cancel a proposed or active bounty. All the funds will be sent to treasury and @@ -579,62 +590,76 @@ decl_module! { /// # /// - O(1). /// # - #[weight = ::WeightInfo::close_bounty_proposed().max(::WeightInfo::close_bounty_active())] - fn close_bounty(origin, #[compact] bounty_id: BountyIndex) -> DispatchResultWithPostInfo { + #[pallet::weight(::WeightInfo::close_bounty_proposed() + .max(::WeightInfo::close_bounty_active()))] + pub fn close_bounty( + origin: OriginFor, + #[pallet::compact] bounty_id: BountyIndex, + ) -> DispatchResultWithPostInfo { T::RejectOrigin::ensure_origin(origin)?; - Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResultWithPostInfo { - let bounty = maybe_bounty.as_ref().ok_or(Error::::InvalidIndex)?; - - match &bounty.status { - 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; - - Self::deposit_event(Event::::BountyRejected(bounty_id, value)); - // Return early, nothing else to do. - return Ok(Some(::WeightInfo::close_bounty_proposed()).into()) - }, - 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()) - }, - 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 err_amount = T::Currency::unreserve(&curator, bounty.curator_deposit); - debug_assert!(err_amount.is_zero()); - // 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()) + Bounties::::try_mutate_exists( + bounty_id, + |maybe_bounty| -> DispatchResultWithPostInfo { + let bounty = maybe_bounty.as_ref().ok_or(Error::::InvalidIndex)?; + + match &bounty.status { + 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; + + Self::deposit_event(Event::::BountyRejected(bounty_id, value)); + // Return early, nothing else to do. + return Ok( + Some(::WeightInfo::close_bounty_proposed()).into() + ) + }, + 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()) + }, + 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 err_amount = + T::Currency::unreserve(&curator, bounty.curator_deposit); + debug_assert!(err_amount.is_zero()); + // 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); + let bounty_account = Self::bounty_account_id(bounty_id); - BountyDescriptions::remove(bounty_id); + BountyDescriptions::::remove(bounty_id); - let balance = T::Currency::free_balance(&bounty_account); - let res = T::Currency::transfer(&bounty_account, &Self::account_id(), balance, AllowDeath); // should not fail - debug_assert!(res.is_ok()); - *maybe_bounty = None; + let balance = T::Currency::free_balance(&bounty_account); + let res = T::Currency::transfer( + &bounty_account, + &Self::account_id(), + balance, + AllowDeath, + ); // should not fail + debug_assert!(res.is_ok()); + *maybe_bounty = None; - Self::deposit_event(Event::::BountyCanceled(bounty_id)); - Ok(Some(::WeightInfo::close_bounty_active()).into()) - }) + Self::deposit_event(Event::::BountyCanceled(bounty_id)); + Ok(Some(::WeightInfo::close_bounty_active()).into()) + }, + ) } /// Extend the expiry time of an active bounty. @@ -647,8 +672,12 @@ decl_module! { /// # /// - O(1). /// # - #[weight = ::WeightInfo::extend_bounty_expiry()] - fn extend_bounty_expiry(origin, #[compact] bounty_id: BountyIndex, _remark: Vec) { + #[pallet::weight(::WeightInfo::extend_bounty_expiry())] + pub fn extend_bounty_expiry( + origin: OriginFor, + #[pallet::compact] bounty_id: BountyIndex, + _remark: Vec, + ) -> DispatchResult { let signer = ensure_signed(origin)?; Bounties::::try_mutate_exists(bounty_id, |maybe_bounty| -> DispatchResult { @@ -657,7 +686,9 @@ decl_module! { match bounty.status { BountyStatus::Active { ref curator, ref mut update_due } => { ensure!(*curator == signer, Error::::RequireCurator); - *update_due = (system::Pallet::::block_number() + T::BountyUpdatePeriod::get()).max(*update_due); + *update_due = (frame_system::Pallet::::block_number() + + T::BountyUpdatePeriod::get()) + .max(*update_due); }, _ => return Err(Error::::UnexpectedStatus.into()), } @@ -666,11 +697,12 @@ decl_module! { })?; Self::deposit_event(Event::::BountyExtended(bounty_id)); + Ok(()) } } } -impl Module { +impl Pallet { // Add public immutables and private mutables. /// The account ID of the treasury pot. @@ -707,7 +739,7 @@ impl Module { T::Currency::reserve(&proposer, bond) .map_err(|_| Error::::InsufficientProposersBalance)?; - BountyCount::put(index + 1); + BountyCount::::put(index + 1); let bounty = Bounty { proposer, @@ -719,22 +751,22 @@ impl Module { }; Bounties::::insert(index, &bounty); - BountyDescriptions::insert(index, description); + BountyDescriptions::::insert(index, description); - Self::deposit_event(RawEvent::BountyProposed(index)); + Self::deposit_event(Event::::BountyProposed(index)); Ok(()) } } -impl pallet_treasury::SpendFunds for Module { +impl pallet_treasury::SpendFunds for Pallet { fn spend_funds( budget_remaining: &mut BalanceOf, imbalance: &mut PositiveImbalanceOf, total_weight: &mut Weight, missed_any: &mut bool, ) { - let bounties_len = BountyApprovals::mutate(|v| { + let bounties_len = BountyApprovals::::mutate(|v| { let bounties_approval_len = v.len() as u32; v.retain(|&index| { Bounties::::mutate(index, |bounty| { @@ -755,7 +787,7 @@ impl pallet_treasury::SpendFunds for Module { bounty.value, )); - Self::deposit_event(RawEvent::BountyBecameActive(index)); + Self::deposit_event(Event::::BountyBecameActive(index)); false } else { *missed_any = true; diff --git a/frame/bounties/src/migrations/mod.rs b/frame/bounties/src/migrations/mod.rs new file mode 100644 index 0000000000000..26d07a0cd5ac8 --- /dev/null +++ b/frame/bounties/src/migrations/mod.rs @@ -0,0 +1,19 @@ +// This file is part of Substrate. + +// Copyright (C) 2019-2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/// Version 4. +pub mod v4; diff --git a/frame/bounties/src/migrations/v4.rs b/frame/bounties/src/migrations/v4.rs new file mode 100644 index 0000000000000..a1ca0e47680b0 --- /dev/null +++ b/frame/bounties/src/migrations/v4.rs @@ -0,0 +1,230 @@ +// This file is part of Substrate. + +// Copyright (C) 2019-2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use frame_support::{ + storage::{generator::StorageValue, StoragePrefixedMap}, + traits::{ + Get, GetStorageVersion, PalletInfoAccess, StorageVersion, + STORAGE_VERSION_STORAGE_KEY_POSTFIX, + }, + weights::Weight, +}; +use sp_core::hexdisplay::HexDisplay; +use sp_io::{hashing::twox_128, storage}; +use sp_std::str; + +use crate as pallet_bounties; + +/// Migrate the storage of the bounties pallet to a new prefix, leaving all other storage untouched +/// +/// This new prefix must be the same as the one set in construct_runtime. For safety, use +/// `PalletInfo` to get it, as: +/// `::PalletInfo::name::`. +/// +/// The migration will look into the storage version in order not to trigger a migration on an up +/// to date storage. Thus the on chain storage version must be less than 4 in order to trigger the +/// migration. +pub fn migrate< + T: pallet_bounties::Config, + P: GetStorageVersion + PalletInfoAccess, + N: AsRef, +>( + old_pallet_name: N, + new_pallet_name: N, +) -> Weight { + let old_pallet_name = old_pallet_name.as_ref(); + let new_pallet_name = new_pallet_name.as_ref(); + + if new_pallet_name == old_pallet_name { + log::info!( + target: "runtime::bounties", + "New pallet name is equal to the old prefix. No migration needs to be done.", + ); + return 0 + } + + let on_chain_storage_version =

::on_chain_storage_version(); + log::info!( + target: "runtime::bounties", + "Running migration to v4 for bounties with storage version {:?}", + on_chain_storage_version, + ); + + if on_chain_storage_version < 4 { + let storage_prefix = pallet_bounties::BountyCount::::storage_prefix(); + frame_support::storage::migration::move_storage_from_pallet( + storage_prefix, + old_pallet_name.as_bytes(), + new_pallet_name.as_bytes(), + ); + log_migration("migration", storage_prefix, old_pallet_name, new_pallet_name); + + let storage_prefix = pallet_bounties::Bounties::::storage_prefix(); + frame_support::storage::migration::move_storage_from_pallet( + storage_prefix, + old_pallet_name.as_bytes(), + new_pallet_name.as_bytes(), + ); + log_migration("migration", storage_prefix, old_pallet_name, new_pallet_name); + + let storage_prefix = pallet_bounties::BountyDescriptions::::storage_prefix(); + frame_support::storage::migration::move_storage_from_pallet( + storage_prefix, + old_pallet_name.as_bytes(), + new_pallet_name.as_bytes(), + ); + log_migration("migration", storage_prefix, old_pallet_name, new_pallet_name); + + let storage_prefix = pallet_bounties::BountyApprovals::::storage_prefix(); + frame_support::storage::migration::move_storage_from_pallet( + storage_prefix, + old_pallet_name.as_bytes(), + new_pallet_name.as_bytes(), + ); + log_migration("migration", storage_prefix, old_pallet_name, new_pallet_name); + + StorageVersion::new(4).put::

(); + ::BlockWeights::get().max_block + } else { + log::warn!( + target: "runtime::bounties", + "Attempted to apply migration to v4 but failed because storage version is {:?}", + on_chain_storage_version, + ); + 0 + } +} + +/// Some checks prior to migration. This can be linked to +/// [`frame_support::traits::OnRuntimeUpgrade::pre_upgrade`] for further testing. +/// +/// Panics if anything goes wrong. +pub fn pre_migration>( + old_pallet_name: N, + new_pallet_name: N, +) { + let old_pallet_name = old_pallet_name.as_ref(); + let new_pallet_name = new_pallet_name.as_ref(); + let storage_prefix_bounties_count = pallet_bounties::BountyCount::::storage_prefix(); + let storage_prefix_bounties = pallet_bounties::Bounties::::storage_prefix(); + let storage_prefix_bounties_description = + pallet_bounties::BountyDescriptions::::storage_prefix(); + let storage_prefix_bounties_approvals = pallet_bounties::BountyApprovals::::storage_prefix(); + log_migration("pre-migration", storage_prefix_bounties_count, old_pallet_name, new_pallet_name); + log_migration("pre-migration", storage_prefix_bounties, old_pallet_name, new_pallet_name); + log_migration( + "pre-migration", + storage_prefix_bounties_description, + old_pallet_name, + new_pallet_name, + ); + log_migration( + "pre-migration", + storage_prefix_bounties_approvals, + old_pallet_name, + new_pallet_name, + ); + + let new_pallet_prefix = twox_128(new_pallet_name.as_bytes()); + let storage_version_key = + [&new_pallet_prefix, &twox_128(STORAGE_VERSION_STORAGE_KEY_POSTFIX)[..]].concat(); + + // ensure nothing is stored in the new prefix. + assert!( + storage::next_key(&new_pallet_prefix).map_or( + // either nothing is there + true, + // or we ensure that the next key has no common prefix with twox_128(new), + // or is the pallet version that is already stored using the pallet name + |next_key| { + storage::next_key(&next_key).map_or(true, |next_key| { + !next_key.starts_with(&new_pallet_prefix) || next_key == storage_version_key + }) + }, + ), + "unexpected next_key({}) = {:?}", + new_pallet_name, + HexDisplay::from(&sp_io::storage::next_key(&new_pallet_prefix).unwrap()), + ); + assert!(

::on_chain_storage_version() < 4); +} + +/// Some checks for after migration. This can be linked to +/// [`frame_support::traits::OnRuntimeUpgrade::post_upgrade`] for further testing. +/// +/// Panics if anything goes wrong. +pub fn post_migration>( + old_pallet_name: N, + new_pallet_name: N, +) { + let old_pallet_name = old_pallet_name.as_ref(); + let new_pallet_name = new_pallet_name.as_ref(); + let storage_prefix_bounties_count = pallet_bounties::BountyCount::::storage_prefix(); + let storage_prefix_bounties = pallet_bounties::Bounties::::storage_prefix(); + let storage_prefix_bounties_description = + pallet_bounties::BountyDescriptions::::storage_prefix(); + let storage_prefix_bounties_approvals = pallet_bounties::BountyApprovals::::storage_prefix(); + log_migration( + "post-migration", + storage_prefix_bounties_count, + old_pallet_name, + new_pallet_name, + ); + log_migration("post-migration", storage_prefix_bounties, old_pallet_name, new_pallet_name); + log_migration( + "post-migration", + storage_prefix_bounties_description, + old_pallet_name, + new_pallet_name, + ); + log_migration( + "post-migration", + storage_prefix_bounties_approvals, + old_pallet_name, + new_pallet_name, + ); + + let old_pallet_prefix = twox_128(old_pallet_name.as_bytes()); + let old_bounties_count_key = + [&old_pallet_prefix, &twox_128(storage_prefix_bounties_count)[..]].concat(); + let old_bounties_key = [&old_pallet_prefix, &twox_128(storage_prefix_bounties)[..]].concat(); + let old_bounties_description_key = + [&old_pallet_prefix, &twox_128(storage_prefix_bounties_description)[..]].concat(); + let old_bounties_approvals_key = + [&old_pallet_prefix, &twox_128(storage_prefix_bounties_approvals)[..]].concat(); + assert!(storage::next_key(&old_bounties_count_key) + .map_or(true, |next_key| !next_key.starts_with(&old_bounties_count_key))); + assert!(storage::next_key(&old_bounties_key) + .map_or(true, |next_key| !next_key.starts_with(&old_bounties_key))); + assert!(storage::next_key(&old_bounties_description_key) + .map_or(true, |next_key| !next_key.starts_with(&old_bounties_description_key))); + assert!(storage::next_key(&old_bounties_approvals_key) + .map_or(true, |next_key| !next_key.starts_with(&old_bounties_approvals_key))); + + assert_eq!(

::on_chain_storage_version(), 4); +} + +fn log_migration(stage: &str, storage_prefix: &[u8], old_pallet_name: &str, new_pallet_name: &str) { + log::info!( + target: "runtime::bounties", + "{} prefix of storage '{}': '{}' ==> '{}'", + stage, + str::from_utf8(storage_prefix).unwrap_or(""), + old_pallet_name, + new_pallet_name, + ); +} diff --git a/frame/bounties/src/tests.rs b/frame/bounties/src/tests.rs index ff058a3601e07..96c09581fdd1e 100644 --- a/frame/bounties/src/tests.rs +++ b/frame/bounties/src/tests.rs @@ -32,9 +32,11 @@ use sp_core::H256; use sp_runtime::{ testing::Header, traits::{BadOrigin, BlakeTwo256, IdentityLookup}, - Perbill, + Perbill, Storage, }; +use super::Event as BountiesEvent; + type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; @@ -160,7 +162,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { t.into() } -fn last_event() -> RawEvent { +fn last_event() -> BountiesEvent { System::events() .into_iter() .map(|r| r.event) @@ -396,7 +398,7 @@ fn propose_bounty_works() { assert_ok!(Bounties::propose_bounty(Origin::signed(0), 10, b"1234567890".to_vec())); - assert_eq!(last_event(), RawEvent::BountyProposed(0)); + assert_eq!(last_event(), BountiesEvent::BountyProposed(0)); let deposit: u64 = 85 + 5; assert_eq!(Balances::reserved_balance(0), deposit); @@ -458,7 +460,7 @@ fn close_bounty_works() { let deposit: u64 = 80 + 5; - assert_eq!(last_event(), RawEvent::BountyRejected(0, deposit)); + assert_eq!(last_event(), BountiesEvent::BountyRejected(0, deposit)); assert_eq!(Balances::reserved_balance(0), 0); assert_eq!(Balances::free_balance(0), 100 - deposit); @@ -690,7 +692,7 @@ fn award_and_claim_bounty_works() { assert_ok!(Bounties::claim_bounty(Origin::signed(1), 0)); - assert_eq!(last_event(), RawEvent::BountyClaimed(0, 56, 3)); + assert_eq!(last_event(), BountiesEvent::BountyClaimed(0, 56, 3)); assert_eq!(Balances::free_balance(4), 14); // initial 10 + fee 4 @@ -729,7 +731,7 @@ fn claim_handles_high_fee() { assert_ok!(Bounties::claim_bounty(Origin::signed(1), 0)); - assert_eq!(last_event(), RawEvent::BountyClaimed(0, 0, 3)); + assert_eq!(last_event(), BountiesEvent::BountyClaimed(0, 0, 3)); assert_eq!(Balances::free_balance(4), 70); // 30 + 50 - 10 assert_eq!(Balances::free_balance(3), 0); @@ -806,7 +808,7 @@ fn award_and_cancel() { assert_ok!(Bounties::unassign_curator(Origin::root(), 0)); assert_ok!(Bounties::close_bounty(Origin::root(), 0)); - assert_eq!(last_event(), RawEvent::BountyCanceled(0)); + assert_eq!(last_event(), BountiesEvent::BountyCanceled(0)); assert_eq!(Balances::free_balance(Bounties::bounty_account_id(0)), 0); @@ -934,6 +936,48 @@ fn extend_expiry() { }); } +#[test] +fn test_migration_v4() { + let mut s = Storage::default(); + + let index: u32 = 10; + + let bounty = Bounty:: { + proposer: 0, + value: 20, + fee: 20, + curator_deposit: 20, + bond: 50, + status: BountyStatus::::Proposed, + }; + + let data = vec![ + (pallet_bounties::BountyCount::::hashed_key().to_vec(), 10.encode().to_vec()), + (pallet_bounties::Bounties::::hashed_key_for(index), bounty.encode().to_vec()), + (pallet_bounties::BountyDescriptions::::hashed_key_for(index), vec![0, 0]), + ( + pallet_bounties::BountyApprovals::::hashed_key().to_vec(), + vec![10 as u32].encode().to_vec(), + ), + ]; + + s.top = data.into_iter().collect(); + + sp_io::TestExternalities::new(s).execute_with(|| { + use frame_support::traits::PalletInfo; + let old_pallet_name = ::PalletInfo::name::() + .expect("Bounties is part of runtime, so it has a name; qed"); + let new_pallet_name = "NewBounties"; + + crate::migrations::v4::pre_migration::(old_pallet_name, new_pallet_name); + crate::migrations::v4::migrate::(old_pallet_name, new_pallet_name); + crate::migrations::v4::post_migration::( + old_pallet_name, + new_pallet_name, + ); + }); +} + #[test] fn genesis_funding_works() { let mut t = frame_system::GenesisConfig::default().build_storage::().unwrap();