-
Notifications
You must be signed in to change notification settings - Fork 60
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
Standard vectors fix #18
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: maciejhirsz <maciej.hirsz@gmail.com>
So a Japanese and English mnemonic for the same entropy results in a different seed based on BIP39. That I overlooked as well. Thanks. |
For non-English (English I assume is ok since it's all ASCII anyway) phrases we should really look into encoding the phrases correctly to UTF-8 NKFD (#1). |
This should be ready to merge in, I'll create a second PR for nfkd since it is out of scope. We can discuss changes there since there are some performance hits. |
@steveatinfincia Could you please have a look and consider merging this PR to v0.6, which includes the failing tests in #17 plus the fixes in the seed generation? |
@steveatinfincia Any chance to review this yet? |
Okay, this PR went a bit stale. I keep this fork up-to-date with the fixes for now, but I would be glad to drop it in favor of this upstream. |
I've done the same on my own account, and I've just published that fork on crates, as cargo isn't happy about having a git-based dependency (not even a dev-dependency). |
Hi Stephen @steveatinfincia ! Do you have some plans on how to progress forward with stabilizing the 0.6 release with or without this branch? You have not reacted to this PR for a month now and I am considering to replace dependency to the |
This PR applies the fix @maciejhirsz mentioned in #17 (giving co-author credit). It also includes @wigy-opensource-developer's test and reverts back to using the rust only crypto crates.
Fixes #17