Missing symbol before some ERC20 tokens name

[cryptomar1o]

Describe the bug For some ERC20 tokens, the symbol is missing before the token names.

Info:

• Suite Version: desktop 22.11.1 (https://github.com/trezor/trezor-suite/commit/73ccf723353e0575a64f58b2f5775d20f072311f)
• Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Safari/537.36
• OS: maOS Monterey
• Bridge: 2.0.27
• Firmware Version: Trezor Model One 1.11.2 regular

How to reproduce Steps to reproduce the behavior:

1. Launch the application Trezor Suite (both desktop and web version, behaviour is the very same)
2. Check on of the following ERC20 token (does not matter if the token has a balance or you add it manually) HEX Index Badger Nexo

Expected behavior The symbol is shown for those tokens before their name as for the other ERC20 tokens

Screenshots

[komret]

It's not a bug, it's a feature, implemented in this commit. const noName = !t.symbol || t.symbol.toLowerCase() === t.name?.toLowerCase(), i.e. do not show the symbol if it is the same as the name.

Is it confusing? Should I remove the behaviour?

[komret]

@sime From Slack: "Is it confusing? -> hard to say, there was just one feedback until now from our users…" https://satoshilabs.slack.com/archives/C01SXSHGK44/p1668758402923239

Please decide whether this behaviour should be changed, feel free to close the issue if not.

[sime]

It's not a bug, it's a feature, implemented in this commit. const noName = !t.symbol || t.symbol.toLowerCase() === t.name?.toLowerCase(), i.e. do not show the symbol if it is the same as the name.

Is it confusing? Should I remove the behaviour?

@gabrielKerekes @vladimirvolek

This code change has introduced into an ADA related PR. Any hesitations about allowing token name and token symbol to appear side by side if they are the same?

[slowbackspace]

I think that should have been cardano-specific change since we had no reason to change rendering of ethereum tokens => bug :disappointed:

That condition should be evaluated only in cardano right? @vladimirvolek

It was implemented because cardano tokens don’t always have a name/symbol. In such a case we are rendering only token fingerprint (unique identifier) and it didn’t make sense to render it twice as it is quite long (see screenshot, first token doesn’t have any name/symbol, second one does).

Should I open a PR? it should be rather quick fix to run that one line of code only in cardano, reverting rendering of ethereum tokens to the state before the cardano PR.

[komret]

Thanks @slowbackspace, the condition to not show the symbol (and the dash) is fine for other networks, too, but the second condition (if symbol is the same as name) should be Cardano-specific or omitted entirely. Also, the variable should be called noSymbol rather than noName, I believe.

[slowbackspace]

Thanks @slowbackspace, the condition to not show the symbol (and the dash) is fine for other networks, too, but the second condition (if symbol is the same as name) should be Cardano-specific or omitted entirely. Also, the variable should be called noSymbol rather than noName, I believe.

I agree the naming is confusing. Second condition is needed in cardano because name and symbol fields both fallback to asset fingerprint in case of missing token name/symbol (see example of the asset below) - don't ask why, I can't remember, but it was necessary if we did not want to rewrite half of the token management in suite which is really tailored only for ethereum tokens.

{
    "type": "BLOCKFROST",
    "name": "asset1cvmyrfrc7lpht2hcjwr9lulzyyjv27uxh3kcz0",
    "address": "6b8d07d69639e9413dd637a1a815a7323c69c86abbafb66dbfdb1aa7",
    "symbol": "asset1cvmyrfrc7lpht2hcjwr9lulzyyjv27uxh3kcz0",
    "balance": "1",
    "decimals": 0
}

The condition is a workaround to figure out that the cardano asset doesn't have a name (technically there is no symbol in cardano asset, at least not on chain, only optional name prop. I think that's why we called the variable noName 😅).

Renaming it to noSymbol and evaluating that 2nd condition only in cardano should solve things, so do you want me to make a PR or will you take care of it?

I am still not sure you need that first part (!t.symbol), IIRC ethereum tokens without symbol are stripped away during account creation, but let's keep it to not break anything.

https://github.com/trezor/trezor-suite/blob/b26e0891f4addcb155298dbc4d41ca8fe79d98db/suite-common/wallet-utils/src/accountUtils.ts#L383

[komret]

@slowbackspace Good, please create a PR and assign me.

[bosomt]

QA OK

Info:

• Suite version: web 22.12.0 (9eda6a34372ed709612b67c006f2602094d2c4c7)
• Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:107.0) Gecko/20100101 Firefox/107.0
• OS: MacIntel
• Screen: 1584x990
• Device: model T 2.5.3 Universal (revision 2f03ace311584988d5aeab58fd1acf24ef54711a)
• Transport: bridge 2.0.32