search

RavenProject / ravenwallet-android

MIT License
18 stars 28 forks source link

All mnemonics #89

Open ben-abraham opened 3 years ago

ben-abraham commented 3 years ago

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

Status: Ready for Review

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

ben-abraham commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

Thoughts on any of these options?

TronBlack commented 3 years ago

My opinion... Detect the language from the operating system. If non-US, then default to using Spanish, Chinese, etc. Default to English if the mnemonic table doesn't match the language used by the OS.

On Fri, Apr 9, 2021 at 10:30 PM Ben Abraham @.***> wrote:

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?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/RavenProject/ravenwallet-android/pull/89#issuecomment-817076032, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACGYTYRCFGGHEA5UVJFEYX3TH7H7BANCNFSM42UGFELA .

ben-abraham commented 3 years ago

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.

TronBlack commented 3 years ago

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 commented 3 years ago

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.