feat(nns): Added (potential|deciding)_voting_power fields to API. #2880
Conversation
…ate conversion from (native) Neuron to NeuronProto. The new conversion takes another argument (time). Therefore, the old interface cannot be maintained.
This comment was marked as resolved.
This comment was marked as resolved.
rs/nns/governance/proto/ic_nns_governance/pb/v1/governance.proto
Outdated
Show resolved
Hide resolved
| @@ -26,7 +26,7 @@ use ic_nervous_system_common::{ | |||
| cmc::CMC, | |||
| ledger, | |||
| ledger::{compute_neuron_staking_subaccount_bytes, IcpLedger}, | |||
| NervousSystemError, E8, ONE_DAY_SECONDS, ONE_YEAR_SECONDS, | |||
| NervousSystemError, E8, ONE_DAY_SECONDS, ONE_MONTH_SECONDS, ONE_YEAR_SECONDS, | |||
There was a problem hiding this comment.
NCR;
I don't like that we have both these constants at the same time. Why can't we use just one of them?
Is ONE_MONTH_SECONDS * 12 == ONE_YEAR_SECONDS?
There was a problem hiding this comment.
Is
ONE_MONTH_SECONDS * 12 == ONE_YEAR_SECONDS?
Yes. This is an abomination. However, this is already a public aspect of the NNS -> it makes sense for us to have a constant.
We certainly could have just ONE_YEAR_SECONDS, but integer division is kind of a pain (because you then need to think about whether there is any remainder). It just so happens that ONE_YEAR_SECONDS IS in fact divisible by 12, but this is non-obvious to most people, esp if you tell them one year is nominally defined as EXACTLY 365.25 days.
There was a problem hiding this comment.
Okay, so if the equality holds, that's good.
Maybe we should add a test to keep it that way?
NCR, as this PR didn't introduce those constants.
There was a problem hiding this comment.
Ok. Let me do that in another PR.
| @@ -314,6 +314,9 @@ fn allocate_neuron(id: u64) -> Neuron { | |||
| neuron_type: None, | |||
| visibility: None, | |||
| voting_power_refreshed_timestamp_seconds: None, | |||
| // These are ignored, because they are derived. | |||
There was a problem hiding this comment.
Nit. Could you mention in the comment which operation is deriving them?
There was a problem hiding this comment.
Not exactly sure what you mean...
Internally, whenever (native) Neuron is converted to NeuronProto (defined in governance.proto), it always populates these fields. Then, whenever NeuronProto is converted to api Neuron (defined in the rs/nns/governance/api dir), these are simply copied. But (native) Neuron is an internal concept -> I don't see how this explanation would relate to what this comment currently says. Are you just saying that the comment should (additionally) say (something to the effect of)
These are populated when converting from the native internal Neuron to Neuron as defined by governance.proto
?
Currently, you only need a few pieces of information to derive these:
- other neuron fields:
a. stake - This is the "base" of voting power. The rest is "just" bonus multipliers.
i. cached_stake_e8s
ii. fees_e8s
iii. staked_maturity_e8s_equivalent
b. aging since
c. dissolve delay - the current time (this is then used to determine current age and dissolve delay)
(In the future, you will also need some settings related to what happens when voting power has not been "refreshed" in a "long" time, but those can still be regarded as "constants", since they would generally not by changed by proposal.)
There was a problem hiding this comment.
We discussed over zoom, the code populating those fields is Neuron::into_proto
There was a problem hiding this comment.
Discussed in Zoom. Yes, what Arshavir was asking for is to add something like "These get derived in Neuron::into_proto.". In Zoom, I said, I would include this change as part of this PR. Arshavir said this is not important enough to invalidate passing tests. Therefore, I now change my mind: I will make this change, but in another PR. That way, this PR can be merged right away.
? I don't see any such feedback. I guess you mean that you were planning to bring them up in discussion. I think LGTM means "no need for additional discussion; merge at will". (At least, this is what it meant at Google.) |
You seem to have resolved my main outstanding question without a discussion, I assume it was by mistake. I used LGTM to indicate that I'm fine with merging the production changes you've made in this PR, but I have questions regarding the tests. Sorry for not being clearer. |
|
Ah. I think what happened is that you replied to one of my guide notes. I think I closed all of those immediately after posting them. (At least, that was my intention.) I think that as a result, I didn't see that feedback, until you linked to it in your previous msg. |
daniel-wong-dfinity-org
deleted the
add-voting-power-fields-to-api-daniel-wong
branch
References
This is part of the "periodic confirmation" feature that was recently approved in proposal 132411.
Closes https://dfinity.atlassian.net/browse/NNS1-3430