Fix/coin link - Githubissues

vuon9 commented 2 years ago

Currently, some links couldn't be open correctly on one (or both) Coingecko, Coinmarketcap websites:

Doge, Buff Doge Coin: the problem is about converting name to ID, also related to the old issue because the name of Doge (name: Dogecoin) is similar to the symbol of the Buff Doge Coin (symbol: DOGECOIN).
Travala.com: coingecko supports travala/travala.com slug but the CoinLink method is using a regex to replace non Alpha numeric chars to dash. So the original name is "Travala.com" and the converted name is "travala-com" -> unable to use. https://github.com/cointop-sh/cointop/blob/8b8db3bd13c366a9657434af71b61400e6b01ad9/pkg/api/util/util.go#L12-L15 shouldn't change the regex because it will impact to yearn.finance coin (it only works with "yearn-finance" slug on Coingecko).

Seems both Coingecko and Coinmarketcap supported slug, please see the below. This PR is about switching to use slug instead of name.

Coingecko supports id

{
    "id": "concierge-io",
    "symbol": "ava",
    "name": "Travala.com",
    "image": "https://assets.coingecko.com/coins/images/3014/large/Travala.png?1558020611",
    "current_price": 2.77,
    "market_cap": 147501823,
    "market_cap_rank": 411,
    "fully_diluted_valuation": null,
    "total_volume": 7569293,
    "high_24h": 2.8,
    "low_24h": 2.71,
    "price_change_24h": 0.01135146,
    "price_change_percentage_24h": 0.41202,
    "market_cap_change_24h": 9243.35,
    "market_cap_change_percentage_24h": 0.00627,
    "circulating_supply": 53373082,
    "total_supply": 61383832,
    "max_supply": null,
    "ath": 6.45,
    "ath_change_percentage": -57.11379,
    "ath_date": "2021-04-14T07:24:28.788Z",
    "atl": 0.01218947,
    "atl_change_percentage": 22576.29765,
    "atl_date": "2019-02-27T00:00:00.000Z",
    "roi": {
        "times": 6.9040071307770114,
        "currency": "usd",
        "percentage": 690.4007130777012
    },
    "last_updated": "2021-11-13T12:54:15.485Z"
}

it is equivalent to API id on the UI

Coinmarketcap supports slug

{
    "id": 1,
    "name": "Bitcoin",
    "symbol": "BTC",
    "slug": "bitcoin",
    "cmc_rank": 5,
    "num_market_pairs": 500,
    "circulating_supply": 16950100,
    "total_supply": 16950100,
    "max_supply": 21000000,
    "last_updated": "2018-06-02T22:51:28.209Z",
    "date_added": "2013-04-28T00:00:00.000Z",
    "tags": [
        "mineable"
    ],
    "platform": null,
    "quote": {
        "USD": {
            "price": 9283.92,
            "volume_24h": 7155680000,
            "volume_change_24h": -0.152774,
            "percent_change_1h": -0.152774,
            "percent_change_24h": 0.518894,
            "percent_change_7d": 0.986573,
            "market_cap": 852164659250.2758,
            "market_cap_dominance": 51,
            "fully_diluted_market_cap": 952835089431.14,
            "last_updated": "2018-08-09T22:53:32.000Z"
        },
        "BTC": {
            "price": 1,
            "volume_24h": 772012,
            "volume_change_24h": 0,
            "percent_change_1h": 0,
            "percent_change_24h": 0,
            "percent_change_7d": 0,
            "market_cap": 17024600,
            "market_cap_dominance": 12,
            "fully_diluted_market_cap": 952835089431.14,
            "last_updated": "2018-08-09T22:53:32.000Z"
        }
    }
}

lyricnz commented 2 years ago

I didn't even know about this openlink functionality! (though miguel referred to it, when we were talking about right-mouse buttons once).

Rather than adding slug to Coin, couldn't we pass the whole Coin to the api method, and let it use whatever it wants?

vuon9 commented 2 years ago

Rather than adding slug to Coin, couldn't we pass the whole Coin to the api method, and let it use whatever it wants?

Sorry, which api method?

lyricnz commented 2 years ago

I was suggesting that the interface method could be

CoinLink(coin Coin) string

Then the implementations could use whichever attribute(s) of Coin they felt like, and not have to pre-emptively squash it into a string called Slug.

coingecko.go: func (s *Service) CoinLink(coin Coin) string { return fmt.Sprintf("https://www.coingecko.com/en/coins/%s", coin.ID) }

coinmarketcap.go: func (s *Service) CoinLink(coin Coin) string { return fmt.Sprintf("https://www.coingecko.com/en/coins/%s", coin.ID) // ID? slug? }

(cmc currently uses v.Name for coin.ID which seems possibly wrong?)

vuon9 commented 2 years ago

Understood your point now. Actually, I came with it previously, but I have changed to the string for a simple & enough type as current. If we need more than 1 parameter for CoinLink, can switch to use the whole Coin pointer later. For now, I think that's fine to keep as string. In any way, we still have to store Slug value for at least Coinmarketcap, so I think a separated field is better than reuse ID field of Coin struct.

(cmc currently uses v.Name for coin.ID which seems possibly wrong?)

I don't have a conclusion because Coingecko and Coinmarketcap has different type for id field, so may be Coin struct can be free to define its own ID, as long as it still works well. I mean it could be change if we found the limitation.

lyricnz commented 2 years ago

(cmc currently uses v.Name for coin.ID which seems possibly wrong?)

I don't have a conclusion because Coingecko and Coinmarketcap has different type for id field, so may be Coin struct can be free to define its own ID, as long as it still works well. I mean it could be change if we found the limitation.

This just looks wrong to me:

        ret = append(ret, apitypes.Coin{
            ID:               util.FormatID(v.Name),
            Name:             util.FormatName(v.Name),
...

Especially when v.Name is not guaranteed to be unique?

vuon9 commented 2 years ago

I agree. I think this PR is tiny PR for just open link problem. The ID, I worry it relates to so many flows to be fixing. I created another issue for it https://github.com/cointop-sh/cointop/issues/279

lyricnz commented 2 years ago

(on CMC at least) needs a cache clear in order to work (structure of Coin changed). Can you think of a way we can detect that situation? (I think value is blank)
on CMC the URL is https://coinmarketcap.com/currencies/xxx which immediately redirects to https://coinmarketcap.com/currencies/xxx/ (trailing slash). Let's include the trailing slash in the url, to save the redirect, which will make it a little faster

FWIW on Coingecko it opens https://www.coingecko.com/en/coins/concierge-io which redirects to https://www.coingecko.com/en/coins/travala but I don't think we can do any better there

cointop-sh / cointop

Fix/coin link #275