chore: lint ics23 golang code (cosmos#161)

* lint * update ci config * remove unused code --------- Co-authored-by: Carlos Rodriguez <[email protected]>
shuoer86 · Sep 15, 2023 · 5f9fbd8 · 5f9fbd8
1 parent e179fae
commit 5f9fbd8
Show file tree

Hide file tree

Showing 11 changed files with 152 additions and 37 deletions.
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))