refactor: ExitSwapExternAmountOut #1244

p0mvn · 2022-04-13T00:19:05Z

Closes: #1130

Description

Re-implemented ExitSwapExternAmountOut with similar logic to v7
introduced PoolExitSwapExternAmountOutExtension

Risks

I was confused about the following part:

Lines 533 to 543 in 523ed35

    
           // TODO: `balancer` contract sends the exit fee to the `factory` contract. 
        
           //       But, it is unclear that how the exit fees in the `factory` contract are handled. 
        
           //       And, it seems to be not good way to send the exit fee to the pool, 
        
           //       because the pool doesn't have the PoolAsset about exit fee. 
        
           //       So, temporarily, just burn the exit fee. 
        
           if exitFee.IsPositive() { 
        
           	err = k.BurnPoolShareFromAccount(ctx, pool, sender, exitFee) 
        
           	if err != nil { 
        
           		return sdk.Int{}, err 
        
           	} 
        
           }

I think that instead of doing that, it is possible to call k.applyExitPoolStateChange(...) so that it takes care of the fee since it does k.BurnPoolShareFromAccount(ctx, pool, exiter, numShares) where numShares do not have the exitFee subtracted.

It seems to me that in the old logic subtracting the fee only to burn it separately was redundant, and I'm not sure if I'm missing some algebra reasoning behind it.

Next Steps

I'm planning to spend some time writing more unit tests for this in a separate PR. Then, spearhead more e2e tests by joining a swap and exiting it using this method.

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

codecov-commenter · 2022-04-13T00:37:26Z

Codecov Report

Merging #1244 (685f439) into main (b6d342a) will decrease coverage by 0.75%.
The diff coverage is 9.32%.

@@            Coverage Diff             @@
##             main    #1244      +/-   ##
==========================================
- Coverage   20.90%   20.14%   -0.76%     
==========================================
  Files         196      203       +7     
  Lines       25425    26824    +1399     
==========================================
+ Hits         5316     5405      +89     
- Misses      19118    20412    +1294     
- Partials      991     1007      +16

Impacted Files	Coverage Δ
x/claim/abci.go	`0.00% <ø> (ø)`
x/claim/client/cli/query.go	`34.45% <ø> (ø)`
x/claim/client/cli/tx.go	`0.00% <ø> (ø)`
x/claim/handler.go	`0.00% <ø> (ø)`
x/claim/keeper/claim.go	`64.33% <ø> (ø)`
x/claim/keeper/grpc_query.go	`2.50% <ø> (ø)`
x/claim/keeper/hooks.go	`28.00% <ø> (ø)`
x/claim/keeper/keeper.go	`87.50% <ø> (ø)`
x/claim/keeper/params.go	`81.81% <ø> (ø)`
x/claim/module.go	`58.06% <ø> (ø)`
... and 106 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 1530e4a...685f439. Read the comment docs.

x/gamm/types/pool.go

alexanderbez · 2022-04-13T10:43:27Z

CHANGELOG.md

@@ -43,6 +43,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ### Features

+* [#1244](https://github.com/osmosis-labs/osmosis/pull/1244) refactor: ExitSwapExternAmountOut


Suggested change

* [#1244](https://github.com/osmosis-labs/osmosis/pull/1244) refactor: ExitSwapExternAmountOut

* [#1244](https://github.com/osmosis-labs/osmosis/pull/1244) Refactor `x/gamm`'s `ExitSwapExternAmountOut`.

x/gamm/types/pool.go

x/gamm/keeper/pool_service.go

alexanderbez · 2022-04-13T10:46:00Z

x/gamm/pool-models/balancer/amm.go

@@ -375,3 +375,62 @@ func (p *Pool) CalcExitPoolShares(ctx sdk.Context, exitingShares sdk.Int, exitFe
 	}
 	return exitedCoins, nil
 }
+
+// balancer notation: pAi - poolshares amount in, given single out


Suggested change

// balancer notation: pAi - poolshares amount in, given single out

// balancer notation: pAi - pool shares amount in, given single out

What is "single out"? I'm not sure how to read this godoc :P

Missed "asset" in "single out". Changed to:

// balancer notation: pAi - pool shares amount in, given single asset out // the second argument requires the tokenWeightOut / total token weight.

Also, there is a similar function for the converse:

osmosis/x/gamm/pool-models/balancer/amm.go

Lines 188 to 190 in b9af906

// balancer notation: pAo - poolshares amount out, given single asset in

// the second argument requires the tokenWeightIn / total token weight.

func calcPoolOutGivenSingleIn(

x/gamm/pool-models/balancer/amm.go

alexanderbez · 2022-04-13T10:49:16Z

x/gamm/pool-models/balancer/amm.go

+		p.GetSwapFee(ctx),
+		p.GetExitFee(ctx))
+
+	if poolAmountInBeforeFee.LTE(sdk.ZeroInt().ToDec()) {


Pretty sure there is a sdk.ZeroDec that we can use.

alexanderbez · 2022-04-13T10:50:08Z

x/gamm/pool-models/balancer/amm.go

+		return sdk.Int{}, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "token amount is zero or negative")
+	}
+
+	if poolAmountInBeforeFee.GT(shareInMaxAmount.ToDec()) {


I'm not super familiar with the balancer amm arithmetic yet, but why do we compare balances to shares here?

We should rename calcPoolOutGivenSingleIn to calcPoolSharesOutGivenSingleIn and the same for the variable, then this makes sense / is correct.

Thanks for catching this

Although renaming calcPoolOutGivenSingleIn to calcPoolSharesOutGivenSingleIn makes sebse, I'm assuming that you are referring to renaming calcPoolInGivenSingleOut to calcPoolSharesInGivenSingleAssetOut in this particular example of ExitSwapExternAmountOut?

x/gamm/keeper/pool_service.go

x/gamm/pool-models/balancer/balancer_pool.go

ValarDragon · 2022-04-14T22:26:25Z

x/gamm/pool-models/balancer/amm.go

+	if err != nil {
+		return sdk.Int{}, err
+	}
+


We need to update the total shares as well, ala

https://github.com/osmosis-labs/osmosis/pull/1244/files#diff-897e45c36167c79b080379402216acbe7c118711dcaa23ca7852cd2a70aca3c1R346

Maybe we refactor share updating + token balance updating for exits into a single re-used function between the two?

Good idea, I extracted this functionality from ExitPool into exitPool

Nice! That change LGTM

…haresInGivenSingleAssetOut

…nAmountOutCmd uncommented

x/gamm/keeper/pool_service.go

x/gamm/pool-models/balancer/amm.go

p0mvn requested review from ValarDragon, alexanderbez and mattverse April 13, 2022 00:38

p0mvn commented Apr 13, 2022

View reviewed changes

x/gamm/types/pool.go Outdated Show resolved Hide resolved

p0mvn marked this pull request as ready for review April 13, 2022 01:08

alexanderbez added the C:x/gamm Changes, features and bugs related to the gamm module. label Apr 13, 2022

alexanderbez reviewed Apr 13, 2022

View reviewed changes

ValarDragon reviewed Apr 13, 2022

View reviewed changes

x/gamm/keeper/pool_service.go Outdated Show resolved Hide resolved

ValarDragon reviewed Apr 13, 2022

View reviewed changes

x/gamm/pool-models/balancer/balancer_pool.go Outdated Show resolved Hide resolved

p0mvn added 4 commits April 14, 2022 10:23

refactor: ExitSwapExternAmountOut

3a505af

better name for interface extension

118bd85

implement types.PoolExitSwapExternAmountOutExtension

cedc684

update changelog

b9af906

p0mvn force-pushed the roman/1130 branch from b19257f to b9af906 Compare April 14, 2022 14:23

github-actions bot added the C:CLI label Apr 14, 2022

p0mvn added 12 commits April 14, 2022 10:24

interface assertion for PoolI in balancer pool

3cee732

err check on extendedPool.ExitSwapExternAmountOut

0426eec

ZeroDec()

5f3e4b9

comma in calcPoolInGivenSingleOut call

a8fd3eb

redundnet type in calcPoolInGivenSingleOut

c7f3728

comment for feeRatio

a3c2e58

fix comment for calcPoolInGivenSingleOut and the converse

10d2e1f

whitespace in ExitSwapExternAmountOut

b566003

fix changelog entry

955831d

godoc for PoolExitSwapExternAmountOutExtension

f2c428a

fmt

ebb8b96

gofumpt

135fc28

ValarDragon reviewed Apr 14, 2022

View reviewed changes

deduct shares on exit, make exit pool logic shared, improve names

0239280

p0mvn marked this pull request as draft April 15, 2022 16:06

p0mvn added 2 commits April 15, 2022 12:37

rename to ExitSwapExactAmountOutExtension

e8966c2

comment out TestNewExitSwapShareAmountInCmd

57d57ed

p0mvn force-pushed the roman/1130 branch from f4a892b to 57d57ed Compare April 15, 2022 16:41

p0mvn added 6 commits April 15, 2022 17:49

fix ExitSwapExactAmountOut by truncating int on return from calcPoolS…

a4ecefb

…haresInGivenSingleAssetOut

Merge branch 'main' into roman/1130

0ea012a

revert x/gamm/cli_test.go to original state with TestNewExitSwapExter…

84c257b

…nAmountOutCmd uncommented

uncomment ExitSwapExactAmountOut in TestActiveBalancerPool

191f9be

change variable name to exitingCoins

8a46f30

fmt

37c95cc

p0mvn marked this pull request as ready for review April 15, 2022 23:24

Update x/gamm/keeper/pool_service.go

795d50c

alexanderbez reviewed Apr 16, 2022

View reviewed changes

x/gamm/keeper/pool_service.go Outdated Show resolved Hide resolved

alexanderbez reviewed Apr 16, 2022

View reviewed changes

x/gamm/pool-models/balancer/amm.go Outdated Show resolved Hide resolved

alexanderbez approved these changes Apr 16, 2022

View reviewed changes

alexanderbez added 2 commits April 16, 2022 07:20

Update x/gamm/pool-models/balancer/amm.go

798c33d

Update x/gamm/keeper/pool_service.go

685f439

ValarDragon approved these changes Apr 18, 2022

View reviewed changes

ValarDragon merged commit 298ad00 into main Apr 18, 2022

ValarDragon deleted the roman/1130 branch April 18, 2022 17:25

faddat mentioned this pull request Apr 19, 2022

Remove claim module #1299

Closed

4 tasks

p0mvn mentioned this pull request Apr 21, 2022

refactor: JoinSwapShareAmountOut #1307

Merged

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

Broken Links Detected #7319

Closed

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

Broken Links Detected #8269

Closed

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

Broken Links Detected notional-labs/osmosis#76

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: ExitSwapExternAmountOut #1244

refactor: ExitSwapExternAmountOut #1244

p0mvn commented Apr 13, 2022 •

edited

Loading

codecov-commenter commented Apr 13, 2022 •

edited

Loading

alexanderbez Apr 13, 2022

alexanderbez Apr 13, 2022

alexanderbez Apr 13, 2022

p0mvn Apr 14, 2022

alexanderbez Apr 13, 2022

alexanderbez Apr 13, 2022

ValarDragon Apr 14, 2022 •

edited

Loading

p0mvn Apr 15, 2022

ValarDragon Apr 14, 2022

p0mvn Apr 15, 2022

ValarDragon Apr 18, 2022

	// TODO: `balancer` contract sends the exit fee to the `factory` contract.
	// But, it is unclear that how the exit fees in the `factory` contract are handled.
	// And, it seems to be not good way to send the exit fee to the pool,
	// because the pool doesn't have the PoolAsset about exit fee.
	// So, temporarily, just burn the exit fee.
	if exitFee.IsPositive() {
	err = k.BurnPoolShareFromAccount(ctx, pool, sender, exitFee)
	if err != nil {
	return sdk.Int{}, err
	}
	}

		@@ -43,6 +43,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

		### Features

		* [#1244](https://github.com/osmosis-labs/osmosis/pull/1244) refactor: ExitSwapExternAmountOut

	// balancer notation: pAi - poolshares amount in, given single out
	// balancer notation: pAi - pool shares amount in, given single out

	// balancer notation: pAo - poolshares amount out, given single asset in
	// the second argument requires the tokenWeightIn / total token weight.
	func calcPoolOutGivenSingleIn(

refactor: ExitSwapExternAmountOut #1244

refactor: ExitSwapExternAmountOut #1244

Conversation

p0mvn commented Apr 13, 2022 • edited Loading

Description

Risks

Next Steps

codecov-commenter commented Apr 13, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Apr 13, 2022 •

edited

Loading

codecov-commenter commented Apr 13, 2022 •

edited

Loading

ValarDragon Apr 14, 2022 •

edited

Loading