Open scscgit opened 5 years ago
iancoleman
commented
5 years ago Thanks for this.
It stems from #371
I'm think I know how to fix this but unfortunately can't do it until a week from now. @immae are you able to look into this in the meantime?
Looks like an event needs to be triggered which calls calcForDerivationPath but that's just a guess.
iancoleman
commented
5 years ago Considering the majority of wallets do follow bip44 for ethereum, I've reverted the changes so ethereum uses change as 0 instead of blank. This has been released as v0.3.14. This has been a source of great confusion in the past (search issues in this repo for ledger or ethereum). https://github.com/iancoleman/bip39/commit/55367b989e026899c4afb8cf0fbb5dc0f60d6786
Thanks for reporting this and including the details to make a suitable decision. Also a good reminder to apply greater care around breaking backwards compatibility in the future. I prefer to merge-and-fix rather than turn PRs away, but in this case seems like more care was needed. Thanks again.
immae
commented
5 years ago Sorry I missed the initial mention from github. I’ll make a PR where the default behavior is to have a "0" as it seems the majority of clients have it.
All my wallets have the empty string as default: ledger and coinomi, so I thought it was quite generic.
scscgit
commented
5 years ago I noticed you've completely removed the Default button, I think it'd be better to instead just let it also update the generated addresses. I don't like to see such UX-friendly features completely disappear :) One of minor implications is that now the "External / Internal" value doesn't update when a user changes the selected coin. It also persists between page refreshes, which I believe is also unintentional (e.g. Coin changes back to BTC, but the value stays empty).
Also, I don't know what's your policy when it comes to backwards compatibility, but I believe it wouldn't hurt to add some small red warning around this specific field, specifically for ETH (and possibly for some other coins in the future). There are two separate concerns:
Nevertheless, thank you very much for taking care of this issue :) This is literally like the only proper tool for key derivation which doesn't rely on black-box generation of entropy (like "online paper wallets"), and I'm really surprised that noone in the ETH community noticed this issue yet :)
immae
commented
5 years ago @scscgit : I would prefer not to put back the default, but I understand your concern.
What do you think?
iancoleman
commented
5 years ago Since the change field should only be one of a few different values, how about radiobuttons instead of a numeric input?
immae
commented
5 years ago I would prefer not to limit the possible input because of the current conventions, but it’s just a matter of preference so why not for radiobutton
scscgit
commented
5 years ago I'm fine with any Default button replacement, as long as the value gets refreshed in aforementioned scenarios (refresh/coin select) and it supports a custom value (which wouldn't apply for radiobuttons, though there is the BIP32 option, so I guess it's okay if the BIP32/BIP44 functionality stays equivalent).
When it comes to the time interval of the hotfix, I believe that even one day could prove fatal to someone who's unlucky, and few weeks are plenty of time. In general, I think people should uphold some explicit rule when it comes to backwards compatibility - e.g. if I made some wallets 2 weeks ago, I'd appreciate an information like "remember the derivation path along with your seed, it may change in the future" unless there's an opposite implicit guarantee. Nevertheless, it's all a design choice, so anything goes if you're transparent about it :)
But in this scenario it's quite straightforward - it's not only about losing wallets generated by this tool, but it's also about importing seeds from third parties, thus you really should add some warning even if only meant for those non-standard Ledger users. with a blank change. Personally I'd expect some non-intrusive notification under "Derived Addresses", as that's where users who don't know the meaning of "change" are supposed to go (and who would blame them - can anyone define a semantical difference between blank and zero? :P).
iancoleman
commented
5 years ago Sure, I can understand wanting to not limit with radibuttons. But the bip44 standard says only 0 or 1 is accepted. Since this is an altcoin it's up to the community (ie you two at the moment, maybe others in the ethereum community can assist) to negotiate the appropriate solution without screwing up the implementation of the actual bip44 standard on the bip44 tab.
The bip44 tab should conform to the bip44 standard (already ledger has messed that up with no change in derivation path but still calling it bip44).
If a wallet isn't following bip44 standard (ie what ledger does) users should probably be using the bip32 tab with path of m/44'/... and accept the complexity of doing so. A helpful message in this tool might be good, but why pollute this tool and make it harder to use for the majority of users because some minority didn't follow standards? That's where documentation and stackexchange and reddit are good solutions to the problem.
It's not right for me to say 'one way or the other'. This is a community (ethereum community) problem and I'm happy for the community to implement the solution they think is best. I can provide suggestions but I can't decide what's right or not when it comes to non-standard implementations. If I absolutely had to say something I'd say 'use bip32 tab and no changes to bip44 tab'.
On the 'default' label for the button... that word doesn't really capture the purpose. Is '0' or '1' default? They both are according to bip44 spec. So maybe it should be labelled 'internal/external'? Or should it be labelled 'blank' since that's the exception being catered for? Or maybe 'ledger' since that's the use case it's really solving? Or maybe 'ethereum-based ledger wallets' since it's only for ethereum-based coins? Or 'no change'? I think a button is the wrong solution.
immae
commented
5 years ago Ok, then I suggest that we implement this radio button feature.
Considering that there are some popular clients that don’t follow closely the BIP 44 standard (Ledger and Coinomi to name them), and that I messed up probably some users for a few weeks, we should add the "null" option to the radio button, together with a warning like "null is not BIP 44 standard but some tools use this value" which helps user find their funds coming from those clients
NB: even though I know about this subtelty, I always spend a lot of time figuring out the correct derivation I have to write in the BIP 32 tab (and that’s why I developped that "default" button for myself in the BIP 44 tab), and I only know I got it right because I know my first address pattern by heart (because of that issue :p ).
Do we all (three) agree and shall I add that button?
scscgit
commented
5 years ago I will personally disagree about there being no default, as the most important use-case is always to simply derive "the wallet" from a seed, and that's the external one (unless there's a different de-facto standard for that specific coin). And I am against forcing any users to "search through issues", as critical information like changes of a default, or a non-standard usage of BIP44 in major clients, should be mentioned somewhere visible. It's completely illogical to expect users to "know the right question to ask". With more coins being added every day, I think that it's important to come up with some sort of coin-specific guideline, e.g. it would be totally enough to just add some coin-specific link to a markdown page (or at least any external page, like GitHub wiki if you want to keep the tool simple) below the Coin selection. Even a simple name like FAQ would be totally user friendly and intuitive, or Troubleshooting may be even better, just hide it unless the coin is ETH. Alternatively, there could be a "Couldn't find your old key using this seed?" link above Derived Addresses.
As long as the page mentions proper defaults of major clients, a history of changes which broke backwards compatibility, and points out to anything which isn't intuitive or breaks the standard, it should be easy to use and manage (people can update it along with any coin-specific pull request).
The button to reset the default change is irrelevant though, as a "global reset" is sufficient - e.g. you could simply switch the coin (though this may not be needed anymore) or refresh a page (I strongly believe that values like account/change should reset!), or even switch the BIP44 tab back and forth - all of those methods are still broken, and there is no other reset option, which should be fixed asap! :)
Also there's currently a huge bug: using a null value of change derives it as a zero :) I feel so bad for practically causing this by my issue :P
wigy-opensource-developer
commented
5 years ago As a wallet developer, I always come to this tool to get inspired about how many fringe use-cases exist for an hd-wallet. I really love how I can learn from its source code. BUT! I can understand that the user-base of this tool contains mostly non-technical users who try to fix problems they faced because of what other wallet implementations screwed up.
I know this comment is mostly off-topic, but slowly this tool could become an educational tool about all the options that exist in the wild in different hd-wallet implementations. In that sense, this FAQ and troubleshooting button seems to be a very good idea.
scscgit
commented
5 years ago ^ [also off-topic] Exactly, I'm glad others share my opinion. I don't know any other tool that's nearly as good e.g. to manage multiple wallets from a sandboxed environment. Even though it's intended to be just a small tool, I believe that it now slowly gains even the responsibility to teach other developers :) By putting a proper framework in place, it would be easy for experts to seamlessly transfer their critical knowledge. The documentation that's out there is seriously lacking when it comes to extending anything. Personally when I tried to create wallets for some dead altcoins which I couldn't compile thanks to undocumented dependencies, I found literally nothing about bip32 public/private, pubKeyHash, scriptHash, wif... And there is no initiative in teaching people how to do secure stuff which should be common sense, like signing your own transaction offline and never exposing the seed to any tool directly, or properly generating the entropy (my idea would be to XOR multiple providers). But at least the today's semi-cloud wallets can put a large "Best Security" stamps on their reputable products... :)
iancoleman
commented
4 years ago a simple name like FAQ would be totally user friendly and intuitive, or Troubleshooting may be even better, just hide it unless the coin is ETH. Alternatively, there could be a "Couldn't find your old key using this seed?" link above Derived Addresses.
This sounds good. There's already a substantial educational section at the bottom of the tool, so I'd gladly add some troubleshooting / historical explanation for this plus a link to it where it's needed.
Where do you think the link "Couldn't find your old key using this seed" would be best positioned? I guess maybe directly above the first address in the table? That seems like the place where users are going to first discover something is not matching.
I have one unrelated feature request ... I would love to see all the fields exposed
Sounds good, please open a separate issue to track that feature.
On the broader topic of backward compatibility and breaking changes, it seems the issue is that there's no clear process for accepting changes to new coins. I can manage bitcoin changes easily, but can't manage altcoins. Do you have suggestions for how to reasonably accept altcoin changes? I was thinking maybe an ACK from a second user before merging. It's far from bulletproof but it's a simple step forward from the current ad-hoc merge policy.
scscgit
commented
4 years ago Any place under derived addresses seems fine to me. When it comes to making it universal, maybe you could add an optional date field like "lastChange" to any coin, which would display something like "The default values of this coin have changed since 10.12.2019, make sure the derivation path is correct if this is an old backup" after the user clicks on "Couldn't find your old key using this seed?". This could be further extended to include any implementation errors, changes of standard, and client upgrades on a per-coin basis. I could also imagine additional UX improvements like turning the changelog into GitHub commit download (or tree) links. (It'd be great if someone else added their opinion so we won't miss something.)
This would directly compensate the issue of wrong pull requests, so it would save you a bit of stress. But in my opinion there's a worse underlying issue with altcoins. As I indirectly mentioned in my feature request, the implementation is currently too much of a black-box - when it comes to integrating new altcoins, I only know of two issues (please correct me if there's something more, I'm not an expert here):
I believe the integration process itself is faulty if it depends on your judgement, because cryptocurrencies should be transparent by definition (people in PR could make this effort instead of you). Thus all the parameters should directly cite some source, either a documentation/white-paper, or a source code. As far as I know this never happens (which further promotes the idea that it's supposed to be just implicit). To me personally, that's the main information I'd be looking for whenever I generate any wallet, because I want to double-check that it's correct (except BTC and ETH, which I trust). On a similar note, when there's an unsupported coin, that's the first step I would take to add it myself. But a "universal (generate/receive only) wallet" is just a dream of mine at the moment, because there's nothing except your tool that even remotely resembles this - they all have some very specific subset of altcoins :)
I think it's a bit hard to separate the issues at the moment (default values, backwards/forward compatibility, new altcoins, altcoin parameters, FAQ guidelines for users vs. developers), so I think it'll be better to consolidate this before I open a "WIF/base58/bip32 guideline+editable fields"-specific issue :)
The "External / Internal" field defining Change value of BIP44 has a Default button, which properly updates the value, but doesn't refresh the Derived Addresses, so that users will falsely trust the stale, incorrect generated values.
I would also like to make sure whether the default value of no value "" in ETH is intentional, as it seems to be incompatible with many major clients, including myetherwallet.com. When I tried eth.mom, I was told that the derivation path containing 0 is used in Jaxx, Metamask, Exodus, imToken, TREZOR (ETH) & Digital Bitbox, whereas the path containing no value is only used by Ledger. This could cause huge issues for new users who use this tool directly, e.g. to derive cold storage wallets.