Update record sanity check (#2741)

* add new error type * add condition to ensure ctx time > record time * format * replace * fix unused import * update co conditions, bypass the case when creating pool * add cmt * update test for checking block height * format * update err message * update ctx & err * resolve conflict * split condition * update condition & add more test * lint * update return err * update changelog --------- Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Matt, Park <[email protected]>
osmosis-labs · Jun 6, 2023 · e0ed430 · e0ed430
1 parent a2e4597
commit e0ed430
Show file tree

Hide file tree

Showing 5 changed files with 132 additions and 23 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -62,6 +62,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   * [#4907](https://github.com/osmosis-labs/osmosis/pull/4847) Add migrate-position cli
   * [#4912](https://github.com/osmosis-labs/osmosis/pull/4912) Export Position_lock_id mappings to GenesisState
   * [#4974](https://github.com/osmosis-labs/osmosis/pull/4974) Add lock id to `MsgSuperfluidUndelegateAndUnbondLockResponse`
+  * [#2741](https://github.com/osmosis-labs/osmosis/pull/2741) Prevent updating the twap record if `ctx.BlockTime <= record.Time` or `ctx.BlockHeight <= record.Height`. Exception, can update the record created when creating the pool in the same block.
 
 ### API breaks
 

diff --git a/x/twap/export_test.go b/x/twap/export_test.go
@@ -48,7 +48,7 @@ func (k Keeper) GetChangedPools(ctx sdk.Context) []uint64 {
 	return k.getChangedPools(ctx)
 }
 
-func (k Keeper) UpdateRecord(ctx sdk.Context, record types.TwapRecord) types.TwapRecord {
+func (k Keeper) UpdateRecord(ctx sdk.Context, record types.TwapRecord) (types.TwapRecord, error) {
 	return k.updateRecord(ctx, record)
 }
 

diff --git a/x/twap/logic.go b/x/twap/logic.go
@@ -144,15 +144,39 @@ func (k Keeper) updateRecords(ctx sdk.Context, poolId uint64) error {
 	}
 
 	for _, record := range records {
-		newRecord := k.updateRecord(ctx, record)
+		newRecord, err := k.updateRecord(ctx, record)
+		if err != nil {
+			return err
+		}
 		k.storeNewRecord(ctx, newRecord)
 	}
 	return nil
 }
 
 // updateRecord returns a new record with updated accumulators and block time
 // for the current block time.
-func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) types.TwapRecord {
+func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) (types.TwapRecord, error) {
+	// Returns error for last updated records in the same block.
+	// Exception: record is initialized when creating a pool,
+	// then the TwapAccumulator variables are zero.
+
+	// Handle record after creating pool
+	// Incase record height should equal to ctx height
+	// But ArithmeticTwapAccumulators should be zero
+	if (record.Height == ctx.BlockHeight() || record.Time.Equal(ctx.BlockTime())) &&
+		!record.P1ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) &&
+		!record.P0ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) {
+		return types.TwapRecord{}, types.InvalidUpdateRecordError{}
+	} else if record.Height > ctx.BlockHeight() || record.Time.After(ctx.BlockTime()) {
+		// Normal case, ctx should be after record height & time
+		return types.TwapRecord{}, types.InvalidUpdateRecordError{
+			RecordBlockHeight: record.Height,
+			RecordTime:        record.Time,
+			ActualBlockHeight: ctx.BlockHeight(),
+			ActualTime:        ctx.BlockTime(),
+		}
+	}
+
 	newRecord := recordWithUpdatedAccumulators(record, ctx.BlockTime())
 	newRecord.Height = ctx.BlockHeight()
 
@@ -164,7 +188,7 @@ func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) types.Twa
 	newRecord.P1LastSpotPrice = newSp1
 	newRecord.LastErrorTime = lastErrorTime
 
-	return newRecord
+	return newRecord, nil
 }
 
 // pruneRecords prunes twap records that happened earlier than recordHistoryKeepPeriod

diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go
@@ -245,7 +245,8 @@ func (s *TestSuite) TestUpdateRecord() {
 				defaultTwoAssetCoins[1].Denom, defaultTwoAssetCoins[0].Denom,
 				test.spotPriceResult1.Sp, test.spotPriceResult1.Err)
 
-			newRecord := s.twapkeeper.UpdateRecord(s.Ctx, test.record)
+			newRecord, err := s.twapkeeper.UpdateRecord(s.Ctx, test.record)
+			s.Require().NoError(err)
 			s.Equal(test.expRecord, newRecord)
 		})
 	}
@@ -581,15 +582,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{
@@ -672,7 +673,8 @@ func (s *TestSuite) TestUpdateRecords() {
 		spOverrides       []spOverride
 		poolDenomOverride []string
 
-		blockTime time.Time
+		blockTime   time.Time
+		blockHeight int64
 
 		expectedHistoricalRecords []expectedResults
 		expectError               error
@@ -922,9 +924,8 @@ func (s *TestSuite) TestUpdateRecords() {
 				},
 			},
 		},
-		// This case should never happen in-practice since ctx.BlockTime
 		// should always be greater than the last record's time.
-		"two-asset; pre-set at t and t + 1, new record inserted between existing": {
+		"new record can't be inserted prior to the last record's update time": {
 			preSetRecords: []types.TwapRecord{baseRecord, tPlus10sp5Record},
 			poolId:        baseRecord.PoolId,
 
@@ -943,26 +944,95 @@ func (s *TestSuite) TestUpdateRecords() {
 				},
 			},
 
-			expectedHistoricalRecords: []expectedResults{
-				// The original record at t.
+			expectError: types.InvalidUpdateRecordError{
+				RecordBlockHeight: tPlus10sp5Record.Height,
+				RecordTime:        tPlus10sp5Record.Time,
+				ActualBlockHeight: (baseRecord.Height + 1),
+				ActualTime:        baseRecord.Time.Add(time.Second * 5),
+			},
+		},
+		// should always be greater than the last record's block.
+		"new record can't be inserted before the last record's update block": {
+			preSetRecords: []types.TwapRecord{mostRecentRecordPoolOne},
+			poolId:        baseRecord.PoolId,
+
+			// Even if lastRecord.Time < ctx.Time,
+			// lastRecord.Height >= ctx.BlockHeight also throws error
+			blockTime:   mostRecentRecordPoolOne.Time.Add(time.Second),
+			blockHeight: mostRecentRecordPoolOne.Height - 1,
+
+			spOverrides: []spOverride{
 				{
-					spotPriceA: baseRecord.P0LastSpotPrice,
-					spotPriceB: baseRecord.P1LastSpotPrice,
+					baseDenom:  mostRecentRecordPoolOne.Asset0Denom,
+					quoteDenom: mostRecentRecordPoolOne.Asset1Denom,
+					overrideSp: sdk.OneDec(),
+				},
+				{
+					baseDenom:  mostRecentRecordPoolOne.Asset1Denom,
+					quoteDenom: mostRecentRecordPoolOne.Asset0Denom,
+					overrideSp: sdk.OneDec().Add(sdk.OneDec()),
+				},
+			},
+
+			expectError: types.InvalidUpdateRecordError{
+				RecordBlockHeight: mostRecentRecordPoolOne.Height,
+				RecordTime:        mostRecentRecordPoolOne.Time,
+				ActualBlockHeight: mostRecentRecordPoolOne.Height - 1,
+				ActualTime:        mostRecentRecordPoolOne.Time.Add(time.Second),
+			},
+		},
+		"new record can be update in same block with last record if accumulators are zero (afterPoolCreate hook called)": {
+			preSetRecords: []types.TwapRecord{baseRecord},
+			poolId:        baseRecord.PoolId,
+
+			// Even if lastRecord.Time < ctx.Time,
+			// lastRecord.Height >= ctx.BlockHeight also throws error
+			blockTime: baseRecord.Time,
+
+			spOverrides: []spOverride{
+				{
+					baseDenom:  baseRecord.Asset0Denom,
+					quoteDenom: baseRecord.Asset1Denom,
+					overrideSp: sdk.OneDec(),
 				},
-				// The new record added.
-				// TODO: it should not be possible to add a record between existing records.
-				// https://github.com/osmosis-labs/osmosis/issues/2686
+				{
+					baseDenom:  baseRecord.Asset1Denom,
+					quoteDenom: baseRecord.Asset0Denom,
+					overrideSp: sdk.OneDec().Add(sdk.OneDec()),
+				},
+			},
+
+			expectedHistoricalRecords: []expectedResults{
 				{
 					spotPriceA:   sdk.OneDec(),
 					spotPriceB:   sdk.OneDec().Add(sdk.OneDec()),
 					isMostRecent: true,
 				},
-				// The original record at t + 1.
+			},
+
+			expectError: nil,
+		},
+		"new record can't be updated in same block with last record if accumulators not equal to zero": {
+			preSetRecords: []types.TwapRecord{mostRecentRecordPoolOne},
+			poolId:        mostRecentRecordPoolOne.PoolId,
+
+			// Even if lastRecord.Time < ctx.Time,
+			// lastRecord.Height >= ctx.BlockHeight also throws error
+			blockTime: mostRecentRecordPoolOne.Time,
+
+			spOverrides: []spOverride{
 				{
-					spotPriceA: tPlus10sp5Record.P0LastSpotPrice,
-					spotPriceB: tPlus10sp5Record.P1LastSpotPrice,
+					baseDenom:  mostRecentRecordPoolOne.Asset0Denom,
+					quoteDenom: mostRecentRecordPoolOne.Asset1Denom,
+					overrideSp: sdk.OneDec(),
+				},
+				{
+					baseDenom:  mostRecentRecordPoolOne.Asset1Denom,
+					quoteDenom: mostRecentRecordPoolOne.Asset0Denom,
+					overrideSp: sdk.OneDec().Add(sdk.OneDec()),
 				},
 			},
+			expectError: types.InvalidUpdateRecordError{},
 		},
 		"multi-asset pool; pre-set at t and t + 1; creates new records": {
 			preSetRecords: []types.TwapRecord{threeAssetRecordAB, threeAssetRecordAC, threeAssetRecordBC, tPlus10sp5ThreeAssetRecordAB, tPlus10sp5ThreeAssetRecordAC, tPlus10sp5ThreeAssetRecordBC},
@@ -1151,6 +1221,9 @@ func (s *TestSuite) TestUpdateRecords() {
 			s.SetupTest()
 			twapKeeper := s.App.TwapKeeper
 			ctx := s.Ctx.WithBlockTime(tc.blockTime)
+			if tc.blockHeight != 0 {
+				ctx = s.Ctx.WithBlockTime(tc.blockTime).WithBlockHeight(tc.blockHeight)
+			}
 
 			if len(tc.spOverrides) > 0 {
 				ammMock := twapmock.NewProgrammedAmmInterface(s.App.PoolManagerKeeper)
@@ -1171,7 +1244,7 @@ func (s *TestSuite) TestUpdateRecords() {
 			err := twapKeeper.UpdateRecords(ctx, tc.poolId)
 
 			if tc.expectError != nil {
-				s.Require().ErrorIs(err, tc.expectError)
+				s.Require().ErrorAs(err, &tc.expectError)
 				return
 			}
 

diff --git a/x/twap/types/errors.go b/x/twap/types/errors.go
@@ -62,3 +62,14 @@ type InvalidRecordCountError struct {
 func (e InvalidRecordCountError) Error() string {
 	return fmt.Sprintf("The number of records do not match, expected: %d\n got: %d", e.Expected, e.Actual)
 }
+
+type InvalidUpdateRecordError struct {
+	RecordBlockHeight int64
+	RecordTime        time.Time
+	ActualBlockHeight int64
+	ActualTime        time.Time
+}
+
+func (e InvalidUpdateRecordError) Error() string {
+	return fmt.Sprintf("failed to update the record, the context time must be greater than record time; record: block %d at %s, actual: block %d at %s", e.RecordBlockHeight, e.RecordTime, e.ActualBlockHeight, e.ActualTime)
+}