housekeeping: cleaning code

Signed-off-by: Borja Aranda <borja.aranda@smartcontract.com>

pkg/timelock/health.go

@@ -50,9 +50,12 @@ func readyHandler(w http.ResponseWriter, r *http.Request) {
 
 func respond(status HealthStatus, w http.ResponseWriter) {
 	var err error
+
 	if status == HealthStatusOK {
 		_, err = w.Write([]byte("OK"))
-	} else {
+	}
+
+	if status == HealthStatusError {
 		w.WriteHeader(http.StatusInternalServerError)
 		_, err = w.Write([]byte("Error"))
 	}

pkg/timelock/operations.go

@@ -17,21 +17,20 @@ import (
 // - The operation is ready to be executed
 // Otherwise the operation will throw an info log and wait for a future tick.
 func (tw *Worker) execute(ctx context.Context, op []*contract.TimelockCallScheduled) {
-	if isReady(ctx, tw.contract, op[0].Id) {
-		tw.logger.Debug().Msgf("execute operation %x", op[0].Id)
+	if !isReady(ctx, tw.contract, op[0].Id) {
+		tw.logger.Info().Msgf("skipping operation %x: not ready", op[0].Id)
+	}
 
-		tx, err := executeCallSchedule(ctx, &tw.executeContract.TimelockTransactor, op, tw.privateKey)
-		if err != nil || tx == nil {
-			tw.logger.Error().Msgf("execute operation %x error: %s", op[0].Id, err.Error())
-		} else {
-			tw.logger.Info().Msgf("execute operation %x success: %s", op[0].Id, tx.Hash())
+	tw.logger.Debug().Msgf("execute operation %x", op[0].Id)
+	tx, err := executeCallSchedule(ctx, &tw.executeContract.TimelockTransactor, op, tw.privateKey)
+	if err != nil || tx == nil {
+		tw.logger.Error().Msgf("execute operation %x error: %s", op[0].Id, err.Error())
+	} else {
+		tw.logger.Info().Msgf("execute operation %x success: %s", op[0].Id, tx.Hash())
 
-			if _, err = bind.WaitMined(ctx, tw.ethClient, tx); err != nil {
-				tw.logger.Error().Msgf("execute operation %x error: %s", op[0].Id, err.Error())
-			}
+		if _, err = bind.WaitMined(ctx, tw.ethClient, tx); err != nil {
+			tw.logger.Error().Msgf("execute operation %x error: %s", op[0].Id, err.Error())
 		}
-	} else {
-		tw.logger.Info().Msgf("skipping operation %x: not ready", op[0].Id)
 	}
 }
 

pkg/timelock/operations_test.go

@@ -12,6 +12,11 @@ import (
 
 func Test_isOperation(t *testing.T) {
 	testWorker, _ := NewTimelockWorker(testNodeURL, testTimelockAddress, testCallProxyAddress, testPrivateKey, testFromBlock, int64(testPollPeriod), testLogger)
+
+	if testWorker == nil {
+		t.Skipf("testWorker is nil, are the env variables in const_test.go set?")
+	}
+
 	var ctx context.Context
 
 	type args struct {
@@ -54,6 +59,11 @@ func Test_isOperation(t *testing.T) {
 
 func Test_isReady(t *testing.T) {
 	testWorker, _ := NewTimelockWorker(testNodeURL, testTimelockAddress, testCallProxyAddress, testPrivateKey, testFromBlock, int64(testPollPeriod), testLogger)
+
+	if testWorker == nil {
+		t.Skipf("testWorker is nil, are the env variables in const_test.go set?")
+	}
+
 	var ctx context.Context
 
 	type args struct {
@@ -96,6 +106,11 @@ func Test_isReady(t *testing.T) {
 
 func Test_isDone(t *testing.T) {
 	testWorker, _ := NewTimelockWorker(testNodeURL, testTimelockAddress, testCallProxyAddress, testPrivateKey, testFromBlock, int64(testPollPeriod), testLogger)
+
+	if testWorker == nil {
+		t.Skipf("testWorker is nil, are the env variables in const_test.go set?")
+	}
+
 	var ctx context.Context
 
 	type args struct {
@@ -138,6 +153,11 @@ func Test_isDone(t *testing.T) {
 
 func Test_isPending(t *testing.T) {
 	testWorker, _ := NewTimelockWorker(testNodeURL, testTimelockAddress, testCallProxyAddress, testPrivateKey, testFromBlock, int64(testPollPeriod), testLogger)
+
+	if testWorker == nil {
+		t.Skipf("testWorker is nil, are the env variables in const_test.go set?")
+	}
+
 	var ctx context.Context
 
 	type args struct {

pkg/timelock/scheduler.go

@@ -90,12 +90,12 @@ func (tw *Worker) runScheduler(ctx context.Context) {
 
 // updateSchedulerDelay updates the internal ticker delay, so it can be reconfigured while running.
 func (tw *Worker) updateSchedulerDelay(t time.Duration) {
-	if t > 0 {
-		tw.ticker.Reset(t)
-		tw.logger.Debug().Msgf("internal min delay changed to %v", t.String())
-	} else {
+	if t <= 0 {
 		tw.logger.Debug().Msgf("internal min delay not changed, invalid duration: %v", t.String())
 	}
+
+	tw.ticker.Reset(t)
+	tw.logger.Debug().Msgf("internal min delay changed to %v", t.String())
 }
 
 // addToScheduler adds a new CallSchedule operation safely to the store.
@@ -133,28 +133,31 @@ func (tw *Worker) isSchedulerBusy() bool {
 // dumpOperationStore dumps to the logger and to the log file the current scheduled unexecuted operations.
 // maps in go don't guarantee order, so that's why we have to find the earliest block.
 func (tw *Worker) dumpOperationStore(now func() time.Time) {
-	if len(tw.store) > 0 {
-		f, err := os.Create(logPath + logFile)
-		if err != nil {
-			tw.logger.Fatal().Msgf("unable to create %s: %s", logPath+logFile, err.Error())
-		}
-		defer f.Close()
+	if len(tw.store) <= 0 {
+		tw.logger.Info().Msgf("no operations to dump")
+		return
+	}
+
+	f, err := os.Create(logPath + logFile)
+	if err != nil {
+		tw.logger.Fatal().Msgf("unable to create %s: %s", logPath+logFile, err.Error())
+	}
+	defer f.Close()
 
-		tw.logger.Info().Msgf("generating logs with pending operations in %s", logPath+logFile)
+	tw.logger.Info().Msgf("generating logs with pending operations in %s", logPath+logFile)
 
-		// Get the earliest block from all the operations stored by sorting them.
-		blocks := make([]int, 0)
-		for _, op := range tw.store {
-			blocks = append(blocks, int(op[0].Raw.BlockNumber))
-		}
-		sort.Ints(blocks)
+	// Get the earliest block from all the operations stored by sorting them.
+	blocks := make([]int, 0)
+	for _, op := range tw.store {
+		blocks = append(blocks, int(op[0].Raw.BlockNumber))
+	}
+	sort.Ints(blocks)
 
-		w := bufio.NewWriter(f)
+	w := bufio.NewWriter(f)
 
-		writeOperationStore(w, tw.logger, tw.store, blocks[0], now)
+	writeOperationStore(w, tw.logger, tw.store, blocks[0], now)
 
-		w.Flush()
-	}
+	w.Flush()
 }
 
 // writeOperationStore writes the operations to the writer.

pkg/timelock/scheduler_test.go

@@ -47,6 +47,10 @@ func Test_newScheduler(t *testing.T) {
 func TestWorker_updateSchedulerDelay(t *testing.T) {
 	testWorker, _ := NewTimelockWorker(testNodeURL, testTimelockAddress, testCallProxyAddress, testPrivateKey, testFromBlock, int64(testPollPeriod), testLogger)
 
+	if testWorker == nil {
+		t.Skipf("testWorker is nil, are the env variables in const_test.go set?")
+	}
+
 	// Should never fail
 	testWorker.updateSchedulerDelay(1 * time.Second)
 	testWorker.updateSchedulerDelay(-1 * time.Second)
@@ -56,6 +60,10 @@ func TestWorker_updateSchedulerDelay(t *testing.T) {
 func TestWorker_isSchedulerBusy(t *testing.T) {
 	testWorker, _ := NewTimelockWorker(testNodeURL, testTimelockAddress, testCallProxyAddress, testPrivateKey, testFromBlock, int64(testPollPeriod), testLogger)
 
+	if testWorker == nil {
+		t.Skipf("testWorker is nil, are the env variables in const_test.go set?")
+	}
+
 	isBusy := testWorker.isSchedulerBusy()
 	assert.Equal(t, false, isBusy, "scheduler should be busy by default")
 
@@ -71,6 +79,10 @@ func TestWorker_isSchedulerBusy(t *testing.T) {
 func TestWorker_setSchedulerBusy(t *testing.T) {
 	testWorker, _ := NewTimelockWorker(testNodeURL, testTimelockAddress, testCallProxyAddress, testPrivateKey, testFromBlock, int64(testPollPeriod), testLogger)
 
+	if testWorker == nil {
+		t.Skipf("testWorker is nil, are the env variables in const_test.go set?")
+	}
+
 	testWorker.setSchedulerBusy()
 	isBusy := testWorker.isSchedulerBusy()
 	assert.Equal(t, true, isBusy, "scheduler should be busy after setSchedulerBusy()")
@@ -79,6 +91,10 @@ func TestWorker_setSchedulerBusy(t *testing.T) {
 func TestWorker_setSchedulerFree(t *testing.T) {
 	testWorker, _ := NewTimelockWorker(testNodeURL, testTimelockAddress, testCallProxyAddress, testPrivateKey, testFromBlock, int64(testPollPeriod), testLogger)
 
+	if testWorker == nil {
+		t.Skipf("testWorker is nil, are the env variables in const_test.go set?")
+	}
+
 	testWorker.setSchedulerFree()
 	isBusy := testWorker.isSchedulerBusy()
 	assert.Equal(t, false, isBusy, "scheduler shouldn't be busy after setSchedulerFree()")
@@ -131,6 +147,7 @@ func Test_dumpOperationStore(t *testing.T) {
 	nowFunc := func() time.Time {
 		return date
 	}
+
 	wantPrefix := fmt.Sprintf("Process stopped at %v\n", nowFunc().In(time.UTC))
 
 	// Write the store to the file.

pkg/timelock/timelock.go

@@ -241,14 +241,14 @@ func (tw *Worker) Listen(ctx context.Context) error {
 				// signal the channel sub.Err() to close it, leading to false nil errors.
 				if err != nil {
 					tw.logger.Info().Msgf("subscription: %s", err.Error())
-					loop = false
 					SetReadyStatus(HealthStatusError)
+					loop = false
 				}
 
 			case signal := <-stopCh:
 				tw.logger.Info().Msgf("received OS signal %s", signal)
-				loop = false
 				SetReadyStatus(HealthStatusError)
+				loop = false
 			}
 		}
 		wg.Done()
@@ -286,8 +286,8 @@ func (tw *Worker) startLog() {
 	if err != nil {
 		tw.logger.Info().Msgf("\tEOA address: unable to determine")
 	}
-	tw.logger.Info().Msgf("\tEOA address: %v", wallet)
 
+	tw.logger.Info().Msgf("\tEOA address: %v", wallet)
 	tw.logger.Info().Msgf("\tStarting from block: %v", tw.fromBlock)
 	tw.logger.Info().Msgf("\tPoll Period: %v", time.Duration(tw.pollPeriod*int64(time.Second)).String())
 }

pkg/timelock/timelock_test.go

@@ -15,6 +15,10 @@ import (
 func TestNewTimelockWorker(t *testing.T) {
 	testWorker, _ := NewTimelockWorker(testNodeURL, testTimelockAddress, testCallProxyAddress, testPrivateKey, testFromBlock, int64(testPollPeriod), testLogger)
 
+	if testWorker == nil {
+		t.Skipf("testWorker is nil, are the env variables in const_test.go set?")
+	}
+
 	type args struct {
 		nodeURL          string
 		timelockAddress  string
@@ -178,6 +182,11 @@ func Test_handleOSSignal(t *testing.T) {
 
 func TestWorker_startLog(t *testing.T) {
 	testWorker, _ := NewTimelockWorker(testNodeURL, testTimelockAddress, testCallProxyAddress, testPrivateKey, testFromBlock, int64(testPollPeriod), testLogger)
+
+	if testWorker == nil {
+		t.Skipf("testWorker is nil, are the env variables in const_test.go set?")
+	}
+
 	tests := []struct {
 		name string
 	}{