Skip to content
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

22 #64

Closed
wants to merge 12 commits into from
Closed

22 #64

wants to merge 12 commits into from

Conversation

skaunov
Copy link
Collaborator

@skaunov skaunov commented Oct 19, 2023

tests weren't touched on purpose to raise additional awareness of those who will be modifying them to recheck implementations against each other

this.

@Divide-By-0 any comments while I didn't start to apply it more widely?
[discussion]: 251fba6#commitcomment-130400727

@Divide-By-0 : just to let to have one more look at the thing while
I continue.
Fun fact: I already was confused by that "hash2" naming, lol.

[discussion]: 251fba6#commitcomment-130400727

@Divide-By-0 : just to let to have one more look at the thing while
I continue.
I need to think a bit if this `fn` [can benefit]
from renaming at all currently.
My current best bet to deal with it in line with #22 is to add agreed
naming suffix with `_computed` to the non-excessive assignment.

[can benefit]: #61
@skaunov
Copy link
Collaborator Author

skaunov commented Oct 19, 2023

I should notice another mildly confusing naming: s and sk. =(

@Divide-By-0
Copy link
Member

No one's using tests yet, feel free to edit those. S isn't ideal I agree, open to anything else.

@Divide-By-0 , I guess it's better to give a look to this one
particular commit; I mean Circom code first of all ofc.

Tomorrow will be squashing, and filling PR.
@Divide-By-0
Copy link
Member

For circom, the code is way cleaner if you do this: #48

Can you merge in that PR first, compile the circuits to make sure they work? It will make circom easier to read and maintain.

@skaunov
Copy link
Collaborator Author

skaunov commented Oct 20, 2023

Can you merge in that PR first, compile the circuits to make sure they work? It will make circom easier to read and maintain.

Ok, am running test suite rn. Merging amount is unexpected. 🤯

@skaunov skaunov marked this pull request as ready for review October 20, 2023 10:11
This was referenced Oct 20, 2023
@skaunov
Copy link
Collaborator Author

skaunov commented Oct 20, 2023

No one's using tests yet, feel free to edit those. S isn't ideal I agree, open to anything else.

I meant that I think it's actually good to preserve reasonable cognitive load in tests. I updated parts which were too confusing; though they're relatively rough and unpolished for good.

I'll think further about "S" though you can see absence of suggestions on my side. X)

@skaunov
Copy link
Collaborator Author

skaunov commented Oct 20, 2023

To be merged via #66 due to merging conflict.

@skaunov skaunov closed this Oct 20, 2023
@skaunov skaunov deleted the 22 branch October 20, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants