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

Jump to bottom

fix(migration): metadata migration was incorrect #7525

Closed
wants to merge 6 commits into from
Closed

fix(migration): metadata migration was incorrect #7525

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c466991
fix metadata migration in ibc
tac0turtle Oct 29, 2024
34a4fdc
fix build
tac0turtle Oct 29, 2024
737c455
linting
tac0turtle Oct 29, 2024
6c4e88a
fix tests
tac0turtle Oct 30, 2024
a229889
add default value back in case
tac0turtle Oct 30, 2024
1668cfb
undo breakage
tac0turtle Oct 30, 2024
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
23 changes: 14 additions & 9 deletions modules/apps/transfer/keeper/migrations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,9 @@ func (m Migrator) MigrateDenomMetadata(ctx sdk.Context) error {
m.keeper.iterateDenomTraces(ctx,
func(dt internaltypes.DenomTrace) (stop bool) {
// check if the metadata for the given denom trace does not already exist
if !m.keeper.bankKeeper.HasDenomMetaData(ctx, dt.IBCDenom()) {
m.keeper.setDenomMetadataWithDenomTrace(ctx, dt)
Comment on lines -46 to -47
Copy link
Member

@damiannolan damiannolan Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If there exists a denom metadata we will not overwrite it.

The only scenario in which this can overwrite an existing denom metadata, entering the if branch here is:

  • Collections/Map.Has (ref) returns has == true but also hits an error for some reason out of our control, meaning that err != nil in HasDenomMetaData here. This would yield a falsey return value and we'd enter if and overwrite the underlying denom metadata

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it is possible that the key constructed in an old version differs from the key constructed in a new version. We should verify that all non-ibc denom metadata's are queryable

denom, present := m.keeper.bankKeeper.GetDenomMetaData(ctx, dt.IBCDenom())
if !present {
m.keeper.setDenomMetadataWithDenomTrace(ctx, dt, denom)
}
return false
})
Expand Down Expand Up @@ -146,15 +147,19 @@ func (k Keeper) iterateDenomTraces(ctx context.Context, cb func(denomTrace inter
}

// setDenomMetadataWithDenomTrace sets an IBC token's denomination metadata
func (k Keeper) setDenomMetadataWithDenomTrace(ctx sdk.Context, denomTrace internaltypes.DenomTrace) {
func (k Keeper) setDenomMetadataWithDenomTrace(ctx sdk.Context, denomTrace internaltypes.DenomTrace, denomMetadata banktypes.Metadata) {
du := make([]*banktypes.DenomUnit, 0, len(denomMetadata.DenomUnits))

for _, dunit := range denomMetadata.DenomUnits {
du = append(du, &banktypes.DenomUnit{
Denom: dunit.Denom,
Exponent: dunit.Exponent,
Aliases: dunit.Aliases,
})
}
Comment on lines +153 to +159
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how this code is reachable.

metadata := banktypes.Metadata{
Description: fmt.Sprintf("IBC token from %s", denomTrace.GetFullDenomPath()),
DenomUnits: []*banktypes.DenomUnit{
{
Denom: denomTrace.BaseDenom,
Exponent: 0,
},
},
DenomUnits: du,
// Setting base as IBC hash denom since bank keepers's SetDenomMetadata uses
// Base as key path and the IBC hash is what gives this token uniqueness
// on the executing chain
Expand Down
1 change: 1 addition & 0 deletions modules/apps/transfer/types/expected_keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type BankKeeper interface {
IsSendEnabledCoins(ctx context.Context, coins ...sdk.Coin) error
HasDenomMetaData(ctx context.Context, denom string) bool
SetDenomMetaData(ctx context.Context, denomMetaData banktypes.Metadata)
GetDenomMetaData(ctx context.Context, denom string) (banktypes.Metadata, bool)
GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin
GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins
}
Expand Down
Loading