refactor: hotkey swap + tests#542
Conversation
distributedstatemachine
added
devnet-skip
hotfix
Co-authored-by: Keith <keith@opentensor.dev>
distributedstatemachine
requested review from
a team,
JohnReedV,
camfairchild,
gztensor and
keithtensor
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.
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.
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.
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.
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.
Comment here needs to be updated.
|
closing as github is being silly and not updating my latest changes |
5 participants
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 fmtandcargo clippyto 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.