refactor: streamline some traits and bounds #207
Conversation
- the main Group trait now uses ::zkcrypto::group::Group - Refactored the usage of generic type associated 'Scalar' across multiple files and functions from 'G::Scalar' to fully qualified '<G as Group>::Scalar'. - No new features were added, functionality remained the same, changes were mostly aimed at improving type inference and handling.
huitseeker
changed the title
Thanks for the PR, @huitseeker! The first commit looks great! I've mixed feelings about the second commit. Could you please elaborate a bit more on what is the gain from taking a dependency on the group crate? I couldn't quite tell if it simplified the Nova code in some way. It looks like it made our code more verbose because we have to disambiguate the two Groups. |
Yes. That's what I was saying, though what we're disambiguating is the two Scalar associated types (which are constrained to be one and the same). Though the disambiguation can be done by chosing a different name for the associated type, rather than using a qualified form, the gains are overall limited to fewer explicit trait requirements ( I was hoping to inform #198, and for @CPerezz to share some insight. Otherwise, as I mentioned, I'm happy to revert the last commit. |
|
Hey! So I kinda see the point of @srinathsetty but I think once #198 is actually done, the syntax could definitely improve. Not sure which is the best way to go for now. Specially since #198 is not even prototyped yet. The main idea for me there is to have the |
This reverts commit 5ee0590.
|
@CPerezz @srinathsetty I've reverted the last commit, leaving addressing the group-based trait to @CPerezz when #198 is more fleshed out. |
|
@huitseeker will try to get at it this week! There's quiote a lot of work on HyperNova |
What this does
zkcrypto::group::Group(2nd commit),What this does not do
zkcrypto::group::GroupEncodingtrait, as the chosenGroupEncodingimplementations forPallasandVestaare the same ([u8;32]) and it would thus not be possible to implement two distinct instances ofCompressedGroupElementon this type, requiring exactly the same sort of wrapper as we have now.Partially helps with #198.
Happy to adapt depending on feedback:
Group::Scalarassociated type to Something Else - feel free to suggest -, alleviating the ambiguity withzkcrypto::group::Group::Scalarand allowing us to replace the qualified instances<G as Group>::ScalartoG::SomethingElse.grouptraits and createGroupExtfor the extra extensions of theGrouptrait #198) as I'm not sure it would help with Reduce duplicate code across different curve cycle providers #202.