barnumbirr / coinmarketcap

A python wrapper around the https://coinmarketcap.com API.
Apache License 2.0
437 stars 110 forks source link

Accept currency symbol input #33

Open barnumbirr opened 6 years ago

barnumbirr commented 6 years ago

See https://github.com/mrsmn/coinmarketcap/issues/25.

ghost commented 6 years ago

Hi @mrsmn ,

I have addressed this in my implementation. Would you like to see my solution?

barnumbirr commented 6 years ago

Hey @timfi,

I'll gladly take a look. I'm not a 100% settled on the idea yet as it's not really part of the original API but I wouldn't mind getting other people's input.

Cheers.

ghost commented 6 years ago

Hi @mrsmn,

I mocked something up, I'm pretty new to python so might have some weird style, but it works. You can comment "symbol_id_list()" after the first run, but I want to make sure my script doesn't miss any new coins and it doesn't run that often.

https://gist.github.com/timfi/f6a8dd7489ea4f138da21a2a1fa14d6c

barnumbirr commented 6 years ago

Hey,

thanks for sharing this. It's indeed what I was afraid of: there's not really an elegant way of solving this. (and don't worry about your python codestyle it looks perfectly fine to me 😄)

I don't know if including this in the coinmarketcap library would do it any good: people that really want to be able to input currency symbols can always come up with code like yours to do so.

What do @JacobHepworth, @404-brain and @benibienz think?

Cheers.

ghost commented 6 years ago

thanks @mrsmn :)

This is indeed the best way I could think of. The only really elegant solution would be if coinmarketcap changes their API. The ever changing cryptomarket doesn't help either..

JacobHepworth commented 6 years ago

The only alternative I can think of would be a scraper but I'm not familiar with CMC's terms of service.

jathpala commented 6 years ago

Have a look at jathpala/coinmarketcap@5a31d20d72e1fca5996901bee999775ea2046a22. It's conceptually very similar to what @timfi has mocked up.

This adds a 'symbol' option to the ticker method. If a symbol is provided but currency is not then then the currency id will be looked up and used. It does this by storing a json dictionary of symbol to currency mappings in a 'symbols.json' data file. This data file can be generated automatically using a provided 'get_symbols' script (the resulting file has to be manually moved to the correct location).

The symbol lookup file is designed to be a static data file distributed with the package. It is not expected that library users would generate this file themselves. In order to support new currencies, the 'get_symbols' script would have to be run by you and a new version of the library with an updated 'symbols.json' file pushed out as a new release. Probably this would only need to be done every few months so I think this is a reasonable way of doing this.

I really don't know much about python packaging so I'm not sure where exactly the data file should live within the source tree and what path should be used to reference it so I may have gotten that bit wrong. I've had a bit of a play and the way I've done it seems to work on my system but I think a bit more testing is required.

Take a look at the diff linked and tell me what you think. If it makes it easier, I am happy to submit it as a pull request but I thought it was worth getting a bit of feedback on the overall design first anyway.

EDIT: Done a bit more testing. Updated the setup information to make sure the data file is packaged. I think it works now. (see jathpala/coinmarketcap@63c9a6b66b6a9980fab382a1077843d3380b194a)

I think this works correctly now. Happy to open a pull request if people like this solution.

benibienz commented 6 years ago

Storing the mappings is inelegant but I think an acceptable solution until CMC changes their API (if ever). Thanks.

jathpala commented 6 years ago

Yeah, I agree it isn't perfect. Unfortunately I think the only truly clean way of doing this is if the API supported using symbols directly. Without that, the mappings need to be stored and looked up, with the three options I see being:

  1. Store it in a static file distributed in the code (this solution)
  2. Include it directly in the source code, maybe in a separate file that can be included.
  3. Provide a function for the user to retrieve the data when they want from CMC

Option 2 might be slightly faster because it avoids the overhead of loading and parsing the json but the script that generated the file would be slightly more complex to generate python code rather than just the json. Realistically the very small performance gain is probably not worth it and simple is always better than complex.

Option 3 has its merits but it would make it more complicated for people using the library as they would have to remember to download the data themselves before using symbols. The code would then get a bit more complex in terms of where is that data stored, how do we know when to use the cached version rather than get fresh data, etc. Again I think more complexity for not much meaningful gain.

The only performance optimisation it may be worth considering is maybe only loading the json data when first using the symbol option. That way, people who are happy to only use the id never have to worry about loading and parsing the json data file. Again, slight added complexity (but not much). Probably not a meaningful performance gain.

gosuto-inzasheru commented 6 years ago

I think the biggest hurdle for cmc to integrate this is that there are several coins with the same symbol. id is always unique whereas symbol is not.

barnumbirr commented 6 years ago

Hey everyone,

I've been quite busy IRL over the last couple of weeks. I'll look into this when I have some spare time.

Cheers.

jathpala commented 6 years ago

Good point about multiple coins using the same symbol @jorijnsmit, I hadn't realised that occured. Unfortunately there's no way to deal with that elegantly without coinmarketcap.com eliminating duplicates on their end.

I still think it's worth implementing symbol lookup because I think many users of this library would need to do such a symbol to id conversion and would just need to do it in their own code if it wasn't in the library.

Still, it's probably not good to silently ignore symbol collisions so I've modified the get_symbols script to detect and exclude such symbols from the lookup dictionary. Now, coins which share a symbol can no longer be looked up by symbol and must be looked up by id only. The file collisions.log (which I move into the docs directory) contains a list of all collisions.

For the record the current list is:

  ACC: adcoin, accelerator-network, acchain
  BET: dao-casino, betacoin
  BLT: bloomtoken, bitcoin-lightning
  BLZ: bluzelle, blazecoin
  BTG: bitcoin-gold, bitgem
  BTM: bytom, bitmark
  CAN: canyacoin, content-and-ad-network
  CAT: bitclave, blockcat, catcoin
  CMS: comsa-eth, comsa-xem
  CMT: cybermiles, comet
  CPC: capricoin, cpchain
  ENT: eternity, entcash
  ETT: encryptotel, encryptotel-eth
  GCC: thegcccoin, guccionecoin
  GTC: game, global-tour-coin
  HMC: harmonycoin-hmc, health-mutual-society
  HNC: helleniccoin, huncoin
  ICN: iconomi, icoin
  KNC: kyber-network, kingn-coin
  MAG: magnet, maggie
  NET: nimiq, netcoin
  PUT: profile-utility-token, putincoin
  QBT: qbao, cubits
  RCN: ripio-credit-network, rcoin
  RMC: russian-mining-coin, remicoin
  WIC: waykichain, wi-coin
  XID: sphre-air, international-diamond
  XIN: mixin, infinity-economics
 BTCS: bitcoin-scrypt, bitcoin-silver
 CASH: cashcoin, cash-poker-pro
 FAIR: faircoin, fairgame
 LBTC: litebitcoin, lightning-bitcoin
 RUPX: rupaya, rupaya-old
SMART: smartcash, smartbillions

For now the responsibility is on the user to be aware of the coins on this list and only look them up by id.

As you can see though, there are a couple of reasonably popular coins on that list including bitcoin-gold, kyber-network and iconomi. There are a few ways to handle this issues: 1) Be consistent on collisions and leave it as is. This prevents lookup of those symbols and the coins can only be looked up by id. 2) Hardcode the preferred lookup for those symbols. For example we could hardcode (in get_symbols.py) that btg should refer to bitcoin-gold. Of course here you would be making a judgement call about the "correct" coin for a symbol. 3) Assign whichever coin has the highest ranking on cmc (i.e. market cap) to the symbol. This should generally lead to the desired effect for popular coins (e.g bitcoin-gold vs bitgem) but may not be desired where both coins are relatively obscure (e.g waykichain vs wi-coin).

I know this is starting to get a little messy but I think it's still manageable. As I said, I do think that if we didn't include it in the library, then the majority of users of the library would probably need to implement similar functionality in their application code.

firepol commented 6 years ago

Hi guys, FYI I created a repo that queries the API by symbol (yet another one, since I read somebody else did something similar). I'm not a python expert, I wanted to play around:

https://github.com/firepol/coinmarketcap-helper

My implementation is similar to @jathpala and I also included a logic for those symbols not found in coinmarketcap e.g. MIOTA (IOTA), YOYO (YOYOW) and I'm sure there are many more, for those I think I'll create a script soon or later instead of adding them manually.

Feel free to have a look ;)

barnumbirr commented 6 years ago

Hello all,

sorry for being MIA for so long (so much stuff to take care of IRL). I think changes to coinmarketcap's public API render this discussion moot. IMHO we can close this issue but before we do I'd like to get people's opinion.

EDIT: I've just released v.5.0.0 of the library to reflect the changes to the API.

Cheers.

firepol commented 6 years ago

Hi @barnumbirr, welcome back!

I think that we have the same problem... we still need a way to map normal symbols such as "BTC", "IOTA", "ETH", "BTG" etc. to the "numeric id" needed by coinmarketcap (which I think it's bad API design, but whatever... I guess we have to live with that).

So with v2 instead of having a dictionary of {'symbol' 'old-coinmarketcap-id'} as a text similar to the cryptocurrency name, now we need {'symbol': 'coinmarketcap-id-as-number'}.

The idea behind this discussion is to have a system to allow to lookup coinmarketcap data by cryptocurrency symbol. I think it's still totally valid.

@jathpala implementation is very good. My implementation has also an idea behind, with the "alias" (for those cryptocurrencies where the known symbol is different than the symbol on coinmarketcap, e.g. see "IOTA", coinmarketcap calls it "MIOTA").

I think for @jathpala, updating his PR to make it work with v2 should not be too complicated. Still a valid PR on my opinion.

In my implementation I handle collisions differently: if a symbol has more matching cryptocurrencies I return a list with all these cryptocurrencies sorted by rank, then it's up to the user to decide what to do: take the first one or use some additional logic to find the exact id...

If you have time to have a look at my repo, feel free to use anything you may find useful. Not sure I will have much time to work on this in the upcoming weeks, but if nobody does anything I'll be up to improve @jathpala 's PR and add also my alias concept.

jathpala commented 6 years ago

Hi,

Sorry for the delayed reply. Yes I agree this issue is now outdated with the changes to the CMC API. I might try to update my code to work with the V2 API but I probably won't have the time for it any time soon. If someone else wanted to give it a go that'd be great.

plgonzalezrx8 commented 5 years ago

THis should be a non-issue. You can use the API to get all the market data, and then write your own functions and methods to get the data you want using the JSON module.