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: multi-quote deviation logic #1110

Merged
merged 18 commits into from
Jul 7, 2022
Merged

Conversation

adamewozniak
Copy link
Collaborator

@adamewozniak adamewozniak commented Jul 6, 2022

This PR solves the issue for the price feeder wherein a validator might configure it to use these exchange rates to get the price of BTC/USD :

  1. BTC/ETH
  2. BTC/USD
  3. ETH/USD

This case is specific to certain assets which have more exchange rates with other tokens than they do with stablecoins.

This PR also takes care of deviation detection for stablecoins before converting everything.

closes: #1083


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 ! to the type prefix if API or client breaking change
  • added appropriate labels to the PR
  • 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 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)

@adamewozniak adamewozniak changed the title fix: multi-quote deviation logic fix fix: non-stablecoin quote deviation logic fix Jul 6, 2022
require.Equal(t, prices[pair.Base], atomPrice)
}

func TestGetComputedPricesCandlesConversion(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This & TestGetComputedPricesTickersConversion are the test cases that don't work without this PR

@adamewozniak adamewozniak marked this pull request as ready for review July 7, 2022 01:26
@adamewozniak adamewozniak requested review from a team as code owners July 7, 2022 01:26
Copy link
Member

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. For this kind of things, would be better if we firstly discuss the solution in the issue before coding (also would help with review)
  2. I'm not sure we should do aggregation between different pairs, because we dismiss arbitrage. Probably only 3-4 base pairs should be possible (USDT, BTC, USD, maybe Atom - and atom only allowed when we don't have other price with significantly better volume)

price-feeder/CHANGELOG.md Outdated Show resolved Hide resolved
price-feeder/oracle/convert_test.go Outdated Show resolved Hide resolved
logger,
convertedCandles,
deviations,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have less than 3 values, then we won't have a price. Is this desirable?

Copy link
Collaborator Author

@adamewozniak adamewozniak Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this is true - with 2 or less values we don't filter anything out due to deviation but we still have a price, because we can't calculate a meaningful standard deviation. Of course the best case is having 3+ values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// We accept any prices that are within (2 * T)𝜎, or for which we couldn't get 𝜎.

price-feeder/oracle/oracle_test.go Outdated Show resolved Hide resolved
price-feeder/oracle/oracle_test.go Outdated Show resolved Hide resolved
price-feeder/oracle/convert.go Outdated Show resolved Hide resolved
@adamewozniak adamewozniak changed the title fix: non-stablecoin quote deviation logic fix feat: multi-quote deviation logic Jul 7, 2022
@adamewozniak
Copy link
Collaborator Author

  1. I'm not sure we should do aggregation between different pairs, because we dismiss arbitrage. Probably only 3-4 base pairs should be possible (USDT, BTC, USD, maybe Atom - and atom only allowed when we don't have other price with significantly better volume)

Agreed, maybe ETH as well - but this should be addressed in another PR

@robert-zaremba
Copy link
Member

Agreed, maybe ETH as well - but this should be addressed in another PR

OK, but I don't want to release a price feeder without that update. Could you add followup issue?

Copy link
Member

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@adamewozniak adamewozniak merged commit a85f6dd into main Jul 7, 2022
@adamewozniak adamewozniak deleted the woz/fix-multi-quote-filtering branch July 7, 2022 19:14
@adamewozniak
Copy link
Collaborator Author

Need to try and backport this to the price feeder release branch for some testnet support

@adamewozniak
Copy link
Collaborator Author

@Mergifyio backport release/price-feeder/v0.2.x

mergify bot pushed a commit that referenced this pull request Jul 14, 2022
* fix multi quote deviation logic

* test++

* tests++

* cl++

* add reddit as a provider

* tests++

* cl++

* moar tests

* fix this test just a tiny bit

* add the multi-quote test case

* make tests less lenient

* add another btc pair to test cases for deviation logic

* cleanup

* cleanliness

* cleanup tests

* compute tests

* clear up some comments & change this to a feature

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
(cherry picked from commit a85f6dd)

# Conflicts:
#	price-feeder/CHANGELOG.md
#	price-feeder/oracle/oracle.go
@mergify
Copy link
Contributor

mergify bot commented Jul 14, 2022

backport release/price-feeder/v0.2.x

✅ Backports have been created

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Jul 14, 2022

backport release/price-feeder/v0.2.x

✅ Backports have been created

@adamewozniak
Copy link
Collaborator Author

@Mergifyio backport release/price-feeder/v0.2.x

@mergify
Copy link
Contributor

mergify bot commented Jul 14, 2022

backport release/price-feeder/v0.2.x

✅ Backports have been created

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Jul 14, 2022

backport release/price-feeder/v0.2.x

✅ Backports have been created

adamewozniak added a commit that referenced this pull request Jul 14, 2022
* feat: multi-quote deviation logic (#1110)

* fix multi quote deviation logic

* test++

* tests++

* cl++

* add reddit as a provider

* tests++

* cl++

* moar tests

* fix this test just a tiny bit

* add the multi-quote test case

* make tests less lenient

* add another btc pair to test cases for deviation logic

* cleanup

* cleanliness

* cleanup tests

* compute tests

* clear up some comments & change this to a feature

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
(cherry picked from commit a85f6dd)

# Conflicts:
#	price-feeder/CHANGELOG.md
#	price-feeder/oracle/oracle.go

* remove some of this stuff

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.

Price-feeder: Allow quote-specific deviation detection
3 participants