remove redundant stake field from private tally #756
Conversation
| } | ||
|
|
||
| pub fn private_encrypted(&self) -> Result<(&EncryptedTally, &Value), TallyError> { | ||
| pub fn private_encrypted(&self) -> Result<(&EncryptedTally, Value), TallyError> { |
There was a problem hiding this comment.
is the stake return value used somewhere? I don't think so, as we don't have such thing for the public tally
There was a problem hiding this comment.
It's used in ProposalManager somewhere, I guess it could probably just return a Result<&EncryptedTally>
chain-vote/src/tally.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn max_stake(&self) -> u64 { |
There was a problem hiding this comment.
I don't think this is necessary and exposing this only for EncryptedTallies seems a bit strange to me
| match self { | ||
| Self::Private { | ||
| state: PrivateTallyState::Encrypted { encrypted_tally }, | ||
| } => Ok(encrypted_tally.max_stake()), |
There was a problem hiding this comment.
can we get rid of the private_total_power function?
chain-vote/src/tally.rs
Outdated
| r: Vec<Ciphertext>, | ||
| fingerprint: ElectionFingerprint, | ||
| max_stake: u64, | ||
| pub max_stake: u64, |
There was a problem hiding this comment.
wait, how did this get public again?
There was a problem hiding this comment.
I thought that was what you were suggesting on the pub fn max_stake(&self) -> u64 above. I guess this way makes me a bit uneasy, since it makes it possible to corrupt the internal state, was there something else you had in mind? FWIW, the only use of the field outside the module is a single function in ProposalManager
There was a problem hiding this comment.
I don't think it should be needed anywhere outside of this module
chain-vote/src/tally.rs
Outdated
| r: Vec<Ciphertext>, | ||
| fingerprint: ElectionFingerprint, | ||
| max_stake: u64, | ||
| pub max_stake: u64, |
There was a problem hiding this comment.
I don't think it should be needed anywhere outside of this module
| } | ||
|
|
||
| if self.check((*total_stake).into(), governance, &result) { | ||
| if self.check(Stake(encrypted_tally.max_stake), governance, &result) { |
There was a problem hiding this comment.
I think it was a mistake also in the previous implementation, this should be the total available voting power, not the one that participated in the election (which can be obtained just by looking at the results)
| } | ||
|
|
||
| if self.check((*total_stake).into(), governance, &result) { | ||
| if self.check(result.participation(), governance, &result) { |
There was a problem hiding this comment.
Sorry if I was misleading, I meant to say we should use something like token_distribution.get_total(), which btw is what we do on master. Rebasing should fix this
zeegomo
left a comment
zeegomo
left a comment
There was a problem hiding this comment.
Please fix the lint, otherwise good to merge
As mentioned in #750 , the
total_stakefield inPrivateTallyState::Encryptedis redundant, since we track it inEncryptedTallynow. This PR removes the unnecessary field