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

All mnemonics #89

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

Conversation

ben-abraham
Copy link
Contributor

@ben-abraham ben-abraham commented Apr 9, 2021

PR to close #83 [Please add other mnemonic languages]

Status: Ready for Review

  • Add missing language wordlists files
  • Add code to support all languages going forward
  • Add/update test cases to support new features and verify existing functionality
  • Ensure compatibility with existing english paper keys
  • New test cases related to "translation" of phrases NOTE below
  • Use system locale to determine which phrase language to use at the time of generation
  • Testnet/Live wallet testing

I've added a new class Bip39Wordlist for grouping all the functionality of a specific wordlist in one place. Most of the wordlist-related JNI functions are called from this class, making it much simpler to write wordlist-aware code.

At the moment, all phrases are stored in English in the BRKeyStore class, this change does not modify that underlying storage at all. This PR makes the assumption that we can leave english phrases as-is in the user preferences, and simply "translate" (not actually) the phrase to that of another language. If this assumption is faulty, please let me know :)

The steps for "translating" the phrase are as follows:

  1. Extract the users always-english phrase from BRKeyStore.
  2. Derive the seed from their phrase based on Bip39Wordlist.DEFAULT_WORDLIST.
  3. Fetch the Bip39Wordlist for the target language.
  4. Use the seed from step 2. to create the phrase in the target language.
  5. Present generated phrase instead of english phrase.

Because this is simply a "translation" for the purposes of the paper key, and no underlying storage is being changed, the only UI place this was implemented was in PaperKeyActivity

Note on tests:
PaperKeyTests has been updated to test validating a phrase in each wordlist available. However I am having issues getting (some?) JNI functions working properly when ran in unit testing mode. Calling any JNI method throws UnsatisfiedLinkError during testing, which appears to be related to what android makes available during non-emulator testing.
Looking for suggestions on the best way to handle this.

Screenshot:
image

Added 7 preset IPFS gateways. 2x Cloudflare, 2x ipfs.io, dweb, GreyH.at, and Infura
Added 'ipfsGateway' setting to BRSharedPrefs.
Added helper function in Utils class for creating an ipfs url from the hash.
Added BRConstants.IPFS_DEFAULT_HOST with default value "ipfs.io"
Moved ipfs.io gateways to top of settings list since they are the default option.
- Added class for managing BIP39 wordlists, with helper functions for various use-cases.
- Removed now-unused class Bip39Reader
- Replaced all usages of Bip39Reader with Bip39Wordlist
Added tests around the new wordlist class, as well as expanding to include all language now.
…ersing the seed value from a given phrase.
…to writing down of your paper key.

Generate displayed paper key in selected language, in the background the phrase is always stored as English.
Renamed `BRCoreKey.getSeedFromPhrase` to `BRCoreKey.getDerivedPhraseKey` to match underlying JNI names.
@ben-abraham
Copy link
Contributor Author

My main issue with this currently, and where I could really use a second opinion, is that if I take a phrase, say:
sketch visual stadium member quarter unknown gain sword over guilt double rough
and enter that into the converter
I get a corresponding seed of
a44de6afdfbfb61f0434531bb36b4ca09c4863a9aaa6f45156935566ed28766a22627a6626994561c9c50455239e4e369cdd13e23950886762486fcfdac2f06e
and root key
xprv9s21ZrQH143K4MyWGgMmrR6bVa1d4mnpXGdcmjr5ByinxYMqX8GBYTm2G6WUryKZTbk3o1s4csqP88yw26HCNqrYT5t7BCeQa8FQbzS87xy

If I "translate" the phrase to spanish, I get
rígido vale seco matiz peluca trébol fuente subir nutria grumo deseo promesa
of which each individual word's index/value matches that of english (ie: sketch is index 1618 in en wordfile, and rígido is 1618 in es)
however the seed and root key come out differently
5895613138d5ea0281e44428acafced0473c733fb88478e2c00c92930a0fe7d88bb31c9f3f7dd6fc77819fbb0028c6d9bd3e9ae4962ee4b9d70a311c1fd3130e
xprv9s21ZrQH143K3oHkPJpuZqVDqgVLiMMJp77dXwjDPPvaLK38CyH3r3r6JBWAdcacQc9rqnBj8FTDuS2yQa1rgBGjbtZrEUXcD6D7nZPKd7t

This would (presumably) mean that a "translated" phrase would not actually restore back to the same wallet/private key, only one that is "translated" back into english before being used.
Am I missing something underlying about the BIP-39 mechanism? I thought it only operated on the seed (or at least called seed in code), that is to say, the 128-bit value the paper-key phrase represents.
I would like to add some tests around this, so I'll see what I can get added.

@ben-abraham
Copy link
Contributor Author

And as I was typing that last comment I more-or-less came across the source of my confusion.
The final key that is used, is hashed based on the actual utf-8 encoding of the phrase itself, along with an optional password (not currently used)
This means that the seed phrase needs to be actively used in it's generating language, not just "translated" on-the-fly for the purposes of paper key backup.

I think the best fix here would just be prompting for the key phrase language sooner, prior to the key actually being generated, and then just storing the generated phrase in it's "native" language.

All the remaining points in the code should simply work with a non-english phrase and key (binary anyway) stored in the settings.

@ben-abraham
Copy link
Contributor Author

Actually, before I go about making any more changes, I would like to seek a second opinion on the best place to add the new prompt.

At the moment, the flow of a new wallet looks like this. Activities are listed with the step performed to advance to the next activity.

  1. Intro (Create)
  2. About (Skip)
  3. T&C (Accept)
  4. Pin (Type)
  5. PinConfirm (Type)
  6. Wallet seed created here
  7. WriteDown (Advance) <--- Where language prompt currently is
  8. PaperKey (Next x12)
  9. PaperKeyProve (Write x2)
  10. Home (Hodl)

A few possible ideas come to mind on how we can insert the prompt into the current flow.

  • Between steps 5/6 (or 3/4), this is the most direct, but feels a little awkward of a flow for users.
  • On activity 3 itself, also a weird place for that prompt
  • Move wallet creation after step 7, would solve ordering, but introduce many potential edge-cases with existing (but not marked as backed up yet) wallets

Thoughts on any of these options?

@TronBlack
Copy link
Contributor

TronBlack commented Apr 10, 2021 via email

Phrase language selection is entirely based on system language at the time of phrase creation.
After the phrase has been created, it is always interpreted as text going forward, so all existing references should continue to work with no issues.
Added helper function for validating whole phrase.
Added helper function for getting current locale in a version-aware manner.
@ben-abraham
Copy link
Contributor Author

Alright! I pulled out the language selection components and just let the locale detection choose which wordlist when creating a new wallet. I also added a version-aware helper function to support the changes to locale classes that didn't seem to be accounted for currently.

So creating a new wallet and then switching to spanish looks like this (app restarted after language switch):

raven_english_to_spanish

The backup phrase remains in english despite the system and application being displayed in the selected locale.
And similarly, the same thing in the other direction:

raven_spanish_to_english

Any "translate" code no longer exists.

…ordering of selected locales.

Also fixed chinese languages not being selected properly.
@TronBlack
Copy link
Contributor

Detecting the language from the OS is the best option for writing down the 12-words.
For restore, it might be best to detect the word list from the words entered.

@ben-abraham
Copy link
Contributor Author

ben-abraham commented Apr 17, 2021

Detecting the language from the OS is the best option for writing down the 12-words.

Done

For restore, it might be best to detect the word list from the words entered.

When restoring, the phrase the user enters is stored in BRSharedPrefs as text, and that is what gets used for keys. This means that if you restore with a spanish phrase on an english locale wallet it will more-or-less 'just work' that the phrase will always be in spanish internally and the rest of the app essentially wont care.

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.

Please add other mnemonic languages.
2 participants