diff --git a/CHANGELOG.md b/CHANGELOG.md index a7eed7859c5..191e5a00c1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#2803](https://github.com/osmosis-labs/osmosis/pull/2803) Fix total pool liquidity CLI query. * [#2914](https://github.com/osmosis-labs/osmosis/pull/2914) Remove out of gas panics from node logs * [#2937](https://github.com/osmosis-labs/osmosis/pull/2937) End block ordering - staking after gov and module sorting. +* [#2923](https://github.com/osmosis-labs/osmosis/pull/2923) TWAP calculation now errors if it uses records that have errored previously. ### Misc Improvements diff --git a/x/twap/api_test.go b/x/twap/api_test.go index 33f307acd1b..2a58dc01037 100644 --- a/x/twap/api_test.go +++ b/x/twap/api_test.go @@ -285,7 +285,23 @@ func (s *TestSuite) TestGetArithmeticTwap() { input: makeSimpleTwapInput(baseTime.Add(-time.Hour), baseTime, baseQuoteAB), expectError: twap.TimeTooOldError{Time: baseTime.Add(-time.Hour)}, }, - "spot price error in record": { + "spot price error in record at record time (start time = record time)": { + recordsToSet: []types.TwapRecord{withLastErrTime(baseRecord, baseTime)}, + ctxTime: tPlusOneMin, + input: makeSimpleTwapInput(baseTime, tPlusOneMin, baseQuoteAB), + expTwap: sdk.NewDec(10), + expectError: spotPriceError, + expectSpErr: baseTime, + }, + "spot price error in record at record time (start time > record time)": { + recordsToSet: []types.TwapRecord{withLastErrTime(baseRecord, baseTime)}, + ctxTime: tPlusOneMin, + input: makeSimpleTwapInput(tPlusOne, tPlusOneMin, baseQuoteAB), + expTwap: sdk.NewDec(10), + expectError: spotPriceError, + expectSpErr: baseTime, + }, + "spot price error in record after record time": { recordsToSet: []types.TwapRecord{withLastErrTime(baseRecord, tPlusOne)}, ctxTime: tPlusOneMin, input: makeSimpleTwapInput(baseTime, tPlusOneMin, baseQuoteAB), @@ -702,6 +718,13 @@ func (s *TestSuite) TestGetArithmeticTwapToNow() { input: makeSimpleTwapToNowInput(baseTime.Add(time.Hour), baseQuoteAB), expectedError: types.StartTimeAfterEndTimeError{StartTime: baseTime.Add(time.Hour), EndTime: tPlusOne}, }, + "spot price error in record at record time (start time > record time)": { + recordsToSet: []types.TwapRecord{withLastErrTime(baseRecord, baseTime)}, + ctxTime: tPlusOneMin, + input: makeSimpleTwapInput(tPlusOne, tPlusOneMin, baseQuoteAB), + expTwap: sdk.NewDec(10), + expectedError: spotPriceError, + }, } for name, test := range tests { s.Run(name, func() { @@ -719,7 +742,7 @@ func (s *TestSuite) TestGetArithmeticTwapToNow() { if test.expectedError != nil { s.Require().Error(err) - s.Require().ErrorIs(err, test.expectedError) + s.Require().Equal(test.expectedError, err) return } s.Require().NoError(err) diff --git a/x/twap/logic.go b/x/twap/logic.go index f90328fe3d5..8fdd167809d 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -196,6 +196,7 @@ func recordWithUpdatedAccumulators(record types.TwapRecord, newTime time.Time) t // getInterpolatedRecord returns a record for this pool, representing its accumulator state at time `t`. // This is achieved by getting the record `r` that is at, or immediately preceding in state time `t`. // To be clear: the record r s.t. `t - r.Time` is minimized AND `t >= r.Time` +// If for the record obtained, r.Time == r.LastErrorTime, this will also hold for the interpolated record. func (k Keeper) getInterpolatedRecord(ctx sdk.Context, poolId uint64, t time.Time, assetA, assetB string) (types.TwapRecord, error) { assetA, assetB, err := types.LexicographicalOrderDenoms(assetA, assetB) if err != nil { @@ -205,6 +206,10 @@ func (k Keeper) getInterpolatedRecord(ctx sdk.Context, poolId uint64, t time.Tim if err != nil { return types.TwapRecord{}, err } + // if it had errored on the last record, make this record inherit the error + if record.Time.Equal(record.LastErrorTime) { + record.LastErrorTime = t + } record = recordWithUpdatedAccumulators(record, t) return record, nil } @@ -225,14 +230,17 @@ func (k Keeper) getMostRecentRecord(ctx sdk.Context, poolId uint64, assetA, asse // computeArithmeticTwap computes and returns an arithmetic TWAP between // two records given the quote asset. // precondition: endRecord.Time >= startRecord.Time -// if endRecord.LastErrorTime is after startRecord.Time, return an error at end + result +// 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) { // see if we need to return an error, due to spot price issues var err error = nil - if endRecord.LastErrorTime.After(startRecord.Time) || endRecord.LastErrorTime.Equal(startRecord.Time) { + if endRecord.LastErrorTime.After(startRecord.Time) || + endRecord.LastErrorTime.Equal(startRecord.Time) || + startRecord.LastErrorTime.Equal(startRecord.Time) { err = errors.New("twap: error in pool spot price occurred between start and end time, twap result may be faulty") } timeDelta := endRecord.Time.Sub(startRecord.Time) diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go index a5e3beecce4..617df0f6c45 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -328,6 +328,7 @@ func (s *TestSuite) TestGetInterpolatedRecord() { testTime time.Time expectedAccumulator sdk.Dec expectedErr error + expectedLastErrTime time.Time }{ "same time with existing record": { recordsToPreSet: baseRecord, @@ -345,6 +346,16 @@ func (s *TestSuite) TestGetInterpolatedRecord() { // 1(spot price) * 1000(one sec in milli-seconds) expectedAccumulator: baseRecord.P0ArithmeticTwapAccumulator.Add(sdk.NewDec(1000)), }, + "call 1 second after existing record with error": { + recordsToPreSet: withLastErrTime(baseRecord, baseTime), + testPoolId: baseRecord.PoolId, + testDenom0: baseRecord.Asset0Denom, + testDenom1: baseRecord.Asset1Denom, + testTime: baseTime.Add(time.Second), + // 1(spot price) * 1000(one sec in milli-seconds) + expectedAccumulator: baseRecord.P0ArithmeticTwapAccumulator.Add(sdk.NewDec(1000)), + expectedLastErrTime: baseTime.Add(time.Second), + }, "call 1 second before existing record": { recordsToPreSet: baseRecord, testPoolId: baseRecord.PoolId, @@ -391,6 +402,13 @@ func (s *TestSuite) TestGetInterpolatedRecord() { s.Require().Equal(test.recordsToPreSet.P1LastSpotPrice, interpolatedRecord.P1LastSpotPrice) s.Require().Equal(test.expectedAccumulator, interpolatedRecord.P0ArithmeticTwapAccumulator) s.Require().Equal(test.expectedAccumulator, interpolatedRecord.P1ArithmeticTwapAccumulator) + if test.recordsToPreSet.Time.Equal(test.recordsToPreSet.LastErrorTime) { + // last error time updated + s.Require().Equal(test.testTime, interpolatedRecord.LastErrorTime) + } else { + // last error time unchanged + s.Require().Equal(test.recordsToPreSet.LastErrorTime, interpolatedRecord.LastErrorTime) + } } }) } @@ -427,6 +445,31 @@ func (s *TestSuite) TestGetInterpolatedRecord_ThreeAsset() { baseRecord[2].P1ArithmeticTwapAccumulator.Add(sdk.NewDec(20000)), }, }, + "call 1 second after existing record with error": { + recordsToPreSet: []types.TwapRecord{ + withLastErrTime(baseRecord[0], baseTime), + withLastErrTime(baseRecord[1], baseTime), + withLastErrTime(baseRecord[2], baseTime), + }, + testTime: baseTime.Add(time.Second), + // P0 and P1 TwapAccumulators both start at 0 + // A 10 spot price * 1000ms = 10000 + // A 10 spot price * 1000ms = 10000 + // B .1 spot price * 1000ms = 100 + expectedP0Accumulator: []sdk.Dec{ + baseRecord[0].P0ArithmeticTwapAccumulator.Add(sdk.NewDec(10000)), + baseRecord[1].P0ArithmeticTwapAccumulator.Add(sdk.NewDec(10000)), + baseRecord[2].P0ArithmeticTwapAccumulator.Add(sdk.NewDec(100)), + }, + // B .1 spot price * 1000ms = 100 + // C 20 spot price * 1000ms = 20000 + // C 20 spot price * 1000ms = 20000 + expectedP1Accumulator: []sdk.Dec{ + baseRecord[0].P1ArithmeticTwapAccumulator.Add(sdk.NewDec(100)), + baseRecord[1].P1ArithmeticTwapAccumulator.Add(sdk.NewDec(20000)), + baseRecord[2].P1ArithmeticTwapAccumulator.Add(sdk.NewDec(20000)), + }, + }, "call 1 second before existing record": { recordsToPreSet: baseRecord, testTime: baseTime.Add(-time.Second), @@ -461,6 +504,13 @@ func (s *TestSuite) TestGetInterpolatedRecord_ThreeAsset() { s.Require().Equal(test.recordsToPreSet[i].P1LastSpotPrice, interpolatedRecord.P1LastSpotPrice) s.Require().Equal(test.expectedP0Accumulator[i], interpolatedRecord.P0ArithmeticTwapAccumulator) s.Require().Equal(test.expectedP1Accumulator[i], interpolatedRecord.P1ArithmeticTwapAccumulator) + if test.recordsToPreSet[i].Time.Equal(test.recordsToPreSet[i].LastErrorTime) { + // last error time updated + s.Require().Equal(test.testTime, interpolatedRecord.LastErrorTime) + } else { + // last error time unchanged + s.Require().Equal(test.recordsToPreSet[i].LastErrorTime, interpolatedRecord.LastErrorTime) + } } } }) @@ -625,13 +675,21 @@ func TestComputeArithmeticTwapWithSpotPriceError(t *testing.T) { expErr: true, }, // should error, since start time may have been used to interpolate this value - "err at StartTime exactly": { + "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), @@ -666,15 +724,15 @@ func (s *TestSuite) TestPruneRecords() { pool1OlderMin2MsRecord, // deleted pool2OlderMin1MsRecordAB, pool2OlderMin1MsRecordAC, pool2OlderMin1MsRecordBC, // deleted - pool3OlderBaseRecord, // kept as newest under keep period + pool3OlderBaseRecord, // kept as newest under keep period pool4OlderPlus1Record := // kept as newest under keep period - s.createTestRecordsFromTime(baseTime.Add(2 * -recordHistoryKeepPeriod)) + s.createTestRecordsFromTime(baseTime.Add(2 * -recordHistoryKeepPeriod)) pool1Min2MsRecord, // kept as newest under keep period pool2Min1MsRecordAB, pool2Min1MsRecordAC, pool2Min1MsRecordBC, // kept as newest under keep period - pool3BaseRecord, // kept as it is at the keep period boundary + pool3BaseRecord, // kept as it is at the keep period boundary pool4Plus1Record := // kept as it is above the keep period boundary - s.createTestRecordsFromTime(baseTime.Add(-recordHistoryKeepPeriod)) + s.createTestRecordsFromTime(baseTime.Add(-recordHistoryKeepPeriod)) // non-ascending insertion order. recordsToPreSet := []types.TwapRecord{ @@ -1050,7 +1108,7 @@ func (s *TestSuite) TestUpdateRecords() { "multi-asset pool; pre-set at t and t + 1; creates new records": { preSetRecords: []types.TwapRecord{threeAssetRecordAB, threeAssetRecordAC, threeAssetRecordBC, tPlus10sp5ThreeAssetRecordAB, tPlus10sp5ThreeAssetRecordAC, tPlus10sp5ThreeAssetRecordBC}, poolId: threeAssetRecordAB.PoolId, - blockTime: threeAssetRecordAB.Time.Add(time.Second * 11), + blockTime: threeAssetRecordAB.Time.Add(time.Second * 11), spOverrides: []spOverride{ { baseDenom: threeAssetRecordAB.Asset0Denom, @@ -1138,7 +1196,7 @@ func (s *TestSuite) TestUpdateRecords() { "multi-asset pool; pre-set at t and t + 1 with err, large spot price, overwrites error time": { preSetRecords: []types.TwapRecord{threeAssetRecordAB, threeAssetRecordAC, threeAssetRecordBC, withLastErrTime(tPlus10sp5ThreeAssetRecordAB, tPlus10sp5ThreeAssetRecordAB.Time), tPlus10sp5ThreeAssetRecordAC, tPlus10sp5ThreeAssetRecordBC}, poolId: threeAssetRecordAB.PoolId, - blockTime: threeAssetRecordAB.Time.Add(time.Second * 11), + blockTime: threeAssetRecordAB.Time.Add(time.Second * 11), spOverrides: []spOverride{ { baseDenom: threeAssetRecordAB.Asset0Denom, @@ -1146,10 +1204,10 @@ func (s *TestSuite) TestUpdateRecords() { overrideSp: sdk.OneDec(), }, { - baseDenom: threeAssetRecordAB.Asset1Denom, - quoteDenom: threeAssetRecordAB.Asset0Denom, - overrideSp: sdk.OneDec().Add(sdk.OneDec()), - }, + baseDenom: threeAssetRecordAB.Asset1Denom, + quoteDenom: threeAssetRecordAB.Asset0Denom, + overrideSp: sdk.OneDec().Add(sdk.OneDec()), + }, { baseDenom: threeAssetRecordAC.Asset0Denom, quoteDenom: threeAssetRecordAC.Asset1Denom, @@ -1167,9 +1225,9 @@ func (s *TestSuite) TestUpdateRecords() { overrideSp: sdk.OneDec(), }, { - baseDenom: threeAssetRecordBC.Asset1Denom, - quoteDenom: threeAssetRecordBC.Asset0Denom, - overrideSp: sdk.OneDec().Add(sdk.OneDec()), + baseDenom: threeAssetRecordBC.Asset1Denom, + quoteDenom: threeAssetRecordBC.Asset0Denom, + overrideSp: sdk.OneDec().Add(sdk.OneDec()), }, }, @@ -1188,7 +1246,7 @@ func (s *TestSuite) TestUpdateRecords() { // The new record AB added. { spotPriceA: sdk.OneDec(), - spotPriceB: sdk.OneDec().Add(sdk.OneDec()), + spotPriceB: sdk.OneDec().Add(sdk.OneDec()), lastErrorTime: tPlus10sp5ThreeAssetRecordAB.Time, isMostRecent: true, }, @@ -1199,8 +1257,8 @@ func (s *TestSuite) TestUpdateRecords() { }, // The original record AC at t + 1. { - spotPriceA: tPlus10sp5ThreeAssetRecordAC.P0LastSpotPrice, - spotPriceB: tPlus10sp5ThreeAssetRecordAC.P1LastSpotPrice, + spotPriceA: tPlus10sp5ThreeAssetRecordAC.P0LastSpotPrice, + spotPriceB: tPlus10sp5ThreeAssetRecordAC.P1LastSpotPrice, }, // The new record AC added. { @@ -1216,14 +1274,14 @@ func (s *TestSuite) TestUpdateRecords() { }, // The original record BC at t + 1. { - spotPriceA: tPlus10sp5ThreeAssetRecordBC.P0LastSpotPrice, - spotPriceB: tPlus10sp5ThreeAssetRecordBC.P1LastSpotPrice, + spotPriceA: tPlus10sp5ThreeAssetRecordBC.P0LastSpotPrice, + spotPriceB: tPlus10sp5ThreeAssetRecordBC.P1LastSpotPrice, }, // The new record BC added. { - spotPriceA: sdk.OneDec(), - spotPriceB: sdk.OneDec().Add(sdk.OneDec()), - isMostRecent: true, + spotPriceA: sdk.OneDec(), + spotPriceB: sdk.OneDec().Add(sdk.OneDec()), + isMostRecent: true, }, }, },