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

tokenfactory: address mutation tests part 1 #2477

Merged
merged 11 commits into from
Aug 25, 2022
Merged

tokenfactory: address mutation tests part 1 #2477

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
25cf6ad
tokenfactory mutation test changes
czarcas7ic Aug 22, 2022
6fdc6ed
rev tests should now pass
czarcas7ic Aug 22, 2022
79178af
revert (wrong branch)
czarcas7ic Aug 22, 2022
fd7d74c
more test improvements
czarcas7ic Aug 23, 2022
118875d
only test message emission
czarcas7ic Aug 23, 2022
6252df7
address Roman's comments
czarcas7ic Aug 23, 2022
83bd44e
address Roman's comments
czarcas7ic Aug 23, 2022
6bcf5a3
fix burn message emit name clash
czarcas7ic Aug 23, 2022
c1fc0d0
Merge branch 'main' into adam/tokenfactory-mutation-1
czarcas7ic Aug 24, 2022
3653a49
address Dev comment
czarcas7ic Aug 24, 2022
db6c8aa
Merge branch 'adam/tokenfactory-mutation-1' of https://github.com/osm…
czarcas7ic Aug 24, 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
5 changes: 0 additions & 5 deletions app/apptesting/test_suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,6 @@ func (s *KeeperTestHelper) SetupValidator(bondStatus stakingtypes.BondStatus) sd
return valAddr
}

// SetupTokenFactory sets up a token module account for the TokenFactoryKeeper.
func (s *KeeperTestHelper) SetupTokenFactory() {
s.App.TokenFactoryKeeper.CreateModuleAccount(s.Ctx)
}

ValarDragon marked this conversation as resolved.
Show resolved Hide resolved
// BeginNewBlock starts a new block.
func (s *KeeperTestHelper) BeginNewBlock(executeNextEpoch bool) {
var valAddr []byte
Expand Down
12 changes: 9 additions & 3 deletions x/tokenfactory/keeper/createdenom.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,20 @@ import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

"github.com/osmosis-labs/osmosis/v11/x/tokenfactory/types"
)

// ConvertToBaseToken converts a fee amount in a whitelisted fee token to the base fee token amount
func (k Keeper) CreateDenom(ctx sdk.Context, creatorAddr string, subdenom string) (newTokenDenom string, err error) {
err = k.chargeForCreateDenom(ctx, creatorAddr, subdenom)
denom, err := k.validateCreateDenom(ctx, creatorAddr, subdenom)
if err != nil {
return "", err
}

denom, err := k.validateCreateDenom(ctx, creatorAddr, subdenom)
err = k.chargeForCreateDenom(ctx, creatorAddr, subdenom)
if err != nil {
return "", err
}
Comment on lines +14 to 22
Copy link
Member Author

Choose a reason for hiding this comment

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

the order here was changed because we were charging to create denoms with invalid inputs

Expand Down Expand Up @@ -77,7 +78,12 @@ func (k Keeper) chargeForCreateDenom(ctx sdk.Context, creatorAddr string, subden
if err != nil {
return err
}
if len(creationFee) > 0 {
if creationFee != nil {
for _, coin := range creationFee {
if !k.bankKeeper.HasBalance(ctx, accAddr, coin) {
return sdkerrors.ErrInsufficientFunds
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this was added after I found that if the creation fee was multiple denoms, it was possible for a user to be charged if they had one of the two required denoms and still not get to create a tokenfactory token

Copy link
Member

Choose a reason for hiding this comment

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

Wait what? FundCommunityPool should call SendCoins which does this check?

Did we have a test for this showing the problem, this seems like something very concerning if true.

Copy link
Member Author

Choose a reason for hiding this comment

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

We didn't have a test for this, and it only showed its head when I added the account doesn't have enough to pay for denom creation fee test case. If you revert just this change and run the test case with prints you will see the following:

preCreateBalance 100000000uion,1000000000uosmo 
postCreateBalance 50000000uion,1000000000uosmo

We had enough uion so it deducted that, but we didnt have enough uosmo and stops there.

Copy link
Member

Choose a reason for hiding this comment

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

This is an error in the test.

The way reversion is supposed to work, is that all tx state changes outside of the ante handler get reverted if the message returns an error. So the test is wrong here, the code was right.

The test was for the keeper function, which is the incorrect layer to test reversion logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see, my apologies for raising a false flag! Will revert this

if err := k.communityPoolKeeper.FundCommunityPool(ctx, creationFee, accAddr); err != nil {
return err
}
Expand Down
69 changes: 54 additions & 15 deletions x/tokenfactory/keeper/createdenom_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

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

"github.com/osmosis-labs/osmosis/v11/x/tokenfactory/keeper"
"github.com/osmosis-labs/osmosis/v11/x/tokenfactory/types"
)

Expand Down Expand Up @@ -57,19 +58,26 @@ func (suite *KeeperTestSuite) TestMsgCreateDenom() {
}

func (suite *KeeperTestSuite) TestCreateDenom() {
defaultDenomCreationFee := types.Params{DenomCreationFee: sdk.NewCoins(sdk.NewCoin("uosmo", sdk.NewInt(50000000)))}
twoDenomCreationFee := types.Params{DenomCreationFee: sdk.NewCoins(sdk.NewCoin("uosmo", sdk.NewInt(50000000)), sdk.NewCoin("uion", sdk.NewInt(50000000)))}
nilCreationFee := types.Params{DenomCreationFee: nil}
largeCreationFee := types.Params{DenomCreationFee: sdk.NewCoins(sdk.NewCoin("uosmo", sdk.NewInt(5000000000)), sdk.NewCoin("uion", sdk.NewInt(50000000)))}
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved
for _, tc := range []struct {
desc string
setup func()
subdenom string
valid bool
desc string
denomCreationFee types.Params
setup func()
subdenom string
valid bool
}{
{
desc: "subdenom too long",
subdenom: "assadsadsadasdasdsadsadsadsadsadsadsklkadaskkkdasdasedskhanhassyeunganassfnlksdflksafjlkasd",
valid: false,
desc: "subdenom too long",
denomCreationFee: defaultDenomCreationFee,
subdenom: "assadsadsadasdasdsadsadsadsadsadsadsklkadaskkkdasdasedskhanhassyeunganassfnlksdflksafjlkasd",
valid: false,
},
{
desc: "subdenom and creator pair already exists",
desc: "subdenom and creator pair already exists",
denomCreationFee: defaultDenomCreationFee,
setup: func() {
_, err := suite.msgServer.CreateDenom(sdk.WrapSDKContext(suite.Ctx), types.NewMsgCreateDenom(suite.TestAccs[0].String(), "bitcoin"))
suite.Require().NoError(err)
Expand All @@ -78,24 +86,53 @@ func (suite *KeeperTestSuite) TestCreateDenom() {
valid: false,
},
{
desc: "success case",
subdenom: "evmos",
valid: true,
desc: "success case: defaultDenomCreationFee",
denomCreationFee: defaultDenomCreationFee,
subdenom: "evmos",
valid: true,
},
{
desc: "subdenom having invalid characters",
subdenom: "bit/***///&&&/coin",
valid: false,
desc: "success case: twoDenomCreationFee",
denomCreationFee: twoDenomCreationFee,
subdenom: "catcoin",
valid: true,
},
{
desc: "success case: nilCreationFee",
denomCreationFee: nilCreationFee,
subdenom: "czcoin",
valid: true,
},
{
desc: "account doesn't have enough to pay for denom creation fee",
denomCreationFee: largeCreationFee,
subdenom: "tooexpensive",
valid: false,
},
{
desc: "subdenom having invalid characters",
denomCreationFee: defaultDenomCreationFee,
subdenom: "bit/***///&&&/coin",
valid: false,
},
} {
suite.SetupTest()
suite.Run(fmt.Sprintf("Case %s", tc.desc), func() {
if tc.setup != nil {
tc.setup()
}
// Create a denom
// Set denom creation fee in params
keeper.Keeper.SetParams(*suite.App.TokenFactoryKeeper, suite.Ctx, tc.denomCreationFee)
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved
denomCreationFee := suite.App.TokenFactoryKeeper.GetParams(suite.Ctx).DenomCreationFee
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved
suite.Require().Equal(tc.denomCreationFee.DenomCreationFee, denomCreationFee)

// note balance, create a tokenfactory denom, then note balance again
preCreateBalance := suite.App.BankKeeper.GetAllBalances(suite.Ctx, suite.TestAccs[0])
res, err := suite.msgServer.CreateDenom(sdk.WrapSDKContext(suite.Ctx), types.NewMsgCreateDenom(suite.TestAccs[0].String(), tc.subdenom))
postCreateBalance := suite.App.BankKeeper.GetAllBalances(suite.Ctx, suite.TestAccs[0])
if tc.valid {
suite.Require().NoError(err)
suite.Require().True(preCreateBalance.Sub(postCreateBalance).IsEqual(denomCreationFee))

// Make sure that the admin is set correctly
queryRes, err := suite.queryClient.DenomAuthorityMetadata(suite.Ctx.Context(), &types.QueryDenomAuthorityMetadataRequest{
Expand All @@ -107,6 +144,8 @@ func (suite *KeeperTestSuite) TestCreateDenom() {

} else {
suite.Require().Error(err)
// Ensure we don't charge if we expect an error
suite.Require().True(preCreateBalance.IsEqual(postCreateBalance))
}
})
}
Expand Down
5 changes: 3 additions & 2 deletions x/tokenfactory/keeper/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@ import (
// InitGenesis initializes the tokenfactory module's state from a provided genesis
// state.
func (k Keeper) InitGenesis(ctx sdk.Context, genState types.GenesisState) {
k.CreateModuleAccount(ctx)

if genState.Params.DenomCreationFee == nil {
genState.Params.DenomCreationFee = sdk.NewCoins()
}
k.SetParams(ctx, genState.Params)

// The call to GetModuleAccount creates a module account if it does not exist.
k.accountKeeper.GetModuleAccount(ctx, types.ModuleName)

for _, genDenom := range genState.GetFactoryDenoms() {
creator, _, err := types.DeconstructDenom(genDenom.GetDenom())
if err != nil {
Expand Down
11 changes: 11 additions & 0 deletions x/tokenfactory/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ func (suite *KeeperTestSuite) TestGenesis() {
},
}
app := suite.App
// remove module account to ensure initGenesis initializes it on its own
tokenfactoryModuleAccount := app.AccountKeeper.GetAccount(suite.Ctx, app.AccountKeeper.GetModuleAddress(types.ModuleName))
app.AccountKeeper.RemoveAccount(suite.Ctx, tokenfactoryModuleAccount)
Comment on lines +35 to +37
Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't remove the module account here, we are never actually testing if initGenesis creates the module account properly

Copy link
Member

Choose a reason for hiding this comment

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

We should make a new suite.SetupGenesisTest() constructor in the future, to not have this be a concern in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

Issue created! #2502

suite.Ctx = app.BaseApp.NewContext(false, tmproto.Header{})
// Test both with bank denom metadata set, and not set.
for i, denom := range genesisState.FactoryDenoms {
Expand All @@ -40,9 +43,17 @@ func (suite *KeeperTestSuite) TestGenesis() {
app.BankKeeper.SetDenomMetaData(suite.Ctx, banktypes.Metadata{Base: denom.GetDenom()})
}
}
// check before initGenesis that the module account is nil
tokenfactoryModuleAccount = app.AccountKeeper.GetAccount(suite.Ctx, app.AccountKeeper.GetModuleAddress(types.ModuleName))
suite.Require().Nil(tokenfactoryModuleAccount)

app.TokenFactoryKeeper.SetParams(suite.Ctx, types.Params{DenomCreationFee: sdk.Coins{sdk.NewInt64Coin("uosmo", 100)}})
app.TokenFactoryKeeper.InitGenesis(suite.Ctx, genesisState)

// check that the module account is now initialized
tokenfactoryModuleAccount = app.AccountKeeper.GetAccount(suite.Ctx, app.AccountKeeper.GetModuleAddress(types.ModuleName))
suite.Require().NotNil(tokenfactoryModuleAccount)

exportedGenesis := app.TokenFactoryKeeper.ExportGenesis(suite.Ctx)
suite.Require().NotNil(exportedGenesis)
suite.Require().Equal(genesisState, *exportedGenesis)
Expand Down
4 changes: 1 addition & 3 deletions x/tokenfactory/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,11 @@ func (suite *KeeperTestSuite) SetupTest() {
suite.Setup()

// Fund every TestAcc with 100 denom creation fees.
fundAccsAmount := sdk.NewCoins(sdk.NewCoin(types.DefaultParams().DenomCreationFee[0].Denom, types.DefaultParams().DenomCreationFee[0].Amount.MulRaw(100)))
fundAccsAmount := sdk.NewCoins(sdk.NewCoin(types.DefaultParams().DenomCreationFee[0].Denom, types.DefaultParams().DenomCreationFee[0].Amount.MulRaw(100)), sdk.NewCoin("uion", sdk.NewInt(100000000)))
p0mvn marked this conversation as resolved.
Show resolved Hide resolved
for _, acc := range suite.TestAccs {
suite.FundAcc(acc, fundAccsAmount)
}

suite.SetupTokenFactory()

suite.queryClient = types.NewQueryClient(suite.QueryHelper)
suite.msgServer = keeper.NewMsgServerImpl(*suite.App.TokenFactoryKeeper)
}
Expand Down
2 changes: 2 additions & 0 deletions x/tokenfactory/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ type BankKeeper interface {
BurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) error

SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error
HasBalance(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coin) bool
}

type AccountKeeper interface {
SetModuleAccount(ctx sdk.Context, macc authtypes.ModuleAccountI)
GetModuleAccount(ctx sdk.Context, moduleName string) authtypes.ModuleAccountI
czarcas7ic marked this conversation as resolved.
Show resolved Hide resolved
}

// CommunityPoolKeeper defines the contract needed to be fulfilled for community pool interactions.
Expand Down