-
Notifications
You must be signed in to change notification settings - Fork 252
Voluntary deregistration #2209
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
base: devnet-ready
Are you sure you want to change the base?
Voluntary deregistration #2209
Conversation
|
Looks good. Need to fix CI |
|
now this is following good design patterns and clear separation between scheduling and queue logic, which helps keep code organized and easy to follow. Integration with coldkey swap is smart, preventing conflicts when owners schedule both actions, and access control is well thought, allowing subnet owners to schedule or cancel but not remove from queue, while root keeps full control. Defensive programming is used, like checking subnet existence and preventing duplicates, but the queue uses Vec with O(n2) removal, so performance could be better if VecDeque is used instead for O(1) operations. There is no max size limit for the queue, which might be risky if someone tries to fill it, and scheduler slots are consumed with each schedule, so rate limiting could be considered. Error handling is a bit inconsistent, some functions return bool while others use Result, and force_set_deregistration_priority could check subnet existence before enqueueing. Documentation is not very clear about the 5day delay and queue order, and some storage items need more comments. Overall, it’s a strong feature with solid architecture and testing, but needs fixes in performance, security, and documentation before merge. One unique idea is to add a warning event when the queue is near full, so admins can monitor for potential abuse, and maybe allow configurable delay for different subnets. |
| pub type PendingServerEmission<T> = | ||
| StorageMap<_, Identity, NetUid, AlphaCurrency, ValueQuery, DefaultZeroAlpha<T>>; | ||
|
|
||
| /// --- MAP ( netuid ) --> pending_emission |
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.
Why?
Description
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
./scripts/fix_rust.shto 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.