-
Notifications
You must be signed in to change notification settings - Fork 148
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
refactor: hotkey swap + tests #542
Conversation
Co-authored-by: Keith <keith@opentensor.dev>
pallets/subtensor/src/swap.rs
Outdated
pub fn swap_total_hotkey_stake( | ||
old_hotkey: &T::AccountId, | ||
new_hotkey: &T::AccountId, | ||
weight: &mut Weight, | ||
) -> DispatchResult { | ||
if let Ok(total_hotkey_stake) = TotalHotkeyStake::<T>::try_get(old_hotkey) { | ||
TotalHotkeyStake::<T>::remove(old_hotkey); | ||
TotalHotkeyStake::<T>::insert(new_hotkey, total_hotkey_stake); | ||
weight.saturating_accrue(T::DbWeight::get().writes(2)); | ||
} | ||
Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods are a noop if the old_hotkey doesn't exist, so unless that's sometimes expected, this should prob return an error in that case in case of silently nooping.
Could also add some debug logs in case we need to introspect behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the hotkey doesnt exist , it would be caught here : https://github.com/opentensor/subtensor/pull/542/files#diff-f17d3c10f0bee92182f2bdb18feb020e8e740d95394451f8c8930a219e1c0b7aR43 . Do you think its sufficient , or do we still need to catch the error in this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, the weight calculation here is missing the read from try_get
, and doesn't need to return a Result
.
for (coldkey, stake_amount) in stakes { | ||
Stake::<T>::insert(new_hotkey, &coldkey, stake_amount); | ||
let _ = Stake::<T>::clear_prefix(old_hotkey, stake_count, None); | ||
writes += 2; // One write for insert and one for remove | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we checked how many keys will be touched here? Could be a very expensive operation
Co-authored-by: orriin <167025436+orriin@users.noreply.github.com>
/// | ||
/// # Returns | ||
/// | ||
/// * `Result<(), Error<T>>` - The result of the operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment here needs to be updated.
closing as github is being silly and not updating my latest changes |
Description
This PR :
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
Checklist
cargo fmt
andcargo clippy
to ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.