Protorev: Update timing for pool updating and dev fee payout

CHANGELOG.md

@@ -49,6 +49,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   * [#4830](https://github.com/osmosis-labs/osmosis/pull/4830) Add gas cost when we AddToGaugeRewards, linearly increase with coins to add
   * [#4886](https://github.com/osmosis-labs/osmosis/pull/4886) Implement MsgSplitRouteSwapExactAmountIn and MsgSplitRouteSwapExactAmountOut that supports route splitting.
   * [#5000](https://github.com/osmosis-labs/osmosis/pull/5000) osmomath.Power panics for base < 1 to temporarily restrict broken logic for such base.
+  * [#4827] (https://github.com/osmosis-labs/osmosis/pull/4827) Protorev: Change highest liquidity pool updating from weekly to daily and change dev fee payout from weekly to after every trade.
 
 ### Misc Improvements
 

app/upgrades/v16/upgrades.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/osmosis-labs/osmosis/v15/app/keepers"
 	"github.com/osmosis-labs/osmosis/v15/app/upgrades"
+	protorevtypes "github.com/osmosis-labs/osmosis/v15/x/protorev/types"
 )
 
 const (
@@ -53,6 +54,10 @@ func CreateUpgradeHandler(
 	keepers *keepers.AppKeepers,
 ) upgradetypes.UpgradeHandler {
 	return func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
+		// Added since when testing the fromVersion was set to 2
+		// and therefore did not run the migradtion for protorev
+		fromVM[protorevtypes.ModuleName] = 1
+
 		// Run migrations before applying any other state changes.
 		// NOTE: DO NOT PUT ANY STATE CHANGES BEFORE RunMigrations().
 		migrations, err := mm.RunMigrations(ctx, configurator, fromVM)

app/upgrades/v16/upgrades_test.go

@@ -14,6 +14,7 @@ import (
 	v16 "github.com/osmosis-labs/osmosis/v15/app/upgrades/v16"
 	cltypes "github.com/osmosis-labs/osmosis/v15/x/concentrated-liquidity/types"
 	poolmanagertypes "github.com/osmosis-labs/osmosis/v15/x/poolmanager/types"
+	protorevtypes "github.com/osmosis-labs/osmosis/v15/x/protorev/types"
 )
 
 type UpgradeTestSuite struct {
@@ -48,6 +49,10 @@ func (suite *UpgradeTestSuite) TestUpgrade() {
 		store := suite.Ctx.KVStore(upgradeStoreKey)
 		versionStore := prefix.NewStore(store, []byte{upgradetypes.VersionMapByte})
 		versionStore.Delete([]byte(cltypes.ModuleName))
+
+		// Ensure proper setup for ProtoRev upgrade testing
+		err := upgradeProtorevSetup(suite)
+		suite.Require().NoError(err)
 	}
 
 	testCases := []struct {
@@ -102,6 +107,9 @@ func (suite *UpgradeTestSuite) TestUpgrade() {
 
 				// Permissionless pool creation is disabled.
 				suite.Require().False(params.IsPermissionlessPoolCreationEnabled)
+
+				// Ensure that the protorev upgrade was successful
+				verifyProtorevUpdateSuccess(suite)
 			},
 			func() {
 			},
@@ -132,3 +140,33 @@ func (suite *UpgradeTestSuite) TestUpgrade() {
 		})
 	}
 }
+
+func upgradeProtorevSetup(suite *UpgradeTestSuite) error {
+	account := apptesting.CreateRandomAccounts(1)[0]
+	suite.App.ProtoRevKeeper.SetDeveloperAccount(suite.Ctx, account)
+
+	devFee := sdk.NewCoin("uosmo", sdk.NewInt(1000000))
+	if err := suite.App.ProtoRevKeeper.SetDeveloperFees(suite.Ctx, devFee); err != nil {
+		return err
+	}
+
+	fundCoin := sdk.NewCoins(sdk.NewCoin("uosmo", sdk.NewInt(1000000)))
+
+	if err := suite.App.AppKeepers.BankKeeper.MintCoins(suite.Ctx, protorevtypes.ModuleName, fundCoin); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func verifyProtorevUpdateSuccess(suite *UpgradeTestSuite) {
+	// Ensure balance was transferred to the developer account
+	devAcc, err := suite.App.ProtoRevKeeper.GetDeveloperAccount(suite.Ctx)
+	suite.Require().NoError(err)
+	suite.Require().Equal(suite.App.BankKeeper.GetBalance(suite.Ctx, devAcc, "uosmo"), sdk.NewCoin("uosmo", sdk.NewInt(1000000)))
+
+	// Ensure developer fees are empty
+	coins, err := suite.App.ProtoRevKeeper.GetAllDeveloperFees(suite.Ctx)
+	suite.Require().NoError(err)
+	suite.Require().Equal(coins, []sdk.Coin{})
+}

x/protorev/keeper/developer_fees.go

@@ -6,6 +6,7 @@ import (
 	"github.com/osmosis-labs/osmosis/v15/x/protorev/types"
 )
 
+// Used in v16 upgrade, can be removed in v17
 // SendDeveloperFeesToDeveloperAccount sends the developer fees from the module account to the developer account
 func (k Keeper) SendDeveloperFeesToDeveloperAccount(ctx sdk.Context) error {
 	// Developer account must be set in order to be able to withdraw developer fees
@@ -32,6 +33,7 @@ func (k Keeper) SendDeveloperFeesToDeveloperAccount(ctx sdk.Context) error {
 	return nil
 }
 
+// Deprecated: Can be removed in v16
 // UpdateDeveloperFees updates the fees that developers can withdraw from the module account
 func (k Keeper) UpdateDeveloperFees(ctx sdk.Context, denom string, profit sdk.Int) error {
 	daysSinceGenesis, err := k.GetDaysSinceModuleGenesis(ctx)
@@ -62,3 +64,37 @@ func (k Keeper) UpdateDeveloperFees(ctx sdk.Context, denom string, profit sdk.In
 
 	return nil
 }
+
+// SendDeveloperFee sends the developer fee from the module account to the developer account
+func (k Keeper) SendDeveloperFee(ctx sdk.Context, arbProfit sdk.Coin) error {
+	// Developer account must be set in order to be able to withdraw developer fees
+	developerAccount, err := k.GetDeveloperAccount(ctx)
+	if err != nil {
+		return err
+	}
+
+	// Get the days since genesis
+	daysSinceGenesis, err := k.GetDaysSinceModuleGenesis(ctx)
+	if err != nil {
+		return err
+	}
+
+	// Initialize the developer profit to 0
+	devProfit := sdk.NewCoin(arbProfit.Denom, sdk.ZeroInt())
+
+	// Calculate the developer fee
+	if daysSinceGenesis < types.Phase1Length {
+		devProfit.Amount = arbProfit.Amount.MulRaw(types.ProfitSplitPhase1).QuoRaw(100)
+	} else if daysSinceGenesis < types.Phase2Length {
+		devProfit.Amount = arbProfit.Amount.MulRaw(types.ProfitSplitPhase2).QuoRaw(100)
+	} else {
+		devProfit.Amount = arbProfit.Amount.MulRaw(types.ProfitSplitPhase3).QuoRaw(100)
+	}
+
+	// Send the developer profit to the developer account
+	if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, developerAccount, sdk.NewCoins(devProfit)); err != nil {
+		return err
+	}
+
+	return nil
+}

x/protorev/keeper/developer_fees_test.go

@@ -7,73 +7,68 @@ import (
 	"github.com/osmosis-labs/osmosis/v15/x/protorev/types"
 )
 
-// TestSendDeveloperFeesToDeveloperAccount tests the SendDeveloperFeesToDeveloperAccount function
-func (suite *KeeperTestSuite) TestSendDeveloperFeesToDeveloperAccount() {
+// pseudoExecuteTrade is a helper function to execute a trade given denom of profit, profit, and days since genesis
+func (suite *KeeperTestSuite) pseudoExecuteTrade(denom string, profit sdk.Int, daysSinceGenesis uint64) error {
+	// Initialize the number of days since genesis
+	suite.App.ProtoRevKeeper.SetDaysSinceModuleGenesis(suite.Ctx, daysSinceGenesis)
+	// Mint the profit to the module account (which will be sent to the developer account later)
+	err := suite.App.AppKeepers.BankKeeper.MintCoins(suite.Ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(denom, profit)))
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// TestSendDeveloperFee tests the SendDeveloperFee function
+func (suite *KeeperTestSuite) TestSendDeveloperFee() {
 	cases := []struct {
-		description   string
-		alterState    func()
-		expectedErr   bool
-		expectedCoins sdk.Coins
+		description       string
+		alterState        func()
+		expectedErr       bool
+		expectedDevProfit sdk.Coin
 	}{
 		{
-			description:   "Send with unset developer account",
-			alterState:    func() {},
-			expectedErr:   true,
-			expectedCoins: sdk.NewCoins(),
+			description:       "Send with unset developer account",
+			alterState:        func() {},
+			expectedErr:       true,
+			expectedDevProfit: sdk.Coin{},
 		},
 		{
-			description: "Send with set developer account",
+			description: "Send with set developer account in first phase",
 			alterState: func() {
 				account := apptesting.CreateRandomAccounts(1)[0]
 				suite.App.ProtoRevKeeper.SetDeveloperAccount(suite.Ctx, account)
 
-				err := suite.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(2000), 0)
+				err := suite.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(1000), 100)
 				suite.Require().NoError(err)
 			},
-			expectedErr:   false,
-			expectedCoins: sdk.NewCoins(sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(400))),
+			expectedErr:       false,
+			expectedDevProfit: sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(20)),
 		},
 		{
-			description: "Send with set developer account (after multiple trades)",
+			description: "Send with set developer account in second phase",
 			alterState: func() {
 				account := apptesting.CreateRandomAccounts(1)[0]
 				suite.App.ProtoRevKeeper.SetDeveloperAccount(suite.Ctx, account)
 
-				// Trade 1
-				err := suite.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(2000), 0)
-				suite.Require().NoError(err)
-
-				// Trade 2
-				err = suite.pseudoExecuteTrade("Atom", sdk.NewInt(2000), 0)
+				err := suite.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(1000), 500)
 				suite.Require().NoError(err)
 			},
-			expectedErr:   false,
-			expectedCoins: sdk.NewCoins(sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(400)), sdk.NewCoin("Atom", sdk.NewInt(400))),
+			expectedErr:       false,
+			expectedDevProfit: sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(10)),
 		},
 		{
-			description: "Send with set developer account (after multiple trades across epochs)",
+			description: "Send with set developer account in third (final) phase",
 			alterState: func() {
 				account := apptesting.CreateRandomAccounts(1)[0]
 				suite.App.ProtoRevKeeper.SetDeveloperAccount(suite.Ctx, account)
 
-				// Trade 1
-				err := suite.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(2000), 0)
-				suite.Require().NoError(err)
-
-				// Trade 2
-				err = suite.pseudoExecuteTrade("Atom", sdk.NewInt(2000), 0)
-				suite.Require().NoError(err)
-
-				// Trade 3 after year 1
-				err = suite.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(2000), 366)
-				suite.Require().NoError(err)
-
-				// Trade 4 after year 2
-				err = suite.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(2000), 366*2)
+				err := suite.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(1000), 1000)
 				suite.Require().NoError(err)
 			},
-			expectedErr:   false,
-			expectedCoins: sdk.NewCoins(sdk.NewCoin("Atom", sdk.NewInt(400)), sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(700))),
+			expectedErr:       false,
+			expectedDevProfit: sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(5)),
 		},
 	}
 
@@ -82,7 +77,7 @@ func (suite *KeeperTestSuite) TestSendDeveloperFeesToDeveloperAccount() {
 			suite.SetupTest()
 			tc.alterState()
 
-			err := suite.App.ProtoRevKeeper.SendDeveloperFeesToDeveloperAccount(suite.Ctx)
+			err := suite.App.ProtoRevKeeper.SendDeveloperFee(suite.Ctx, sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(100)))
 			if tc.expectedErr {
 				suite.Require().Error(err)
 			} else {
@@ -91,83 +86,11 @@ func (suite *KeeperTestSuite) TestSendDeveloperFeesToDeveloperAccount() {
 
 			developerAccount, err := suite.App.ProtoRevKeeper.GetDeveloperAccount(suite.Ctx)
 			if !tc.expectedErr {
-				developerFees := suite.App.AppKeepers.BankKeeper.GetAllBalances(suite.Ctx, developerAccount)
-				suite.Require().Equal(tc.expectedCoins, developerFees)
+				developerFee := suite.App.AppKeepers.BankKeeper.GetBalance(suite.Ctx, developerAccount, types.OsmosisDenomination)
+				suite.Require().Equal(tc.expectedDevProfit, developerFee)
 			} else {
 				suite.Require().Error(err)
 			}
 		})
 	}
 }
-
-// TestUpdateDeveloperFees tests the UpdateDeveloperFees function
-func (suite *KeeperTestSuite) TestUpdateDeveloperFees() {
-	cases := []struct {
-		description string
-		denom       string
-		profit      sdk.Int
-		alterState  func()
-		expected    sdk.Coin
-	}{
-		{
-			description: "Update developer fees in year 1",
-			denom:       types.OsmosisDenomination,
-			profit:      sdk.NewInt(200),
-			alterState:  func() {},
-			expected:    sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(40)),
-		},
-		{
-			description: "Update developer fees in year 2",
-			denom:       types.OsmosisDenomination,
-			profit:      sdk.NewInt(200),
-			alterState: func() {
-				suite.App.ProtoRevKeeper.SetDaysSinceModuleGenesis(suite.Ctx, 366)
-			},
-			expected: sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(20)),
-		},
-		{
-			description: "Update developer fees after year 2",
-			denom:       types.OsmosisDenomination,
-			profit:      sdk.NewInt(200),
-			alterState: func() {
-				suite.App.ProtoRevKeeper.SetDaysSinceModuleGenesis(suite.Ctx, 731)
-
-			},
-			expected: sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(10)),
-		},
-		{
-			description: "Update developer fees after year 10",
-			denom:       types.OsmosisDenomination,
-			profit:      sdk.NewInt(200),
-			alterState: func() {
-				suite.App.ProtoRevKeeper.SetDaysSinceModuleGenesis(suite.Ctx, 365*10+1)
-
-			},
-			expected: sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(10)),
-		},
-	}
-
-	for _, tc := range cases {
-		suite.Run(tc.description, func() {
-			suite.SetupTest()
-			tc.alterState()
-
-			err := suite.App.ProtoRevKeeper.UpdateDeveloperFees(suite.Ctx, tc.denom, tc.profit)
-			suite.Require().NoError(err)
-
-			developerFees, err := suite.App.ProtoRevKeeper.GetDeveloperFees(suite.Ctx, tc.denom)
-			suite.Require().NoError(err)
-			suite.Require().Equal(tc.expected, developerFees)
-		})
-	}
-}
-
-// pseudoExecuteTrade is a helper function to execute a trade given denom of profit, profit, and days since genesis
-func (suite *KeeperTestSuite) pseudoExecuteTrade(denom string, profit sdk.Int, daysSinceGenesis uint64) error {
-	// Initialize the number of days since genesis
-	suite.App.ProtoRevKeeper.SetDaysSinceModuleGenesis(suite.Ctx, daysSinceGenesis)
-	// Mint the profit to the module account (which will be sent to the developer account later)
-	suite.App.AppKeepers.BankKeeper.MintCoins(suite.Ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(denom, profit)))
-	// Update the developer fees
-	return suite.App.ProtoRevKeeper.UpdateDeveloperFees(suite.Ctx, denom, profit)
-}

x/protorev/keeper/epoch_hook.go

@@ -33,20 +33,16 @@ func (h EpochHooks) BeforeEpochStart(ctx sdk.Context, epochIdentifier string, ep
 func (h EpochHooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, epochNumber int64) error {
 	if h.k.GetProtoRevEnabled(ctx) {
 		switch epochIdentifier {
-		case "week":
-			// Distribute developer fees to the developer account. We do not error check because the developer account
-			// may not have been set by this point (gets set by the admin account after module genesis)
-			_ = h.k.SendDeveloperFeesToDeveloperAccount(ctx)
-
-			// Update the pools in the store
-			return h.k.UpdatePools(ctx)
 		case "day":
 			// Increment number of days since module genesis to properly calculate developer fees after cyclic arbitrage trades
 			if daysSinceGenesis, err := h.k.GetDaysSinceModuleGenesis(ctx); err != nil {
 				h.k.SetDaysSinceModuleGenesis(ctx, 1)
 			} else {
 				h.k.SetDaysSinceModuleGenesis(ctx, daysSinceGenesis+1)
 			}
+
+			// Update the pools in the store
+			return h.k.UpdatePools(ctx)
 		}
 	}
 

x/protorev/keeper/genesis.go

@@ -48,6 +48,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState) {
 		k.SetDeveloperAccount(ctx, account)
 	}
 
+	// TODO: Figure out if this can be removed in v16
 	// Set the developer fees that have been collected.
 	for _, fee := range genState.DeveloperFees {
 		if err := k.SetDeveloperFees(ctx, fee); err != nil {
@@ -101,6 +102,7 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
 	}
 	genesis.TokenPairArbRoutes = routes
 
+	// TODO: Figure out of this function needs to be in the v16 version of the module.
 	// Export the base denoms used for cyclic route building.
 	baseDenoms, err := k.GetAllBaseDenoms(ctx)
 	if err != nil {