No "Symbol" in Cosmos token metadata

[jtremback]

ERC20 has the following metadata fields (for example):

Name: Sommelier
Symbol: SOMM
Decimals: 18

Cosmos metadata looks like this:

{
    Description: "The native staking token of the Cosmos Hub.",
    DenomUnits: []*bank.DenomUnit{
        {Denom: "uatom", Exponent: uint32(0), Aliases: []string{"microatom"}},
        {Denom: "matom", Exponent: uint32(3), Aliases: []string{"milliatom"}},
        {Denom: "atom", Exponent: uint32(6), Aliases: []string{}},
    },
    Base:    "uatom",
    Display: "atom",
},

Our current strategy is simply to use the Display field as both Name and Symbol. It's unclear from this example whether Display corresponds to name or symbol, because in Atom's case, it is both the token's name, and is short enough to be used as it's symbol.

If Display is really a "symbol", then ERC20's deployed by Gravity will not have a nicely readable name. If Display is really a "name", then ERC20's deployed by Gravity could have a very long ERC20 symbol (for example "Sommelier"), possibly breaking interfaces of Ethereum tools that expect a short 3-4 letter symbol.

We need to discuss exactly what is going on in the Cosmos metadata.

[zmanian]

I think the intended mapping is Display is symbol and Name should be Description.

[jtremback]

In that example, is Description being used incorrectly?

It comes from here:

https://github.com/cosmos/gaia/pull/564/files

[jtremback]

I could also see one of the aliases for the denomUnit that corresponds to Display being the name

[fedekunze]

Something like this?

{
    Description: "The native staking token of the Sommelier Chain",
    DenomUnits: []*bank.DenomUnit{
        {Denom: "asomm", Exponent: uint32(0), Aliases: []string{"attosomm"}},
        {Denom: "somm", Exponent: uint32(18), Aliases: []string{"SOMM", "Sommelier"}},
    },
    Base:    "asomm",
    Display: "somm",
},

[fedekunze]

you can generalize it as follows:

{
    Description: name + " ERC20 token representation as a Cosmos Coin",
    DenomUnits: []*bank.DenomUnit{
        {Denom: SI prefix + lowercase(symbol), Exponent: uint32(0), Aliases: []string{SI prefix name + lowercase(symbol)}},
        {Denom: lowercase(symbol), Exponent: uint32(decimals), Aliases: []string{uppercase(symbol), name}},
    },
    Base:  SI prefix + lowercase(symbol),
    Display: lowercase(symbol),
},

[jtremback]

Thanks @fedekunze your last comment clears it up.

So I could write code that iterates through the DenomUnits[], then finds the one that has a Denom matching Display. Then I would pull the decimals out of Exponent, and pull the Symbol out of Aliases[0] and the Name out of Aliases[1].

This seems very brittle. For example, the current Atom metadata will not work with this procedure! If we used this, the Atom ERC20 will not have a name or a symbol. I could also see other blockchains messing this up for their tokens. I would need to write a fallback to handle those cases, probably by just using Display as both Name and Symbol as we do now.

But there are a lot of ways that it could be broken which are not easy for my code to detect. For example, someone could put the Name in Aliases[0] and the Symbol in Aliases[1]. For this reason, I think it's best not to even use this procedure. I think it's actually safest to just keep using Display as both Name and Symbol.

I can't understand why the Cosmos Metadata struct was written this way. I think it should be changed. Not just for Ethereum compatibility. The only things most interfaces on any blockchain care about are Name, Symbol, and Decimals. Nobody cares about the fact that 1/1000 of an Atom is called a milliatom and 1/1000 of a Sommelier is called a Sauvignon Blanc.

I think that Name, Symbol, and Decimals fields should be added to the bank.Metadata struct at the top level. For now, I will keep using Display as both Name and Symbol, since it seems like the safest, although slightly sub-optimal solution. I will also add a backdoor into the Gravity ERC20's so that deployed ERC20's can have their Name and Symbol updated when this issue is fixed on Cosmos.

[fedekunze]

I think that Name, Symbol, and Decimals fields should be added to the bank.Metadata struct at the top level.

I agree that the Name and Symbol fields should be added to the Metadata, I also think that we should rename Exponent to Decimals for consistency.

[jtremback]

I'm not a fan of the current procedure of iterating through DenomUnits to find the one matching Display, and then pulling the Exponents field off of that, but I think we confirmed previously that it will always work, right?

I think you said that the property that there is a DenomUnit matching Display is checked by validation somewhere else, right?

[fedekunze]

I think you said that the property that there is a DenomUnit matching Display is checked by validation somewhere else, right?

yeah it's checked on the validation. The base denom is always the first element and the display denom should (although not enforced) be the last element

[jtremback]

yeah it's checked on the validation. The base denom is always the first element and the display denom should (although not enforced) be the last element

But it is enforced that the display denom is present in the DenomUnits array, regardless of its position, correct?

[jtremback]

This has been fixed on Cosmos-SDK, but we need to load the new version into Gravity to start using it, and making it so that the Cosmos-originated asset logic looks at the name and symbol, instead of just the display field.

[jkilpatr]

@jtremback we are updated to the latest version of the SDK, this should be possible now.