diff --git a/frame/sudo/src/mock.rs b/frame/sudo/src/mock.rs index 2e2a4abafcd98..5eaf7eb814e3a 100644 --- a/frame/sudo/src/mock.rs +++ b/frame/sudo/src/mock.rs @@ -44,7 +44,6 @@ pub mod logger { #[pallet::pallet] #[pallet::generate_store(pub(super) trait Store)] - #[pallet::without_storage_info] pub struct Pallet(PhantomData); #[pallet::call] @@ -57,7 +56,7 @@ pub mod logger { ) -> DispatchResultWithPostInfo { // Ensure that the `origin` is `Root`. ensure_root(origin)?; - >::append(i); + >::try_append(i).map_err(|_| "couldn't append")?; Self::deposit_event(Event::AppendI32 { value: i, weight }); Ok(().into()) } @@ -70,8 +69,8 @@ pub mod logger { ) -> DispatchResultWithPostInfo { // Ensure that the `origin` is some signed account. let sender = ensure_signed(origin)?; - >::append(i); - >::append(sender.clone()); + >::try_append(i).map_err(|_| "couldn't append")?; + >::try_append(sender.clone()).map_err(|_| "couldn't append")?; Self::deposit_event(Event::AppendI32AndAccount { sender, value: i, weight }); Ok(().into()) } @@ -86,11 +85,12 @@ pub mod logger { #[pallet::storage] #[pallet::getter(fn account_log)] - pub(super) type AccountLog = StorageValue<_, Vec, ValueQuery>; + pub(super) type AccountLog = + StorageValue<_, BoundedVec>, ValueQuery>; #[pallet::storage] #[pallet::getter(fn i32_log)] - pub(super) type I32Log = StorageValue<_, Vec, ValueQuery>; + pub(super) type I32Log = StorageValue<_, BoundedVec>, ValueQuery>; } type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index db8c8da4f869b..4635b16167931 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -35,8 +35,8 @@ mod members; pub use members::{AllowAll, DenyAll, Filter}; pub use members::{ AsContains, ChangeMembers, Contains, ContainsLengthBound, ContainsPair, Everything, - EverythingBut, FromContainsPair, InitializeMembers, InsideBoth, IsInVec, Nothing, - SortedMembers, TheseExcept, + EverythingBut, FromContainsPair, InitializeMembers, InsideBoth, IsInVec, LengthBoundMaximum, + LengthBoundMinimum, Nothing, SortedMembers, TheseExcept, }; mod validation; diff --git a/frame/support/src/traits/members.rs b/frame/support/src/traits/members.rs index 8c69a2aaccb33..9c667b0aa676b 100644 --- a/frame/support/src/traits/members.rs +++ b/frame/support/src/traits/members.rs @@ -17,6 +17,7 @@ //! Traits for dealing with the idea of membership. +use sp_runtime::traits::Get; use sp_std::{marker::PhantomData, prelude::*}; /// A trait for querying whether a type can be said to "contain" a value. @@ -260,6 +261,24 @@ pub trait ContainsLengthBound { fn max_len() -> usize; } +/// A wrapper struct to convert an object which implements `ContainsLengthBound`, and returns +/// the maximum length as a `Get`. Useful for implementing other bounding conditions. +pub struct LengthBoundMaximum(PhantomData); +impl Get for LengthBoundMaximum { + fn get() -> u32 { + T::max_len() as u32 + } +} + +/// A wrapper struct to convert an object which implements `ContainsLengthBound`, and returns +/// the minimum length as a `Get`. Useful for implementing other bounding conditions. +pub struct LengthBoundMinimum(PhantomData); +impl Get for LengthBoundMinimum { + fn get() -> u32 { + T::min_len() as u32 + } +} + /// Trait for type that can handle the initialization of account IDs at genesis. pub trait InitializeMembers { /// Initialize the members to the given `members`. diff --git a/frame/tips/src/lib.rs b/frame/tips/src/lib.rs index 71af87b42b55b..0f86d4a76c44e 100644 --- a/frame/tips/src/lib.rs +++ b/frame/tips/src/lib.rs @@ -66,13 +66,13 @@ use sp_runtime::{ }; use sp_std::prelude::*; -use codec::{Decode, Encode}; +use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ traits::{ ContainsLengthBound, Currency, EnsureOrigin, ExistenceRequirement::KeepAlive, Get, - OnUnbalanced, ReservableCurrency, SortedMembers, + LengthBoundMaximum, OnUnbalanced, ReservableCurrency, SortedMembers, }, - Parameter, + BoundedVec, Parameter, }; pub use pallet::*; @@ -83,12 +83,17 @@ pub type NegativeImbalanceOf = pallet_treasury::NegativeImbalanceOf { /// The hash of the reason for the tip. The reason should be a human-readable UTF-8 encoded /// string. A URL would be sensible. @@ -103,7 +108,7 @@ pub struct OpenTip< /// scheduled. closes: Option, /// The members who have voted for this tip. Sorted by AccountId. - tips: Vec<(AccountId, Balance)>, + tips: BoundedVec<(AccountId, Balance), LengthBoundMaximum>, /// Whether this tip should result in the finder taking a fee. finders_fee: bool, } @@ -120,7 +125,6 @@ pub mod pallet { #[pallet::pallet] #[pallet::generate_store(pub(super) trait Store)] #[pallet::storage_version(STORAGE_VERSION)] - #[pallet::without_storage_info] pub struct Pallet(_); #[pallet::config] @@ -170,7 +174,7 @@ pub mod pallet { _, Twox64Concat, T::Hash, - OpenTip, T::BlockNumber, T::Hash>, + OpenTip, T::BlockNumber, T::Hash, T::Tippers>, OptionQuery, >; @@ -179,7 +183,7 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn reasons)] pub type Reasons, I: 'static = ()> = - StorageMap<_, Identity, T::Hash, Vec, OptionQuery>; + StorageMap<_, Identity, T::Hash, BoundedVec, OptionQuery>; #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] @@ -210,6 +214,8 @@ pub mod pallet { StillOpen, /// The tip cannot be claimed/closed because it's still in the countdown period. Premature, + /// Too many tips are being stored. + TipsTooBig, } #[pallet::call] @@ -241,10 +247,9 @@ pub mod pallet { ) -> DispatchResult { let finder = ensure_signed(origin)?; - ensure!( - reason.len() <= T::MaximumReasonLength::get() as usize, - Error::::ReasonTooBig - ); + // make reason bounded + let reason: BoundedVec = + reason.try_into().map_err(|_| Error::::ReasonTooBig)?; let reason_hash = T::Hashing::hash(&reason[..]); ensure!(!Reasons::::contains_key(&reason_hash), Error::::AlreadyKnown); @@ -262,7 +267,7 @@ pub mod pallet { finder, deposit, closes: None, - tips: vec![], + tips: BoundedVec::default(), finders_fee: true, }; Tips::::insert(&hash, tip); @@ -334,6 +339,10 @@ pub mod pallet { who: T::AccountId, #[pallet::compact] tip_value: BalanceOf, ) -> DispatchResult { + // make reason bounded + let reason: BoundedVec = + reason.try_into().map_err(|_| Error::::ReasonTooBig)?; + let tipper = ensure_signed(origin)?; ensure!(T::Tippers::contains(&tipper), BadOrigin); let reason_hash = T::Hashing::hash(&reason[..]); @@ -342,7 +351,9 @@ pub mod pallet { Reasons::::insert(&reason_hash, &reason); Self::deposit_event(Event::NewTip { tip_hash: hash }); - let tips = vec![(tipper.clone(), tip_value)]; + let tips = vec![(tipper.clone(), tip_value)] + .try_into() + .map_err(|_| Error::::TipsTooBig)?; let tip = OpenTip { reason: reason_hash, who, @@ -390,7 +401,7 @@ pub mod pallet { ensure!(T::Tippers::contains(&tipper), BadOrigin); let mut tip = Tips::::get(hash).ok_or(Error::::UnknownTip)?; - if Self::insert_tip_and_check_closing(&mut tip, tipper, tip_value) { + if Self::insert_tip_and_check_closing(&mut tip, tipper, tip_value)? { Self::deposit_event(Event::TipClosing { tip_hash: hash }); } Tips::::insert(&hash, tip); @@ -476,26 +487,31 @@ impl, I: 'static> Pallet { /// /// `O(T)` and one storage access. fn insert_tip_and_check_closing( - tip: &mut OpenTip, T::BlockNumber, T::Hash>, + tip: &mut OpenTip, T::BlockNumber, T::Hash, T::Tippers>, tipper: T::AccountId, tip_value: BalanceOf, - ) -> bool { + ) -> Result { match tip.tips.binary_search_by_key(&&tipper, |x| &x.0) { Ok(pos) => tip.tips[pos] = (tipper, tip_value), - Err(pos) => tip.tips.insert(pos, (tipper, tip_value)), + Err(pos) => tip + .tips + .try_insert(pos, (tipper, tip_value)) + .map_err(|_| Error::::TipsTooBig)?, } Self::retain_active_tips(&mut tip.tips); let threshold = (T::Tippers::count() + 1) / 2; if tip.tips.len() >= threshold && tip.closes.is_none() { tip.closes = Some(frame_system::Pallet::::block_number() + T::TipCountdown::get()); - true + Ok(true) } else { - false + Ok(false) } } /// Remove any non-members of `Tippers` from a `tips` vector. `O(T)`. - fn retain_active_tips(tips: &mut Vec<(T::AccountId, BalanceOf)>) { + fn retain_active_tips( + tips: &mut BoundedVec<(T::AccountId, BalanceOf), LengthBoundMaximum>, + ) { let members = T::Tippers::sorted_members(); let mut members_iter = members.iter(); let mut member = members_iter.next(); @@ -521,7 +537,7 @@ impl, I: 'static> Pallet { /// Plus `O(T)` (`T` is Tippers length). fn payout_tip( hash: T::Hash, - tip: OpenTip, T::BlockNumber, T::Hash>, + tip: OpenTip, T::BlockNumber, T::Hash, T::Tippers>, ) { let mut tips = tip.tips; Self::retain_active_tips(&mut tips); @@ -552,7 +568,7 @@ impl, I: 'static> Pallet { Self::deposit_event(Event::TipClosed { tip_hash: hash, who: tip.who, payout }); } - pub fn migrate_retract_tip_for_tip_new(module: &[u8], item: &[u8]) { + pub fn migrate_retract_tip_for_tip_new(module: &[u8], item: &[u8]) -> Result<(), ()> { /// An open tipping "motion". Retains all details of a tip including information on the /// finder and the members who have voted. #[derive(Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug)] @@ -598,10 +614,12 @@ impl, I: 'static> Pallet { finder, deposit, closes: old_tip.closes, - tips: old_tip.tips, + tips: BoundedVec::try_from(old_tip.tips).map_err(|_| ())?, finders_fee, }; Tips::::insert(hash, new_tip) } + + Ok(()) } } diff --git a/frame/tips/src/tests.rs b/frame/tips/src/tests.rs index 194ecc60d9890..bd789146b2475 100644 --- a/frame/tips/src/tests.rs +++ b/frame/tips/src/tests.rs @@ -30,7 +30,7 @@ use sp_runtime::{ use sp_storage::Storage; use frame_support::{ - assert_noop, assert_ok, + assert_noop, assert_ok, bounded_vec, pallet_prelude::GenesisBuild, parameter_types, storage::StoragePrefixedMap, @@ -103,6 +103,7 @@ impl pallet_balances::Config for Test { thread_local! { static TEN_TO_FOURTEEN: RefCell> = RefCell::new(vec![10,11,12,13,14]); } +#[derive(PartialEq, Debug)] pub struct TenToFourteen; impl SortedMembers for TenToFourteen { fn sorted_members() -> Vec { @@ -470,7 +471,7 @@ fn test_last_reward_migration() { sp_io::TestExternalities::new(s).execute_with(|| { let module = pallet_tips::Tips::::module_prefix(); let item = pallet_tips::Tips::::storage_prefix(); - Tips::migrate_retract_tip_for_tip_new(module, item); + Tips::migrate_retract_tip_for_tip_new(module, item).unwrap(); // Test w/ finder assert_eq!( @@ -481,7 +482,7 @@ fn test_last_reward_migration() { finder: 20, deposit: 30, closes: Some(13), - tips: vec![(40, 50), (60, 70)], + tips: bounded_vec![(40, 50), (60, 70)], finders_fee: true, }) ); @@ -495,7 +496,7 @@ fn test_last_reward_migration() { finder: Default::default(), deposit: 0, closes: Some(13), - tips: vec![(40, 50), (60, 70)], + tips: bounded_vec![(40, 50), (60, 70)], finders_fee: false, }) ); @@ -507,13 +508,13 @@ fn test_migration_v4() { let reason1 = BlakeTwo256::hash(b"reason1"); let hash1 = BlakeTwo256::hash_of(&(reason1, 10u64)); - let tip = OpenTip:: { + let tip = OpenTip:: { reason: reason1, who: 10, finder: 20, deposit: 30, closes: Some(13), - tips: vec![(40, 50), (60, 70)], + tips: bounded_vec![(40, 50), (60, 70)], finders_fee: true, }; diff --git a/primitives/runtime/src/bounded/bounded_vec.rs b/primitives/runtime/src/bounded/bounded_vec.rs index c9c9f851d3249..183296e52434f 100644 --- a/primitives/runtime/src/bounded/bounded_vec.rs +++ b/primitives/runtime/src/bounded/bounded_vec.rs @@ -273,6 +273,17 @@ impl BoundedVec { self.0 } + /// Exactly the same semantics as [`slice::sort_by_key`]. + /// + /// This is safe since sorting cannot change the number of elements in the vector. + pub fn sort_by_key(&mut self, f: F) + where + F: FnMut(&T) -> K, + K: Ord, + { + self.0.sort_by_key(f) + } + /// Exactly the same semantics as [`slice::sort_by`]. /// /// This is safe since sorting cannot change the number of elements in the vector.