feat(spike): taker fee #6034

czarcas7ic · 2023-08-14T00:16:50Z

Closes: #XXX

What is the purpose of the change

This feature adds taker fee logic to poolmanager when making a swap. It is important to note that all swap logic messages are routed through poolmanager. This means that if you call a swap method directly in GAMM or CL (and not via the message), taker fees will not be charged.

There are two txfees module accounts that taker fees accrue and get swapped in: An account for staking rewards and an account for community pool.

If the taker fee is in osmo:

the parameterized community pool percent goes directly to the community pool
the parameterized staking rewards percent goes to the txfees staking rewards module account, and is sent to the actual staking rewards module account at epoch.

If the taker fee is non osmo but a whitelisted quote asset:

the parameterized community pool percent goes directly to the community pool
the parameterized staking rewards percent goes to the txfees staking rewards module account, is swapped to OSMO at epoch, then sent to the actual staking rewards module account.

If the taker fee is non osmo and non whitelisted quote asset:

the parameterized community pool percent goes to the txfees community pool module account and is swapped to the parameterized asset (defaults to USDC) at epoch, then is sent to the actual community pool module account
the parameterized staking rewards percent goes to the txfees staking rewards module account, is swapped to OSMO at epoch, then sent to the actual staking rewards module account.

hooks.go, taker_fee.go, and router.go will be the ones to pay the most attention to.

Pre v19 Release:

Add all missing tests for hooks.go
Add all missing tests for taker_fee.go
Add all missing tests for upgrades.go
Add an E2E test for this. The CL test indirectly tested this properly, so we really just need a test that checks the module accounts and ensures the epoch swaps and distributes properly
In upgrade handler, set default taker fee to 0
Backport v18 upgrade handler to main

Post v19 Release:

Make issue to remove whitelisted quote denoms in CL module in v20
Have skip change the takerFee to non zero in their test suite so they can confirm the number changes

Testing and Verifying

TBD

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes?
Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

Specification (x/{module}/README.md)
Osmosis documentation site
Code comments?
N/A

czarcas7ic · 2023-08-17T20:34:09Z

devbot help

devbot-wizard · 2023-08-17T20:34:10Z

Hi! I'm DevBot, a bot that helps with common tasks in the development process. Commands:

devbot fix: Fix basic errors in the PR. (e.g. imports, changelog conflicts, go mod conflicts)
devbot merge base: Merge the base branch into the PR branch.
devbot add changelog [misc/feat/bug/api/sec] [message]: Add a changelog entry to the PR. (e.g. devbot add changelog feat Added a new feature)
- If message is blank, defaults to PR title.
devbot re-pr: Re-PR the PR, useful for external contributors where we have no edit perms.

czarcas7ic · 2023-08-17T20:34:27Z

devbot add changelog feat

ValarDragon · 2023-08-28T04:15:15Z

x/poolmanager/msg_server.go

+		),
+	})
+
+	return &types.MsgSetDenomPairTakerFeeResponse{Success: true}, nil


Why do we have a success field in response, should we just remove this?

(not returning an error => success, right)

Follows the same format as other messages with no important return values. If we decide this, we should remove it from all of them at a single time imo.

I'm unaware of us doing this convention. I at least think we shouldn't introduce the design pattern more, unless this is replicated in the SDK

ValarDragon · 2023-08-28T04:16:30Z

x/poolmanager/types/gov_test.go

+	}
+}
+
+func TestDenomPairTakerFeeProposal_ValidateBasic(t *testing.T) {


I'd like to come back to this later, where I feel like we can make some more general method to get these guarantees

x/poolmanager/types/msgs.go

ValarDragon · 2023-08-28T04:19:10Z

x/poolmanager/types/params.go

+		TakerFeeParams: TakerFeeParams{
+			DefaultTakerFee: sdk.MustNewDecFromStr("0.0015"), // 0.15%


I think we should be setting the precise param we want in the upgrade handler, and not have so many denoms in defaults

ValarDragon · 2023-08-28T04:22:58Z

x/poolmanager/types/params.go

+		AuthorizedQuoteDenoms: []string{
+			"uosmo",
+			"ibc/27394FB092D2ECCD56123C74F36E4C1F926001CEADA9CA97EA622B25F41E5EB2", // ATOM
+			"ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", // DAI
+			"ibc/D189335C6E4A68B513C10AB227BF1C1D38C746766278BA3EEB4FB14124F1D858", // USDC
+		},


(For testnets) we may want it s.t. if this list is blank, it allows anything.

That can be done in a mainnet non-breaking way, so can be done later.

ValarDragon · 2023-08-28T05:11:56Z

I propose we get pool-manager side in a state were proud of as one PR, and then txfees as second

x/poolmanager/taker_fee.go

AlpinYukseloglu

Core logic LGTM, nice work. Approving to get this base PR in, understanding that there are potential changes that will need to be done in follow up PRs as discussed on call

* add taker fee determination and extraction * remove osmo multi hop logic * route to both community pool and staking rewards * fix a handfull of tests * assign taker fee to pool at time of creation * pull taker fee direct from pool struct * fix more tests * fix tests, set back up osmo multi hop discount * correct taker fee extraction * regen mocks * abstract taker fee to pool manager (highest lvl) * fix extract cmd * update changelog * tidy * remove param that no longer exists * add extra params to genesis logic * add back osmo multihop tests * add comment * fix e2e keeping prints * fixes * more test fixes * remove prints * more naive approach to determining taker fee * fix e2e * re-enable disabled test * minor cleaning * simplify params * not nullable * clean up * add whitelist set message denom pair taker fees * use real addresses * add gov prop for denom pair taker fee update * clean * move logic to its own taker_fee.go file * add CLI for gov prop for denomPairTakerFee * add admin address denomPairTakerFee cli msg * clean and simplifications * use authorized quote denoms from poolmanger * remove stableswap taker fee * sim msg change * add test for CLI * msg server tests * add route_test test * add gov_test.go * add msgs_test.go * remove print line * change from v18 to v19 * Update upgrades.go * set default taker fee to zero in upgrade handler * conflicts * Update proto/osmosis/poolmanager/v1beta1/genesis.proto * Update proto/osmosis/poolmanager/v1beta1/genesis.proto * Update x/poolmanager/taker_fee.go * Generated protofile changes * rename extractTakerFeeAndDistribute to chargeTakerFee * clean up * comment * rename * godoc for NonNativeFeeCollectorForCommunityPoolName * baseDenom to defaultFeesDenom name change * fix upgrade handler * fix AfterEpochEnd order of denoms when getting pools; reduce code dupl; tests * fix track volume * add key separator for pool manager * add comment to denom pair route * add basic lexicographical key test * update chargeTakerFee godoc * set the default taker fee back to non zero for e2e * change PoolMangerGetParams API to GetAuthorizedQuoteDenoms (same for setters) * update txfees & poolmanager READMEs with takerFee * fix merge main * add comment to denom pair taker fee * de-dup taker fee param validation * Update x/poolmanager/types/msgs.go * Apply suggestions from code review * Update x/poolmanager/taker_fee.go * remove unneeded setup test * get pool creation free from previous * take non native out of name * undo param pull instead of default * use current pool creation fee * use default * e2e --------- Co-authored-by: devbot-wizard <141283918+devbot-wizard@users.noreply.github.com> Co-authored-by: Roman <roman@osmosis.team> Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: alpo <yukseloglua@berkeley.edu> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Dev Ojha <dojha@berkeley.edu> (cherry picked from commit 5c8fd80) # Conflicts: # CHANGELOG.md # tests/e2e/e2e_test.go # x/poolmanager/export_test.go # x/poolmanager/router.go # x/poolmanager/types/keys.go

* feat(spike): taker fee (#6034) * add taker fee determination and extraction * remove osmo multi hop logic * route to both community pool and staking rewards * fix a handfull of tests * assign taker fee to pool at time of creation * pull taker fee direct from pool struct * fix more tests * fix tests, set back up osmo multi hop discount * correct taker fee extraction * regen mocks * abstract taker fee to pool manager (highest lvl) * fix extract cmd * update changelog * tidy * remove param that no longer exists * add extra params to genesis logic * add back osmo multihop tests * add comment * fix e2e keeping prints * fixes * more test fixes * remove prints * more naive approach to determining taker fee * fix e2e * re-enable disabled test * minor cleaning * simplify params * not nullable * clean up * add whitelist set message denom pair taker fees * use real addresses * add gov prop for denom pair taker fee update * clean * move logic to its own taker_fee.go file * add CLI for gov prop for denomPairTakerFee * add admin address denomPairTakerFee cli msg * clean and simplifications * use authorized quote denoms from poolmanger * remove stableswap taker fee * sim msg change * add test for CLI * msg server tests * add route_test test * add gov_test.go * add msgs_test.go * remove print line * change from v18 to v19 * Update upgrades.go * set default taker fee to zero in upgrade handler * conflicts * Update proto/osmosis/poolmanager/v1beta1/genesis.proto * Update proto/osmosis/poolmanager/v1beta1/genesis.proto * Update x/poolmanager/taker_fee.go * Generated protofile changes * rename extractTakerFeeAndDistribute to chargeTakerFee * clean up * comment * rename * godoc for NonNativeFeeCollectorForCommunityPoolName * baseDenom to defaultFeesDenom name change * fix upgrade handler * fix AfterEpochEnd order of denoms when getting pools; reduce code dupl; tests * fix track volume * add key separator for pool manager * add comment to denom pair route * add basic lexicographical key test * update chargeTakerFee godoc * set the default taker fee back to non zero for e2e * change PoolMangerGetParams API to GetAuthorizedQuoteDenoms (same for setters) * update txfees & poolmanager READMEs with takerFee * fix merge main * add comment to denom pair taker fee * de-dup taker fee param validation * Update x/poolmanager/types/msgs.go * Apply suggestions from code review * Update x/poolmanager/taker_fee.go * remove unneeded setup test * get pool creation free from previous * take non native out of name * undo param pull instead of default * use current pool creation fee * use default * e2e --------- Co-authored-by: devbot-wizard <141283918+devbot-wizard@users.noreply.github.com> Co-authored-by: Roman <roman@osmosis.team> Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: alpo <yukseloglua@berkeley.edu> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Dev Ojha <dojha@berkeley.edu> (cherry picked from commit 5c8fd80) # Conflicts: # CHANGELOG.md # tests/e2e/e2e_test.go # x/poolmanager/export_test.go # x/poolmanager/router.go # x/poolmanager/types/keys.go * fix merge conflicts * change json * inc num heights to wait * Update .github/workflows/test.yml * test with v18 init image * removed old upgrade logic --------- Co-authored-by: Adam Tucker <adam@osmosis.team> Co-authored-by: Adam Tucker <adamleetucker@outlook.com> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>

* add taker fee determination and extraction * remove osmo multi hop logic * route to both community pool and staking rewards * fix a handfull of tests * assign taker fee to pool at time of creation * pull taker fee direct from pool struct * fix more tests * fix tests, set back up osmo multi hop discount * correct taker fee extraction * regen mocks * abstract taker fee to pool manager (highest lvl) * fix extract cmd * update changelog * tidy * remove param that no longer exists * add extra params to genesis logic * add back osmo multihop tests * add comment * fix e2e keeping prints * fixes * more test fixes * remove prints * more naive approach to determining taker fee * fix e2e * re-enable disabled test * minor cleaning * simplify params * not nullable * clean up * add whitelist set message denom pair taker fees * use real addresses * add gov prop for denom pair taker fee update * clean * move logic to its own taker_fee.go file * add CLI for gov prop for denomPairTakerFee * add admin address denomPairTakerFee cli msg * clean and simplifications * use authorized quote denoms from poolmanger * remove stableswap taker fee * sim msg change * add test for CLI * msg server tests * add route_test test * add gov_test.go * add msgs_test.go * remove print line * change from v18 to v19 * Update upgrades.go * set default taker fee to zero in upgrade handler * conflicts * Update proto/osmosis/poolmanager/v1beta1/genesis.proto * Update proto/osmosis/poolmanager/v1beta1/genesis.proto * Update x/poolmanager/taker_fee.go * Generated protofile changes * rename extractTakerFeeAndDistribute to chargeTakerFee * clean up * comment * rename * godoc for NonNativeFeeCollectorForCommunityPoolName * baseDenom to defaultFeesDenom name change * fix upgrade handler * fix AfterEpochEnd order of denoms when getting pools; reduce code dupl; tests * fix track volume * add key separator for pool manager * add comment to denom pair route * add basic lexicographical key test * update chargeTakerFee godoc * set the default taker fee back to non zero for e2e * change PoolMangerGetParams API to GetAuthorizedQuoteDenoms (same for setters) * update txfees & poolmanager READMEs with takerFee * fix merge main * add comment to denom pair taker fee * de-dup taker fee param validation * Update x/poolmanager/types/msgs.go * Apply suggestions from code review * Update x/poolmanager/taker_fee.go * remove unneeded setup test * get pool creation free from previous * take non native out of name * undo param pull instead of default * use current pool creation fee * use default * e2e --------- Co-authored-by: devbot-wizard <141283918+devbot-wizard@users.noreply.github.com> Co-authored-by: Roman <roman@osmosis.team> Co-authored-by: github-actions <github-actions@github.com> Co-authored-by: alpo <yukseloglua@berkeley.edu> Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Dev Ojha <dojha@berkeley.edu>

czarcas7ic added 3 commits August 11, 2023 10:15

add taker fee determination and extraction

4eb8594

remove osmo multi hop logic

12dccfa

route to both community pool and staking rewards

e857cc9

github-actions bot added C:x/txfees C:app-wiring Changes to the app folder C:x/poolmanager labels Aug 14, 2023

fix a handfull of tests

f773225

github-actions bot added the C:x/gamm Changes, features and bugs related to the gamm module. label Aug 14, 2023

assign taker fee to pool at time of creation

0e54b35

github-actions bot added the C:x/concentrated-liquidity label Aug 14, 2023

pull taker fee direct from pool struct

4d421fd

github-actions bot added the T:build label Aug 14, 2023

czarcas7ic added 6 commits August 14, 2023 14:53

Merge branch 'main' into adam/taker-fee-feat

8d1c8c6

fix more tests

4177b7a

fix tests, set back up osmo multi hop discount

49e6aff

correct taker fee extraction

e57f4a6

regen mocks

547232f

abstract taker fee to pool manager (highest lvl)

5ea549b

github-actions bot added the C:simulator Edits simulator or simulations label Aug 17, 2023

czarcas7ic added 2 commits August 16, 2023 20:59

Merge branch 'main' into adam/taker-fee-feat

d43a1e5

fix extract cmd

914cedc

github-actions bot added the C:x/superfluid label Aug 17, 2023

update changelog

5a68fa6

czarcas7ic added the V:state/breaking State machine breaking PR label Aug 17, 2023

czarcas7ic added 3 commits August 17, 2023 20:29

Merge branch 'main' into adam/taker-fee-feat

4312222

tidy

f0afc3d

remove param that no longer exists

a3863d5

ValarDragon reviewed Aug 28, 2023

View reviewed changes

x/poolmanager/types/msgs.go Outdated Show resolved Hide resolved

Update x/poolmanager/types/msgs.go

303bfb9

ValarDragon reviewed Aug 28, 2023

View reviewed changes

x/poolmanager/taker_fee.go Outdated Show resolved Hide resolved

ValarDragon reviewed Aug 28, 2023

View reviewed changes

x/poolmanager/taker_fee.go Show resolved Hide resolved

Apply suggestions from code review

e7b7a20

ValarDragon reviewed Aug 28, 2023

View reviewed changes

x/poolmanager/taker_fee.go Outdated Show resolved Hide resolved

Update x/poolmanager/taker_fee.go

d5e9123

AlpinYukseloglu approved these changes Aug 28, 2023

View reviewed changes

czarcas7ic added 7 commits August 28, 2023 10:01

remove unneeded setup test

708442e

get pool creation free from previous

759c310

take non native out of name

ecaa3f3

undo param pull instead of default

87e4310

use current pool creation fee

84c8873

use default

726532e

e2e

e48e614

github-actions bot added the T:CI label Aug 28, 2023

czarcas7ic merged commit 5c8fd80 into main Aug 28, 2023
1 check passed

czarcas7ic deleted the adam/taker-fee-feat branch August 28, 2023 17:11

mergify bot mentioned this pull request Aug 28, 2023

feat(spike): taker fee (backport #6034) #6212

Merged

p0mvn mentioned this pull request Sep 1, 2023

fix: e2e twap flakiness #6259

Closed

github-actions bot mentioned this pull request Mar 15, 2024

Broken Links Detected #7752

Closed

github-actions bot mentioned this pull request May 1, 2024

Broken Links Detected #8183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(spike): taker fee #6034

feat(spike): taker fee #6034

czarcas7ic commented Aug 14, 2023 •

edited

Loading

czarcas7ic commented Aug 17, 2023

devbot-wizard commented Aug 17, 2023

czarcas7ic commented Aug 17, 2023

ValarDragon Aug 28, 2023

czarcas7ic Aug 28, 2023

ValarDragon Aug 28, 2023

ValarDragon Aug 28, 2023

ValarDragon Aug 28, 2023

ValarDragon Aug 28, 2023

ValarDragon commented Aug 28, 2023

AlpinYukseloglu left a comment

		TakerFeeParams: TakerFeeParams{
		DefaultTakerFee: sdk.MustNewDecFromStr("0.0015"), // 0.15%

feat(spike): taker fee #6034

feat(spike): taker fee #6034

Conversation

czarcas7ic commented Aug 14, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

czarcas7ic commented Aug 17, 2023

devbot-wizard commented Aug 17, 2023

czarcas7ic commented Aug 17, 2023

ValarDragon Aug 28, 2023

Choose a reason for hiding this comment

czarcas7ic Aug 28, 2023

Choose a reason for hiding this comment

ValarDragon Aug 28, 2023

Choose a reason for hiding this comment

ValarDragon Aug 28, 2023

Choose a reason for hiding this comment

ValarDragon Aug 28, 2023

Choose a reason for hiding this comment

ValarDragon Aug 28, 2023

Choose a reason for hiding this comment

ValarDragon commented Aug 28, 2023

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

czarcas7ic commented Aug 14, 2023 •

edited

Loading