privacy-scaling-explorations / halo2

https://privacy-scaling-explorations.github.io/halo2/
Other
208 stars 129 forks source link

[post frontend-backend split] Discuss names for new types / functions #263

Closed ed255 closed 7 months ago

ed255 commented 9 months ago

With the frontend-backend split, new types and functions have been introduced that mirror existing functions and types. Let's discuss the best naming for those.

This is the status of the initial frontend-backend PR:

Split VS Non-Split API

New middleware types (not mirroring)

Simplified types that mirror types that have frontend/backend specific behaviour

Some initial discussion:

As you can see, the current naming is not fully consistent, so it can be improved!

Discussion 1: Suffixes or not? Since the mirrored types live under different paths, we could use colliding names. For example we could have

Dicussion 2: For all the types that are mirrored -> simplified, use the same prefix when they appear in the middleware. For example all mirrored types under middleware (seen in the 3rd group list) would have Mid as suffix, or another suffix we agree upon. So ArgumentV2 would be ArgumentMid and so on. Can you think of a better suffix?

Discussion 3: Non-split VS split API. Should we distinguish the new API with some suffix? Usually code will either use the old API or the new API, not both, so I think it's viable to have this, without being confusing:


Blocked by https://github.com/privacy-scaling-explorations/halo2/pull/254

adria0 commented 9 months ago

Some thoughts:

ed255 commented 9 months ago

Some thoughts:

Thanks for your thoughts!

  • Since we are splitting HALO2 in order to support different frontends and backends, eventually we will have frontend_xxxx and backend_yyyy, although also temporally, for now, frontend and backend will be just a good naming. So middleware is what is expected not going to change, so, I'm on NOT suffix middleware, if we have to suffix something because this is going to be the invariant over time.

That makes sense!

  • Anyway in general, I find suffix/prefix a little verbose when you are implementing the structs/functions that they are used in the same module, so I'm on letting the developer if wants to use middleware::Gate as GateMid or just middleware::Gate.

Regarding that, I'm afraid of confusing the developer when working with the codebase, because they may see a Gate and not know which one it is at first glance. They may see a function that requires a Gate and have a mental overhead to figure out which one it must be. I guess doing middleware::Gate as GateMid would be better, but we may end up with a mixed style, where we rename to avoid collision, but sometimes we only import middleware::Gate without renaming. With the suffix approach it's easy to grep for GateMid and get all places where it's used.

  • About the "should we distinguish the new API with some suffix?": I do not get what are you referring to... using different crates is a way to suffixing new API, is this about suffixing names or at module level?

What I mean is that currently we have these:

where the v2 is used to distinguish that this is the frontend-backend split API. But they could be:

And I think the latter is better. Even though keygen_pk, keygen_vk collides in name with the old API, they are never mixed in the same code.

duguorong009 commented 7 months ago

@ed255 I believe this issue is no longer needed, since we already merged frontend-backend split PRs. How about closing the issue as Done?

cc @davidnevadoc

ed255 commented 7 months ago

@ed255 I believe this issue is no longer needed, since we already merged frontend-backend split PRs. How about closing the issue as Done?

cc @davidnevadoc

Actually there was a missing thing, I just opened this PR https://github.com/privacy-scaling-explorations/halo2/pull/312 to resolve it, and after that we can close this issue :)