From 474f298f6a58b0dcd99dfcc9aebd14a5135ccc92 Mon Sep 17 00:00:00 2001 From: Roman Date: Mon, 20 Feb 2023 11:10:16 -0500 Subject: [PATCH] fix(x/twap): incorrect time delta due to nanoseconds in time (#4359) * fix(x/twap): incorrect time delta due to nanoseconds in time * remove logs * more clean ups * changelog * restore arithmetic twap test * remove old * unskip geometric * Update x/twap/types/utils.go Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com> * comment --------- Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com> --- CHANGELOG.md | 2 ++ tests/e2e/e2e_test.go | 2 -- x/twap/keeper_test.go | 6 ++++++ x/twap/logic.go | 2 +- x/twap/logic_test.go | 6 ++++++ x/twap/strategy.go | 4 ++-- x/twap/strategy_test.go | 14 ++++++++++++++ x/twap/types/utils.go | 18 ++++++++++++------ x/twap/types/utils_test.go | 12 ++++++++++++ 9 files changed, 55 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f18223bfb64..6de61d14c0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#3715](https://github.com/osmosis-labs/osmosis/pull/3715) Fix x/gamm (golang API) CalculateSpotPrice, balancer.SpotPrice and Stableswap.SpotPrice base and quote asset. * [#3746](https://github.com/osmosis-labs/osmosis/pull/3746) Make ApplyFuncIfNoErr logic preserve panics for OutOfGas behavior. * [#4306](https://github.com/osmosis-labs/osmosis/pull/4306) Prevent adding more tokens to an already finished gauge +* [#4359](https://github.com/osmosis-labs/osmosis/pull/4359) Fix incorrect time delta due to nanoseconds in time causing twap jitter. + ## v14.0.1 diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index a096fedca25..abc0677e2ac 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -1163,8 +1163,6 @@ func (s *IntegrationTestSuite) TestExpeditedProposals() { // Upon swapping 1_000_000 uosmo for stake, supply changes, making uosmo less expensive. // As a result of the swap, twap changes to 0.5. func (s *IntegrationTestSuite) TestGeometricTWAP() { - s.T().Skip("TODO: investigate further: https://github.com/osmosis-labs/osmosis/issues/4342") - const ( // This pool contains 1_000_000 uosmo and 2_000_000 stake. // Equals weights. diff --git a/x/twap/keeper_test.go b/x/twap/keeper_test.go index 4c9ec635691..a6ead5a4f3c 100644 --- a/x/twap/keeper_test.go +++ b/x/twap/keeper_test.go @@ -26,6 +26,7 @@ var ( tMinOne = baseTime.Add(-time.Second) tPlusOneMin = baseTime.Add(time.Minute) basePoolId uint64 = 1 + oneHundredNanoseconds = 100 * time.Nanosecond ) type TestSuite struct { @@ -479,6 +480,11 @@ func withPrice1Set(twapRecord types.TwapRecord, price1ToSet sdk.Dec) types.TwapR return twapRecord } +func withTime(twapRecord types.TwapRecord, time time.Time) types.TwapRecord { + twapRecord.Time = time + return twapRecord +} + func newRecord(poolId uint64, t time.Time, sp0, accum0, accum1, geomAccum sdk.Dec) types.TwapRecord { return types.TwapRecord{ PoolId: poolId, diff --git a/x/twap/logic.go b/x/twap/logic.go index d517b925e5a..e6fcb7e6e01 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -182,7 +182,7 @@ func recordWithUpdatedAccumulators(record types.TwapRecord, newTime time.Time) t return record } newRecord := record - timeDelta := newTime.Sub(record.Time) + timeDelta := types.CanonicalTimeMs(newTime) - types.CanonicalTimeMs(record.Time) newRecord.Time = newTime // record.LastSpotPrice is the last spot price from the block the record was created in, diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go index 5515dadb48e..870e3baf0f5 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -290,6 +290,12 @@ func TestRecordWithUpdatedAccumulators(t *testing.T) { newTime: defaultRecord.Time.Add(time.Second), expRecord: newExpRecord(oneDec.Add(OneSec), twoDec.Add(OneSec), pointFiveDec), }, + + "nanoseconds in time of the original record do not affect final result": { + record: withTime(defaultRecord, defaultRecord.Time.Add(oneHundredNanoseconds)), + newTime: time.Unix(2, 0), + expRecord: newExpRecord(oneDec.Add(OneSec.MulInt64(10)), twoDec.Add(OneSec.QuoInt64(10)), pointFiveDec.Add(geometricTenSecAccum)), + }, } for name, test := range tests { diff --git a/x/twap/strategy.go b/x/twap/strategy.go index a449b7f56ae..b2e3cc5362c 100644 --- a/x/twap/strategy.go +++ b/x/twap/strategy.go @@ -34,7 +34,7 @@ func (s *arithmetic) computeTwap(startRecord types.TwapRecord, endRecord types.T } else { accumDiff = endRecord.P1ArithmeticTwapAccumulator.Sub(startRecord.P1ArithmeticTwapAccumulator) } - timeDelta := endRecord.Time.Sub(startRecord.Time) + timeDelta := types.CanonicalTimeMs(endRecord.Time) - types.CanonicalTimeMs(startRecord.Time) return types.AccumDiffDivDuration(accumDiff, timeDelta) } @@ -47,7 +47,7 @@ func (s *geometric) computeTwap(startRecord types.TwapRecord, endRecord types.Tw return sdk.ZeroDec() } - timeDelta := endRecord.Time.Sub(startRecord.Time) + timeDelta := types.CanonicalTimeMs(endRecord.Time) - types.CanonicalTimeMs(startRecord.Time) arithmeticMeanOfLogPrices := types.AccumDiffDivDuration(accumDiff, timeDelta) exponent := arithmeticMeanOfLogPrices diff --git a/x/twap/strategy_test.go b/x/twap/strategy_test.go index d3354e17565..e3c23b2862c 100644 --- a/x/twap/strategy_test.go +++ b/x/twap/strategy_test.go @@ -146,6 +146,13 @@ func (s *TestSuite) TestComputeArithmeticStrategyTwap() { s, pointOneAccum, tenSecAccum, 100*time.Second, sdk.NewDecWithPrec(1, 1)), "accumulator = 10*OneSec, t=100s. 0 base accum (asset 1)": testCaseFromDeltasAsset1(s, sdk.ZeroDec(), OneSec.MulInt64(10), 100*time.Second, sdk.NewDecWithPrec(1, 1)), + + "start record time with nanoseconds does not change result": { + startRecord: newOneSidedRecord(baseTime.Add(oneHundredNanoseconds), sdk.ZeroDec(), true), + endRecord: newOneSidedRecord(tPlusOne, OneSec, true), + quoteAsset: denom0, + expTwap: sdk.OneDec(), + }, } for name, test := range tests { s.Run(name, func() { @@ -278,6 +285,13 @@ func (s *TestSuite) TestComputeGeometricStrategyTwap() { expTwap: sdk.ZeroDec(), }, + + "start record time with nanoseconds does not change result": { + startRecord: newOneSidedGeometricRecord(baseTime.Add(oneHundredNanoseconds), sdk.ZeroDec()), + endRecord: newOneSidedGeometricRecord(tPlusOne, geometricTenSecAccum), + quoteAsset: denom0, + expTwap: sdk.NewDec(10), + }, } for name, tc := range tests { diff --git a/x/twap/types/utils.go b/x/twap/types/utils.go index 1bd70a1d28f..7ed86258681 100644 --- a/x/twap/types/utils.go +++ b/x/twap/types/utils.go @@ -37,16 +37,14 @@ func GetAllUniqueDenomPairs(denoms []string) []DenomPair { // SpotPriceMulDuration returns the spot price multiplied by the time delta, // that is the spot price between the current and last TWAP record. // A single second accounts for 1_000_000_000 when converted to int64. -func SpotPriceMulDuration(sp sdk.Dec, timeDelta time.Duration) sdk.Dec { - deltaMS := timeDelta.Milliseconds() - return sp.MulInt64(deltaMS) +func SpotPriceMulDuration(sp sdk.Dec, timeDeltaMs int64) sdk.Dec { + return sp.MulInt64(timeDeltaMs) } // AccumDiffDivDuration returns the accumulated difference divided by the the // time delta, that is the spot price between the current and last TWAP record. -func AccumDiffDivDuration(accumDiff sdk.Dec, timeDelta time.Duration) sdk.Dec { - deltaMS := timeDelta.Milliseconds() - return accumDiff.QuoInt64(deltaMS) +func AccumDiffDivDuration(accumDiff sdk.Dec, timeDeltaMs int64) sdk.Dec { + return accumDiff.QuoInt64(timeDeltaMs) } // LexicographicalOrderDenoms takes two denoms and returns them to be in lexicographically ascending order. @@ -62,6 +60,14 @@ func LexicographicalOrderDenoms(denom0, denom1 string) (string, string, error) { return denom0, denom1, nil } +// CanonicalTimeMs returns the canonical time in milliseconds used for twap +// math computations in UTC. Removes any monotonic clock reading prior to conversion to ms. +// In twap, we assume all calculations are done in milliseconds. Therefore, this conversion +// is necessary to make sure that there are no rounding errors. +func CanonicalTimeMs(twapTime time.Time) int64 { + return twapTime.Round(0).UnixMilli() +} + // DenomPair contains pair of assetA and assetB denoms which belong to a pool. type DenomPair struct { Denom0 string diff --git a/x/twap/types/utils_test.go b/x/twap/types/utils_test.go index 480797e483e..d757513c934 100644 --- a/x/twap/types/utils_test.go +++ b/x/twap/types/utils_test.go @@ -3,6 +3,7 @@ package types import ( "fmt" "testing" + "time" "github.com/stretchr/testify/require" @@ -66,3 +67,14 @@ func TestLexicographicalOrderDenoms(t *testing.T) { }) } } + +func TestCanonicalTimeMs(t *testing.T) { + const expectedMs int64 = 2 + + newYorkLocation, err := time.LoadLocation("America/New_York") + require.NoError(t, err) + time := time.Unix(0, int64(time.Millisecond+999999+1)).In(newYorkLocation) + + actualTime := CanonicalTimeMs(time) + require.Equal(t, expectedMs, actualTime) +}