Skip to content

Conversation

@yelhousni
Copy link
Collaborator

@yelhousni yelhousni commented Jun 20, 2024

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 type: 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
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

Copy link
Collaborator

@ivokub ivokub left a comment

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
@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

Labels

type: cleanup Suggestion to clean up the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants