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

feat: add metadata for IBC tokens #3104

Merged
merged 43 commits into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
b1359b2
add metadata after mint
0xmuralik Feb 2, 2023
4070c24
set base denom as ibc hash denom
0xmuralik Feb 2, 2023
d0c2b83
comment
0xmuralik Feb 2, 2023
d0662a4
remove unsed funcs from keeper
0xmuralik Feb 3, 2023
ef8310a
Merge branch 'main' into murali/ibc-denom
0xmuralik Feb 6, 2023
e460e35
genesis setmetadata
0xmuralik Feb 6, 2023
850ded9
set metadata in migration
0xmuralik Feb 10, 2023
9e7924a
Merge branch 'main' into murali/ibc-denom
0xmuralik Feb 10, 2023
e257dcb
Merge branch 'main' into murali/ibc-denom
0xmuralik Feb 14, 2023
21c81e5
new migration
0xmuralik Feb 16, 2023
dafd5f1
test new migration
0xmuralik Feb 16, 2023
7763f74
Merge branch 'main' into murali/ibc-denom
0xmuralik Feb 17, 2023
a2b159f
Update modules/apps/transfer/keeper/migrations.go
0xmuralik May 3, 2023
663a0fd
import seperation
0xmuralik May 3, 2023
737cba5
replace MetaData with Metadata
0xmuralik May 3, 2023
c30d23d
Merge branch 'main' of https://github.com/0xmuralik/ibc-go into mural…
0xmuralik May 3, 2023
d894142
Merge branch 'murali/ibc-denom' of https://github.com/0xmuralik/ibc-g…
0xmuralik May 3, 2023
1640753
Merge branch 'main' into murali/ibc-denom
Jun 3, 2023
9501c78
fix typo
Jun 5, 2023
de08338
add initial migration documentation
Jun 5, 2023
324986c
fixed tests
Jun 5, 2023
e4e24d2
add logging message
Jul 4, 2023
8001ff5
Merge branch 'main' into murali/ibc-denom
Jul 8, 2023
ff7e7ff
Merge branch 'main' into murali/ibc-denom
DimitrisJim Aug 28, 2023
dac13bb
Bump consensus version.
DimitrisJim Aug 28, 2023
d16383b
Nits:
DimitrisJim Aug 28, 2023
c29568a
Merge branch 'main' into murali/ibc-denom
Aug 29, 2023
17f23ae
Merge branch 'main' into murali/ibc-denom
DimitrisJim Aug 29, 2023
771dd3c
addressing some of the feedback
Aug 31, 2023
e94f138
add more test cases
Aug 31, 2023
2794a64
rename variables
Sep 4, 2023
b97b8ee
check metadata in onrecvpacket tests
Sep 4, 2023
bcfe839
fix: remove test assertion on errors, state will be reverted by basea…
colin-axner Sep 6, 2023
b8e0f63
Merge branch 'main' into murali/ibc-denom
Sep 6, 2023
7091998
address review comment
Sep 6, 2023
ab7c94f
lint, lint, lint
Sep 6, 2023
a433aad
Merge branch 'main' into murali/ibc-denom
Sep 7, 2023
ca1304b
Merge branch 'main' into murali/ibc-denom
colin-axner Sep 7, 2023
1c2626d
fix test
Sep 7, 2023
0e1c34e
Merge branch 'main' into murali/ibc-denom
Sep 10, 2023
b22484a
address review comments
Sep 11, 2023
ec3d294
Merge branch 'main' into murali/ibc-denom
Sep 11, 2023
aaa6786
Merge branch 'main' into murali/ibc-denom
Sep 11, 2023
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
4 changes: 4 additions & 0 deletions docs/migrations/v7-to-v8.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ TODO: https://github.com/cosmos/ibc-go/pull/3505 (extra parameter added to trans
)
```

### Transfer migration

An automatic migration handler (TODO: add link after https://github.com/cosmos/ibc-go/pull/3104 is merged) is configured in the transfer module to set the [denomination metadata](https://github.com/cosmos/cosmos-sdk/blob/95178ce036741ae6aa7af131fa9fccf3e13fff7a/proto/cosmos/bank/v1beta1/bank.proto#L96-L125) for the IBC denominations of all vouchers minted by the transfer module.

## IBC Apps

TODO:
Expand Down
1 change: 1 addition & 0 deletions modules/apps/transfer/keeper/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, state types.GenesisState) {

for _, trace := range state.DenomTraces {
k.SetDenomTrace(ctx, trace)
k.SetDenomMetadata(ctx, trace)
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
}

// Only try to bind to port if it is not already bound, since we may already own
Expand Down
16 changes: 16 additions & 0 deletions modules/apps/transfer/keeper/migrations.go
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ func (m Migrator) MigrateTraces(ctx sdk.Context) error {
if !equalTraces(newTrace, dt) {
newTraces = append(newTraces, newTrace)
}

return false
})

Expand All @@ -65,6 +66,21 @@ func (m Migrator) MigrateTraces(ctx sdk.Context) error {
return nil
}

// MigrateMigrateDenomMetadataMetadata sets token metadata for all the IBC denom traces
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
func (m Migrator) MigrateDenomMetadata(ctx sdk.Context) error {
m.keeper.IterateDenomTraces(ctx,
func(dt types.DenomTrace) (stop bool) {
// check if the metadata for the given denom trace already exists
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
if !m.keeper.bankKeeper.HasDenomMetaData(ctx, dt.IBCDenom()) {
m.keeper.SetDenomMetadata(ctx, dt)
}
return false
})

m.keeper.Logger(ctx).Info("successfully added metadata to IBC voucher denominations")
return nil
}

// MigrateTotalEscrowForDenom migrates the total amount of source chain tokens in escrow.
func (m Migrator) MigrateTotalEscrowForDenom(ctx sdk.Context) error {
var totalEscrowed sdk.Coins
Expand Down
80 changes: 80 additions & 0 deletions modules/apps/transfer/keeper/migrations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"
banktestutil "github.com/cosmos/cosmos-sdk/x/bank/testutil"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

transferkeeper "github.com/cosmos/ibc-go/v7/modules/apps/transfer/keeper"
transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
Expand Down Expand Up @@ -235,3 +236,82 @@ func (suite *KeeperTestSuite) TestMigrateTotalEscrowForDenom() {
})
}
}

func (suite *KeeperTestSuite) TestMigratorMigrateMetadata() {
var (
denomTrace transfertypes.DenomTrace
expectedMetadata banktypes.Metadata
)

testCases := []struct {
msg string
malleate func()
}{
{
"success with denom trace with one hop",
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func() {
denomTrace = transfertypes.DenomTrace{
BaseDenom: "foo",
Path: "transfer/channel-0",
}

expectedMetadata = banktypes.Metadata{
Description: "IBC token from transfer/channel-0/foo",
DenomUnits: []*banktypes.DenomUnit{
{
Denom: "foo",
Exponent: 0,
},
},
Base: denomTrace.IBCDenom(), // ibc/EB7094899ACFB7A6F2A67DB084DEE2E9A83DEFAA5DEF92D9A9814FFD9FF673FA
Display: "transfer/channel-0/foo",
Name: "transfer/channel-0/foo IBC token",
Symbol: "FOO",
}
},
},
{
"success with denom trace with two hops",
func() {
denomTrace = transfertypes.DenomTrace{
BaseDenom: "ubar",
Path: "transfer/channel-1/transfer/channel-2",
}

expectedMetadata = banktypes.Metadata{
Description: "IBC token from transfer/channel-1/transfer/channel-2/ubar",
DenomUnits: []*banktypes.DenomUnit{
{
Denom: "ubar",
Exponent: 0,
},
},
Base: denomTrace.IBCDenom(), // ibc/8243B3EAA19BAB1DB3B0020B81C0C5A953E7B22C042CEE44E639A11A238BA57C
Display: "transfer/channel-1/transfer/channel-2/ubar",
Name: "transfer/channel-1/transfer/channel-2/ubar IBC token",
Symbol: "UBAR",
}
},
},
}

for _, tc := range testCases {
suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
suite.SetupTest() // reset
ctx := suite.chainA.GetContext()

tc.malleate()

suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(ctx, denomTrace)

// run migration
migrator := transferkeeper.NewMigrator(suite.chainA.GetSimApp().TransferKeeper)
err := migrator.MigrateDenomMetadata(suite.chainA.GetContext())
suite.Require().NoError(err)

denomMetadata, ok := suite.chainA.GetSimApp().BankKeeper.GetDenomMetaData(ctx, expectedMetadata.Base)
suite.Require().True(ok)
suite.Require().Equal(expectedMetadata, denomMetadata)
})
}
}
39 changes: 39 additions & 0 deletions modules/apps/transfer/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

"github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
Expand Down Expand Up @@ -277,6 +278,10 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
return errorsmod.Wrap(err, "failed to mint IBC tokens")
}

if !k.bankKeeper.HasDenomMetaData(ctx, voucherDenom) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if we are running migrations such that all traces have metadata set, shouldn't we just make setting metadata a functionality of SetDenomTrace? This avoids accidental incidents where the trace is set but the metadata isn't. There should never be an instance when you want to set the denom trace without setting the metadata as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we could do that, although SetDenomTrace is a setter function for a specific key in the store, so it would squeak to me a bit if we add SetDenomMetadata there as well. Maybe we could have a helper function that wraps both SetDenomTrace and SetDenomMetadata. Happy to hear other people's thoughts.

colin-axner marked this conversation as resolved.
Show resolved Hide resolved
k.SetDenomMetadata(ctx, denomTrace)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

// send to receiver
if err := k.bankKeeper.SendCoinsFromModuleToAccount(
ctx, types.ModuleName, receiver, sdk.NewCoins(voucher),
Expand Down Expand Up @@ -423,3 +428,37 @@ func (k Keeper) DenomPathFromHash(ctx sdk.Context, denom string) (string, error)
fullDenomPath := denomTrace.GetFullDenomPath()
return fullDenomPath, nil
}

// SetDenomMetadata sets an IBC token's denomination metadata
func (k Keeper) SetDenomMetadata(ctx sdk.Context, denomTrace types.DenomTrace) {
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
metadata := banktypes.Metadata{
Description: getMetadataDescription(denomTrace),
DenomUnits: []*banktypes.DenomUnit{
{
Denom: denomTrace.BaseDenom,
Exponent: 0,
},
},
// Setting base as IBC hash denom since bank keepers's SetDenomMetaData uses
// Base as storeKey and the bank keeper will only have the IBC hash to get
// the denom metadata
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
Base: denomTrace.IBCDenom(),
Display: denomTrace.GetFullDenomPath(),
Name: getMetadataName(denomTrace),
Symbol: getMetadataSymbol(denomTrace),
}

k.bankKeeper.SetDenomMetaData(ctx, metadata)
}

func getMetadataDescription(denomTrace types.DenomTrace) string {
return fmt.Sprintf("IBC token from %s", denomTrace.GetFullDenomPath())
}

func getMetadataName(denomTrace types.DenomTrace) string {
return fmt.Sprintf("%s IBC token", denomTrace.GetFullDenomPath())
}

func getMetadataSymbol(denomTrace types.DenomTrace) string {
return strings.ToUpper(denomTrace.BaseDenom)
}
6 changes: 5 additions & 1 deletion modules/apps/transfer/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {
if err := cfg.RegisterMigration(types.ModuleName, 3, m.MigrateParams); err != nil {
panic(fmt.Sprintf("failed to migrate transfer app version 3 to 4 (self-managed params migration): %v", err))
}

if err := cfg.RegisterMigration(types.ModuleName, 4, m.MigrateDenomMetadata); err != nil {
panic(fmt.Sprintf("failed to migrate transfer app from version 4 to 5 (set denom metadata migration): %v", err))
}
damiannolan marked this conversation as resolved.
Show resolved Hide resolved
}

// InitGenesis performs genesis initialization for the ibc-transfer module. It returns
Expand All @@ -136,7 +140,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw
}

// ConsensusVersion implements AppModule/ConsensusVersion defining the current version of transfer.
func (AppModule) ConsensusVersion() uint64 { return 4 }
func (AppModule) ConsensusVersion() uint64 { return 5 }

// BeginBlock implements the AppModule interface
func (AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) {
Expand Down
3 changes: 3 additions & 0 deletions modules/apps/transfer/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package types
import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types"
Expand All @@ -25,6 +26,8 @@ type BankKeeper interface {
SendCoinsFromAccountToModule(ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error
BlockedAddr(addr sdk.AccAddress) bool
IsSendEnabledCoin(ctx sdk.Context, coin sdk.Coin) bool
HasDenomMetaData(ctx sdk.Context, denom string) bool
SetDenomMetaData(ctx sdk.Context, denomMetaData banktypes.Metadata)
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
GetBalance(ctx sdk.Context, addr sdk.AccAddress, denom string) sdk.Coin
GetAllBalances(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins
}
Expand Down