chore: lint ics23 golang code (#161)

faddat · crodriguezvega · web-flow · commit 5f9fbd8aaa5a · 2023-09-15T10:23:03.000+02:00
* lint

* update ci config

* remove unused code

---------

Co-authored-by: Carlos Rodriguez &lt;carlos@interchain.io&gt;
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
@@ -23,13 +23,13 @@ jobs:
       - uses: actions/setup-go@v4
         with:
           # ci is set to go1.19 to match developer setups
-          go-version: 1.19.2
+          go-version: "1.21"
       - uses: actions/checkout@v3
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v3
         with:
           # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version
-          version: v1.50.0
+          version: v1.54.2
           working-directory: go
 
   test:
@@ -39,7 +39,7 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v4
         with:
-          go-version: 1.19.2
+          go-version: "1.21"
       - name: test
         working-directory: ./go
         run: make test
@@ -51,7 +51,7 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v4
         with:
-          go-version: 1.19.2
+          go-version: "1.21"
       - name: Run coverage
         working-directory: ./go
         run: go test -race -coverprofile=coverage.txt -covermode=atomic
diff --git a/go/.golangci.yml b/go/.golangci.yml
@@ -1,14 +1,14 @@
 run:
-  tests: false
-  # timeout for analysis, e.g. 30s, 5m, default is 1m
+  tests: true
+  #   # timeout for analysis, e.g. 30s, 5m, default is 1m
   timeout: 5m
 
 linters:
   disable-all: true
   enable:
-    - depguard
-    - dogsled
     - exportloopref
+    - errcheck
+    - gci
     - goconst
     - gocritic
     - gofumpt
@@ -18,9 +18,99 @@ linters:
     - ineffassign
     - misspell
     - nakedret
-    - nolintlint
     - staticcheck
+    - thelper
+    - typecheck
     - stylecheck
+    - revive
     - typecheck
+    - tenv
     - unconvert
+    # Prefer unparam over revive's unused param. It is more thorough in its checking.
+    - unparam
     - unused
+    - misspell
+
+issues:
+  exclude-rules:
+    - text: 'differs only by capitalization to method'
+      linters:
+        - revive
+    - text: 'Use of weak random number generator'
+      linters:
+        - gosec
+
+  max-issues-per-linter: 10000
+  max-same-issues: 10000
+
+linters-settings:
+  gci:
+    sections:
+      - standard # Standard section: captures all standard packages.
+      - default # Default section: contains all imports that could not be matched to another section type.
+      - blank # blank imports
+      - dot # dot imports
+      - prefix(cosmossdk.io)
+      - prefix(github.com/cosmos/cosmos-sdk)
+      - prefix(github.com/cometbft/cometbft)
+      - prefix(github.com/cosmos/ics23)
+    custom-order: true
+  revive:
+    enable-all-rules: true
+    # Do NOT whine about the following, full explanation found in:
+    # https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md#description-of-available-rules
+    rules:
+      - name: use-any
+        disabled: true
+      - name: if-return
+        disabled: true
+      - name: max-public-structs
+        disabled: true
+      - name: cognitive-complexity
+        disabled: true
+      - name: argument-limit
+        disabled: true
+      - name: cyclomatic
+        disabled: true
+      - name: file-header
+        disabled: true
+      - name: function-length
+        disabled: true
+      - name: function-result-limit
+        disabled: true
+      - name: line-length-limit
+        disabled: true
+      - name: flag-parameter
+        disabled: true
+      - name: add-constant
+        disabled: true
+      - name: empty-lines
+        disabled: true
+      - name: banned-characters
+        disabled: true
+      - name: deep-exit
+        disabled: true
+      - name: confusing-results
+        disabled: true
+      - name: unused-parameter
+        disabled: true
+      - name: modifies-value-receiver
+        disabled: true
+      - name: early-return
+        disabled: true
+      - name: nested-structs
+        disabled: true
+      - name: confusing-naming
+        disabled: true
+      - name: defer
+        disabled: true
+      # Disabled in favour of unparam.
+      - name: unused-parameter
+        disabled: true
+      - name: unhandled-error
+        disabled: false
+        arguments:
+          - 'fmt.Printf'
+          - 'fmt.Print'
+          - 'fmt.Println'
+          - 'myFunction'
diff --git a/go/fuzz_test.go b/go/fuzz_test.go
@@ -61,8 +61,6 @@ func init() {
 	batchVectorDataSeeds = bsL
 }
 
-var specVectorTestData = VectorsTestData()
-
 func FuzzVerifyNonMembership(f *testing.F) {
 	if testing.Short() {
 		f.Skip("in -short mode")
diff --git a/go/go.mod b/go/go.mod
@@ -1,6 +1,6 @@
 module github.com/cosmos/ics23/go
 
-go 1.19
+go 1.21
 
 require (
 	github.com/cosmos/gogoproto v1.4.11
diff --git a/go/ops.go b/go/ops.go
@@ -12,7 +12,6 @@ import (
 	_ "crypto/sha256"
 	// adds sha512 capability to crypto.SHA512
 	_ "crypto/sha512"
-
 	// adds ripemd160 capability to crypto.RIPEMD160
 	_ "golang.org/x/crypto/ripemd160" //nolint:staticcheck
 )
@@ -164,15 +163,24 @@ func doHash(hashOp HashOp, preimage []byte) ([]byte, error) {
 	case HashOp_BITCOIN:
 		// ripemd160(sha256(x))
 		sha := crypto.SHA256.New()
-		sha.Write(preimage)
+		_, err := sha.Write(preimage)
+		if err != nil {
+			return nil, err
+		}
 		tmp := sha.Sum(nil)
-		hash := crypto.RIPEMD160.New()
-		hash.Write(tmp)
-		return hash.Sum(nil), nil
+		bitcoinHash := crypto.RIPEMD160.New()
+		_, err = bitcoinHash.Write(tmp)
+		if err != nil {
+			return nil, err
+		}
+		return bitcoinHash.Sum(nil), nil
 	case HashOp_SHA512_256:
-		hash := crypto.SHA512_256.New()
-		hash.Write(preimage)
-		return hash.Sum(nil), nil
+		shaHash := crypto.SHA512_256.New()
+		_, err := shaHash.Write(preimage)
+		if err != nil {
+			return nil, err
+		}
+		return shaHash.Sum(nil), nil
 	}
 	return nil, fmt.Errorf("unsupported hashop: %d", hashOp)
 }
@@ -183,7 +191,10 @@ type hasher interface {
 
 func hashBz(h hasher, preimage []byte) ([]byte, error) {
 	hh := h.New()
-	hh.Write(preimage)
+	_, err := hh.Write(preimage)
+	if err != nil {
+		return nil, err
+	}
 	return hh.Sum(nil), nil
 }
 
@@ -193,8 +204,8 @@ func prepareLeafData(hashOp HashOp, lengthOp LengthOp, data []byte) ([]byte, err
 	if err != nil {
 		return nil, err
 	}
-	ldata, err := doLengthOp(lengthOp, hdata)
-	return ldata, err
+
+	return doLengthOp(lengthOp, hdata)
 }
 
 func validateSpec(spec *ProofSpec) bool {
diff --git a/go/ops_data_test.go b/go/ops_data_test.go
@@ -17,6 +17,7 @@ type LeafOpTestStruct struct {
 }
 
 func LeafOpTestData(t *testing.T) map[string]LeafOpTestStruct {
+	t.Helper()
 	fname := filepath.Join("..", "testdata", "TestLeafOpData.json")
 	ffile, err := os.Open(fname)
 	if err != nil {
@@ -39,6 +40,7 @@ type InnerOpTestStruct struct {
 }
 
 func InnerOpTestData(t *testing.T) map[string]InnerOpTestStruct {
+	t.Helper()
 	fname := filepath.Join("..", "testdata", "TestInnerOpData.json")
 	ffile, err := os.Open(fname)
 	if err != nil {
@@ -60,6 +62,7 @@ type DoHashTestStruct struct {
 }
 
 func DoHashTestData(t *testing.T) map[string]DoHashTestStruct {
+	t.Helper()
 	fname := filepath.Join("..", "testdata", "TestDoHashData.json")
 	ffile, err := os.Open(fname)
 	if err != nil {
diff --git a/go/ops_test.go b/go/ops_test.go
@@ -14,13 +14,13 @@ func TestLeafOp(t *testing.T) {
 			res, err := tc.Op.Apply(tc.Key, tc.Value)
 			// short-circuit with error case
 			if tc.IsErr && err == nil {
-				t.Fatal("Expected error, but got none")
+				t.Fatal("expected error, but got none")
 			}
-			if tc.IsErr == false && err != nil {
+			if !tc.IsErr && err != nil {
 				t.Fatal(err)
 			}
 			if !bytes.Equal(res, tc.Expected) {
-				t.Errorf("Bad result: %s vs %s", toHex(res), toHex(tc.Expected))
+				t.Errorf("bad result: %s vs %s", toHex(res), toHex(tc.Expected))
 			}
 		})
 	}
@@ -35,7 +35,7 @@ func TestInnerOp(t *testing.T) {
 			if tc.IsErr && err == nil {
 				t.Fatal("Expected error, but got none")
 			}
-			if tc.IsErr == false && err != nil {
+			if !tc.IsErr && err != nil {
 				t.Fatal(err)
 			}
 			if !bytes.Equal(res, tc.Expected) {
diff --git a/go/proof.go b/go/proof.go
@@ -200,7 +200,7 @@ func (p *ExistenceProof) CheckAgainstSpec(spec *ProofSpec) error {
 		if err := inner.CheckAgainstSpec(spec, layerNum); err != nil {
 			return fmt.Errorf("inner, %w", err)
 		}
-		layerNum += 1
+		layerNum++
 	}
 	return nil
 }
@@ -367,7 +367,7 @@ func getPadding(spec *InnerSpec, branch int32) (minPrefix, maxPrefix, suffix int
 
 	// count how many children are in the suffix
 	suffix = (len(spec.ChildOrder) - 1 - idx) * int(spec.ChildSize)
-	return
+	return minPrefix, maxPrefix, suffix
 }
 
 // leftBranchesAreEmpty returns true if the padding bytes correspond to all empty siblings
diff --git a/go/proof_data_test.go b/go/proof_data_test.go
@@ -14,6 +14,7 @@ type ExistenceProofTestStruct struct {
 }
 
 func ExistenceProofTestData(t *testing.T) map[string]ExistenceProofTestStruct {
+	t.Helper()
 	fname := filepath.Join("..", "testdata", "TestExistenceProofData.json")
 	ffile, err := os.Open(fname)
 	if err != nil {
@@ -35,6 +36,7 @@ type CheckLeafTestStruct struct {
 }
 
 func CheckLeafTestData(t *testing.T) map[string]CheckLeafTestStruct {
+	t.Helper()
 	fname := filepath.Join("..", "testdata", "TestCheckLeafData.json")
 	ffile, err := os.Open(fname)
 	if err != nil {
@@ -56,6 +58,7 @@ type CheckAgainstSpecTestStruct struct {
 }
 
 func CheckAgainstSpecTestData(t *testing.T) map[string]CheckAgainstSpecTestStruct {
+	t.Helper()
 	fname := filepath.Join("..", "testdata", "TestCheckAgainstSpecData.json")
 	ffile, err := os.Open(fname)
 	if err != nil {
@@ -94,6 +97,7 @@ type EmptyBranchTestStruct struct {
 }
 
 func EmptyBranchTestData(t *testing.T) []EmptyBranchTestStruct {
+	t.Helper()
 	emptyChild := SpecWithEmptyChild.InnerSpec.EmptyChild
 
 	return []EmptyBranchTestStruct{
diff --git a/go/proof_test.go b/go/proof_test.go
@@ -12,11 +12,14 @@ func TestExistenceProof(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			res, err := tc.Proof.Calculate()
 			// short-circuit with error case
-			if tc.IsErr && err == nil {
-				t.Fatal("Expected error, but got none")
-			}
-			if tc.IsErr == false && err != nil {
-				t.Fatal(err)
+			if tc.IsErr {
+				if err == nil {
+					t.Fatal("Expected error, but got none")
+				}
+			} else {
+				if err != nil {
+					t.Fatal(err)
+				}
 			}
 			if !bytes.Equal(res, tc.Expected) {
 				t.Errorf("Bad result: %s vs %s", toHex(res), toHex(tc.Expected))
diff --git a/go/vectors_data_test.go b/go/vectors_data_test.go

Original file line number	Diff line number	Diff line change
`@@ -61,8 +61,6 @@ func init() {`
`61`	`61`	`batchVectorDataSeeds = bsL`
`62`	`62`	`}`
`63`	`63`
`64`		`-var specVectorTestData = VectorsTestData()`
`65`		`-`
`66`	`64`	`func FuzzVerifyNonMembership(f *testing.F) {`
`67`	`65`	`if testing.Short() {`
`68`	`66`	`f.Skip("in -short mode")`
Original file line number	Diff line number	Diff line change
`@@ -17,6 +17,7 @@ type LeafOpTestStruct struct {`
`17`	`17`	`}`
`18`	`18`
`19`	`19`	`func LeafOpTestData(t *testing.T) map[string]LeafOpTestStruct {`
	`20`	`+ t.Helper()`
`20`	`21`	`fname := filepath.Join("..", "testdata", "TestLeafOpData.json")`
`21`	`22`	`ffile, err := os.Open(fname)`
`22`	`23`	`if err != nil {`
`@@ -39,6 +40,7 @@ type InnerOpTestStruct struct {`
`39`	`40`	`}`
`40`	`41`
`41`	`42`	`func InnerOpTestData(t *testing.T) map[string]InnerOpTestStruct {`
	`43`	`+ t.Helper()`
`42`	`44`	`fname := filepath.Join("..", "testdata", "TestInnerOpData.json")`
`43`	`45`	`ffile, err := os.Open(fname)`
`44`	`46`	`if err != nil {`
`@@ -60,6 +62,7 @@ type DoHashTestStruct struct {`
`60`	`62`	`}`
`61`	`63`
`62`	`64`	`func DoHashTestData(t *testing.T) map[string]DoHashTestStruct {`
	`65`	`+ t.Helper()`
`63`	`66`	`fname := filepath.Join("..", "testdata", "TestDoHashData.json")`
`64`	`67`	`ffile, err := os.Open(fname)`
`65`	`68`	`if err != nil {`
Original file line number	Diff line number	Diff line change
`@@ -14,13 +14,13 @@ func TestLeafOp(t *testing.T) {`
`14`	`14`	`res, err := tc.Op.Apply(tc.Key, tc.Value)`
`15`	`15`	`// short-circuit with error case`
`16`	`16`	`if tc.IsErr && err == nil {`
`17`		`- t.Fatal("Expected error, but got none")`
	`17`	`+ t.Fatal("expected error, but got none")`
`18`	`18`	`}`
`19`		`- if tc.IsErr == false && err != nil {`
	`19`	`+ if !tc.IsErr && err != nil {`
`20`	`20`	`t.Fatal(err)`
`21`	`21`	`}`
`22`	`22`	`if !bytes.Equal(res, tc.Expected) {`
`23`		`- t.Errorf("Bad result: %s vs %s", toHex(res), toHex(tc.Expected))`
	`23`	`+ t.Errorf("bad result: %s vs %s", toHex(res), toHex(tc.Expected))`
`24`	`24`	`}`
`25`	`25`	`})`
`26`	`26`	`}`
`@@ -35,7 +35,7 @@ func TestInnerOp(t *testing.T) {`
`35`	`35`	`if tc.IsErr && err == nil {`
`36`	`36`	`t.Fatal("Expected error, but got none")`
`37`	`37`	`}`
`38`		`- if tc.IsErr == false && err != nil {`
	`38`	`+ if !tc.IsErr && err != nil {`
`39`	`39`	`t.Fatal(err)`
`40`	`40`	`}`
`41`	`41`	`if !bytes.Equal(res, tc.Expected) {`
Original file line number	Diff line number	Diff line change
`@@ -200,7 +200,7 @@ func (p ExistenceProof) CheckAgainstSpec(spec ProofSpec) error {`
`200`	`200`	`if err := inner.CheckAgainstSpec(spec, layerNum); err != nil {`
`201`	`201`	`return fmt.Errorf("inner, %w", err)`
`202`	`202`	`}`
`203`		`- layerNum += 1`
	`203`	`+ layerNum++`
`204`	`204`	`}`
`205`	`205`	`return nil`
`206`	`206`	`}`
`@@ -367,7 +367,7 @@ func getPadding(spec *InnerSpec, branch int32) (minPrefix, maxPrefix, suffix int`
`367`	`367`
`368`	`368`	`// count how many children are in the suffix`
`369`	`369`	`suffix = (len(spec.ChildOrder) - 1 - idx) * int(spec.ChildSize)`
`370`		`- return`
	`370`	`+ return minPrefix, maxPrefix, suffix`
`371`	`371`	`}`
`372`	`372`
`373`	`373`	`// leftBranchesAreEmpty returns true if the padding bytes correspond to all empty siblings`
Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,7 @@ type ExistenceProofTestStruct struct {`
`14`	`14`	`}`
`15`	`15`
`16`	`16`	`func ExistenceProofTestData(t *testing.T) map[string]ExistenceProofTestStruct {`
	`17`	`+ t.Helper()`
`17`	`18`	`fname := filepath.Join("..", "testdata", "TestExistenceProofData.json")`
`18`	`19`	`ffile, err := os.Open(fname)`
`19`	`20`	`if err != nil {`
`@@ -35,6 +36,7 @@ type CheckLeafTestStruct struct {`
`35`	`36`	`}`
`36`	`37`
`37`	`38`	`func CheckLeafTestData(t *testing.T) map[string]CheckLeafTestStruct {`
	`39`	`+ t.Helper()`
`38`	`40`	`fname := filepath.Join("..", "testdata", "TestCheckLeafData.json")`
`39`	`41`	`ffile, err := os.Open(fname)`
`40`	`42`	`if err != nil {`
`@@ -56,6 +58,7 @@ type CheckAgainstSpecTestStruct struct {`
`56`	`58`	`}`
`57`	`59`
`58`	`60`	`func CheckAgainstSpecTestData(t *testing.T) map[string]CheckAgainstSpecTestStruct {`
	`61`	`+ t.Helper()`
`59`	`62`	`fname := filepath.Join("..", "testdata", "TestCheckAgainstSpecData.json")`
`60`	`63`	`ffile, err := os.Open(fname)`
`61`	`64`	`if err != nil {`
`@@ -94,6 +97,7 @@ type EmptyBranchTestStruct struct {`
`94`	`97`	`}`
`95`	`98`
`96`	`99`	`func EmptyBranchTestData(t *testing.T) []EmptyBranchTestStruct {`
	`100`	`+ t.Helper()`
`97`	`101`	`emptyChild := SpecWithEmptyChild.InnerSpec.EmptyChild`
`98`	`102`
`99`	`103`	`return []EmptyBranchTestStruct{`