feat(nns): Count stale and expired voting power neurons. #3090
Conversation
Most of the changes here are for breaking out voting power into deciding vs. potential, and copying those around, because of duplicate types. Another metric in this PR: When the current prune following cycle began. # References This is part of the "periodic confirmation" feature that was recently approved in proposal [132411][prop]. [prop]: https://dashboard.internetcomputer.org/proposal/132411 [Next PR 👉][next] [next]: #3090
daniel-wong-dfinity-org
force-pushed
the
count-stale-and-expired-voting-power-neurons-daniel-wong
branch
from
deed965 to
c5239e0
Compare
daniel-wong-dfinity-org
removed this pull request from the merge queue due to a manual request
| // Here, we assume that the neuron was not refreshed in the future. | ||
| // This doesn't always hold in tests though, due to the difficulty | ||
| // of constructing realistic data/scenarios. | ||
| now_seconds.saturating_sub(neuron.voting_power_refreshed_timestamp_seconds()); |
There was a problem hiding this comment.
We have a comment interrupting setting a variable here - could we move the comment up above the let statement?
There was a problem hiding this comment.
Will do.
I think this is slightly better though. In general, comments should be as close to the thing that they are talking about as possible. In particular, this comment is not about the variable; it is about the expression (that we use to set the variable). More specifically, this comment tries to justify that use of saturating_sub (which is unusual).
| // of constructing realistic data/scenarios. | ||
| now_seconds.saturating_sub(neuron.voting_power_refreshed_timestamp_seconds()); | ||
| let Some(seconds_losing_voting_power) = seconds_since_voting_power_refreshed | ||
| .checked_sub(voting_power_economics.get_start_reducing_voting_power_after_seconds()) |
There was a problem hiding this comment.
This is kind of a clever way to do this, but wouldn't it be simpler to use a comparison?
There was a problem hiding this comment.
Will do.
Is this really exotic??
Yes, None is not as explicit as observed < limit.
For better or worse, we have pledged to Andy to use checked_sub (et. al.) instead of the - operator. Therefore, I assume the team is familiar with checked_sub. In that case, I think this should not be considered particularly "clever".
Tangent
In an ideal world, we wouldn't use u64 et. al. Instead, we should be like Motoko and Python (and to a lesser degree, the Google C++ style guide): Int. Int is closed under subtraction (and all the other math operations, except divide by 0 is ofc undefined), just like math. In that case, I would consider doing it like this:
let excess = observed - limit; // no checked_sub tyranny
if excess < 0 {
return;
}
if excess < uber_limit {
// merely stale
} else {
// expired
}
max-dfinity
deleted the
count-stale-and-expired-voting-power-neurons-daniel-wong
branch
|
omnibus follow up ticket: https://dfinity.atlassian.net/browse/NNS1-3446 |
Here,
I think the main thing we really care about here is the number of such neurons in those two (sub)categories. However, it is not much harder to gather additional statistics (thanks to existing code for compiling statistics for arbitrary neuron (sub)categories). Therefore, a whole bunch of other stats about these two subcategories of neurons is also exported.
FWIW, if you strip away the highly repetitive code (and don't count test code), this change is actually pretty small.
References
This is part of the "periodic confirmation" feature that was recently approved in proposal 132411.
Closes NNS1-3418.
👈 Previous PR