Config: AssetEnabled upgrade #1735

gbjk · 2024-11-29T06:19:18Z

Config:

Moves AssetEnabled from being a bool pointer to just being a bool

Exchanges:

Rename StoreAssetPairFormat to StoreAssetPairStore
StorePairs(enabled) set assetEnabled if pairs list is not empty
StorePairs(enabled) add pairs to Available as well
- This is a convenience for all the test that have to do both, as well as users. Can be dropped if contentious
Fix error formatting for DisableAssetWebsocketSupport logging
Various assertifications in tests
Simplify exchange's defaultPairFormatting handling and make it somewhat consistent.
- Could do more, but there's a larger overhaul of this coming
SetAssetEnabled will not error on no-op (already disabled or enabled)

Bitfinex:

Fix tests for MarginFunding subs

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

New feature (non-breaking change which adds functionality)

How has this been tested

go test ./... -race
golangci-lint run

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.

Also assertifies

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.

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

shazbert

Quick test on gateio, when I set everything but spot assets to false for asset enabled it still fetches and subscribes all assets. Doesn't seem to be happening on master, once resolved I will do a more in-depth check.

gbjk · 2024-12-13T03:30:28Z

Quick test on gateio, when I set everything but spot assets to false for asset enabled it still fetches and subscribes all assets. Doesn't seem to be happening on master, once resolved I will do a more in-depth check.

Fixed gbjk@65c05a063

shazbert

great cleanup! tested and runs great. Just some minor nits.

shazbert · 2024-12-26T21:48:17Z

cmd/exchange_template/wrapper_file.tmpl

 	if err != nil {
-		log.Errorln(log.ExchangeSys, err)
+		log.Errorf(log.ExchangeSys, "%s error storing `%s` default asset formats: %s", {{.Variable}}.Name, asset.Spot, err)


Can you wrap all these details on the error paths in StoreAssetPairStore? That way we don't need to have this.

The whole direction here was the opposite of this change request.
The ideology I'm following is that functions return the error they encounter, without their own context.
If you call a go standard library function, you'll get the same thing.
It's down to the caller to decide if it wants to give context to that error, and what that context looks like from it's perspective.
From the perspective of the function caller, they know what they called, and so the error returned should be undecorated.

Case in point: StoreAssetPairStore is not default asset formats so these callers all needed to clarify the context they were calling that function in.

This is why you'll often see me decorate errors one level above where GCT has previously been doing it.
Sometimes it can feel slightly weighty, in this example where we copy the same context everywhere.

We could definitely add a common error for errStoringDefaultAssetFormats though, and I think that's warranted.
I'll circle back to this after the others and see if you've replied :)

Adding one more bit of context:
This implies that functions must also add their own context.
e.g. If StoreAssetPairStore encountered some error interacting with a database, like "Record not found", We'd expect that error to come without context.
So StoreAssetPairStore should add the context error searching for glove size in database: %w.

From an implementation perspective, maintaining this standard can introduce toil during implementation and reviews, especially since any failure in SetDefaults would inherently cause the exchange to malfunction. Differentiating between errors in this context seems redundant. Still suggest wrapping asset and exchange for the exchange method.
But I will leave this open for other peoples perspective. @thrasher- @gloriousCode

shazbert · 2024-12-26T21:54:16Z

config/config_test.go

-	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


Suggested change

// be added to the enabled pairs list due to the atLestOneEnabled code

// be added to the enabled pairs list due to the atLeastOneEnabled code

Imma fixup into 4d32d83 cos I obviously can't fixup into 14c72c9

Fixed gbjk@36e3dffbc

shazbert · 2024-12-26T22:30:46Z

currency/manager_test.go

-	if err == nil {
-		t.Fatal("unexpected result")
-	}
+	assert.NoError(t, err, "Setting to disabled twice should not error")


Suggestion: I usually return an error on no-op because I find it easier to track my own whoopsies, I also like the fact if I change it by CLI it then returns that it failed. I do see the idempotency aspect of the new behaviour though. Keep this unresolved for other peoples input if you want to pass on this.

I believe if something wants to error on it already being disabled/enabled, that's it's problem.
The responsibility of this function is to do what it's told 😃
So I'm happy to add a check in api server or CLI for you to see if it's already enabled/disabled and error there.
Would you like that?

No thanks, if it's going to stay like this, having that check would need to go to all potential API calls cli, scripting etc. State change responsibility is not a major concern 🤷 just easier here but it's such a minor thing.

shazbert · 2024-12-26T22:37:43Z

engine/rpcserver_test.go

+	b := e.GetBase()
+
+	for _, a := range []asset.Item{asset.Spot, asset.Margin, asset.CoinMarginedFutures, asset.USDTMarginedFutures} {
+		fmt := currency.PairStore{


fmt conflicts with a package name maybe just ps?

Agreed, Thanks! 🙏

Fixed gbjk@d6c896634

shazbert · 2024-12-26T22:42:48Z

exchanges/bitmex/bitmex_wrapper.go

+			ConfigFormat:  &currency.PairFormat{Uppercase: true, Delimiter: currency.DashDelimiter},
+		}
+		if a == asset.Spot {
+			ps.RequestFormat = &currency.PairFormat{Uppercase: true, Delimiter: currency.UnderscoreDelimiter}


Suggested change

ps.RequestFormat = &currency.PairFormat{Uppercase: true, Delimiter: currency.UnderscoreDelimiter}

ps.RequestFormat.Delimiter = currency.UnderscoreDelimiter

Fixed gbjk@bfa72e20a

codecov · 2024-12-27T02:36:14Z

Codecov Report

Attention: Patch coverage is 71.68142% with 64 lines in your changes missing coverage. Please review.

Project coverage is 37.38%. Comparing base (105591b) to head (bfa72e2).
Report is 28 commits behind head on master.

Files with missing lines	Patch %	Lines
exchanges/exchange.go	44.44%	10 Missing and 15 partials ⚠️
exchanges/binance/binance_wrapper.go	33.33%	4 Missing and 2 partials ⚠️
exchanges/bitfinex/bitfinex_wrapper.go	66.66%	3 Missing and 2 partials ⚠️
exchanges/bitmex/bitmex_wrapper.go	66.66%	3 Missing and 2 partials ⚠️
exchanges/kraken/kraken_wrapper.go	66.66%	3 Missing and 2 partials ⚠️
exchanges/deribit/deribit_wrapper.go	42.85%	2 Missing and 2 partials ⚠️
exchanges/btse/btse_wrapper.go	76.92%	2 Missing and 1 partial ⚠️
exchanges/bybit/bybit_wrapper.go	25.00%	2 Missing and 1 partial ⚠️
exchanges/huobi/huobi_wrapper.go	80.00%	2 Missing and 1 partial ⚠️
exchanges/kucoin/kucoin_wrapper.go	80.00%	2 Missing and 1 partial ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1735      +/-   ##
==========================================
+ Coverage   37.13%   37.38%   +0.24%     
==========================================
  Files         414      416       +2     
  Lines      180198   178480    -1718     
==========================================
- Hits        66925    66723     -202     
+ Misses     105413   104001    -1412     
+ Partials     7860     7756     -104

Files with missing lines	Coverage Δ
backtester/engine/setup.go	`58.59% <100.00%> (+0.70%)`	⬆️
config/config.go	`87.30% <ø> (+0.25%)`	⬆️
config/versions/v3.go	`100.00% <100.00%> (ø)`
currency/manager.go	`93.66% <100.00%> (-0.35%)`	⬇️
exchanges/sharedtestvalues/sharedtestvalues.go	`76.59% <100.00%> (+11.28%)`	⬆️
exchanges/binanceus/binanceus_wrapper.go	`41.04% <60.00%> (+0.15%)`	⬆️
exchanges/btse/btse_wrapper.go	`44.63% <76.92%> (+0.85%)`	⬆️
exchanges/bybit/bybit_wrapper.go	`46.63% <25.00%> (+3.42%)`	⬆️
exchanges/huobi/huobi_wrapper.go	`39.60% <80.00%> (-0.37%)`	⬇️
exchanges/kucoin/kucoin_wrapper.go	`31.86% <80.00%> (+0.12%)`	⬆️
... and 6 more

... and 45 files with indirect coverage changes

…the asset

shazbert

Current changes tACK. 🚀

gbjk requested review from gloriousCode and shazbert November 29, 2024 06:19

gbjk self-assigned this Nov 29, 2024

gbjk requested a review from thrasher- November 29, 2024 06:19

gbjk added the review me This pull request is ready for review label Nov 29, 2024

gbjk force-pushed the feature/config_assetenabled branch 2 times, most recently from 3697ede to 5fb08d2 Compare November 30, 2024 06:05

gloriousCode added the nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged label Dec 1, 2024

gbjk force-pushed the feature/config_assetenabled branch from 026f88f to 272827a Compare December 2, 2024 01:28

gbjk mentioned this pull request Dec 4, 2024

Config: OrderManager upgrade #1737

Open

3 tasks

gbjk force-pushed the feature/config_assetenabled branch from 942b49b to e2d81d0 Compare December 9, 2024 08:13

gbjk removed the nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged label Dec 9, 2024

gbjk added 10 commits December 11, 2024 09:10

Config: Move assetEnabled upgrade to Version management

4d32d83

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

9253089

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)

95d9c4f

RPCServer: Fix tests expecting StoreAssetPairFormat to enable the asset

f67c6fe

Also assertifies

Bitfinex: Fix tests for MarginFunding subs

7b5b7d4

GCTWrapper: Improve TestMain clarity

7caff83

Linter: Fixes for exchange.go

ce57ece

BTSE: Add futures to testconfig

cb6f6f0

Config: AssetEnabled upgrade should respect assetTypes

06558ca

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

shazbert requested changes Dec 13, 2024

View reviewed changes

gbjk force-pushed the feature/config_assetenabled branch from c092bfa to 65c05a0 Compare December 13, 2024 03:28

fixup! CurrencyManager: Set AssetEnabled when StorePairs(enabled)

4a4fb99

gbjk force-pushed the feature/config_assetenabled branch from 65c05a0 to 4a4fb99 Compare December 13, 2024 03:44

gbjk mentioned this pull request Dec 17, 2024

Huobi: V2 subscription support #1703

Open

5 tasks

shazbert requested changes Dec 26, 2024

View reviewed changes

fixup! Config: Move assetEnabled upgrade to Version management

36e3dff

gbjk added 2 commits December 27, 2024 09:41

fixup! RPCServer: Fix tests expecting StoreAssetPairFormat to enable …

d6c8966

…the asset

fixup! Exchanges: Rename StoreAssetPairStore

bfa72e2

gbjk requested a review from shazbert December 27, 2024 02:52

shazbert approved these changes Dec 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config: AssetEnabled upgrade #1735

Config: AssetEnabled upgrade #1735

gbjk commented Nov 29, 2024 •

edited

Loading

shazbert left a comment

gbjk commented Dec 13, 2024

shazbert left a comment

shazbert Dec 26, 2024

gbjk Dec 27, 2024

gbjk Dec 27, 2024

shazbert Dec 27, 2024

shazbert Dec 26, 2024

gbjk Dec 27, 2024

shazbert Dec 26, 2024

gbjk Dec 27, 2024

shazbert Dec 27, 2024

shazbert Dec 26, 2024

gbjk Dec 27, 2024

shazbert Dec 26, 2024

gbjk Dec 27, 2024

codecov bot commented Dec 27, 2024 •

edited

Loading

shazbert left a comment

	// be added to the enabled pairs list due to the atLestOneEnabled code
	// be added to the enabled pairs list due to the atLeastOneEnabled code

	ps.RequestFormat = &currency.PairFormat{Uppercase: true, Delimiter: currency.UnderscoreDelimiter}
	ps.RequestFormat.Delimiter = currency.UnderscoreDelimiter

Config: AssetEnabled upgrade #1735

Are you sure you want to change the base?

Config: AssetEnabled upgrade #1735

Conversation

gbjk commented Nov 29, 2024 • edited Loading

Type of change

How has this been tested

shazbert left a comment

Choose a reason for hiding this comment

gbjk commented Dec 13, 2024

shazbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 27, 2024 • edited Loading

Codecov Report

shazbert left a comment

Choose a reason for hiding this comment

gbjk commented Nov 29, 2024 •

edited

Loading

codecov bot commented Dec 27, 2024 •

edited

Loading