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.

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

Conversation

czarcas7ic
Copy link
Member

@czarcas7ic czarcas7ic commented Aug 22, 2022

Addresses: #2105

What is the purpose of the change

Brings mutation tests for tokenfactory from 37% to 48% passing.

Brief Changelog

  • Remove unused function
  • No longer charge for invalid create denom attempts
  • Expand CreateDenom tests
  • Properly test init genesis
  • Test CreateModuleAccount for tokenfactory
  • Test all tokenfactory msg server messages

Testing and Verifying

This change is already covered by existing tests

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
  • How is the feature or change documented? not applicable

@github-actions github-actions bot added C:app-wiring Changes to the app folder C:x/tokenfactory labels Aug 22, 2022
Comment on lines +15 to 23
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
}
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

Comment on lines 81 to 86
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

Comment on lines +35 to +37
// 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)
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

@github-actions github-actions bot added the C:x/twap Changes to the twap module label Aug 22, 2022
@github-actions github-actions bot removed the C:x/twap Changes to the twap module label Aug 22, 2022
@czarcas7ic czarcas7ic self-assigned this Aug 22, 2022
@czarcas7ic czarcas7ic marked this pull request as ready for review August 23, 2022 00:39
@czarcas7ic czarcas7ic requested a review from a team August 23, 2022 00:39
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

Great work! Left some comments, please let me know what you think

x/tokenfactory/keeper/createdenom_test.go Outdated Show resolved Hide resolved
x/tokenfactory/keeper/createdenom_test.go Outdated Show resolved Hide resolved
x/tokenfactory/keeper/createdenom_test.go Outdated Show resolved Hide resolved
x/tokenfactory/keeper/keeper_test.go Outdated Show resolved Hide resolved
x/tokenfactory/keeper/msg_server_test.go Outdated Show resolved Hide resolved
x/tokenfactory/types/expected_keepers.go Outdated Show resolved Hide resolved
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

LGTM once we understand the duplicate event

app/apptesting/test_suite.go Show resolved Hide resolved
x/tokenfactory/keeper/createdenom_test.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Aug 23, 2022
Comment on lines +133 to +134
tokenFactoryKeeper := suite.App.TokenFactoryKeeper
bankKeeper := suite.App.BankKeeper
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Nice job! Only question is clarity around that FundCommunityPool comment

@czarcas7ic czarcas7ic merged commit ed236e5 into main Aug 25, 2022
@czarcas7ic czarcas7ic deleted the adam/tokenfactory-mutation-1 branch August 25, 2022 00:08
@github-actions github-actions bot mentioned this pull request Mar 15, 2024
@github-actions github-actions bot mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder C:docs Improvements or additions to documentation C:x/tokenfactory
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants