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

Standard vectors fix #18

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

QuestofIranon
Copy link
Contributor

@QuestofIranon QuestofIranon commented Feb 1, 2019

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

@wigy-opensource-developer

So a Japanese and English mnemonic for the same entropy results in a different seed based on BIP39. That I overlooked as well. Thanks.

@maciejhirsz
Copy link
Contributor

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).

@QuestofIranon
Copy link
Contributor Author

QuestofIranon commented Feb 2, 2019

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.

@wigy-opensource-developer

@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?

@QuestofIranon
Copy link
Contributor Author

@steveatinfincia Any chance to review this yet?

@wigy-opensource-developer

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.

@maciejhirsz
Copy link
Contributor

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).

@wigy-opensource-developer

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 tiny-bip39 create from Maciej's fork soon if you stay unresponsive. Still thanks a lot for this tool, it was a great help to deal with the complexity this BIP-0039 hides.

wigy-opensource-developer pushed a commit to wigy-opensource-developer/bip39-rs that referenced this pull request Jun 17, 2021
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