Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi: Switch default coin types to SLIP0044. #1293

Merged
merged 1 commit into from
Jul 4, 2018

Conversation

markusrichter
Copy link
Contributor

Use the coin type numbers defined in SLIP0044:
https://github.com/satoshilabs/slips/blob/master/slip-0044.md

They are not going to change and dcrwallet already uses them.

@davecgh
Copy link
Member

davecgh commented Jun 13, 2018

I have previously suggested this (for mainnet, we shouldn't change testnet), but @jrick had some reservations with it. He can chime in for discussion.

@dajohi dajohi requested a review from jrick June 14, 2018 02:18
@jrick
Copy link
Member

jrick commented Jun 14, 2018

I'm not sure it's a good idea that we change the existing values of these fields. Wallet does have at test for it, to ensure behavior doesn't actually change, but not all applications will.

I also think that we need to have both the legacy and SLIP0044 coin types in the params. This would enable applications such as dcrwallet and dcraddrgen to use the values in the params themselves instead of hardcoding the legacy coin types.

My suggestion is to create a new field in the params, SLIP0044CoinType, with the new values, and leave the old values alone. Comments should be added to the old values so that implementers know if they are building a new application to use the SLIP0044 coin types instead.

@markusrichter
Copy link
Contributor Author

https://godoc.org/github.com/decred/dcrd/chaincfg?importers doesn't show any major open-source importers outside the Decred ecosystem, but I'm fine with adding a SLIP0044CoinType to the params and keeping HDCoinType untouched as the legacy coin type. What's your take @davecgh?

@davecgh
Copy link
Member

davecgh commented Jun 14, 2018

I don't have any qualms to keep the legacy coin type around, but I personally would prefer either updating the existing field (for mainnet) with the SLIP0044 value and introducing a new LegacyCoinType field to house the existing one, or even better renaming the existing field to LegacyCoinType and simultaneously introducing the new SLIP0044CoinType with the appropriate comments, of course.

I lean towards the latter because it will break the build of anyone who relies on it which will force them to make a choice as to which one they want to use as opposed to either just silently updating it in the case of the former, or having a bunch of code out there that isn't paying close attention and continuing inadvertently using the legacy coin type because they haven't noticed as I suspect would happen with @jrick's suggestion of leaving the existing one intact and just adding the new one.

@markusrichter
Copy link
Contributor Author

I like @davecgh's idea of introducing a LegacyCoinType field, breaking old code, and forcing consumers of the package to make a conscious decision. @jrick: Fine with you?

@jrick
Copy link
Member

jrick commented Jun 14, 2018

Breaking code is fine right now. Just keep in mind that a change like that won't be possible without a major version bump (and new package) once we fully support vgo.

Use the coin type numbers defined in SLIP0044:
https://github.com/satoshilabs/slips/blob/master/slip-0044.md

HDCoinType has been removed, to break old code on purpose.
If possible, the new SLIP0044CoinType should be used instead.
For backwards compatibility, the old HDCoinType values have been kept as
LegacyCoinType.
@markusrichter
Copy link
Contributor Author

I adjusted the code accordingly.

HDCoinType has been removed, to break old code on purpose.
If possible, the new SLIP0044CoinType should be used instead.
For backwards compatibility, the old HDCoinType values have been kept as
LegacyCoinType.

@davecgh davecgh merged commit e0d9a35 into decred:master Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants