tokenfactory: address mutation tests part 1 #2477
Conversation
github-actions
bot
added
C:app-wiring
| 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 | ||
| } |
There was a problem hiding this comment.
the order here was changed because we were charging to create denoms with invalid inputs
x/tokenfactory/keeper/createdenom.go
Outdated
| if creationFee != nil { | ||
| for _, coin := range creationFee { | ||
| if !k.bankKeeper.HasBalance(ctx, accAddr, coin) { | ||
| return sdkerrors.ErrInsufficientFunds | ||
| } | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah I see, my apologies for raising a false flag! Will revert this
| // 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) |
There was a problem hiding this comment.
If we don't remove the module account here, we are never actually testing if initGenesis creates the module account properly
There was a problem hiding this comment.
We should make a new suite.SetupGenesisTest() constructor in the future, to not have this be a concern in the future
| tokenFactoryKeeper := suite.App.TokenFactoryKeeper | ||
| bankKeeper := suite.App.BankKeeper |
There was a problem hiding this comment.
I think this is getting excessive with aliasing. Don't need to revert or anything, but I'd suggest not feeling a strong need to interlace this pattern everywhere imo
Addresses: #2105
What is the purpose of the change
Brings mutation tests for tokenfactory from 37% to 48% passing.
Brief Changelog
Testing and Verifying
This change is already covered by existing tests
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? no