feat(x/twap): Geometric Mean TWAP POC #3420

p0mvn · 2022-11-17T03:37:59Z

Part of: #3013

What is the purpose of the change

This is a proof-of-concept Geometric Mean TWAP implementation.

While the arithmetic mean TWAPs are much more widely used, they should theoretically be less accurate in measuring a geometric Brownian motion process (which is how price movements are usually modeled)

Arithmetic TWAP tends to overweight higher prices relative to lower ones.

Therefore, we also support a geometric mean TWAP.

The core functionality stays similar to the arithmetic mean TWAP. However, instead of computing the geometric mean TWAP naively, we use the following property:

GeometricTwap(P) = 1.0001^{ArithmeticTwap(log_{1.0001}{P})}

Naive computation is expensive and easily overflows. As a result, we track logarithms of prices instead of prices themselves in the accumulators.
When geometric twap is requested, we first compute the arithmetic mean of the logarithms, and then exponentiate it with the same base as the logarithm

Brief Changelog

added geometric twap accumulator to the twap record
implemented geometric accumulator update
refactored computeArithmeticTwap method to be more general and named it computeTwap
implemented computedGeometricTwap method that is called by computeTwap
added tests

Testing and Verifying

This change added tests.

Tracked Future Work

Blocking TODOs

overflow tests, potentially choose different log base and improve Pow() function
test(x/twap): split up TestComputeArithmeticTwap_ThreeAsset into arithmetic and geometric #3443
hand-calculated tests for computeGeometricTwap
twap record migrations in the upgrade handler

Clean up TODOs

address all TODOs for prices and add pruning tests
spec and tests for osmomath Dec's TwapLog

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? no
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
How is the feature or change documented? not applicable

x/twap/README.md

…eGeometricTwap stub

nicolaslara · 2022-11-18T08:06:11Z

osmomath/math.go

+
+// TODO: spec and tests
+func TwapLog(x sdk.Dec) sdk.Dec {
+	return BigDecFromSDKDec(x).TickLog().SDKDec()


nit: I think the use of "tick" here is confusing, since afaik that's a concentrated liquidity term.

TickLog refers to 1.0001 base log which we call as tick.

I'm open to suggestions. This is the best semantics I could think of to refer to this specific choice of base.

Agreed, I also recommend we just do log2

I'm trying to understand this why was 1.0001 base log used instead of log_2 in first place?

nicolaslara · 2022-11-18T08:43:13Z

x/twap/logic.go

+	logP0SpotPrice := osmomath.TwapLog(record.P0LastSpotPrice)
+	// p0NewGeomAccum = log_{1.0001}{P_0} * timeDelta
+	p0NewGeomAccum := types.SpotPriceMulDuration(logP0SpotPrice, timeDelta)
+	newRecord.GeometricTwapAccumulator = newRecord.GeometricTwapAccumulator.Add(p0NewGeomAccum)


Why is it enough to store this for P0? Is it cause we can later obtain p1 via 1/p0?

Exactly. We use this property: Geometric mean of the reciprocals is the reciprocal of the geometric mean.

I attached the proof in the computeGeometricTwap function.

nicolaslara · 2022-11-18T08:49:44Z

x/twap/logic.go

 // precondition: endRecord.Time >= startRecord.Time
 // if (endRecord.LastErrorTime >= startRecord.Time) returns an error at end + result
 // if (startRecord.LastErrorTime == startRecord.Time) returns an error at end + result
 // if (endRecord.Time == startRecord.Time) returns endRecord.LastSpotPrice
 // else returns
 // (endRecord.Accumulator - startRecord.Accumulator) / (endRecord.Time - startRecord.Time)
-func computeArithmeticTwap(startRecord types.TwapRecord, endRecord types.TwapRecord, quoteAsset string) (sdk.Dec, error) {
+func computeTwap(startRecord types.TwapRecord, endRecord types.TwapRecord, quoteAsset string, twapType twapType) (sdk.Dec, error) {


In this function we short circuit if timeDelta == 0 and return endRecord.PxLastSpotPrice. Shouldn't that be different for Geometric twap? i.e.: osmomath.Pow(osmomath.Tick, endRecord.PxLastSpotPrice)

The last price is not stored in log scale. Only the accumulator is. So returning the actual price should be what we want

ahh, of course... that makes sense.

czarcas7ic

Still grasping the concepts, but overall logic appears sound to me

czarcas7ic · 2022-11-17T04:27:20Z

x/twap/api_test.go


 	// accumA = 10 seconds * (spot price = 10) = OneSec * 10 * 10
 	// accumB = 10 seconds * (spot price = 0.1) = OneSec
 	// accumC = 10 seconds * (spot price = 20) = OneSec * 10 * 20
 	accumA, accumB, accumC sdk.Dec = OneSec.MulInt64(10 * 10), OneSec, OneSec.MulInt64(10 * 20)

+	// geomAccumAB = 10 seconds * (log_{1.0001}{spot price = 10})
+	geomAccumAB = geometricTenSecAccum.MulInt64(10)
+	geomAccumAC = geomAccumAB


Is the geomAccumAC = geomAccumAB because we use the spot price of the first asset (A) which they share?

It's due to how our tests are set up. Specifically, newThreeAssetPoolTwapRecordWithDefaults

czarcas7ic · 2022-11-18T17:58:21Z

x/twap/api_test.go

+		OneSec.MulInt64(3),          // accum B
+		sdk.ZeroDec(),               // TODO: choose correct
+	)

 	tPlus20sp2ThreeAssetRecordAB, tPlus20sp2ThreeAssetRecordAC, tPlus20sp2ThreeAssetRecordBC = newThreeAssetPoolTwapRecordWithDefaults(
 		baseTime.Add(20*time.Second),
 		sdk.NewDec(2),                // spot price 0
 		OneSec.MulInt64(10*10+5*10),  // accum A
 		OneSec.MulInt64(3),           // accum B
-		OneSec.MulInt64(20*10+10*10)) // accum C
+		OneSec.MulInt64(20*10+10*10), // accum C
+		sdk.ZeroDec(),                // TODO: choose correct
+		sdk.ZeroDec(),                // TODO: choose correct
+		sdk.ZeroDec(),                // TODO: choose correct
+	)


What do we mean here by "choose correct"?

I need to add more tests where these values would be used.

Currently, this is set to zero to avoid existing arithmetic twap tests from failing due to geometric accumulators not being initialized.

czarcas7ic · 2022-11-18T18:06:09Z

x/twap/logic.go

+	}
+	return computeGeometricTwap(startRecord, endRecord, quoteAsset), err
+}
+


we should explicitly take twapType == geometricTwapType as and else if here. Otherwise we might accidentally compute geomtric twap for an invalid type input

Good point. I changed twapType to be a boolean so this is not possible anymore

czarcas7ic · 2022-11-18T18:08:55Z

x/twap/store_test.go

+	baseRecord := withPrice0Set(newEmptyPriceRecord(1, baseTime, denom0, denom1), sdk.OneDec())
 	tMin1 := baseTime.Add(-time.Second)
-	tMin1Record := newEmptyPriceRecord(1, tMin1, denom0, denom1)
+	tMin1Record := withPrice0Set(newEmptyPriceRecord(1, tMin1, denom0, denom1), sdk.OneDec())
 	tPlus1 := baseTime.Add(time.Second)
-	tPlus1Record := newEmptyPriceRecord(1, tPlus1, denom0, denom1)
+	tPlus1Record := withPrice0Set(newEmptyPriceRecord(1, tPlus1, denom0, denom1), sdk.OneDec())


why are we now setting price0 instead of initializing with empty price record?

price0 of 0 would panic because the logarithm of 0 is undefined.

This is safe because, logically, spot price of 0 should be undefined as well

ValarDragon · 2022-11-18T21:58:03Z

osmomath/math.go

@@ -15,6 +15,8 @@ var powPrecision, _ = sdk.NewDecFromStr("0.00000001")
 var zero sdk.Dec = sdk.ZeroDec()

 var (
+	Tick = sdk.MustNewDecFromStr("1.0001")


Why are we doing logs respective to this base, rather than just 2

I'm tracking this as one of the "Blocking TODOs" in the PR description to revisit next.

I've been following uniswap whitepaper. However, I've also come to the conclusion that we don't need this choice of base and plan to choose the new one while working on overflow tests.

With the base of 2, I've also run into overflow problems so I would like to tackle the correct choice of base next in a separate PR.

Changed base to 2 but still might need to revisit once overflow tests are added

SG! (FWIW im not at all married to the base of 2! Just if we don't have a reason for any particular base, or some number being superior to 2, would prefer to default to 2)

ValarDragon · 2022-11-18T21:59:17Z

x/twap/README.md

+
+Therefore, we also support a geometric mean TWAP.
+
+The core functionality stays similar to the arithmetic mean TWAP. However, instead of computing the geometric mean TWAP naively, we use the following property:


First the naive should be defined

Then how we compute it =p

x/twap/README.md

p0mvn · 2022-11-18T23:32:12Z

@czarcas7ic @ValarDragon @nicolaslara all requested changes are added

czarcas7ic

LGTM as first step, acknowledging the remaining TODOs. Nice work 👍

p0mvn · 2022-11-23T02:55:52Z

While this is not urgent, this PR is still waiting for one more approval

x/twap/logic.go

stackman27 · 2022-11-25T00:36:47Z

osmomath/math.go

+
+// TODO: spec and tests
+func TwapLog(x sdk.Dec) sdk.Dec {
+	return BigDecFromSDKDec(x).TickLog().SDKDec()


I'm trying to understand this why was 1.0001 base log used instead of log_2 in first place?

stackman27

LGTM!

p0mvn · 2022-11-27T23:11:29Z

Merging this to continue making progress on geometrict TWAP. The next items are tracked in: #3013

github-actions bot added C:docs Improvements or additions to documentation C:x/twap Changes to the twap module labels Nov 17, 2022

p0mvn added the V:state/breaking State machine breaking PR label Nov 17, 2022

p0mvn marked this pull request as ready for review November 17, 2022 03:53

p0mvn requested a review from a team November 17, 2022 03:54

nicolaslara reviewed Nov 17, 2022

View reviewed changes

x/twap/README.md Show resolved Hide resolved

p0mvn force-pushed the roman/geometric-twap branch from edeced1 to ccc7c5a Compare November 17, 2022 18:04

p0mvn added 9 commits November 17, 2022 22:50

feat(x/twap): geometric TWAP

89aa40d

add tests for spot price of zero and spot price of one

f4a64f6

refactor computeArithmaticTwap share logic with geometric, add comput…

fa593f3

…eGeometricTwap stub

implement computeGeometricTwap and basic tests

58d7774

finalize TestComputeGeometricTwap

6f298ab

finalize TestComputeTwap

d23f266

basic docs

780347d

nolint

90ee704

format formula

84bb483

p0mvn force-pushed the roman/geometric-twap branch from 6fd661d to 84bb483 Compare November 18, 2022 03:51

nicolaslara reviewed Nov 18, 2022

View reviewed changes

czarcas7ic reviewed Nov 18, 2022

View reviewed changes

ValarDragon reviewed Nov 18, 2022

View reviewed changes

x/twap/README.md Outdated Show resolved Hide resolved

p0mvn added 5 commits November 18, 2022 14:26

Merge branch 'main' into roman/geometric-twap

d1c56fe

change twap type to boolean

1ef5efb

change base to 2

b44558d

define naive

36841f5

twapPow with base 2

a4f4a94

p0mvn requested review from nicolaslara, czarcas7ic and ValarDragon November 18, 2022 23:32

czarcas7ic approved these changes Nov 19, 2022

View reviewed changes

stackman27 reviewed Nov 25, 2022

View reviewed changes

p0mvn mentioned this pull request Nov 25, 2022

feat(x/twap): implement reusable TWAP API #3514

Closed

2 tasks

Sishir's comments

7ad6f2e

p0mvn mentioned this pull request Nov 27, 2022

[x/TWAP] Expose a geometric TWAP API #3529

Merged

stackman27 approved these changes Nov 27, 2022

View reviewed changes

This was referenced Nov 27, 2022

test(twap): ensure test coverage for twapLog and twapPow #3543

Closed

[EPIC][x/twap] Geometric TWAP #3013

Closed

hieuvubk mentioned this pull request Nov 28, 2022

feat(app/upgrades): implement twap record migrations for geometric logic #3542

Closed

p0mvn merged commit f364fe4 into main Nov 28, 2022

p0mvn deleted the roman/geometric-twap branch November 28, 2022 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x/twap): Geometric Mean TWAP POC #3420

feat(x/twap): Geometric Mean TWAP POC #3420

p0mvn commented Nov 17, 2022 •

edited

Loading

nicolaslara Nov 18, 2022

p0mvn Nov 18, 2022

ValarDragon Nov 18, 2022

stackman27 Nov 25, 2022

nicolaslara Nov 18, 2022

p0mvn Nov 18, 2022

nicolaslara Nov 18, 2022

p0mvn Nov 18, 2022

nicolaslara Nov 18, 2022

czarcas7ic left a comment

czarcas7ic Nov 17, 2022

p0mvn Nov 18, 2022

czarcas7ic Nov 18, 2022

p0mvn Nov 18, 2022

czarcas7ic Nov 18, 2022

p0mvn Nov 18, 2022

czarcas7ic Nov 18, 2022

p0mvn Nov 18, 2022

ValarDragon Nov 18, 2022

p0mvn Nov 18, 2022

p0mvn Nov 18, 2022

ValarDragon Nov 19, 2022

ValarDragon Nov 18, 2022

p0mvn Nov 18, 2022

p0mvn commented Nov 18, 2022

czarcas7ic left a comment

p0mvn commented Nov 23, 2022

stackman27 Nov 25, 2022

stackman27 left a comment

p0mvn commented Nov 27, 2022

feat(x/twap): Geometric Mean TWAP POC #3420

feat(x/twap): Geometric Mean TWAP POC #3420

Conversation

p0mvn commented Nov 17, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Tracked Future Work

Blocking TODOs

Clean up TODOs

Documentation and Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Nov 18, 2022

czarcas7ic left a comment

Choose a reason for hiding this comment

p0mvn commented Nov 23, 2022

Choose a reason for hiding this comment

stackman27 left a comment

Choose a reason for hiding this comment

p0mvn commented Nov 27, 2022

p0mvn commented Nov 17, 2022 •

edited

Loading