Skip to content

Commit

Permalink
Removed storage getter (#2111)
Browse files Browse the repository at this point in the history
# Goal
The goal of this PR is to remove `getter` function for storage items.

Closes #2105
  • Loading branch information
aramikm authored Aug 5, 2024
1 parent bdf1578 commit 9919861
Show file tree
Hide file tree
Showing 32 changed files with 249 additions and 245 deletions.
3 changes: 0 additions & 3 deletions designdocs/capacity.md
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,6 @@ Storage for target Capacity usage.
/// - Keys: MSA Id
/// - Value: [`CapacityDetails`](types::CapacityDetails)
#[pallet::storage]
#[pallet::getter(fn get_capacity_for)]
pub type CapacityLedger<T: Config> =
StorageMap<_, Twox64Concat, MessageSourceId, CapacityDetails<BalanceOf<T>, T::EpochNumber>>;
```
Expand All @@ -333,7 +332,6 @@ Storage for epoch length
```rust
/// Storage for the epoch length
#[pallet::storage]
#[pallet::getter(fn get_epoch_length)]
pub type EpochLength<T: Config> = StorageValue<_, BlockNumberFor::<T>, ValueQuery, EpochLengthDefault<T>>;
```

Expand Down Expand Up @@ -520,7 +518,6 @@ trait Replenishable {
/// Storage for the current epoch number
#[pallet::storage]
#[pallet::whitelist_storage]
#[pallet::getter(fn get_current_epoch)]
pub type CurrentEpoch<T: Config> = StorageValue<_, T::EpochNumber, ValueQuery>;
```

Expand Down
2 changes: 0 additions & 2 deletions designdocs/provider_boosting_implementation.md
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,6 @@ pub struct RewardPoolInfo<Balance> {

/// Reward Pool history
#[pallet::storage]
#[pallet::getter(fn get_reward_pool_for_era)]
pub type StakingRewardPool<T: Config> = <CountedStorageMap<_, Twox64Concat, RewardEra, RewardPoolInfo<T>;
```

Expand All @@ -243,7 +242,6 @@ Storage is whitelisted because it's accessed every block and would improperly ad
```rust
#[pallet::storage]
#[pallet::whitelist_storage]
#[pallet::getter(fn get_current_era)]
/// Similar to CurrentEpoch
pub type CurrentEraInfo<T:Config> = StorageValue<_, T::RewardEraInfo, ValueQuery>;

Expand Down
6 changes: 3 additions & 3 deletions pallets/capacity/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub fn create_funded_account<T: Config>(
// In the benchmarks we expect a new epoch to always start so as to test worst case scenario.
pub fn set_up_epoch<T: Config>(current_block: BlockNumberFor<T>, current_epoch: T::EpochNumber) {
CurrentEpoch::<T>::set(current_epoch);
let epoch_start = current_block.saturating_sub(Capacity::<T>::get_epoch_length());
let epoch_start = current_block.saturating_sub(EpochLength::<T>::get());
CurrentEpochInfo::<T>::set(EpochInfo { epoch_start });
}

Expand Down Expand Up @@ -78,7 +78,7 @@ benchmarks! {
}: {
Capacity::<T>::on_initialize(current_block);
} verify {
assert_eq!(current_epoch.saturating_add(1u32.into()), Capacity::<T>::get_current_epoch());
assert_eq!(current_epoch.saturating_add(1u32.into()), CurrentEpoch::<T>::get());
assert_eq!(current_block, CurrentEpochInfo::<T>::get().epoch_start);
}
unstake {
Expand Down Expand Up @@ -120,7 +120,7 @@ benchmarks! {
let epoch_length: BlockNumberFor<T> = 9u32.into();
}: _ (RawOrigin::Root, epoch_length)
verify {
assert_eq!(Capacity::<T>::get_epoch_length(), 9u32.into());
assert_eq!(EpochLength::<T>::get(), 9u32.into());
assert_last_event::<T>(Event::<T>::EpochLengthUpdated {blocks: epoch_length}.into());
}

Expand Down
56 changes: 25 additions & 31 deletions pallets/capacity/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,15 +157,13 @@ pub mod pallet {
/// - Keys: AccountId
/// - Value: [`StakingDetails`](types::StakingDetails)
#[pallet::storage]
#[pallet::getter(fn get_staking_account_for)]
pub type StakingAccountLedger<T: Config> =
StorageMap<_, Twox64Concat, T::AccountId, StakingDetails<T>>;

/// Storage to record how many tokens were targeted to an MSA.
/// - Keys: AccountId, MSA Id
/// - Value: [`StakingTargetDetails`](types::StakingTargetDetails)
#[pallet::storage]
#[pallet::getter(fn get_target_for)]
pub type StakingTargetLedger<T: Config> = StorageDoubleMap<
_,
Twox64Concat,
Expand All @@ -179,19 +177,16 @@ pub mod pallet {
/// - Keys: MSA Id
/// - Value: [`CapacityDetails`](types::CapacityDetails)
#[pallet::storage]
#[pallet::getter(fn get_capacity_for)]
pub type CapacityLedger<T: Config> =
StorageMap<_, Twox64Concat, MessageSourceId, CapacityDetails<BalanceOf<T>, T::EpochNumber>>;

/// Storage for the current epoch number
#[pallet::storage]
#[pallet::whitelist_storage]
#[pallet::getter(fn get_current_epoch)]
pub type CurrentEpoch<T: Config> = StorageValue<_, T::EpochNumber, ValueQuery>;

/// Storage for the current epoch info
#[pallet::storage]
#[pallet::getter(fn get_current_epoch_info)]
pub type CurrentEpochInfo<T: Config> =
StorageValue<_, EpochInfo<BlockNumberFor<T>>, ValueQuery>;

Expand All @@ -203,12 +198,10 @@ pub mod pallet {

/// Storage for the epoch length
#[pallet::storage]
#[pallet::getter(fn get_epoch_length)]
pub type EpochLength<T: Config> =
StorageValue<_, BlockNumberFor<T>, ValueQuery, EpochLengthDefault<T>>;

#[pallet::storage]
#[pallet::getter(fn get_unstake_unlocking_for)]
pub type UnstakeUnlocks<T: Config> =
StorageMap<_, Twox64Concat, T::AccountId, UnlockChunkList<T>>;

Expand Down Expand Up @@ -434,7 +427,7 @@ impl<T: Config> Pallet<T> {
ensure!(amount > Zero::zero(), Error::<T>::ZeroAmountNotAllowed);
ensure!(T::TargetValidator::validate(target), Error::<T>::InvalidTarget);

let staking_account = Self::get_staking_account_for(&staker).unwrap_or_default();
let staking_account = StakingAccountLedger::<T>::get(&staker).unwrap_or_default();
let stakable_amount = Self::get_stakable_amount_for(&staker, amount);

ensure!(stakable_amount > Zero::zero(), Error::<T>::BalanceTooLowtoStake);
Expand Down Expand Up @@ -463,10 +456,11 @@ impl<T: Config> Pallet<T> {
staking_account.deposit(amount).ok_or(ArithmeticError::Overflow)?;

let capacity = Self::capacity_generated(amount);
let mut target_details = Self::get_target_for(&staker, &target).unwrap_or_default();
let mut target_details =
StakingTargetLedger::<T>::get(&staker, &target).unwrap_or_default();
target_details.deposit(amount, capacity).ok_or(ArithmeticError::Overflow)?;

let mut capacity_details = Self::get_capacity_for(target).unwrap_or_default();
let mut capacity_details = CapacityLedger::<T>::get(target).unwrap_or_default();
capacity_details.deposit(&amount, &capacity).ok_or(ArithmeticError::Overflow)?;

Self::set_staking_account_and_lock(&staker, staking_account)?;
Expand All @@ -482,7 +476,7 @@ impl<T: Config> Pallet<T> {
staker: &T::AccountId,
staking_account: &StakingDetails<T>,
) -> Result<(), DispatchError> {
let unlocks = Self::get_unstake_unlocking_for(staker).unwrap_or_default();
let unlocks = UnstakeUnlocks::<T>::get(staker).unwrap_or_default();
let total_to_lock: BalanceOf<T> = staking_account
.active
.checked_add(&unlock_chunks_total::<T>(&unlocks))
Expand Down Expand Up @@ -527,7 +521,7 @@ impl<T: Config> Pallet<T> {
amount: BalanceOf<T>,
) -> Result<BalanceOf<T>, DispatchError> {
let mut staking_account =
Self::get_staking_account_for(unstaker).ok_or(Error::<T>::NotAStakingAccount)?;
StakingAccountLedger::<T>::get(unstaker).ok_or(Error::<T>::NotAStakingAccount)?;
ensure!(amount <= staking_account.active, Error::<T>::AmountToUnstakeExceedsAmountStaked);

let actual_unstaked_amount = staking_account.withdraw(amount)?;
Expand All @@ -539,10 +533,10 @@ impl<T: Config> Pallet<T> {
unstaker: &T::AccountId,
actual_unstaked_amount: BalanceOf<T>,
) -> Result<(), DispatchError> {
let current_epoch: T::EpochNumber = Self::get_current_epoch();
let current_epoch: T::EpochNumber = CurrentEpoch::<T>::get();
let thaw_at =
current_epoch.saturating_add(T::EpochNumber::from(T::UnstakingThawPeriod::get()));
let mut unlocks = Self::get_unstake_unlocking_for(unstaker).unwrap_or_default();
let mut unlocks = UnstakeUnlocks::<T>::get(unstaker).unwrap_or_default();

let unlock_chunk: UnlockChunk<BalanceOf<T>, T::EpochNumber> =
UnlockChunk { value: actual_unstaked_amount, thaw_at };
Expand All @@ -568,11 +562,11 @@ impl<T: Config> Pallet<T> {
pub(crate) fn do_withdraw_unstaked(
staker: &T::AccountId,
) -> Result<BalanceOf<T>, DispatchError> {
let current_epoch = Self::get_current_epoch();
let current_epoch = CurrentEpoch::<T>::get();
let mut total_unlocking: BalanceOf<T> = Zero::zero();

let mut unlocks =
Self::get_unstake_unlocking_for(staker).ok_or(Error::<T>::NoUnstakedTokensAvailable)?;
UnstakeUnlocks::<T>::get(staker).ok_or(Error::<T>::NoUnstakedTokensAvailable)?;
let amount_withdrawn = unlock_chunks_reap_thawed::<T>(&mut unlocks, current_epoch);
ensure!(!amount_withdrawn.is_zero(), Error::<T>::NoThawedTokenAvailable);

Expand All @@ -583,7 +577,7 @@ impl<T: Config> Pallet<T> {
UnstakeUnlocks::<T>::set(staker, Some(unlocks));
}

let staking_account = Self::get_staking_account_for(staker).unwrap_or_default();
let staking_account = StakingAccountLedger::<T>::get(staker).unwrap_or_default();
let total_locked = staking_account.active.saturating_add(total_unlocking);
if total_locked.is_zero() {
T::Currency::thaw(&FreezeReason::CapacityStaking.into(), staker)?;
Expand All @@ -599,10 +593,10 @@ impl<T: Config> Pallet<T> {
target: MessageSourceId,
amount: BalanceOf<T>,
) -> Result<BalanceOf<T>, DispatchError> {
let mut staking_target_details = Self::get_target_for(&unstaker, &target)
let mut staking_target_details = StakingTargetLedger::<T>::get(&unstaker, &target)
.ok_or(Error::<T>::StakerTargetRelationshipNotFound)?;
let mut capacity_details =
Self::get_capacity_for(target).ok_or(Error::<T>::TargetCapacityNotFound)?;
CapacityLedger::<T>::get(target).ok_or(Error::<T>::TargetCapacityNotFound)?;

let capacity_to_withdraw = if staking_target_details.amount.eq(&amount) {
staking_target_details.capacity
Expand Down Expand Up @@ -640,10 +634,10 @@ impl<T: Config> Pallet<T> {

fn start_new_epoch_if_needed(current_block: BlockNumberFor<T>) -> Weight {
// Should we start a new epoch?
if current_block.saturating_sub(Self::get_current_epoch_info().epoch_start) >=
Self::get_epoch_length()
if current_block.saturating_sub(CurrentEpochInfo::<T>::get().epoch_start) >=
EpochLength::<T>::get()
{
let current_epoch = Self::get_current_epoch();
let current_epoch = CurrentEpoch::<T>::get();
CurrentEpoch::<T>::set(current_epoch.saturating_add(1u32.into()));
CurrentEpochInfo::<T>::set(EpochInfo { epoch_start: current_block });
T::WeightInfo::on_initialize()
Expand All @@ -661,7 +655,7 @@ impl<T: Config> Nontransferable for Pallet<T> {

/// Return the remaining capacity for the Provider MSA Id
fn balance(msa_id: MessageSourceId) -> Self::Balance {
match Self::get_capacity_for(msa_id) {
match CapacityLedger::<T>::get(msa_id) {
Some(capacity_details) => capacity_details.remaining_capacity,
None => BalanceOf::<T>::zero(),
}
Expand All @@ -670,7 +664,7 @@ impl<T: Config> Nontransferable for Pallet<T> {
/// Spend capacity: reduce remaining capacity by the given amount
fn deduct(msa_id: MessageSourceId, amount: Self::Balance) -> Result<(), DispatchError> {
let mut capacity_details =
Self::get_capacity_for(msa_id).ok_or(Error::<T>::TargetCapacityNotFound)?;
CapacityLedger::<T>::get(msa_id).ok_or(Error::<T>::TargetCapacityNotFound)?;

capacity_details
.deduct_capacity_by_amount(amount)
Expand All @@ -685,7 +679,7 @@ impl<T: Config> Nontransferable for Pallet<T> {
/// Increase all totals for the MSA's CapacityDetails.
fn deposit(msa_id: MessageSourceId, amount: Self::Balance) -> Result<(), DispatchError> {
let mut capacity_details =
Self::get_capacity_for(msa_id).ok_or(Error::<T>::TargetCapacityNotFound)?;
CapacityLedger::<T>::get(msa_id).ok_or(Error::<T>::TargetCapacityNotFound)?;
capacity_details.deposit(&amount, &Self::capacity_generated(amount));
Self::set_capacity_for(msa_id, capacity_details);
Ok(())
Expand All @@ -697,9 +691,9 @@ impl<T: Config> Replenishable for Pallet<T> {

fn replenish_all_for(msa_id: MessageSourceId) -> Result<(), DispatchError> {
let mut capacity_details =
Self::get_capacity_for(msa_id).ok_or(Error::<T>::TargetCapacityNotFound)?;
CapacityLedger::<T>::get(msa_id).ok_or(Error::<T>::TargetCapacityNotFound)?;

capacity_details.replenish_all(&Self::get_current_epoch());
capacity_details.replenish_all(&CurrentEpoch::<T>::get());

Self::set_capacity_for(msa_id, capacity_details);

Expand All @@ -713,14 +707,14 @@ impl<T: Config> Replenishable for Pallet<T> {
amount: Self::Balance,
) -> Result<(), DispatchError> {
let mut capacity_details =
Self::get_capacity_for(msa_id).ok_or(Error::<T>::TargetCapacityNotFound)?;
capacity_details.replenish_by_amount(amount, &Self::get_current_epoch());
CapacityLedger::<T>::get(msa_id).ok_or(Error::<T>::TargetCapacityNotFound)?;
capacity_details.replenish_by_amount(amount, &CurrentEpoch::<T>::get());
Ok(())
}

fn can_replenish(msa_id: MessageSourceId) -> bool {
if let Some(capacity_details) = Self::get_capacity_for(msa_id) {
return capacity_details.can_replenish(Self::get_current_epoch())
if let Some(capacity_details) = CapacityLedger::<T>::get(msa_id) {
return capacity_details.can_replenish(CurrentEpoch::<T>::get());
}
false
}
Expand Down
32 changes: 16 additions & 16 deletions pallets/capacity/src/tests/epochs_tests.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::mock::*;
use crate::{
tests::testing_utils::{run_to_block, system_run_to_block},
Config, EpochLength, Error, Event,
Config, CurrentEpoch, CurrentEpochInfo, EpochLength, Error, Event,
};
use frame_support::{assert_noop, assert_ok, traits::Get};
use frame_system::pallet_prelude::BlockNumberFor;
Expand All @@ -13,7 +13,7 @@ fn set_epoch_length_happy_path() {
let epoch_length: BlockNumberFor<Test> = 9;
assert_ok!(Capacity::set_epoch_length(RuntimeOrigin::root(), epoch_length));

let storage_epoch_length: BlockNumberFor<Test> = Capacity::get_epoch_length();
let storage_epoch_length: BlockNumberFor<Test> = EpochLength::<Test>::get();
assert_eq!(epoch_length, storage_epoch_length);

System::assert_last_event(Event::EpochLengthUpdated { blocks: epoch_length }.into());
Expand Down Expand Up @@ -44,7 +44,7 @@ fn set_epoch_length_errors_when_not_submitted_as_root() {
#[test]
fn get_epoch_length_should_return_max_epoch_length_when_unset() {
new_test_ext().execute_with(|| {
let epoch_length: BlockNumberFor<Test> = Capacity::get_epoch_length();
let epoch_length: BlockNumberFor<Test> = EpochLength::<Test>::get();
let max_epoch_length: BlockNumberFor<Test> = <Test as Config>::MaxEpochLength::get();

assert_eq!(epoch_length, max_epoch_length);
Expand All @@ -54,7 +54,7 @@ fn get_epoch_length_should_return_max_epoch_length_when_unset() {
fn get_epoch_length_should_return_storage_epoch_length() {
new_test_ext().execute_with(|| {
EpochLength::<Test>::set(101u32);
let epoch_length: BlockNumberFor<Test> = Capacity::get_epoch_length();
let epoch_length: BlockNumberFor<Test> = EpochLength::<Test>::get();

assert_eq!(epoch_length, 101u32);
});
Expand All @@ -64,14 +64,14 @@ fn get_epoch_length_should_return_storage_epoch_length() {
fn start_new_epoch_if_needed_when_not_starting_from_zero() {
new_test_ext().execute_with(|| {
EpochLength::<Test>::set(100u32);
let epoch_info = Capacity::get_current_epoch_info();
let cur_epoch = Capacity::get_current_epoch();
let epoch_info = CurrentEpochInfo::<Test>::get();
let cur_epoch = CurrentEpoch::<Test>::get();
// Run the system to block 999 before initializing Capacity to emulate
// deploying to an existing network where blockheight is greater than 0.
system_run_to_block(999);
run_to_block(1000);
let after_epoch = Capacity::get_current_epoch();
let after_epoch_info = Capacity::get_current_epoch_info();
let after_epoch = CurrentEpoch::<Test>::get();
let after_epoch_info = CurrentEpochInfo::<Test>::get();
assert_eq!(epoch_info.epoch_start, 0);
assert_eq!(after_epoch_info.epoch_start, 1000);
assert_eq!(after_epoch, cur_epoch + 1);
Expand All @@ -84,27 +84,27 @@ fn start_new_epoch_if_needed_when_epoch_length_changes() {
EpochLength::<Test>::set(100u32);
// Starting block = 100
system_run_to_block(100);
let epoch_info = Capacity::get_current_epoch_info();
let cur_epoch = Capacity::get_current_epoch();
let epoch_info = CurrentEpochInfo::<Test>::get();
let cur_epoch = CurrentEpoch::<Test>::get();
assert_eq!(epoch_info.epoch_start, 0);
assert_eq!(cur_epoch, 0);
run_to_block(150);
let middle_epoch = Capacity::get_current_epoch();
let middle_epoch_info = Capacity::get_current_epoch_info();
let middle_epoch = CurrentEpoch::<Test>::get();
let middle_epoch_info = CurrentEpochInfo::<Test>::get();
assert_eq!(middle_epoch, 1);
assert_eq!(middle_epoch_info.epoch_start, 101);
// Change epoch length = 20
EpochLength::<Test>::set(20u32);
run_to_block(151);
let after_epoch = Capacity::get_current_epoch();
let after_epoch_info = Capacity::get_current_epoch_info();
let after_epoch = CurrentEpoch::<Test>::get();
let after_epoch_info = CurrentEpochInfo::<Test>::get();
// epoch 1 starts at 101
// epoch 2 starts at 151 (First block after epoch length change)
assert_eq!(after_epoch_info.epoch_start, 151);
assert_eq!(after_epoch, 2);
run_to_block(171);
let last_epoch = Capacity::get_current_epoch();
let last_epoch_info = Capacity::get_current_epoch_info();
let last_epoch = CurrentEpoch::<Test>::get();
let last_epoch_info = CurrentEpochInfo::<Test>::get();
assert_eq!(last_epoch, 3);
// epoch 3 starts at 171 (151 + 20)
assert_eq!(last_epoch_info.epoch_start, 171);
Expand Down
Loading

0 comments on commit 9919861

Please sign in to comment.