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

add korean word module to hd mnemonic #1158

Merged
merged 2 commits into from
Jul 14, 2023

Conversation

ChrisCho-H
Copy link
Contributor

Korea is one of the largest in terms of the number of users in crypto.
I refer to bip39.js where Korean support is implemented.
expect zero harm with useful utility.

@theanmolsharma
Copy link
Collaborator

theanmolsharma commented Jun 24, 2023

I'd like to see some tests before we get it merged.

@ChrisCho-H
Copy link
Contributor Author

image

I've tested and it works well!
You can try mnemonic-test to verify.

@theanmolsharma
Copy link
Collaborator

Tested ACK.
Before we merge, I'd like you to squash all commits into a single commit.

@ChrisCho-H
Copy link
Contributor Author

alright

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Patch coverage: 50.00% and no project coverage change.

Comparison is base (b005869) 69.55% compared to head (49a0cca) 69.56%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1158   +/-   ##
=======================================
  Coverage   69.55%   69.56%           
=======================================
  Files         158      159    +1     
  Lines       26603    26607    +4     
=======================================
+ Hits        18505    18509    +4     
  Misses       8098     8098           
Impacted Files Coverage Δ
lib/hd/mnemonic.js 82.14% <ø> (ø)
lib/hd/wordlist-browser.js 0.00% <0.00%> (ø)
lib/hd/words/index.js 0.00% <0.00%> (ø)
lib/hd/wordlist.js 90.90% <100.00%> (+10.90%) ⬆️
lib/hd/words/korean.js 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pinheadmz
Copy link
Member

Where is the word list from? I don't understand the language but it doesn't look like it matches

https://github.com/bitcoinjs/bip39/blob/master/src/wordlists/korean.json

or

https://github.com/bitcoin/bips/blob/master/bip-0039/korean.txt

Same question about the test vectors, where are those from?

@ChrisCho-H
Copy link
Contributor Author

It's same with the former one(bitcoinjs/bip39).
Test vector is just generated from given seed, and the phrase is one in the wordlist.

@pinheadmz
Copy link
Member

@pinheadmz
Copy link
Member

ok please just fix the lint error (single quotes)

@ChrisCho-H
Copy link
Contributor Author

line error fixed

@pinheadmz pinheadmz merged commit bb0375e into bcoin-org:master Jul 14, 2023
5 of 6 checks passed
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