Fix clippy warnings and format README

[kigawas]

Fix #72 Fix #74

[kigawas]

Readme was formatted by https://github.com/DavidAnson/vscode-markdownlint

[omershlo]

hi @kigawas - in my audit1 branch I also fixed clippy warnings a couple of days ago (its an audit branch and clippy is part of the audit). I do see that you have some good fixes in this PR as well as the new readme, but I am not sure how to merge them in my branch. Can you help please?

As a side comment we are working on adding tags which will make life easier for all.

[kigawas]

We can merge this PR first and resolve conflicts later

[omershlo]

@oleiba

[kigawas]

moved some dependencies to dev-dependencies, mainly rocket

[kigawas]

Now there're no obvious clippy warnings, but still need to improve

Let's leave it to others PRs in the future

[omershlo]

LGTM. great work.

[kigawas]

can you merge?

[omershlo]

yes - I just want to have another set of eyes for review first (cc: @gbenattar ) from one of the consumers of the library.

[kigawas]

no problem, actually I hope https://github.com/KZen-networks/curv/pull/52 can get merged first to resolve ring version conflicts....

[omershlo]

Ring conflicts will be resolved by removing ring from Curv. I don't see any other way. We will find someone to do it very soon.

[kigawas]

Ring conflicts will be resolved by removing ring from Curv. I don't see any other way. We will find someone to do it very soon.

If you use old merkle, it may also run into this problem, so the best solution is just to update all to latest

[omershlo]

worst case - we will either implement a merkle tree from scratch or remove it from the library.

[kigawas]

That'll be a long story maybe. If I were you I'll use dependabot to keep every library up to date

[omershlo]

done. I replaced ring with rust-crypto in this pr: https://github.com/KZen-networks/curv/pull/42 would be lovely if you could review it before I merge

[kigawas]

@omershlo https://github.com/KZen-networks/zk-paillier this repo also depends on ring, can you have a look at it?

[omershlo]

ah yes, it will be super easy to change as well. I will do it later today

[omershlo]

done. please review: https://github.com/KZen-networks/zk-paillier/pull/7

[kigawas]

Changed t, n from usize to u16

[kigawas]

@omershlo https://github.com/mortendahl/rust-paillier and its fork https://github.com/KZen-networks/rust-paillier

this repo also takes the old ring, can you have a look :smiley:

[omershlo]

sure, rust-paillier uses Ring the same way zk-paillier used (copy paste). I can do it in a few days. Thanks for noticing.

[kigawas]

@omershlo I just did a fix on rust-paillier

https://github.com/mortendahl/rust-paillier/pull/68

and

https://github.com/KZen-networks/rust-paillier/pull/2

[kigawas]

Looks like cache: cargo gets it stuck

https://travis-ci.org/KZen-networks/multi-party-ecdsa/builds/583438826#L183

Removed it for now

[kigawas]

@omershlo IMO it's safe to merge this, since there's no logic change

[omershlo]

ok, thanks. will do

[omershlo]

HI @kigawas : 1) I see that in some places you removed some derived traits, and add debug derive. why is that? if possible I prefer to not change the derives unless it is a must. I see it for example in GG mta.rs 2) It is hard to review GG test.rs but it looks like you renamed some variables and added some as well. can you be more specific as to what are the changes you made and why?

[kigawas]

For 1, mainly in this commit https://github.com/KZen-networks/multi-party-ecdsa/pull/71/commits/8a7d1a5d501b6cc66170b17c88fd26506dcb5e56

Rational: Only deriving ParitialEq for part of messages is not consistent, actually, the comparing equal part should be left to user. Like this

use multi_party_ecdsa::protocols::multi_party_ecdsa::gg_2018::party_i::{
    KeyGenBroadcastMessage1 as KeyGenCommit, KeyGenDecommitMessage1 as KeyGenDecommit,
};
use curv::cryptographic_primitives::proofs::sigma_dlog::DLogProof;
use curv::cryptographic_primitives::secret_sharing::feldman_vss::VerifiableSS;
use curv::FE;

#[derive(Clone, Debug, Serialize, Deserialize)]
pub enum KeyGenMessage {
    CommitAndDecommit(PeerIndex, KeyGenCommit, KeyGenDecommit),
    VSS(PeerIndex, VerifiableSS),
    SecretShare(PeerIndex, FE),
    Proof(PeerIndex, DLogProof),
}

impl PartialEq for KeyGenMessage {
    fn eq(&self, other: &Self) -> bool {
        match (self, other) {
            (Self::CommitAndDecommit(ia, ca, da), Self::CommitAndDecommit(ib, cb, db)) => {
                ia == ib
                    && ca.com == cb.com && ca.e == cb.e
                    && da.blind_factor == db.blind_factor
                    && da.y_i == db.y_i
            }
            (Self::VSS(ia, vssa), Self::VSS(ib, vssb)) => ia == ib && vssa == vssb,
            (Self::SecretShare(ia, ssa), Self::SecretShare(ib, ssb)) => ia == ib && ssa == ssb,
            (Self::Proof(ia, pa), Self::Proof(ib, pb)) => ia == ib && pa == pb,
            _ => false,
        }
    }
}

[kigawas]

For 2, I only remove the redundant mod like

// test.rs
#[cfg(test)]
mod tests {  // <- no need
    fn test_a() {}
    fn test_b() {}
}

// lib.rs
mod test;

to

// test.rs
fn test_a() {}
fn test_b() {}

// lib.rs
#[cfg(test)]
mod test;

[kigawas]

This should also be applied to bench

[kigawas]

Also add test of checking signature against other secp256k1 library (https://github.com/sorpaas/libsecp256k1-rs)

https://github.com/KZen-networks/multi-party-ecdsa/pull/71/commits/cce7832fd9797b921d13865fc02673927e8aa17d

[omershlo]

1) some examples for new variables: raw_msg raw_pk compact

I don't understand why they are necessary ?

2)I think that It is impossible to implement PatrtialEq from outside the library if some elements in the struct are private. Usually when there's a PartialEq derive it means that it is was required by some other library. Therefore I am not very excited with removing them for now

[kigawas]

If you add more variables, you have to add new names

Simply deriving PartialEq is not enough, user has to adapt it to his own needs, which means that comparing two messages are equal or what makes them equal should be left to user's implementation. It's certainly possible, the example above was copied from real codes I'm writing.

And if you define "messages", obviously they are used outside, and their fields should be public.

[kigawas]

Of course, deriving PartialEq for all messages is also a solution, which makes user write less codes.

I have no preference for this though, both of them are okay

[omershlo]

@kigawas is it possible to divide this PR into few smaller PRs? I think it will be easier to digest and merge faster overall.

[kigawas]

@omershlo mainly 3 parts

1. fix clippy warnings and switch to rust 2018
2. refactor test and demo
3. minor derive traits change

you can check it by commit

[omershlo]

Hi @kigawas , we recently merged a PR containing known issues raised by the audit. Can you take the latest v0.2.4 into this PR?

@oleiba do you still have open issues here?

[omershlo]

@kigawas I see you changed the param file to params.json. In that case can you also provide such file in the repo (replacing the old params file) ?

[omershlo]

I finished my review. I approve once the comments above are resolved. I am also wondering if we should add to this pr #72 ... wdyt?