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

fix: seed recovery validation and unlock fix #467

Merged
merged 6 commits into from
Nov 28, 2019
Merged

Conversation

hanwencheng
Copy link
Contributor

@hanwencheng hanwencheng commented Nov 22, 2019

closes #461 .

tested with brain wallet seed with extra space:

  1. ' ' (two spaces) Checked signing and account recovery.
  2. 'a ' (trailing spaces)
  3. ' h' (heading spaces)
  4. 'a h' (two spaces in middle)

@hanwencheng hanwencheng changed the title Hanwen seed fix fix: seed recovery validation and unlock fix Nov 22, 2019
sjeohp-zz
sjeohp-zz previously approved these changes Nov 27, 2019
@hanwencheng hanwencheng mentioned this pull request Nov 27, 2019
Tbaut
Tbaut previously approved these changes Nov 28, 2019
Copy link
Contributor

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working well (tested with Ethereum e2e)
Not sure what to do with that debounce honestly. If you can't manage to make it work I'd remove it all together, all it does now is delay the response by 200ms.

setIsSeedValid(validateSeed(inputSeedPhrase, bip39));
})
.catch(() => setIsSeedValid(inputSeedPhrase, false));
const debouncedAddressGeneration = debounce(addressGeneration, 200);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This debounce isn't working. I remember struggling a lot with it in the past (in screens/AccountRecover.js).
To verify what I said, add a console.log in addressGeneration, you'll see that it's printed at every key stroke. I couldn't manage to make it work in a reasonable time in this component though :(.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, let me have a try.

Copy link
Contributor Author

@hanwencheng hanwencheng Nov 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, in debounce.js we create new instance of timeout at each call of debounce, so that it is not cleared.

@hanwencheng hanwencheng dismissed stale reviews from Tbaut and sjeohp-zz via 583f8cf November 28, 2019 11:21
Copy link
Contributor

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! didn't test again but that's a nit :)

@hanwencheng hanwencheng merged commit bfd67b3 into master Nov 28, 2019
@hanwencheng hanwencheng deleted the hanwen-seed-fix branch November 28, 2019 15:56
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.

No feedback when entering the recovery phrase
3 participants