perf(protorev): base denom as param (#7508)

* base denom protorev param performance change

* register param and fix tests

* delete old KVStore values in the upgrade handler

* add changelog entry

* remove BaseDenom from genState

* clean approach in upgrade handler

* clean up

* Revert "clean up"

This reverts commit e7e9d2e.

* Revert "clean approach in upgrade handler"

This reverts commit 96f9716.

* Revert "remove BaseDenom from genState"

This reverts commit 0b723b4.

* Revert "add changelog entry"

This reverts commit 1297bec.

* Revert "delete old KVStore values in the upgrade handler"

This reverts commit d9b489a.

* Revert "register param and fix tests"

This reverts commit 072eafc.

* Revert "base denom protorev param performance change"

This reverts commit f86211a.

* protorev perf non param

* remove deprecated delete

* reset proto

* final cleanups

* quick test fix

* reserve pool_weights proto field

CHANGELOG.md

@@ -47,6 +47,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * [#7250](https://github.com/osmosis-labs/osmosis/pull/7250) Further filter spam gauges from epoch distribution.
 * [#7472](https://github.com/osmosis-labs/osmosis/pull/7472) Refactor TWAP keys to only require a single key format. Significantly lowers TWAP-caused writes
 * [#7499](https://github.com/osmosis-labs/osmosis/pull/7499) Slight speed/gas improvements to CL CreatePosition and AddToPosition
+* [#7508](https://github.com/osmosis-labs/osmosis/pull/7508) Improve protorev performance by removing iterator and storing base denoms as a single object rather than an array.
 * [#7562](https://github.com/osmosis-labs/osmosis/pull/7562) Speedup Protorev estimation logic by removing unnecessary taker fee simulations.
 
 ## v23.0.0

app/upgrades/v24/upgrades.go

@@ -23,6 +23,18 @@ func CreateUpgradeHandler(
 			return nil, err
 		}
 
+		// We no longer use the base denoms array and instead use the repeated base denoms field for performance reasons.
+		// We retrieve the old base denoms array from the KVStore, delete the array from the KVStore, and set them as a repeated field in the new KVStore.
+		baseDenoms, err := keepers.ProtoRevKeeper.DeprecatedGetAllBaseDenoms(ctx)
+		if err != nil {
+			return nil, err
+		}
+		keepers.ProtoRevKeeper.DeprecatedDeleteBaseDenoms(ctx)
+		err = keepers.ProtoRevKeeper.SetBaseDenoms(ctx, baseDenoms)
+		if err != nil {
+			return nil, err
+		}
+
 		// Now that the TWAP keys are refactored, we can delete all time indexed TWAPs
 		// since we only need the pool indexed TWAPs.
 		keepers.TwapKeeper.DeleteAllHistoricalTimeIndexedTWAPs(ctx)

app/upgrades/v24/upgrades_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/osmosis-labs/osmosis/osmoutils"
 	"github.com/osmosis-labs/osmosis/v23/app/apptesting"
 
+	protorevtypes "github.com/osmosis-labs/osmosis/v23/x/protorev/types"
 	"github.com/osmosis-labs/osmosis/v23/x/twap/types"
 	twaptypes "github.com/osmosis-labs/osmosis/v23/x/twap/types"
 )
@@ -37,6 +38,9 @@ func TestUpgradeTestSuite(t *testing.T) {
 func (s *UpgradeTestSuite) TestUpgrade() {
 	s.Setup()
 
+	// TWAP Setup
+	//
+
 	// Manually set up TWAP records indexed by both pool ID and time.
 	twapStoreKey := s.App.GetKey(twaptypes.ModuleName)
 	store := s.Ctx.KVStore(twapStoreKey)
@@ -75,11 +79,35 @@ func (s *UpgradeTestSuite) TestUpgrade() {
 	s.Require().Len(twapRecords, 1)
 	s.Require().Equal(twap, twapRecords[0])
 
+	// PROTOREV Setup
+	//
+
+	// Set the old KVStore base denoms
+	s.App.ProtoRevKeeper.DeprecatedSetBaseDenoms(s.Ctx, []protorevtypes.BaseDenom{
+		{Denom: protorevtypes.OsmosisDenomination, StepSize: osmomath.NewInt(1_000_000)},
+		{Denom: "atom", StepSize: osmomath.NewInt(1_000_000)},
+		{Denom: "weth", StepSize: osmomath.NewInt(1_000_000)}})
+	oldBaseDenoms, err := s.App.ProtoRevKeeper.DeprecatedGetAllBaseDenoms(s.Ctx)
+	s.Require().NoError(err)
+	s.Require().Equal(3, len(oldBaseDenoms))
+	s.Require().Equal(oldBaseDenoms[0].Denom, protorevtypes.OsmosisDenomination)
+	s.Require().Equal(oldBaseDenoms[1].Denom, "atom")
+	s.Require().Equal(oldBaseDenoms[2].Denom, "weth")
+
+	// The new KVStore should be set to the default
+	newBaseDenoms, err := s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
+	s.Require().NoError(err)
+	s.Require().Equal(protorevtypes.DefaultBaseDenoms, newBaseDenoms)
+
+	// Run the upgrade
 	dummyUpgrade(s)
 	s.Require().NotPanics(func() {
 		s.App.BeginBlocker(s.Ctx, abci.RequestBeginBlock{})
 	})
 
+	// TWAP Tests
+	//
+
 	// TWAP records indexed by time should be completely removed.
 	twapRecords, err = osmoutils.GatherValuesFromStorePrefix(store, []byte(HistoricalTWAPTimeIndexPrefix), types.ParseTwapFromBz)
 	s.Require().NoError(err)
@@ -90,6 +118,19 @@ func (s *UpgradeTestSuite) TestUpgrade() {
 	s.Require().NoError(err)
 	s.Require().Len(twapRecords, 1)
 	s.Require().Equal(twap, twapRecords[0])
+
+	// PROTOREV Tests
+	//
+
+	// The new KVStore should return the old KVStore values
+	newBaseDenoms, err = s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
+	s.Require().NoError(err)
+	s.Require().Equal(oldBaseDenoms, newBaseDenoms)
+
+	// The old KVStore base denoms should be deleted
+	oldBaseDenoms, err = s.App.ProtoRevKeeper.DeprecatedGetAllBaseDenoms(s.Ctx)
+	s.Require().NoError(err)
+	s.Require().Empty(oldBaseDenoms)
 }
 
 func dummyUpgrade(s *UpgradeTestSuite) {

proto/osmosis/protorev/v1beta1/genesis.proto

@@ -25,15 +25,10 @@ message GenesisState {
     (gogoproto.nullable) = false,
     (gogoproto.moretags) = "yaml:\"base_denoms\""
   ];
-  // The pool weights that are being used to calculate the weight (compute cost)
-  // of each route.
-  //
-  // DEPRECATED: This field is deprecated and will be removed in the next
-  // release. It is replaced by the `info_by_pool_type` field.
-  PoolWeights pool_weights = 4 [
-    (gogoproto.nullable) = false,
-    (gogoproto.moretags) = "yaml:\"pool_weights\""
-  ];
+  // DEPRECATED: pool_weights are weights that are being used to calculate the
+  // compute cost of each route. This field is deprecated.
+  // It is replaced by the `info_by_pool_type` field.
+  reserved 4;
   // The number of days since module genesis.
   uint64 days_since_module_genesis = 5
       [ (gogoproto.moretags) = "yaml:\"days_since_module_genesis\"" ];

proto/osmosis/protorev/v1beta1/protorev.proto

@@ -184,6 +184,15 @@ message BaseDenom {
   ];
 }
 
+// BaseDenoms represents all of the base denoms that the module uses for its
+// arbitrage trades.
+message BaseDenoms {
+  repeated BaseDenom base_denoms = 1 [
+    (gogoproto.nullable) = false,
+    (gogoproto.moretags) = "yaml:\"base_denoms\""
+  ];
+}
+
 message AllProtocolRevenue {
   osmosis.poolmanager.v1beta1.TakerFeesTracker taker_fees_tracker = 1 [
     (gogoproto.moretags) = "yaml:\"taker_fees_tracker\"",

x/protorev/keeper/msg_server.go

@@ -149,9 +149,6 @@ func (m MsgServer) SetBaseDenoms(c context.Context, msg *types.MsgSetBaseDenoms)
 		m.k.DeleteAllPoolsForBaseDenom(ctx, baseDenom.Denom)
 	}
 
-	// Delete the old base denoms
-	m.k.DeleteBaseDenoms(ctx)
-
 	if err := m.k.SetBaseDenoms(ctx, msg.BaseDenoms); err != nil {
 		return nil, err
 	}

x/protorev/keeper/protorev.go

@@ -76,12 +76,13 @@ func (k Keeper) DeleteAllTokenPairArbRoutes(ctx sdk.Context) {
 	k.DeleteAllEntriesForKeyPrefix(ctx, types.KeyPrefixTokenPairRoutes)
 }
 
-// GetAllBaseDenoms returns all of the base denoms (sorted by priority in descending order) used to build cyclic arbitrage routes
-func (k Keeper) GetAllBaseDenoms(ctx sdk.Context) ([]types.BaseDenom, error) {
+// DeprecatedGetAllBaseDenoms returns all of the base denoms (sorted by priority in descending order) used to build cyclic arbitrage routes
+// After v24 upgrade, this method should be deleted. We now use the param store.
+func (k Keeper) DeprecatedGetAllBaseDenoms(ctx sdk.Context) ([]types.BaseDenom, error) {
 	baseDenoms := make([]types.BaseDenom, 0)
 
 	store := ctx.KVStore(k.storeKey)
-	iterator := sdk.KVStorePrefixIterator(store, types.KeyPrefixBaseDenoms)
+	iterator := sdk.KVStorePrefixIterator(store, types.KeyPrefixDeprecatedBaseDenoms)
 
 	defer iterator.Close()
 	for ; iterator.Valid(); iterator.Next() {
@@ -97,13 +98,14 @@ func (k Keeper) GetAllBaseDenoms(ctx sdk.Context) ([]types.BaseDenom, error) {
 	return baseDenoms, nil
 }
 
-// SetBaseDenoms sets all of the base denoms used to build cyclic arbitrage routes. The base denoms priority
+// DeprecatedSetBaseDenoms sets all of the base denoms used to build cyclic arbitrage routes. The base denoms priority
 // order is going to match the order of the base denoms in the slice.
-func (k Keeper) SetBaseDenoms(ctx sdk.Context, baseDenoms []types.BaseDenom) error {
-	store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixBaseDenoms)
+// After v24 upgrade, this method should be deleted. We now use the param store.
+func (k Keeper) DeprecatedSetBaseDenoms(ctx sdk.Context, baseDenoms []types.BaseDenom) error {
+	store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixDeprecatedBaseDenoms)
 
 	for i, baseDenom := range baseDenoms {
-		key := types.GetKeyPrefixBaseDenom(uint64(i))
+		key := types.DeprecatedGetKeyPrefixBaseDenom(uint64(i))
 
 		bz, err := baseDenom.Marshal()
 		if err != nil {
@@ -115,9 +117,36 @@ func (k Keeper) SetBaseDenoms(ctx sdk.Context, baseDenoms []types.BaseDenom) err
 	return nil
 }
 
-// DeleteBaseDenoms deletes all of the base denoms
-func (k Keeper) DeleteBaseDenoms(ctx sdk.Context) {
-	k.DeleteAllEntriesForKeyPrefix(ctx, types.KeyPrefixBaseDenoms)
+// DeprecatedDeleteBaseDenoms deletes all of the base denoms.
+// After v24 upgrade, this method should be deleted. We now use the param store.
+func (k Keeper) DeprecatedDeleteBaseDenoms(ctx sdk.Context) {
+	k.DeleteAllEntriesForKeyPrefix(ctx, types.KeyPrefixDeprecatedBaseDenoms)
+}
+
+// GetAllBaseDenoms returns all of the base denoms (sorted by priority in descending order) used to build cyclic arbitrage routes
+func (k Keeper) GetAllBaseDenoms(ctx sdk.Context) ([]types.BaseDenom, error) {
+	store := ctx.KVStore(k.storeKey)
+	bz := store.Get(types.KeyPrefixBaseDenoms)
+	baseDenoms := types.BaseDenoms{}
+	err := baseDenoms.Unmarshal(bz)
+	if err != nil {
+		return []types.BaseDenom{}, err
+	}
+	return baseDenoms.BaseDenoms, nil
+}
+
+// SetBaseDenoms sets all of the base denoms used to build cyclic arbitrage routes. The base denoms priority
+// order is going to match the order of the base denoms in the slice.
+func (k Keeper) SetBaseDenoms(ctx sdk.Context, baseDenoms []types.BaseDenom) error {
+	newBaseDenoms := types.BaseDenoms{BaseDenoms: baseDenoms}
+	store := ctx.KVStore(k.storeKey)
+	test, err := newBaseDenoms.Marshal()
+	if err != nil {
+		return err
+	}
+
+	store.Set(types.KeyPrefixBaseDenoms, test)
+	return nil
 }
 
 // GetPoolForDenomPair returns the id of the highest liquidity pool between the base denom and the denom to match

x/protorev/keeper/protorev_test.go

@@ -67,12 +67,6 @@ func (s *KeeperTestSuite) TestGetAllBaseDenoms() {
 	s.Require().Equal(baseDenoms[1].Denom, "Atom")
 	s.Require().Equal(baseDenoms[2].Denom, "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7")
 
-	// Should be able to delete all base denoms
-	s.App.ProtoRevKeeper.DeleteBaseDenoms(s.Ctx)
-	baseDenoms, err = s.App.ProtoRevKeeper.GetAllBaseDenoms(s.Ctx)
-	s.Require().NoError(err)
-	s.Require().Equal(0, len(baseDenoms))
-
 	// Should be able to set the base denoms
 	err = s.App.ProtoRevKeeper.SetBaseDenoms(s.Ctx, []types.BaseDenom{{Denom: "osmo"}, {Denom: "atom"}, {Denom: "weth"}})
 	s.Require().NoError(err)