Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Last error time now updates during record interpolation #2923

Merged
merged 10 commits into from
Nov 4, 2022
Merged

Fix: Last error time now updates during record interpolation #2923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8d1e259
Last error time now updates during record interpolation
migueldingli1997 Oct 1, 2022
8e359f7
Add CHANGELOG entry for #2923
migueldingli1997 Oct 1, 2022
7e4f459
Revert "Last error time now updates during record interpolation"
migueldingli1997 Oct 14, 2022
87d6dfe
Revert "Add CHANGELOG entry for #2923"
migueldingli1997 Oct 14, 2022
9d206ee
Merge remote-tracking branch 'upstream/main'
migueldingli1997 Oct 14, 2022
d41127c
Update error time during interpolation and update error checks in twa…
migueldingli1997 Oct 14, 2022
2bf434e
Add changelog entry
migueldingli1997 Oct 14, 2022
ae7ef89
Update x/twap/logic.go
migueldingli1997 Oct 19, 2022
f335b19
Update CHANGELOG.md
migueldingli1997 Oct 31, 2022
6a27a9c
Fix lint issue
migueldingli1997 Oct 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
27 changes: 25 additions & 2 deletions x/twap/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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)": {
mattverse marked this conversation as resolved.
Show resolved Hide resolved
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() {
Expand All @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ErrorIs does not work for spotPriceError so I changed it to an Equals

return
}
s.Require().NoError(err)
Expand Down
12 changes: 10 additions & 2 deletions x/twap/logic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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) {
migueldingli1997 marked this conversation as resolved.
Show resolved Hide resolved
record.LastErrorTime = t
}
record = recordWithUpdatedAccumulators(record, t)
return record, nil
}
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious, do we not need to add startRecord.LastErrorTime.Equal(startRecord.Time)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the case where:

  • endRecord = {Time: 2, LastErrTime: 0}
  • startRecord = {Time: 1, LastErrTime: 1}

The conditions will evaluate to the following:

  • endRecord.LastErrorTime.After(startRecord.Time) - False
  • endRecord.LastErrorTime.Equal(startRecord.Time) - False
  • startRecord.LastErrorTime.Equal(startRecord.Time) - True

Without the third condition, the calculation proceeds. And we do not want it to proceed because the startRecord has an error at its time.

The case is definitely possible because the startRecord might have been interpolated before being passed to this function (e.g. from {Time: 0, LastErrTime: 0} to {Time: 1, LastErrTime: 1}) while the endRecord is untouched.

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)
Expand Down
102 changes: 80 additions & 22 deletions x/twap/logic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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)
}
}
})
}
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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)
}
}
}
})
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1138,18 +1196,18 @@ 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,
quoteDenom: threeAssetRecordAB.Asset1Denom,
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,
Expand All @@ -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()),
},
},

Expand All @@ -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,
},
Expand All @@ -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.
{
Expand All @@ -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,
},
},
},
Expand Down