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 Portuguese wordlist #19

Merged
merged 8 commits into from
Oct 17, 2023
Merged

Conversation

mikeobank
Copy link
Contributor

Wordlist fetched from https://raw.githubusercontent.com/bitcoin/bips/master/bip-0039/portuguese.txt. And manually wrapped in Typescript const and .split().

  • Do we need more tests?
  • Could imagine some form of automation including integrity checks for the wordlists txt files would be nice to have?

@paulmillr
Copy link
Owner

Could imagine some form of automation including integrity checks for the wordlists txt files would be nice to have?

yes, a script that downloads bip39 wordlists from the urls and verifies them would be great

@mikeobank
Copy link
Contributor Author

I've added a little script to fetch a specific language's wordlist and wrap this into a .ts file.

Also noticed that there currently is a naming mismatch with https://github.com/bitcoin/bips/tree/master/bip-0039 for the Chinese wordlists. chinese_simplified.txt => simplified-chinese.ts and chinese_traditional.txt => traditional-chinese.ts. Think it would be best to clean this up, although this would result in a breaking change.

@paulmillr
Copy link
Owner

Thanks, that looks great. I prefer dashes to underscores. We can just replace underscores with dashes in the download script.

@mikeobank
Copy link
Contributor Author

Ok, can live with that. What about the order of language name and variety? Swap those around too?

@paulmillr
Copy link
Owner

On one hand, I won't mind adding aliases if this makes people life comfy. On the other hand, we say "simplified chinese" and not "chinese simplified" in life. So, up to you. In any case we won't have underscore names so there won't be 1:1 mapping even if we add aliases.

@mikeobank
Copy link
Contributor Author

Yes, I agree, am also in favor of readability. The script now swaps the language and variety around. Adding aliases seems overkill to me. Please review and let me know if any more changes are necessary.

@paulmillr paulmillr merged commit f35ddb0 into paulmillr:main Oct 17, 2023
1 check passed
@paulmillr
Copy link
Owner

Thanks! Looking great. The next step would be to check validity in CI. Maybe a check on every commit is overkill and limiting it to releases is ok. On the other hand the library is feature complete and rarely updated so we may as well keep it to commits. I will work on it whenever i'll have time - if anyone wants to help its also cool

@mikeobank
Copy link
Contributor Author

Validity checks during build / CI would be nice, yes

Some other things I was thinking of:

  • Check if word list arrays are sorted alphabetically (not sure if this is the case for Chinese, Japanese and Korean though)
  • Also specify validations in Typescript's type system
  • Move the parsing of multi line string => word list array to the build step. This would relieve the .split('\n') operation happening at runtime / import at the moment. Maybe turn the arrays into JSON data if browsers can handle that.

Will have a deeper look into it when I have time. Thanks for merging and closing this PR.

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.

None yet

2 participants