Page MenuHomePhabricator

[Cashtab] [BCH Deprecation] [Mnemonic] Pt 7/7 - Unwrap validateMnemonicWordList
ClosedPublic

Authored by emack on Nov 18 2022, 10:44.

Details

Summary

T2730

Depends on D12526

Stacked diff to localize existing uses of BCH.Mnemonic throughout the app and deprecate the BCH.Mnemonic object.

This diff unwraps the validateMnemonicWordList function which brings the mnemonicToEntropy function in bip39 into Cashtab and ensures it returns true/false as the validation outcome rather than the custom strings that require further parsing.

Pt 1/7 - Implement BCH.Mnemonic.toSeed locally
Pt 2/7 - Implement BCH.Mnemonic.generate locally
Pt 3/7 - Implement BCH.Mnemonic.wordLists locally
Pt 4/7 - Implement BCH.Mnemonic.validate locally
Pt 5/7 - Deprecate BCH.Mnemonic
Pt 6/7 - Optimize bip39 bundle via webpack IgnorePlugin
Pt 7/7 - Unwrap validateMnemonicWordList

Test Plan
  • npm test
  • npm start
  • create a new wallet, note the mnemonic, delete wallet
  • import the wallet via mnemonic
  • ensure successful send and receive of XEC to this imported wallet

Diff Detail

Event Timeline

emack requested review of this revision.Nov 18 2022, 10:44
bytesofman added inline comments.
web/cashtab/src/utils/validation.js
12 ↗(On Diff #36417)

I miscommunicated about what I was looking for here.

I think it's okay to leave the actual logic within the bip39 module.

I just think it's kind of weird that the function returns Blank mnemonic, Invalid mnemonic, or Valid mnemonic

Should just return true or false

This revision now requires changes to proceed.Nov 21 2022, 08:03
emack marked an inline comment as done.

Removed the validateMnemonicWordList wrapper in validation.js and now useWallet.validateMnemonic() directly calls on bip39 and returns a boolean outcome validating the given mnemonic.

bytesofman added inline comments.
web/cashtab/src/utils/__tests__/validation.test.js
54 ↗(On Diff #36429)

Since we have a bit of preprocessing, let's include a small selection of unit tests

  1. a valid mnemonic returning true
  2. an invalid mnemonic returning false
  3. an empty string returning false
This revision now requires changes to proceed.Nov 21 2022, 14:57
emack marked an inline comment as done.

added uni tests

Sorry I didn't notice this sooner. Now that this function is just a normal function without any dependence on the BCH param, it should be moved out ot useWallet.js

Please move it to validation.js , and unit tests to the correct place.

You can do it in this stack or create a new task + new diffs, whichever you prefer.

This revision is now accepted and ready to land.Nov 22 2022, 08:07
  • moved validateMnemonic from useWallet to validation
  • updated references to the new location in Configure and Onboarding components
  • added unit tests to validation.test.js

I understand we just wanted to get this done, but adding this as an amendment to this diff instead of another part of the stack has complicated things, this is more than just a 'nit' correction to an already green diff.

Moving the function to validation should be a separate diff. It can be on this stack if you like, or you can make a separate task and then do its own diff.

This revision now requires changes to proceed.Nov 22 2022, 11:13

Reverted migration of validateMnemonic back into useWallet. Will create a separate standalone diff to land this.

This revision is now accepted and ready to land.Nov 22 2022, 14:37