dalek-cryptography / bulletproofs

A pure-Rust implementation of Bulletproofs using Ristretto.
MIT License
1.05k stars 216 forks source link

Rename R1CSProof/R1CSError to Proof/Error to use r1cs:: prefix instead #261

Closed greyspectrum closed 5 years ago

greyspectrum commented 5 years ago

This is an attempt to address #247. I'm quite new to Rust, so it's entirely possible that I misunderstood the intent of the issue (this PR is a simple search and replace on the type names). If that's the case, I apologize in advance and would be happy to work on this further.

cathieyun commented 5 years ago

This looks perfect to me - all instances of r1cs::R1CSProof and r1cs::R1CSError have been correctly renamed to r1cs::Proof and r1cs::Error.

I would give this an approval right away, but I think @oleganza should also take a look since he opened the issue and might have more things to add.

Thanks for your help @greyspectrum !

hdevalence commented 5 years ago

Right now we have RangeProof and R1CSProof for the range proofs and constraint system proofs. Since we can't change the name of the RangeProof without breaking stability, would it be confusing to have RangeProof and Proof? The constraint system proof type is disambiguated as r1cs::Proof by the module name, but only when the client code imports it that way -- I don't know if that's going to always be done, for instance in Rustdoc output.

greyspectrum commented 5 years ago

This may be helpful, potentially:

https://github.com/rust-lang/rfcs/pull/356

...although they acknowledge that this can cause confusion for common/popular type names.

oleganza commented 5 years ago

In the linked RFC proposal there's an interesting comment:

image

I personally prefer to not use renamed imports (except in pathological cases where otherwise things are too ugly), to make the code make sense locally w/o too much context. So if there's no more-or-less global entity "TcpSocket", then making it up in the imports line is going to add another entity to the cognitive load. So it's better to either have TcpSocket defined in its library this way, or if it's tcp::Socket, then use it as such, to make that link clear.

Now, if I am in the library author's shoes, then for me the question becomes: which one should we use (TcpSocket or tcp::Socket), assuming that I don't want to encourage imports as TcpSocket.

greyspectrum commented 5 years ago

If r1cs::Proof is potentially ambiguous, would the best alternative be to leave things as they are or preface all of the types in r1cs with the module name?

use bulletproofs::r1cs::{
    R1CSConstraintSystem, R1CSError, R1CSProof, R1CSProver, R1CSRandomizedConstraintSystem, R1CSVariable, R1CSVerifier,
};

It's awfully repetitive, but isn't ambiguous or inconsistent.

Edit: on second thought, that seems like the worst option (R1CSConstraintSystem is especially awful). Perhaps it was best as it was, all options seem somewhat suboptimal.