-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Bugfix: duplicated word in seedphrase #1337
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
built release mode to both OS's, all looks good, QA Passed 👍
|
||
findNextAvailableIndex = () => { | ||
const { confirmedWords } = this.state; | ||
for (let i = 0; i < 12; i++) if (!confirmedWords[i].word) return i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of
for (let i = 0; i < 12; i++) if (!confirmedWords[i].word) return i;
return 12;
could do
return confirmedWords.findIndex(obj => !word)
and then use this.findNextAvailableIndex() === -1
on line 218
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
createWordsDictionary = () => { | ||
const dict = {}; | ||
this.words.forEach((word, i) => { | ||
dict[[word, i]] = { currentPosition: undefined }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not exactly clear to anyone ready the code that the keys of dict
will be strings of the form 'word,i'
The code could be a bit clearer if the keys were explicitly set with strings.
For example:
dict[`${word},${i}`] = { currentPosition: undefined };
It would then be clearer what is happening when the key is split on line 280
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it if it's not clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed code. It all looks good and that it works fine. Made a couple comments on code improvements that can be made, but they don't need to block merging.
@ibrahimtaveras00 can you do other QA pass? I've changed the code, just to be sure. I'm pushing the hardcoded seed phrase for you to test it. |
This reverts commit 54acd16.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latest updates look good, QA passed 👍
* progress * cleaner code * fix check * works * almost there * fix some bugs * cleaner * select empty box * done * undo custom changes * Revert "undo custom changes" This reverts commit 774113e. * Revert "Revert "undo custom changes"" This reverts commit 468ae85. * fix bug * snaps * Revert "undo custom changes" This reverts commit 774113e. * Revert "Revert "undo custom changes"" This reverts commit 54acd16. * review comments * seedPhraseReady * Revert "Revert "undo custom changes"" This reverts commit 54acd16.
Description
Fix for issue with duplicated words on seedphrase
Checklist
Issue
Fixes #1233
Fixes #1020