No side-effects in getters #39
Conversation
fix: idiomatic name for getters
Make signers a private member of AggregatorState Make state a private member on SignatureAggregator Prefer returning Result to asserts
Co-authored-by: Hamy Ratoanina <[email protected]>
change: move `dedup_signers` inside `include_signers` change: use `signers()` everywhere change: add a `TooManySigners` error
|
@Nashtare Is there anything else I can do to get this one over the finish line? |
| /// | ||
| /// # Returns | ||
| /// | ||
| /// A slice of [`Signer`]s. |
There was a problem hiding this comment.
I'd rather have it mention A reference to the internal list of Signer`s, to emphasize that it's borrowed
| /// # Panics | ||
| /// | ||
| /// If the [`signer.participant_index`] doesn't match the [`public_key.index`] . | ||
| /// # Returns error if the [`signer.participant_index`] does not match the |
There was a problem hiding this comment.
| /// # Returns error if the [`signer.participant_index`] does not match the | |
| /// # Errors if the [`signer.participant_index`] does not match the |
| // Deduplicate partipating [`Signer`]s and check that there is | ||
| // the correct number of signers given the [`ThresholdParameters`]. |
There was a problem hiding this comment.
the second part of the comment is kind of misleading, we're checking that it does not exceed the number of participants (as it technically can't have more parties involved than there are registered). It doesn't check against the threshold. But it doesn't check that we gathered enough signers to meet the threshold for instance
| Error::TooManySigners(signers, n_param) => { | ||
| write!( | ||
| f, | ||
| "Too many signers ({}) given the DKG instance parameters ({}).", |
There was a problem hiding this comment.
nit: parameters may refer to both N and T here, maybe emphasize that we're talking about the total number of participants? (although it may be implied, as we don't mind having more signers than the threshold, just out of clarity).
| ) -> FrostResult<C, ()> { | ||
| if participant_index != public_key.index { | ||
| return Err(Error::IndexMismatch(participant_index, public_key.index)); | ||
| } |
There was a problem hiding this comment.
It's fine as out of scope of this PR, but I think this is the kind of function signature that could be simplified. As the index is part of the PK, we could remove the need to pass participant_index. (granted that applications still enforce correct provenance of this message, as described in the docs).
There was a problem hiding this comment.
I agree. Happy to take a look at this in an upcoming PR.
|
@Nashtare Given the I don't have push access to this branch, would you be ok merging this as-is and I'll address the comments and concerns in a separate PR (by way of a fork, as a normal contributor)? |
|
Replaced by #43 |
The
get_signers()method has side effects, which is a bit of a code smell. The deduplicationand sanity checks should not run as part of a getter. The naming is not idiomatic in rust, where
get_prefixes are considered redundant. Finally, the deduplication method crashes the processif the dedup'd number of signers falls below the threshold, which is annoying to users of the
library.
Ideally the refactoring should continue one more step and run the deduplication&checks everytime a Signer is added, but that is future work.
This PR also makes the
signersa private member, and enforces sorting&deduplication in the setter, guaranteeing a valid list of signers at all times.