Fix issues identified during testing (#31)

* 🐛 Fix identified issues

* test

* 🔨 Code refactoring

* Add implementation to validates version specific handlers are registered before starting server

* 👌 Applied suggestions

* Update default config file

---------

Co-authored-by: hatef <[email protected]>

Makefile

@@ -6,6 +6,7 @@ APP_NAME = faultdetector
 GREEN = \033[1;32m
 BLUE = \033[1;34m
 COLOR_END = \033[0;39m
+CONFIG_PATH = $(realpath $(config))
 
 build: # Builds the application and create a binary at ./bin/
 	@echo "$(BLUE)» Building fault detector application binary... $(COLOR_END)"
@@ -58,18 +59,21 @@ docker-build: # Builds docker image
 
 .PHONY: docker-run
 docker-run: # Runs docker image, use `make docker-run config={PATH_TO_CONFIG_FILE}` to provide custom config and to provide slack access token use `make docker-run slack_access_token={ACCESS_TOKEN}`
-ifdef config
-	@echo "$(BLUE) Running docker image...$(COLOR_END)"
-ifdef slack_access_token
-	@docker run -p 8080:8080 -v $(config):/home/onchain/faultdetector/config.yaml -t -e SLACK_ACCESS_TOKEN_KEY=$(slack_access_token) $(APP_NAME)
-else
-	@docker run -p 8080:8080 -v $(config):/home/onchain/faultdetector/config.yaml -t $(APP_NAME)
-endif
+	@echo "$(BLUE) Starting docker container $(APP_NAME)...$(COLOR_END)"
+ifneq ($(and $(config),$(slack_access_token)),)
+	@docker run --name $(APP_NAME) -p 8080:8080 -d -v $(CONFIG_PATH):/home/onchain/faultdetector/config.yaml -t -e SLACK_ACCESS_TOKEN_KEY=$(slack_access_token) $(APP_NAME)
+else ifdef config
+	@docker run --name $(APP_NAME) -p 8080:8080 -d -v $(CONFIG_PATH):/home/onchain/faultdetector/config.yaml -t $(APP_NAME)
+else ifdef slack_access_token
+	@docker run --name $(APP_NAME) -p 8080:8080 -d -t -e SLACK_ACCESS_TOKEN_KEY=$(slack_access_token) $(APP_NAME)
 else
-	@echo "$(BLUE) Running docker image...$(COLOR_END)"
-	@docker run -p 8080:8080 $(APP_NAME)
+	@docker run --name $(APP_NAME) -p 8080:8080 -d $(APP_NAME)
 endif
 
+docker-stop:
+	@echo "$(BLUE) Stopping and removing docker container $(APP_NAME)...$(COLOR_END)"
+	@docker rm -f $(APP_NAME)
+
 .PHONY: help
 help: # Show help for each of the Makefile recipes
 	@grep -E '^[a-zA-Z0-9 -]+:.*#'  Makefile | sort | while read -r l; do printf "$(GREEN)$$(echo $$l | cut -f 1 -d':')$(COLOR_END):$$(echo $$l | cut -f 2- -d'#')\n"; done

cmd/faultdetector/main.go

@@ -50,7 +50,7 @@ func NewApp(ctx context.Context, logger log.Logger) (*App, error) {
 	flag.Parse()
 	config, err := getAppConfig(logger, *configFilepath)
 	if err != nil {
-		logger.Errorf("Failed at parsing config with error %w", err)
+		logger.Errorf("Failed at parsing config with error %v", err)
 		return nil, err
 	}
 
@@ -66,7 +66,7 @@ func NewApp(ctx context.Context, logger log.Logger) (*App, error) {
 			config.Notification,
 		)
 		if err != nil {
-			logger.Errorf("Failed to initialize notification service, error: %w", err)
+			logger.Errorf("Failed to initialize notification service, error: %v", err)
 			return nil, err
 		}
 	}
@@ -114,6 +114,7 @@ func (app *App) Start() {
 	app.wg.Add(1)
 	go app.faultDetector.Start()
 
+	app.apiServer.RegisterHandlersForVersions(app.faultDetector, app.config.Api.RegisterVersions, app.config.Api.BasePath)
 	app.wg.Add(1)
 	go app.apiServer.Start()
 
@@ -136,7 +137,7 @@ func (app *App) Start() {
 			app.logger.Errorf("Received error of %v", err)
 			if app.notification != nil {
 				if err := app.notification.Notify("Error while starting application"); err != nil {
-					app.logger.Errorf("Failed to send notification, error: %w", err)
+					app.logger.Errorf("Failed to send notification, error: %v", err)
 				}
 			}
 			return
@@ -148,7 +149,7 @@ func (app *App) stop() {
 	app.faultDetector.Stop()
 	err := app.apiServer.Stop()
 	if err != nil {
-		app.logger.Error("Server shutdown not successful: %w", err)
+		app.logger.Error("Server shutdown not successful: %v", err)
 	}
 }
 
@@ -158,13 +159,13 @@ func main() {
 
 	logger, err := log.NewDefaultProductionLogger()
 	if err != nil {
-		logger.Errorf("Failed to create logger, %w", err)
+		logger.Errorf("Failed to create logger, %v", err)
 		return
 	}
 
 	app, err := NewApp(ctx, logger)
 	if err != nil {
-		logger.Errorf("Failed to create app, %w", err)
+		logger.Errorf("Failed to create app, %v", err)
 		return
 	}
 
@@ -180,10 +181,10 @@ func getAppConfig(logger log.Logger, configFilepath string) (*config.Config, err
 	splits := strings.FieldsFunc(configFilenameWithExt, func(r rune) bool { return r == '.' })
 	configType := splits[len(splits)-1] // Config file extension
 
+	viper.AddConfigPath(configDir)
 	viper.AddConfigPath(".")
 	viper.AddConfigPath("..")
 	viper.AddConfigPath("$HOME/.op-fault-detector")
-	viper.AddConfigPath(configDir)
 	viper.SetConfigName(configFilenameWithExt)
 	viper.SetConfigType(configType)
 	err := viper.ReadInConfig()

cmd/faultdetector/main_test.go

@@ -102,6 +102,7 @@ func prepareFaultDetector(t *testing.T, ctx context.Context, logger log.Logger,
 	if !mock {
 		fd, _ = faultdetector.NewFaultDetector(ctx, logger, erroChan, wg, config.FaultDetectorConfig, reg, testNotificationService)
 	} else {
+		mx := new(sync.RWMutex)
 		metrics := faultdetector.NewFaultDetectorMetrics(reg)
 
 		// Create chain API clients
@@ -136,7 +137,7 @@ func prepareFaultDetector(t *testing.T, ctx context.Context, logger log.Logger,
 			L2OutputIndex: 2,
 		}, nil)
 
-		fd = faultdetector.GetFaultDetector(ctx, logger, l1RpcApi, l2RpcApi, oracle, faultProofWindow, currentOutputIndex, metrics, testNotificationService, false, wg, erroChan)
+		fd = faultdetector.GetFaultDetector(ctx, logger, l1RpcApi, l2RpcApi, oracle, faultProofWindow, currentOutputIndex, metrics, testNotificationService, false, wg, erroChan, mx)
 	}
 
 	return fd

config.yaml

@@ -5,7 +5,7 @@ system:
 # API related configurations
 api:
   server:
-    host: "127.0.0.1"
+    host: "0.0.0.0"
     port: 8080
   base_path: "/api"
   register_versions:

pkg/api/handlers/v1/status.go

@@ -7,14 +7,13 @@ import (
 )
 
 type statusResponse struct {
-	Status string `json:"status"`
+	Ok bool `json:"ok"`
 }
 
 // GetStatus is the handler for the 'GET /api/v1/status' endpoint.
-func GetStatus(c *gin.Context) {
+func GetStatus(c *gin.Context, isFaultDetected bool) {
 	status := statusResponse{
-		Status: "ok",
+		Ok: !isFaultDetected,
 	}
-
 	c.IndentedJSON(http.StatusOK, status)
 }

pkg/api/server.go

@@ -8,13 +8,17 @@ import (
 	"sync"
 	"time"
 
+	"github.com/LiskHQ/op-fault-detector/pkg/api/handlers"
+	v1 "github.com/LiskHQ/op-fault-detector/pkg/api/handlers/v1"
 	"github.com/LiskHQ/op-fault-detector/pkg/api/middlewares"
-	"github.com/LiskHQ/op-fault-detector/pkg/api/routes"
 	"github.com/LiskHQ/op-fault-detector/pkg/config"
+	"github.com/LiskHQ/op-fault-detector/pkg/faultdetector"
 	"github.com/LiskHQ/op-fault-detector/pkg/log"
 	"github.com/gin-gonic/gin"
 )
 
+var areVersionsHandlerRegistered bool = false
+
 // HTTPServer embeds the http.Server along with the various other properties.
 type HTTPServer struct {
 	server    *http.Server
@@ -25,10 +29,37 @@ type HTTPServer struct {
 	errorChan chan error
 }
 
+// registerHandlers is responsible to register all handlers for routes without any base path.
+func registerHandlers(logger log.Logger, router *gin.Engine) {
+	router.GET("/ping", handlers.GetPing)
+}
+
+// RegisterHandlersForVersions is responsible to register API version specific route handlers.
+func (w *HTTPServer) RegisterHandlersForVersions(fd *faultdetector.FaultDetector, versions []string, basePath string) {
+	baseGroup := w.router.Group(basePath)
+	for _, version := range versions {
+		group := baseGroup.Group(version)
+		switch version {
+		case "v1":
+			group.GET("/status", func(c *gin.Context) {
+				v1.GetStatus(c, fd.IsFaultDetected())
+			})
+
+		default:
+			w.logger.Warningf("No routes and handlers defined for version %s. Please verify the API config.", version)
+		}
+	}
+	areVersionsHandlerRegistered = true
+}
+
 // Start starts the HTTP API server.
 func (w *HTTPServer) Start() {
 	defer w.wg.Done()
 
+	if !areVersionsHandlerRegistered {
+		w.errorChan <- fmt.Errorf("API specific versions handler are not registered")
+	}
+
 	w.logger.Infof("Starting the HTTP server on %s.", w.server.Addr)
 	err := w.server.ListenAndServe()
 	if err != nil {
@@ -72,13 +103,7 @@ func NewHTTPServer(ctx context.Context, logger log.Logger, wg *sync.WaitGroup, c
 	// Register handlers for routes without any base path
 	logger.Debug("Registering handlers for non-versioned endpoints.")
 
-	routes.RegisterHandlers(logger, router)
-
-	// Register handlers for routes following the base path
-	basePath := config.Api.BasePath
-	baseGroup := router.Group(basePath)
-	logger.Debugf("Registering handlers for endpoints under path '%s'.", basePath)
-	routes.RegisterHandlersByGroup(logger, baseGroup, config.Api.RegisterVersions)
+	registerHandlers(logger, router)
 
 	host := config.Api.Server.Host
 	port := config.Api.Server.Port

pkg/config/config.go

@@ -14,7 +14,7 @@ import (
 
 var (
 	providerEndpointRegex = regexp.MustCompile(`^(http|https|ws|wss)://`)
-	addressRegex          = regexp.MustCompile(`(\b0x[a-f0-9]{40}\b)`)
+	addressRegex          = regexp.MustCompile(`(\b0x[A-Fa-f0-9]{40}\b)`)
 	hostRegex             = regexp.MustCompile(`^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$`)
 	basePathRegex         = regexp.MustCompile(`^/?api$`)
 	registerVersionRegex  = regexp.MustCompile(`^v[1-9]\d*$`)