Config: AssetEnabled upgrade (#1735)

* Config: Move assetEnabled upgrade to Version management

* Assets: Do not error on asset not enabled, or disabled

This became more messy with Disabling something that's defaulted to
disabled.
Taking an idealogical stance against erroring that what you want to have
done is already done.

* CurrencyManager: Set AssetEnabled when StorePairs(enabled)

* RPCServer: Fix tests expecting StoreAssetPairFormat to enable the asset

Also assertifies

* Bitfinex: Fix tests for MarginFunding subs

* GCTWrapper: Improve TestMain clarity

* BTSE: Add futures to testconfig

* Exchanges: Rename StoreAssetPairStore

Previously we were calling it "Format", but accepting everything from
the PairStore.
We were also defaulting to turning the Asset on.

Now callers need to get their AssetEnabled set as they want it, so
there's no magic

This change also moves responsibility for error wrapping outside to the
caller.

* Config: AssetEnabled upgrade should respect assetTypes

Previously we ignored the field and just turned on everything.
I think that was because we couldn't get at the old value.
In either case, we have the option to do better, and respect the
assetEnabled value

* Config: Improve exchange config version upgrade error messages

config/config.go

@@ -904,16 +904,6 @@ func (c *Config) CheckExchangeConfigValues() error {
 			continue
 		}
 
-		for _, a := range assets {
-			if err := e.CurrencyPairs.IsAssetEnabled(a); errors.Is(err, currency.ErrAssetIsNil) {
-				// Checks if we have an old config without the ability to enable disable the entire asset
-				log.Warnf(log.ConfigMgr, "Exchange %s: upgrading config for asset type %s and setting enabled.\n", e.Name, a)
-				if err := e.CurrencyPairs.SetAssetEnabled(a, true); err != nil {
-					return err
-				}
-			}
-		}
-
 		if enabled := e.CurrencyPairs.GetAssetTypes(true); len(enabled) == 0 {
 			// turn on an asset if all disabled
 			log.Warnf(log.ConfigMgr, "%s assets disabled, turning on asset %s", e.Name, assets[0])

config/config_test.go

@@ -720,10 +720,10 @@ func TestCheckPairConsistency(t *testing.T) {
 	t.Parallel()
 
 	var c Config
-	err := c.CheckPairConsistency("asdf")
-	if !errors.Is(err, ErrExchangeNotFound) {
-		t.Fatalf("received: '%v' buy expected: '%v'", err, ErrExchangeNotFound)
-	}
+	p1 := currency.NewPairWithDelimiter("LTC", "USD", "_")
+	p2 := currency.NewPairWithDelimiter("BTC", "USD", "_")
+
+	assert.ErrorIs(t, c.CheckPairConsistency("asdf"), ErrExchangeNotFound)
 
 	c.Exchanges = append(c.Exchanges,
 		Exchange{
@@ -731,18 +731,9 @@ func TestCheckPairConsistency(t *testing.T) {
 		},
 	)
 
-	// Test nil pair store
-	err = c.CheckPairConsistency(testFakeExchangeName)
-	if !errors.Is(err, errPairsManagerIsNil) {
-		t.Fatalf("received: '%v' buy expected: '%v'", err, errPairsManagerIsNil)
-	}
+	assert.ErrorIs(t, c.CheckPairConsistency(testFakeExchangeName), errPairsManagerIsNil)
 
-	enabled, err := currency.NewPairDelimiter("BTC_USD", "_")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	c.Exchanges[0].CurrencyPairs = &currency.PairsManager{
+	pm := &currency.PairsManager{
 		Pairs: map[asset.Item]*currency.PairStore{
 			asset.Spot: {
 				RequestFormat: &currency.PairFormat{
@@ -754,123 +745,51 @@ func TestCheckPairConsistency(t *testing.T) {
 					Delimiter: "_",
 				},
 				Enabled: currency.Pairs{
-					enabled,
+					p2,
 				},
 			},
 		},
 	}
+	c.Exchanges[0].CurrencyPairs = pm
 
-	// Test for nil avail pairs
-	err = c.CheckPairConsistency(testFakeExchangeName)
-	if !errors.Is(err, nil) {
-		t.Fatalf("received: '%v' buy expected: '%v'", err, nil)
-	}
-
-	p1, err := currency.NewPairDelimiter("LTC_USD", "_")
-	if err != nil {
-		t.Fatal(err)
-	}
+	assert.NoError(t, c.CheckPairConsistency(testFakeExchangeName), "Should not error on empty available pairs")
+	assert.Empty(t, pm.Pairs[asset.Spot].Enabled, "Unavailable pairs should be removed from enabled")
 
 	// Test that enabled pair is not found in the available pairs
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Available = currency.Pairs{
-		p1,
-	}
+	pm.Pairs[asset.Spot].Available = currency.Pairs{p1}
 
 	// LTC_USD is only found in the available pairs list and should therefore
-	// be added to the enabled pairs list due to the atLestOneEnabled code
-	err = c.CheckPairConsistency(testFakeExchangeName)
-	if err != nil {
-		t.Fatal(err)
-	}
+	// be added to the enabled pairs list due to the atLeastOneEnabled code
+	assert.NoError(t, c.CheckPairConsistency(testFakeExchangeName), "Should not error when adding a pair from available to enabled")
+	require.Equal(t, 1, len(pm.Pairs[asset.Spot].Enabled), "One pair must be enabled")
+	assert.True(t, slices.Contains(pm.Pairs[asset.Spot].Enabled, p1), "Newly enabled pair should be in Enabled")
 
-	if len(c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Enabled) != 1 {
-		t.Fatal("there should be at least one pair located in this list")
-	}
+	pm.Pairs[asset.Spot].Available = currency.Pairs{p1, p2}
+	assert.NoError(t, c.CheckPairConsistency(testFakeExchangeName), "Should not error with no changes to be made")
 
-	if !c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Enabled[0].Equal(p1) {
-		t.Fatal("LTC_USD should be contained in the enabled pairs list")
-	}
+	pm.Pairs[asset.Spot].Enabled = nil
+	assert.NoError(t, c.CheckPairConsistency(testFakeExchangeName), "Should not error when adding a pair from available to enabled to fulfil atLeastOne")
+	assert.NotEmpty(t, pm.Pairs[asset.Spot].Enabled, "One pair should be enabled")
 
-	p2, err := currency.NewPairDelimiter("BTC_USD", "_")
-	if err != nil {
-		t.Fatal(err)
-	}
+	pm.Pairs[asset.Spot].Enabled = currency.Pairs{p1, p2}
+	assert.NoError(t, c.CheckPairConsistency(testFakeExchangeName), "CheckPairConsistency should not error with when removing an invalid pair")
 
-	// Add the BTC_USD pair and see result
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Available = currency.Pairs{
-		p1, p2,
-	}
+	assert.NoError(t, c.CheckPairConsistency(testFakeExchangeName), "CheckPairConsistency should not error with consistent pairs")
 
-	if err := c.CheckPairConsistency(testFakeExchangeName); err != nil {
-		t.Fatal(err)
-	}
+	pm.Pairs[asset.Spot].AssetEnabled = true
+	pm.Pairs[asset.Spot].Enabled = currency.Pairs{}
+	assert.NoError(t, c.CheckPairConsistency(testFakeExchangeName), "CheckPairConsistency should not error with spot asset enabled but no pairs")
 
-	// Test that an empty enabled pair is populated with an available pair
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Enabled = nil
-	if err := c.CheckPairConsistency(testFakeExchangeName); err != nil {
-		t.Error("unexpected result", err)
-	}
+	pm.Pairs[asset.Spot].AssetEnabled = true
+	pm.Pairs[asset.Spot].Enabled = currency.Pairs{currency.NewPair(currency.DASH, currency.USD)}
+	assert.NoError(t, c.CheckPairConsistency(testFakeExchangeName), "CheckPairConsistency should not error with spot asset enabled and enabled pairs")
 
-	if len(c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Enabled) != 1 {
-		t.Fatal("should be populated with at least one currency pair")
-	}
+	pm.Pairs[asset.Spot].AssetEnabled = false
+	pm.Pairs[asset.Spot].Enabled = currency.Pairs{}
+	assert.NoError(t, c.CheckPairConsistency(testFakeExchangeName), "CheckPairConsistency should not error with spot asset disabled and no enabled pairs")
 
-	// Test that an invalid enabled pair is removed from the list
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Enabled = currency.Pairs{
-		p1,
-		p2,
-	}
-	if err := c.CheckPairConsistency(testFakeExchangeName); err != nil {
-		t.Error("unexpected result")
-	}
-
-	// Test when no update is required as the available pairs and enabled pairs
-	// are consistent
-	if err := c.CheckPairConsistency(testFakeExchangeName); err != nil {
-		t.Error("unexpected result")
-	}
-
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].AssetEnabled = convert.BoolPtr(true)
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Enabled = currency.Pairs{}
-
-	// Test no conflict and at least one on enabled asset type
-	if err := c.CheckPairConsistency(testFakeExchangeName); err != nil {
-		t.Error("unexpected result")
-	}
-
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].AssetEnabled = convert.BoolPtr(true)
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Enabled = currency.Pairs{currency.NewPair(currency.DASH, currency.USD)}
-
-	// Test with conflict and at least one on enabled asset type
-	if err := c.CheckPairConsistency(testFakeExchangeName); err != nil {
-		t.Error("unexpected result")
-	}
-
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].AssetEnabled = convert.BoolPtr(false)
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Enabled = currency.Pairs{}
-
-	// Test no conflict and at least one on disabled asset type
-	if err := c.CheckPairConsistency(testFakeExchangeName); err != nil {
-		t.Error("unexpected result")
-	}
-
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Enabled = currency.Pairs{
-		currency.NewPair(currency.DASH, currency.USD),
-		p1,
-		p2,
-	}
-
-	// Test with conflict and at least one on disabled asset type
-	if err := c.CheckPairConsistency(testFakeExchangeName); err != nil {
-		t.Error("unexpected result")
-	}
-
-	c.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].AssetEnabled = nil
-
-	// assetType enabled failure check
-	if err := c.CheckPairConsistency(testFakeExchangeName); err != nil {
-		t.Error("unexpected result")
-	}
+	pm.Pairs[asset.Spot].Enabled = currency.Pairs{currency.NewPair(currency.DASH, currency.USD), p1, p2}
+	assert.NoError(t, c.CheckPairConsistency(testFakeExchangeName), "CheckPairConsistency should not error with spot asset disabled but enabled pairs")
 }
 
 func TestSupportsPair(t *testing.T) {
@@ -884,7 +803,7 @@ func TestSupportsPair(t *testing.T) {
 				CurrencyPairs: &currency.PairsManager{
 					Pairs: map[asset.Item]*currency.PairStore{
 						asset.Spot: {
-							AssetEnabled:  convert.BoolPtr(true),
+							AssetEnabled:  true,
 							Available:     []currency.Pair{currency.NewPair(currency.BTC, currency.USD)},
 							ConfigFormat:  fmt,
 							RequestFormat: fmt,
@@ -1538,7 +1457,7 @@ func TestCheckExchangeConfigValues(t *testing.T) {
 	cfg.Exchanges = append(cfg.Exchanges, cpy[0])
 
 	cfg.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].Enabled = nil
-	cfg.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].AssetEnabled = convert.BoolPtr(false)
+	cfg.Exchanges[0].CurrencyPairs.Pairs[asset.Spot].AssetEnabled = false
 	err = cfg.CheckExchangeConfigValues()
 	require.NoError(t, err)
 
@@ -1711,7 +1630,7 @@ func TestCheckConfig(t *testing.T) {
 					LastUpdated:     0,
 					Pairs: map[asset.Item]*currency.PairStore{
 						asset.Spot: {
-							AssetEnabled:  convert.BoolPtr(true),
+							AssetEnabled:  true,
 							Available:     currency.Pairs{cp1, cp2},
 							Enabled:       currency.Pairs{cp1},
 							ConfigFormat:  &currency.EMPTYFORMAT,

config/versions/v4.go

@@ -0,0 +1,86 @@
+package versions
+
+import (
+	"bytes"
+	"context"
+	"errors"
+	"fmt"
+	"strings"
+
+	"github.com/buger/jsonparser"
+)
+
+var (
+	errUpgradingAssetTypes    = errors.New("error upgrading assetTypes")
+	errUpgradingCurrencyPairs = errors.New("error upgrading currencyPairs.pairs")
+)
+
+// Version4 is an Exchange upgrade to move currencyPairs.assetTypes to currencyPairs.pairs.*.assetEnabled
+type Version4 struct {
+}
+
+func init() {
+	Manager.registerVersion(4, &Version4{})
+}
+
+// Exchanges returns all exchanges: "*"
+func (v *Version4) Exchanges() []string { return []string{"*"} }
+
+// UpgradeExchange sets AssetEnabled: true for all assets listed in assetTypes, and false for any with no field
+func (v *Version4) UpgradeExchange(_ context.Context, e []byte) ([]byte, error) {
+	toEnable := map[string]bool{}
+
+	assetTypesFn := func(asset []byte, valueType jsonparser.ValueType, _ int, _ error) {
+		if valueType == jsonparser.String {
+			toEnable[string(asset)] = true
+		}
+	}
+	_, err := jsonparser.ArrayEach(e, assetTypesFn, "currencyPairs", "assetTypes")
+	if err != nil && !errors.Is(err, jsonparser.KeyPathNotFoundError) {
+		return e, fmt.Errorf("%w: %w", errUpgradingAssetTypes, err)
+	}
+
+	assetEnabledFn := func(assetBytes []byte, v []byte, _ jsonparser.ValueType, _ int) (err error) {
+		asset := string(assetBytes)
+		if toEnable[asset] {
+			e, err = jsonparser.Set(e, []byte(`true`), "currencyPairs", "pairs", asset, "assetEnabled")
+		} else {
+			var vT jsonparser.ValueType
+			_, vT, _, err = jsonparser.Get(v, "assetEnabled")
+			switch {
+			case vT == jsonparser.Null, errors.Is(err, jsonparser.KeyPathNotFoundError):
+				e, err = jsonparser.Set(e, []byte(`false`), "currencyPairs", "pairs", asset, "assetEnabled")
+			case err == nil && vT != jsonparser.Boolean:
+				err = fmt.Errorf("assetEnabled: %w (`%s`)", jsonparser.UnknownValueTypeError, vT)
+			}
+		}
+		if err != nil {
+			err = fmt.Errorf("%w for asset `%s`", err, asset)
+		}
+		return err
+	}
+	if err = jsonparser.ObjectEach(bytes.Clone(e), assetEnabledFn, "currencyPairs", "pairs"); err != nil {
+		return e, fmt.Errorf("%w: %w", errUpgradingCurrencyPairs, err)
+	}
+	e = jsonparser.Delete(e, "currencyPairs", "assetTypes")
+	return e, err
+}
+
+// DowngradeExchange moves AssetEnabled assets into AssetType field
+func (v *Version4) DowngradeExchange(_ context.Context, e []byte) ([]byte, error) {
+	assetTypes := []string{}
+
+	assetEnabledFn := func(asset []byte, v []byte, _ jsonparser.ValueType, _ int) error {
+		if b, err := jsonparser.GetBoolean(v, "assetEnabled"); err == nil {
+			if b {
+				assetTypes = append(assetTypes, fmt.Sprintf("%q", asset))
+			}
+			e = jsonparser.Delete(e, "currencyPairs", "pairs", string(asset), "assetEnabled")
+		}
+		return nil
+	}
+	if err := jsonparser.ObjectEach(bytes.Clone(e), assetEnabledFn, "currencyPairs", "pairs"); err != nil {
+		return e, err
+	}
+	return jsonparser.Set(e, []byte(`[`+strings.Join(assetTypes, ",")+`]`), "currencyPairs", "assetTypes")
+}

config/versions/v4_test.go

@@ -0,0 +1,91 @@
+package versions
+
+import (
+	"bytes"
+	"context"
+	"testing"
+
+	"github.com/buger/jsonparser"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestVersion4ExchangeType(t *testing.T) {
+	t.Parallel()
+	assert.Implements(t, (*ExchangeVersion)(nil), new(Version4))
+}
+
+func TestVersion4Exchanges(t *testing.T) {
+	t.Parallel()
+	assert.Equal(t, []string{"*"}, new(Version4).Exchanges())
+}
+
+func TestVersion4Upgrade(t *testing.T) {
+	t.Parallel()
+
+	_, err := new(Version4).UpgradeExchange(context.Background(), []byte{})
+	require.ErrorIs(t, err, errUpgradingAssetTypes)
+
+	_, err = new(Version4).UpgradeExchange(context.Background(), []byte(`{}`))
+	require.ErrorIs(t, err, errUpgradingCurrencyPairs)
+
+	in := []byte(`{"name":"Cracken","currencyPairs":{"assetTypes":["spot"],"pairs":{"spot":{"enabled":"BTC-AUD","available":"BTC-AUD"},"futures":{"assetEnabled":true},"options":{},"margin":{"assetEnabled":null}}}}`)
+	out, err := new(Version4).UpgradeExchange(context.Background(), in)
+	require.NoError(t, err)
+	require.NotEmpty(t, out)
+
+	_, _, _, err = jsonparser.Get(out, "currencyPairs", "assetTypes") //nolint:dogsled // Ignored return values really not needed
+	assert.ErrorIs(t, err, jsonparser.KeyPathNotFoundError, "assetTypes should be removed")
+
+	e, err := jsonparser.GetBoolean(out, "currencyPairs", "pairs", "spot", "assetEnabled")
+	require.NoError(t, err, "Must find assetEnabled for spot")
+	assert.True(t, e, "assetEnabled should be set to true")
+
+	e, err = jsonparser.GetBoolean(out, "currencyPairs", "pairs", "futures", "assetEnabled")
+	require.NoError(t, err, "Must find assetEnabled for futures")
+	assert.True(t, e, "assetEnabled should be set to true")
+
+	e, err = jsonparser.GetBoolean(out, "currencyPairs", "pairs", "options", "assetEnabled")
+	require.NoError(t, err, "Must find assetEnabled for options")
+	assert.False(t, e, "assetEnabled should be set to false")
+
+	e, err = jsonparser.GetBoolean(out, "currencyPairs", "pairs", "margin", "assetEnabled")
+	require.NoError(t, err, "Must find assetEnabled for margin")
+	assert.False(t, e, "assetEnabled should be set to false")
+
+	out2, err := new(Version4).UpgradeExchange(context.Background(), out)
+	require.NoError(t, err, "Must not error on re-upgrading")
+	assert.Equal(t, out, out2, "Should not affect an already upgraded config")
+
+	in = []byte(`{"name":"Cracken","currencyPairs":{"assetTypes":["spot"],"pairs":{"spot":{"assetEnabled":{}}}}}`)
+	_, err = new(Version4).UpgradeExchange(context.Background(), in)
+	require.NoError(t, err)
+
+	in = []byte(`{"name":"Cracken","currencyPairs":{"assetTypes":["spot"],"pairs":{"margin":{"assetEnabled":{}}}}}`)
+	_, err = new(Version4).UpgradeExchange(context.Background(), in)
+	require.ErrorIs(t, err, jsonparser.UnknownValueTypeError)
+	require.ErrorContains(t, err, "`margin`")
+	require.ErrorContains(t, err, "`object`")
+}
+
+func TestVersion4Downgrade(t *testing.T) {
+	t.Parallel()
+
+	in := []byte(`{"name":"Cracken","currencyPairs":{"pairs":{"spot":{"enabled":"BTC-AUD","available":"BTC-AUD","assetEnabled":true},"futures":{"assetEnabled":false},"options":{},"options_combo":{"assetEnabled":true}}}}`)
+	out, err := new(Version4).DowngradeExchange(context.Background(), in)
+	require.NoError(t, err)
+	require.NotEmpty(t, out)
+
+	v, vT, _, err := jsonparser.Get(out, "currencyPairs", "assetTypes")
+	require.NoError(t, err, "assetTypes must be found")
+	require.Equal(t, jsonparser.Array, vT, "assetTypes must be an array")
+	require.Equal(t, `["spot","options_combo"]`, string(v), "assetTypes must be correct")
+
+	assetEnabledFn := func(k []byte, v []byte, _ jsonparser.ValueType, _ int) error {
+		_, err = jsonparser.GetBoolean(v, "assetEnabled")
+		require.ErrorIsf(t, err, jsonparser.KeyPathNotFoundError, "assetEnabled must be removed from %s", k)
+		return nil
+	}
+	err = jsonparser.ObjectEach(bytes.Clone(out), assetEnabledFn, "currencyPairs", "pairs")
+	require.NoError(t, err, "Must not error visiting currencyPairs")
+}

config/versions/versions.go

@@ -168,7 +168,7 @@ func exchangeDeploy(ctx context.Context, patch ExchangeVersion, method func(Exch
 		defer func() { i++ }()
 		name, err := jsonparser.GetString(exchOrig, "name")
 		if err != nil {
-			errs = common.AppendError(errs, fmt.Errorf("%w: %w `name`: %w", errModifyingExchange, common.ErrGettingField, err))
+			errs = common.AppendError(errs, fmt.Errorf("%w [%d]: %w `name`: %w", errModifyingExchange, i, common.ErrGettingField, err))
 			return
 		}
 		for _, want := range wanted {
@@ -177,12 +177,12 @@ func exchangeDeploy(ctx context.Context, patch ExchangeVersion, method func(Exch
 			}
 			exchNew, err := method(patch, ctx, exchOrig)
 			if err != nil {
-				errs = common.AppendError(errs, fmt.Errorf("%w: %w", errModifyingExchange, err))
+				errs = common.AppendError(errs, fmt.Errorf("%w for `%s`: %w", errModifyingExchange, name, err))
 				continue
 			}
 			if !bytes.Equal(exchNew, exchOrig) {
 				if j, err = jsonparser.Set(j, exchNew, "exchanges", "["+strconv.Itoa(i)+"]"); err != nil {
-					errs = common.AppendError(errs, fmt.Errorf("%w: %w `exchanges.[%d]`: %w", errModifyingExchange, common.ErrSettingField, i, err))
+					errs = common.AppendError(errs, fmt.Errorf("%w `%s`/exchanges[%d]: %w: %w", errModifyingExchange, name, i, common.ErrSettingField, err))
 				}
 			}
 		}

config/versions/versions_test.go

@@ -58,6 +58,7 @@ func TestDeploy(t *testing.T) {
 	_, err = m.Deploy(context.Background(), in, UseLatestVersion)
 	require.Implements(t, (*ExchangeVersion)(nil), m.versions[1])
 	require.ErrorIs(t, err, errUpgrade)
+	require.ErrorContains(t, err, "for `Juan`")
 
 	j2, err = m.Deploy(context.Background(), j, 0)
 	require.NoError(t, err)
@@ -83,6 +84,7 @@ func TestExchangeDeploy(t *testing.T) {
 	require.ErrorIs(t, err, common.ErrGettingField)
 	require.ErrorIs(t, err, jsonparser.KeyPathNotFoundError)
 	require.ErrorContains(t, err, "`name`")
+	require.ErrorContains(t, err, "[0]")
 
 	in = []byte(`{"version":0,"exchanges":[{"name":"Juan"},{"name":"Megashaft"}]}`)
 	_, err = exchangeDeploy(context.Background(), v, ExchangeVersion.UpgradeExchange, in)