remove exit fee param #4440

hieuvubk · 2023-02-27T10:07:05Z

Closes: #4371

What is the purpose of the change

Refer from #4371 , we should remove all exit fee params & remove pools with exit fee

Brief Changelog

Removed ExitFee in every related funcs & tests
Created old proto types for migration
Migrate old pool => new, remove pool which has exit fee != 0
Upgrade handler
Testing

Testing and Verifying

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added unit test that validates ...
Added integration tests for end-to-end deployment with ...
Extended integration test for ...
Manually verified the change by ...

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

p0mvn · 2023-03-06T01:13:39Z

tests/e2e/scripts/pool1A.json

@@ -2,6 +2,5 @@
    "weights": "5ibc/ED07A3391A112B175915CD8FAF43A2DA8E4790EDE12566649D0C2F97716B8518,5uosmo",
    "initial-deposit": "499404ibc/ED07A3391A112B175915CD8FAF43A2DA8E4790EDE12566649D0C2F97716B8518,500000uosmo",
    "swap-fee": "0.01",
-    "exit-fee": "0.01",


e2e is failing because this is removed.

Basically, it tries to create a pool for pre-upgrade Osmosis (v14) that still has and expects this parameter.

I suggest copying this file and naming it pool1A-legacy.json and using it in:

osmosis/tests/e2e/configurer/upgrade.go

Line 128 in ae97cec

config.PreUpgradePoolId = chainANode.CreateBalancerPool("pool1A.json", initialization.ValidatorWalletName)

Also, please do the same for pool1B.json. Please create an issue to track the removal of these legacy files later

p0mvn · 2023-03-11T03:56:29Z

proto/osmosis/gamm/pool-models/balancer/balancerPool.proto

-  string exit_fee = 2 [
-    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Dec",
-    (gogoproto.moretags) = "yaml:\"exit_fee\"",
-    (gogoproto.nullable) = false
-  ];
-  SmoothWeightChangeParams smooth_weight_change_params = 3 [


I think we should be able to make this field simply as reserved. That way, we would not need to generate v2 types and perform risky and complex migrations.

:wq d

p0mvn · 2023-03-20T19:23:07Z

Hi @hieuvubk . Any updates on this?

hieuvubk · 2023-03-21T06:51:16Z

Hi @hieuvubk . Any updates on this?

Hi @p0mvn . I reverted to use reserved. Its work with unit test but still got e2e err cause some poolId that we want to remove was not existed on testing

p0mvn · 2023-03-24T02:18:35Z

Hi @hieuvubk . Any updates on this?

Hi @p0mvn . I reverted to use reserved. Its work with unit test but still got e2e err cause some poolId that we want to remove was not existed on testing

Thanks for the update.

There are some known issues with e2e right now and the upgrade hander code gen in #4575

I'm going to work on resolving them this week. Let's wait until that PR is in, once it is, I can help with looking into e2e here.

p0mvn · 2023-03-25T21:28:25Z

x/gamm/keeper/grpc_query.go

 	totalSharesAmount := pool.GetTotalShares()
 	if req.ShareInAmount.GTE(totalSharesAmount) || req.ShareInAmount.LTE(sdk.ZeroInt()) {
 		return nil, sdkerrors.Wrapf(types.ErrInvalidMathApprox, "share ratio is zero or negative")
 	}

-	exitCoins, err := pool.CalcExitPoolCoinsFromShares(sdkCtx, req.ShareInAmount, exitFee)
+	exitCoins, err := pool.CalcExitPoolCoinsFromShares(sdkCtx, req.ShareInAmount, sdk.ZeroDec())


Why do we provide sdk.ZeroDec() as an argument? Let's just remove exit fee from the API?

p0mvn · 2023-03-25T21:28:56Z

app/upgrades/v15/upgrades.go

+		//  N.B.: this is done to avoid initializing genesis for gamm module.
+		// Otherwise, it would overwrite migrations with InitGenesis().
+		// See RunMigrations() for details.
+		fromVM[gammtypes.ModuleName] = 0


This is only needed for newly added modules

Suggested change

// N.B.: this is done to avoid initializing genesis for gamm module.

// Otherwise, it would overwrite migrations with InitGenesis().

// See RunMigrations() for details.

fromVM[gammtypes.ModuleName] = 0

p0mvn · 2023-03-25T21:30:58Z

app/upgrades/v15/upgrades.go

+
+func removeExitFee(ctx sdk.Context, gammKeeper gammkeeper.Keeper, poolsWithExitFee []uint64) {
+	for _, poolId := range poolsWithExitFee {
+		err := gammKeeper.DeletePool(ctx, poolId)


Why are we deleting these pools? The pools should stay, only the exit fee setting is removed

p0mvn · 2023-03-26T03:12:32Z

Hi @hieuvubk . Any updates on this?

Hi @p0mvn . I reverted to use reserved. Its work with unit test but still got e2e err cause some poolId that we want to remove was not existed on testing

Thanks for the update.

There are some known issues with e2e right now and the upgrade hander code gen in #4575

I'm going to work on resolving them this week. Let's wait until that PR is in, once it is, I can help with looking into e2e here.

The known e2e issues are now resolved in: #4575

Please try rebasing and addressing my earlier comments. Please let me know if there are issues still after

hieuvubk · 2023-03-27T10:05:50Z

@p0mvn Hi Roman i addressed change above, e2e passed now. Thank u

p0mvn · 2023-03-31T03:12:09Z

Discussed offline. Closing in favor of: #4767

Thanks @hieuvubk

remove exit fee param

f84fce3

remove exit-fee in e2e

eac9f6f

github-actions bot added the T:build label Feb 27, 2023

hieuvubk added 2 commits February 28, 2023 16:09

fix proto

12583ff

create old proto for upgrade

7e8f7c0

p0mvn reviewed Mar 6, 2023

View reviewed changes

hieuvubk added 10 commits March 9, 2023 21:00

update proto for old types

bc5de98

codec

536f69a

implement poolI interface

f0c4d6a

convert from old => new types

a1f0370

migrate store

1a90863

upgrade handler

9483832

test

d88c9c6

fix e2e

22dbceb

lint

3148b6c

fix test: change hex string for new types

ea88524

p0mvn added A:backport/v15.x backport patches to v15.x branch V:state/breaking State machine breaking PR and removed A:backport/v15.x backport patches to v15.x branch labels Mar 11, 2023

p0mvn reviewed Mar 11, 2023

View reviewed changes

try to use reversed

267c0f8

hieuvubk added 8 commits March 13, 2023 15:59

testing

c956971

set up test

59d74c9

testing

441196f

testing

3a05a49

test 3 pools

8455809

test passed

0bf6f3f

:wq d

fix proto

11d4f34

Merge branch 'main' into remove_exit_fee

2db6074

hieuvubk and others added 3 commits March 21, 2023 12:14

merge main

96780d9

format

a15654a

Update go.mod

b040201

p0mvn reviewed Mar 25, 2023

View reviewed changes

p0mvn mentioned this pull request Mar 26, 2023

[EPIC]: CosmWasm Pool Type #4389

Closed

hieuvubk added 3 commits March 27, 2023 15:21

remove all exitfee

456f19f

merge main

292d853

fix upgrade handler

464cde3

p0mvn mentioned this pull request Mar 30, 2023

refactor: remove GetTotalShares, GetTotalLiquidity and GetExitFee from PoolI #4801

Merged

p0mvn closed this Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove exit fee param #4440

remove exit fee param #4440

hieuvubk commented Feb 27, 2023 •

edited

Loading

p0mvn Mar 6, 2023

p0mvn Mar 11, 2023

p0mvn commented Mar 20, 2023

hieuvubk commented Mar 21, 2023

p0mvn commented Mar 24, 2023

p0mvn Mar 25, 2023

p0mvn Mar 25, 2023

p0mvn Mar 25, 2023

p0mvn commented Mar 26, 2023

hieuvubk commented Mar 27, 2023

p0mvn commented Mar 31, 2023

remove exit fee param #4440

remove exit fee param #4440

Conversation

hieuvubk commented Feb 27, 2023 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

p0mvn Mar 6, 2023

Choose a reason for hiding this comment

p0mvn Mar 11, 2023

Choose a reason for hiding this comment

p0mvn commented Mar 20, 2023

hieuvubk commented Mar 21, 2023

p0mvn commented Mar 24, 2023

p0mvn Mar 25, 2023

Choose a reason for hiding this comment

p0mvn Mar 25, 2023

Choose a reason for hiding this comment

p0mvn Mar 25, 2023

Choose a reason for hiding this comment

p0mvn commented Mar 26, 2023

hieuvubk commented Mar 27, 2023

p0mvn commented Mar 31, 2023

hieuvubk commented Feb 27, 2023 •

edited

Loading