chore: lint and add go linting action (#114)

* lint and add go linting * Update go/Makefile * fix Co-authored-by: Marko Baricevic <[email protected]>
cosmos · Nov 14, 2022 · 41288bd · 41288bd
1 parent 4d119ac
commit 41288bd
Show file tree

Hide file tree

Showing 9 changed files with 126 additions and 55 deletions.
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
@@ -35,4 +35,20 @@ jobs:
       - name: Clippy linting on workspace (host functions only)
         working-directory: ./rust  
         run: cargo clippy --tests --no-default-features --features host-functions -- -D warnings
+
+  golangci:
+    name: golangci-lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/setup-go@v3
+        with:
+          # ci is set to go1.19 to match developer setups
+          go-version: 1.19.2
+      - 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
+          working-directory: go
 
diff --git a/go/.golangci.yml b/go/.golangci.yml
@@ -0,0 +1,26 @@
+run:
+  tests: false
+  # timeout for analysis, e.g. 30s, 5m, default is 1m
+  timeout: 5m
+
+linters:
+  disable-all: true
+  enable:
+    - depguard
+    - dogsled
+    - exportloopref
+    - goconst
+    - gocritic
+    - gofumpt
+    - gosec
+    - gosimple
+    - govet
+    - ineffassign
+    - misspell
+    - nakedret
+    - nolintlint
+    - staticcheck
+    - stylecheck
+    - typecheck
+    - unconvert
+    - unused
diff --git a/go/Makefile b/go/Makefile
@@ -18,3 +18,24 @@ install-proto-dep:
 	@go install github.com/regen-network/cosmos-proto/protoc-gen-gocosmos
 
 
+golangci_lint_cmd=golangci-lint
+golangci_version=v1.50.0
+
+lint:
+	@echo "--> Running linter"
+	@go install github.com/golangci/golangci-lint/cmd/golangci-lint@$(golangci_version)
+	@$(golangci_lint_cmd) run --timeout=10m
+
+lint-fix:
+	@echo "--> Running linter"
+	@go install github.com/golangci/golangci-lint/cmd/golangci-lint@$(golangci_version)
+	@$(golangci_lint_cmd) run --fix --out-format=tab --issues-exit-code=0
+
+.PHONY: lint lint-fix
+
+format:
+	@go install mvdan.cc/gofumpt@latest
+	@go install github.com/golangci/golangci-lint/cmd/golangci-lint@$(golangci_version)
+	find . -name '*.go' -type f -not -name "*.pb.go"  | xargs gofumpt -w -l
+	$(golangci_lint_cmd) run --fix
+.PHONY: format
diff --git a/go/ics23.go b/go/ics23.go
@@ -1,20 +1,21 @@
-/**
+/*
+*
 This implements the client side functions as specified in
 https://github.com/cosmos/ibc/tree/main/spec/core/ics-023-vector-commitments
 
 In particular:
 
-  // Assumes ExistenceProof
-  type verifyMembership = (root: CommitmentRoot, proof: CommitmentProof, key: Key, value: Value) => boolean
+	// Assumes ExistenceProof
+	type verifyMembership = (root: CommitmentRoot, proof: CommitmentProof, key: Key, value: Value) => boolean
 
-  // Assumes NonExistenceProof
-  type verifyNonMembership = (root: CommitmentRoot, proof: CommitmentProof, key: Key) => boolean
+	// Assumes NonExistenceProof
+	type verifyNonMembership = (root: CommitmentRoot, proof: CommitmentProof, key: Key) => boolean
 
-  // Assumes BatchProof - required ExistenceProofs may be a subset of all items proven
-  type batchVerifyMembership = (root: CommitmentRoot, proof: CommitmentProof, items: Map<Key, Value>) => boolean
+	// Assumes BatchProof - required ExistenceProofs may be a subset of all items proven
+	type batchVerifyMembership = (root: CommitmentRoot, proof: CommitmentProof, items: Map<Key, Value>) => boolean
 
-  // Assumes BatchProof - required NonExistenceProofs may be a subset of all items proven
-  type batchVerifyNonMembership = (root: CommitmentRoot, proof: CommitmentProof, keys: Set<Key>) => boolean
+	// Assumes BatchProof - required NonExistenceProofs may be a subset of all items proven
+	type batchVerifyNonMembership = (root: CommitmentRoot, proof: CommitmentProof, keys: Set<Key>) => boolean
 
 We make an adjustment to accept a Spec to ensure the provided proof is in the format of the expected merkle store.
 This can avoid an range of attacks on fake preimages, as we need to be careful on how to map key, value -> leaf

diff --git a/go/ops.go b/go/ops.go
@@ -13,16 +13,16 @@ import (
 	_ "crypto/sha512"
 
 	// adds ripemd160 capability to crypto.RIPEMD160
-	_ "golang.org/x/crypto/ripemd160"
+	_ "golang.org/x/crypto/ripemd160" //nolint:staticcheck
 )
 
 // Apply will calculate the leaf hash given the key and value being proven
 func (op *LeafOp) Apply(key []byte, value []byte) ([]byte, error) {
 	if len(key) == 0 {
-		return nil, errors.New("Leaf op needs key")
+		return nil, errors.New("leaf op needs key")
 	}
 	if len(value) == 0 {
-		return nil, errors.New("Leaf op needs value")
+		return nil, errors.New("leaf op needs value")
 	}
 	pkey, err := prepareLeafData(op.PrehashKey, op.Length, key)
 	if err != nil {
@@ -32,8 +32,11 @@ func (op *LeafOp) Apply(key []byte, value []byte) ([]byte, error) {
 	if err != nil {
 		return nil, fmt.Errorf("prehash value, %w", err)
 	}
-	data := append(op.Prefix, pkey...)
+
+	data := op.Prefix
+	data = append(data, pkey...)
 	data = append(data, pvalue...)
+
 	return doHash(op.Hash, data)
 }
 
@@ -42,49 +45,52 @@ func (op *LeafOp) CheckAgainstSpec(spec *ProofSpec) error {
 	lspec := spec.LeafSpec
 
 	if op.Hash != lspec.Hash {
-		return fmt.Errorf("Unexpected HashOp: %d", op.Hash)
+		return fmt.Errorf("unexpected HashOp: %d", op.Hash)
 	}
 	if op.PrehashKey != lspec.PrehashKey {
-		return fmt.Errorf("Unexpected PrehashKey: %d", op.PrehashKey)
+		return fmt.Errorf("unexpected PrehashKey: %d", op.PrehashKey)
 	}
 	if op.PrehashValue != lspec.PrehashValue {
-		return fmt.Errorf("Unexpected PrehashValue: %d", op.PrehashValue)
+		return fmt.Errorf("unexpected PrehashValue: %d", op.PrehashValue)
 	}
 	if op.Length != lspec.Length {
-		return fmt.Errorf("Unexpected LengthOp: %d", op.Length)
+		return fmt.Errorf("unexpected LengthOp: %d", op.Length)
 	}
 	if !bytes.HasPrefix(op.Prefix, lspec.Prefix) {
-		return fmt.Errorf("Leaf Prefix doesn't start with %X", lspec.Prefix)
+		return fmt.Errorf("leaf Prefix doesn't start with %X", lspec.Prefix)
 	}
 	return nil
 }
 
 // Apply will calculate the hash of the next step, given the hash of the previous step
 func (op *InnerOp) Apply(child []byte) ([]byte, error) {
 	if len(child) == 0 {
-		return nil, errors.New("Inner op needs child value")
+		return nil, errors.New("inner op needs child value")
 	}
-	preimage := append(op.Prefix, child...)
+
+	preimage := op.Prefix
+	preimage = append(preimage, child...)
 	preimage = append(preimage, op.Suffix...)
+
 	return doHash(op.Hash, preimage)
 }
 
 // CheckAgainstSpec will verify the InnerOp is in the format defined in spec
 func (op *InnerOp) CheckAgainstSpec(spec *ProofSpec) error {
 	if op.Hash != spec.InnerSpec.Hash {
-		return fmt.Errorf("Unexpected HashOp: %d", op.Hash)
+		return fmt.Errorf("unexpected HashOp: %d", op.Hash)
 	}
 
 	leafPrefix := spec.LeafSpec.Prefix
 	if bytes.HasPrefix(op.Prefix, leafPrefix) {
-		return fmt.Errorf("Inner Prefix starts with %X", leafPrefix)
+		return fmt.Errorf("inner Prefix starts with %X", leafPrefix)
 	}
 	if len(op.Prefix) < int(spec.InnerSpec.MinPrefixLength) {
-		return fmt.Errorf("InnerOp prefix too short (%d)", len(op.Prefix))
+		return fmt.Errorf("innerOp prefix too short (%d)", len(op.Prefix))
 	}
 	maxLeftChildBytes := (len(spec.InnerSpec.ChildOrder) - 1) * int(spec.InnerSpec.ChildSize)
 	if len(op.Prefix) > int(spec.InnerSpec.MaxPrefixLength)+maxLeftChildBytes {
-		return fmt.Errorf("InnerOp prefix too long (%d)", len(op.Prefix))
+		return fmt.Errorf("innerOp prefix too long (%d)", len(op.Prefix))
 	}
 	return nil
 }
@@ -137,7 +143,7 @@ func doHash(hashOp HashOp, preimage []byte) ([]byte, error) {
 		hash.Write(preimage)
 		return hash.Sum(nil), nil
 	}
-	return nil, fmt.Errorf("Unsupported hashop: %d", hashOp)
+	return nil, fmt.Errorf("unsupported hashop: %d", hashOp)
 }
 
 // doLengthOp will calculate the proper prefix and return it prepended
@@ -152,12 +158,12 @@ func doLengthOp(lengthOp LengthOp, data []byte) ([]byte, error) {
 		return res, nil
 	case LengthOp_REQUIRE_32_BYTES:
 		if len(data) != 32 {
-			return nil, fmt.Errorf("Data was %d bytes, not 32", len(data))
+			return nil, fmt.Errorf("data was %d bytes, not 32", len(data))
 		}
 		return data, nil
 	case LengthOp_REQUIRE_64_BYTES:
 		if len(data) != 64 {
-			return nil, fmt.Errorf("Data was %d bytes, not 64", len(data))
+			return nil, fmt.Errorf("data was %d bytes, not 64", len(data))
 		}
 		return data, nil
 	case LengthOp_FIXED32_LITTLE:
@@ -171,7 +177,7 @@ func doLengthOp(lengthOp LengthOp, data []byte) ([]byte, error) {
 		// case LengthOp_FIXED64_BIG:
 		// case LengthOp_FIXED64_LITTLE:
 	}
-	return nil, fmt.Errorf("Unsupported lengthop: %d", lengthOp)
+	return nil, fmt.Errorf("unsupported lengthop: %d", lengthOp)
 }
 
 func encodeVarintProto(l int) []byte {

diff --git a/go/proof.go b/go/proof.go
@@ -98,30 +98,29 @@ func (p *ExistenceProof) Verify(spec *ProofSpec, root CommitmentRoot, key []byte
 	}
 
 	if !bytes.Equal(key, p.Key) {
-		return fmt.Errorf("Provided key doesn't match proof")
+		return fmt.Errorf("provided key doesn't match proof")
 	}
 	if !bytes.Equal(value, p.Value) {
-		return fmt.Errorf("Provided value doesn't match proof")
+		return fmt.Errorf("provided value doesn't match proof")
 	}
 
 	calc, err := p.Calculate()
 	if err != nil {
-		return fmt.Errorf("Error calculating root, %w", err)
+		return fmt.Errorf("error calculating root, %w", err)
 	}
 	if !bytes.Equal(root, calc) {
-		return fmt.Errorf("Calculcated root doesn't match provided root")
+		return fmt.Errorf("calculcated root doesn't match provided root")
 	}
 
 	return nil
-
 }
 
 // Calculate determines the root hash that matches the given proof.
 // You must validate the result is what you have in a header.
 // Returns error if the calculations cannot be performed.
 func (p *ExistenceProof) Calculate() (CommitmentRoot, error) {
 	if p.GetLeaf() == nil {
-		return nil, errors.New("Existence Proof needs defined LeafOp")
+		return nil, errors.New("existence Proof needs defined LeafOp")
 	}
 
 	// leaf step takes the key and value as input
@@ -151,24 +150,24 @@ func (p *NonExistenceProof) Calculate() (CommitmentRoot, error) {
 	case p.Right != nil:
 		return p.Right.Calculate()
 	default:
-		return nil, errors.New("Nonexistence proof has empty Left and Right proof")
+		return nil, errors.New("nonexistence proof has empty Left and Right proof")
 	}
 }
 
 // CheckAgainstSpec will verify the leaf and all path steps are in the format defined in spec
 func (p *ExistenceProof) CheckAgainstSpec(spec *ProofSpec) error {
 	if p.GetLeaf() == nil {
-		return errors.New("Existence Proof needs defined LeafOp")
+		return errors.New("existence Proof needs defined LeafOp")
 	}
 	err := p.Leaf.CheckAgainstSpec(spec)
 	if err != nil {
 		return fmt.Errorf("leaf, %w", err)
 	}
 	if spec.MinDepth > 0 && len(p.Path) < int(spec.MinDepth) {
-		return fmt.Errorf("InnerOps depth too short: %d", len(p.Path))
+		return fmt.Errorf("innerOps depth too short: %d", len(p.Path))
 	}
 	if spec.MaxDepth > 0 && len(p.Path) > int(spec.MaxDepth) {
-		return fmt.Errorf("InnerOps depth too long: %d", len(p.Path))
+		return fmt.Errorf("innerOps depth too long: %d", len(p.Path))
 	}
 
 	for _, inner := range p.Path {
@@ -208,25 +207,28 @@ func (p *NonExistenceProof) Verify(spec *ProofSpec, root CommitmentRoot, key []b
 			return errors.New("key is not left of right proof")
 		}
 	}
+
 	if leftKey != nil {
 		if bytes.Compare(key, leftKey) <= 0 {
 			return errors.New("key is not right of left proof")
 		}
 	}
 
-	if leftKey == nil {
+	switch {
+	case leftKey == nil:
 		if !IsLeftMost(spec.InnerSpec, p.Right.Path) {
 			return errors.New("left proof missing, right proof must be left-most")
 		}
-	} else if rightKey == nil {
+	case rightKey == nil:
 		if !IsRightMost(spec.InnerSpec, p.Left.Path) {
 			return errors.New("right proof missing, left proof must be right-most")
 		}
-	} else { // in the middle
+	default:
 		if !IsLeftNeighbor(spec.InnerSpec, p.Left.Path, p.Right.Path) {
 			return errors.New("right proof missing, left proof must be right-most")
 		}
 	}
+
 	return nil
 }
 
@@ -387,14 +389,14 @@ func rightBranchesAreEmpty(spec *InnerSpec, op *InnerOp) bool {
 // the index of this branch
 func getPosition(order []int32, branch int32) int {
 	if branch < 0 || int(branch) >= len(order) {
-		panic(fmt.Errorf("Invalid branch: %d", branch))
+		panic(fmt.Errorf("invalid branch: %d", branch))
 	}
 	for i, item := range order {
 		if branch == item {
 			return i
 		}
 	}
-	panic(fmt.Errorf("Branch %d not found in order %v", branch, order))
+	panic(fmt.Errorf("branch %d not found in order %v", branch, order))
 }
 
 // This will look at the proof and determine which order it is...
@@ -407,5 +409,5 @@ func orderFromPadding(spec *InnerSpec, inner *InnerOp) (int32, error) {
 			return branch, nil
 		}
 	}
-	return 0, errors.New("Cannot find any valid spacing for this node")
+	return 0, errors.New("cannot find any valid spacing for this node")
 }