Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: taker fee performance refactor #7555

Merged
merged 18 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
51 changes: 26 additions & 25 deletions app/modules.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,31 +103,32 @@ 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.DeprecatedFeeCollectorForCommunityPoolName: 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.
Expand Down
15 changes: 7 additions & 8 deletions x/gamm/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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,
},
}

Expand Down Expand Up @@ -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{
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good news for other parts of disk size too haha

},
"invalid - two hops, denom does not exist": {
routes: []poolmanagertypes.SwapAmountOutRoute{
Expand Down
40 changes: 14 additions & 26 deletions x/poolmanager/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand All @@ -478,22 +478,17 @@ type TakerFeeParams struct {

Not shown here is a separate KVStore, which holds overrides for the defaultTakerFee.

There are also two module accounts involved:
The module account that holds all taker fees prior to distribution is named the `taker_fee_collector`. This used to be two separate module accounts, but was combined into one to simplify the distribution process.

```proto
non_native_fee_collector: osmo1g7ajkk295vactngp74shkfrprvjrdwn662dg26
non_native_fee_collector_community_pool: osmo1f3xhl0gqmyhnu49c8k3j7fkdv75ug0xjtaqu09
```

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.
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved

In the takerFee params, there are two distribution categories:
1. Taker fees generated in OSMO
Expand All @@ -508,31 +503,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.

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.
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.

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.
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 swapped to the `CommunityPoolDenomToSwapNonWhitelistedAssetsTo` defined in the `poolmanger` 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 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.
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved

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.
ValarDragon marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions x/poolmanager/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
Expand Down Expand Up @@ -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{},
Expand Down
11 changes: 0 additions & 11 deletions x/poolmanager/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Comment on lines -669 to -679
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer checked at time of swap, moved to epoch logic

// 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
Expand Down
Loading