Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix storage item typos #834

Draft
wants to merge 2 commits into
base: devnet-ready
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions pallets/subtensor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,7 @@ pub mod pallet {
pub type HotkeyEmissionTempo<T> =
StorageValue<_, u64, ValueQuery, DefaultHotkeyEmissionTempo<T>>;
#[pallet::storage]
/// Map ( hot ) --> emission | Accumulated hotkey emission.
/// Name corrected: PendingdHotkeyEmission => PendingHotkeyEmission
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this comment ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's useful for people who index the chain like me. When I search the updated name there's nothing relevant, but the old name gives results:

I happened to be aware of this typo, but if I wasn't and wanted to understand why my storage calls weren't working in earlier blocks this comment in the code would make it a lot easier to discover the reason.

pub type PendingdHotkeyEmission<T: Config> = StorageMap<
_,
Blake2_128Concat,
Expand All @@ -790,6 +790,16 @@ pub mod pallet {
DefaultAccumulatedEmission<T>,
>;
#[pallet::storage]
/// Map ( hot ) --> emission | Accumulated hotkey emission.
pub type PendingHotkeyEmission<T: Config> = StorageMap<
_,
Blake2_128Concat,
T::AccountId,
u64,
ValueQuery,
DefaultAccumulatedEmission<T>,
>;
#[pallet::storage]
/// Map ( hot, cold ) --> block_number | Last add stake increase.
pub type LastAddStakeIncrease<T: Config> = StorageDoubleMap<
_,
Expand Down Expand Up @@ -939,10 +949,14 @@ pub mod pallet {
pub type BlocksSinceLastStep<T> =
StorageMap<_, Identity, u16, u64, ValueQuery, DefaultBlocksSinceLastStep<T>>;
#[pallet::storage]
/// --- MAP ( netuid ) --> last_mechanism_step_block
/// Name corrected: LastMechansimStepBlock => LastMechanismStepBlock
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

pub type LastMechansimStepBlock<T> =
StorageMap<_, Identity, u16, u64, ValueQuery, DefaultLastMechanismStepBlock<T>>;
#[pallet::storage]
/// --- MAP ( netuid ) --> last_mechanism_step_block
pub type LastMechanismStepBlock<T> =
StorageMap<_, Identity, u16, u64, ValueQuery, DefaultLastMechanismStepBlock<T>>;
#[pallet::storage]
/// --- MAP ( netuid ) --> subnet_owner
pub type SubnetOwner<T: Config> =
StorageMap<_, Identity, u16, T::AccountId, ValueQuery, DefaultSubnetOwner<T>>;
Expand Down
4 changes: 3 additions & 1 deletion pallets/subtensor/src/macros/hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ mod hooks {
// Storage version v8 -> v9
.saturating_add(migrations::migrate_fix_total_coldkey_stake::migrate_fix_total_coldkey_stake::<T>())
// Migrate Delegate Ids on chain
.saturating_add(migrations::migrate_chain_identity::migrate_set_hotkey_identities::<T>());
.saturating_add(migrations::migrate_chain_identity::migrate_set_hotkey_identities::<T>())
// Migration to fix typos in storage items
.saturating_add(migrations::migrate_fix_storage_typos::migrate_rename_storage_items::<T>());
weight
}

Expand Down
59 changes: 59 additions & 0 deletions pallets/subtensor/src/migrations/migrate_fix_storage_typos.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
use super::*;
use frame_support::{traits::Get, weights::Weight};
use scale_info::prelude::string::String;

/// Migrates storage items with typos to their correctly named counterparts.
///
/// - `PendingdHotkeyEmission` is migrated to `PendingHotkeyEmission`.
/// - `LastMechansimStepBlock` is migrated to `LastMechanismStepBlock`.
///
/// # Returns
/// The cumulative weight of the migration process.
pub fn migrate_rename_storage_items<T: Config>() -> Weight {
let migration_name = b"migrate_rename_storage_items_v1".to_vec();

let mut weight = T::DbWeight::get().reads(1);

// Check if the migration has already been run.
if HasMigrationRun::<T>::get(&migration_name) {
log::info!(
"Migration '{:?}' has already run. Skipping.",
migration_name
);
return weight;
}

log::info!(
"Running migration '{}'",
String::from_utf8_lossy(&migration_name)
);

// === Migrate `PendingdHotkeyEmission` to `PendingHotkeyEmission` and wipe old storage ===
for (account_id, emission) in PendingdHotkeyEmission::<T>::drain() {
// Insert the value into the new storage
PendingHotkeyEmission::<T>::insert(account_id, emission);

// Add weight for the write operation (the `drain` method takes care of the removal)
weight = weight.saturating_add(T::DbWeight::get().writes(1));
}

// === Migrate `LastMechansimStepBlock` to `LastMechanismStepBlock` and wipe old storage ===
for (netuid, block) in LastMechansimStepBlock::<T>::drain() {
// Insert the value into the new storage
LastMechanismStepBlock::<T>::insert(netuid, block);

// Add weight for the write operation (the `drain` method takes care of the removal)
weight = weight.saturating_add(T::DbWeight::get().writes(1));
}

// === Mark Migration as Completed ===
HasMigrationRun::<T>::insert(&migration_name, true);
weight = weight.saturating_add(T::DbWeight::get().writes(1));

log::info!(
"Migration '{:?}' completed.",
String::from_utf8_lossy(&migration_name)
);

weight
}
1 change: 1 addition & 0 deletions pallets/subtensor/src/migrations/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pub mod migrate_chain_identity;
pub mod migrate_create_root_network;
pub mod migrate_delete_subnet_21;
pub mod migrate_delete_subnet_3;
pub mod migrate_fix_storage_typos;
pub mod migrate_fix_total_coldkey_stake;
pub mod migrate_init_total_issuance;
pub mod migrate_populate_owned_hotkeys;
Expand Down
62 changes: 61 additions & 1 deletion pallets/subtensor/tests/migration.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![allow(unused, clippy::indexing_slicing, clippy::panic, clippy::unwrap_used)]
mod mock;
use frame_support::{assert_ok, weights::Weight};
use frame_support::{assert_ok, traits::StorageVersion, weights::Weight};
use frame_system::Config;
use mock::*;
use pallet_subtensor::*;
Expand Down Expand Up @@ -430,3 +430,63 @@ fn run_migration_and_check(migration_name: &'static str) -> frame_support::weigh
// Return the weight of the executed migration
weight
}

#[test]
fn test_migrate_rename_storage_items() {
new_test_ext(1).execute_with(|| {
let migration_name = b"migrate_rename_storage_items_v1";

let account_ids: [U256; 3] = [
AccountId::from([1u8; 32]),
AccountId::from([2u8; 32]),
AccountId::from([3u8; 32]),
];

let netuids: Vec<u16> = vec![42, 43, 44];
let emission_values: Vec<u64> = vec![4, 1000, 0, u64::MAX];
let block_values: Vec<u64> = vec![11, 5000, 1, u64::MAX];

// Insert multiple entries into old storage items with typos
for (i, account_id) in account_ids.iter().enumerate() {
PendingdHotkeyEmission::<Test>::insert(account_id, emission_values[i]);
}
for (i, netuid) in netuids.iter().enumerate() {
LastMechansimStepBlock::<Test>::insert(*netuid, block_values[i]);
}

assert!(!HasMigrationRun::<Test>::get(migration_name.to_vec()));

let weight: frame_support::weights::Weight =
pallet_subtensor::migrations::migrate_fix_storage_typos::migrate_rename_storage_items::<
Test,
>();

assert!(
weight != Weight::zero(),
"Migration weight should be non-zero"
);

assert!(HasMigrationRun::<Test>::get(migration_name.to_vec()));

// Verify data has been migrated to the new storage items
for (i, account_id) in account_ids.iter().enumerate() {
let expected_emission = emission_values[i];
assert_eq!(
PendingHotkeyEmission::<Test>::get(account_id),
expected_emission
);
}
for (i, netuid) in netuids.iter().enumerate() {
let expected_block = block_values[i];
assert_eq!(LastMechanismStepBlock::<Test>::get(*netuid), expected_block);
}

// Verify all items are removed from the old storage
for account_id in account_ids.iter() {
assert!(!PendingdHotkeyEmission::<Test>::contains_key(account_id));
}
for netuid in netuids.iter() {
assert!(!LastMechansimStepBlock::<Test>::contains_key(*netuid));
}
});
}
Loading