Enable linter in the CI #527

K1li4nL · 2024-05-24T09:52:56Z

No description provided.

github-actions · 2024-05-24T09:53:07Z

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

github-actions · 2024-05-24T09:53:10Z

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

AnomalRoil

Any chances this could be rebased against drandmerge without including the x86 changes from Matteo branch ? (might have to do some git cherry-picking)

Now CI does the x86 checks in there already I think.

AnomalRoil · 2024-06-06T12:25:57Z

pairing/circl_bls12381/suite_test.go

-			//p = g.Point().Mul(g.Scalar().Pick(rand), nil)
-			//return
+			// p = g.Point().Mul(g.Scalar().Pick(rand), nil)
+			// return
 			/*}*/


this seems weird that it would all be commented. Has Pick been implemented for GT now maybe?

Looking at their repo, it seems they didn't, should we remove that code ?

AnomalRoil · 2024-06-06T12:27:01Z

proof/dleq/dleq.go

+	g kyber.Point,
+	h kyber.Point,


I think we want to stick to the convention that points are using Capital letters. Can you add an exception for that linter for kyber.Point elements?

So far I couldn't find anyway to add such exceptions, either we disable this check or add //nolint:... everywhere it is needed

I reverted these changes

AnomalRoil · 2024-06-06T12:27:11Z

proof/dleq/dleq.go

+	g []kyber.Point,
+	h []kyber.Point,


proof/hash.go

AnomalRoil

Ups, used the wrong button.

.golangci.yml

AnomalRoil · 2024-06-12T07:03:40Z

group/edwards25519/curve.go

 	digest[0] &= 0xf8
 	digest[31] &= 0x7f
 	digest[31] |= 0x40

-	secret := c.Scalar().(*scalar)
+	secret := c.Scalar().(*scalar) //nolint:errcheck // V4 may bring better error handling


This sounds like either a TODO, or an issue that should be created, no?

AnomalRoil · 2024-06-12T07:05:43Z

group/edwards25519/point.go

@@ -31,48 +31,48 @@ type point struct {
 	varTime bool
 }

-func (P *point) String() string {
+func (p *point) String() string {


Points should be capitalized.

AnomalRoil · 2024-06-12T07:06:12Z

group/edwards25519/point_vartime.go

@@ -4,6 +4,6 @@ package edwards25519
 // but variable time implementation can be used. Set this only on Points
 // which represent public information. Using variable time algorithms to
 // operate on private information can result in timing side-channels.
-func (P *point) AllowVarTime(varTime bool) {
-	P.varTime = varTime
+func (p *point) AllowVarTime(varTime bool) {


Points should be capitalized

AnomalRoil · 2024-06-12T07:08:25Z

group/p256/curve.go

@@ -99,10 +98,9 @@ func (p *curvePoint) Pick(rand cipher.Stream) kyber.Point {
 	return p.Embed(nil, rand)
 }

-// Pick a curve point containing a variable amount of embedded data.
+// Embed pick a curve point containing a variable amount of embedded data.


Suggested change

// Embed pick a curve point containing a variable amount of embedded data.

// Embed picks a curve point containing a variable amount of embedded data.

AnomalRoil · 2024-06-12T07:08:33Z

group/p256/curve.go

@@ -121,7 +119,7 @@ func (p *curvePoint) Embed(data []byte, rand cipher.Stream) kyber.Point {
 	}
 }

-// Extract embedded data from a curve point
+// Data extract embedded data from a curve point


Suggested change

// Data extract embedded data from a curve point

// Data extracts embedded data from a curve point

AnomalRoil · 2024-06-12T07:09:06Z

group/p256/curve.go

-func (p *curvePoint) Set(P kyber.Point) kyber.Point {
-	p.x = P.(*curvePoint).x
-	p.y = P.(*curvePoint).y
+func (p *curvePoint) Set(a kyber.Point) kyber.Point {


kyber.Point should be capitalized, arguably curvePoint too.

AnomalRoil · 2024-06-12T07:10:28Z

group/var_ed25519/curve.go

@@ -267,22 +269,19 @@ func (c *curve) onCurve(x, y *mod.Int) bool {

 // Sanity-check a point to ensure that it is on the curve
 // and within the appropriate subgroup.
-func (c *curve) validPoint(P point) bool {
+func (c *curve) validPoint(p point) bool {


AnomalRoil · 2024-06-12T07:10:36Z

group/var_ed25519/curve.go

@@ -295,7 +294,7 @@ func (c *curve) embedLen() int {

 // Pick a [pseudo-]random curve point with optional embedded data,
 // filling in the point's x,y coordinates
-func (c *curve) embed(P point, data []byte, rand cipher.Stream) {
+func (c *curve) embed(p point, data []byte, rand cipher.Stream) {


AnomalRoil · 2024-06-12T07:15:22Z

shuffle/simple.go

@@ -50,7 +50,7 @@ type SimpleShuffle struct {
 }

 // Simple helper to compute G^{ab-cd} for Theta vector computation.
-func thenc(grp kyber.Group, G kyber.Point,
+func thenc(grp kyber.Group, g kyber.Point,


AnomalRoil · 2024-06-13T09:55:55Z

proof/dleq/dleq.go

 	if len(G) != len(H) || len(H) != len(secrets) {
-		return nil, nil, nil, errorDifferentLengths
+		return nil, nil, nil, errDifferentLengths


Since this is an error return by a public method, the error should be exported as well to allow wrapping and unwrapping of error.

AnomalRoil · 2024-06-13T09:56:19Z

proof/dleq/dleq.go

@@ -131,7 +169,7 @@ func (p *Proof) Verify(suite Suite, G kyber.Point, H kyber.Point, xG kyber.Point
 	a := suite.Point().Add(rG, cxG)
 	b := suite.Point().Add(rH, cxH)
 	if !(p.VG.Equal(a) && p.VH.Equal(b)) {
-		return errorInvalidProof
+		return errInvalidProof


Same, please export this error

AnomalRoil · 2024-06-13T13:10:34Z

pairing/bn254/test_vectors.go

Could you move these to a proper test file? Maybe test_vectors_test.go ?

AnomalRoil · 2024-06-13T13:12:09Z

pairing/bn256/point.go

-	p = p.Clone().(*pointG1)
+	p, ok := p.Clone().(*pointG1)
+	if !ok {
+		return nil, errors.New("invalid type cast")


Make it an exported error ErrTypeCast since you re-use it multiple times.

AnomalRoil · 2024-06-13T13:14:19Z

proof/dleq/dleq.go

+var errDifferentLengths = errors.New("inputs of different lengths")
+var errInvalidProof = errors.New("invalid proof")


see below about exporting errors that are returned by exported methods.

AnomalRoil · 2024-06-13T13:33:02Z

share/pvss/pvss.go

@@ -152,16 +184,22 @@ func DecShareBatch(suite Suite, H kyber.Point, X []kyber.Point, sH []kyber.Point
 // log_{G}(X) == log_{sG}(sX). Note that X = xG and sX = s(xG) = x(sG).
 func VerifyDecShare(suite Suite, G kyber.Point, X kyber.Point, encShare *PubVerShare, decShare *PubVerShare) error {
 	if err := decShare.P.Verify(suite, G, decShare.S.V, X, encShare.S.V); err != nil {
-		return errorDecVerification
+		return errDecVerification


Can we change the signature of this function to have a bool?

Using errors to Verify cryptography is a bad idea:

https://www.youtube.com/watch?v=4opI0w0Xs5c

the other option would be to export that error and them wrap it with fmt.Errorf("invalid: %w", ErrDecVerification) so that the error cannot be "bypassed" but it's still possible to match on it.

share/pvss/pvss.go

share/dkg/pedersen/structs.go

sonarqubecloud · 2024-06-24T12:53:32Z

Quality Gate failed

Failed conditions
80.3% Duplication on New Code (required ≤ 10%)

See analysis details on SonarCloud

AnomalRoil

I'm like 108/110 files through... Will just have to test a few extra things wrt to logging changes I guess.

AnomalRoil · 2024-06-24T12:56:46Z

group/edwards25519/fe_test.go

-	b0 = b1.Mod(b1, prime)
+	b1 = b1.Mod(b1, prime)


nice catch. Would have been nicer to have a new test vector to detect it ;)

AnomalRoil · 2024-06-24T18:49:53Z

share/dkg/pedersen/dkg.go

-	d.c.Info(append([]interface{}{"generator"}, keyvals...))
+	d.c.Info("generator", keyvals)
 }

 func (d *DistKeyGenerator) Error(keyvals ...interface{}) {
-	d.c.Info(append([]interface{}{"generator"}, keyvals...))
+	d.c.Info("generator", keyvals)
 }

 func (c *Config) Info(keyvals ...interface{}) {
 	if c.Log != nil {
-		c.Log.Info(append([]interface{}{"dkg-log"}, keyvals...))
+		c.Log.Info("dkg-log", keyvals)
 	}
 }

 func (c *Config) Error(keyvals ...interface{}) {
 	if c.Log != nil {
-		c.Log.Error(append([]interface{}{"dkg-log"}, keyvals...))
+		c.Log.Error("dkg-log", keyvals)


I think I remember adding these at some point because our logger weren't working properly otherwise.

I'll need to test this locally actually with a couple loggers I guess T.T

K1li4nL added the safe PR label May 24, 2024

K1li4nL force-pushed the kilian-fix-ci-linter branch from 7cd82b7 to bb528f3 Compare May 24, 2024 10:00

K1li4nL changed the base branch from drandmerge to ci-32bit-update May 24, 2024 10:02

K1li4nL marked this pull request as ready for review May 31, 2024 14:48

K1li4nL force-pushed the kilian-fix-ci-linter branch 2 times, most recently from 65139ba to 704ecd1 Compare June 2, 2024 11:55

K1li4nL changed the base branch from ci-32bit-update to matteo-int-xplatform June 2, 2024 11:55

K1li4nL force-pushed the kilian-fix-ci-linter branch from 2f7e39e to 539bc4c Compare June 6, 2024 06:12

K1li4nL requested a review from pierluca June 6, 2024 06:23

AnomalRoil approved these changes Jun 6, 2024

View reviewed changes

AnomalRoil requested changes Jun 6, 2024

View reviewed changes

K1li4nL force-pushed the kilian-fix-ci-linter branch from edda608 to 5689ca1 Compare June 11, 2024 06:10

K1li4nL changed the base branch from matteo-int-xplatform to drandmerge June 11, 2024 06:10

K1li4nL force-pushed the kilian-fix-ci-linter branch from 5689ca1 to f71a11f Compare June 11, 2024 06:36

AnomalRoil reviewed Jun 12, 2024

View reviewed changes

K1li4nL force-pushed the kilian-fix-ci-linter branch from 56524c5 to ecba6df Compare June 12, 2024 09:20

AnomalRoil requested changes Jun 13, 2024

View reviewed changes

K1li4nL force-pushed the kilian-fix-ci-linter branch from ac0ecb3 to c4d497b Compare June 14, 2024 21:14

K1li4nL added 10 commits June 24, 2024 14:50

Setup golangci action

52eb401

Enable golangci-lint contextcheck

8b26390

Add missing error handling

ce3e597

Rename unused parameters

2d85251

ignore IDE .idea

dae9ffc

Ignore interface bloat in group.go

fbd289c

Fix some formatting

9fc0d79

Add error handling

757dd4a

Fix asalint

cd5d67e

minor fixes

c3bc175

K1li4nL added 25 commits June 24, 2024 14:50

Lint fix curve25519

d8b38c0

Lint fix xof package

e56fec2

Update golangci rules

67ff4a5

Better var name

2e142d7

Add missing error handling

adfe6eb

Update golangci rules

36ffc7b

Remove useless nolint

96cf62e

ignore / fix lint errors

aabb0b8

Allow capital var name for point convention

1530249

Make points name capital

9f9e0f7

Errors fix

35a55aa

Typos

f8d0235

Remove unused lint rules

496168c

Remove unnecessary checks

4256cb3

Export dleq errors

00b5900

Rename test file

b1a18ef

Export cast error

fee07a2

Wrap errors

a8185f2

Minor refactoring

99e4f5c

Export errors

27d3630

Multiple fix

fe1f54f

Fix ibe lint errors

ee97c83

Add lint exceptions

d528c20

Fix mistake fe_test

872c62a

Clean up

5a15399

K1li4nL force-pushed the kilian-fix-ci-linter branch from c69a232 to 5a15399 Compare June 24, 2024 12:51

AnomalRoil reviewed Jun 24, 2024

View reviewed changes

AnomalRoil merged commit 38e3744 into drandmerge Jul 1, 2024
12 checks passed

AnomalRoil deleted the kilian-fix-ci-linter branch July 1, 2024 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable linter in the CI #527

Enable linter in the CI #527

K1li4nL commented May 24, 2024

github-actions bot commented May 24, 2024

github-actions bot commented May 24, 2024

AnomalRoil left a comment

AnomalRoil Jun 6, 2024

K1li4nL Jun 6, 2024 •

edited

Loading

AnomalRoil Jun 6, 2024

K1li4nL Jun 6, 2024

K1li4nL Jun 12, 2024

AnomalRoil Jun 6, 2024

K1li4nL Jun 12, 2024

AnomalRoil left a comment

AnomalRoil Jun 12, 2024

K1li4nL Jun 12, 2024

AnomalRoil Jun 12, 2024

K1li4nL Jun 12, 2024

AnomalRoil Jun 12, 2024

AnomalRoil Jun 12, 2024

AnomalRoil Jun 12, 2024

AnomalRoil Jun 12, 2024

AnomalRoil Jun 12, 2024

AnomalRoil Jun 12, 2024

AnomalRoil Jun 12, 2024

AnomalRoil Jun 13, 2024

AnomalRoil Jun 13, 2024

AnomalRoil Jun 13, 2024

AnomalRoil Jun 13, 2024

AnomalRoil Jun 13, 2024

AnomalRoil Jun 13, 2024

AnomalRoil Jun 13, 2024

sonarqubecloud bot commented Jun 24, 2024

AnomalRoil left a comment

AnomalRoil Jun 24, 2024

AnomalRoil Jun 24, 2024

	// Embed pick a curve point containing a variable amount of embedded data.
	// Embed picks a curve point containing a variable amount of embedded data.

	// Data extract embedded data from a curve point
	// Data extracts embedded data from a curve point

		var errDifferentLengths = errors.New("inputs of different lengths")
		var errInvalidProof = errors.New("invalid proof")

Enable linter in the CI #527

Enable linter in the CI #527

Conversation

K1li4nL commented May 24, 2024

github-actions bot commented May 24, 2024

github-actions bot commented May 24, 2024

AnomalRoil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K1li4nL Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnomalRoil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 24, 2024

Quality Gate failed

AnomalRoil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K1li4nL Jun 6, 2024 •

edited

Loading