Prevent unnecessary balanced strategy re-randomization #60
Conversation
|
This pr is to implement issue #52 |
Reidmcc
changed the title
plugins/balancedLevelProvider.go
Outdated
| @@ -30,6 +30,12 @@ type balancedLevelProvider struct { | |||
|
|
|||
| // precomputed before construction | |||
| randGen *rand.Rand | |||
|
|
|||
| //keeps the starting balances from the previous run to check if anything has changed | |||
There was a problem hiding this comment.
comments explaining the use of variables should be on the right side of the declaration (like for the variables above)
In general for comments in the code:
as a convention it should have a space after // and should begin with a lowercase character
plugins/balancedLevelProvider.go
Outdated
| @@ -30,6 +30,12 @@ type balancedLevelProvider struct { | |||
|
|
|||
| // precomputed before construction | |||
| randGen *rand.Rand | |||
|
|
|||
| //keeps the starting balances from the previous run to check if anything has changed | |||
| lastMaxAssetBase float64 | |||
There was a problem hiding this comment.
these three variables can be in a new section of: // uninitialized.
this section will come after randGen
plugins/balancedLevelProvider.go
Outdated
| @@ -68,8 +74,14 @@ func makeBalancedLevelProvider( | |||
| validateSpread(carryoverInclusionProbability) | |||
|
|
|||
| randGen := rand.New(rand.NewSource(time.Now().UnixNano())) | |||
|
|
|||
| //set starting values for the previous-run variables | |||
There was a problem hiding this comment.
this (and it's usage below) is not needed since these are uninitialized based on the comment above -- it's sometimes better to be specific but the uninitialized concept is used in a few places in kelp right now.
plugins/balancedLevelProvider.go
Outdated
| @@ -93,6 +108,26 @@ func validateSpread(spread float64) { | |||
|
|
|||
| // GetLevels impl. | |||
| func (p *balancedLevelProvider) GetLevels(maxAssetBase float64, maxAssetQuote float64) ([]api.Level, error) { | |||
| log.Printf("Last base balance was: %v", p.lastMaxAssetBase) | |||
There was a problem hiding this comment.
- we typically start these log lines with a lowercase character
- use
%.7finstead of%v - add
\nafter%.7f-- we always add newlines to printf statements as that will help immensely when we add a new logging framework - we should also print the current balances for easy viewing
putting this all together, it should be:
log.Printf("balance for base asset: last=%.7f, current=%.7f\n", p.lastMaxAssetBase, maxAssetBase)
plugins/balancedLevelProvider.go
Outdated
|
|
||
| //Checking whether balances have changed meaningfully | ||
| //We have to give the balances some room to move because slightly different balance values come back from Horizon even if nothing changed | ||
| //Probably because floating point math is the worst |
There was a problem hiding this comment.
we should try and address this jitter, adding this error buffer as a solution should only be the last resort.
we're using 7 units of precision everywhere which is also used in horizon so we should be able to fix this. Can you give me details on when we're getting different results from horizon?
| log.Println("Balances remain essentially the same as the previous cycle, leave levels as they are.") | ||
| return p.lastLevels, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
the code that is unchanged should be extracted into a new function:
func (p *balancedLevelProvider) recomputeLevels(maxAssetBase float64, maxAssetQuote float64) ([]api.Level, error) -- this will allow us to easily test it (whenever we get to that)
we can return p.recomputeLevels(...) after we set the 3 last values as you've done below
|
@nikhilsaraf I've made the style edits and implemented the new function. I figured out the balance jitter. It's not a precision problem, it's a transaction fee problem. For example, my XLM balance values for two consecutive cycles were: There's a couple ways we could deal with this:
For now I've slimmed down the error buffer to only allow balance reductions. I agree that it's not optimal. |
|
@Reidmcc I'm adding support for handling trades/fills on an account for use in the mirror strategy. This will be added as general infrastructure for the whole bot so any strategy can be set up to be "notified" of a trade on the account. With that infrastructure in place we could actually use it to set a boolean flag on the balancedLevelProvider anytime we encounter a trade for the account. If you want to wait for that (end of next week at the very latest) then it may be a more elegant solution that would eliminate us having to worry about XLM fees. |
|
@nikhilsaraf implementing the checks with the upcoming trade flag sounds good. I'm in no hurry; I've been running this live since I submitted it two weeks ago :-D Seems like we'll still need the level-retention and re-submission part of this. I can remove the balance check parts and slim it down to just level keeping. |
…into balanced_no_rerand
|
@nikhilsaraf I've merged in the new commits post repo move. It doesn't seemed to have fixed the Travis issue. Those commits are showing as changes now. |
|
@Reidmcc I've fixed the travis checks. The commits are probably showing up as separate changes because they were merged from the If that's not possible then it may just be easier to open a new PR. In that case it would be easier to do this towards the end of next week once I've merged the fill tracking changes. |
|
@nikhilsaraf yeah, that's what I was trying to do; I merged the commits to my local branch from interstellar/master and pushed to the remote. Anyway, I'll do a new pull request rather than try to salvage this mess. It will be different code anyway using the fill tracking. |
|
@Reidmcc I've uploaded the fill tracking, you can take a look at how that works for the mirror strategy as an example: https://github.com/interstellar/kelp/blob/master/plugins/mirrorStrategy.go#L300 |
Here's the update to balancedLevelProvider to have it remember what its balances were last run and not mess with the levels if they didn't really change.
The code creates a very small range of balances to consider "the same" because the
maxAssetvariables come back slightly different every run-through, even if no levels were touched. My guess is that it's a floating-point math problem.