Fixed some spelling issues and generally made the code follow cryptosystem naming conventions:
changed Cypher -> Ciphertext
PrivateKey -> SecretKey
ZNP -> ZKP (since it's zero-knowledge proofs...)
Also made the test for Dec(Enc(m)) = m more robust by generating keys on each run (instead of having a fixed keypair) as I ran into an issue where the previous tests passed.
Fixed some spelling issues and generally made the code follow cryptosystem naming conventions:
1) changed Cypher -> Ciphertext
2) PrivateKey -> SecretKey
3) ZNP -> ZKP (since it's zero-knowledge proofs...)
Also made the test for Dec(Enc(m)) = m more robust by generating keys on each run (instead of having a fixed keypair) as I ran into an issue where the previous tests passed.
@sachaservan I believe all your renaming are good. Often, renaming apis break things for no good reason. Generally speaking, we all live well with the horrible naming surrounding us (who called that language "Go"?!)
I have asked the users of the package their point of view. Just to be 100% sure everyone agree on the changes. I presume you work with them but I am not sure.
Otherwise, fantastic work. Thanks for the pull request. Everything is good.
cheers!
didier
@sachaservan I believe all your renaming are good. Often, renaming apis break things for no good reason. Generally speaking, we all live well with the horrible naming surrounding us (who called that language "Go"?!)
I have asked the users of the package their point of view. Just to be 100% sure everyone agree on the changes. I presume you work with them but I am not sure.
Otherwise, fantastic work. Thanks for the pull request. Everything is good.
cheers!
didier
I don't find couple of changes necessary.
In my opinion privateKey name is fine and changing Cypher to Cyphertext has impact on us - please see details in comments. I'll continue the review when we agree on solution for the commented things.
@didiercrunch thank you for drawing our attention, we don't work with @sachaservan.
I don't find couple of changes necessary.
In my opinion `privateKey` name is fine and changing `Cypher` to `Cyphertext` has impact on us - please see details in comments. I'll continue the review when we agree on solution for the commented things.
@didiercrunch thank you for drawing our attention, we don't work with @sachaservan.
I'm not sure if we need to change this name. Cypher is an internal struct name specific for this implementation. C field is more eligible to change to Ciphertext in my opinion.
Plus changing it makes a big mess and has a great impact on projects utilizing this repo.
I'm not sure if we need to change this name. `Cypher` is an internal struct name specific for this implementation. `C` field is more eligible to change to `Ciphertext ` in my opinion.
Plus changing it makes a big mess and has a great impact on projects utilizing this repo.
I don't think that we need to generate p and q values for most of tests. In some tests values of p and q are fixed to reduce the time of execution since randomness is not required in most cases.
I don't think that we need to generate `p` and `q` values for most of tests. In some tests values of `p` and `q` are fixed to reduce the time of execution since randomness is not required in most cases.
Anyways `p` and `q` are not completely random values, and there is already a function to generate them: https://github.com/didiercrunch/paillier/blob/753322e473bf8ee20267c7824e68ae47360cc69b/safe_prime_generator.go#L61
In the end, I believe we all agree that this pull request does not bring new features and add backward incompatibilities. Consequently, I will close it without merging the pull request.
I am very sorry because I believe the pull request brought nicer code and fixed my horrible spelling. Unfortunately, at a too high cost.
cheers,
didier
In the end, I believe we all agree that this pull request does not bring new features and add backward incompatibilities. Consequently, I will close it without merging the pull request.
I am very sorry because I believe the pull request brought nicer code and fixed my horrible spelling. Unfortunately, at a too high cost.
cheers,
didier
Some thoughts from my side: if we want to change the naming at some point, we should use Go type aliases. This will allow for the adoption of the new naming and on the other side will not be a huge breaking change for existing projects using the library. The upgrade is possible but does not have to be done at once.
Some thoughts from my side: if we want to change the naming at some point, we should use Go type aliases. This will allow for the adoption of the new naming and on the other side will not be a huge breaking change for existing projects using the library. The upgrade is possible but does not have to be done at once.
Absolutely agree with @nkuba here. I understand the intention was to make those tests more real-world, but for the real-world Paillier, we'll probably use 1024 or 2048 modulus and this would have a significant impact on tests performance.
Absolutely agree with @nkuba here. I understand the intention was to make those tests more real-world, but for the real-world Paillier, we'll probably use 1024 or 2048 modulus and this would have a significant impact on tests performance.
Also made the test for Dec(Enc(m)) = m more robust by generating keys on each run (instead of having a fixed keypair) as I ran into an issue where the previous tests passed.
@sachaservan What was the issue?
> Also made the test for Dec(Enc(m)) = m more robust by generating keys on each run (instead of having a fixed keypair) as I ran into an issue where the previous tests passed.
@sachaservan What was the issue?
Pull request closed
Please reopen this pull request to perform a merge.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixed some spelling issues and generally made the code follow cryptosystem naming conventions:
Also made the test for Dec(Enc(m)) = m more robust by generating keys on each run (instead of having a fixed keypair) as I ran into an issue where the previous tests passed.
@pdyraga @nkuba, are you fine with those backward incompatible changes?
@sachaservan I believe all your renaming are good. Often, renaming apis break things for no good reason. Generally speaking, we all live well with the horrible naming surrounding us (who called that language "Go"?!)
I have asked the users of the package their point of view. Just to be 100% sure everyone agree on the changes. I presume you work with them but I am not sure.
Otherwise, fantastic work. Thanks for the pull request. Everything is good.
cheers!
didier
I don't find couple of changes necessary.
In my opinion
privateKeyname is fine and changingCyphertoCyphertexthas impact on us - please see details in comments. I'll continue the review when we agree on solution for the commented things.@didiercrunch thank you for drawing our attention, we don't work with @sachaservan.
@@ -163,2 +162,4 @@func CreateSecretKey(p, q *big.Int) *SecretKey {n := new(big.Int).Mul(p, q)lambda := computePhi(p, q)g := new(big.Int).Add(n, big.NewInt(1))Paillier is an asymmetric cryptographic algorithm so the
PrivateKeyis 100% fine for this, there is no need to change it.I'm not sure if we need to change this name.
Cypheris an internal struct name specific for this implementation.Cfield is more eligible to change toCiphertextin my opinion.Plus changing it makes a big mess and has a great impact on projects utilizing this repo.
I don't think that we need to generate
pandqvalues for most of tests. In some tests values ofpandqare fixed to reduce the time of execution since randomness is not required in most cases.Anyways
pandqare not completely random values, and there is already a function to generate them: https://github.com/didiercrunch/paillier/blob/753322e473bf8ee20267c7824e68ae47360cc69b/safe_prime_generator.go#L61In the end, I believe we all agree that this pull request does not bring new features and add backward incompatibilities. Consequently, I will close it without merging the pull request.
I am very sorry because I believe the pull request brought nicer code and fixed my horrible spelling. Unfortunately, at a too high cost.
cheers,
didier
Some thoughts from my side: if we want to change the naming at some point, we should use Go type aliases. This will allow for the adoption of the new naming and on the other side will not be a huge breaking change for existing projects using the library. The upgrade is possible but does not have to be done at once.
Absolutely agree with @nkuba here. I understand the intention was to make those tests more real-world, but for the real-world Paillier, we'll probably use 1024 or 2048 modulus and this would have a significant impact on tests performance.
@sachaservan What was the issue?
Pull request closed