Auto-swap non-osmo tx fees to osmo #1145

AlpinYukseloglu · 2022-03-25T04:51:07Z

Closes: #1121

Description

Moved DeductFeeDecorator from auth -> txfees module
Created second module account in txfees module to receive non-OSMO tx fees
Implemented logic to route non-OSMO fees to second module account
Set up AfterEpochEnd listener to trigger fee swaps into base denom + transfer to the main module account
Did all the necessary wiring in app & txfees module (hook-related + misc.)
Set up testing for all major changes/additions (primarily in feedecorator_test.go and hooks_test.go)

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

…ccount, fee routing logic, epoch hooks etc.

codecov-commenter · 2022-03-25T05:11:05Z

Codecov Report

Merging #1145 (f341152) into main (0b3e383) will decrease coverage by 0.73%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #1145      +/-   ##
==========================================
- Coverage   20.26%   19.53%   -0.74%     
==========================================
  Files         203      200       -3     
  Lines       26824    27532     +708     
==========================================
- Hits         5436     5377      -59     
- Misses      20378    21153     +775     
+ Partials     1010     1002       -8

Impacted Files	Coverage Δ
x/txfees/keeper/feedecorator.go	`60.82% <46.93%> (-14.18%)`	⬇️
x/txfees/keeper/hooks.go	`72.72% <72.72%> (ø)`
x/txfees/keeper/keeper.go	`92.85% <100.00%> (+5.35%)`	⬆️
x/claim/genesis.go
x/claim/keeper/grpc_query.go
x/claim/handler.go
x/claim/client/cli/tx.go
x/claim/keeper/params.go
x/claim/module.go
x/claim/keeper/claim.go
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b3e383...f341152. Read the comment docs.

x/txfees/keeper/hooks.go

alexanderbez

Nice work! Just left some initial minor feedback

app/ante.go

x/txfees/keeper/feedecorator.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

into auto-fee-swaps

app/keepers.go

x/txfees/keeper/hooks.go

into auto-fee-swaps

app/apptesting/test_suite.go

app/keepers.go

x/txfees/keeper/feedecorator.go

x/txfees/keeper/feedecorator_test.go

x/txfees/keeper/hooks.go

…enom loop

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

x/txfees/keeper/hooks.go

alexanderbez · 2022-04-20T14:02:29Z

@mattverse wanna give this one more look?

mattverse

Left some minor comments!

Also, would it be possible to add documnetation in spec before we merge this?

x/txfees/types/expected_keepers.go

mattverse · 2022-04-21T04:59:29Z

x/txfees/keeper/keeper.go


 func NewKeeper(
 	cdc codec.Codec,
+	accountKeeper types.AccountKeeper,
+	bankKeeper *bankkeeper.BaseKeeper,


reminder: Should use bank keeper from expected_keepers, instead of using it in a direct import form!

mattverse · 2022-04-21T05:04:50Z

x/txfees/keeper/hooks_test.go

+	coins := sdk.NewCoins(sdk.NewInt64Coin(uion, 10),
+		sdk.NewInt64Coin(atom, 20),
+		sdk.NewInt64Coin(ust, 14))
+
+	swapFee := sdk.NewDec(0)
+
+	expectedOutput1, err := uionPool.CalcOutAmtGivenIn(suite.Ctx,
+		sdk.Coins{sdk.Coin{Denom: uion, Amount: coins.AmountOf(uion)}},
+		baseDenom,
+		swapFee)
+	suite.Require().NoError(err)
+	expectedOutput2, err := atomPool.CalcOutAmtGivenIn(suite.Ctx,
+		sdk.Coins{sdk.Coin{Denom: atom, Amount: coins.AmountOf(atom)}},
+		baseDenom,
+		swapFee)
+	suite.Require().NoError(err)
+	expectedOutput3, err := ustPool.CalcOutAmtGivenIn(suite.Ctx,
+		sdk.Coins{sdk.Coin{Denom: ust, Amount: coins.AmountOf(ust)}},
+		baseDenom,
+		swapFee)
+	suite.Require().NoError(err)


nit: would it be possible to change these to foo, bar, baz? I personally don't prefer using token names directly.

I actually think this is better than foo bar baz

mattverse · 2022-04-21T06:03:06Z

x/txfees/keeper/hooks_test.go

+	suite.App.EpochsKeeper.AfterEpochEnd(futureCtx, params.DistrEpochIdentifier, int64(1))
+
+	suite.Require().Empty(suite.App.BankKeeper.GetAllBalances(suite.Ctx, moduleAddrNonNativeFee))
+	suite.Require().True(suite.App.BankKeeper.GetBalance(suite.Ctx, moduleAddrFee, baseDenom).Amount.GTE(fullExpectedOutput.Amount.TruncateInt()))


Can we change this to testing w/ precise number instead of using GTE?

x/txfees/keeper/feedecorator.go

x/txfees/keeper/feedecorator_test.go

ValarDragon · 2022-04-21T14:13:55Z

Because this is a legacy PR thats been open so long, I'm in favor of merging it, and doing spec updates in a second issue/PR,

Right now theres not really a well formatted spec for this module to begin with, and I don't think its worth blocking the merge which has several useful updates, and will keep getting merge conflicts, until the spec is done

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

AlpinYukseloglu added 9 commits March 24, 2022 09:10

migrate deductfeesdecorator to txfees module & set up second module a…

98eed0b

…ccount, fee routing logic, epoch hooks etc.

add second module account to supported module accounts in app

bb1857c

set up txFeesKeeper with new keeper interface in app

88b4401

add hook receiver to epoch hooks for txfees module

2b7f32f

set up AfterEpochEnd hook to swap all non-OSMO fees to OSMO

0becf2a

clean up feedecorator comments

ad8bd35

add relevant keepers to TxFeeKeeper struct

c2c848e

revert accidental file changes due to branch mixup

6467af6

revert accidental file changes due to branch mixup

e5d46e1

AlpinYukseloglu added 2 commits March 25, 2022 10:26

add rudimentary swapping logic

cfae2f0

clean up comments and spacing

528edfd

AlpinYukseloglu commented Mar 25, 2022

View reviewed changes

x/txfees/keeper/hooks.go Outdated Show resolved Hide resolved

AlpinYukseloglu added 4 commits March 25, 2022 11:38

add gammkeeper and move swap calls under it

9db3006

comments for intended feedecorator tests

8850485

fix swap parameters to get correct pool id and minimum output

26c3316

set up feegrantkeeper in suite app for testing

3f40623

alexanderbez requested changes Mar 27, 2022

View reviewed changes

AlpinYukseloglu and others added 8 commits March 27, 2022 16:35

Update x/txfees/keeper/feedecorator.go

2d1ceb2

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/txfees/keeper/feedecorator.go

081d9a7

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/txfees/keeper/feedecorator.go

aa5775e

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/txfees/keeper/feedecorator.go

184b939

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/txfees/keeper/feedecorator.go

4bce525

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Apply suggestions from code review

5032ca1

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Merge branch 'auto-fee-swaps' of https://github.com/osmosis-labs/osmosis

9719f75

into auto-fee-swaps

update comments

b9026d1

alexanderbez reviewed Mar 28, 2022

View reviewed changes

app/keepers.go Outdated Show resolved Hide resolved

app/keepers.go Outdated Show resolved Hide resolved

AlpinYukseloglu and others added 3 commits March 28, 2022 12:10

attempt to migrate test from legacytx to txbuilder

349bc70

updates

d8fbd81

update txbuilder setup

8002532

ValarDragon and others added 3 commits April 15, 2022 16:20

Fix formatting

709bceb

Not sure why make format didn't catch this

05b3fbc

import fixes

3d8e7dc

ValarDragon reviewed Apr 15, 2022

View reviewed changes

x/txfees/keeper/hooks.go Outdated Show resolved Hide resolved

ValarDragon reviewed Apr 15, 2022

View reviewed changes

x/txfees/keeper/hooks.go Outdated Show resolved Hide resolved

ValarDragon and others added 4 commits April 15, 2022 16:37

Delete fee grant keeper

2b8140d

Merge branch 'auto-fee-swaps' of https://github.com/osmosis-labs/osmosis

dbc7cb1

into auto-fee-swaps

gerge branch 'auto-fee-swaps' of https://github.com/osmosis-labs/osmosis

c152510

into auto-fee-swaps

fix tests to accommodate removal of fee grant keeper

faf48a5

alexanderbez reviewed Apr 18, 2022

View reviewed changes

AlpinYukseloglu and others added 2 commits April 18, 2022 08:33

remove panic errors at epoch end and remove potential for unbounded d…

ab34ca3

…enom loop

Apply suggestions from code review

366da35

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

AlpinYukseloglu mentioned this pull request Apr 18, 2022

Shorten receiver name in apptesting test suite #1280

Closed

Update epoch code + DRY some of the test

84ab533

alexanderbez approved these changes Apr 19, 2022

View reviewed changes

x/txfees/keeper/hooks.go Show resolved Hide resolved

x/txfees/keeper/hooks.go Show resolved Hide resolved

ValarDragon approved these changes Apr 20, 2022

View reviewed changes

mattverse reviewed Apr 21, 2022

View reviewed changes

ValarDragon and others added 5 commits April 21, 2022 09:15

Merge branch 'main' into auto-fee-swaps

08c4a8f

formatting

f3c0eee

formatting

85d215b

Fix build & keeper import

2c22499

Apply suggestions from code review

f341152

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>

ValarDragon added the A:automerge label Apr 22, 2022

ValarDragon requested a review from mattverse April 22, 2022 04:43

ValarDragon merged commit a33c3c2 into main Apr 22, 2022

ValarDragon deleted the auto-fee-swaps branch April 22, 2022 04:53

ValarDragon mentioned this pull request Apr 22, 2022

Update txfees module README / spec for autoswappig update #1322

Closed

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

Broken Links Detected #8269

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-swap non-osmo tx fees to osmo #1145

Auto-swap non-osmo tx fees to osmo #1145

AlpinYukseloglu commented Mar 25, 2022 •

edited

codecov-commenter commented Mar 25, 2022 •

edited

alexanderbez left a comment

alexanderbez commented Apr 20, 2022

mattverse left a comment

mattverse Apr 21, 2022

mattverse Apr 21, 2022

ValarDragon Apr 21, 2022

mattverse Apr 21, 2022

ValarDragon commented Apr 21, 2022

Auto-swap non-osmo tx fees to osmo #1145

Auto-swap non-osmo tx fees to osmo #1145

Conversation

AlpinYukseloglu commented Mar 25, 2022 • edited

Description

codecov-commenter commented Mar 25, 2022 • edited

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez commented Apr 20, 2022

mattverse left a comment

Choose a reason for hiding this comment

mattverse Apr 21, 2022

Choose a reason for hiding this comment

mattverse Apr 21, 2022

Choose a reason for hiding this comment

ValarDragon Apr 21, 2022

Choose a reason for hiding this comment

mattverse Apr 21, 2022

Choose a reason for hiding this comment

ValarDragon commented Apr 21, 2022

AlpinYukseloglu commented Mar 25, 2022 •

edited

codecov-commenter commented Mar 25, 2022 •

edited