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

Implement ElectrumWords::get_invalid_word #9466

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

b4n6-b4n6
Copy link

@b4n6-b4n6 b4n6-b4n6 commented Aug 31, 2024

Fixes monero-project/monero-gui#4342

Please review and feedback

@b4n6-b4n6
Copy link
Author

b4n6-b4n6 commented Aug 31, 2024

New error message with specific invalid word is displayed

Screenshot from 2024-08-31 11-25-57
Screenshot from 2024-08-31 11-25-51

@b4n6-b4n6
Copy link
Author

Old error message is displayed on checksum failure

Screenshot from 2024-08-31 11-25-30
Screenshot from 2024-08-31 11-25-24

@@ -371,6 +371,59 @@ namespace crypto
return true;
}

std::string get_invalid_word(const epee::wipeable_string &words)
{
// If there's a new language added, add an instance of it here.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth the additional diff to find where else language lists are defined and leave a comment directing devs here for language-related work

Copy link
Contributor

@sneurlax sneurlax left a comment

Choose a reason for hiding this comment

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

These changes look good to me, I'd just leave an additional comment wherever else Languages are defined directing devs working on languages here

or some other solution which uses a general list of the Languages

@sneurlax
Copy link
Contributor

sneurlax commented Sep 5, 2024

... but ... I actually have to recheck if this is really needed. I forgot that only the first 3 letters really matter...

This might still be helpful

@b4n6-b4n6
Copy link
Author

b4n6-b4n6 commented Sep 6, 2024

... but ... I actually have to recheck if this is really needed. I forgot that only the first 3 letters really matter...

This might still be helpful

  1. I think this is needed as in practice it is tedious to identify misspelt word when writing words off paper wallet
  2. 24 word seeds are not used in practice anymore, right? if they are used in practice then a misspelling (in the first 3-4 letters) can derive wrong word and result in an new empty wallet. 25 word seeds will fail to restore as checksum won't match

@b4n6-b4n6
Copy link
Author

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GUI][Restore wallet] Misspelt seed word is not highlighted
3 participants