diff --git a/Cargo.lock b/Cargo.lock index 65578627f9..3dbcfe75a4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6120,6 +6120,7 @@ dependencies = [ "frame-benchmarking", "frame-support", "frame-system", + "log", "pallet-balances", "pallet-preimage", "parity-scale-codec", diff --git a/pallets/crowdloan/Cargo.toml b/pallets/crowdloan/Cargo.toml index 1739a85b7c..e8d582fa44 100644 --- a/pallets/crowdloan/Cargo.toml +++ b/pallets/crowdloan/Cargo.toml @@ -21,6 +21,7 @@ frame-support.workspace = true frame-system.workspace = true sp-runtime.workspace = true sp-std.workspace = true +log = { workspace = true } [dev-dependencies] pallet-balances = { default-features = true, workspace = true } @@ -39,6 +40,7 @@ std = [ "sp-runtime/std", "sp-std/std", "sp-io/std", + "log/std", "sp-core/std", "pallet-balances/std", "pallet-preimage/std", diff --git a/pallets/crowdloan/src/benchmarking.rs b/pallets/crowdloan/src/benchmarking.rs index e36f792261..0891baf5af 100644 --- a/pallets/crowdloan/src/benchmarking.rs +++ b/pallets/crowdloan/src/benchmarking.rs @@ -68,6 +68,7 @@ mod benchmarks { target_address: Some(target_address.clone()), call: Some(T::Preimages::bound(*call).unwrap()), finalized: false, + contributors_count: 1, }) ); // ensure the creator has been deducted the deposit diff --git a/pallets/crowdloan/src/lib.rs b/pallets/crowdloan/src/lib.rs index 9b49d5e778..1d4ed4e263 100644 --- a/pallets/crowdloan/src/lib.rs +++ b/pallets/crowdloan/src/lib.rs @@ -7,7 +7,7 @@ extern crate alloc; -use alloc::{boxed::Box, vec, vec::Vec}; +use alloc::{boxed::Box, vec}; use codec::{Decode, Encode}; use frame_support::{ PalletId, @@ -25,6 +25,7 @@ use frame_support::{ use frame_system::pallet_prelude::*; use scale_info::TypeInfo; use sp_runtime::traits::CheckedSub; +use sp_std::vec::Vec; use weights::WeightInfo; pub use pallet::*; @@ -33,6 +34,7 @@ use subtensor_macros::freeze_struct; pub type CrowdloanId = u32; mod benchmarking; +mod migrations; mod mock; mod tests; pub mod weights; @@ -42,11 +44,14 @@ pub type CurrencyOf = ::Currency; pub type BalanceOf = as fungible::Inspect<::AccountId>>::Balance; +// Define a maximum length for the migration key +type MigrationKeyMaxLen = ConstU32<128>; + pub type BoundedCallOf = Bounded<::RuntimeCall, ::Hashing>; /// A struct containing the information about a crowdloan. -#[freeze_struct("6b86ccf70fc1b8f1")] +#[freeze_struct("5db9538284491545")] #[derive(Encode, Decode, Eq, PartialEq, Ord, PartialOrd, RuntimeDebug, TypeInfo, MaxEncodedLen)] pub struct CrowdloanInfo { /// The creator of the crowdloan. @@ -71,6 +76,8 @@ pub struct CrowdloanInfo { pub call: Option, /// Whether the crowdloan has been finalized. pub finalized: bool, + /// The number of contributors to the crowdloan. + pub contributors_count: u32, } pub type CrowdloanInfoOf = CrowdloanInfo< @@ -134,6 +141,10 @@ pub mod pallet { /// The maximum number of contributors that can be refunded in a single refund. #[pallet::constant] type RefundContributorsLimit: Get; + + // The maximum number of contributors that can contribute to a crowdloan. + #[pallet::constant] + type MaxContributors: Get; } /// A map of crowdloan ids to their information. @@ -162,6 +173,11 @@ pub mod pallet { #[pallet::storage] pub type CurrentCrowdloanId = StorageValue<_, CrowdloanId, OptionQuery>; + /// Storage for the migration run status. + #[pallet::storage] + pub type HasMigrationRun = + StorageMap<_, Identity, BoundedVec, bool, ValueQuery>; + #[pallet::event] #[pallet::generate_deposit(pub(super) fn deposit_event)] pub enum Event { @@ -253,6 +269,21 @@ pub mod pallet { NotReadyToDissolve, /// The deposit cannot be withdrawn from the crowdloan. DepositCannotBeWithdrawn, + /// The maximum number of contributors has been reached. + MaxContributorsReached, + } + + #[pallet::hooks] + impl Hooks> for Pallet { + fn on_runtime_upgrade() -> frame_support::weights::Weight { + let mut weight = frame_support::weights::Weight::from_parts(0, 0); + + weight = weight + // Add the contributors count for each crowdloan + .saturating_add(migrations::migrate_add_contributors_count::()); + + weight + } } #[pallet::call] @@ -342,6 +373,7 @@ pub mod pallet { target_address, call, finalized: false, + contributors_count: 1, }; Crowdloans::::insert(crowdloan_id, &crowdloan); @@ -398,6 +430,12 @@ pub mod pallet { Error::::ContributionTooLow ); + // Ensure the crowdloan has not reached the maximum number of contributors + ensure!( + crowdloan.contributors_count < T::MaxContributors::get(), + Error::::MaxContributorsReached + ); + // Ensure contribution does not overflow the actual raised amount // and it does not exceed the cap let left_to_raise = crowdloan @@ -415,11 +453,21 @@ pub mod pallet { .checked_add(amount) .ok_or(Error::::Overflow)?; - // Compute the new total contribution and ensure it does not overflow. - let contribution = Contributions::::get(crowdloan_id, &contributor) - .unwrap_or(Zero::zero()) - .checked_add(amount) - .ok_or(Error::::Overflow)?; + // Compute the new total contribution and ensure it does not overflow, we + // also increment the contributor count if the contribution is new. + let contribution = + if let Some(contribution) = Contributions::::get(crowdloan_id, &contributor) { + contribution + .checked_add(amount) + .ok_or(Error::::Overflow)? + } else { + // We have a new contribution + crowdloan.contributors_count = crowdloan + .contributors_count + .checked_add(1) + .ok_or(Error::::Overflow)?; + amount + }; // Ensure contributor has enough balance to pay ensure!( @@ -476,6 +524,10 @@ pub mod pallet { Contributions::::insert(crowdloan_id, &who, crowdloan.deposit); } else { Contributions::::remove(crowdloan_id, &who); + crowdloan.contributors_count = crowdloan + .contributors_count + .checked_sub(1) + .ok_or(Error::::Underflow)?; } CurrencyOf::::transfer( @@ -625,6 +677,10 @@ pub mod pallet { refund_count = refund_count.checked_add(1).ok_or(Error::::Overflow)?; } + crowdloan.contributors_count = crowdloan + .contributors_count + .checked_sub(refund_count) + .ok_or(Error::::Underflow)?; Crowdloans::::insert(crowdloan_id, &crowdloan); // Clear refunded contributors @@ -682,6 +738,7 @@ pub mod pallet { creator_contribution, Preservation::Expendable, )?; + Contributions::::remove(crowdloan_id, &crowdloan.creator); // Clear the call from the preimage storage if let Some(call) = crowdloan.call { diff --git a/pallets/crowdloan/src/migrations/migrate_add_contributors_count.rs b/pallets/crowdloan/src/migrations/migrate_add_contributors_count.rs new file mode 100644 index 0000000000..3b094843ce --- /dev/null +++ b/pallets/crowdloan/src/migrations/migrate_add_contributors_count.rs @@ -0,0 +1,188 @@ +use alloc::string::String; +use frame_support::{BoundedVec, migration::storage_key_iter, traits::Get, weights::Weight}; +use subtensor_macros::freeze_struct; + +use crate::*; + +mod old_storage { + use super::*; + + #[freeze_struct("84bcbf9b8d3f0ddf")] + #[derive(Encode, Decode, Debug)] + pub struct OldCrowdloanInfo { + pub creator: AccountId, + pub deposit: Balance, + pub min_contribution: Balance, + pub end: BlockNumber, + pub cap: Balance, + pub funds_account: AccountId, + pub raised: Balance, + pub target_address: Option, + pub call: Option, + pub finalized: bool, + } +} + +pub fn migrate_add_contributors_count() -> Weight { + let migration_name = BoundedVec::truncate_from(b"migrate_add_contributors_count".to_vec()); + let mut weight = T::DbWeight::get().reads(1); + + if HasMigrationRun::::get(&migration_name) { + log::info!( + "Migration '{:?}' has already run. Skipping.", + migration_name + ); + return weight; + } + + log::info!( + "Running migration '{}'", + String::from_utf8_lossy(&migration_name) + ); + + let pallet_name = b"Crowdloan"; + let item_name = b"Crowdloans"; + let crowdloans = storage_key_iter::< + CrowdloanId, + old_storage::OldCrowdloanInfo< + T::AccountId, + BalanceOf, + BlockNumberFor, + BoundedCallOf, + >, + Twox64Concat, + >(pallet_name, item_name) + .collect::>(); + weight = weight.saturating_add(T::DbWeight::get().reads(crowdloans.len() as u64)); + + for (id, crowdloan) in crowdloans { + let contributions = Contributions::::iter_key_prefix(id) + .collect::>() + .len(); + weight = weight.saturating_add(T::DbWeight::get().reads(contributions as u64)); + + Crowdloans::::insert( + id, + CrowdloanInfo { + creator: crowdloan.creator, + deposit: crowdloan.deposit, + min_contribution: crowdloan.min_contribution, + end: crowdloan.end, + cap: crowdloan.cap, + funds_account: crowdloan.funds_account, + raised: crowdloan.raised, + target_address: crowdloan.target_address, + call: crowdloan.call, + finalized: crowdloan.finalized, + contributors_count: contributions as u32, + }, + ); + weight = weight.saturating_add(T::DbWeight::get().writes(1)); + } + + HasMigrationRun::::insert(&migration_name, true); + weight = weight.saturating_add(T::DbWeight::get().writes(1)); + + log::info!( + "Migration '{:?}' completed successfully.", + String::from_utf8_lossy(&migration_name) + ); + + weight +} + +#[cfg(test)] +mod tests { + use frame_support::{Hashable, storage::unhashed::put_raw}; + use sp_core::U256; + use sp_io::hashing::twox_128; + + use super::*; + use crate::mock::{Test, TestState}; + + #[test] + fn test_migrate_add_contributors_count_works() { + TestState::default().build_and_execute(|| { + let pallet_name = twox_128(b"Crowdloan"); + let storage_name = twox_128(b"Crowdloans"); + let prefix = [pallet_name, storage_name].concat(); + + let items = vec![ + ( + old_storage::OldCrowdloanInfo { + creator: U256::from(1), + deposit: 100u64, + min_contribution: 10u64, + end: 100u64, + cap: 1000u64, + funds_account: U256::from(2), + raised: 0u64, + target_address: None, + call: None::>, + finalized: false, + }, + vec![(U256::from(1), 100)], + ), + ( + old_storage::OldCrowdloanInfo { + creator: U256::from(1), + deposit: 100u64, + min_contribution: 10u64, + end: 100u64, + cap: 1000u64, + funds_account: U256::from(2), + raised: 0u64, + target_address: None, + call: None::>, + finalized: false, + }, + vec![ + (U256::from(1), 100), + (U256::from(2), 100), + (U256::from(3), 100), + ], + ), + ( + old_storage::OldCrowdloanInfo { + creator: U256::from(1), + deposit: 100u64, + min_contribution: 10u64, + end: 100u64, + cap: 1000u64, + funds_account: U256::from(2), + raised: 0u64, + target_address: None, + call: None::>, + finalized: false, + }, + vec![ + (U256::from(1), 100), + (U256::from(2), 100), + (U256::from(3), 100), + (U256::from(4), 100), + (U256::from(5), 100), + ], + ), + ]; + + for (id, (crowdloan, contributions)) in items.into_iter().enumerate() { + let key = [prefix.clone(), (id as u32).twox_64_concat()].concat(); + put_raw(&key, &crowdloan.encode()); + + for (contributor, amount) in contributions { + Contributions::::insert(id as u32, contributor, amount); + } + } + + migrate_add_contributors_count::(); + + assert!(Crowdloans::::get(0).is_some_and(|c| c.contributors_count == 1)); + assert!(Crowdloans::::get(1).is_some_and(|c| c.contributors_count == 3)); + assert!(Crowdloans::::get(2).is_some_and(|c| c.contributors_count == 5)); + + assert!(HasMigrationRun::::get(BoundedVec::truncate_from( + b"migrate_add_contributors_count".to_vec() + ))); + }); + } +} diff --git a/pallets/crowdloan/src/migrations/mod.rs b/pallets/crowdloan/src/migrations/mod.rs new file mode 100644 index 0000000000..f6701fb83a --- /dev/null +++ b/pallets/crowdloan/src/migrations/mod.rs @@ -0,0 +1,2 @@ +mod migrate_add_contributors_count; +pub use migrate_add_contributors_count::*; diff --git a/pallets/crowdloan/src/mock.rs b/pallets/crowdloan/src/mock.rs index 980b9fa26b..78cf15717c 100644 --- a/pallets/crowdloan/src/mock.rs +++ b/pallets/crowdloan/src/mock.rs @@ -111,6 +111,7 @@ parameter_types! { pub const MinimumBlockDuration: u64 = 20; pub const MaximumBlockDuration: u64 = 100; pub const RefundContributorsLimit: u32 = 5; + pub const MaxContributors: u32 = 10; } impl pallet_crowdloan::Config for Test { @@ -125,6 +126,7 @@ impl pallet_crowdloan::Config for Test { type MinimumBlockDuration = MinimumBlockDuration; type MaximumBlockDuration = MaximumBlockDuration; type RefundContributorsLimit = RefundContributorsLimit; + type MaxContributors = MaxContributors; } // A test pallet used to test some behavior of the crowdloan pallet diff --git a/pallets/crowdloan/src/tests.rs b/pallets/crowdloan/src/tests.rs index 45bc61e40a..1e03854b1f 100644 --- a/pallets/crowdloan/src/tests.rs +++ b/pallets/crowdloan/src/tests.rs @@ -1,7 +1,7 @@ #![cfg(test)] #![allow(clippy::arithmetic_side_effects, clippy::unwrap_used)] -use frame_support::{assert_err, assert_ok, traits::StorePreimage}; +use frame_support::{StorageDoubleMap, assert_err, assert_ok, traits::StorePreimage}; use frame_system::pallet_prelude::BlockNumberFor; use sp_core::U256; use sp_runtime::DispatchError; @@ -46,6 +46,7 @@ fn test_create_succeeds() { target_address: None, call: Some(call), finalized: false, + contributors_count: 1, }) ); // ensure the crowdloan account has the deposit @@ -330,8 +331,15 @@ fn test_contribute_succeeds() { // run some blocks run_to_block(10); - // first contribution to the crowdloan from creator let crowdloan_id: CrowdloanId = 0; + + // only the creator has contributed so far + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 1) + ); + + // first contribution to the crowdloan from creator let amount: BalanceOf = 50; assert_ok!(Crowdloan::contribute( RuntimeOrigin::signed(creator), @@ -351,6 +359,10 @@ fn test_contribute_succeeds() { pallet_crowdloan::Contributions::::get(crowdloan_id, creator), Some(100) ); + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 1) + ); assert_eq!( Balances::free_balance(creator), 200 - amount - initial_deposit @@ -377,6 +389,10 @@ fn test_contribute_succeeds() { pallet_crowdloan::Contributions::::get(crowdloan_id, contributor1), Some(100) ); + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 2) + ); assert_eq!(Balances::free_balance(contributor1), 500 - amount); // third contribution to the crowdloan @@ -400,6 +416,10 @@ fn test_contribute_succeeds() { pallet_crowdloan::Contributions::::get(crowdloan_id, contributor2), Some(50) ); + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 3) + ); assert_eq!(Balances::free_balance(contributor2), 200 - amount); // ensure the contributions are present in the funds account @@ -656,6 +676,62 @@ fn test_contribute_fails_if_contribution_is_below_minimum_contribution() { }); } +#[test] +fn test_contribute_fails_if_max_contributors_has_been_reached() { + TestState::default() + .with_balance(U256::from(1), 100) + .with_balance(U256::from(2), 100) + .with_balance(U256::from(3), 100) + .with_balance(U256::from(4), 100) + .with_balance(U256::from(5), 100) + .with_balance(U256::from(6), 100) + .with_balance(U256::from(7), 100) + .with_balance(U256::from(8), 100) + .with_balance(U256::from(9), 100) + .with_balance(U256::from(10), 100) + .with_balance(U256::from(11), 100) + .build_and_execute(|| { + // create a crowdloan + let creator: AccountOf = U256::from(1); + let initial_deposit: BalanceOf = 50; + let min_contribution: BalanceOf = 10; + let cap: BalanceOf = 1000; + let end: BlockNumberFor = 50; + + assert_ok!(Crowdloan::create( + RuntimeOrigin::signed(creator), + initial_deposit, + min_contribution, + cap, + end, + Some(noop_call()), + None + )); + + // run some blocks + run_to_block(10); + + // contribute to the crowdloan + let crowdloan_id: CrowdloanId = 0; + let amount: BalanceOf = 20; + for i in 2..=10 { + let contributor: AccountOf = U256::from(i); + assert_ok!(Crowdloan::contribute( + RuntimeOrigin::signed(contributor), + crowdloan_id, + amount + )); + } + + // try to contribute + let contributor: AccountOf = U256::from(10); + assert_err!( + Crowdloan::contribute(RuntimeOrigin::signed(contributor), crowdloan_id, amount), + pallet_crowdloan::Error::::MaxContributorsReached + ); + }); +} + #[test] fn test_contribute_fails_if_contributor_has_insufficient_balance() { TestState::default() @@ -743,6 +819,12 @@ fn test_withdraw_from_contributor_succeeds() { // run some more blocks past the end of the contribution period run_to_block(60); + // ensure the contributor count is correct + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 3) + ); + // withdraw from contributor1 assert_ok!(Crowdloan::withdraw( RuntimeOrigin::signed(contributor1), @@ -753,6 +835,10 @@ fn test_withdraw_from_contributor_succeeds() { pallet_crowdloan::Contributions::::get(crowdloan_id, contributor1), None, ); + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 2) + ); // ensure the contributor1 has the correct amount assert_eq!( pallet_balances::Pallet::::free_balance(contributor1), @@ -769,6 +855,10 @@ fn test_withdraw_from_contributor_succeeds() { pallet_crowdloan::Contributions::::get(crowdloan_id, contributor2), None, ); + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 1) + ); // ensure the contributor2 has the correct amount assert_eq!( pallet_balances::Pallet::::free_balance(contributor2), @@ -818,6 +908,12 @@ fn test_withdraw_from_creator_with_contribution_over_deposit_succeeds() { amount )); + // ensure the contributor count is correct + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 1) + ); + // withdraw let crowdloan_id: CrowdloanId = 0; assert_ok!(Crowdloan::withdraw( @@ -835,6 +931,11 @@ fn test_withdraw_from_creator_with_contribution_over_deposit_succeeds() { pallet_crowdloan::Contributions::::get(crowdloan_id, creator), Some(initial_deposit), ); + // ensure the contributor count hasn't changed because deposit is kept + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 1) + ); // ensure the crowdloan account has the correct amount let funds_account = pallet_crowdloan::Pallet::::funds_account(crowdloan_id); @@ -1478,6 +1579,12 @@ fn test_refund_succeeds() { )); } + // ensure the contributor count is correct + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 7) + ); + // run some more blocks past the end of the contribution period run_to_block(60); @@ -1487,6 +1594,12 @@ fn test_refund_succeeds() { crowdloan_id )); + // ensure the contributor count is correct, we processed 5 refunds + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 2) + ); + // ensure the crowdloan account has the correct amount let funds_account = pallet_crowdloan::Pallet::::funds_account(crowdloan_id); assert_eq!(Balances::free_balance(funds_account), 350 - 5 * amount); @@ -1510,6 +1623,13 @@ fn test_refund_succeeds() { crowdloan_id )); + // ensure the contributor count is correct, we processed 1 more refund + // keeping deposit + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 1) + ); + // ensure the crowdloan account has the correct amount assert_eq!( pallet_balances::Pallet::::free_balance(funds_account), @@ -1638,15 +1758,15 @@ fn test_dissolve_succeeds() { // run some blocks past end run_to_block(60); - // refund the contributions let crowdloan_id: CrowdloanId = 0; - assert_ok!(Crowdloan::refund( - RuntimeOrigin::signed(creator), - crowdloan_id - )); + + // ensure the contributor count is correct + assert!( + pallet_crowdloan::Crowdloans::::get(crowdloan_id) + .is_some_and(|c| c.contributors_count == 1) + ); // dissolve the crowdloan - let crowdloan_id: CrowdloanId = 0; assert_ok!(Crowdloan::dissolve( RuntimeOrigin::signed(creator), crowdloan_id @@ -1655,6 +1775,11 @@ fn test_dissolve_succeeds() { // ensure the crowdloan is removed from the crowdloans map assert!(pallet_crowdloan::Crowdloans::::get(crowdloan_id).is_none()); + // ensure the contributions are removed + assert!(!pallet_crowdloan::Contributions::::contains_prefix( + crowdloan_id + )); + // ensure the event is emitted assert_eq!( last_event(), diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index a70315d324..9fc76f69dc 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -1445,6 +1445,7 @@ parameter_types! { 432000 // 60 days maximum (60 * 24 * 60 * 60 / 12) }; pub const RefundContributorsLimit: u32 = 50; + pub const MaxContributors: u32 = 500; } impl pallet_crowdloan::Config for Runtime { @@ -1459,6 +1460,7 @@ impl pallet_crowdloan::Config for Runtime { type MinimumBlockDuration = MinimumBlockDuration; type MaximumBlockDuration = MaximumBlockDuration; type RefundContributorsLimit = RefundContributorsLimit; + type MaxContributors = MaxContributors; } // Create the runtime by composing the FRAME pallets that were previously configured.