Page MenuHomePhabricator
Differential D12530

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

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

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC66dff6ebbcf3: [Cashtab] [BCH Deprecation] [Mnemonic] Pt 7/7 - Unwrap validateMnemonicWordList
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

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emack created this revision.Nov 18 2022, 10:44
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 18 2022, 10:44
emack requested review of this revision.Nov 18 2022, 10:44
Harbormaster completed remote builds in B20978: Diff 36388.Nov 18 2022, 10:46
emack updated this revision to Diff 36417.Nov 20 2022, 00:23

rebase

Harbormaster completed remote builds in B21000: Diff 36417.Nov 20 2022, 00:26
bytesofman requested changes to this revision.Nov 21 2022, 08:03
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 updated this revision to Diff 36429.Nov 21 2022, 12:18
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.

Harbormaster completed remote builds in B21011: Diff 36429.Nov 21 2022, 12:21
bytesofman requested changes to this revision.Nov 21 2022, 14:57
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 updated this revision to Diff 36471.Nov 22 2022, 07:58
emack marked an inline comment as done.

added uni tests

Harbormaster completed remote builds in B21034: Diff 36471.Nov 22 2022, 08:01
emack updated this revision to Diff 36472.Nov 22 2022, 08:04

Updated unit tests

Harbormaster completed remote builds in B21035: Diff 36472.Nov 22 2022, 08:07
bytesofman accepted this revision.Nov 22 2022, 08:07

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
emack updated this revision to Diff 36482.Nov 22 2022, 10:38
  • moved validateMnemonic from useWallet to validation
  • updated references to the new location in Configure and Onboarding components
  • added unit tests to validation.test.js
Harbormaster completed remote builds in B21045: Diff 36482.Nov 22 2022, 10:41
bytesofman requested changes to this revision.Nov 22 2022, 11:13

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
emack updated this revision to Diff 36484.Nov 22 2022, 12:09

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

Harbormaster completed remote builds in B21046: Diff 36484.Nov 22 2022, 12:12
bytesofman accepted this revision.Nov 22 2022, 14:37
This revision is now accepted and ready to land.Nov 22 2022, 14:37
Closed by commit rABC66dff6ebbcf3: [Cashtab] [BCH Deprecation] [Mnemonic] Pt 7/7 - Unwrap validateMnemonicWordList (authored by emack). · Explain WhyNov 22 2022, 21:49
This revision was automatically updated to reflect the committed changes.
emack added a commit: rABC66dff6ebbcf3: [Cashtab] [BCH Deprecation] [Mnemonic] Pt 7/7 - Unwrap validateMnemonicWordList.

Revision Contents
Changeset List

PathSize
web/
cashtab/
src/
hooks/
35 lines
utils/
__tests__/
21 lines
15 lines

Diff 36529

View Options

web/cashtab/src/hooks/useWallet.js

Loading...
View Options

web/cashtab/src/utils/__tests__/validation.test.js

Loading...
View Options

web/cashtab/src/utils/validation.js

Loading...