-
Notifications
You must be signed in to change notification settings - Fork 608
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
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
9265936
refactor taker fees for performance
czarcas7ic 9c89b04
Merge branch 'main' into adam/taker-fee-performance-refactor
czarcas7ic f84f908
add changelog
czarcas7ic 60d2075
remove unusued func
czarcas7ic 2265841
update taker fee readme
czarcas7ic 3735cbe
keep comm pool funds separate in case of failed swap/distribution
czarcas7ic 5a3ab08
clarify README even more
czarcas7ic d0a7adb
add taker fee staker collector module account
czarcas7ic f64b9b0
clean up
czarcas7ic b3da3a8
Update x/poolmanager/README.md
czarcas7ic bb4d747
Update x/poolmanager/README.md
czarcas7ic c384206
Update x/poolmanager/README.md
czarcas7ic d9e7d6b
Update x/txfees/types/keys.go
czarcas7ic 19a0c45
Update x/poolmanager/README.md
czarcas7ic cf51e75
add comment about epoch coming before staking
czarcas7ic c929e7f
Merge branch 'main' into adam/taker-fee-performance-refactor
czarcas7ic 4304da2
Merge branch 'main' into adam/taker-fee-performance-refactor
czarcas7ic 0ba21a1
abstract telementary
czarcas7ic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good job on docs, this made this much easier to understand! |
||
- 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. | ||
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 | ||
|
@@ -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 `poolmanger` params above, and then sent all at once to the community pool after all swaps at epoch have taken place. | ||
czarcas7ic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. | ||
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
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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