-
Notifications
You must be signed in to change notification settings - Fork 168
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
Correct a comment and an error #358
Conversation
pallets/subtensor/src/block_step.rs
Outdated
@@ -517,7 +517,7 @@ impl<T: Config> Pallet<T> { | |||
} | |||
} | |||
|
|||
// Performs the burn adjustment by multiplying the current difficulty by the ratio ( reg_actual + reg_target / reg_target * reg_target ) | |||
// Performs the burn adjustment by multiplying the current burn by the ratio ( reg_actual + reg_target / reg_target * reg_target ) | |||
// We use I110F18 to avoid any overflows on u64. Also min_burn and max_burn bound the range. | |||
// | |||
pub fn adjust_burn( |
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.
Actually, the function name is off: It doesn't adjust anything, just returns updated value. Maybe we should rename it too? Something like get_upgated_burn
is more accurate.
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.
yes, the function doesn't update anything. just calculate the burn to be adjusted.
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.
yes let's do the rename then, could just be upgraded_burn
pallets/subtensor/src/block_step.rs
Outdated
@@ -517,7 +517,7 @@ impl<T: Config> Pallet<T> { | |||
} | |||
} | |||
|
|||
// Performs the burn adjustment by multiplying the current difficulty by the ratio ( reg_actual + reg_target / reg_target * reg_target ) | |||
// Performs the burn adjustment by multiplying the current burn by the ratio ( reg_actual + reg_target / reg_target * reg_target ) |
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.
Is updated burn calculated correctly? Why is there "target_registrations_per_interval + target_registrations_per_interval" in the code (addition vs. multiplication)?
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.
you are right. according to code, it should be addition. will update it also
fixes #342
Description
Find out an wrong error type and wrong comment.
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.