From bb58dcf968919facf90fb0c99c3d1c5a090d6e09 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Sun, 7 Jan 2024 15:21:01 -0600 Subject: [PATCH 1/3] Remove using an iterator from updating TWAP records --- x/twap/logic.go | 6 +++--- x/twap/store.go | 17 +++++++++++++++++ x/twap/types/keys.go | 6 ++++++ 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/x/twap/logic.go b/x/twap/logic.go index b45063e2e0c..a4e82b95bb1 100644 --- a/x/twap/logic.go +++ b/x/twap/logic.go @@ -127,13 +127,13 @@ func (k Keeper) EndBlock(ctx sdk.Context) { // - the number of records does not match expected relative to the // number of denoms in the pool. func (k Keeper) updateRecords(ctx sdk.Context, poolId uint64) error { - // Will only err if pool doesn't have most recent entry set - records, err := k.GetAllMostRecentRecordsForPool(ctx, poolId) + denoms, err := k.poolmanagerKeeper.RouteGetPoolDenoms(ctx, poolId) if err != nil { return err } - denoms, err := k.poolmanagerKeeper.RouteGetPoolDenoms(ctx, poolId) + // Will only err if pool doesn't have most recent entry set + records, err := k.GetAllMostRecentRecordsForPoolWithDenoms(ctx, poolId, denoms) if err != nil { return err } diff --git a/x/twap/store.go b/x/twap/store.go index a6892ec5f6e..eb2ced39a68 100644 --- a/x/twap/store.go +++ b/x/twap/store.go @@ -151,6 +151,23 @@ func (k Keeper) GetAllMostRecentRecordsForPool(ctx sdk.Context, poolId uint64) ( return types.GetAllMostRecentTwapsForPool(store, poolId) } +// GetAllMostRecentRecordsForPool returns all most recent twap records +// (in state representation) for the provided pool id. +func (k Keeper) GetAllMostRecentRecordsForPoolWithDenoms(ctx sdk.Context, poolId uint64, denoms []string) ([]types.TwapRecord, error) { + store := ctx.KVStore(k.storeKey) + // if length != 2, use iterator + if len(denoms) != 2 { + return types.GetAllMostRecentTwapsForPool(store, poolId) + } + // else, directly fetch the key. + asset0Denom, asset1Denom, err := types.LexicographicalOrderDenoms(denoms[0], denoms[1]) + if err != nil { + return []types.TwapRecord{}, err + } + record, err := types.GetMostRecentTwapForPool(store, poolId, asset0Denom, asset1Denom) + return []types.TwapRecord{record}, err +} + // getAllHistoricalTimeIndexedTWAPs returns all historical TWAPs indexed by time. func (k Keeper) GetAllHistoricalTimeIndexedTWAPs(ctx sdk.Context) ([]types.TwapRecord, error) { return osmoutils.GatherValuesFromStorePrefix(ctx.KVStore(k.storeKey), []byte(types.HistoricalTWAPTimeIndexPrefix), types.ParseTwapFromBz) diff --git a/x/twap/types/keys.go b/x/twap/types/keys.go index ac3e52a350f..cf1c57087bb 100644 --- a/x/twap/types/keys.go +++ b/x/twap/types/keys.go @@ -92,6 +92,12 @@ func GetAllMostRecentTwapsForPool(store sdk.KVStore, poolId uint64) ([]TwapRecor return osmoutils.GatherValuesFromStore(store, []byte(startPrefix), []byte(endPrefix), ParseTwapFromBz) } +func GetMostRecentTwapForPool(store sdk.KVStore, poolId uint64, denom1, denom2 string) (TwapRecord, error) { + key := FormatMostRecentTWAPKey(poolId, denom1, denom2) + bz := store.Get(key) + return ParseTwapFromBz(bz) +} + func ParseTwapFromBz(bz []byte) (twap TwapRecord, err error) { if len(bz) == 0 { return TwapRecord{}, errors.New("twap not found") From 37e5905e36c59dcc462acaa2c7444943f34bb4c6 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Mon, 8 Jan 2024 21:30:12 -0600 Subject: [PATCH 2/3] Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index be595ef7d39..547b2a00b23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#7203](https://github.com/osmosis-labs/osmosis/pull/7203) Make a maximum number of pools of 100 billion. * [#7259](https://github.com/osmosis-labs/osmosis/pull/7259) Lower gas and CPU overhead of chargeTakerFee (in every swap) * [#7258](https://github.com/osmosis-labs/osmosis/pull/7258) Remove an iterator call in CL swaps and spot price calls. +* [#7266](https://github.com/osmosis-labs/osmosis/pull/7266) Remove an iterator call in updating TWAP records ### Bug Fixes From ac869292759840b75f4e983f455e5dbca26f9dee Mon Sep 17 00:00:00 2001 From: mattverse Date: Mon, 29 Jan 2024 14:57:20 +0900 Subject: [PATCH 3/3] Add test case --- x/twap/store_test.go | 71 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/x/twap/store_test.go b/x/twap/store_test.go index fc53c9b4588..2cafdc7e5ac 100644 --- a/x/twap/store_test.go +++ b/x/twap/store_test.go @@ -148,6 +148,77 @@ func (s *TestSuite) TestGetAllMostRecentRecordsForPool() { } } +func (s *TestSuite) TestGetAllMostRecentRecordsForPoolWithDenoms() { + baseRecord := newEmptyPriceRecord(1, baseTime, denom0, denom1) + + tPlusOneRecord := newEmptyPriceRecord(1, tPlusOne, denom0, denom1) + tests := map[string]struct { + recordsToSet []types.TwapRecord + poolId uint64 + denoms []string + expectedRecords []types.TwapRecord + expectedError bool + }{ + "single record: provide denom, fetch store with key": { + recordsToSet: []types.TwapRecord{baseRecord}, + poolId: 1, + denoms: []string{denom0, denom1}, + expectedRecords: []types.TwapRecord{baseRecord}, + }, + "single record: do not provide denom, fetch state using iterator": { + recordsToSet: []types.TwapRecord{baseRecord}, + poolId: 1, + expectedRecords: []types.TwapRecord{baseRecord}, + }, + "single record: query invalid denoms": { + recordsToSet: []types.TwapRecord{baseRecord}, + poolId: 1, + denoms: []string{"foo", "bar"}, + expectedError: true, + }, + "query non-existent pool": { + recordsToSet: []types.TwapRecord{baseRecord}, + poolId: 2, + expectedRecords: []types.TwapRecord{}, + }, + "set two records with different time": { + recordsToSet: []types.TwapRecord{baseRecord, tPlusOneRecord}, + poolId: 1, + denoms: []string{denom0, denom1}, + expectedRecords: []types.TwapRecord{tPlusOneRecord}, + }, + "set multi-asset pool record - reverse order": { + recordsToSet: []types.TwapRecord{ + newEmptyPriceRecord(1, baseTime, denom0, denom2), + newEmptyPriceRecord(1, baseTime, denom1, denom2), + newEmptyPriceRecord(1, baseTime, denom0, denom1), + }, + poolId: 1, + expectedRecords: []types.TwapRecord{ + newEmptyPriceRecord(1, baseTime, denom0, denom1), + newEmptyPriceRecord(1, baseTime, denom0, denom2), + newEmptyPriceRecord(1, baseTime, denom1, denom2), + }, + }, + } + + for name, test := range tests { + s.Run(name, func() { + s.SetupTest() + for _, record := range test.recordsToSet { + s.twapkeeper.StoreNewRecord(s.Ctx, record) + } + actualRecords, err := s.twapkeeper.GetAllMostRecentRecordsForPoolWithDenoms(s.Ctx, test.poolId, test.denoms) + if test.expectedError { + s.Require().Error(err) + } else { + s.Require().NoError(err) + s.Require().Equal(test.expectedRecords, actualRecords) + } + }) + } +} + // TestGetRecordAtOrBeforeTime takes a list of records as test cases, // and runs storeNewRecord for everything in sequence. // Then it runs GetRecordAtOrBeforeTime, and sees if its equal to expected