-
Notifications
You must be signed in to change notification settings - Fork 609
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 x/tokenfactory
genesis import denoms reset x/bank
existing denom metadata
#5532
fix: fix x/tokenfactory
genesis import denoms reset x/bank
existing denom metadata
#5532
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Logic change lgtm, requesting some test additions before we can merge this
_, exists := k.bankKeeper.GetDenomMetaData(ctx, denom) | ||
if !exists { | ||
denomMetaData := banktypes.Metadata{ | ||
DenomUnits: []*banktypes.DenomUnit{{ | ||
Denom: denom, | ||
Exponent: 0, | ||
}}, | ||
Base: denom, | ||
} | ||
|
||
k.bankKeeper.SetDenomMetaData(ctx, denomMetaData) | ||
k.bankKeeper.SetDenomMetaData(ctx, denomMetaData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can unit tests for CreateDenom
be added to confirm the new logic introduced as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the unit test to check CreateDenom
if the metadata is set correctly, the metadata existing case is already covered by this test
@dadamu Would you also be able to add CHANGELOG entry? |
@mattverse Added the CHANGELOG entry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the PR!
What is the purpose of the change
This pull request fixes the
x/tokenfactory
genesis import resetx/bank
existing denom metadata, say,x/bank
genesis has the metadata of facotory/osmos1.../testtoken with{Display: test}
,x/tokenfactory
init genesis phase will replace it with the empty disply, more details as follows:Before
x/tokenfactory
init genesis, thex/bank
set metadata as:After
x/tokenfactory
init genesis, the metadata is replaced into:Testing and Verifying
This change added tests and can be verified as follows:
x/bank
imported metadata won't be reset byx/tokenfactory
Documentation and Release Note
Unreleased
section ofCHANGELOG.md
?Where is the change documented?
x/{module}/README.md
)