Possible unreported error from GetArithmeticTwap

[migueldingli1997]

System information

Osmosis version: v12.0.0 OS & Version: OSX Commit hash: aa727b5bf25019d561da5d6f82dd9a3892fda2be

Expected behaviour

Assume t0 < t1 < t2, where these are time values

Say the store has the following two TWAP records:

• {time: t0, lastErrorTime: t0} (erroneous)
• {time: t2, lastErrorTime: t0}

And say we want to GetArithmeticTwap with startTime=t1 and endTime=t2. When GetArithmeticTwap is called, the getInterpolatedRecord function will:

• Obtain the record at or before t1, which is {time: t0, lastErrorTime: t0}
• Interpolate this record to {time: t1, lastErrorTime: t0} (time updated)
• Obtain the record at or before t2, which is {time: t2, lastErrorTime: t0}
• Interpolate this record to {time: t2, lastErrorTime: t0}

My understanding here is that the first record should still be considered erroneous even after it was interpolated, since the interpolation is based on erroneous values. The interpolated records are fed into computeArithmeticTwap, which I expect to return an error due to the use of interpolated records based on erroneous values.

Actual behaviour

The computeArithmeticTwap function errors only if endRecord.LastErrorTime (i.e. t0) is after or equal to startRecord.Time (i.e. t1). In our case, t0 < t1 and so the function does not error.

Steps to reproduce the behaviour

Add this failing test to x/twap/api_test.go:

func (s *TestSuite) TestGetArithmeticTwap_ExpectError() {
	t0 := baseTime
	t1 := tPlusOne
	t2 := tPlusOne.Add(time.Second)
	
	recordsToSet := []types.TwapRecord{withLastErrTime(baseRecord, t0)}
	// note: baseRecord.Time == baseRecord.LastErrTime, so baseRecord is erroneous

	ctxTime := tPlusOneMin
	input := makeSimpleTwapInput(t1, t2, baseQuoteAB)

	s.SetupTest()
	s.preSetRecords(recordsToSet)
	s.Ctx = s.Ctx.WithBlockTime(ctxTime)

	_, err := s.twapkeeper.GetArithmeticTwap(s.Ctx, input.poolId,
		input.quoteAssetDenom, input.baseAssetDenom,
		input.startTime, input.endTime)

	s.Require().Error(err)
}

[ValarDragon]

Oh wow, your definitely right. Great catch!!

We should add a catch for this situation here: https://github.com/osmosis-labs/osmosis/blob/3c7eab90146396d7cb5ee7f4580244be78bf44f6/x/twap/logic.go#L196-L210

I think the following should suffice

	record, err := k.getRecordAtOrBeforeTime(ctx, poolId, t, assetA, assetB)
	if err != nil {
		return types.TwapRecord{}, err
	}
	if record.Time.Equal(record.LastErrTime) {
		record.LastErrTime = t
	}

Lets get a fix for this into v13.

Would you be interested in PR'ing a fix?

[migueldingli1997]

Yep I can PR a fix. However, just out of curiosity, would it make more sense to modify the recordWithUpdatedAccumulators function instead, given that it is responsible for updating the record's time. It might as well update the error time as well?

https://github.com/osmosis-labs/osmosis/blob/3c7eab90146396d7cb5ee7f4580244be78bf44f6/x/twap/logic.go#L170-L194

i.e.

newRecord.Time = newTime // this line already exists
if record.Time.Equal(record.LastErrTime) {
	newRecord.LastErrTime = newRecord.Time
}

[ValarDragon]

That SGTM!