Skip to content
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

Merged
merged 11 commits into from
Mar 4, 2022
Merged

feat: tvwap calculations #601

merged 11 commits into from
Mar 4, 2022

Conversation

adamewozniak
Copy link
Collaborator

@adamewozniak adamewozniak commented Mar 3, 2022

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...

  • included the correct type prefix in the PR title
  • added appropriate labels to the PR
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@toteki
Copy link
Member

toteki commented Mar 3, 2022

Be wary of any issues related to this comment

@adamewozniak adamewozniak marked this pull request as ready for review March 3, 2022 02:19
@toteki
Copy link
Member

toteki commented Mar 3, 2022

Is the plan to do that unraveling of CurrencyPair slices in a separate PR?

@adamewozniak
Copy link
Collaborator Author

Is the plan to do that unraveling of CurrencyPair slices in a separate PR?

@toteki wdym? if you mean the piece that sets the price for voting, those changes are in oracle.go

@toteki
Copy link
Member

toteki commented Mar 3, 2022

@toteki wdym? if you mean the piece that sets the price for voting, those changes are in oracle.go

The stuff in the binance provider that still relies on a strict Ticker,Candle,Ticker,Candle... ordering of the input slice - assuming it gets separated back into two functions will it be a different PR? possibly by @RafilxTenfen

// 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
}

@adamewozniak
Copy link
Collaborator Author

adamewozniak commented Mar 3, 2022

@toteki wdym? if you mean the piece that sets the price for voting, those changes are in oracle.go

The stuff in the binance provider that still relies on a strict Ticker,Candle,Ticker,Candle... ordering of the input slice - assuming it gets separated back into two functions will it be a different PR? possibly by @RafilxTenfen

// 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

@alexanderbez
Copy link
Contributor

Right! #592 (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 ?

price-feeder/oracle/oracle.go Show resolved Hide resolved
price-feeder/oracle/oracle.go Outdated Show resolved Hide resolved
price-feeder/oracle/oracle.go Outdated Show resolved Hide resolved
price-feeder/oracle/util.go Show resolved Hide resolved
price-feeder/oracle/oracle.go Show resolved Hide resolved
price-feeder/oracle/oracle.go Outdated Show resolved Hide resolved
price-feeder/oracle/oracle.go Outdated Show resolved Hide resolved
price-feeder/oracle/oracle.go Show resolved Hide resolved
price-feeder/oracle/oracle.go Outdated Show resolved Hide resolved
price-feeder/oracle/oracle.go Show resolved Hide resolved
price-feeder/oracle/provider/binance.go Show resolved Hide resolved
price-feeder/oracle/provider/binance.go Outdated Show resolved Hide resolved
You are young and life is long, and there is time to kill today
Copy link
Contributor

@RafilxTenfen RafilxTenfen left a 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

price-feeder/oracle/provider/provider.go Outdated Show resolved Hide resolved
price-feeder/oracle/provider/provider.go Outdated Show resolved Hide resolved
price-feeder/oracle/util.go Outdated Show resolved Hide resolved
price-feeder/oracle/util.go Outdated Show resolved Hide resolved
price-feeder/oracle/util.go Outdated Show resolved Hide resolved
Copy link
Member

@toteki toteki left a 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

@adamewozniak adamewozniak requested a review from toteki March 4, 2022 16:36
Copy link
Contributor

@alexanderbez alexanderbez left a 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.

@toteki
Copy link
Member

toteki commented Mar 4, 2022

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

@adamewozniak adamewozniak merged commit 84da2c5 into main Mar 4, 2022
@adamewozniak adamewozniak deleted the woz/tvwapcalc-clean branch March 4, 2022 19:37
mergify bot pushed a commit that referenced this pull request Mar 4, 2022
(cherry picked from commit 84da2c5)
alexanderbez pushed a commit that referenced this pull request Mar 4, 2022
(cherry picked from commit 84da2c5)

Co-authored-by: Adam Wozniak <29418299+adamewozniak@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants