Introduce internal module

[alanefl]

Moves group, hash, proof, and util into their own internal module. This is so that library users must import from internal in order to fish out non-accumulator functionality from group, hash, proof, or util. I thought about naming this hazardous instead of internal, but the former seemed like overkill, especially considering that some users might actually want to use one or two operations from group, hash, etc.

I'd initially seen this on the 11th recommendation here (page 11), and had been discussing it for a while. Seems like a great, low-cost way to signal what APIs we're 100% OK with exposing (accumulator) and which we consider as "use with caution" (internal), since some people are bound to ignore documentation. Feedback welcome.

[eddiew]

I dislike the idea of introducing an extra layer of nesting with low functional value - we already export the accumulator and vector_commitment modules at the top level. So when a dev wants to use the library normally, they do

use accumulator::{Accumulator, VectorCommitment}

and when they want to use something internal they do

use accumulator::hash::hash_to_prime

The nesting that already exists is sufficient to signal that the nested modules are internal. (although we should probably not export the uint module at the top-level)

And re the idea of denoting the nesting as hazardous, our internal modules should be explicitly NOT hazardous to use. If we consider them hazardous, what are we doing using them ourselves?

[whaatt]

I agree mostly with Eddie; that said, we could make some of the "internal" modules private (i.e. mod x instead of pub mod x in lib.rs, and then just explicitly re-export the types/functions we want people to use.

For instance, we could explicitly re-export hash_to_prime and each of the group types.

[alanefl]

Maybe the purpose of the nesting didn't come across clearly in my initial comment.

@eddiew

I dislike the idea of introducing an extra layer of nesting with low functional value

The functional value of the nesting is a heavy signal to a careless end user that lower-level APIs should be used with caution, if at all. It is well known that people screw up crypto frequently because of permissive library APIs

The nesting that already exists is sufficient to signal that the nested modules are internal.

The proposed nesting is not only to signal that nested modules are internal, but that they should be used with caution, if at all, esp. to people that will not read documentation

And re the idea of denoting the nesting as hazardous, our internal modules should be explicitly NOT > hazardous to use. If we consider them hazardous, what are we doing using them ourselves?

This is a library, not an internal tool. The intended audience for this product are non crypto experts and the "hazardous" tag is a signal to them, not to us. Besides, it is not the routines themselves that are hazardous but how they could be misused by people that have not spent months thinking about them

---

Nesting may not be the most elegant way to make the APIs robust to misuse (@whaatt we can also do something like what you suggest). But I still think it's crucial we have some mechanism for not just making everything easily accessible/encouraged to an end-user

[whaatt]

Yeah, I think the best of both worlds is a mechanism that uses the Rust visibility modifiers (pub), in that it disallows use of internal components while avoiding the cruft of an extra namespace.

If it turns out later that we want to expose internal functionality, we can debate potential solutions (e.g. a separate crate or a namespace) at that point. The solution would also probably depend on the particular functionality; for instance, I wouldn't feel comfortable letting people create and use Rsa2048 elems since we treat {-x, x} as the same element.

[eddiew]

I hear your concern but I still think the proposed solution is too heavy handed. Better would be to clarify via documentation that nested modules are lower level.

It's not even really the case that they're unsafe - typical misuse of crypto libraries is from people rolling their own authentication or encryption schemes from low level components, which is kind of different from someone wanting to use hash_to_prime or something else from our library instead of the accumulator or vector_commitment primitives.

Essentially I think the value of the proposed nesting as a signal is overstated, and isn't worth the annoyance of working with it.

On Thu, Feb 28, 2019 at 2:06 PM Alan Flores-Lopez notifications@github.com wrote:

Maybe the purpose of the nesting didn't come across clearly in my initial comment.

@eddiew https://github.com/eddiew

I dislike the idea of introducing an extra layer of nesting with low functional value

The functional value of the nesting is a heavy signal to a careless end user that lower-level APIs should be used with caution, if at all. It is well known that people screw up crypto frequently because of permissive library APIs

The nesting that already exists is sufficient to signal that the nested modules are internal.

The proposed nesting is not only to signal that nested modules are internal, but that they should be used with caution, if at all, esp. to people that will not read documentation

And re the idea of denoting the nesting as hazardous, our internal modules should be explicitly NOT > hazardous to use. If we consider them hazardous, what are we doing using them ourselves?

This is a library, not an internal tool. The intended audience for this product are non crypto experts and the "hazardous" tag is a signal to them, not to us. Besides, it is not the routines themselves that are hazardous but how they could be misused by people that have not spent months thinking about them

Nesting may not be the most elegant way to make the APIs robust to misuse ( @whaatt https://github.com/whaatt we can also do something like what you suggest). But I still think it's crucial we have some mechanism for not just making everything easily accessible/encouraged to an end-user

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/cambrianlabs/accumulator/pull/31#issuecomment-468397067, or mute the thread https://github.com/notifications/unsubscribe-auth/ABsmxVzE9wPVZ7iX3k4e3oHcy-GpCvqOks5vSCi9gaJpZM4bTK8O .

[alanefl]

Ok, closing this PR for now. I'll add an issue to keep track of this conversation -- we can resolve it when we feel confident we've come up with a good solution (documentation/Rust visibility modifiers).