From 1cdb233a0045b1f59a2e37fac8571f88cd5064e5 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Mon, 7 Oct 2019 17:29:07 +0200 Subject: [PATCH 1/3] add lint command to Makefile --- Makefile | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Makefile b/Makefile index acca9e7..874282c 100644 --- a/Makefile +++ b/Makefile @@ -2,14 +2,35 @@ PKG := github.com/lightninglabs/loop GOTEST := GO111MODULE=on go test -v +GO_BIN := ${GOPATH}/bin + +GOFILES_NOVENDOR = $(shell find . -type f -name '*.go' -not -path "./vendor/*") GOLIST := go list $(PKG)/... | grep -v '/vendor/' +LINT_BIN := $(GO_BIN)/golangci-lint +LINT_PKG := github.com/golangci/golangci-lint/cmd/golangci-lint +LINT_COMMIT := v1.18.0 +LINT = $(LINT_BIN) run -v + +DEPGET := cd /tmp && GO111MODULE=on go get -v XARGS := xargs -L 1 TEST_FLAGS = -test.timeout=20m UNIT := $(GOLIST) | $(XARGS) env $(GOTEST) $(TEST_FLAGS) +$(LINT_BIN): + @$(call print, "Fetching linter") + $(DEPGET) $(LINT_PKG)@$(LINT_COMMIT) + unit: @$(call print, "Running unit tests.") $(UNIT) + +fmt: + @$(call print, "Formatting source.") + gofmt -l -w -s $(GOFILES_NOVENDOR) + +lint: $(LINT_BIN) + @$(call print, "Linting source.") + $(LINT) \ No newline at end of file From 36838cf7f464cf73b0201798063b2caffeae4250 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Mon, 7 Oct 2019 17:29:24 +0200 Subject: [PATCH 2/3] multi: fix most obvious linter errors --- client.go | 2 +- cmd/loop/loopin.go | 3 +-- cmd/loop/loopout.go | 3 +-- cmd/loop/main.go | 6 +++--- cmd/loop/quote.go | 3 +-- executor.go | 6 +++--- lndclient/invoices_client.go | 4 ++-- lndclient/log.go | 3 ++- lndclient/router_client.go | 3 ++- log.go | 5 ++--- loopdb/loopin.go | 5 ++++- loopdb/loopout.go | 5 ++++- loopin.go | 2 +- loopout_test.go | 6 +++--- server_mock_test.go | 7 ------- store_mock_test.go | 5 ----- sweep/sweeper.go | 2 +- test/log.go | 3 ++- test/testutils.go | 5 ++++- test/timeout.go | 5 ++++- testcontext_test.go | 8 -------- 21 files changed, 41 insertions(+), 50 deletions(-) diff --git a/client.go b/client.go index 008f6c1..0cf2164 100644 --- a/client.go +++ b/client.go @@ -384,7 +384,7 @@ func (s *Client) LoopOutQuote(ctx context.Context, return &LoopOutQuote{ SwapFee: swapFee, MinerFee: minerFee, - PrepayAmount: btcutil.Amount(quote.PrepayAmount), + PrepayAmount: quote.PrepayAmount, SwapPaymentDest: quote.SwapPaymentDest, CltvDelta: quote.CltvDelta, }, nil diff --git a/cmd/loop/loopin.go b/cmd/loop/loopin.go index 36b6be2..6680cf4 100644 --- a/cmd/loop/loopin.go +++ b/cmd/loop/loopin.go @@ -41,8 +41,7 @@ func loopIn(ctx *cli.Context) error { args = args.Tail() default: // Show command help if no arguments and flags were provided. - cli.ShowCommandHelp(ctx, "in") - return nil + return cli.ShowCommandHelp(ctx, "in") } amt, err := parseAmt(amtStr) diff --git a/cmd/loop/loopout.go b/cmd/loop/loopout.go index 2fc2ef7..75ed7a5 100644 --- a/cmd/loop/loopout.go +++ b/cmd/loop/loopout.go @@ -59,8 +59,7 @@ func loopOut(ctx *cli.Context) error { args = args.Tail() default: // Show command help if no arguments and flags were provided. - cli.ShowCommandHelp(ctx, "out") - return nil + return cli.ShowCommandHelp(ctx, "out") } amt, err := parseAmt(amtStr) diff --git a/cmd/loop/main.go b/cmd/loop/main.go index 35fae92..b777548 100644 --- a/cmd/loop/main.go +++ b/cmd/loop/main.go @@ -97,7 +97,7 @@ type limits struct { } func getLimits(amt btcutil.Amount, quote *looprpc.QuoteResponse) *limits { - maxSwapRoutingFee := getMaxRoutingFee(btcutil.Amount(amt)) + maxSwapRoutingFee := getMaxRoutingFee(amt) maxPrepayRoutingFee := getMaxRoutingFee(btcutil.Amount( quote.PrepayAmt, )) @@ -126,7 +126,7 @@ func displayLimits(swapType loop.Type, amt btcutil.Amount, l *limits, if l.maxPrepayRoutingFee != nil { totalSuccessMax += *l.maxPrepayRoutingFee } - + if swapType == loop.TypeIn && externalHtlc { fmt.Printf("On-chain fee for external loop in is not " + "included.\nSufficient fees will need to be paid " + @@ -135,7 +135,7 @@ func displayLimits(swapType loop.Type, amt btcutil.Amount, l *limits, } fmt.Printf("Max swap fees for %d Loop %v: %d\n", - btcutil.Amount(amt), swapType, totalSuccessMax, + amt, swapType, totalSuccessMax, ) fmt.Printf("CONTINUE SWAP? (y/n), expand fee detail (x): ") diff --git a/cmd/loop/quote.go b/cmd/loop/quote.go index 2c7fdfd..ec4ddd0 100644 --- a/cmd/loop/quote.go +++ b/cmd/loop/quote.go @@ -28,8 +28,7 @@ func quote(ctx *cli.Context) error { // Show command help if the incorrect number arguments and/or flags were // provided. if ctx.NArg() != 1 || ctx.NumFlags() > 1 { - cli.ShowCommandHelp(ctx, "quote") - return nil + return cli.ShowCommandHelp(ctx, "quote") } args := ctx.Args() diff --git a/executor.go b/executor.go index 93126e9..8366eec 100644 --- a/executor.go +++ b/executor.go @@ -69,7 +69,7 @@ func (s *executor) run(mainCtx context.Context, select { case h := <-blockEpochChan: - setHeight(int32(h)) + setHeight(h) case err := <-blockErrorChan: return err case <-mainCtx.Done(): @@ -134,10 +134,10 @@ func (s *executor) run(mainCtx context.Context, delete(blockEpochQueues, doneID) case h := <-blockEpochChan: - setHeight(int32(h)) + setHeight(h) for _, queue := range blockEpochQueues { select { - case queue.ChanIn() <- int32(h): + case queue.ChanIn() <- h: case <-mainCtx.Done(): return mainCtx.Err() } diff --git a/lndclient/invoices_client.go b/lndclient/invoices_client.go index 61e6de1..11841b2 100644 --- a/lndclient/invoices_client.go +++ b/lndclient/invoices_client.go @@ -52,10 +52,10 @@ func (s *invoicesClient) WaitForFinished() { func (s *invoicesClient) SettleInvoice(ctx context.Context, preimage lntypes.Preimage) error { - rpcCtx, cancel := context.WithTimeout(ctx, rpcTimeout) + timeoutCtx, cancel := context.WithTimeout(ctx, rpcTimeout) defer cancel() - rpcCtx = s.invoiceMac.WithMacaroonAuth(ctx) + rpcCtx := s.invoiceMac.WithMacaroonAuth(timeoutCtx) _, err := s.client.SettleInvoice(rpcCtx, &invoicesrpc.SettleInvoiceMsg{ Preimage: preimage[:], }) diff --git a/lndclient/log.go b/lndclient/log.go index d972f21..5a976f1 100644 --- a/lndclient/log.go +++ b/lndclient/log.go @@ -1,8 +1,9 @@ package lndclient import ( - "github.com/btcsuite/btclog" "os" + + "github.com/btcsuite/btclog" ) // log is a logger that is initialized with no output filters. This diff --git a/lndclient/router_client.go b/lndclient/router_client.go index 7a33168..f99d736 100644 --- a/lndclient/router_client.go +++ b/lndclient/router_client.go @@ -4,9 +4,10 @@ import ( "context" "encoding/hex" "fmt" + "time" + "github.com/lightningnetwork/lnd/lnrpc" "github.com/lightningnetwork/lnd/routing/route" - "time" "github.com/lightningnetwork/lnd/channeldb" "google.golang.org/grpc/codes" diff --git a/log.go b/log.go index 3423dd5..d9829e1 100644 --- a/log.go +++ b/log.go @@ -12,9 +12,8 @@ import ( // means the package will not perform any logging by default until the caller // requests it. var ( - backendLog = btclog.NewBackend(logWriter{}) - logger = backendLog.Logger("CLIENT") - servicesLogger = backendLog.Logger("SERVICES") + backendLog = btclog.NewBackend(logWriter{}) + logger = backendLog.Logger("CLIENT") ) // logWriter implements an io.Writer that outputs to both standard output and diff --git a/loopdb/loopin.go b/loopdb/loopin.go index efe0bbe..d971afe 100644 --- a/loopdb/loopin.go +++ b/loopdb/loopin.go @@ -127,7 +127,10 @@ func deserializeLoopInContract(value []byte) (*LoopInContract, error) { return nil, err } - binary.Read(r, byteOrder, &contract.AmountRequested) + err = binary.Read(r, byteOrder, &contract.AmountRequested) + if err != nil { + return nil, err + } n, err := r.Read(contract.SenderKey[:]) if err != nil { diff --git a/loopdb/loopout.go b/loopdb/loopout.go index 0176ec9..7dc173a 100644 --- a/loopdb/loopout.go +++ b/loopdb/loopout.go @@ -84,7 +84,10 @@ func deserializeLoopOutContract(value []byte, chainParams *chaincfg.Params) ( return nil, err } - binary.Read(r, byteOrder, &contract.AmountRequested) + err = binary.Read(r, byteOrder, &contract.AmountRequested) + if err != nil { + return nil, err + } contract.PrepayInvoice, err = wire.ReadVarString(r, 0) if err != nil { diff --git a/loopin.go b/loopin.go index 229718f..bc33be5 100644 --- a/loopin.go +++ b/loopin.go @@ -493,7 +493,7 @@ func (s *loopInSwap) waitForSwapComplete(ctx context.Context, case err := <-swapInvoiceErr: return err - // An update to the swap invoice occured. Check the new state + // An update to the swap invoice occurred. Check the new state // and update the swap state accordingly. case update := <-swapInvoiceChan: s.log.Infof("Received swap invoice update: %v", diff --git a/loopout_test.go b/loopout_test.go index c491e26..4292985 100644 --- a/loopout_test.go +++ b/loopout_test.go @@ -176,7 +176,7 @@ func TestCustomSweepConfTarget(t *testing.T) { // Notify the confirmation notification for the HTLC. ctx.AssertRegisterConf() - blockEpochChan <- int32(ctx.Lnd.Height + 1) + blockEpochChan <- ctx.Lnd.Height + 1 htlcTx := wire.NewMsgTx(2) htlcTx.AddTxOut(&wire.TxOut{ @@ -238,7 +238,7 @@ func TestCustomSweepConfTarget(t *testing.T) { // The sweep should have a fee that corresponds to the custom // confirmation target. - sweepTx := assertSweepTx(testRequest.SweepConfTarget) + _ = assertSweepTx(testRequest.SweepConfTarget) // We'll then notify the height at which we begin using the default // confirmation target. @@ -249,7 +249,7 @@ func TestCustomSweepConfTarget(t *testing.T) { // We should expect to see another sweep using the higher fee since the // spend hasn't been confirmed yet. - sweepTx = assertSweepTx(DefaultSweepConfTarget) + sweepTx := assertSweepTx(DefaultSweepConfTarget) // Notify the spend so that the swap reaches its final state. ctx.NotifySpend(sweepTx, 0) diff --git a/server_mock_test.go b/server_mock_test.go index 44cc555..9afc7c4 100644 --- a/server_mock_test.go +++ b/server_mock_test.go @@ -4,7 +4,6 @@ import ( "context" "errors" - "testing" "time" "github.com/btcsuite/btcd/chaincfg" @@ -20,20 +19,14 @@ var ( testLoopOutOnChainCltvDelta = int32(30) testChargeOnChainCltvDelta = int32(100) - testCltvDelta = 50 testSwapFee = btcutil.Amount(210) - testInvoiceExpiry = 180 * time.Second testFixedPrepayAmount = btcutil.Amount(100) testMinSwapAmount = btcutil.Amount(10000) testMaxSwapAmount = btcutil.Amount(1000000) - testTxConfTarget = 2 - testRepublishDelay = 10 * time.Second ) // serverMock is used in client unit tests to simulate swap server behaviour. type serverMock struct { - t *testing.T - expectedSwapAmt btcutil.Amount swapInvoiceAmt btcutil.Amount prepayInvoiceAmt btcutil.Amount diff --git a/store_mock_test.go b/store_mock_test.go index 317c864..9d932a8 100644 --- a/store_mock_test.go +++ b/store_mock_test.go @@ -25,11 +25,6 @@ type storeMock struct { t *testing.T } -type finishData struct { - preimage lntypes.Hash - result loopdb.SwapState -} - // NewStoreMock instantiates a new mock store. func newStoreMock(t *testing.T) *storeMock { return &storeMock{ diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 93d6007..038ba9f 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -112,7 +112,7 @@ func (s *Sweeper) GetSweepFee(ctx context.Context, case *btcutil.AddressPubKeyHash: weightEstimate.AddP2PKHOutput() default: - return 0, fmt.Errorf("unknown adress type %T", destAddr) + return 0, fmt.Errorf("unknown address type %T", destAddr) } addInputEstimate(&weightEstimate) diff --git a/test/log.go b/test/log.go index 37fa69b..e351129 100644 --- a/test/log.go +++ b/test/log.go @@ -1,8 +1,9 @@ package test import ( - "github.com/btcsuite/btclog" "os" + + "github.com/btcsuite/btclog" ) // log is a logger that is initialized with no output filters. This diff --git a/test/testutils.go b/test/testutils.go index de7d7c7..0f441d2 100644 --- a/test/testutils.go +++ b/test/testutils.go @@ -91,5 +91,8 @@ func GetInvoice(hash lntypes.Hash, amt btcutil.Amount, memo string) ( // DumpGoroutines dumps all currently running goroutines. func DumpGoroutines() { - pprof.Lookup("goroutine").WriteTo(os.Stdout, 1) + err := pprof.Lookup("goroutine").WriteTo(os.Stdout, 1) + if err != nil { + panic(err) + } } diff --git a/test/timeout.go b/test/timeout.go index 1f6a8ce..52d4762 100644 --- a/test/timeout.go +++ b/test/timeout.go @@ -15,7 +15,10 @@ func Guard(t *testing.T) func() { go func() { select { case <-time.After(5 * time.Second): - pprof.Lookup("goroutine").WriteTo(os.Stdout, 1) + err := pprof.Lookup("goroutine").WriteTo(os.Stdout, 1) + if err != nil { + panic(err) + } panic("test timeout") case <-done: diff --git a/testcontext_test.go b/testcontext_test.go index 6e3ab7e..8b4f014 100644 --- a/testcontext_test.go +++ b/testcontext_test.go @@ -21,14 +21,6 @@ var ( 1, 1, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4, 4, 4, }) - testPrepayPreimage = lntypes.Preimage([32]byte{ - 1, 1, 1, 1, 2, 2, 2, 2, - 3, 3, 3, 3, 4, 4, 4, 4, - 1, 1, 1, 1, 2, 2, 2, 2, - 3, 3, 3, 3, 4, 4, 4, 5, - }) - - testStartingHeight = uint32(600) ) // testContext contains functionality to support client unit tests. From a0a03b49726679da1f6bc5703b3164cd6cf7e546 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Mon, 7 Oct 2019 17:35:32 +0200 Subject: [PATCH 3/3] only lint new issues, add linting to travis run --- .golangci.yml | 43 +++++++++++++++++++++++++++++++++++++++++++ .travis.yml | 2 +- 2 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 .golangci.yml diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..91e5c21 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,43 @@ +run: + # timeout for analysis + deadline: 4m + + # Linting uses a lot of memory. Keep it under control by only running a single + # worker. + concurrency: 1 + +linters-settings: + gofmt: + # simplify code: gofmt with `-s` option, true by default + simplify: true + +linters: + enable-all: true + disable: + # Global variables are used in many places throughout the code base. + - gochecknoglobals + + # Some lines are over 80 characters on purpose and we don't want to make them + # even longer by marking them as 'nolint'. + - lll + + # We don't care (enough) about misaligned structs to lint that. + - maligned + + # We have long functions, especially in tests. Moving or renaming those would + # trigger funlen problems that we may not want to solve at that time. + - funlen + + # Disable for now as we haven't yet tuned the sensitivity to our codebase + # yet. Enabling by default for example, would also force new contributors to + # potentially extensively refactor code, when they want to smaller change to + # land. + - gocyclo + + # Instances of table driven tests that don't pre-allocate shouldn't trigger + # the linter. + - prealloc + +issues: + # Only show newly introduced problems. + new-from-rev: 36838cf7f464cf73b0201798063b2caffeae4250 diff --git a/.travis.yml b/.travis.yml index 3ee542c..cb7729c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -18,7 +18,7 @@ sudo: required script: - export GO111MODULE=on - - make unit + - make lint unit after_script: - echo "Uploading to termbin.com..." && find *.log | xargs -I{} sh -c "cat {} | nc termbin.com 9999 | xargs -r0 printf '{} uploaded to %s'"