-
Notifications
You must be signed in to change notification settings - Fork 261
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
FixedBase
refactor: rename msm
-> mul
, declutter the interface and hide aux methods
#746
Conversation
Actually, I think |
What do you think about just moving this to the |
Sure, we can move it. I'm open to other refactors too, the question is how useful this method is. |
What other refactors are you thinking of, actually, aside from moving? |
In general this method is useful whenever you want to multiply the same element by different scalars. This is useful primarily in setup methods for, e.g., Groth16 or KZG, but only in testing. So I'd like to keep it, and just make it easier to use. What other refactors are you thinking of, actually, aside from moving? I'll take a stab at these changes, and then we can compare and see if they're worth it. |
9cc62a9
to
9b1655d
Compare
@mmagician I've added my changes; let me know if there's anything further you'd like to change. |
Looks good, I'd approve if I weren't the PR author 👍🏼 Thanks for the changes! |
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.
Thanks!
Description
I don't think it's worth doing a bigger refactor proposed in #432 - yet still the two MSMs we had were confusing people. This PR is sort of bridging this gap of having a documented, simple interface that is clearly distinct from
VariableBaseMSM
.I renamed
FixedBase::msm
intoFixedBase::mul
and introduces the following refactors:mul
private. (Edit:get_window_table
also remains public)mul
interface: takesg
and list of scalars as the parameters. The rest of the parameters can be inferred, and the internal methods called frommul
directly.@msmirnov18
closes: #432
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