Fix: Flip existing twapRecords base/quote price denoms (#5939)

* added logic

* added upgrade handler test

* added changelog

* flip spot price too

* adams suggestions

* rebased

* adams final suggestion

* roman comments

* roman suggestion

* cleanup

* romans suggestion

* lint

* misc cleanup

* added test

* more tests

* added verbose tests

* nit

* romans comment for different twap request

* nit

* roman suggestion

* adams suggestion

CHANGELOG.md

@@ -60,6 +60,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * [#5874](https://github.com/osmosis-labs/osmosis/pull/5874) Remove Partial Migration from superfluid migration to CL
 * [#5901](https://github.com/osmosis-labs/osmosis/pull/5901) Adding support for CW pools in ProtoRev
 * [#5937](https://github.com/osmosis-labs/osmosis/pull/5937) feat: add SetScalingFactorController gov prop
+* [#5939](https://github.com/osmosis-labs/osmosis/pull/5939) Fix: Flip existing twapRecords base/quote price denoms 
+* [#5938](https://github.com/osmosis-labs/osmosis/pull/5938) Chore: Fix valset amino codec
 
 ### BugFix
 

app/upgrades/v17/upgrades.go

@@ -17,6 +17,8 @@ import (
 	"github.com/osmosis-labs/osmosis/v17/app/keepers"
 	"github.com/osmosis-labs/osmosis/v17/app/upgrades"
 	"github.com/osmosis-labs/osmosis/v17/x/protorev/types"
+
+	poolmanagertypes "github.com/osmosis-labs/osmosis/v17/x/poolmanager/types"
 )
 
 func CreateUpgradeHandler(
@@ -33,6 +35,18 @@ func CreateUpgradeHandler(
 			return nil, err
 		}
 
+		// get all the existing CL pools
+		pools, err := keepers.ConcentratedLiquidityKeeper.GetPools(ctx)
+		if err != nil {
+			return nil, err
+		}
+
+		// migrate twap records for CL Pools
+		err = FlipTwapSpotPriceRecords(ctx, pools, keepers)
+		if err != nil {
+			return nil, err
+		}
+
 		// Get community pool address.
 		communityPoolAddress := keepers.AccountKeeper.GetModuleAddress(distrtypes.ModuleName)
 
@@ -128,3 +142,41 @@ func CreateUpgradeHandler(
 		return migrations, nil
 	}
 }
+
+// FlipTwapSpotPriceRecords flips the denoms and spot price of twap record of a given pool.
+func FlipTwapSpotPriceRecords(ctx sdk.Context, pools []poolmanagertypes.PoolI, keepers *keepers.AppKeepers) error {
+	for _, pool := range pools {
+		poolId := pool.GetId()
+		twapRecordHistoricalPoolIndexed, err := keepers.TwapKeeper.GetAllHistoricalPoolIndexedTWAPsForPoolId(ctx, poolId)
+		if err != nil {
+			return err
+		}
+
+		for _, historicalTwapRecord := range twapRecordHistoricalPoolIndexed {
+			oldRecord := historicalTwapRecord
+			historicalTwapRecord.Asset0Denom, historicalTwapRecord.Asset1Denom = oldRecord.Asset1Denom, oldRecord.Asset0Denom
+			historicalTwapRecord.P0LastSpotPrice, historicalTwapRecord.P1LastSpotPrice = oldRecord.P1LastSpotPrice, oldRecord.P0LastSpotPrice
+
+			keepers.TwapKeeper.StoreHistoricalTWAP(ctx, historicalTwapRecord)
+			keepers.TwapKeeper.DeleteHistoricalRecord(ctx, oldRecord)
+		}
+
+		clPoolTwapRecords, err := keepers.TwapKeeper.GetAllMostRecentRecordsForPool(ctx, poolId)
+		if err != nil {
+			return err
+		}
+
+		for _, twapRecord := range clPoolTwapRecords {
+			twapRecord.LastErrorTime = time.Time{}
+			oldRecord := twapRecord
+
+			twapRecord.Asset0Denom, twapRecord.Asset1Denom = oldRecord.Asset1Denom, oldRecord.Asset0Denom
+			twapRecord.P0LastSpotPrice, twapRecord.P1LastSpotPrice = oldRecord.P1LastSpotPrice, oldRecord.P0LastSpotPrice
+
+			keepers.TwapKeeper.StoreNewRecord(ctx, twapRecord)
+			keepers.TwapKeeper.DeleteMostRecentRecord(ctx, oldRecord)
+		}
+	}
+
+	return nil
+}

app/upgrades/v17/upgrades_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"sort"
 	"testing"
+	"time"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
@@ -19,6 +20,7 @@ import (
 	v17 "github.com/osmosis-labs/osmosis/v17/app/upgrades/v17"
 	cltypes "github.com/osmosis-labs/osmosis/v17/x/concentrated-liquidity/types"
 	poolmanagertypes "github.com/osmosis-labs/osmosis/v17/x/poolmanager/types"
+	"github.com/osmosis-labs/osmosis/v17/x/twap/types"
 )
 
 type UpgradeTestSuite struct {
@@ -54,6 +56,32 @@ func dummyUpgrade(suite *UpgradeTestSuite) {
 	suite.Ctx = suite.Ctx.WithBlockHeight(dummyUpgradeHeight)
 }
 
+func dummyTwapRecord(poolId uint64, t time.Time, asset0 string, asset1 string, sp0, accum0, accum1, geomAccum sdk.Dec) types.TwapRecord {
+	return types.TwapRecord{
+		PoolId:      poolId,
+		Time:        t,
+		Asset0Denom: asset0,
+		Asset1Denom: asset1,
+
+		P0LastSpotPrice:             sp0,
+		P1LastSpotPrice:             sdk.OneDec().Quo(sp0),
+		P0ArithmeticTwapAccumulator: accum0,
+		P1ArithmeticTwapAccumulator: accum1,
+		GeometricTwapAccumulator:    geomAccum,
+	}
+}
+
+func assertTwapFlipped(suite *UpgradeTestSuite, pre, post types.TwapRecord) {
+	suite.Require().Equal(pre.Asset0Denom, post.Asset1Denom)
+	suite.Require().Equal(pre.Asset1Denom, post.Asset0Denom)
+	suite.Require().Equal(pre.P0LastSpotPrice, post.P1LastSpotPrice)
+	suite.Require().Equal(pre.P1LastSpotPrice, post.P0LastSpotPrice)
+}
+
+func assertEqual(suite *UpgradeTestSuite, pre, post interface{}) {
+	suite.Require().Equal(pre, post)
+}
+
 func (suite *UpgradeTestSuite) TestUpgrade() {
 	upgradeSetup := func() {
 		// This is done to ensure that we run the InitGenesis() logic for the new modules
@@ -125,10 +153,61 @@ func (suite *UpgradeTestSuite) TestUpgrade() {
 					lastPoolID = poolID
 				}
 
-				return expectedCoinsUsedInUpgradeHandler, lastPoolID
+				existingPool := suite.PrepareConcentratedPoolWithCoins("ibc/1480B8FD20AD5FCAE81EA87584D269547DD4D436843C1D20F15E00EB64743EF4", "uosmo")
+				existingPool2 := suite.PrepareConcentratedPoolWithCoins("akash", "uosmo")
+				existingBalancerPoolId := suite.PrepareBalancerPoolWithCoins(sdk.NewCoin("atom", sdk.NewInt(10000000000)), sdk.NewCoin("uosmo", sdk.NewInt(10000000000)))
+
+				// create few TWAP records for the pools
+				t1 := dummyTwapRecord(existingPool.GetId(), time.Now().Add(-time.Hour*24), "ibc/1480B8FD20AD5FCAE81EA87584D269547DD4D436843C1D20F15E00EB64743EF4", "uosmo", sdk.NewDec(10),
+					sdk.OneDec().MulInt64(10*10),
+					sdk.OneDec().MulInt64(3),
+					sdk.ZeroDec())
+
+				t2 := dummyTwapRecord(existingPool.GetId(), time.Now().Add(-time.Hour*10), "ibc/1480B8FD20AD5FCAE81EA87584D269547DD4D436843C1D20F15E00EB64743EF4", "uosmo", sdk.NewDec(30),
+					sdk.OneDec().MulInt64(10*10+10),
+					sdk.OneDec().MulInt64(5),
+					sdk.ZeroDec())
+
+				t3 := dummyTwapRecord(existingPool.GetId(), time.Now().Add(-time.Hour), "ibc/1480B8FD20AD5FCAE81EA87584D269547DD4D436843C1D20F15E00EB64743EF4", "uosmo", sdk.NewDec(20),
+					sdk.OneDec().MulInt64(10*10+10*5),
+					sdk.OneDec().MulInt64(10),
+					sdk.ZeroDec())
+
+				t4 := dummyTwapRecord(existingPool2.GetId(), time.Now().Add(-time.Hour*24), "akash", "uosmo", sdk.NewDec(10),
+					sdk.OneDec().MulInt64(10*10*10),
+					sdk.OneDec().MulInt64(5),
+					sdk.ZeroDec())
+
+				t5 := dummyTwapRecord(existingPool2.GetId(), time.Now().Add(-time.Hour), "akash", "uosmo", sdk.NewDec(20),
+					sdk.OneDec().MulInt64(10),
+					sdk.OneDec().MulInt64(2),
+					sdk.ZeroDec())
+
+				t6 := dummyTwapRecord(existingBalancerPoolId, time.Now().Add(-time.Hour), "atom", "uosmo", sdk.NewDec(10),
+					sdk.OneDec().MulInt64(10),
+					sdk.OneDec().MulInt64(10),
+					sdk.ZeroDec())
+
+				t7 := dummyTwapRecord(existingBalancerPoolId, time.Now().Add(-time.Minute*20), "atom", "uosmo", sdk.NewDec(50),
+					sdk.OneDec().MulInt64(10*5),
+					sdk.OneDec().MulInt64(5),
+					sdk.ZeroDec())
+
+				// store TWAP records
+				suite.App.TwapKeeper.StoreNewRecord(suite.Ctx, t1)
+				suite.App.TwapKeeper.StoreNewRecord(suite.Ctx, t2)
+				suite.App.TwapKeeper.StoreNewRecord(suite.Ctx, t3)
+				suite.App.TwapKeeper.StoreNewRecord(suite.Ctx, t4)
+				suite.App.TwapKeeper.StoreNewRecord(suite.Ctx, t5)
+				suite.App.TwapKeeper.StoreNewRecord(suite.Ctx, t6)
+				suite.App.TwapKeeper.StoreNewRecord(suite.Ctx, t7)
+
+				return expectedCoinsUsedInUpgradeHandler, existingBalancerPoolId
 
 			},
 			func(ctx sdk.Context, keepers *keepers.AppKeepers, expectedCoinsUsedInUpgradeHandler sdk.Coins, lastPoolID uint64) {
+				lastPoolIdMinusOne := lastPoolID - 1
+				lastPoolIdMinusTwo := lastPoolID - 2
 				stakingParams := suite.App.StakingKeeper.GetParams(suite.Ctx)
 				stakingParams.BondDenom = "uosmo"
 				suite.App.StakingKeeper.SetParams(suite.Ctx, stakingParams)
@@ -137,12 +216,71 @@ func (suite *UpgradeTestSuite) TestUpgrade() {
 				communityPoolAddress := suite.App.AccountKeeper.GetModuleAddress(distrtypes.ModuleName)
 				communityPoolBalancePre := suite.App.BankKeeper.GetAllBalances(suite.Ctx, communityPoolAddress)
 
+				clPool1TwapRecordPreUpgrade, err := keepers.TwapKeeper.GetAllMostRecentRecordsForPool(ctx, lastPoolIdMinusTwo)
+				suite.Require().NoError(err)
+
+				clPool1TwapRecordHistoricalPoolIndexPreUpgrade, err := keepers.TwapKeeper.GetAllHistoricalPoolIndexedTWAPsForPoolId(ctx, lastPoolIdMinusTwo)
+				suite.Require().NoError(err)
+
+				clPool2TwapRecordPreUpgrade, err := keepers.TwapKeeper.GetAllMostRecentRecordsForPool(ctx, lastPoolIdMinusOne)
+				suite.Require().NoError(err)
+
+				clPool2TwapRecordHistoricalPoolIndexPreUpgrade, err := keepers.TwapKeeper.GetAllHistoricalPoolIndexedTWAPsForPoolId(ctx, lastPoolIdMinusOne)
+				suite.Require().NoError(err)
+
+				clPoolsTwapRecordHistoricalTimeIndexPreUpgrade, err := keepers.TwapKeeper.GetAllHistoricalTimeIndexedTWAPs(ctx)
+				suite.Require().NoError(err)
+
 				// Run upgrade handler.
 				dummyUpgrade(suite)
 				suite.Require().NotPanics(func() {
 					suite.App.BeginBlocker(suite.Ctx, abci.RequestBeginBlock{})
 				})
 
+				clPool1TwapRecordPostUpgrade, err := keepers.TwapKeeper.GetAllMostRecentRecordsForPool(ctx, lastPoolIdMinusTwo)
+				suite.Require().NoError(err)
+
+				clPool1TwapRecordHistoricalPoolIndexPostUpgrade, err := keepers.TwapKeeper.GetAllHistoricalPoolIndexedTWAPsForPoolId(ctx, lastPoolIdMinusTwo)
+				suite.Require().NoError(err)
+
+				clPool2TwapRecordPostUpgrade, err := keepers.TwapKeeper.GetAllMostRecentRecordsForPool(ctx, lastPoolIdMinusOne)
+				suite.Require().NoError(err)
+
+				clPool2TwapRecordHistoricalPoolIndexPostUpgrade, err := keepers.TwapKeeper.GetAllHistoricalPoolIndexedTWAPsForPoolId(ctx, lastPoolIdMinusOne)
+				suite.Require().NoError(err)
+
+				clPoolsTwapRecordHistoricalTimeIndexPostUpgrade, err := keepers.TwapKeeper.GetAllHistoricalTimeIndexedTWAPs(ctx)
+				suite.Require().NoError(err)
+
+				// check that all TWAP records aren't empty
+				suite.Require().NotEmpty(clPool1TwapRecordPostUpgrade)
+				suite.Require().NotEmpty(clPool1TwapRecordHistoricalPoolIndexPostUpgrade)
+				suite.Require().NotEmpty(clPool2TwapRecordPostUpgrade)
+				suite.Require().NotEmpty(clPool2TwapRecordHistoricalPoolIndexPostUpgrade)
+				suite.Require().NotEmpty(clPoolsTwapRecordHistoricalTimeIndexPostUpgrade)
+
+				for _, data := range []struct {
+					pre, post []types.TwapRecord
+				}{
+					{clPool1TwapRecordPreUpgrade, clPool1TwapRecordPostUpgrade},
+					{clPool1TwapRecordHistoricalPoolIndexPreUpgrade, clPool1TwapRecordHistoricalPoolIndexPostUpgrade},
+					{clPool2TwapRecordPreUpgrade, clPool2TwapRecordPostUpgrade},
+					{clPool2TwapRecordHistoricalPoolIndexPreUpgrade, clPool2TwapRecordHistoricalPoolIndexPostUpgrade},
+				} {
+					for i := range data.post {
+						assertTwapFlipped(suite, data.pre[i], data.post[i])
+					}
+				}
+
+				for i := range clPoolsTwapRecordHistoricalTimeIndexPostUpgrade {
+					record := clPoolsTwapRecordHistoricalTimeIndexPostUpgrade[i]
+					if record.PoolId == lastPoolIdMinusOne || record.PoolId == lastPoolIdMinusTwo {
+						assertTwapFlipped(suite, clPoolsTwapRecordHistoricalTimeIndexPreUpgrade[i], record)
+					} else if record.PoolId == lastPoolID {
+						assertEqual(suite, clPoolsTwapRecordHistoricalTimeIndexPreUpgrade[i], record)
+					}
+				}
+
 				// Retrieve the community pool balance (and the feePool balance) after the upgrade
 				communityPoolBalancePost := suite.App.BankKeeper.GetAllBalances(suite.Ctx, communityPoolAddress)
 				feePoolCommunityPoolPost := suite.App.DistrKeeper.GetFeePool(suite.Ctx).CommunityPool

x/twap/export_test.go

@@ -24,18 +24,14 @@ func (k Keeper) GetRecordAtOrBeforeTime(ctx sdk.Context, poolId uint64, time tim
 	return k.getRecordAtOrBeforeTime(ctx, poolId, time, asset0Denom, asset1Denom)
 }
 
-func (k Keeper) GetAllHistoricalTimeIndexedTWAPs(ctx sdk.Context) ([]types.TwapRecord, error) {
-	return k.getAllHistoricalTimeIndexedTWAPs(ctx)
+func (k Keeper) TrackChangedPool(ctx sdk.Context, poolId uint64) {
+	k.trackChangedPool(ctx, poolId)
 }
 
 func (k Keeper) GetAllHistoricalPoolIndexedTWAPs(ctx sdk.Context) ([]types.TwapRecord, error) {
 	return k.getAllHistoricalPoolIndexedTWAPs(ctx)
 }
 
-func (k Keeper) TrackChangedPool(ctx sdk.Context, poolId uint64) {
-	k.trackChangedPool(ctx, poolId)
-}
-
 func (k Keeper) GetChangedPools(ctx sdk.Context) []uint64 {
 	return k.getChangedPools(ctx)
 }

x/twap/keeper.go

@@ -73,7 +73,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState *types.GenesisState) {
 func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
 	// These are ordered in increasing order, guaranteed by the iterator
 	// that is prefixed by time.
-	twapRecords, err := k.getAllHistoricalTimeIndexedTWAPs(ctx)
+	twapRecords, err := k.GetAllHistoricalTimeIndexedTWAPs(ctx)
 	if err != nil {
 		panic(err)
 	}

x/twap/keeper_test.go

@@ -383,7 +383,7 @@ func (s *TestSuite) createTestRecordsFromTime(t time.Time) (types.TwapRecord, ty
 // - 3 records at time t
 // - 3 records t time t + 1 seconds
 // all returned records belong to the same pool with poolId
-func (s *TestSuite) createTestRecordsFromTimeInPool(t time.Time, poolId uint64) (types.TwapRecord, types.TwapRecord, types.TwapRecord, types.TwapRecord, types.TwapRecord, types.TwapRecord,
+func (s *TestSuite) CreateTestRecordsFromTimeInPool(t time.Time, poolId uint64) (types.TwapRecord, types.TwapRecord, types.TwapRecord, types.TwapRecord, types.TwapRecord, types.TwapRecord,
 	types.TwapRecord, types.TwapRecord, types.TwapRecord, types.TwapRecord, types.TwapRecord, types.TwapRecord,
 ) {
 	baseRecordAB := newEmptyPriceRecord(poolId, t, denom0, denom1)

x/twap/store.go

@@ -52,7 +52,7 @@ func (k Keeper) getChangedPools(ctx sdk.Context) []uint64 {
 }
 
 // storeHistoricalTWAP writes a twap to the store, in all needed indexing.
-func (k Keeper) storeHistoricalTWAP(ctx sdk.Context, twap types.TwapRecord) {
+func (k Keeper) StoreHistoricalTWAP(ctx sdk.Context, twap types.TwapRecord) {
 	store := ctx.KVStore(k.storeKey)
 	key1 := types.FormatHistoricalTimeIndexTWAPKey(twap.Time, twap.PoolId, twap.Asset0Denom, twap.Asset1Denom)
 	key2 := types.FormatHistoricalPoolIndexTWAPKey(twap.PoolId, twap.Asset0Denom, twap.Asset1Denom, twap.Time)
@@ -110,12 +110,12 @@ func (k Keeper) pruneRecordsBeforeTimeButNewest(ctx sdk.Context, lastKeptTime ti
 			continue
 		}
 
-		k.deleteHistoricalRecord(ctx, twapToRemove)
+		k.DeleteHistoricalRecord(ctx, twapToRemove)
 	}
 	return nil
 }
 
-func (k Keeper) deleteHistoricalRecord(ctx sdk.Context, twap types.TwapRecord) {
+func (k Keeper) DeleteHistoricalRecord(ctx sdk.Context, twap types.TwapRecord) {
 	store := ctx.KVStore(k.storeKey)
 	key1 := types.FormatHistoricalTimeIndexTWAPKey(twap.Time, twap.PoolId, twap.Asset0Denom, twap.Asset1Denom)
 	key2 := types.FormatHistoricalPoolIndexTWAPKey(twap.PoolId, twap.Asset0Denom, twap.Asset1Denom, twap.Time)
@@ -152,22 +152,34 @@ func (k Keeper) GetAllMostRecentRecordsForPool(ctx sdk.Context, poolId uint64) (
 }
 
 // getAllHistoricalTimeIndexedTWAPs returns all historical TWAPs indexed by time.
-func (k Keeper) getAllHistoricalTimeIndexedTWAPs(ctx sdk.Context) ([]types.TwapRecord, error) {
+func (k Keeper) GetAllHistoricalTimeIndexedTWAPs(ctx sdk.Context) ([]types.TwapRecord, error) {
 	return osmoutils.GatherValuesFromStorePrefix(ctx.KVStore(k.storeKey), []byte(types.HistoricalTWAPTimeIndexPrefix), types.ParseTwapFromBz)
 }
 
 // getAllHistoricalPoolIndexedTWAPs returns all historical TWAPs indexed by pool id.
-// nolint: unused
 func (k Keeper) getAllHistoricalPoolIndexedTWAPs(ctx sdk.Context) ([]types.TwapRecord, error) {
 	return osmoutils.GatherValuesFromStorePrefix(ctx.KVStore(k.storeKey), []byte(types.HistoricalTWAPPoolIndexPrefix), types.ParseTwapFromBz)
 }
 
+// GetAllHistoricalPoolIndexedTWAPsForPoolId returns HistoricalTwapRecord for a pool give poolId.
+func (k Keeper) GetAllHistoricalPoolIndexedTWAPsForPoolId(ctx sdk.Context, poolId uint64) ([]types.TwapRecord, error) {
+	return osmoutils.GatherValuesFromStorePrefix(ctx.KVStore(k.storeKey), types.FormatKeyPoolTwapRecords(poolId), types.ParseTwapFromBz)
+}
+
 // StoreNewRecord stores a record, in both the most recent record store and historical stores.
 func (k Keeper) StoreNewRecord(ctx sdk.Context, twap types.TwapRecord) {
 	store := ctx.KVStore(k.storeKey)
 	key := types.FormatMostRecentTWAPKey(twap.PoolId, twap.Asset0Denom, twap.Asset1Denom)
 	osmoutils.MustSet(store, key, &twap)
-	k.storeHistoricalTWAP(ctx, twap)
+	k.StoreHistoricalTWAP(ctx, twap)
+}
+
+// DeleteMostRecentRecord deletes a given record in most recent record store.
+// Note that if there are entries in historical indexes for this record, they are not deleted by this method.
+func (k Keeper) DeleteMostRecentRecord(ctx sdk.Context, twap types.TwapRecord) {
+	store := ctx.KVStore(k.storeKey)
+	key := types.FormatMostRecentTWAPKey(twap.PoolId, twap.Asset0Denom, twap.Asset1Denom)
+	store.Delete(key)
 }
 
 // getRecordAtOrBeforeTime on a given input (id, t, asset0, asset1)