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

refactor: Update median tracking for historacle pricing #1632

Merged
merged 32 commits into from
Dec 9, 2022

Conversation

rbajollari
Copy link
Contributor

Description

closes: #1621

Things that still need to be done after this PR:

  • Add new median stat keeper methods as grpc queries
  • Build out oracle endblocker integration test to test historacle functionality

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)

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2022

Codecov Report

Merging #1632 (8e41f41) into main (514380a) will decrease coverage by 0.04%.
The diff coverage is 77.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1632      +/-   ##
==========================================
- Coverage   54.22%   54.18%   -0.05%     
==========================================
  Files          72       73       +1     
  Lines        7454     7547      +93     
==========================================
+ Hits         4042     4089      +47     
- Misses       3100     3143      +43     
- Partials      312      315       +3     
Impacted Files Coverage Δ
x/oracle/abci.go 6.79% <0.00%> (-40.78%) ⬇️
x/oracle/genesis.go 4.06% <0.00%> (-0.18%) ⬇️
x/oracle/keeper/migrations.go 25.00% <0.00%> (ø)
x/oracle/keeper/params.go 80.39% <64.70%> (+27.56%) ⬆️
x/oracle/keeper/historic_price.go 70.23% <74.74%> (-11.59%) ⬇️
util/decmath/decmath.go 98.11% <98.11%> (ø)
util/bytes.go 100.00% <100.00%> (ø)
x/oracle/keeper/genesis.go 87.23% <100.00%> (+1.86%) ⬆️
x/oracle/keeper/grpc_query.go 93.65% <100.00%> (+1.80%) ⬆️
x/oracle/types/genesis.go 63.63% <100.00%> (ø)
... and 7 more

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@rbajollari rbajollari marked this pull request as ready for review December 1, 2022 16:00
@rbajollari rbajollari requested review from a team as code owners December 1, 2022 16:00
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.

Need to fix IterateMedians

x/oracle/keeper/historic_price.go Outdated Show resolved Hide resolved
x/oracle/keeper/historic_price.go Show resolved Hide resolved
Copy link
Contributor

@zarazan zarazan left a comment

Choose a reason for hiding this comment

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

Great job getting this together! It needed a ton of changes since last time. I think we're almost there, a couple tiny comments from me and some consensus on the iteration method and I think its good.

util/pricestats.go Outdated Show resolved Hide resolved
util/pricestats.go Outdated Show resolved Hide resolved
x/oracle/types/keys.go Show resolved Hide resolved
x/oracle/keeper/historic_price.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zarazan zarazan left a comment

Choose a reason for hiding this comment

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

I think @robert-zaremba's concerns are resolved but idk if he wants to take a look before merging.

@adamewozniak adamewozniak requested review from robert-zaremba and removed request for robert-zaremba December 5, 2022 23: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.

thanks for fixing the denom prefix for iterations.

  • Please verify if these change are in line with the updated: 3909192

x/oracle/keeper/historic_price.go Show resolved Hide resolved
util/pricestats.go Outdated Show resolved Hide resolved
util/pricestats.go Outdated Show resolved Hide resolved
util/pricestats.go Outdated Show resolved Hide resolved
x/oracle/abci.go Outdated Show resolved Hide resolved
util/pricestats_test.go Outdated Show resolved Hide resolved
x/oracle/keeper/historic_price.go Show resolved Hide resolved
x/oracle/keeper/migrations.go Show resolved Hide resolved
@rbajollari rbajollari requested a review from a team as a code owner December 6, 2022 00:44
@rbajollari
Copy link
Contributor Author

@robert-zaremba @zarazan @adamewozniak Updated the keeper methods in the historacle design doc to match the actual implementation in this PR (some of the stuff agreed upon in #1631 wasn't put into the doc). Lmk if anything looks wrong in there.

this is due to e2e tests failing with a change in the oracle parameters query
@adamewozniak
Copy link
Collaborator

note that this PR makes the most updated version of the price feeder incompatible & we'll need to test PF against the release that includes this PR
cc @rbajollari / @zarazan

x/oracle/abci.go Fixed Show fixed Hide fixed
x/oracle/abci.go Fixed Show fixed Hide fixed
x/oracle/abci.go Fixed Show fixed Hide fixed
x/oracle/abci.go Fixed Show fixed Hide fixed
x/oracle/abci.go Fixed Show fixed Hide fixed
x/oracle/abci.go Fixed Show fixed Hide fixed
x/oracle/abci.go Fixed Show fixed Hide fixed
x/oracle/abci.go Fixed Show fixed Hide fixed
x/oracle/abci.go Fixed Show fixed Hide fixed
proto/umee/oracle/v1/genesis.proto Show resolved Hide resolved
util/decmath/decmath.go Outdated Show resolved Hide resolved
util/decmath/decmath.go Outdated Show resolved Hide resolved
util/decmath/decmath.go Outdated Show resolved Hide resolved
util/decmath/decmath.go Outdated Show resolved Hide resolved
x/oracle/keeper/genesis.go Outdated Show resolved Hide resolved
x/oracle/keeper/grpc_query_test.go Show resolved Hide resolved
x/oracle/keeper/historic_price.go Show resolved Hide resolved
x/oracle/keeper/historic_price.go Show resolved Hide resolved
x/oracle/keeper/historic_price.go Outdated Show resolved Hide resolved
rbajollari and others added 8 commits December 8, 2022 10:07
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
@@ -63,14 +58,15 @@
}

if experimental {
// Stamp rate every stamp period if asset is set to have historic stats tracked
if isPeriodLastBlock(ctx, params.StampPeriod) && params.HistoricAcceptList.Contains(ballotDenom.Denom) {
if isPeriodLastBlock(ctx, params.HistoricStampPeriod) {

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
if isPeriodLastBlock(ctx, params.MedianPeriod) && params.HistoricAcceptList.Contains(ballotDenom.Denom) {
k.CalcAndSetMedian(ctx, ballotDenom.Denom)
// Calculate and stamp median/median deviation if median stamp period has passed
if isPeriodLastBlock(ctx, params.MedianStampPeriod) {

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
k.CalcAndSetMedian(ctx, ballotDenom.Denom)
// Calculate and stamp median/median deviation if median stamp period has passed
if isPeriodLastBlock(ctx, params.MedianStampPeriod) {
if err = k.CalcAndSetHistoricMedian(ctx, ballotDenom.Denom); err != nil {

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
k.DeleteHistoricPrice(ctx, v.String(), pruneBlock)
// Prune historic prices and medians outside pruning period determined by
// the stamp period multiplied by the max stamps.
if experimental && isPeriodLastBlock(ctx, params.HistoricStampPeriod) {

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
pruneHistoricPeriod := params.HistoricStampPeriod*(params.MaximumPriceStamps) - params.VotePeriod
pruneMedianPeriod := params.MedianStampPeriod*(params.MaximumMedianStamps) - params.VotePeriod
for _, v := range params.AcceptList {
k.DeleteHistoricPrice(ctx, v.SymbolDenom, uint64(ctx.BlockHeight())-pruneHistoricPeriod)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
pruneMedianPeriod := params.MedianStampPeriod*(params.MaximumMedianStamps) - params.VotePeriod
for _, v := range params.AcceptList {
k.DeleteHistoricPrice(ctx, v.SymbolDenom, uint64(ctx.BlockHeight())-pruneHistoricPeriod)
k.DeleteHistoricMedian(ctx, v.SymbolDenom, uint64(ctx.BlockHeight())-pruneMedianPeriod)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
for _, v := range params.AcceptList {
k.DeleteHistoricPrice(ctx, v.SymbolDenom, uint64(ctx.BlockHeight())-pruneHistoricPeriod)
k.DeleteHistoricMedian(ctx, v.SymbolDenom, uint64(ctx.BlockHeight())-pruneMedianPeriod)
k.DeleteHistoricMedianDeviation(ctx, v.SymbolDenom, uint64(ctx.BlockHeight())-pruneMedianPeriod)

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
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. Thanks for keeping improving the PR.

x/oracle/keeper/historic_price.go Show resolved Hide resolved
@rbajollari rbajollari merged commit 0903f03 into main Dec 9, 2022
@rbajollari rbajollari deleted the ryan/historacle-median-tracking branch December 9, 2022 00:05
@toteki
Copy link
Member

toteki commented Dec 9, 2022

Note: be sure to run make proto-all in these PRs.

In this case, there's a little whitespace that gets changed by make proto-format and some changes not reflected in swagger/ folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor historacle pricing median tracking
6 participants