feat: taker fee performance refactor

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
+* [#7555](https://github.com/osmosis-labs/osmosis/pull/7555) Refactor taker fees, distribute via a single module account, track once at epoch
 
 ## v23.0.0
 

app/modules.go

@@ -103,31 +103,33 @@ import (
 // moduleAccountPermissions defines module account permissions
 // TODO: Having to input nil's here is unacceptable, we need a way to automatically derive this.
 var moduleAccountPermissions = map[string][]string{
-	authtypes.FeeCollectorName:                    nil,
-	distrtypes.ModuleName:                         nil,
-	ibchookstypes.ModuleName:                      nil,
-	icatypes.ModuleName:                           nil,
-	icqtypes.ModuleName:                           nil,
-	minttypes.ModuleName:                          {authtypes.Minter, authtypes.Burner},
-	minttypes.DeveloperVestingModuleAcctName:      nil,
-	stakingtypes.BondedPoolName:                   {authtypes.Burner, authtypes.Staking},
-	stakingtypes.NotBondedPoolName:                {authtypes.Burner, authtypes.Staking},
-	govtypes.ModuleName:                           {authtypes.Burner},
-	ibctransfertypes.ModuleName:                   {authtypes.Minter, authtypes.Burner},
-	gammtypes.ModuleName:                          {authtypes.Minter, authtypes.Burner},
-	incentivestypes.ModuleName:                    {authtypes.Minter, authtypes.Burner},
-	protorevtypes.ModuleName:                      {authtypes.Minter, authtypes.Burner},
-	lockuptypes.ModuleName:                        {authtypes.Minter, authtypes.Burner},
-	poolincentivestypes.ModuleName:                nil,
-	superfluidtypes.ModuleName:                    {authtypes.Minter, authtypes.Burner},
-	txfeestypes.ModuleName:                        nil,
-	txfeestypes.FeeCollectorForStakingRewardsName: nil,
-	txfeestypes.FeeCollectorForCommunityPoolName:  nil,
-	wasmtypes.ModuleName:                          {authtypes.Burner},
-	tokenfactorytypes.ModuleName:                  {authtypes.Minter, authtypes.Burner},
-	valsetpreftypes.ModuleName:                    {authtypes.Staking},
-	poolmanagertypes.ModuleName:                   nil,
-	cosmwasmpooltypes.ModuleName:                  nil,
+	authtypes.FeeCollectorName:               nil,
+	distrtypes.ModuleName:                    nil,
+	ibchookstypes.ModuleName:                 nil,
+	icatypes.ModuleName:                      nil,
+	icqtypes.ModuleName:                      nil,
+	minttypes.ModuleName:                     {authtypes.Minter, authtypes.Burner},
+	minttypes.DeveloperVestingModuleAcctName: nil,
+	stakingtypes.BondedPoolName:              {authtypes.Burner, authtypes.Staking},
+	stakingtypes.NotBondedPoolName:           {authtypes.Burner, authtypes.Staking},
+	govtypes.ModuleName:                      {authtypes.Burner},
+	ibctransfertypes.ModuleName:              {authtypes.Minter, authtypes.Burner},
+	gammtypes.ModuleName:                     {authtypes.Minter, authtypes.Burner},
+	incentivestypes.ModuleName:               {authtypes.Minter, authtypes.Burner},
+	protorevtypes.ModuleName:                 {authtypes.Minter, authtypes.Burner},
+	lockuptypes.ModuleName:                   {authtypes.Minter, authtypes.Burner},
+	poolincentivestypes.ModuleName:           nil,
+	superfluidtypes.ModuleName:               {authtypes.Minter, authtypes.Burner},
+	txfeestypes.ModuleName:                   nil,
+	txfeestypes.NonNativeTxFeeCollectorName:  nil,
+	txfeestypes.TakerFeeStakersName:          nil,
+	txfeestypes.TakerFeeCommunityPoolName:    nil,
+	txfeestypes.TakerFeeCollectorName:        nil,
+	wasmtypes.ModuleName:                     {authtypes.Burner},
+	tokenfactorytypes.ModuleName:             {authtypes.Minter, authtypes.Burner},
+	valsetpreftypes.ModuleName:               {authtypes.Staking},
+	poolmanagertypes.ModuleName:              nil,
+	cosmwasmpooltypes.ModuleName:             nil,
 }
 
 // appModules return modules to initialize module manager.

x/gamm/keeper/msg_server_test.go

@@ -47,7 +47,7 @@ func (s *KeeperTestSuite) TestSwapExactAmountIn_Events() {
 			tokenIn:               sdk.NewCoin("foo", osmomath.NewInt(tokenIn)),
 			tokenOutMinAmount:     osmomath.NewInt(tokenInMinAmount),
 			expectedSwapEvents:    1,
-			expectedMessageEvents: 4, // 1 gamm + 3 events emitted by other keeper methods.
+			expectedMessageEvents: 3, // 1 gamm + 2 events emitted by other keeper methods.
 		},
 		"two hops": {
 			routes: []poolmanagertypes.SwapAmountInRoute{
@@ -63,7 +63,7 @@ func (s *KeeperTestSuite) TestSwapExactAmountIn_Events() {
 			tokenIn:               sdk.NewCoin("foo", osmomath.NewInt(tokenIn)),
 			tokenOutMinAmount:     osmomath.NewInt(tokenInMinAmount),
 			expectedSwapEvents:    2,
-			expectedMessageEvents: 8, // 1 gamm + 7 events emitted by other keeper methods.
+			expectedMessageEvents: 6, // 1 gamm + 5 events emitted by other keeper methods.
 		},
 		"invalid - two hops, denom does not exist": {
 			routes: []poolmanagertypes.SwapAmountInRoute{
@@ -76,10 +76,9 @@ func (s *KeeperTestSuite) TestSwapExactAmountIn_Events() {
 					TokenOutDenom: "baz",
 				},
 			},
-			tokenIn:               sdk.NewCoin(doesNotExistDenom, osmomath.NewInt(tokenIn)),
-			tokenOutMinAmount:     osmomath.NewInt(tokenInMinAmount),
-			expectedMessageEvents: 1,
-			expectError:           true,
+			tokenIn:           sdk.NewCoin(doesNotExistDenom, osmomath.NewInt(tokenIn)),
+			tokenOutMinAmount: osmomath.NewInt(tokenInMinAmount),
+			expectError:       true,
 		},
 	}
 
@@ -151,7 +150,7 @@ func (s *KeeperTestSuite) TestSwapExactAmountOut_Events() {
 			tokenOut:              sdk.NewCoin("foo", osmomath.NewInt(tokenOut)),
 			tokenInMaxAmount:      osmomath.NewInt(tokenInMaxAmount),
 			expectedSwapEvents:    1,
-			expectedMessageEvents: 4, // 1 gamm + 3 events emitted by other keeper methods.
+			expectedMessageEvents: 3, // 1 gamm + 2 events emitted by other keeper methods.
 		},
 		"two hops": {
 			routes: []poolmanagertypes.SwapAmountOutRoute{
@@ -167,7 +166,7 @@ func (s *KeeperTestSuite) TestSwapExactAmountOut_Events() {
 			tokenOut:              sdk.NewCoin("foo", osmomath.NewInt(tokenOut)),
 			tokenInMaxAmount:      osmomath.NewInt(tokenInMaxAmount),
 			expectedSwapEvents:    2,
-			expectedMessageEvents: 8, // 1 gamm + 7 events emitted by other keeper methods.
+			expectedMessageEvents: 6, // 1 gamm + 5 events emitted by other keeper methods.
 		},
 		"invalid - two hops, denom does not exist": {
 			routes: []poolmanagertypes.SwapAmountOutRoute{

x/poolmanager/README.md

@@ -460,7 +460,7 @@ Here is the following process for the `EstimateTradeBasedOnPriceImpactConcentrat
 
 9. If a viable trade amount is found, the function performs a final estimation of `tokenOut` considering the swap fee and returns the estimated trade.
 
-## Take Fee
+## Taker Fees
 
 Taker fee distribution is defined in the poolmanager module’s param store:
 
@@ -478,22 +478,25 @@ type TakerFeeParams struct {
 
 Not shown here is a separate KVStore, which holds overrides for the defaultTakerFee.
 
-There are also two module accounts involved:
+At time of swap, all taker fees are sent to the `taker_fee_collector` module account. At epoch:
 
-```proto
-non_native_fee_collector: osmo1g7ajkk295vactngp74shkfrprvjrdwn662dg26
-non_native_fee_collector_community_pool: osmo1f3xhl0gqmyhnu49c8k3j7fkdv75ug0xjtaqu09
-```
+- Non native taker fees
+    - For Community Pool: Sent to `non_native_fee_collector_community_pool` module account, swapped to `CommunityPoolDenomToSwapNonWhitelistedAssetsTo`, then sent to community pool
+    - For Stakers: Sent to `non_native_fee_collector_stakers` module account, swapped to OSMO, then sent to auth module account, which distributes it to stakers
+    - The sub-module accounts here are used so that, if a swap fails, the tokens that fail to swap are not grouped back into the wrong taker fee category in the next epoch
+- OSMO taker fees
+    - For Community Pool: Sent directly to community pool
+    - For Stakers: Sent directly to auth module account, which distributes it to stakers
 
-Lets go through the lifecycle to better understand how taker fee works in a variety of situations, and how each of these parameters and module accounts are used.
+Lets go through the lifecycle to better understand how taker fee works in a variety of situations, and how the module account and distribution parameters are used depending on the input token.
 
 ### Example 1: Non OSMO taker fee
 
 A user makes a swap of USDC to OSMO. First, the protocol checks the KVStore to determine if the the denom pair has a taker fee override. If the pair exists in the KVStore, the taker fee override is used. If the pair does not exist, the defaultTakerFee is used.
 
 In this example, defaultTakerFee is 0.02%. A USDC<>OSMO KVStore exists with an override of 0.01%. Therefore, 0.01% is used.
 
-Now, imagine the amount in is 1000 USDC. This means that the amount of takerFee utilized is 0.01% of 1000, which is 1 USDC. 
+Now, imagine the amount in is 1000 USDC. This means that the amount of takerFee utilized is 0.01% of 1000, which is 1 USDC.
 
 In the takerFee params, there are two distribution categories:
 1. Taker fees generated in OSMO
@@ -508,31 +511,24 @@ type TakerFeeDistributionPercentage struct {
 }
 ```
 
-For simplicity sake, let’s say staking rewards is 40% and community pool is 60%. This means that out of the 1 USDC taken, 0.4 USDC is meant for staking rewards and 0.6 USDC is meant for community pool.
+For simplicity sake, let’s say staking rewards are 40% and community pool is 60%. This means that out of the 1 USDC taken, 0.4 USDC is meant for staking rewards and 0.6 USDC is meant for community pool.
+
+At time of swap, all 1 USDC is sent to the `taker_fee_collector` module account. Nothing is done with any taker fee funds until epoch.
 
-Starting with the community pool funds, the protocol checks if the fee is a whitelisted fee token. If it is, it is sent directly to the community pool. If it is not, it is sent to the `non_native_fee_collector_community_pool` module address. At epoch, the funds in this account are swapped to the `CommunityPoolDenomToSwapNonWhitelistedAssetsTo` defined in the `poolmanger` params above, and then sent all at once to the community pool at that time.
+Starting with the community pool funds, at epoch, the protocol checks if the token is a whitelisted fee token. If it is, it is sent directly to the community pool. If it is not, the funds are sent to the `non_native_fee_collector_community_pool`, swapped to the `CommunityPoolDenomToSwapNonWhitelistedAssetsTo` defined in the `poolmanager` params above, and then sent all at once to the community pool after all swaps at epoch have taken place.
 
-Next, for staking rewards, since this is a non-OSMO token, it is sent directly to the `non_native_fee_collector`. At epoch, all funds in this module account are swapped to OSMO and distributed directly to OSMO stakers.
+Next, for staking rewards, since this is a non-OSMO token, it is swapped to OSMO and sent to the auth module account, which distributes it to stakers.
 
 ### Example 2: OSMO taker fee
 
 This example does not differ much from the previous example. In this example, a user is swapping 1000 OSMO for USDC.
 
-Just as before, we search for a KVStore taker fee override before utilizing the default taker fee. Just as before (order does not matter), a KVStore entry for OSMO<>USDC exists, so we utilize a 0.01% taker fee instead of the 0.02% default taker fee. 0.01% of 1000 OSMO is 1 OSMO.
-
-We now check the `OsmoTakerFeeDistribution`. In this example, let’s say its 20% to community pool and 80% to stakers. This means that 0.2 OSMO is set for community pool and 0.8 is set for stakers.
-
-For community pool, this is just a direct send to community pool.
+We search for a KVStore taker fee override before utilizing the default taker fee. Just as before (order does not matter), a KVStore entry for OSMO<>USDC exists, so we utilize a 0.01% taker fee instead of the 0.02% default taker fee. 0.01% of 1000 OSMO is 1 OSMO.
 
-For staking, we actually ALSO send this to the `non_native_fee_collector`. At epoch time, this OSMO is just skipped over, while everything else is swapped to OSMO. At the very end, it takes the OSMO directly sent to the `non_native_fee_collector` along with the non native tokens that were just swapped to OSMO and distributes it to stakers.
+At time of swap, all 1 OSMO is sent to the `taker_fee_collector` module account. Again, nothing is done with any taker fee funds until epoch.
 
-### Important Note: How to extract the data
+At epoch, we check the `OsmoTakerFeeDistribution`. In this example, let’s say its 20% to community pool and 80% to stakers. This means that 0.2 OSMO is set for community pool and 0.8 is set for stakers.
 
-If one were to take the total amount of tokens in the two module accounts (`non_native_fee_collector` and `non_native_fee_collector_community_pool`), this would be slightly over exaggerating the amount that is generated from taker fees. This is because, when a user uses a non-native token as a FEE TOKEN, this is also sent to the `non_native_fee_collector`. So there are two options here to extract the info:
+For community pool, this is just a direct send of the OSMO to the community pool.
 
-1. Track the delta of the module accounts 1 block after epoch X and 1 block before epoch X+1. Also, track the total non osmo txfees generated in this period. Subtract the total non osmo txfees generated in this period from the delta of the `non_native_fee_collector`. Add this to the delta of `non_native_fee_collector_community_pool` values. This is the taker fees generated
-2. This is the less better way, but you can track the `SendCoinsFromAccountToModule` events from each block. The problem with this is, imagine I swap USDC to OSMO and use USDC as txfee. This would generate three `SendCoinsFromAccountToModule` events:
-   1. Txfee gets sent to `non_native_fee_collector`
-   2. Part of taker fee gets sent to `non_native_fee_collector`
-   3. Other part of taker fee gets sent to `non_native_fee_collector_community_pool`
-   You could make an assumption here that the txfee is going to be the smaller of the two that gets sent to the `non_native_fee_collector`, or better the order of operations is going to always be the same, so you can figure out if the first or second send to `non_native_fee_collector` is the txfee and not track that value
+For staking, the OSMO is directly sent to the auth module account, which distributes it to stakers.

x/poolmanager/msg_server_test.go

@@ -49,7 +49,7 @@ func (s *KeeperTestSuite) TestSplitRouteSwapExactAmountIn() {
 			tokenoutMinAmount: min_amount,
 
 			expectedSplitRouteSwapEvent: 1,
-			expectedMessageEvents:       15, // 4 pool creation + 11 events in SplitRouteExactAmountIn keeper methods
+			expectedMessageEvents:       12, // 4 pool creation + 8 events in SplitRouteExactAmountIn keeper methods
 		},
 		"error: empty route": {
 			routes:            []types.SwapAmountInSplitRoute{},
@@ -139,7 +139,7 @@ func (s *KeeperTestSuite) TestSplitRouteSwapExactAmountOut() {
 			tokenoutMaxAmount: max_amount,
 
 			expectedSplitRouteSwapEvent: 1,
-			expectedMessageEvents:       16, // 4 pool creation + 12 events in SplitRouteExactAmountOut keeper methods
+			expectedMessageEvents:       12, // 4 pool creation + 8 events in SplitRouteExactAmountOut keeper methods
 		},
 		"error: empty route": {
 			routes:            []types.SwapAmountOutSplitRoute{},

x/poolmanager/router.go

@@ -645,17 +645,6 @@ func (k Keeper) TotalLiquidity(ctx sdk.Context) (sdk.Coins, error) {
 	return totalLiquidity, nil
 }
 
-// isDenomWhitelisted checks if the denom provided exists in the list of authorized quote denoms.
-// If it does, it returns true, otherwise false.
-func isDenomWhitelisted(denom string, authorizedQuoteDenoms []string) bool {
-	for _, authorizedQuoteDenom := range authorizedQuoteDenoms {
-		if denom == authorizedQuoteDenom {
-			return true
-		}
-	}
-	return false
-}
-
 // nolint: unused
 // trackVolume converts the input token into OSMO units and adds it to the global tracked volume for the given pool ID.
 // Fails quietly if an OSMO paired pool cannot be found, although this should only happen in rare scenarios where OSMO is