-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unify sponge gadget #150
Unify sponge gadget #150
Conversation
Co-authored-by: Alex Xiong <[email protected]>
+ add warning comments
as per PR review suggestion
@chancharles92 Yes |
} | ||
|
||
impl<F> RescueGadget<F> for PlonkCircuit<F> | ||
impl<F> RescueGadget<RescueStateVar, F, F> for PlonkCircuit<F> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be helpful to have a type-alias or wrapper for the native/non-native gadget's API?
E.g., currently, we need to write: RescueGadget::<RescueStateVar, F, F>::rescue_sponge_no_padding
.
It'll be great if the user only needs to write RescueNativeGadget::<F>::rescue_sponge_no_padding
.
Similarly, for non-native setting, it'll be great if the user only needs to write RescueNonNativeGadget<F, T>::rescue_sponge_no_padding
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would indeed improve the ergonomics of using the gadget. I'll have a look, thx for the suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM. Better if Binyi's comment is addressed.
@chancharles92 I've introduced a type alias for the trait. |
So trait alias feature is available in rust nightly (see unstable book) Because after all it's an internal ergonomic issue, and we can tolerate this temporarily imo. |
Can we create new traits |
No, unfortunately not cleanly, I tried that before as well. I think it's the same issue under the hood, we will run into needing to use the trait as dynamic. For example, we could define:
But then it requires you to use it as:
I think the current approach is better in that we explicitly define it as If we decide to go with current design, we can definitely add a tracking issue in jellyfish for a trait alias. |
Alright, "keeping the current design" sounds good to me! I'll leave the issue creation to you Marti, thanks! |
I see, sounds good to me. Let's add an issue for that, then we can approve. |
Tracking issue here: #165. |
Hmm I thought we were sticking with the type alias. That was the "current solution" at the time of writing :) |
Description
Builds on top of #146. Still WIP, but I'd like to get some comments early on to avoid too much (or rather: even more...) refactoring later.
Refactor native & non-native sponge gadgets:
RescueStateVarGen
traitGenericStateVar
would be better? Other ideas?SpongeGadget
trait intomod.rs
, generic overRescueStateVarGen
PermutationGadget
trait intomod.rs
, generic overRescueStateVarGen
Downside:
RescueGadget
, the downstream users need to disambiguate the call, e.g. instead of:now you need to write
I feel that the ergonomics of the call is worse.
If you have any idea on how to keep a single
SpongeGadget
trait instead ofRescueGadget/RescueNonNativeGadget
, but without requiring disambiguation on every single call, please share :)On the other hand, we should soon be able to benefit from being generic, for example: generic over a
DigestAlgorithmGadget
in the implementation of theMerkleTreeGadget
, in line with #142:closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pending
section inCHANGELOG.md
Files changed
in the GitHub PR explorer