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 <[email protected]>

* comment

---------

Co-authored-by: Matt, Park <[email protected]>

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
 

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.

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,

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,

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 {

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

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 {

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

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