Address review concerns from PR 39 (#43)

* add: Add a dedup_signers
fix: idiomatic name for getters

* fix: broken benches

* Do sorting and deduplication when adding signers
Make signers a private member of AggregatorState
Make state a private member on SignatureAggregator
Prefer returning Result to asserts

* Update src/error.rs

Co-authored-by: Hamy Ratoanina <[email protected]>

* Review feedback
change: move `dedup_signers` inside `include_signers`
change: use `signers()` everywhere
change: add a `TooManySigners` error

* chore: appease clippy

* chore: doc updates as per PR review

* Update src/sign/signature.rs

Co-authored-by: Robin Salen <[email protected]>

---------

Co-authored-by: Hamy Ratoanina <[email protected]>
Co-authored-by: Robin Salen <[email protected]>

benches/sign.rs

@@ -158,14 +158,16 @@ fn criterion_benchmark(c: &mut Criterion) {
     let mut aggregator = SignatureAggregator::new(params, group_key, &message[..]);
 
     for i in 1..THRESHOLD_OF_PARTICIPANTS + 1 {
-        aggregator.include_signer(
-            i,
-            participants_public_comshares[(i - 1) as usize].commitments[0],
-            &participants_secret_keys[(i - 1) as usize].to_public(),
-        );
+        aggregator
+            .include_signer(
+                i,
+                participants_public_comshares[(i - 1) as usize].commitments[0],
+                &participants_secret_keys[(i - 1) as usize].to_public(),
+            )
+            .unwrap();
     }
 
-    let signers = aggregator.get_signers().clone();
+    let signers = aggregator.signers().to_vec();
     let message_hash = Secp256k1Sha256::h4(&message[..]);
     let message_hash_copy = message_hash;
 

src/error.rs

@@ -27,6 +27,8 @@ pub enum Error<C: CipherSuite> {
     ComplaintVerificationError,
     /// The index of a participant is zero
     IndexIsZero,
+    /// The index of a signer does not match the index in the public key
+    IndexMismatch(u32, u32),
     /// GroupVerifyingKey generation failure
     InvalidGroupKey,
     /// Invalid NiZK proof of knowledge
@@ -45,6 +47,8 @@ pub enum Error<C: CipherSuite> {
     InvalidMSMParameters,
     /// Too many invalid participants, with their indices
     TooManyInvalidParticipants(Vec<u32>),
+    /// Too many unique signers given the [`crate::parameters::ThresholdParameters`].
+    TooManySigners(usize, u32),
     /// The participant is missing commitment shares
     MissingCommitmentShares,
     /// Invalid binding factor
@@ -76,10 +80,22 @@ impl<C: CipherSuite> core::fmt::Display for Error<C> {
             Error::ShareVerificationError => write!(f, "The secret share is not correct."),
             Error::ComplaintVerificationError => write!(f, "The complaint is not correct."),
             Error::IndexIsZero => write!(f, "The indexs of a participant cannot be 0."),
+            Error::IndexMismatch(participant_idx, pubkey_idx) => write!(
+                f,
+                "Index mismatch between participant index ({}) and the public key index ({}).",
+                participant_idx, pubkey_idx
+            ),
             Error::InvalidGroupKey => write!(
                 f,
                 "Could not generate a valid group key with the given commitments."
             ),
+            Error::TooManySigners(signers, n_param) => {
+                write!(
+                    f,
+                    "Too many signers ({}) given the DKG instance parameters (total participants set to {}).",
+                    signers, n_param
+                )
+            }
             Error::InvalidProofOfKnowledge => write!(
                 f,
                 "The NiZK proof of knowledge of the secret key is not correct."

src/lib.rs

@@ -1455,7 +1455,7 @@
 //! #
 //! # aggregator.include_signer(1, alice_public_comshares.commitments[0], &alice_public_key);
 //! # aggregator.include_signer(3, carol_public_comshares.commitments[0], &carol_public_key);
-//! let signers = aggregator.get_signers();
+//! let signers = aggregator.signers();
 //! # Ok(()) }
 //! # fn main() { assert!(do_test().is_ok()); }
 //! ```
@@ -1530,7 +1530,7 @@
 //! # aggregator.include_signer(1, alice_public_comshares.commitments[0], &alice_public_key);
 //! # aggregator.include_signer(3, carol_public_comshares.commitments[0], &carol_public_key);
 //! #
-//! # let signers = aggregator.get_signers();
+//! # let signers = aggregator.signers();
 //! # let message_hash = Secp256k1Sha256::h4(&message[..]);
 //!
 //! let alice_partial = alice_secret_key.sign(