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

feat: add metadata for IBC tokens #3104

Merged
merged 43 commits into from
Sep 12, 2023
+277 −15
Merged

feat: add metadata for IBC tokens #3104

Show file tree
Hide file tree
Changes from 9 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
crodriguezvega Jun 3, 2023
9501c78
fix typo
crodriguezvega Jun 5, 2023
de08338
add initial migration documentation
crodriguezvega Jun 5, 2023
324986c
fixed tests
crodriguezvega Jun 5, 2023
e4e24d2
add logging message
crodriguezvega Jul 4, 2023
8001ff5
Merge branch 'main' into murali/ibc-denom
crodriguezvega 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
crodriguezvega Aug 29, 2023
17f23ae
Merge branch 'main' into murali/ibc-denom
DimitrisJim Aug 29, 2023
771dd3c
addressing some of the feedback
crodriguezvega Aug 31, 2023
e94f138
add more test cases
crodriguezvega Aug 31, 2023
2794a64
rename variables
crodriguezvega Sep 4, 2023
b97b8ee
check metadata in onrecvpacket tests
crodriguezvega 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
crodriguezvega Sep 6, 2023
7091998
address review comment
crodriguezvega Sep 6, 2023
ab7c94f
lint, lint, lint
crodriguezvega Sep 6, 2023
a433aad
Merge branch 'main' into murali/ibc-denom
crodriguezvega Sep 7, 2023
ca1304b
Merge branch 'main' into murali/ibc-denom
colin-axner Sep 7, 2023
1c2626d
fix test
crodriguezvega Sep 7, 2023
0e1c34e
Merge branch 'main' into murali/ibc-denom
crodriguezvega Sep 10, 2023
b22484a
address review comments
crodriguezvega Sep 11, 2023
ec3d294
Merge branch 'main' into murali/ibc-denom
crodriguezvega Sep 11, 2023
aaa6786
Merge branch 'main' into murali/ibc-denom
crodriguezvega 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
1 change: 1 addition & 0 deletions modules/apps/transfer/keeper/genesis.go
View file
Open in desktop
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)
}

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

if !m.keeper.bankKeeper.HasDenomMetaData(ctx, newTrace.IBCDenom()) {
m.keeper.SetDenomMetaData(ctx, newTrace)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This migration may have already been run and might not be run again. I think it is safer to make a separate migration only setting the denom metadata. Please let me know if you have any questions on how to do this. I'm also happy to receive that change in a followup pr, it would make reviews faster/easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a new migration and registered it with as version 3 migration. Feel free to take up the task if required.

return false
})

Expand Down
37 changes: 37 additions & 0 deletions modules/apps/transfer/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

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"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
Expand Down Expand Up @@ -282,6 +283,10 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
return err
}

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)
}

// send to receiver
if err := k.bankKeeper.SendCoinsFromModuleToAccount(
ctx, types.ModuleName, receiver, sdk.NewCoins(voucher),
Expand Down Expand Up @@ -310,6 +315,38 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
return nil
}

func (k Keeper) SetDenomMetaData(ctx sdk.Context, denomTrace types.DenomTrace) {
metadata := banktypes.Metadata{
Description: getMetaDataDescription(denomTrace),
DenomUnits: []*banktypes.DenomUnit{
{
Denom: denomTrace.BaseDenom,
Exponent: 0,
},
},
// Setting base as IBChash Denom as SetDenomMetaData uses Base as storeKey
// and the bank keeper will only have the IBCHash to get the denom metadata
Base: denomTrace.IBCDenom(),
Display: denomTrace.GetFullDenomPath(),
Name: getMetaDataName(denomTrace),
Symbol: getMetaDataSymbol(denomTrace),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have an example print out of how this might look? Would be great if we could get product input on the desired formatting? cc @womensrights @adiraviraj @tmsdkeys

Copy link
Contributor

Choose a reason for hiding this comment

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

For context, this pr would add metadata about IBC tokens into the state machine. This means when clients query a bank balance, instead of only seeing ibc/{hash} they could also provide a flag to obtain back the metadata information stored here, such as the base denomination or the full history of this tokens hops (via channel/portIDs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a unit test for the new migration. The example metadata is available here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've known people asking for this on Discord, to have the metadata about IBC tokens as you would regular sdk tokens so think it's a good idea. Wrt the formatting, looks okay for me. The Base field should normally equal DenomUnits.Denom with Exponent according to the bank module's metadata, but the reasoning is provided why it needs to deviate.
Rest looks okay for me. Maybe @womensrights has insights from user research?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a great first step, but also adding in a human readable name to the current output would precisely cover what the metadata is used for. Front end developers are currently maintaining their own registries of assets to be displayed for their application. An example of such a registry is for Injective. Front ends need:

  • token symbol, this is covered
  • the location of the decimal place for the denom, I think this is covered from the exponent, from my understanding, e.g. the precision of ATOM is 6 dp, ATOM exponent is 6, uATOM is 0. In the example output given its not clear though as you have used an exponent of 0 for ubar and foo, foo should have exponent 6 assuming it is an sdk coin?
  • human readable name, having the path is good, but then you would need to check which chain the token came from additionally to make this human readable

So I think this is already an improvement, just if there was also a way to add an additional human readable name somehow then this would be great but I'm not sure this is easily done. Also as a sidenote, I know there is a constraint on the metadata from the sdk types so this is not easy to change, but for the ibc denoms, you don't get any new information from looking at the Display compared to Name so in the long term I would question the need to have both

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the input, @womensrights!

Copy link
Contributor

Choose a reason for hiding this comment

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

For the exponent, as you linked the FungibleTokenPacketData there isn't explicitly an exponent field but where does the logic to know the decimal place for the amount in the packet for a specific denom come from in this case?

The chainID would also not completely work as sometimes the chainID is not the same as what is considered the human branded name for the chain or you would need to remove the additional numbers. e.g. I think bandchain has chainID "laozi-mainnet" and the human readable name would be Bandchain

Copy link
Contributor

Choose a reason for hiding this comment

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

For the exponent, as you linked the FungibleTokenPacketData there isn't explicitly an exponent field but where does the logic to know the decimal place for the amount in the packet for a specific denom come from in this case?

The amount comes from the sdk Coin that is used in MsgTransfer. Whatever that amount is, we put it in the FungibleTokenPacketData, but we don't have any information about the exponent.

The chainID would also not completely work as sometimes the chainID is not the same as what is considered the human branded name for the chain or you would need to remove the additional numbers. e.g. I think bandchain has chainID "laozi-mainnet" and the human readable name would be Bandchain

True, good point! Then I guess we cannot do anything about this at the moment?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok I see, I guess this adr has not yet been implemented and right now its assumed that sdk coins all have 6 decimal places.

Yes I don't think there can be anything done about the chainIDs

}

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)
}

// OnAcknowledgementPacket responds to the the success or failure of a packet
// acknowledgement written on the receiving chain. If the acknowledgement
// was a success then nothing occurs. If the acknowledgement failed, then
Expand Down
3 changes: 3 additions & 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 @@ -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/cosmos-sdk/x/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
}

// ChannelKeeper defines the expected IBC channel keeper
Expand Down