Mnemonic validation flexibility within Valora #6372

tarikbellamine · 2021-01-05T05:10:09Z

Description

This PR aims to do two things:

Allow the mnemonic to be entered in any language recognized by Valora (i.e., Valora does not need to be set to the language the mnemonic is)
Validate the mnemonic regardless of accented letters

Other changes

No

Tested

Yes

Related issues

Backwards compatibility

Yes

jeanregisser · 2021-01-05T10:33:28Z

Allow the mnemonic to be entered in any language recognized by Valora (i.e., Valora does not need to be set to the language the mnemonic is)

Looks like #5825 already fixed this.

tarikbellamine · 2021-01-05T20:55:01Z

Allow the mnemonic to be entered in any language recognized by Valora (i.e., Valora does not need to be set to the language the mnemonic is)

Looks like #5825 already fixed this.

Not in all cases. This variable definition: const languages = defaultLanguage ? [defaultLanguage] : getAllLanguages().filter((lang) => lang !== defaultLanguage) has an edge case where it will break if the param defaultLanguage was specified but not the same as the mnemonic language.

gastonponti

The bip39 from the saga is the only required thing to change.
The other are suggestions, but I strongly recommend to keep the ability of choosing the dictionary

packages/mobile/src/import/saga.ts

packages/sdk/utils/src/account.ts

gastonponti · 2021-01-05T21:43:46Z

packages/sdk/utils/src/account.ts

@@ -54,21 +55,20 @@ export async function generateMnemonic(
  return bip39ToUse.generateMnemonic(strength, undefined, getWordList(language))
 }

-export function validateMnemonic(
-  mnemonic: string,
-  defaultLanguage?: MnemonicLanguages,


I'm not entire sure of removing these. In a way, if you know the language, you will avoid to search in every dictionary.
I'm more into the idea of searching in the dictionary that the user said, or in all of them otherwise (nil/null).

Thinking more of a common utils side, and not our use case in Valora

I thought about my approach this way:

Good - eliminate the possibility of user error (i.e., if there is a mismatch between defaultLanguage and mnemonic language)

Bad - slightly less efficient as it defaults to checking all languages. However, there are not very many languages and bip39.validateMnemonic appears very optimized and will fail quickly on a language mismatch

What do you think?

Ok, I agree. We could eventually have another function that requires an specific language if we see that we need to optimise that (which are not the actual cases due to we are only using these to create/update accounts, and won't be called too often)

jeanregisser · 2021-01-06T13:12:30Z

Allow the mnemonic to be entered in any language recognized by Valora (i.e., Valora does not need to be set to the language the mnemonic is)

Looks like #5825 already fixed this.

Not in all cases. This variable definition: const languages = defaultLanguage ? [defaultLanguage] : getAllLanguages().filter((lang) => lang !== defaultLanguage) has an edge case where it will break if the param defaultLanguage was specified but not the same as the mnemonic language.

We were not passing a default language anymore with Gonza's fix:

celo-monorepo/packages/mobile/src/import/saga.ts

Line 32 in 51f643d

if (!validateMnemonic(phrase, undefined, bip39)) {

Good that we're making this even more robust 👍

gastonponti

LGTM 👍

…ignore accents

gastonponti

Great! I really like the mnemonic conversion approach.
I'm approving it, but remember to add the lowerCase

packages/sdk/utils/src/account.ts

Co-authored-by: Gaston Ponti <pontigaston@gmail.com>

Lss-Ankit · 2021-01-13T09:15:04Z

Hi @tarikbellamine I have verified this on latest Android Internal Build V1.8.0(1004294330) & Test Flight Build V1.8.0(40) & observed following:

User able to login when user paste English language Account key by selecting "Espanol(America Latina)" language while log in.
User able to login with English language account key by selecting "Portugues(Brasil)" language while log in.
Also I have verified by entering Account key in Espanol(America Latin) language and got error "Clave de cuenta Invalida" after tapping on Restore button.
Attachment: Espanol(AmericaLatin)LanguageError.mp4
Also it is observed that by entering Account key in Portueguese(Brasil) language and user got error "Chave da cuenta Invalida" after tapping on Restore button.
Attachment: Portuegues(Brasil)Language.mp4
Also it is observed that "Invalid Account Key" error message displayed when user enter account key in "Espanol(America Latin)" or "Portueguese(Brasil)" language.

Let me know if i need to test anything else Thanks!

tarikbellamine · 2021-01-13T09:19:47Z

@Lss-Ankit could you please test one more thing: enter in the Spanish with the accents and without the accents. For example, if the Spanish account key has the a word like niño then it should also be able to be entered like nino (without the special ñ)

Lss-Ankit · 2021-01-13T11:32:21Z

Hi @tarikbellamine I have verified following scenarioes:

By entering Spanish account key with accents user able to login in application

Attachment: WithAccents.mp4

Also it is observed that by entering Spanish account key without accents user able to login in application.

Attachment: WithoutAccents.mp4

It is observed that account key shown in English language even user login in app by selecting Portuguese(Brasil) langauge & user not able to set Account key in Portuguese language. Is it expected?

Attachment: AccountKeyEnglishEvenLangaugePortueguese.jpg
Verified on devices: iPhone SE 2 (14.3), iPhone iphone 6+ (12.4.5), Samsung Galaxy A5(7.0)
Let me know anything else need to check.

tarikbellamine · 2021-01-13T20:23:24Z

@jeanregisser is this expected or was adding the Portuguese support supposed to include the Account Key as well?

tarikbellamine requested review from barbaraliau, cmcewen, gastonponti, i1skn, jeanregisser, jmrossy, mcortesi, medhakothari and a team as code owners January 5, 2021 05:10

tarikbellamine requested review from gnardini and removed request for a team, jeanregisser, mcortesi, jmrossy, cmcewen, barbaraliau, medhakothari and i1skn January 5, 2021 05:10

jeanregisser added the wallet label Jan 5, 2021

gastonponti reviewed Jan 5, 2021

View reviewed changes

gastonponti approved these changes Jan 6, 2021

View reviewed changes

tarikbellamine added automerge Have PR merge automatically when checks pass and removed automerge Have PR merge automatically when checks pass labels Jan 6, 2021

tarikbellamine added 4 commits January 6, 2021 14:24

changing mnemonic logic to allow for any language recognition and to …

cc7a638

…ignore accents

simplify validation logic

0fc03a0

normalizing mnemonics and word lists to ignore accents

737361a

add test

1303448

tarikbellamine added 3 commits January 6, 2021 14:24

remove unneeded test

ad6c727

pr feedback

4ea608e

fixing incorrectly accented words rather than stripping accents

892ef34

tarikbellamine force-pushed the tarikbellamine/mnemonic-accents branch from bd7d664 to 892ef34 Compare January 6, 2021 22:27

tarikbellamine added 3 commits January 6, 2021 15:44

add new tests

2ea4e91

optimized functions by using a map instead of array

ef4ef48

remove console logs

126ac5a

gastonponti approved these changes Jan 7, 2021

View reviewed changes

packages/sdk/utils/src/account.ts Outdated Show resolved Hide resolved

Lowercase mnemonic to noramlize irregular capitalizations

0743338

Co-authored-by: Gaston Ponti <pontigaston@gmail.com>

tarikbellamine added the automerge Have PR merge automatically when checks pass label Jan 7, 2021

mergify bot added 3 commits January 7, 2021 18:13

Merge branch 'master' into tarikbellamine/mnemonic-accents

41f7d63

Merge branch 'master' into tarikbellamine/mnemonic-accents

af8aa82

Merge branch 'master' into tarikbellamine/mnemonic-accents

027023c

mergify bot merged commit 4276a18 into master Jan 7, 2021

mergify bot deleted the tarikbellamine/mnemonic-accents branch January 7, 2021 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mnemonic validation flexibility within Valora #6372

Mnemonic validation flexibility within Valora #6372

tarikbellamine commented Jan 5, 2021 •

edited

Loading

jeanregisser commented Jan 5, 2021

tarikbellamine commented Jan 5, 2021 •

edited

Loading

gastonponti left a comment

gastonponti Jan 5, 2021

tarikbellamine Jan 5, 2021

gastonponti Jan 6, 2021

jeanregisser commented Jan 6, 2021 •

edited

Loading

gastonponti left a comment

gastonponti left a comment

Lss-Ankit commented Jan 13, 2021

tarikbellamine commented Jan 13, 2021

Lss-Ankit commented Jan 13, 2021

tarikbellamine commented Jan 13, 2021

Mnemonic validation flexibility within Valora #6372

Mnemonic validation flexibility within Valora #6372

Conversation

tarikbellamine commented Jan 5, 2021 • edited Loading

Description

Other changes

Tested

Related issues

Backwards compatibility

jeanregisser commented Jan 5, 2021

tarikbellamine commented Jan 5, 2021 • edited Loading

gastonponti left a comment

Choose a reason for hiding this comment

gastonponti Jan 5, 2021

Choose a reason for hiding this comment

tarikbellamine Jan 5, 2021

Choose a reason for hiding this comment

gastonponti Jan 6, 2021

Choose a reason for hiding this comment

jeanregisser commented Jan 6, 2021 • edited Loading

gastonponti left a comment

Choose a reason for hiding this comment

gastonponti left a comment

Choose a reason for hiding this comment

Lss-Ankit commented Jan 13, 2021

tarikbellamine commented Jan 13, 2021

Lss-Ankit commented Jan 13, 2021

tarikbellamine commented Jan 13, 2021

tarikbellamine commented Jan 5, 2021 •

edited

Loading

tarikbellamine commented Jan 5, 2021 •

edited

Loading

jeanregisser commented Jan 6, 2021 •

edited

Loading