From 29539a16abe2ce5d467df7305afa17d1dc32c0cb Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Wed, 14 Sep 2022 22:23:45 +0700 Subject: [PATCH 01/17] add new error type --- x/twap/types/errors.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/x/twap/types/errors.go b/x/twap/types/errors.go index f0e5185e781..2a15a37938a 100644 --- 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 InvalidRecordTimeError struct { + RecordBlockHeight int64 + RecordTime time.Time + ActualBlockHeight int64 + ActualTime time.Time +} + +func (e InvalidRecordTimeError) Error() string { + return fmt.Sprintf("Can not update the record, the context time must be greater than record time. Record: block %d at %s\n Actual: block %d at %s\n", e.RecordBlockHeight, e.RecordTime, e.ActualBlockHeight, e.ActualTime) +} From 7b87cd8bef12f79e7a4aa229f56683fc4b63887b Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Wed, 14 Sep 2022 22:24:09 +0700 Subject: [PATCH 02/17] add condition to ensure ctx time > record time --- x/twap/export_test.go | 2 +- x/twap/logic.go | 17 ++++++++++++++--- x/twap/logic_test.go | 27 +++++++-------------------- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/x/twap/export_test.go b/x/twap/export_test.go index f4cf043f7ca..00b225a6721 100644 --- a/x/twap/export_test.go +++ b/x/twap/export_test.go @@ -44,7 +44,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 index b73f6a2ac92..d32f929cc5c 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -134,7 +134,10 @@ 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 @@ -142,8 +145,16 @@ func (k Keeper) updateRecords(ctx sdk.Context, poolId uint64) error { // 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) { newRecord := recordWithUpdatedAccumulators(record, ctx.BlockTime()) + if record.Height >= ctx.BlockHeight() || record.Time.After(ctx.BlockTime()) || record.Time.Equal(ctx.BlockTime()) { + return types.TwapRecord{}, types.InvalidRecordTimeError{ + RecordBlockHeight: record.Height, + RecordTime: record.Time, + ActualBlockHeight: ctx.BlockHeight(), + ActualTime: ctx.BlockTime(), + } + } newRecord.Height = ctx.BlockHeight() newSp0, newSp1, lastErrorTime := getSpotPrices( @@ -154,7 +165,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 index 856e2f0ec6d..53d2527af36 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -253,7 +253,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) }) } @@ -900,25 +901,11 @@ func (s *TestSuite) TestUpdateRecords() { }, }, - expectedHistoricalRecords: []expectedResults{ - // The original record at t. - { - spotPriceA: baseRecord.P0LastSpotPrice, - spotPriceB: baseRecord.P1LastSpotPrice, - }, - // 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 - { - spotPriceA: sdk.OneDec(), - spotPriceB: sdk.OneDec().Add(sdk.OneDec()), - isMostRecent: true, - }, - // The original record at t + 1. - { - spotPriceA: tPlus10sp5Record.P0LastSpotPrice, - spotPriceB: tPlus10sp5Record.P1LastSpotPrice, - }, + expectError: types.InvalidRecordTimeError{ + RecordBlockHeight: tPlus10sp5Record.Height, + RecordTime: tPlus10sp5Record.Time, + ActualBlockHeight: (baseRecord.Height + 1), + ActualTime: baseRecord.Time.Add(time.Second * 5), }, }, // TODO: complete multi-asset pool tests: From 0cbfc0487785a3b178b8b70e1773f1f6b2150383 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Wed, 14 Sep 2022 22:30:08 +0700 Subject: [PATCH 03/17] format --- x/twap/logic.go | 4 ++-- x/twap/logic_test.go | 19 +++++++++---------- x/twap/types/errors.go | 4 ++-- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/x/twap/logic.go b/x/twap/logic.go index d32f929cc5c..2403f57f983 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -150,9 +150,9 @@ func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) (types.Tw if record.Height >= ctx.BlockHeight() || record.Time.After(ctx.BlockTime()) || record.Time.Equal(ctx.BlockTime()) { return types.TwapRecord{}, types.InvalidRecordTimeError{ RecordBlockHeight: record.Height, - RecordTime: record.Time, + RecordTime: record.Time, ActualBlockHeight: ctx.BlockHeight(), - ActualTime: ctx.BlockTime(), + ActualTime: ctx.BlockTime(), } } newRecord.Height = ctx.BlockHeight() diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go index 53d2527af36..38a47e63d1c 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -168,7 +168,6 @@ func (s *TestSuite) TestNewTwapRecord() { s.Require().Equal(sdk.ZeroDec(), twapRecord.P0ArithmeticTwapAccumulator) s.Require().Equal(sdk.ZeroDec(), twapRecord.P1ArithmeticTwapAccumulator) } - }) } } @@ -540,16 +539,16 @@ func (s *TestSuite) TestPruneRecords() { recordHistoryKeepPeriod := s.twapkeeper.RecordHistoryKeepPeriod(s.Ctx) pool1OlderMin2MsRecord, // deleted - pool2OlderMin1MsRecord, // deleted - pool3OlderBaseRecord, // kept as newest under keep period + pool2OlderMin1MsRecord, // deleted + 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 - pool2Min1MsRecord, // kept as newest under keep period - pool3BaseRecord, // kept as it is at the keep period boundary + pool2Min1MsRecord, // kept as newest under keep period + 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{ @@ -614,7 +613,7 @@ func (s *TestSuite) TestUpdateRecords() { isMostRecent bool } - var spError = errors.New("spot price error") + spError := errors.New("spot price error") validateRecords := func(expectedRecords []expectedResults, actualRecords []types.TwapRecord) { s.Require().Equal(len(expectedRecords), len(actualRecords)) @@ -903,9 +902,9 @@ func (s *TestSuite) TestUpdateRecords() { expectError: types.InvalidRecordTimeError{ RecordBlockHeight: tPlus10sp5Record.Height, - RecordTime: tPlus10sp5Record.Time, + RecordTime: tPlus10sp5Record.Time, ActualBlockHeight: (baseRecord.Height + 1), - ActualTime: baseRecord.Time.Add(time.Second * 5), + ActualTime: baseRecord.Time.Add(time.Second * 5), }, }, // TODO: complete multi-asset pool tests: diff --git a/x/twap/types/errors.go b/x/twap/types/errors.go index 2a15a37938a..10775dd82b8 100644 --- a/x/twap/types/errors.go +++ b/x/twap/types/errors.go @@ -65,9 +65,9 @@ func (e InvalidRecordCountError) Error() string { type InvalidRecordTimeError struct { RecordBlockHeight int64 - RecordTime time.Time + RecordTime time.Time ActualBlockHeight int64 - ActualTime time.Time + ActualTime time.Time } func (e InvalidRecordTimeError) Error() string { From b09e65f8bdde68e794ff6b353dadc79555a23c46 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Thu, 15 Sep 2022 00:06:50 +0700 Subject: [PATCH 04/17] replace --- x/twap/logic.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/x/twap/logic.go b/x/twap/logic.go index 2403f57f983..5c5904a4b56 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -8,6 +8,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/osmosis-labs/osmosis/v12/x/twap/types" + gammkeeper "github.com/osmosis-labs/osmosis/v12/x/gamm/keeper" ) func newTwapRecord(k types.AmmInterface, ctx sdk.Context, poolId uint64, denom0, denom1 string) (types.TwapRecord, error) { @@ -146,8 +147,7 @@ func (k Keeper) updateRecords(ctx sdk.Context, poolId uint64) error { // 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, error) { - newRecord := recordWithUpdatedAccumulators(record, ctx.BlockTime()) - if record.Height >= ctx.BlockHeight() || record.Time.After(ctx.BlockTime()) || record.Time.Equal(ctx.BlockTime()) { + if record.Height > ctx.BlockHeight() || record.Time.After(ctx.BlockTime()) || record.Time.Equal(ctx.BlockTime()) { return types.TwapRecord{}, types.InvalidRecordTimeError{ RecordBlockHeight: record.Height, RecordTime: record.Time, @@ -155,6 +155,7 @@ func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) (types.Tw ActualTime: ctx.BlockTime(), } } + newRecord := recordWithUpdatedAccumulators(record, ctx.BlockTime()) newRecord.Height = ctx.BlockHeight() newSp0, newSp1, lastErrorTime := getSpotPrices( From dfc23074573e1f4639b9c9b499d84638957af454 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Thu, 15 Sep 2022 00:14:23 +0700 Subject: [PATCH 05/17] fix unused import --- x/twap/logic.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x/twap/logic.go b/x/twap/logic.go index 5c5904a4b56..6a5e88df45c 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -8,7 +8,6 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/osmosis-labs/osmosis/v12/x/twap/types" - gammkeeper "github.com/osmosis-labs/osmosis/v12/x/gamm/keeper" ) func newTwapRecord(k types.AmmInterface, ctx sdk.Context, poolId uint64, denom0, denom1 string) (types.TwapRecord, error) { @@ -147,7 +146,7 @@ func (k Keeper) updateRecords(ctx sdk.Context, poolId uint64) error { // 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, error) { - if record.Height > ctx.BlockHeight() || record.Time.After(ctx.BlockTime()) || record.Time.Equal(ctx.BlockTime()) { + if record.Height >= ctx.BlockHeight() || record.Time.After(ctx.BlockTime()) || record.Time.Equal(ctx.BlockTime()) { return types.TwapRecord{}, types.InvalidRecordTimeError{ RecordBlockHeight: record.Height, RecordTime: record.Time, From cd78749ee28bc3dfedb3a904290c8661a7277340 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Fri, 23 Sep 2022 21:27:09 +0700 Subject: [PATCH 06/17] update co conditions, bypass the case when creating pool --- x/twap/logic.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x/twap/logic.go b/x/twap/logic.go index 6a5e88df45c..64a25bc3e4a 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -146,7 +146,9 @@ func (k Keeper) updateRecords(ctx sdk.Context, poolId uint64) error { // 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, error) { - if record.Height >= ctx.BlockHeight() || record.Time.After(ctx.BlockTime()) || record.Time.Equal(ctx.BlockTime()) { + if (record.Height >= ctx.BlockHeight() || !record.Time.Before(ctx.BlockTime())) && + !record.P1ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) && + !record.P0ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) { return types.TwapRecord{}, types.InvalidRecordTimeError{ RecordBlockHeight: record.Height, RecordTime: record.Time, From 5e9e64b35d6bc21c02e3d788874cdd568f0fa10c Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Fri, 23 Sep 2022 21:27:25 +0700 Subject: [PATCH 07/17] add cmt --- x/twap/logic.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x/twap/logic.go b/x/twap/logic.go index 64a25bc3e4a..a2b4a116cbd 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -146,6 +146,9 @@ func (k Keeper) updateRecords(ctx sdk.Context, poolId uint64) error { // 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, 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. if (record.Height >= ctx.BlockHeight() || !record.Time.Before(ctx.BlockTime())) && !record.P1ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) && !record.P0ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) { From 5c3e98a7a9a30c2c1b7f50952cbd450cf19a7b85 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Mon, 26 Sep 2022 15:57:26 +0700 Subject: [PATCH 08/17] update test for checking block height --- x/twap/logic_test.go | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go index 1233f222ad5..9bd0c9ff906 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -9,6 +9,8 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" + "github.com/osmosis-labs/osmosis/v12/app/apptesting/osmoassert" gammtypes "github.com/osmosis-labs/osmosis/v12/x/gamm/types" "github.com/osmosis-labs/osmosis/v12/x/twap" @@ -757,6 +759,7 @@ func (s *TestSuite) TestUpdateRecords() { ammMock twapmock.ProgrammedAmmInterface spOverrides []spOverride blockTime time.Time + blockHeight int64 expectedHistoricalRecords []expectedResults expectError error @@ -1006,9 +1009,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 before the last record's update time": { preSetRecords: []types.TwapRecord{baseRecord, tPlus10sp5Record}, poolId: baseRecord.PoolId, @@ -1034,6 +1036,36 @@ func (s *TestSuite) TestUpdateRecords() { 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{ + { + baseDenom: mostRecentRecordPoolOne.Asset0Denom, + quoteDenom: mostRecentRecordPoolOne.Asset1Denom, + overrideSp: sdk.OneDec(), + }, + { + baseDenom: mostRecentRecordPoolOne.Asset1Denom, + quoteDenom: mostRecentRecordPoolOne.Asset0Denom, + overrideSp: sdk.OneDec().Add(sdk.OneDec()), + }, + }, + + expectError: types.InvalidRecordTimeError{ + RecordBlockHeight: mostRecentRecordPoolOne.Height, + RecordTime: mostRecentRecordPoolOne.Time, + ActualBlockHeight: mostRecentRecordPoolOne.Height - 1, + ActualTime: mostRecentRecordPoolOne.Time.Add(time.Second), + }, + }, "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, @@ -1221,6 +1253,9 @@ func (s *TestSuite) TestUpdateRecords() { s.SetupTest() twapKeeper := s.App.TwapKeeper ctx := s.Ctx.WithBlockTime(tc.blockTime) + if tc.blockHeight != 0 { + ctx = s.Ctx.WithBlockHeader(tmproto.Header{Time: tc.blockTime, Height: tc.blockHeight}) + } if len(tc.spOverrides) > 0 { ammMock := twapmock.NewProgrammedAmmInterface(s.App.GAMMKeeper) From f7fcdc328200222bb4dfd32d15696f43b8c67caa Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Mon, 26 Sep 2022 15:58:20 +0700 Subject: [PATCH 09/17] format --- x/twap/logic_test.go | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go index 9bd0c9ff906..7ff95d683bf 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -759,7 +759,7 @@ func (s *TestSuite) TestUpdateRecords() { ammMock twapmock.ProgrammedAmmInterface spOverrides []spOverride blockTime time.Time - blockHeight int64 + blockHeight int64 expectedHistoricalRecords []expectedResults expectError error @@ -1041,9 +1041,9 @@ func (s *TestSuite) TestUpdateRecords() { preSetRecords: []types.TwapRecord{mostRecentRecordPoolOne}, poolId: baseRecord.PoolId, - // Even if lastRecord.Time < ctx.Time, + // Even if lastRecord.Time < ctx.Time, // lastRecord.Height >= ctx.BlockHeight also throws error - blockTime: mostRecentRecordPoolOne.Time.Add(time.Second), + blockTime: mostRecentRecordPoolOne.Time.Add(time.Second), blockHeight: mostRecentRecordPoolOne.Height - 1, spOverrides: []spOverride{ @@ -1069,7 +1069,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, @@ -1157,7 +1157,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, @@ -1165,10 +1165,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, @@ -1186,9 +1186,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()), }, }, @@ -1207,7 +1207,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, }, @@ -1218,8 +1218,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. { @@ -1235,14 +1235,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, }, }, }, From 0409367fcc58ad69077f4613d5ac2669ac8552b1 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Wed, 28 Sep 2022 01:12:29 +0700 Subject: [PATCH 10/17] update err message --- x/twap/types/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/twap/types/errors.go b/x/twap/types/errors.go index 10775dd82b8..7e93707e85e 100644 --- a/x/twap/types/errors.go +++ b/x/twap/types/errors.go @@ -71,5 +71,5 @@ type InvalidRecordTimeError struct { } func (e InvalidRecordTimeError) Error() string { - return fmt.Sprintf("Can not update the record, the context time must be greater than record time. Record: block %d at %s\n Actual: block %d at %s\n", e.RecordBlockHeight, e.RecordTime, e.ActualBlockHeight, e.ActualTime) + 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) } From 24da39c00acc7bec4bb18ac353c6c197b5939487 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Tue, 11 Oct 2022 11:59:24 +0700 Subject: [PATCH 11/17] update ctx & err --- x/twap/logic.go | 2 +- x/twap/logic_test.go | 10 ++++------ x/twap/types/errors.go | 4 ++-- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/x/twap/logic.go b/x/twap/logic.go index 9487fef032a..5f2ffced5fd 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -150,7 +150,7 @@ func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) (types.Tw if (record.Height >= ctx.BlockHeight() || !record.Time.Before(ctx.BlockTime())) && !record.P1ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) && !record.P0ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) { - return types.TwapRecord{}, types.InvalidRecordTimeError{ + return types.TwapRecord{}, types.InvalidUpdateRecordError{ RecordBlockHeight: record.Height, RecordTime: record.Time, ActualBlockHeight: ctx.BlockHeight(), diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go index 7ff95d683bf..9d209b97f04 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -9,8 +9,6 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" - tmproto "github.com/tendermint/tendermint/proto/tendermint/types" - "github.com/osmosis-labs/osmosis/v12/app/apptesting/osmoassert" gammtypes "github.com/osmosis-labs/osmosis/v12/x/gamm/types" "github.com/osmosis-labs/osmosis/v12/x/twap" @@ -1010,7 +1008,7 @@ func (s *TestSuite) TestUpdateRecords() { }, }, // should always be greater than the last record's time. - "new record can't be inserted before the last record's update time": { + "new record can't be inserted prior to the last record's update time": { preSetRecords: []types.TwapRecord{baseRecord, tPlus10sp5Record}, poolId: baseRecord.PoolId, @@ -1029,7 +1027,7 @@ func (s *TestSuite) TestUpdateRecords() { }, }, - expectError: types.InvalidRecordTimeError{ + expectError: types.InvalidUpdateRecordError{ RecordBlockHeight: tPlus10sp5Record.Height, RecordTime: tPlus10sp5Record.Time, ActualBlockHeight: (baseRecord.Height + 1), @@ -1059,7 +1057,7 @@ func (s *TestSuite) TestUpdateRecords() { }, }, - expectError: types.InvalidRecordTimeError{ + expectError: types.InvalidUpdateRecordError{ RecordBlockHeight: mostRecentRecordPoolOne.Height, RecordTime: mostRecentRecordPoolOne.Time, ActualBlockHeight: mostRecentRecordPoolOne.Height - 1, @@ -1254,7 +1252,7 @@ func (s *TestSuite) TestUpdateRecords() { twapKeeper := s.App.TwapKeeper ctx := s.Ctx.WithBlockTime(tc.blockTime) if tc.blockHeight != 0 { - ctx = s.Ctx.WithBlockHeader(tmproto.Header{Time: tc.blockTime, Height: tc.blockHeight}) + ctx = s.Ctx.WithBlockTime(tc.blockTime).WithBlockHeight(tc.blockHeight) } if len(tc.spOverrides) > 0 { diff --git a/x/twap/types/errors.go b/x/twap/types/errors.go index 7e93707e85e..f7c07432bde 100644 --- a/x/twap/types/errors.go +++ b/x/twap/types/errors.go @@ -63,13 +63,13 @@ 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 InvalidRecordTimeError struct { +type InvalidUpdateRecordError struct { RecordBlockHeight int64 RecordTime time.Time ActualBlockHeight int64 ActualTime time.Time } -func (e InvalidRecordTimeError) Error() string { +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) } From bc79f48a251f8e2ca03108f8d128b749caed74f8 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Thu, 6 Apr 2023 12:58:37 +0700 Subject: [PATCH 12/17] resolve conflict --- x/twap/logic_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go index 651b45a044f..e5b6a9a6516 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -667,11 +667,13 @@ func (s *TestSuite) TestUpdateRecords() { } tests := map[string]struct { - preSetRecords []types.TwapRecord - poolId uint64 - ammMock twapmock.ProgrammedPoolManagerInterface - spOverrides []spOverride - blockTime time.Time + preSetRecords []types.TwapRecord + poolId uint64 + ammMock twapmock.ProgrammedPoolManagerInterface + spOverrides []spOverride + poolDenomOverride []string + + blockTime time.Time blockHeight int64 expectedHistoricalRecords []expectedResults From c6f1b8f378529759e78b2c3bbfe2ce014e1641ec Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Fri, 7 Apr 2023 12:26:10 +0700 Subject: [PATCH 13/17] split condition --- x/twap/logic.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/x/twap/logic.go b/x/twap/logic.go index 4fd95590508..ed4d4d015bb 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -159,9 +159,18 @@ func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) (types.Tw // Returns error for last updated records in the same block. // Exception: record is initialized when creating a pool, // then the TwapAccumulator variables are zero. - if (record.Height >= ctx.BlockHeight() || !record.Time.Before(ctx.BlockTime())) && + + // 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{}, fmt.Errorf("Invalid zero twap accumulator") + } + + // Normal case, ctx should be after record height & time + if (record.Height > ctx.BlockHeight() || !record.Time.Before(ctx.BlockTime())) { return types.TwapRecord{}, types.InvalidUpdateRecordError{ RecordBlockHeight: record.Height, RecordTime: record.Time, From 277d7a7aa0dcc050f8996c83d9e8c82f4b6d37c6 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Fri, 7 Apr 2023 14:34:24 +0700 Subject: [PATCH 14/17] update condition & add more test --- x/twap/logic.go | 8 +++---- x/twap/logic_test.go | 55 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/x/twap/logic.go b/x/twap/logic.go index ed4d4d015bb..4a24eedc83e 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -167,10 +167,9 @@ func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) (types.Tw !record.P1ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) && !record.P0ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) { return types.TwapRecord{}, fmt.Errorf("Invalid zero twap accumulator") - } - - // Normal case, ctx should be after record height & time - if (record.Height > ctx.BlockHeight() || !record.Time.Before(ctx.BlockTime())) { + } 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, @@ -178,6 +177,7 @@ func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) (types.Tw ActualTime: ctx.BlockTime(), } } + newRecord := recordWithUpdatedAccumulators(record, ctx.BlockTime()) newRecord.Height = ctx.BlockHeight() diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go index e5b6a9a6516..618679de94f 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -981,6 +981,59 @@ func (s *TestSuite) TestUpdateRecords() { 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(), + }, + { + 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, + }, + }, + + 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{ + { + baseDenom: mostRecentRecordPoolOne.Asset0Denom, + quoteDenom: mostRecentRecordPoolOne.Asset1Denom, + overrideSp: sdk.OneDec(), + }, + { + baseDenom: mostRecentRecordPoolOne.Asset1Denom, + quoteDenom: mostRecentRecordPoolOne.Asset0Denom, + overrideSp: sdk.OneDec().Add(sdk.OneDec()), + }, + }, + expectError: fmt.Errorf("Invalid zero twap accumulator"), + }, "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, @@ -1191,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 } From 2f5103df73e269cc47b4b22be8772ffabd9390aa Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Fri, 7 Apr 2023 14:40:43 +0700 Subject: [PATCH 15/17] lint --- x/twap/logic.go | 5 ++--- x/twap/logic_test.go | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/x/twap/logic.go b/x/twap/logic.go index 4a24eedc83e..55e1c136aa9 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -167,8 +167,7 @@ func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) (types.Tw !record.P1ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) && !record.P0ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) { return types.TwapRecord{}, fmt.Errorf("Invalid zero twap accumulator") - } else - if (record.Height > ctx.BlockHeight() || record.Time.After(ctx.BlockTime())) { + } 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, @@ -177,7 +176,7 @@ func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) (types.Tw ActualTime: ctx.BlockTime(), } } - + newRecord := recordWithUpdatedAccumulators(record, ctx.BlockTime()) newRecord.Height = ctx.BlockHeight() diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go index 618679de94f..69c09293dec 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -582,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{ @@ -673,8 +673,8 @@ func (s *TestSuite) TestUpdateRecords() { spOverrides []spOverride poolDenomOverride []string - blockTime time.Time - blockHeight int64 + blockTime time.Time + blockHeight int64 expectedHistoricalRecords []expectedResults expectError error @@ -987,7 +987,7 @@ func (s *TestSuite) TestUpdateRecords() { // Even if lastRecord.Time < ctx.Time, // lastRecord.Height >= ctx.BlockHeight also throws error - blockTime: baseRecord.Time, + blockTime: baseRecord.Time, spOverrides: []spOverride{ { @@ -1004,8 +1004,8 @@ func (s *TestSuite) TestUpdateRecords() { expectedHistoricalRecords: []expectedResults{ { - spotPriceA: sdk.OneDec(), - spotPriceB: sdk.OneDec().Add(sdk.OneDec()), + spotPriceA: sdk.OneDec(), + spotPriceB: sdk.OneDec().Add(sdk.OneDec()), isMostRecent: true, }, }, @@ -1018,7 +1018,7 @@ func (s *TestSuite) TestUpdateRecords() { // Even if lastRecord.Time < ctx.Time, // lastRecord.Height >= ctx.BlockHeight also throws error - blockTime: mostRecentRecordPoolOne.Time, + blockTime: mostRecentRecordPoolOne.Time, spOverrides: []spOverride{ { From 6574fa689944d4ea62977f905cbec2dc1940af89 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Wed, 12 Apr 2023 04:03:24 +0700 Subject: [PATCH 16/17] update return err --- x/twap/logic.go | 2 +- x/twap/logic_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x/twap/logic.go b/x/twap/logic.go index 55e1c136aa9..ec9a93ce060 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -166,7 +166,7 @@ func (k Keeper) updateRecord(ctx sdk.Context, record types.TwapRecord) (types.Tw if (record.Height == ctx.BlockHeight() || record.Time.Equal(ctx.BlockTime())) && !record.P1ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) && !record.P0ArithmeticTwapAccumulator.Equal(sdk.ZeroDec()) { - return types.TwapRecord{}, fmt.Errorf("Invalid zero twap accumulator") + 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{ diff --git a/x/twap/logic_test.go b/x/twap/logic_test.go index 69c09293dec..e5776ebf968 100644 --- a/x/twap/logic_test.go +++ b/x/twap/logic_test.go @@ -1032,7 +1032,7 @@ func (s *TestSuite) TestUpdateRecords() { overrideSp: sdk.OneDec().Add(sdk.OneDec()), }, }, - expectError: fmt.Errorf("Invalid zero twap accumulator"), + 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}, From a00dbcfb48cec11752dab1b70538be662c542820 Mon Sep 17 00:00:00 2001 From: Hieu Vu <72878483+hieuvubk@users.noreply.github.com> Date: Fri, 5 May 2023 14:09:09 +0700 Subject: [PATCH 17/17] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa6001fa312..e7891a89e08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#4582](https://github.com/osmosis-labs/osmosis/pull/4582) Consistently generate build tags metadata, to return a comma-separated list without stray quotes. This affects the output from `version` CLI subcommand and server info API calls. * [#4549](https://github.com/osmosis-labs/osmosis/pull/4549) Add single pool price estimate queries * [#4767](https://github.com/osmosis-labs/osmosis/pull/4767) Disable create pool with non-zero exit fee + * [#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