Protorev: Update timing for pool updating and dev fee payout (#4827)

* Update highest liquidity pools in daily epoch instead of weekly * Minimal code change to send dev profit after every trade - Not optimal because it still stores state, but want to show minimal working version - All tests pass after setting dev account for the tests, no new tests added * Handles dev fee payment without storing in kvstore - removes functions no longer necessary - Still needs upgrade migration work - Still needs a way to pay current dev fee payment to us during the upgrade - still needs to get tests to pass * Increase consensus version * Revert "Handles dev fee payment without storing in kvstore" This reverts commit 1f2f3dc. * add upgrade logic, new functions, and deprecated comments * add another todo * Revert "add another todo" This reverts commit 49c7eaf. * add another todo * Panic on migration error * Hardcode protorev from version to 1 - When testing via the upgrades_test.go file, the protorev migration would not run since it treated the from and to version to be 2 (pulling from the new consensus version set) - When adding this line and hardcoding the from version to 1, the upgrade processes as expected * Add tests to ensure protorev upgrade is successful * Remove deprecated comment to pass linter * lint * add changelog entry * Change documentation to reflect new timing cadence * Change developer_fees test for new SendDeveloperFee method - Removes tests for no longer used functions - Adds a test for the new SendDeveloperFee method * Update app/upgrades/v16/upgrades.go fix typo Co-authored-by: David Terpay <[email protected]> * move test to top of file, helper at bottom * fix merge conflict typo * added migration function to upgrades.go * bump consensus version to 2 * comment out everything upgrade related to isolate problem * Revert "comment out everything upgrade related to isolate problem" This reverts commit da4efca. * Revert "bump consensus version to 2" This reverts commit 127dc5b. * log instead of pass back up errors * Remove commented out code * clean up comments * add clarifying comment * fix typpo Co-authored-by: Roman <[email protected]> * update v15 prev version tag in e2e container * remove e2e test that checks protorev dev account not initialized * return err if protorev dev account payment errors during upgrade * Add dev fee payment check when testing trade execution --------- Co-authored-by: David Terpay <[email protected]> Co-authored-by: stackman27 <[email protected]> Co-authored-by: Roman <[email protected]>
osmosis-labs · Jun 6, 2023 · 7ceceaf · 7ceceaf
1 parent 81f474b
commit 7ceceaf
Show file tree

Hide file tree

Showing 11 changed files with 152 additions and 135 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -50,6 +50,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
 

diff --git a/app/upgrades/v16/upgrades.go b/app/upgrades/v16/upgrades.go
@@ -128,6 +128,11 @@ func CreateUpgradeHandler(
 
 		updateTokenFactoryParams(ctx, keepers.TokenFactoryKeeper)
 
+		// Transfers out all the dev fees in kvstore to dev account during upgrade
+		if err := keepers.ProtoRevKeeper.SendDeveloperFeesToDeveloperAccount(ctx); err != nil {
+			return nil, err
+		}
+
 		return migrations, nil
 	}
 }

diff --git a/app/upgrades/v16/upgrades_test.go b/app/upgrades/v16/upgrades_test.go
@@ -16,6 +16,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 {
@@ -50,6 +51,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 {
@@ -115,6 +120,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() {
 				// Validate that tokenfactory params have been updated
@@ -149,3 +157,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{})
+}
diff --git a/x/protorev/keeper/developer_fees.go b/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
+}
diff --git a/x/protorev/keeper/developer_fees_test.go b/x/protorev/keeper/developer_fees_test.go
@@ -7,168 +7,90 @@ import (
 	"github.com/osmosis-labs/osmosis/v15/x/protorev/types"
 )
 
-// TestSendDeveloperFeesToDeveloperAccount tests the SendDeveloperFeesToDeveloperAccount function
-func (s *KeeperTestSuite) TestSendDeveloperFeesToDeveloperAccount() {
+// 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]
-				s.App.ProtoRevKeeper.SetDeveloperAccount(s.Ctx, account)
+				suite.App.ProtoRevKeeper.SetDeveloperAccount(suite.Ctx, account)
 
-				err := s.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(2000), 0)
-				s.Require().NoError(err)
+				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]
-				s.App.ProtoRevKeeper.SetDeveloperAccount(s.Ctx, account)
+				suite.App.ProtoRevKeeper.SetDeveloperAccount(suite.Ctx, account)
 
-				// Trade 1
-				err := s.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(2000), 0)
-				s.Require().NoError(err)
-
-				// Trade 2
-				err = s.pseudoExecuteTrade("Atom", sdk.NewInt(2000), 0)
-				s.Require().NoError(err)
+				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]
-				s.App.ProtoRevKeeper.SetDeveloperAccount(s.Ctx, account)
-
-				// Trade 1
-				err := s.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(2000), 0)
-				s.Require().NoError(err)
-
-				// Trade 2
-				err = s.pseudoExecuteTrade("Atom", sdk.NewInt(2000), 0)
-				s.Require().NoError(err)
-
-				// Trade 3 after year 1
-				err = s.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(2000), 366)
-				s.Require().NoError(err)
+				suite.App.ProtoRevKeeper.SetDeveloperAccount(suite.Ctx, account)
 
-				// Trade 4 after year 2
-				err = s.pseudoExecuteTrade(types.OsmosisDenomination, sdk.NewInt(2000), 366*2)
-				s.Require().NoError(err)
+				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)),
 		},
 	}
 
 	for _, tc := range cases {
-		s.Run(tc.description, func() {
-			s.SetupTest()
+		suite.Run(tc.description, func() {
+			suite.SetupTest()
 			tc.alterState()
 
-			err := s.App.ProtoRevKeeper.SendDeveloperFeesToDeveloperAccount(s.Ctx)
+			err := suite.App.ProtoRevKeeper.SendDeveloperFee(suite.Ctx, sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(100)))
 			if tc.expectedErr {
-				s.Require().Error(err)
+				suite.Require().Error(err)
 			} else {
-				s.Require().NoError(err)
+				suite.Require().NoError(err)
 			}
 
-			developerAccount, err := s.App.ProtoRevKeeper.GetDeveloperAccount(s.Ctx)
+			developerAccount, err := suite.App.ProtoRevKeeper.GetDeveloperAccount(suite.Ctx)
 			if !tc.expectedErr {
-				developerFees := s.App.AppKeepers.BankKeeper.GetAllBalances(s.Ctx, developerAccount)
-				s.Require().Equal(tc.expectedCoins, developerFees)
+				developerFee := suite.App.AppKeepers.BankKeeper.GetBalance(suite.Ctx, developerAccount, types.OsmosisDenomination)
+				suite.Require().Equal(tc.expectedDevProfit, developerFee)
 			} else {
-				s.Require().Error(err)
+				suite.Require().Error(err)
 			}
 		})
 	}
 }
 
-// TestUpdateDeveloperFees tests the UpdateDeveloperFees function
-func (s *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() {
-				s.App.ProtoRevKeeper.SetDaysSinceModuleGenesis(s.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() {
-				s.App.ProtoRevKeeper.SetDaysSinceModuleGenesis(s.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() {
-				s.App.ProtoRevKeeper.SetDaysSinceModuleGenesis(s.Ctx, 365*10+1)
-			},
-			expected: sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(10)),
-		},
-	}
-
-	for _, tc := range cases {
-		s.Run(tc.description, func() {
-			s.SetupTest()
-			tc.alterState()
-
-			err := s.App.ProtoRevKeeper.UpdateDeveloperFees(s.Ctx, tc.denom, tc.profit)
-			s.Require().NoError(err)
-
-			developerFees, err := s.App.ProtoRevKeeper.GetDeveloperFees(s.Ctx, tc.denom)
-			s.Require().NoError(err)
-			s.Require().Equal(tc.expected, developerFees)
-		})
-	}
-}
-
 // pseudoExecuteTrade is a helper function to execute a trade given denom of profit, profit, and days since genesis
-func (s *KeeperTestSuite) pseudoExecuteTrade(denom string, profit sdk.Int, daysSinceGenesis uint64) error {
+func (suite *KeeperTestSuite) pseudoExecuteTrade(denom string, profit sdk.Int, daysSinceGenesis uint64) error {
 	// Initialize the number of days since genesis
-	s.App.ProtoRevKeeper.SetDaysSinceModuleGenesis(s.Ctx, daysSinceGenesis)
+	suite.App.ProtoRevKeeper.SetDaysSinceModuleGenesis(suite.Ctx, daysSinceGenesis)
 	// Mint the profit to the module account (which will be sent to the developer account later)
-	err := s.App.AppKeepers.BankKeeper.MintCoins(s.Ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(denom, profit)))
+	err := suite.App.AppKeepers.BankKeeper.MintCoins(suite.Ctx, types.ModuleName, sdk.NewCoins(sdk.NewCoin(denom, profit)))
 	if err != nil {
 		return err
 	}
-	// Update the developer fees
-	return s.App.ProtoRevKeeper.UpdateDeveloperFees(s.Ctx, denom, profit)
+
+	return nil
 }
diff --git a/x/protorev/keeper/epoch_hook.go b/x/protorev/keeper/epoch_hook.go
@@ -31,20 +31,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)
 		}
 	}
 

diff --git a/x/protorev/keeper/posthandler_test.go b/x/protorev/keeper/posthandler_test.go
@@ -12,6 +12,7 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
 
+	"github.com/osmosis-labs/osmosis/v15/app/apptesting"
 	gammtypes "github.com/osmosis-labs/osmosis/v15/x/gamm/types"
 	poolmanagertypes "github.com/osmosis-labs/osmosis/v15/x/poolmanager/types"
 	"github.com/osmosis-labs/osmosis/v15/x/protorev/keeper"
@@ -90,6 +91,10 @@ func (s *KeeperTestSuite) TestAnteHandle() {
 	acc1 := s.App.AccountKeeper.NewAccountWithAddress(s.Ctx, addr0)
 	s.App.AccountKeeper.SetAccount(s.Ctx, acc1)
 
+	// Set protorev developer account
+	devAccount := apptesting.CreateRandomAccounts(1)[0]
+	s.App.ProtoRevKeeper.SetDeveloperAccount(s.Ctx, devAccount)
+
 	// Keep testing order consistent to make adding tests easier
 	// Add all tests that are not expected to execute a trade first
 	// Then track number of trades and profits for the rest of the tests