feat: Update oracle endblocker for historic # #1580

rbajollari · 2022-11-11T16:59:12Z

Description

closes: #1542

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

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 · 2022-11-11T17:02:37Z

Codecov Report

Merging #1580 (26ce029) into main (3953226) will decrease coverage by 1.04%.
The diff coverage is 22.22%.

@@            Coverage Diff             @@
##             main    #1580      +/-   ##
==========================================
- Coverage   55.09%   54.05%   -1.05%     
==========================================
  Files          68       71       +3     
  Lines        6835     7150     +315     
==========================================
+ Hits         3766     3865      +99     
- Misses       2787     2988     +201     
- Partials      282      297      +15

Impacted Files	Coverage Δ
x/oracle/keeper/historic_price.go	`81.81% <0.00%> (-9.78%)`	⬇️
x/oracle/module.go	`53.24% <0.00%> (ø)`
x/oracle/abci.go	`47.57% <27.77%> (ø)`
x/oracle/types/params.go	`96.15% <100.00%> (+0.04%)`	⬆️
x/oracle/genesis.go	`4.23% <0.00%> (ø)`

x/oracle/abci.go

+	if isPeriodLastBlock(ctx, params.PrunePeriod) {
+		pruneBlock := uint64(ctx.BlockHeight()) - params.PrunePeriod
+		for _, v := range params.AcceptList {
+			k.DeleteHistoricPrice(ctx, v.String(), pruneBlock)


robert-zaremba

endblocker tests should be extended to cover new functionality
Median computation should be correctly benchmarked (with right amount of data, eg: 20 tokens, with full amount of prices).
need to validate the stamp price usage.

x/oracle/abci.go

x/oracle/keeper/historic_price.go

x/oracle/abci.go

zarazan · 2022-11-21T18:42:26Z

Could we use the experimental flag instead of commenting out the code? And then the test can set the experimental flag.

…acle endblocker

x/oracle/abci.go

+
+			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) {


x/oracle/abci.go

+
+			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) {


x/oracle/abci.go

+			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) {
+					k.AddHistoricPrice(ctx, ballotDenom.Denom, exchangeRate)


x/oracle/abci.go

+				}
+
+				// Set median price every median period if asset is set to have historic stats tracked
+				if isPeriodLastBlock(ctx, params.MedianPeriod) && params.HistoricAcceptList.Contains(ballotDenom.Denom) {


x/oracle/abci.go

+				}
+
+				// Set median price every median period if asset is set to have historic stats tracked
+				if isPeriodLastBlock(ctx, params.MedianPeriod) && params.HistoricAcceptList.Contains(ballotDenom.Denom) {


x/oracle/abci.go

+
+				// Set median price every median period if asset is set to have historic stats tracked
+				if isPeriodLastBlock(ctx, params.MedianPeriod) && params.HistoricAcceptList.Contains(ballotDenom.Denom) {
+					k.CalcAndSetMedian(ctx, ballotDenom.Denom)


x/oracle/abci.go

 		k.SlashAndResetMissCounters(ctx)
 	}

+	// Prune historic prices every prune period
+	if isPeriodLastBlock(ctx, params.PrunePeriod) && experimental {


zarazan

Couple comments, looks great!

x/oracle/abci.go


 		k.ClearExchangeRates(ctx)

+		if isPeriodLastBlock(ctx, params.MedianPeriod) && experimental {


x/oracle/abci.go


 		k.ClearExchangeRates(ctx)

+		if isPeriodLastBlock(ctx, params.MedianPeriod) && experimental {
+			k.ClearMedians(ctx)


x/oracle/abci.go


 		k.ClearExchangeRates(ctx)

+		if isPeriodLastBlock(ctx, params.MedianPeriod) && experimental {
+			k.ClearMedians(ctx)
+			k.ClearMedianDeviations(ctx)


zarazan

Love it, thanks for adding that test!

x/oracle/abci_test.go

robert-zaremba

The functionality looks wrong. I think we wanted to have a rolling median. Let's confirm in Slack. Blocking for the moment.

Update oracle endblocker for historic #

f071cec

rbajollari added the C:x/oracle label Nov 11, 2022

rbajollari self-assigned this Nov 11, 2022

github-advanced-security bot found potential problems Nov 11, 2022

View reviewed changes

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

rbajollari and others added 3 commits November 13, 2022 13:58

Stamp prices and set median after updating exchange rate

1869ba0

Empty-Commit

4b46f4b

Merge branch 'main' into ryan/historacle-endblocker

a79b457

github-advanced-security bot found potential problems Nov 15, 2022

View reviewed changes

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

Update endblocker based on keeper updates

75b832c

github-advanced-security bot found potential problems Nov 15, 2022

View reviewed changes

Merge branch 'main' into ryan/historacle-endblocker

ee9c854

robert-zaremba reviewed Nov 17, 2022

View reviewed changes

x/oracle/abci.go Outdated Show resolved Hide resolved

x/oracle/abci.go Outdated Show resolved Hide resolved

x/oracle/keeper/historic_price.go Outdated Show resolved Hide resolved

x/oracle/abci.go Outdated Show resolved Hide resolved

rbajollari and others added 4 commits November 17, 2022 08:46

Merge branch 'main' into ryan/historacle-endblocker

e30263d

Merge branch 'main' into ryan/historacle-endblocker

ba20509

Integrate keeper and param changes

d15b5b0

Merge branch 'main' into ryan/historacle-endblocker

c939a47

github-advanced-security bot found potential problems Nov 17, 2022

View reviewed changes

rbajollari added 2 commits November 17, 2022 20:02

Merge branch 'main' into ryan/historacle-endblocker

180ee65

Merge branch 'main' into ryan/historacle-endblocker

879271b

rbajollari mentioned this pull request Nov 21, 2022

Benchmark calc and set medians for historacle # #1612

Open

Fix comment

c3de8b6

rbajollari marked this pull request as ready for review November 21, 2022 16:50

rbajollari requested a review from a team as a code owner November 21, 2022 16:50

Comment out median calc and setting in endblocker

c779eb7

github-advanced-security bot found potential problems Nov 21, 2022

View reviewed changes

x/oracle/abci.go Fixed Show fixed Hide fixed

rbajollari and others added 2 commits November 21, 2022 14:14

Make historacle keeper methods only called in experimental mode in or…

68671c5

…acle endblocker

Merge branch 'main' into ryan/historacle-endblocker

3bef9b1

github-advanced-security bot found potential problems Nov 21, 2022

View reviewed changes

zarazan reviewed Nov 21, 2022

View reviewed changes

x/oracle/abci.go Outdated Show resolved Hide resolved

x/oracle/abci.go Show resolved Hide resolved

rbajollari and others added 3 commits November 22, 2022 10:07

Merge branch 'main' into ryan/historacle-endblocker

61a730f

Merge branch 'main' into ryan/historacle-endblocker

1b3ba2a

Add abci_test

02d813c

github-advanced-security bot found potential problems Nov 22, 2022

View reviewed changes

rbajollari requested a review from zarazan November 22, 2022 20:11

zarazan approved these changes Nov 22, 2022

View reviewed changes

robert-zaremba reviewed Nov 22, 2022

View reviewed changes

x/oracle/abci_test.go Show resolved Hide resolved

robert-zaremba approved these changes Nov 22, 2022

View reviewed changes

Merge branch 'main' into ryan/historacle-endblocker

26ce029

rbajollari merged commit f468a4c into main Nov 23, 2022

rbajollari deleted the ryan/historacle-endblocker branch November 23, 2022 00:36

rbajollari mentioned this pull request Nov 23, 2022

Integration Tests for Oracle EndBlocker #1619

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update oracle endblocker for historic # #1580

feat: Update oracle endblocker for historic # #1580

rbajollari commented Nov 11, 2022

codecov-commenter commented Nov 11, 2022 •

edited

Loading

robert-zaremba left a comment

zarazan commented Nov 21, 2022 •

edited

Loading

zarazan left a comment

zarazan left a comment

robert-zaremba left a comment


		k.ClearExchangeRates(ctx)

		if isPeriodLastBlock(ctx, params.MedianPeriod) && experimental {

feat: Update oracle endblocker for historic # #1580

feat: Update oracle endblocker for historic # #1580

Conversation

rbajollari commented Nov 11, 2022

Description

Author Checklist

Reviewers Checklist

codecov-commenter commented Nov 11, 2022 • edited Loading

Codecov Report

robert-zaremba left a comment

Choose a reason for hiding this comment

zarazan commented Nov 21, 2022 • edited Loading

zarazan left a comment

Choose a reason for hiding this comment

zarazan left a comment

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 11, 2022 •

edited

Loading

zarazan commented Nov 21, 2022 •

edited

Loading