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

Conversation

czarcas7ic
Copy link
Member

@czarcas7ic czarcas7ic commented Feb 19, 2024

Closes: #7519

What is the purpose of the change

There were three glaring issues with our taker fee implementation:

  1. At time of swap, we sent funds to separate module accounts for taker fees going to stakers and taker fees going to community pool, requiring multiple bank sends per swap instead of just one
  2. The txfees module account was shared with taker fees going to stakers, making it confusing to on-board to every time
  3. We updated the protocol revenue tracker after every swap, when we really only need to do this once per epoch

This PR fixes these issues, drastically reducing the performance overhead that taker fees currently have on Osmosis.

@czarcas7ic czarcas7ic added the V:state/breaking State machine breaking PR label Feb 19, 2024
@github-actions github-actions bot added C:x/gamm Changes, features and bugs related to the gamm module. C:x/txfees C:app-wiring Changes to the app folder C:x/poolmanager labels Feb 19, 2024
@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Feb 19, 2024
Comment on lines -648 to -679
// 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
}

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

Comment on lines -160 to -172
expectedTakerFeeToStakers := []sdk.Coin{sdk.NewCoin(tc.expectedResult.Denom, roundup(expectedTakerFeeToStakersAmount))}
expectedTakerFeeToCommunityPool := []sdk.Coin{sdk.NewCoin(tc.expectedResult.Denom, expectedTakerFeeToCommunityPoolAmount.TruncateInt())}

// Validate results.
s.Require().Equal(tc.expectedResult.String(), tokenInAfterTakerFee.String())
expectedTakerFeeTrackerForStakersAfter := []sdk.Coin{}
if !expectedTakerFeeToStakers[0].IsZero() {
expectedTakerFeeTrackerForStakersAfter = expectedTakerFeeToStakers
}
s.Require().Equal(expectedTakerFeeTrackerForStakersAfter, takerFeeTrackerForStakersAfter)
expectedTakerFeeTrackerForCommunityPoolAfter := []sdk.Coin{}
if !expectedTakerFeeToCommunityPool[0].IsZero() {
expectedTakerFeeTrackerForCommunityPoolAfter = expectedTakerFeeToCommunityPool
Copy link
Member Author

Choose a reason for hiding this comment

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

The tracker is no longer updated at fee charge time, so this does not need to be tested here.

x/txfees/keeper/hooks.go Outdated Show resolved Hide resolved
Comment on lines +459 to +463
s.Require().Len(communityPoolBalanceDelta, 4)
s.Require().Equal(communityPoolBalanceDelta[0].Denom, otherPreSwapDenom)
s.Require().Equal(communityPoolBalanceDelta[1].Denom, denomWithNoPool)
s.Require().Equal(communityPoolBalanceDelta[2].Denom, preSwapDenom)
s.Require().Equal(communityPoolBalanceDelta[3].Denom, communityPoolDenom)
Copy link
Member Author

Choose a reason for hiding this comment

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

All these denoms are whitelisted quote assets, so they are direct sends to the community pool. The denomWithNoProtorevLink is not whitelisted, so it stays in the communityPoolCollectorAddress.

@czarcas7ic czarcas7ic marked this pull request as ready for review February 20, 2024 00:57
@@ -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

non_native_fee_collector: osmo1g7ajkk295vactngp74shkfrprvjrdwn662dg26
non_native_fee_collector_community_pool: osmo1f3xhl0gqmyhnu49c8k3j7fkdv75ug0xjtaqu09
```
- Non native taker fees
Copy link
Member

Choose a reason for hiding this comment

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

Good job on docs, this made this much easier to understand!

// FeeCollectorName the module account name for the fee collector account address.
FeeCollectorName = "fee_collector"
// NonNativeTxFeeCollectorName is the module account name for the alt fee collector account address (used for auto-swapping non-OSMO tx fees).
NonNativeTxFeeCollectorName = "non_native_fee_collector"
Copy link
Member

Choose a reason for hiding this comment

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

SLightly worried that this used to be FeeCollectorForStakingRewardsName and what happens if there are leftover funds in the addr

Copy link
Member Author

@czarcas7ic czarcas7ic Feb 20, 2024

Choose a reason for hiding this comment

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

Not sure I am understanding, @ValarDragon the address is exactly the same, I just changed the reference name of it because it was confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh I understand now, nice job

Copy link
Member

Choose a reason for hiding this comment

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

great job on comments inthis file

@ValarDragon
Copy link
Member

ValarDragon commented Feb 22, 2024

Nice job on this! This did definitely take me awhile to review, and I think its because we mixed two things:

  • A number of logic moves
  • A number of good variable renames
  • Some slight logic changes

If we find review time become painful for things like this, maybe a good idea for us to separate out renames + logic changes next time. I felt like I had to reconsider all the logic for some of the simple rename things due to git diff inadequacy

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

This LGTM, though I think we should do a second pass on this code in a second PR and simplify the hooks.go code

@czarcas7ic
Copy link
Member Author

Sound good, sorry about that, will try and split PRs like this up more in the future.

@czarcas7ic czarcas7ic merged commit b6a2155 into main Feb 23, 2024
1 check passed
@czarcas7ic czarcas7ic deleted the adam/taker-fee-performance-refactor branch February 23, 2024 20:33
@github-actions github-actions bot mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:docs Improvements or additions to documentation C:x/gamm Changes, features and bugs related to the gamm module. C:x/poolmanager C:x/txfees V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Make TakerFee revenues send once per epoch
2 participants