bitcoinjs / bip44-constants

This package provides BIP44 coin constants as found here: https://github.com/satoshilabs/slips/blob/master/slip-0044.md
MIT License
88 stars 63 forks source link

Index of coins is not matching the original source (index is missing from the dataset) #51

Closed slavafomin closed 3 years ago

slavafomin commented 3 years ago

Hello!

According to the SLIP-0044 list, the Binance (BNB) "coin type" is 714, however, this library lists it as 633.

Is this an error? Should these indices be consistent?

Thanks.

junderw commented 3 years ago

It's 714 here too. Update to latest version. Every update is now a major version update, so just npm installing will not pull the latest version by default.

You need to reinstall using: npm install --save bip44-constants@latest https://github.com/bitcoinjs/bip44-constants/blob/4bd7592dcc7df38887141f52d8be370bf81ea0c0/index.js#L637

junderw commented 3 years ago

This library is automated so that every day at 2am ish UTC, we pull the latest master branch of SLIP44 and publish it.

junderw commented 3 years ago

It was 02:42 UTC https://github.com/bitcoinjs/bip44-constants/blob/4bd7592dcc7df38887141f52d8be370bf81ea0c0/.github/workflows/npmpublish.yml#L1-L34

slavafomin commented 3 years ago

@junderw That's strange. I'm using the latest version and BNB has index of 633, not 714.

714 is $DAG.

slavafomin commented 3 years ago

Here's the runtime debugging of the update.js:

Screenshot_2021-08-03_02-22-07

slavafomin commented 3 years ago

screenshot-github com-2021 08 03-02_26_22

Looks like the problem is that you are removing "empty" rows therefore breaking the indexing. I don't think that it can be easily fixed due to the fact that every change would be a breaking one: you either need to introduce another value (real index) to the entries or to add empty entries to the array. I think the latter would be a better option. What do you think?

The culprit: https://github.com/bitcoinjs/bip44-constants/blob/c622b33fa8b012dcd87eda583d004ae444d3ac42/update.js#L20

slavafomin commented 3 years ago

Actually, I think the breaking change is necessary here. If you would look into the last entries of the table you will see that index is not increasing monotonically: screenshot-github com-2021 08 03-02_43_38

Looks like it was a poor design decision to omit the index from the list in the first place and it needs to be added back (therefore the breaking change).

Using the opportunity, I would suggest to re-implement the parsing logic and maybe improve the library itself so it would provide a more convenient API and the TypeScript support. It could be released under a new name, e.g.: bip44-constants-v2 in order to make clear that this is indeed a breaking change (and not a simple update). I can make a new version if you'd like.

junderw commented 3 years ago

I am confused.

What is the point of using this library if you already have 714 hard coded in your app?

The point of this library is "I don't know what the SLIP44 number for BNB is... I will load this library and search for it using filter..."

If your app has const BNB_INDEX = 714; const myNumber = bip44Constants[BNB_INDEX]; then why do you even need bip44Constants? You have the 714 number.

The empty rows are useless data.


I think you are over-thinking this. You have a fundamental misunderstanding of what this library is for. If you read the usage example in the README you can see how it is intended to be used.

slavafomin commented 3 years ago

Actually, I don't have a hardcoded index in my app. I've noticed the discrepancy accidentally. I'm not sure where this index is used, but having inconsistent data between the two sources is not really cool in my book :)

junderw commented 3 years ago
  1. The point of this library is to find the number of the coin.
  2. Your issue is "the index of the array doesn't match the number of the coin"
  3. Which implies you already know the index and are trying to use it to get the coin...? But you already have the number if you are assuming the index should equal the number and you already have the index.

Maybe this would be easier to understand your issue if you could post your code?

slavafomin commented 3 years ago

By the way, after looking into the usage example, I can see that there is no guarantees regarding the index of the entries, so there is no actual harm or contract breaking. However, the fact that there is no way to get the correct index from the provided dataset could be an issue for someone.

I'm not using the index of the coin, so my use case is not affected by it. I've just noticed the discrepancy and decided to let you know about it :)