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.

Sign up for GitHub

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

Jump to bottom

refactor: correct comments #511

Merged
merged 7 commits into from
Jul 19, 2024
Merged

refactor: correct comments #511

merged 7 commits into from
Jul 19, 2024

Conversation

yelhousni
Copy link
Collaborator

@yelhousni yelhousni commented Jun 20, 2024 •
edited

Description

Placeholder PR to correct comments and add missing ones.

Type of change

  • This change requires a documentation update

How has this been tested?

N/A

How has this been benchmarked?

N/A

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@yelhousni yelhousni added the cleanup Suggestion to clean up the code label Jun 20, 2024
@yelhousni yelhousni self-assigned this Jun 20, 2024
@yelhousni yelhousni marked this pull request as draft June 20, 2024 14:19
@yelhousni yelhousni changed the title refactor: correct IsZero comments refactor: correct comments Jun 20, 2024
@yelhousni yelhousni marked this pull request as ready for review July 2, 2024 14:02
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 2, 2024

📦 github.com/consensys/gnark-crypto/ecc/bls12-377/fr/permutation
🛑 build failed

📦 github.com/consensys/gnark-crypto/ecc/bls12-377/fr/plookup
🛑 build failed

📦 github.com/consensys/gnark-crypto/ecc/bls12-377/kzg
🛑 build failed

ecc/bls12-377/kzg/kzg.go:226:10: totalG1.JointScalarMultiplication undefined (type bls12377.G1Jac has no field or method JointScalarMultiplication)

📦 github.com/consensys/gnark-crypto/ecc/bls12-378/fr/fft
❌ TestBitReverse 0s

📦 github.com/consensys/gnark-crypto/ecc/bls12-378/fr/fri
❌ TestFRI 0s

+ verifying wrong opening should fail: OK, passed 10 tests.

📦 github.com/consensys/gnark-crypto/ecc/bls12-378/kzg

ecc/bls12-378/kzg/kzg.go:226:10: totalG1.JointScalarMultiplication undefined (type bls12378.G1Jac has no field or method JointScalarMultiplication)

📦 github.com/consensys/gnark-crypto/ecc/bls12-381
❌ TestG1AffineBatchScalarMultiplication 0s

❌ TestG1AffineCofactorCleaning 0s

❌ TestG1AffineConversions 0s

❌ TestG1AffineEndomorphism 0s

❌ TestG1AffineIsOnCurve 0s

❌ TestG1AffineOps 0s

📦 github.com/consensys/gnark-crypto/ecc/bls12-381/ecdsa
❌ TestECDSA 0s

📦 github.com/consensys/gnark-crypto/ecc/bls12-381/fp
❌ TestElementAdd 0s

❌ TestElementBatchInvert 0s

❌ TestElementBitLen 0s

❌ TestElementButterflies 0s

+ z.SetInterface must match z.SetString with int8: OK, passed 200 tests.
+ Neg: having the receiver as operand should output the same result: OK,
   passed 200 tests.
+ z.SetInterface must match z.SetString with int16: OK, passed 200 tests.
+ z.SetInt64 must match z.SetString: OK, passed 200 tests.

❌ TestElementBytes 0s

❌ TestElementDiv 0s

❌ TestElementDouble 0s

+ z.SetInterface must match z.SetString with int64: OK, passed 200 tests.
+ z.SetInterface must match z.SetString with int: OK, passed 200 tests.
+ z.SetInterface must match z.SetString with uint8: OK, passed 200 tests.
+ Div: having the receiver as operand should output the same result: OK,
   passed 200 tests.
+ z.SetInterface must match z.SetString with uint16: OK, passed 200 tests.
+ Double: having the receiver as operand should output the same result:
   OK, passed 200 tests.

❌ TestElementEqual 0s

❌ TestElementExp 0s

❌ TestElementFixedExp 0s

❌ TestElementHalve 0s

❌ TestElementInverse 0s

❌ TestElementInverseExp 0s

❌ TestElementLegendre 0s

❌ TestElementLexicographicallyLargest 0s

+ Assembly implementation must be consistent with generic one: OK, passed
   200 tests.
+ x.fromMont().toMont() == x: OK, passed 200 tests.

❌ TestElementMul 0s

❌ TestElementMulByConstants 0s

❌ TestElementNegativeExp 0s

❌ TestElementReduce 0s

❌ TestElementSelect 0s

❌ TestElementSetInterface 0s

❌ TestElementSqrt 0s

+ Neg: operation result must match big.Int result: OK, passed 200 tests.
+ z.SetInterface must match z.SetString with int32: OK, passed 200 tests.
+ Neg: operation result must be smaller than modulus: OK, passed 200 tests.

❌ TestElementSquare 0s

❌ TestElementSub 0s

📦 github.com/consensys/gnark-crypto/ecc/bls12-381/fr
❌ TestElementAdd 0s

❌ TestElementBatchInvert 0s

❌ TestElementBitLen 0s

❌ TestElementButterflies 0s

❌ TestElementBytes 0s

❌ TestElementDiv 0s

❌ TestElementDouble 0s

❌ TestElementEqual 0s

❌ TestElementExp 0s

❌ TestElementFixedExp 0s

❌ TestElementFromMont 0s

❌ TestElementHalve 0s

❌ TestElementInverse 0s

❌ TestElementInverseExp 0s

❌ TestElementLegendre 0s

+ Square: operation result must be smaller than modulus: OK, passed 200
   tests.
+ Mul: having the receiver as operand should output the same result: OK,
   passed 200 tests.
+ Sub: having the receiver as operand should output the same result: OK,
   passed 200 tests.
+ Add: having the receiver as operand should output the same result: OK,
   passed 200 tests.
+ Div: having the receiver as operand should output the same result: OK,
   passed 200 tests.

❌ TestElementLexicographicallyLargest 0s

❌ TestElementMul 0s

❌ TestElementMulByConstants 0s

❌ TestElementNeg 0s

❌ TestElementNegativeExp 0s

❌ TestElementNewElement 0s

❌ TestElementReduce 0s

❌ TestElementSelect 0s

❌ TestElementSetInt64 0s

❌ TestElementSetInterface 0s

❌ TestElementSqrt 0s

❌ TestElementSquare 0s

+ Square: having the receiver as operand should output the same result:
   OK, passed 200 tests.
+ Square: operation result must match big.Int result: OK, passed 200 tests.
+ reduce should output a result smaller than modulus: OK, passed 200 tests.

❌ TestElementSub 0s

📦 github.com/consensys/gnark-crypto/ecc/bls12-381/fr/fft
❌ TestBitReverse 0s

📦 github.com/consensys/gnark-crypto/ecc/bls12-381/fr/fri
❌ TestFRI 0s

📦 github.com/consensys/gnark-crypto/ecc/bls12-381/fr/gkr
❌ TestGkrVectors 0s

❌ TestGkrVectors/single_identity_gate_two_instances_prover 0s

❌ TestGkrVectors/single_identity_gate_two_instances_verifier 0s

❌ TestGkrVectors/single_input_two_identity_gates_two_instances_prover 0s

❌ TestGkrVectors/single_input_two_identity_gates_two_instances_verifier 0s

❌ TestGkrVectors/single_input_two_outs_two_instances_prover 0s

❌ TestGkrVectors/single_input_two_outs_two_instances_verifier 0s

❌ TestGkrVectors/single_mimc_gate_four_instances_prover 0s

❌ TestGkrVectors/single_mimc_gate_four_instances_verifier 0s

❌ TestGkrVectors/single_mimc_gate_two_instances_prover 0s

❌ TestGkrVectors/single_mimc_gate_two_instances_verifier 0s

ivokub
ivokub approved these changes Jul 3, 2024
View reviewed changes
Copy link
Collaborator

@ivokub ivokub left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But I guess we could keep the PR and merge when we merge with other audit fixes?

@gbotrel
Copy link
Collaborator

gbotrel commented Jul 18, 2024

should we merge @yelhousni ?

@yelhousni yelhousni merged commit 21087f2 into master Jul 19, 2024
7 checks passed
@yelhousni yelhousni deleted the docs/up-comments branch July 19, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivokub ivokub ivokub approved these changes

@ThomasPiellard ThomasPiellard Awaiting requested review from ThomasPiellard

@Tabaie Tabaie Awaiting requested review from Tabaie

Assignees

@yelhousni yelhousni

Labels
cleanup Suggestion to clean up the code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@yelhousni @gbotrel @ivokub