From 8fa42f9e6ee64838eefa012fb78a63fa766e94e7 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Thu, 1 Feb 2024 18:24:12 -0600 Subject: [PATCH] Remove using an iterator from updating TWAP records (#7266) * Remove using an iterator from updating TWAP records * Changelog * Add test case --------- Co-authored-by: mattverse Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com> (cherry picked from commit 9193ee48d130f584fe17ccf27d9aae9c3db14227) --- CHANGELOG.md | 3 ++ x/twap/logic.go | 6 ++-- x/twap/store.go | 17 +++++++++++ x/twap/store_test.go | 71 ++++++++++++++++++++++++++++++++++++++++++++ x/twap/types/keys.go | 6 ++++ 5 files changed, 100 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a81a8b8f7a6..093cc8a94bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#7346](https://github.com/osmosis-labs/osmosis/pull/7346) Prevent heavy gRPC load from app hashing nodes +### Misc Improvements +* [#7266](https://github.com/osmosis-labs/osmosis/pull/7266) Remove an iterator call in updating TWAP records + ## v22.0.0 ### Fee Market Parameter Updates diff --git a/x/twap/logic.go b/x/twap/logic.go index 57daf5d7430..43e5c1edc2a 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 1b9c9a5502b..d7e28d772a7 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/store_test.go b/x/twap/store_test.go index 33ad0d2fa4e..94b53192e5c 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 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")