search

trezor / definitions

1 stars 5 forks source link

Grdddj/coins details #8

Closed grdddj closed 1 year ago

grdddj commented 1 year ago

Fixes https://github.com/trezor/definitions/issues/7:

matejcik commented 1 year ago

we can move wallets.json but everything else is used in firmware generation so that's a no-go. instead we probably want to add the trezor-common submodule again

matejcik commented 1 year ago

ACK on the coins_details.py changes

grdddj commented 1 year ago

we can move wallets.json but everything else is used in firmware generation so that's a no-go. instead we probably want to add the trezor-common submodule again

So we would keep just the simplified versions of coin_info.py and coins_details.py here and would have the trezor-common submodule for access to the defs directory, and we would be using the local ethereum-lists instead of defs/ethereum-lists?

Seems alright to me, but cannot we still delete at least coins_details.py and the coinmarket things from trezor-common, or will we keep using it there?

grdddj commented 1 year ago

So we would keep just the simplified versions of coin_info.py and coins_details.py here and would have the trezor-common submodule for access to the defs directory, and we would be using the local ethereum-lists instead of defs/ethereum-lists?

Done like so in the force-push, with do_coins_details.sh script in the root

matejcik commented 1 year ago

Seems alright to me, but cannot we still delete at least coins_details.py and the coinmarket things from trezor-common, or will we keep using it there?

coins_details yes. coinmarketcap no because it's used by some other script that calculates fee limits

So we would keep just the simplified versions of coin_info.py and coins_details.py here and would have the trezor-common submodule for access to the defs directory, and we would be using the local ethereum-lists instead of defs/ethereum-lists?

exactly -- defs/ethereum-lists will not exist there soon so we need the local version

matejcik commented 1 year ago

can't we use coin_info.py from trezor-common though? seems more robust in case the format of things in defs/ changes

grdddj commented 1 year ago

can't we use coin_info.py from trezor-common though? seems more robust in case the format of things in defs/ changes

Unfortunately, we cannot (easily), because the functions _load_ethereum_networks and _load_erc20_tokens are no longer there, they are replaced by _load_builtin_ethereum_networks and *builtin*tokens, which do not do what we want.

We could probably do some monkey-patching of these functions or define our own collect_coin_info (and then coin_info_with_duplicates), but it seems too much hacky for me

matejcik commented 1 year ago

uhmm but we should be taking the eth/erc20 info from definitions-latest and not from trezor-common. in fact you can completely ignore eth and erc20 categories coming from coin_info

matejcik commented 1 year ago

...for that matter, if we have trezor-common submodule, we could use coin_info to get the list of builtins instead of downloading it, right?

grdddj commented 1 year ago

...for that matter, if we have trezor-common submodule, we could use coin_info to get the list of builtins instead of downloading it, right?

Correct, good point. Done in cc2a6bb

grdddj commented 1 year ago

Turned out I had a bug in the command updating the submodules - they would never be updated this way :)

Fixed in 6b795e8

Currently, I am unable to finish downloading the new definitions, as the connection to coingecko is unreliable for me - tried two times with the same results (script taking a couple of minutes and then throwing this error):

requests.exceptions.RetryError: HTTPSConnectionPool(host='api.coingecko.com', port=443): Max retries exceeded with url: /api/v3/coins/markets?order=market_cap_desc&page=1&per_page=100&sparkline=false&vs_currency=usd (Caused by ResponseError('too many 502 error responses'))

Also, updating the trezor-common submodule broke the coins_details.py (deleted coins_details.override.json, modified support.json). Because of the support.json, where most of the coins are missing now, we are getting only around 70 coins, the rest is ignored in check_missing_data because they are neither t1_enabled nor t2_enabled. Trying to fix it somehow

matejcik commented 1 year ago

(script taking a couple of minutes and then throwing this error):

increase delays between requests?

the rest is ignored in check_missing_data because they are neither t1_enabled nor t2_enabled.

I went briefly through the code and you don't seem to be using definitions-latest at all? not sure if that was unclear before, but let me repeat: eth and erc20 data from trezor-common should be completely ignored, and instead filled with local data from definitions-latest. you can just hardcode t1_enabled and t2_enabled to true for this local data.

grdddj commented 1 year ago

increase delays between requests?

Will try

I went briefly through the code and you don't seem to be using definitions-latest at all? not sure if that was unclear before, but let me repeat: eth and erc20 data from trezor-common should be completely ignored, and instead filled with local data from definitions-latest. you can just hardcode t1_enabled and t2_enabled to true for this local data.

That's right, I do not use definitions-latest.json during coins_details.json update, I take all the ETH data from local ethereum-lists/{chains,tokens} (and do not take any ETH data from trezor-common). We probably need to read from ethereum-lists because of all the links and wallet data. It is a good idea to enforce t1_enabled = t2_enabled = True for all the things from definitions-latest.json, but there should be the same coins as we get from ethereum-lists. Will try it

matejcik commented 1 year ago

but there should be the same coins as we get from ethereum-lists

no, because you're not doing the "fetch buttload of tokens from coingecko" step that way :) and yes, we want to output, at least for now, the coingecko tokens as part of coins-details.

our definitions-latest doesn't have homepage links, i'll check if we need them, hopefully not. github entry we can ignore wallets, we can hardcode mew + mycrypto (+ metamask?) as before, plus anything coming from wallets.json

one thing unclear is how to handle "same token on different networks", because our data does not have that linkage iirc. for now let's do it the easy way and consider these as separate entries.

(i don't remember whether coins-details is a list or a dict? if it is a dict, we can make it a list to simplify the matter of unique keys)

matejcik commented 1 year ago

it looks like coingecko was under a DDoS yesterday and the errors you see might be countermeasures. it's possible that we'll need to wait longer for this to work again

Hannsek commented 1 year ago

Can it be related to this: https://satoshilabs.slack.com/archives/C03QP26JEN8/p1679769069464689 ?

matejcik commented 1 year ago

our definitions-latest doesn't have homepage links, i'll check if we need them, hopefully not.

Confirmed that we don't, so you can remove the "links" section too. I don't think we need "type" either. we still need name, shortcut, t1_enabled, t2_enabled, wallet.

grdddj commented 1 year ago

The suggestions above were applied in recent commits.

There are currently more than 12700 entries in coins_details.json.

It is a dict, and it seems the issue of "same token on different networks" is being handled by the construction of the keys into this dict - erc20:{CHAIN}:{SHORTCUT} - so the same tokens on different networks (CHAIN) are there once for each network.

Some questions:

matejcik commented 1 year ago

Some questions:

not sure about answers, i'll find out.

matejcik commented 1 year ago
  • generally not sure if coins_details.json has correct data now, we could share the current state with somebody who can validate it

the format is confirmed ok

  • should we include all the testnet networks? in the old code some of them were filtered out (those with Testnet in the name), but some others were still included (like Ropsten or Rinkeby)
  • currently all the ETH have wallet=WALLETS_ETH_3RDPARTY- do we want to include Suite into it as well?

let's ignore these issue for this iteration and see what comes up. keep in mind that half the processing is now happening elsewhere so they might do their own filtering?

grdddj commented 1 year ago
  • generally not sure if coins_details.json has correct data now, we could share the current state with somebody who can validate it

the format is confirmed ok

  • should we include all the testnet networks? in the old code some of them were filtered out (those with Testnet in the name), but some others were still included (like Ropsten or Rinkeby)
  • currently all the ETH have wallet=WALLETS_ETH_3RDPARTY- do we want to include Suite into it as well?

let's ignore these issue for this iteration and see what comes up. keep in mind that half the processing is now happening elsewhere so they might do their own filtering?

Thanks for the verification. Seems coingecko API is alright now, I was able to call the ./do_update.sh and generate the most up-to-date versions of definitions and coins-details. We have around 12850 coins currently

Hannsek commented 1 year ago

should we include all the testnet networks? in the old code some of them were filtered out (those with Testnet in the name), but some others were still included (like Ropsten or Rinkeby) Do not include all testnet networks. currently all the ETH have wallet=WALLETS_ETH_3RDPARTY- do we want to include Suite into it as well? Yes, please include Suite.

STew790 commented 1 year ago

Approving after discussion with Jirka and eshop team.