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

fix: fix the account number of x/tokenfactory module account #5534

Merged
merged 2 commits into from
Jun 21, 2023
Merged

fix: fix the account number of x/tokenfactory module account #5534

merged 2 commits into from
Jun 21, 2023

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented Jun 15, 2023

What is the purpose of the change

This pull request fixes the account number of x/tokenfactory is wrongly set by genesis since CreateModuleAccount only stores the module account with default account number 0, and it it also missing an update to global_account_number inside x/auth.

To solve it, CreateModuleAccount should call AccountKeeper.GetModuleAccount instead of AccountKeeper.SetModuleAccount, or it lost to set account number properly, see:
https://github.com/cosmos/cosmos-sdk/blob/main/x/auth/keeper/keeper.go#L252

Testing and Verifying

This change added tests and can be verified as follows:

  • Added unit test that validates the account number of x/tokenfactory module account is correctly set

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@dadamu dadamu changed the title fix: fix the account number of module account fix: fix the account number of x/tokenfactory module account Jun 15, 2023
@@ -83,6 +82,5 @@ func (k *Keeper) SetContractKeeper(contractKeeper types.ContractKeeper) {
// it purely mints and burns them on behalf of the admin of respective denoms,
// and sends to the relevant address.
func (k Keeper) CreateModuleAccount(ctx sdk.Context) {
moduleAcc := authtypes.NewEmptyModuleAccount(types.ModuleName, authtypes.Minter, authtypes.Burner)
k.accountKeeper.SetModuleAccount(ctx, moduleAcc)
k.accountKeeper.GetModuleAccount(ctx, types.ModuleName)
Copy link
Contributor Author

@dadamu dadamu Jun 16, 2023

Choose a reason for hiding this comment

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

No, it should be created at the genesis phase, see x/gov calls GetModuleAccount to create the module account as follows:
https://github.com/cosmos/cosmos-sdk/blob/release/v0.45.x/x/gov/genesis.go#L19

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

I think the change makes sense, tests seem to be failing can you take a look into the tests please 🙏

@mattverse
Copy link
Member

let's also add a changelog for this please

@mattverse mattverse added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Jun 16, 2023
@dadamu
Copy link
Contributor Author

dadamu commented Jun 16, 2023

@mattverse Added changelog entry. The last test failing seems not related to this change if I am not wrong, and also test is passed by manually and the latest test checking action.

@dadamu
Copy link
Contributor Author

dadamu commented Jun 20, 2023

@mattverse Rebased

@dadamu dadamu requested a review from mattverse June 20, 2023 08:12
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

LGTM

@mattverse mattverse merged commit e3b8818 into osmosis-labs:main Jun 21, 2023
@dadamu dadamu deleted the paul/fix-tokenfactory-create-module-account branch June 21, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/tokenfactory V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants