Config: OrderManager upgrade (#1737)

* Config: Allow missing Versions

This allows easier development of non-stacked version upgrades.
Though the PRs still need to be merged sequentially, or renumbered right
before merging

* Config: Move OrderManager upgrade to Version management

backtester/engine/setup.go

@@ -31,7 +31,6 @@ import (
 	"github.com/thrasher-corp/gocryptotrader/backtester/funding/trackingcurrencies"
 	"github.com/thrasher-corp/gocryptotrader/backtester/report"
 	gctcommon "github.com/thrasher-corp/gocryptotrader/common"
-	"github.com/thrasher-corp/gocryptotrader/common/convert"
 	"github.com/thrasher-corp/gocryptotrader/common/key"
 	gctconfig "github.com/thrasher-corp/gocryptotrader/config"
 	"github.com/thrasher-corp/gocryptotrader/currency"
@@ -211,7 +210,7 @@ func (bt *BackTest) SetupFromConfig(cfg *config.Config, templatePath, output str
 	bt.orderManager, err = engine.SetupOrderManager(bt.exchangeManager, &engine.CommunicationManager{}, &sync.WaitGroup{}, &gctconfig.OrderManager{
 		Verbose:                       verbose,
 		ActivelyTrackFuturesPositions: trackFuturesPositions,
-		RespectOrderHistoryLimits:     convert.BoolPtr(true),
+		RespectOrderHistoryLimits:     true,
 	})
 	if err != nil {
 		return err

config/config.go

@@ -1330,24 +1330,6 @@ func (c *Config) CheckCurrencyStateManager() {
 	}
 }
 
-// CheckOrderManagerConfig ensures the order manager is setup correctly
-func (c *Config) CheckOrderManagerConfig() {
-	m.Lock()
-	defer m.Unlock()
-	if c.OrderManager.Enabled == nil {
-		c.OrderManager.Enabled = convert.BoolPtr(true)
-		c.OrderManager.ActivelyTrackFuturesPositions = true
-	}
-	if c.OrderManager.RespectOrderHistoryLimits == nil {
-		c.OrderManager.RespectOrderHistoryLimits = convert.BoolPtr(true)
-	}
-	if c.OrderManager.ActivelyTrackFuturesPositions && c.OrderManager.FuturesTrackingSeekDuration >= 0 {
-		// one isn't likely to have a perpetual futures order open
-		// for longer than a year
-		c.OrderManager.FuturesTrackingSeekDuration = -time.Hour * 24 * 365
-	}
-}
-
 // CheckConnectionMonitorConfig checks and if zero value assigns default values
 func (c *Config) CheckConnectionMonitorConfig() {
 	m.Lock()
@@ -1662,7 +1644,6 @@ func (c *Config) CheckConfig() error {
 	c.CheckConnectionMonitorConfig()
 	c.CheckDataHistoryMonitorConfig()
 	c.CheckCurrencyStateManager()
-	c.CheckOrderManagerConfig()
 	c.CheckCommunicationsConfig()
 	c.CheckClientBankAccounts()
 	c.CheckBankAccountConfig()

config/config_types.go

@@ -128,11 +128,11 @@ type EncryptionKeyProvider func(confirmKey bool) ([]byte, error)
 
 // OrderManager holds settings used for the order manager
 type OrderManager struct {
-	Enabled                       *bool         `json:"enabled"`
+	Enabled                       bool          `json:"enabled"`
 	Verbose                       bool          `json:"verbose"`
 	ActivelyTrackFuturesPositions bool          `json:"activelyTrackFuturesPositions"`
 	FuturesTrackingSeekDuration   time.Duration `json:"futuresTrackingSeekDuration"`
-	RespectOrderHistoryLimits     *bool         `json:"respectOrderHistoryLimits"`
+	RespectOrderHistoryLimits     bool          `json:"respectOrderHistoryLimits"`
 	CancelOrdersOnShutdown        bool          `json:"cancelOrdersOnShutdown"`
 }
 

config/versions/v5.go

@@ -0,0 +1,72 @@
+package versions
+
+import (
+	"context"
+	"errors"
+	"strconv"
+	"time"
+
+	"github.com/buger/jsonparser"
+)
+
+// Version5 implements ConfigVersion
+type Version5 struct{}
+
+func init() {
+	Manager.registerVersion(5, &Version5{})
+}
+
+// defaultConfig contains the stateless V5 representation of orderbookManager
+// Note: Do not be tempted to use a constant for Duration. Whilst defaults are still written to config, we need to manage default upgrades discretely.
+var defaultFuturesTrackingSeekDuration = strconv.FormatInt(int64(time.Hour)*24*365, 10)
+
+var defaultConfig = []byte(`{
+  "enabled": true,
+  "verbose": false,
+  "activelyTrackFuturesPositions": true,
+  "futuresTrackingSeekDuration": ` + defaultFuturesTrackingSeekDuration + `,
+  "cancelOrdersOnShutdown": false,
+  "respectOrderHistoryLimits": true
+ }`)
+
+// UpgradeConfig handles upgrading config for OrderManager:
+// * Sets OrderManager config to defaults if it doesn't exist or enabled is null
+// * Sets respectOrderHistoryLimits to true if it doesn't exist or is null
+// * Sets futuresTrackingSeekDuration to positive if it's negative
+func (v *Version5) UpgradeConfig(_ context.Context, e []byte) ([]byte, error) {
+	_, valueType, _, err := jsonparser.Get(e, "orderManager", "enabled")
+	switch {
+	case errors.Is(err, jsonparser.KeyPathNotFoundError), valueType == jsonparser.Null:
+		return jsonparser.Set(e, defaultConfig, "orderManager")
+	case err != nil:
+		return e, err
+	}
+
+	_, valueType, _, err = jsonparser.Get(e, "orderManager", "respectOrderHistoryLimits")
+	if errors.Is(err, jsonparser.KeyPathNotFoundError) || valueType == jsonparser.Null {
+		if e, err = jsonparser.Set(e, []byte(`true`), "orderManager", "respectOrderHistoryLimits"); err != nil {
+			return e, err
+		}
+	}
+
+	if i, err := jsonparser.GetInt(e, "orderManager", "futuresTrackingSeekDuration"); err != nil {
+		if e, err = jsonparser.Set(e, []byte(defaultFuturesTrackingSeekDuration), "orderManager", "futuresTrackingSeekDuration"); err != nil {
+			return e, err
+		}
+	} else if i < 0 {
+		if e, err = jsonparser.Set(e, []byte(strconv.FormatInt(-i, 10)), "orderManager", "futuresTrackingSeekDuration"); err != nil {
+			return e, err
+		}
+	}
+	return e, nil
+}
+
+// DowngradeConfig just reverses the futuresTrackingSeekDuration to negative, and leaves everything else alone
+func (v *Version5) DowngradeConfig(_ context.Context, e []byte) ([]byte, error) {
+	if i, err := jsonparser.GetInt(e, "orderManager", "futuresTrackingSeekDuration"); err == nil && i > 0 {
+		if e, err = jsonparser.Set(e, []byte(strconv.FormatInt(-i, 10)), "orderManager", "futuresTrackingSeekDuration"); err != nil {
+			return e, err
+		}
+	}
+	return e, nil
+}

config/versions/v5_test.go

@@ -0,0 +1,65 @@
+package versions
+
+import (
+	"bytes"
+	"context"
+	"encoding/json" //nolint:depguard // Direct use of golang json for Compact func
+	"strings"
+	"testing"
+
+	"github.com/buger/jsonparser"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+var (
+	expDef   = `{"orderManager":{"enabled":true,"verbose":false,"activelyTrackFuturesPositions":true,"futuresTrackingSeekDuration":31536000000000000,"cancelOrdersOnShutdown":false,"respectOrderHistoryLimits":true}}`
+	expUser1 = `{"orderManager":{"enabled":false,"verbose":true,"activelyTrackFuturesPositions":false,"futuresTrackingSeekDuration":47000,"cancelOrdersOnShutdown":true,"respectOrderHistoryLimits":true}}`
+	expUser2 = strings.Replace(expUser1, `mits":true`, `mits":false`, 1)
+)
+
+var tests = []struct {
+	name string
+	in   string
+	out  string
+	err  error
+}{
+	{name: "Bad input should error", err: jsonparser.KeyPathNotFoundError},
+	{name: "Missing orderManager should use the defaults", in: "{}", out: expDef},
+	{name: "Enabled null should use defaults", in: strings.Replace(expDef, "true", "null", 1), out: expDef},
+	{name: "RespectOrderHistoryLimits should be added if missing", in: strings.Replace(expUser1, `,"respectOrderHistoryLimits":true`, "", 1), out: expUser1},
+	{name: "RespectOrderHistoryLimits null should default true", in: strings.Replace(expUser1, `mits":true`, `mits":null`, 1), out: expUser1},
+	{name: "FutureTracking should be reversed", in: strings.Replace(expUser1, "47", "-47", 1), out: expUser1},
+	{name: "Configured orderManager should be left alone", in: expUser2, out: expUser2},
+}
+
+func TestVersion5Upgrade(t *testing.T) {
+	t.Parallel()
+	for _, tt := range tests {
+		_ = t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			out, err := new(Version5).UpgradeConfig(context.Background(), []byte(tt.in))
+			if tt.err != nil {
+				require.ErrorIs(t, err, tt.err)
+				return
+			}
+			require.NoError(t, err)
+			b := new(bytes.Buffer)
+			require.NoError(t, json.Compact(b, out), "json.Compact must not error")
+			require.Equal(t, tt.out, b.String())
+		})
+	}
+}
+
+func TestVersion5Downgrade(t *testing.T) {
+	t.Parallel()
+
+	expReversed := strings.Replace(expUser1, "47", "-47", 1)
+	out, err := new(Version5).DowngradeConfig(context.Background(), []byte(expUser1))
+	require.NoError(t, err)
+	assert.Equal(t, expReversed, string(out), "DowngradeConfig should just reverse the futuresTrackingSeekDuration")
+
+	out, err = new(Version5).DowngradeConfig(context.Background(), []byte(expReversed))
+	require.NoError(t, err)
+	assert.Equal(t, expReversed, string(out), "DowngradeConfig should leave an already negative futuresTrackingSeekDuration alone")
+}

config/versions/versions.go

@@ -31,7 +31,6 @@ import (
 const UseLatestVersion = math.MaxUint16
 
 var (
-	errMissingVersion        = errors.New("missing version")
 	errVersionIncompatible   = errors.New("version does not implement ConfigVersion or ExchangeVersion")
 	errModifyingExchange     = errors.New("error modifying exchange config")
 	errNoVersions            = errors.New("error retrieving latest config version: No config versions are registered")
@@ -124,10 +123,20 @@ func (m *manager) Deploy(ctx context.Context, j []byte, version uint16) ([]byte,
 			exchMethod = ExchangeVersion.DowngradeExchange
 		}
 
-		log.Printf("Running %s config version %v\n", action, patchVersion)
-
 		patch := m.versions[patchVersion]
 
+		current = patchVersion
+		if target < current {
+			current = patchVersion - 1
+		}
+
+		if patch == nil {
+			log.Printf("Skipping missing config version %v\n", patchVersion)
+			continue
+		}
+
+		log.Printf("Running %s config version %v\n", action, patchVersion)
+
 		if cPatch, ok := patch.(ConfigVersion); ok {
 			if j, err = configMethod(cPatch, ctx, j); err != nil {
 				return j, fmt.Errorf("%w %s %v: %w", errApplyingVersion, action, patchVersion, err)
@@ -226,13 +235,10 @@ func (m *manager) checkVersions() error {
 	defer m.m.RUnlock()
 	for ver, v := range m.versions {
 		switch v.(type) {
-		case ExchangeVersion, ConfigVersion:
+		case ExchangeVersion, ConfigVersion, nil:
 		default:
 			return fmt.Errorf("%w: %v", errVersionIncompatible, ver)
 		}
-		if v == nil {
-			return fmt.Errorf("%w: v%v", errMissingVersion, ver)
-		}
 	}
 	return nil
 }

engine/engine.go

@@ -165,7 +165,7 @@ func validateSettings(b *Engine, s *Settings, flagSet FlagSet) {
 	b.Settings = *s
 
 	flagSet.WithBool("coinmarketcap", &b.Settings.EnableCoinmarketcapAnalysis, b.Config.Currency.CryptocurrencyProvider.Enabled)
-	flagSet.WithBool("ordermanager", &b.Settings.EnableOrderManager, b.Config.OrderManager.Enabled != nil && *b.Config.OrderManager.Enabled)
+	flagSet.WithBool("ordermanager", &b.Settings.EnableOrderManager, b.Config.OrderManager.Enabled)
 
 	flagSet.WithBool("currencyconverter", &b.Settings.EnableCurrencyConverter, b.Config.Currency.ForexProviders.IsEnabled("currencyconverter"))
 

engine/order_manager.go

@@ -39,15 +39,15 @@ func SetupOrderManager(exchangeManager iExchangeManager, communicationsManager i
 	if cfg == nil {
 		return nil, fmt.Errorf("%w OrderManager", errNilConfig)
 	}
-
-	var respectOrderHistoryLimits bool
-	if cfg.RespectOrderHistoryLimits != nil {
-		respectOrderHistoryLimits = *cfg.RespectOrderHistoryLimits
+	if cfg.ActivelyTrackFuturesPositions && cfg.FuturesTrackingSeekDuration <= 0 {
+		return nil, errInvalidFuturesTrackingSeekDuration
 	}
+
 	om := &OrderManager{
 		shutdown:                      make(chan struct{}),
 		activelyTrackFuturesPositions: cfg.ActivelyTrackFuturesPositions,
-		respectOrderHistoryLimits:     respectOrderHistoryLimits,
+		futuresPositionSeekDuration:   cfg.FuturesTrackingSeekDuration,
+		respectOrderHistoryLimits:     cfg.RespectOrderHistoryLimits,
 		orderStore: store{
 			Orders:                    make(map[string][]*order.Detail),
 			exchangeManager:           exchangeManager,
@@ -60,15 +60,6 @@ func SetupOrderManager(exchangeManager iExchangeManager, communicationsManager i
 			CancelOrdersOnShutdown: cfg.CancelOrdersOnShutdown,
 		},
 	}
-	if cfg.ActivelyTrackFuturesPositions {
-		if cfg.FuturesTrackingSeekDuration > 0 {
-			cfg.FuturesTrackingSeekDuration *= -1
-		}
-		if cfg.FuturesTrackingSeekDuration == 0 {
-			cfg.FuturesTrackingSeekDuration = defaultOrderSeekTime
-		}
-		om.futuresPositionSeekDuration = cfg.FuturesTrackingSeekDuration
-	}
 	return om, nil
 }
 
@@ -728,7 +719,7 @@ func (m *OrderManager) processOrders() {
 					return
 				}
 				if sd.IsZero() {
-					sd = time.Now().Add(m.futuresPositionSeekDuration)
+					sd = time.Now().Add(-m.futuresPositionSeekDuration)
 				}
 				positions, err = exchanges[x].GetFuturesPositionOrders(context.TODO(), &futures.PositionsRequest{
 					Asset:                     enabledAssets[y],

engine/order_manager_test.go

@@ -13,7 +13,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/thrasher-corp/gocryptotrader/common"
-	"github.com/thrasher-corp/gocryptotrader/common/convert"
 	"github.com/thrasher-corp/gocryptotrader/config"
 	"github.com/thrasher-corp/gocryptotrader/currency"
 	exchange "github.com/thrasher-corp/gocryptotrader/exchanges"
@@ -162,30 +161,23 @@ func (f omfExchange) GetFuturesPositions(_ context.Context, req *futures.Positio
 
 func TestSetupOrderManager(t *testing.T) {
 	_, err := SetupOrderManager(nil, nil, nil, nil)
-	if !errors.Is(err, errNilExchangeManager) {
-		t.Errorf("error '%v', expected '%v'", err, errNilExchangeManager)
-	}
+	assert.ErrorIs(t, err, errNilExchangeManager)
+
 	_, err = SetupOrderManager(NewExchangeManager(), nil, nil, nil)
-	if !errors.Is(err, errNilCommunicationsManager) {
-		t.Errorf("error '%v', expected '%v'", err, errNilCommunicationsManager)
-	}
+	assert.ErrorIs(t, err, errNilCommunicationsManager)
+
 	_, err = SetupOrderManager(NewExchangeManager(), &CommunicationManager{}, nil, &config.OrderManager{})
-	if !errors.Is(err, errNilWaitGroup) {
-		t.Errorf("error '%v', expected '%v'", err, errNilWaitGroup)
-	}
+	assert.ErrorIs(t, err, errNilWaitGroup)
+
 	var wg sync.WaitGroup
 	_, err = SetupOrderManager(NewExchangeManager(), &CommunicationManager{}, &wg, &config.OrderManager{})
-	if !errors.Is(err, nil) {
-		t.Errorf("error '%v', expected '%v'", err, nil)
-	}
-	_, err = SetupOrderManager(NewExchangeManager(), &CommunicationManager{}, &wg, &config.OrderManager{ActivelyTrackFuturesPositions: true, FuturesTrackingSeekDuration: 0})
-	if !errors.Is(err, nil) {
-		t.Errorf("error '%v', expected '%v'", err, nil)
-	}
+	require.NoError(t, err)
+
+	_, err = SetupOrderManager(NewExchangeManager(), &CommunicationManager{}, &wg, &config.OrderManager{ActivelyTrackFuturesPositions: true})
+	require.ErrorIs(t, err, errInvalidFuturesTrackingSeekDuration)
+
 	_, err = SetupOrderManager(NewExchangeManager(), &CommunicationManager{}, &wg, &config.OrderManager{ActivelyTrackFuturesPositions: true, FuturesTrackingSeekDuration: time.Hour})
-	if !errors.Is(err, nil) {
-		t.Errorf("error '%v', expected '%v'", err, nil)
-	}
+	require.NoError(t, err)
 }
 
 func TestOrderManagerStart(t *testing.T) {
@@ -1546,7 +1538,7 @@ func TestGetOpenFuturesPosition(t *testing.T) {
 		t.Fatalf("received: '%v' but expected: '%v'", err, nil)
 	}
 	o, err = SetupOrderManager(em, &CommunicationManager{}, wg, &config.OrderManager{
-		Enabled:                       convert.BoolPtr(true),
+		Enabled:                       true,
 		FuturesTrackingSeekDuration:   time.Hour,
 		Verbose:                       true,
 		ActivelyTrackFuturesPositions: true,

engine/order_manager_types.go

@@ -13,20 +13,20 @@ import (
 // OrderManagerName is an exported subsystem name
 const OrderManagerName = "orders"
 
-// vars for the fund manager package
+// Public Errors
 var (
-	// ErrOrdersAlreadyExists occurs when the order already exists in the manager
-	ErrOrdersAlreadyExists = errors.New("order already exists")
-	// ErrOrderIDCannotBeEmpty occurs when an order does not have an ID
+	ErrOrdersAlreadyExists  = errors.New("order already exists")
 	ErrOrderIDCannotBeEmpty = errors.New("orderID cannot be empty")
-	// ErrOrderNotFound occurs when an order is not found in the orderstore
-	ErrOrderNotFound = errors.New("order does not exist")
+	ErrOrderNotFound        = errors.New("order does not exist")
+)
 
+var (
 	errNilCommunicationsManager = errors.New("cannot start with nil communications manager")
 	errNilOrder                 = errors.New("nil order received")
 	errFuturesTrackingDisabled  = errors.New("tracking futures positions disabled. enable it via config under orderManager activelyTrackFuturesPositions")
 	orderManagerInterval        = time.Second * 10
-	defaultOrderSeekTime        = -time.Hour * 24 * 365
+
+	errInvalidFuturesTrackingSeekDuration = errors.New("invalid config value for futuresTrackingSeekDuration")
 )
 
 type orderManagerConfig struct {