-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: tvwap calculations #601
Conversation
Be wary of any issues related to this comment |
Is the plan to do that unraveling of |
@toteki wdym? if you mean the piece that sets the price for voting, those changes are in |
The stuff in the binance provider that still relies on a strict // SubscribeTickers subscribe all currency pairs into ticker and candle channels.
func (p *BinanceProvider) SubscribeTickers(cps ...types.CurrencyPair) error {
pairs := make([]string, len(cps)*2)
iterator := 0
for _, cp := range cps {
pairs[iterator] = currencyPairToBinanceTickerPair(cp)
iterator++
pairs[iterator] = currencyPairToBinanceCandlePair(cp)
iterator++
}
if err := p.subscribePairs(pairs...); err != nil {
return err
}
p.setSubscribedPairs(cps...)
return nil
} |
Right! The comment you referred to - Yeah, I'd rather do that in a separate PR since this one is already pretty lengthy and needs to be well-reviewed |
Let's do that + "refactoring our anomaly detection logic for these candles" in a separate PR and then we can finally close #542 @adamewozniak @RafilxTenfen ? |
You are young and life is long, and there is time to kill today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one comment
- besides that, it looks pretty good to me ;D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only had naming / comment suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like to see an approval from @toteki too prior to merging, but otherwise, this looks good.
My approval's already in (since my original notes were just naming/comments I submitted it as an "approve" and not a "request changes") e: The error -> 0 change on vwap looks good too |
(cherry picked from commit 84da2c5)
Description
Uses tvwap for our providers when available.
Only thing left here is refactoring our anomaly detection logic for these candles - putting that in this PR would've been too much to review.
progress on: #542
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change