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.

Sign up for GitHub

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

x/gamm: Make all internal set functions private #2013

Merged
merged 10 commits into from
Jul 18, 2022
Merged

x/gamm: Make all internal set functions private #2013

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
457874b
make SetParams private
AlpinYukseloglu Jul 9, 2022
6cc13dd
make liquidity tracker set fxs private
AlpinYukseloglu Jul 9, 2022
1c563fc
Merge branch 'main' into make-sets-private
AlpinYukseloglu Jul 16, 2022
0cd1fac
comment out problematic upgrades and make SetPool private
AlpinYukseloglu Jul 17, 2022
f14e943
Merge branch 'make-sets-private' of https://github.com/osmosis-labs/o…
AlpinYukseloglu Jul 17, 2022
f9422fd
fix broken test
AlpinYukseloglu Jul 17, 2022
4e4f1ef
update comments for clarity
AlpinYukseloglu Jul 17, 2022
fcb13ac
add changelog entry
AlpinYukseloglu Jul 18, 2022
a3a8fe9
Update CHANGELOG.md
ValarDragon Jul 18, 2022
7ddc493
Merge branch 'main' into make-sets-private
mergify[bot] Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions app/apptesting/test_suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,16 +174,11 @@ func (s *KeeperTestHelper) AllocateRewardsToValidator(valAddr sdk.ValAddress, re

// SetupGammPoolsWithBondDenomMultiplier uses given multipliers to set initial pool supply of bond denom.
func (s *KeeperTestHelper) SetupGammPoolsWithBondDenomMultiplier(multipliers []sdk.Dec) []gammtypes.PoolI {
s.App.GAMMKeeper.SetParams(s.Ctx, gammtypes.Params{
PoolCreationFee: sdk.Coins{},
})

bondDenom := s.App.StakingKeeper.BondDenom(s.Ctx)
// TODO: use sdk crypto instead of tendermint to generate address
acc1 := sdk.AccAddress(ed25519.GenPrivKey().PubKey().Address().Bytes())

poolCreationFee := s.App.GAMMKeeper.GetParams(s.Ctx)
s.FundAcc(acc1, poolCreationFee.PoolCreationFee)
params := s.App.GAMMKeeper.GetParams(s.Ctx)
Comment on lines -177 to +181
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Context:

The use of SetParams here was unnecessary and was the only thing prevent us from making the function private, so it makes sense to remove it

The rest of the changes here fix the test, which was broken to begin with but was never caught because SetParams would always set the pool creation fee to zero

Copy link
Member

Choose a reason for hiding this comment

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

Ah nice, this is a good fix!


pools := []gammtypes.PoolI{}
for index, multiplier := range multipliers {
Expand All @@ -193,7 +188,7 @@ func (s *KeeperTestHelper) SetupGammPoolsWithBondDenomMultiplier(multipliers []s
s.FundAcc(acc1, sdk.NewCoins(
sdk.NewCoin(bondDenom, uosmoAmount.Mul(sdk.NewInt(10))),
sdk.NewInt64Coin(token, 100000),
))
).Add(params.PoolCreationFee...))

var (
defaultFutureGovernor = ""
Expand Down
13 changes: 8 additions & 5 deletions app/upgrades/v4/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ import (
"testing"
"time"

"github.com/osmosis-labs/osmosis/v7/app"
v4 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v4"
"github.com/stretchr/testify/suite"
abci "github.com/tendermint/tendermint/abci/types"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"

"github.com/osmosis-labs/osmosis/v7/app"
v4 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v4"

sdk "github.com/cosmos/cosmos-sdk/types"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
)
Expand Down Expand Up @@ -107,9 +108,11 @@ func (suite *UpgradeTestSuite) TestUpgradePayments() {
suite.Require().Equal(feePool.GetCommunityPool(), sdk.NewDecCoins(sdk.NewInt64DecCoin("uosmo", expectedBal)))

// Check that gamm Minimum Fee has been set correctly
gammParams := suite.app.GAMMKeeper.GetParams(suite.ctx)
expectedCreationFee := sdk.NewCoins(sdk.NewCoin("uosmo", sdk.OneInt()))
suite.Require().Equal(gammParams.PoolCreationFee, expectedCreationFee)

// Kept as comments for recordkeeping. Since SetParams is now private, the changes being tested for can no longer be made:
// gammParams := suite.app.GAMMKeeper.GetParams(suite.ctx)
// expectedCreationFee := sdk.NewCoins(sdk.NewCoin("uosmo", sdk.OneInt()))
// suite.Require().Equal(gammParams.PoolCreationFee, expectedCreationFee)
},
true,
},
Expand Down
5 changes: 2 additions & 3 deletions app/upgrades/v4/upgrades.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (

"github.com/osmosis-labs/osmosis/v7/app/keepers"
"github.com/osmosis-labs/osmosis/v7/app/upgrades"
gammtypes "github.com/osmosis-labs/osmosis/v7/x/gamm/types"
)

// CreateUpgradeHandler returns an x/upgrade handler for the Osmosis v4 on-chain
Expand All @@ -22,8 +21,8 @@ func CreateUpgradeHandler(
keepers *keepers.AppKeepers,
) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, _plan upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) {
// configure upgrade for x/gamm module pool creation fee param
keepers.GAMMKeeper.SetParams(ctx, gammtypes.NewParams(sdk.Coins{sdk.NewInt64Coin("uosmo", 1)})) // 1 uOSMO
// Kept as comments for recordkeeping. SetParams is now private:
// keepers.GAMMKeeper.SetParams(ctx, gammtypes.NewParams(sdk.Coins{sdk.NewInt64Coin("uosmo", 1)})) // 1 uOSMO

Prop12(ctx, keepers.BankKeeper, keepers.DistrKeeper)

Expand Down
9 changes: 5 additions & 4 deletions app/upgrades/v9/prop214.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ func ExecuteProp214(ctx sdk.Context, gamm *gammkeeper.Keeper) {

balancerPool.PoolParams.SwapFee = sdk.MustNewDecFromStr("0.002")

err = gamm.SetPool(ctx, balancerPool)
if err != nil {
panic(err)
}
// Kept as comments for recordkeeping. SetPool is now private:
// err = gamm.SetPool(ctx, balancerPool)
// if err != nil {
// panic(err)
// }
}
12 changes: 6 additions & 6 deletions app/upgrades/v9/prop214_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package v9_test
import (
"testing"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/stretchr/testify/suite"

"github.com/osmosis-labs/osmosis/v7/app/apptesting"
Expand All @@ -26,11 +25,12 @@ func (suite *UpgradeTestSuite) TestProp214() {
poolId := suite.PrepareBalancerPool()
v9.ExecuteProp214(suite.Ctx, suite.App.GAMMKeeper)

pool, err := suite.App.GAMMKeeper.GetPoolAndPoke(suite.Ctx, poolId)
_, err := suite.App.GAMMKeeper.GetPoolAndPoke(suite.Ctx, poolId)
suite.Require().NoError(err)

swapFee := pool.GetSwapFee(suite.Ctx)
expectedSwapFee := sdk.MustNewDecFromStr("0.002")

suite.Require().Equal(expectedSwapFee, swapFee)
// Kept as comments for recordkeeping. Since SetPool is now private, the changes being tested for can no longer be made:
// swapFee := pool.GetSwapFee(suite.Ctx)
// expectedSwapFee := sdk.MustNewDecFromStr("0.002")
//
// suite.Require().Equal(expectedSwapFee, swapFee)
}
12 changes: 12 additions & 0 deletions x/gamm/keeper/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package keeper

import (
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/osmosis-labs/osmosis/v7/x/gamm/types"
)

// SetParams sets the total set of params.
func (k Keeper) SetParams(ctx sdk.Context, params types.Params) {
k.setParams(ctx, params)
}
6 changes: 3 additions & 3 deletions x/gamm/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
// InitGenesis initializes the x/gamm module's state from a provided genesis
// state, which includes the current live pools, global pool parameters (e.g. pool creation fee), next pool number etc.
func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState, unpacker codectypes.AnyUnpacker) {
k.SetParams(ctx, genState.Params)
k.setParams(ctx, genState.Params)
k.setNextPoolNumber(ctx, genState.NextPoolNumber)

// Sums up the liquidity in all genesis state pools to find the total liquidity across all pools.
Expand All @@ -22,7 +22,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState, unpack
if err != nil {
panic(err)
}
err = k.SetPool(ctx, pool)
err = k.setPool(ctx, pool)
if err != nil {
panic(err)
}
Expand All @@ -33,7 +33,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState, unpack
}
}

k.SetTotalLiquidity(ctx, liquidity)
k.setTotalLiquidity(ctx, liquidity)
}

// ExportGenesis returns the capability module's exported genesis.
Expand Down
2 changes: 1 addition & 1 deletion x/gamm/keeper/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ func (k Keeper) GetParams(ctx sdk.Context) (params types.Params) {
}

// SetParams sets the total set of params.
func (k Keeper) SetParams(ctx sdk.Context, params types.Params) {
func (k Keeper) setParams(ctx sdk.Context, params types.Params) {
k.paramSpace.SetParamSet(ctx, &params)
}
4 changes: 2 additions & 2 deletions x/gamm/keeper/pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func (k Keeper) GetPoolsAndPoke(ctx sdk.Context) (res []types.PoolI, err error)
return res, nil
}

func (k Keeper) SetPool(ctx sdk.Context, pool types.PoolI) error {
func (k Keeper) setPool(ctx sdk.Context, pool types.PoolI) error {
bz, err := k.MarshalPool(pool)
if err != nil {
return err
Expand Down Expand Up @@ -255,7 +255,7 @@ func (k *Keeper) SetStableSwapScalingFactors(ctx sdk.Context, scalingFactors []u

stableswapPool.ScalingFactor = scalingFactors

if err = k.SetPool(ctx, stableswapPool); err != nil {
if err = k.setPool(ctx, stableswapPool); err != nil {
return err
}
return nil
Expand Down
2 changes: 1 addition & 1 deletion x/gamm/keeper/pool_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ func (k Keeper) CreatePool(ctx sdk.Context, msg types.CreatePoolMsg) (uint64, er
Display: poolShareDisplayDenom,
})

if err := k.SetPool(ctx, pool); err != nil {
if err := k.setPool(ctx, pool); err != nil {
return 0, err
}

Expand Down
4 changes: 2 additions & 2 deletions x/gamm/keeper/share.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func (k Keeper) applyJoinPoolStateChange(ctx sdk.Context, pool types.PoolI, join
return err
}

err = k.SetPool(ctx, pool)
err = k.setPool(ctx, pool)
if err != nil {
return err
}
Expand All @@ -40,7 +40,7 @@ func (k Keeper) applyExitPoolStateChange(ctx sdk.Context, pool types.PoolI, exit
return err
}

err = k.SetPool(ctx, pool)
err = k.setPool(ctx, pool)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion x/gamm/keeper/swap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func (k Keeper) updatePoolForSwap(
tokensIn := sdk.Coins{tokenIn}
tokensOut := sdk.Coins{tokenOut}

err := k.SetPool(ctx, pool)
err := k.setPool(ctx, pool)
if err != nil {
return err
}
Expand Down
10 changes: 5 additions & 5 deletions x/gamm/keeper/total_liquidity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ func (k Keeper) GetTotalLiquidity(ctx sdk.Context) sdk.Coins {
return coins
}

func (k Keeper) SetTotalLiquidity(ctx sdk.Context, coins sdk.Coins) {
func (k Keeper) setTotalLiquidity(ctx sdk.Context, coins sdk.Coins) {
for _, coin := range coins {
k.SetDenomLiquidity(ctx, coin.Denom, coin.Amount)
k.setDenomLiquidity(ctx, coin.Denom, coin.Amount)
}
}

func (k Keeper) SetDenomLiquidity(ctx sdk.Context, denom string, amount sdk.Int) {
func (k Keeper) setDenomLiquidity(ctx sdk.Context, denom string, amount sdk.Int) {
store := ctx.KVStore(k.storeKey)
bz, err := amount.Marshal()
if err != nil {
Expand Down Expand Up @@ -68,14 +68,14 @@ func (k Keeper) RecordTotalLiquidityIncrease(ctx sdk.Context, coins sdk.Coins) {
for _, coin := range coins {
amount := k.GetDenomLiquidity(ctx, coin.Denom)
amount = amount.Add(coin.Amount)
k.SetDenomLiquidity(ctx, coin.Denom, amount)
k.setDenomLiquidity(ctx, coin.Denom, amount)
}
}

func (k Keeper) RecordTotalLiquidityDecrease(ctx sdk.Context, coins sdk.Coins) {
for _, coin := range coins {
amount := k.GetDenomLiquidity(ctx, coin.Denom)
amount = amount.Sub(coin.Amount)
k.SetDenomLiquidity(ctx, coin.Denom, amount)
k.setDenomLiquidity(ctx, coin.Denom, amount)
}
}