Version 3 #74
Version 3 #74
Conversation
| @@ -61,12 +61,13 @@ function mnemonicToSeedHex (mnemonic, password) { | |||
| function mnemonicToEntropy (mnemonic, wordlist) { | |||
| wordlist = wordlist || DEFAULT_WORDLIST | |||
|
|
|||
| var words = unorm.nfkd(mnemonic).split(' ') | |||
| var words = mnemonic.split(wordlist.separator) | |||
There was a problem hiding this comment.
@fanatid was unorm being used here to normalize the separator? I don't think we need it [here] now?
There was a problem hiding this comment.
Yea, I think we do not need unorm now
There was a problem hiding this comment.
@dcousens This is not good. There is a possibility the user will input using normal spaces. (depending on which device/keyboard/IME they use)
normalizing beforehand fixed this, since both ASCII space and \u3000 space would both turn into ASCII space.
Your new method will fail if a Japanese user uses ASCII spaces for whatever reason.
Also, Japanese fails on new line 65 var index = wordlist.words.indexOf(word) because the wordlist is normalized strings but normal Japanese input is different String-wise (and Buffer-wise).
The normalize needs to stay, unfortunately... :-/
There was a problem hiding this comment.
If the normalized separator = ASCII space assumption is ever broken for a new wordlist we'd need to revisit this... maybe it needs a comment.
Also, the var index = wordlist.words.indexOf(word) actually is a "every language except English" problem, and not Japanese specifically.
There was a problem hiding this comment.
@dabura667 I will restore the use of unorm here.
There was a problem hiding this comment.
sounds like there needs to be a test for at least a japanese seed phrase
dcousens
requested review from
dabura667 and
fanatid
and removed request for
dabura667
|
utACK |
| @@ -61,12 +61,13 @@ function mnemonicToSeedHex (mnemonic, password) { | |||
| function mnemonicToEntropy (mnemonic, wordlist) { | |||
| wordlist = wordlist || DEFAULT_WORDLIST | |||
|
|
|||
| var words = unorm.nfkd(mnemonic).split(' ') | |||
| var words = mnemonic.split(wordlist.separator) | |||
There was a problem hiding this comment.
@dcousens This is not good. There is a possibility the user will input using normal spaces. (depending on which device/keyboard/IME they use)
normalizing beforehand fixed this, since both ASCII space and \u3000 space would both turn into ASCII space.
Your new method will fail if a Japanese user uses ASCII spaces for whatever reason.
Also, Japanese fails on new line 65 var index = wordlist.words.indexOf(word) because the wordlist is normalized strings but normal Japanese input is different String-wise (and Buffer-wise).
The normalize needs to stay, unfortunately... :-/
|
there are Japanese test cases linked in the BIP39 bip that I made. |
|
@dcousens just wondering is anything missing for this PR to be released? Would be happy to help if so! Also as my primary concern is bundle size would you consider a smaller PR that adds ESM bundling and/or exports all functions in separate files to allow cherry picking imports? Thanks! |
|
any progress? |
|
Async is supported as a new method (backwards compatible) here: #100 |
|
Closing this in favor of #104
Separators are really not too necessary IMO... and as for removing HEX methods... yeah that makes sense. I'll add these to the PR scope. |
mnemonicToSeedHex, remove implicit hex support, Buffer onlypbkdf2, aka, async