exchanges: shift GetDefaultConfig wrapper function to exchange.go #1472
Conversation
…ition, to ensure set defaults doesn't get called twice and to reduce code
shazbert
added
low priority
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1472 +/- ##
==========================================
+ Coverage 35.89% 35.96% +0.06%
==========================================
Files 411 411
Lines 177595 177227 -368
==========================================
- Hits 63752 63734 -18
+ Misses 106058 105705 -353
- Partials 7785 7788 +3
|
shazbert
force-pushed
the
default_config
branch
from
74fbfdb
to
72e5947
Compare
exchanges/interfaces.go
Outdated
| @@ -188,3 +189,11 @@ type MarginManagement interface { | |||
| futures.PNLCalculation | |||
| GetFuturesContractDetails(ctx context.Context, item asset.Item) ([]futures.Contract, error) | |||
| } | |||
|
|
|||
| // LimitedScope defines a subset of the exchange interface | |||
| type LimitedScope interface { | |||
There was a problem hiding this comment.
I think it would be good to rename it to be more relevant to its limited scope. ConfigGeneration
There was a problem hiding this comment.
I purged it, I was being lazy, you can see why in recent update.
exchanges/exchange.go
Outdated
| @@ -1943,3 +1944,37 @@ func (b *Base) Bootstrap(_ context.Context) (continueBootstrap bool, err error) | |||
| func (b *Base) IsVerbose() bool { | |||
| return b.Verbose | |||
| } | |||
|
|
|||
| // GetDefaultConfig returns a default exchange config | |||
| func (b *Base) GetDefaultConfig(ctx context.Context, instance LimitedScope) (*config.Exchange, error) { | |||
There was a problem hiding this comment.
I agree with your PR assessment that its a bit jank. I think this can become an exported function instead
func GetDefaultConfig(ctx context.Context, instance IBotExchange) (*config.Exchange, error) {
That or LimitedScope, but I don't see much problem with using IBotExchange
| if err != nil { | ||
| log.Errorln(common.LiveStrategy, err) | ||
| } | ||
| log.Errorln(common.LiveStrategy, err) |
There was a problem hiding this comment.
| log.Errorln(common.LiveStrategy, err) | |
| if err != nil { | |
| if err != nil { | |
| if err == nil { | |
| } else { | |
| log.Errorln(common.LiveStrategy, err) | |
| } | |
| } | |
| } |
😄
| if exch.GetName() == "" { | ||
| exch.SetDefaults() | ||
| } | ||
|
|
||
| b := exch.GetBase() | ||
|
|
||
| exchCfg, err := b.GetStandardConfig() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| err = b.SetupDefaults(exchCfg) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This function is so weird! "get me the default config" means setup some parts of the exchange, reset some exchange values to the default config's and then make REST calls.
I'm not going to make you refactor this, it's how its been and refactoring isn't really going to improve much right now. Just a fun function
PR Description
Shifts
GetDefaultConfigwrapper function toexchange.goMitigates potential issue when its called in a wrapper test and overwrites memory after initial
SetDefaultmethod func is called.Also RMs a bunch of code wrapper side which is nice but its signature is a tad janky. Optionally we can disconnect it as a method and just have an exported function.
Type of change
Please delete options that are not relevant and add an
xin[]as item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and
also consider improving test coverage whilst working on a certain feature or package.
Checklist