engine/sync-manager: add config support #1326

gloriousCode · 2023-08-22T06:30:04Z

PR Description

This is a quick & basic QoL improvement which adds the sync manager config to our config file. No longer do you need to add so many params when running the application to change behaviour. You can still use the running params to override your config.

Also, the sync manager is a pretty trustworthy subsystem now, so I've added the ability to modify what logging events to output. You can toggle whether you want to see:

Switch protocol updates
Initial sync updates
Event updates

Errors, default subsystem logs all still show and Verbose is still a thing.

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
TestGetDefaultSyncManagerConfig()

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally and on Github Actions/AppVeyor with my changes
Any dependent changes have been merged and published in downstream modules

shazbert

Looks good. Minor nits.

config/config.go

shazbert · 2023-08-23T02:00:13Z

config/config.go

+		log.Warnf(log.ConfigMgr, "invalid sync manager pair format value %v, defaulting to %v\n", c.SyncManagerConfig.PairFormatDisplay, c.CurrencyPairFormat)
+		c.SyncManagerConfig.PairFormatDisplay = c.CurrencyPairFormat
+	}
+	if c.SyncManagerConfig.TimeoutREST <= 0 {


if I purge fiat display currency it purges and never comes back and stops the sync manager from working🙏

Well don't do that 😆 Will look into it

This is still an issue, when I remove this fiatdisplayCurrency from the config it stops the starting of the syncMnager

shazbert · 2023-08-23T02:09:22Z

engine/helpers.go

-					FiatDisplayCurrency:     bot.Config.Currency.FiatDisplayCurrency,
-					Verbose:                 bot.Settings.Verbose,
+				cfg := bot.Config.SyncManagerConfig
+				cfg.SynchronizeTicker = bot.Settings.EnableTickerSyncing && cfg.SynchronizeTicker


When ticker sync is enabled in config this pattern used to override if set using flagSet.WithBool functionality in engine.

.\gocryptotrader -orderbooksync=true -tickersync=false

Could you rephrase that please?

When ticker sync is set to true in config, when I try to set it to false via command line it will not. I don't english well.

For the inverse if I have both set to false in config and I run this command, it doesn't work:

.\gocryptotrader --orderbooksync=true --tickersync=true

Default app param values have now become false to address this. But then that will mean you can't disable it via command line.

I'm thinking to remove them based on what you're after

edit: still reworking

Resolved, I forgot about the lovely existing helper functions

shazbert

Thanks for changes, just 2 things I noticed.

shazbert · 2023-08-24T03:51:27Z

config/config.go

+		log.Warnf(log.ConfigMgr, "invalid sync manager pair format value %v, defaulting to %v\n", c.SyncManagerConfig.PairFormatDisplay, c.CurrencyPairFormat)
+		c.SyncManagerConfig.PairFormatDisplay = c.CurrencyPairFormat
+	}
+	if c.SyncManagerConfig.TimeoutREST <= 0 {


This is still an issue, when I remove this fiatdisplayCurrency from the config it stops the starting of the syncMnager

shazbert · 2023-08-24T03:53:09Z

config/config.go

+		c.SyncManagerConfig.TimeoutWebsocket = DefaultSyncerTimeoutWebsocket
+	}
+	if c.SyncManagerConfig.PairFormatDisplay == nil {
+		log.Warnf(log.ConfigMgr, "invalid sync manager pair format value %v, using default format eg BTC-USD\n", c.SyncManagerConfig.PairFormatDisplay)


shazbert · 2023-08-24T03:57:49Z

engine/helpers.go

-					FiatDisplayCurrency:     bot.Config.Currency.FiatDisplayCurrency,
-					Verbose:                 bot.Settings.Verbose,
+				cfg := bot.Config.SyncManagerConfig
+				cfg.SynchronizeTicker = bot.Settings.EnableTickerSyncing && cfg.SynchronizeTicker


For the inverse if I have both set to false in config and I run this command, it doesn't work:

.\gocryptotrader --orderbooksync=true --tickersync=true

shazbert

LGTM Thanks!

thrasher-

Nice! Just some minor nitters

config/config.go

engine/sync_manager.go

Co-authored-by: Adrian Gallagher <[email protected]>

thrasher-

Like a glorious beef stroganoff!

shazbert

Ch ACK

* allows sync manager customisation for values and logs * config-example add * who doesnt like more coverage? * ensures you can actually disable it via config el oh el * less ifs, better control * fix verbose * sync trades default false * fix summary being printed when not enabled * fixes config checker and output * nits * I can put this behind me now * Fixed logCaSiNg Co-authored-by: Adrian Gallagher <[email protected]> * combines if statements --------- Co-authored-by: Adrian Gallagher <[email protected]>

allows sync manager customisation for values and logs

0bbbe6f

gloriousCode added the review me This pull request is ready for review label Aug 22, 2023

gloriousCode requested a review from a team August 22, 2023 06:30

gloriousCode self-assigned this Aug 22, 2023

gloriousCode added 6 commits August 22, 2023 16:30

config-example add

9a04818

who doesnt like more coverage?

e46f1f9

ensures you can actually disable it via config el oh el

46c69f5

less ifs, better control

a2ed2cc

fix verbose

ccb1bc0

sync trades default false

2e485fd

shazbert requested changes Aug 23, 2023

View reviewed changes

fix summary being printed when not enabled

56242f8

gloriousCode added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged and removed review me This pull request is ready for review labels Aug 23, 2023

fixes config checker and output

c373c59

gloriousCode added review me This pull request is ready for review and removed reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Aug 23, 2023

gloriousCode requested a review from shazbert August 23, 2023 23:43

shazbert requested changes Aug 24, 2023

View reviewed changes

nits

f706f15

gloriousCode requested a review from shazbert August 24, 2023 04:30

gloriousCode added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged and removed review me This pull request is ready for review labels Aug 24, 2023

I can put this behind me now

27346d5

gloriousCode added review me This pull request is ready for review and removed reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Aug 24, 2023

shazbert approved these changes Aug 24, 2023

View reviewed changes

thrasher- requested changes Aug 29, 2023

View reviewed changes

gloriousCode and others added 2 commits August 29, 2023 15:01

Fixed logCaSiNg

8a82a77

Co-authored-by: Adrian Gallagher <[email protected]>

combines if statements

d2d274d

gloriousCode requested a review from thrasher- August 29, 2023 05:13

thrasher- changed the title ~~sync-manager: add config~~ egnine/sync-manager: add config support Aug 29, 2023

thrasher- approved these changes Aug 29, 2023

View reviewed changes

shazbert approved these changes Aug 29, 2023

View reviewed changes

thrasher- merged commit d3102a0 into thrasher-corp:master Aug 29, 2023

thrasher- changed the title ~~egnine/sync-manager: add config support~~ engine/sync-manager: add config support Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

engine/sync-manager: add config support #1326

engine/sync-manager: add config support #1326

gloriousCode commented Aug 22, 2023

shazbert left a comment

shazbert Aug 23, 2023

gloriousCode Aug 23, 2023

shazbert Aug 24, 2023

shazbert Aug 23, 2023

gloriousCode Aug 23, 2023

shazbert Aug 23, 2023

shazbert Aug 24, 2023

gloriousCode Aug 24, 2023 •

edited

Loading

gloriousCode Aug 24, 2023

shazbert left a comment

shazbert Aug 24, 2023

shazbert Aug 24, 2023

shazbert Aug 24, 2023

shazbert left a comment

thrasher- left a comment

thrasher- left a comment

shazbert left a comment

engine/sync-manager: add config support #1326

engine/sync-manager: add config support #1326

Conversation

gloriousCode commented Aug 22, 2023

PR Description

How has this been tested

Checklist

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

gloriousCode Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

shazbert left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

gloriousCode Aug 24, 2023 •

edited

Loading