refactor(x/twap): handle spot price error case in the context of geom…

…etric twap (#3845)

* refactor(x/twap): handle spot price error case

* supporting test cases

* table-driven log tests

osmomath/sigfig_round_test.go

@@ -70,6 +70,13 @@ func TestSigFigRound(t *testing.T) {
 			tenToSigFig:    sdk.NewInt(100),
 			expectedResult: sdk.MustNewDecFromStr("0.087"),
 		},
+
+		{
+			name:           "minimum decimal is still kept",
+			decimal:        sdk.NewDecWithPrec(1, 18),
+			tenToSigFig:    sdk.NewInt(10),
+			expectedResult: sdk.NewDecWithPrec(1, 18),
+		},
 	}
 
 	for i, tc := range testCases {

x/gamm/pool-models/balancer/pool_suite_test.go

@@ -766,6 +766,22 @@ func (suite *KeeperTestSuite) TestBalancerSpotPriceBounds() {
 			quoteDenomWeight:    sdk.NewInt(100),
 			expectError:         true,
 		},
+		{
+			name:                "internal error due to spot price precision being too small, resulting in 0 spot price",
+			quoteDenomPoolInput: sdk.NewCoin(baseDenom, sdk.OneInt()),
+			quoteDenomWeight:    sdk.NewInt(100),
+			baseDenomPoolInput:  sdk.NewCoin(quoteDenom, sdk.NewDec(10).PowerMut(19).TruncateInt().Sub(sdk.NewInt(2))),
+			baseDenomWeight:     sdk.NewInt(100),
+			expectError:         true,
+		},
+		{
+			name:                "at min spot price",
+			quoteDenomPoolInput: sdk.NewCoin(baseDenom, sdk.OneInt()),
+			quoteDenomWeight:    sdk.NewInt(100),
+			baseDenomPoolInput:  sdk.NewCoin(quoteDenom, sdk.NewDec(10).PowerMut(18).TruncateInt()),
+			baseDenomWeight:     sdk.NewInt(100),
+			expectedOutput:      sdk.OneDec().Quo(sdk.NewDec(10).PowerMut(18)),
+		},
 	}
 
 	for _, tc := range tests {

x/twap/logic.go

@@ -190,6 +190,20 @@ func recordWithUpdatedAccumulators(record types.TwapRecord, newTime time.Time) t
 	p1NewAccum := types.SpotPriceMulDuration(record.P1LastSpotPrice, timeDelta)
 	newRecord.P1ArithmeticTwapAccumulator = newRecord.P1ArithmeticTwapAccumulator.Add(p1NewAccum)
 
+	// If the last spot price is zero, then the logarithm is undefined.
+	// As a result, we cannot update the geometric accumulator.
+	// We set the last error time to be the new time, and return the record.
+	if record.P0LastSpotPrice.IsZero() {
+		newRecord.LastErrorTime = newTime
+		return newRecord
+	}
+
+	// logP0SpotPrice = log_{2}{P_0}
+	logP0SpotPrice := twapLog(record.P0LastSpotPrice)
+	// p0NewGeomAccum = log_{2}{P_0} * timeDelta
+	p0NewGeomAccum := types.SpotPriceMulDuration(logP0SpotPrice, timeDelta)
+	newRecord.GeometricTwapAccumulator = newRecord.GeometricTwapAccumulator.Add(p0NewGeomAccum)
+
 	return newRecord
 }
 
@@ -243,11 +257,25 @@ func computeArithmeticTwap(startRecord types.TwapRecord, endRecord types.TwapRec
 		}
 		return endRecord.P1LastSpotPrice, err
 	}
-	var accumDiff sdk.Dec
-	if quoteAsset == startRecord.Asset0Denom {
-		accumDiff = endRecord.P0ArithmeticTwapAccumulator.Sub(startRecord.P0ArithmeticTwapAccumulator)
-	} else {
-		accumDiff = endRecord.P1ArithmeticTwapAccumulator.Sub(startRecord.P1ArithmeticTwapAccumulator)
+
+	return strategy.computeTwap(startRecord, endRecord, quoteAsset), err
+}
+
+// twapLog returns the logarithm of the given spot price, base 2.
+// Panics if zero is given.
+func twapLog(price sdk.Dec) sdk.Dec {
+	if price.IsZero() {
+		panic("twap: cannot take logarithm of zero")
+	}
+
+	return osmomath.BigDecFromSDKDec(price).LogBase2().SDKDec()
+}
+
+// twapPow exponentiates 2 to the given exponent.
+func twapPow(exponent sdk.Dec) sdk.Dec {
+	exp2 := osmomath.Exp2(osmomath.BigDecFromSDKDec(exponent.Abs()))
+	if exponent.IsNegative() {
+		return osmomath.OneDec().Quo(exp2).SDKDec()
 	}
 	return types.AccumDiffDivDuration(accumDiff, timeDelta), err
 }

x/twap/logic_test.go

@@ -9,6 +9,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/stretchr/testify/require"
 
+	"github.com/osmosis-labs/osmosis/osmomath"
 	"github.com/osmosis-labs/osmosis/osmoutils"
 	"github.com/osmosis-labs/osmosis/osmoutils/osmoassert"
 	gammtypes "github.com/osmosis-labs/osmosis/v13/x/gamm/types"
@@ -260,7 +261,27 @@ func TestRecordWithUpdatedAccumulators(t *testing.T) {
 		"same time, accumulator should not change": {
 			record:    defaultRecord,
 			newTime:   time.Unix(1, 0),
-			expRecord: newExpRecord(oneDec, twoDec),
+			expRecord: newExpRecord(oneDec, twoDec, pointFiveDec),
+		},
+		"sp0 - zero spot price - accum0 unchanged, accum1 updated, geom accum unchanged, last err time set": {
+			record:    withPrice0Set(defaultRecord, sdk.ZeroDec()),
+			newTime:   defaultRecord.Time.Add(time.Second),
+			expRecord: withLastErrTime(newExpRecord(oneDec, twoDec.Add(sdk.NewDecWithPrec(1, 1).Mul(OneSec)), pointFiveDec), defaultRecord.Time.Add(time.Second)),
+		},
+		"sp1 - zero spot price - accum0 updated, accum1 unchanged, geom accum updated correctly": {
+			record:    withPrice1Set(defaultRecord, sdk.ZeroDec()),
+			newTime:   defaultRecord.Time.Add(time.Second),
+			expRecord: newExpRecord(tenSecAccum.Add(oneDec), twoDec, pointFiveDec.Add(geometricTenSecAccum)),
+		},
+		"both sp - zero spot price - accum0 unchange, accum1 unchanged, geom accum unchanged": {
+			record:    withPrice1Set(withPrice0Set(defaultRecord, sdk.ZeroDec()), sdk.ZeroDec()),
+			newTime:   defaultRecord.Time.Add(time.Second),
+			expRecord: withLastErrTime(newExpRecord(oneDec, twoDec, pointFiveDec), defaultRecord.Time.Add(time.Second)),
+		},
+		"spot price of one - geom accumulator 0": {
+			record:    withPrice1Set(withPrice0Set(defaultRecord, sdk.OneDec()), sdk.OneDec()),
+			newTime:   defaultRecord.Time.Add(time.Second),
+			expRecord: newExpRecord(oneDec.Add(OneSec), twoDec.Add(OneSec), pointFiveDec),
 		},
 	}
 
@@ -1424,3 +1445,205 @@ func (s *TestSuite) TestAfterCreatePool() {
 		})
 	}
 }
+
+// This tests the behavior of computeArithmeticTwap, around error returning
+// when there has been an intermediate spot price error.
+func (s *TestSuite) TestComputeArithmeticTwapWithSpotPriceError() {
+	newOneSidedRecordWErrorTime := func(time time.Time, accum sdk.Dec, useP0 bool, errTime time.Time) types.TwapRecord {
+		record := newOneSidedRecord(time, accum, useP0)
+		record.LastErrorTime = errTime
+		return record
+	}
+
+	arithStrategy := &twap.ArithmeticTwapStrategy{
+		TwapKeeper: *s.App.TwapKeeper,
+	}
+
+	tests := map[string]computeTwapTestCase{
+		// should error, since end time may have been used to interpolate this value
+		"errAtEndTime from end record": {
+			startRecord: newOneSidedRecord(baseTime, sdk.ZeroDec(), true),
+			endRecord:   newOneSidedRecordWErrorTime(tPlusOne, OneSec, true, tPlusOne),
+			quoteAsset:  denom0,
+			expTwap:     sdk.OneDec(),
+			expErr:      true,
+		},
+		// should error, since start time may have been used to interpolate this value
+		"err at StartTime exactly from end record": {
+			startRecord: newOneSidedRecord(baseTime, sdk.ZeroDec(), true),
+			endRecord:   newOneSidedRecordWErrorTime(tPlusOne, OneSec, true, baseTime),
+			quoteAsset:  denom0,
+			expTwap:     sdk.OneDec(),
+			expErr:      true,
+		},
+		// should error, since start record is erroneous
+		"err at StartTime exactly from start record": {
+			startRecord: newOneSidedRecordWErrorTime(baseTime, sdk.ZeroDec(), true, baseTime),
+			endRecord:   newOneSidedRecord(tPlusOne, OneSec, true),
+			quoteAsset:  denom0,
+			expTwap:     sdk.OneDec(),
+			expErr:      true,
+		},
+		"err before StartTime": {
+			startRecord: newOneSidedRecord(baseTime, sdk.ZeroDec(), true),
+			endRecord:   newOneSidedRecordWErrorTime(tPlusOne, OneSec, true, tMinOne),
+			quoteAsset:  denom0,
+			expTwap:     sdk.OneDec(),
+			expErr:      false,
+		},
+		// Should not happen, but if it did would error
+		"err after EndTime": {
+			startRecord: newOneSidedRecord(baseTime, sdk.ZeroDec(), true),
+			endRecord:   newOneSidedRecordWErrorTime(tPlusOne, OneSec.MulInt64(2), true, baseTime.Add(20*time.Second)),
+			quoteAsset:  denom0,
+			expTwap:     sdk.OneDec().MulInt64(2),
+			expErr:      true,
+		},
+	}
+	for name, test := range tests {
+		s.Run(name, func() {
+			actualTwap, err := twap.ComputeTwap(test.startRecord, test.endRecord, test.quoteAsset, arithStrategy)
+			s.Require().Equal(test.expTwap, actualTwap)
+			osmoassert.ConditionalError(s.T(), test.expErr, err)
+		})
+	}
+}
+
+// TestTwapLog_CorrectBase tests that the base of 2 is used for the twap log function.
+// log_2{16} = 4
+func (s *TestSuite) TestTwapLog_CorrectBase() {
+	logOf := sdk.NewDec(16)
+	expectedValue := sdk.NewDec(4)
+
+	result := twap.TwapLog(logOf)
+
+	s.Require().Equal(expectedValue, result)
+}
+
+func (s *TestSuite) TestTwapLog() {
+	smallestAdditiveTolerance := osmomath.ErrTolerance{
+		AdditiveTolerance: sdk.SmallestDec(),
+	}
+
+	testcases := []struct {
+		name        string
+		price       sdk.Dec
+		expected    sdk.Dec
+		expectPanic bool
+	}{
+		{
+			"max spot price",
+			gammtypes.MaxSpotPrice,
+			// log_2{2^128 - 1} = 128
+			sdk.MustNewDecFromStr("127.999999999999999999"),
+			false,
+		},
+		{
+			"zero price - panic",
+			sdk.ZeroDec(),
+			sdk.Dec{},
+			true,
+		},
+		{
+			"smallest dec",
+			sdk.SmallestDec(),
+			// https://www.wolframalpha.com/input?i=log+base+2+of+%2810%5E-18%29+with+20+digits
+			sdk.MustNewDecFromStr("59.794705707972522262").Neg(),
+			false,
+		},
+	}
+
+	for _, tc := range testcases {
+		s.Run(tc.name, func() {
+			osmoassert.ConditionalPanic(s.T(), tc.expectPanic, func() {
+				result := twap.TwapLog(tc.price)
+
+				smallestAdditiveTolerance.CompareBigDec(
+					osmomath.BigDecFromSDKDec(tc.expected),
+					osmomath.BigDecFromSDKDec(result),
+				)
+			})
+		})
+	}
+}
+
+// TestTwapPow_CorrectBase tests that the base of 2 is used for the twap power function.
+// 2^3 = 8
+func (s *TestSuite) TestTwapPow_CorrectBase() {
+	exponentValue := osmomath.NewBigDec(3)
+	expectedValue := sdk.NewDec(8)
+
+	result := twap.TwapPow(exponentValue.SDKDec())
+
+	s.Require().Equal(expectedValue, result)
+}
+
+// TestTwapPow_NegativeExponent tests that twap pow can handle a negative exponent
+// 2^-1 = 0.5
+func (s *TestSuite) TestTwapPow_NegativeExponent() {
+	expectedResult := sdk.MustNewDecFromStr("0.5")
+	result := twap.TwapPow(oneDec.Neg())
+	s.Require().Equal(expectedResult, result)
+}
+
+func testCaseFromDeltas(s *TestSuite, startAccum, accumDiff sdk.Dec, timeDelta time.Duration, expectedTwap sdk.Dec) computeTwapTestCase {
+	return computeTwapTestCase{
+		newOneSidedRecord(baseTime, startAccum, true),
+		newOneSidedRecord(baseTime.Add(timeDelta), startAccum.Add(accumDiff), true),
+		[]twap.TwapStrategy{
+			&twap.ArithmeticTwapStrategy{
+				TwapKeeper: *s.App.TwapKeeper,
+			},
+		},
+		denom0,
+		expectedTwap,
+		false,
+		false,
+	}
+}
+
+func testCaseFromDeltasAsset1(s *TestSuite, startAccum, accumDiff sdk.Dec, timeDelta time.Duration, expectedTwap sdk.Dec) computeTwapTestCase {
+	return computeTwapTestCase{
+		newOneSidedRecord(baseTime, startAccum, false),
+		newOneSidedRecord(baseTime.Add(timeDelta), startAccum.Add(accumDiff), false),
+		[]twap.TwapStrategy{
+			&twap.ArithmeticTwapStrategy{
+				TwapKeeper: *s.App.TwapKeeper,
+			},
+		},
+		denom1,
+		expectedTwap,
+		false,
+		false,
+	}
+}
+
+func geometricTestCaseFromDeltas0(s *TestSuite, startAccum, accumDiff sdk.Dec, timeDelta time.Duration, expectedTwap sdk.Dec) computeTwapTestCase {
+	return computeTwapTestCase{
+		newOneSidedGeometricRecord(baseTime, startAccum),
+		newOneSidedGeometricRecord(baseTime.Add(timeDelta), startAccum.Add(accumDiff)),
+		[]twap.TwapStrategy{
+			&twap.GeometricTwapStrategy{
+				TwapKeeper: *s.App.TwapKeeper,
+			},
+		},
+		denom0,
+		expectedTwap,
+		false,
+		false,
+	}
+}
+
+func geometricTestCaseFromDeltas1(s *TestSuite, startAccum, accumDiff sdk.Dec, timeDelta time.Duration, expectedTwap sdk.Dec) computeTwapTestCase {
+	return geometricTestCaseFromDeltas0(s, startAccum, accumDiff, timeDelta, sdk.OneDec().Quo(expectedTwap))
+}
+
+func testThreeAssetCaseFromDeltas(startAccum, accumDiff sdk.Dec, timeDelta time.Duration, expectedTwap sdk.Dec) computeThreeAssetArithmeticTwapTestCase {
+	return computeThreeAssetArithmeticTwapTestCase{
+		newThreeAssetOneSidedRecord(baseTime, startAccum, true),
+		newThreeAssetOneSidedRecord(baseTime.Add(timeDelta), startAccum.Add(accumDiff), true),
+		[]string{denom0, denom0, denom1},
+		[]sdk.Dec{expectedTwap, expectedTwap, expectedTwap},
+		false,
+	}
+}